Re: [PATCH] Add _STRING_INLINE_unaligned

["H.J. Lu" <hjl dot tools at gmail dot com>]

On Thu, Jan 14, 2016 at 11:24 AM, H.J. Lu <hjl.tools@gmail.com> wrote:
> On Fri, Jan 8, 2016 at 9:37 AM, Wilco Dijkstra <Wilco.Dijkstra@arm.com> wrote:
>> As discussed in https://sourceware.org/ml/libc-alpha/2015-10/msg00403.html ,
>> the setting of _STRING_ARCH_unaligned currently controls the external
>> GLIBC ABI as well as selecting the use of unaligned accesses withing GLIBC.
>>
>> Since _STRING_ARCH_unaligned was recently changed for AArch64, this
>> would potentially break the ABI in GLIBC 2.23, so split the uses and add
>> _STRING_INLINE_unaligned to select the string ABI. This setting must be fixed for
>> each target, while _STRING_ARCH_unaligned may be changed from release to release.
>>
>> Built and tested on AArch64, x86 and x64. OK for GLIBC 2.23?
>>
>> ChangeLog:
>> 2016-01-08  Wilco Dijkstra  <wdijkstr@arm.com>
>>
>>         * bits/string.h (_STRING_INLINE_unaligned): New define.
>>         * string/string-inlines.c: Use _STRING_INLINE_unaligned.
>>         * string/bits/string2.h (_STRING_ARCH_unaligned): Remove
>>         unused conditional includes.
>>         * sysdeps/aarch64/bits/string.h (_STRING_INLINE_unaligned): Add define.
>>         * sysdeps/m68k/m680x0/m68020/bits/string.h
>>         (_STRING_INLINE_unaligned): Likewise.
>>         * sysdeps/sparc/bits/string.h (_STRING_INLINE_unaligned): Likewise.
>>         * sysdeps/s390/bits/string.h (_STRING_INLINE_unaligned): Likewise.
>>         * sysdeps/x86/bits/string.h (_STRING_INLINE_unaligned): Likewise.
>>
>>
>>
>
> This patch can't be applied since it has odd characters.
>
> Here is a patch to support -Os build.  Tested on x86-64 and
> i686 without any code changes.
>
> Can you combine your patch with mine to move
> _STRING_ARCH_unaligned to string_private.h and add
> STRING_INLINE_unaligned to bits/string.h?
>

Here is the combined patch.  _STRING_ARCH_unaligned is
removed from all installed header files:

[hjl@gnu-6 glibc]$ git grep _STRING_ARCH_unaligned | grep -v ChangeLog
bits/string.h:   after the first release of a new target (unlike
_STRING_ARCH_unaligned
crypt/md5.c:#if !_STRING_ARCH_unaligned
crypt/sha256.c:#if _STRING_ARCH_unaligned
crypt/sha256.c:#if !_STRING_ARCH_unaligned
crypt/sha512.c:#if !_STRING_ARCH_unaligned
iconv/gconv_simple.c:#if !_STRING_ARCH_unaligned
iconv/gconv_simple.c:#if !_STRING_ARCH_unaligned
iconv/gconv_simple.c:#if !_STRING_ARCH_unaligned
iconv/gconv_simple.c:#if !_STRING_ARCH_unaligned
iconv/loop.c:#if _STRING_ARCH_unaligned || !defined DEFINE_UNALIGNED
iconv/loop.c:#if !defined DEFINE_UNALIGNED && !_STRING_ARCH_unaligned \
iconv/skeleton.c:#if _STRING_ARCH_unaligned
iconv/skeleton.c:  (!_STRING_ARCH_unaligned              \
include/arpa/nameser.h:#if _STRING_ARCH_unaligned
include/string.h:/* Get _STRING_ARCH_unaligned.  */
nscd/nscd_gethst_r.c:#if !_STRING_ARCH_unaligned
nscd/nscd_getserv_r.c:#if !_STRING_ARCH_unaligned
nscd/nscd_helper.c:#if !_STRING_ARCH_unaligned
nscd/nscd_helper.c:#if !_STRING_ARCH_unaligned
nscd/nscd_helper.c:#if !_STRING_ARCH_unaligned
resolv/res_send.c:#if _STRING_ARCH_unaligned
resolv/res_send.c:#if _STRING_ARCH_unaligned
stdlib/getenv.c:#if __BYTE_ORDER == __LITTLE_ENDIAN || !_STRING_ARCH_unaligned
stdlib/getenv.c:#if _STRING_ARCH_unaligned
stdlib/getenv.c:#if _STRING_ARCH_unaligned
stdlib/getenv.c:#if _STRING_ARCH_unaligned
sysdeps/aarch64/string_private.h:/* Define _STRING_ARCH_unaligned.
AArch64 version.
sysdeps/aarch64/string_private.h:#define _STRING_ARCH_unaligned 1
sysdeps/generic/string_private.h:/* Define _STRING_ARCH_unaligned.
Generic version.
sysdeps/generic/string_private.h:#define _STRING_ARCH_unaligned   0
sysdeps/m68k/m680x0/m68020/string_private.h:/* Define
_STRING_ARCH_unaligned.  m680x0 version, x >= 2.
sysdeps/m68k/m680x0/m68020/string_private.h:#define _STRING_ARCH_unaligned   1
sysdeps/s390/string_private.h:/* Define _STRING_ARCH_unaligned.  S/390 version.
sysdeps/s390/string_private.h:#define _STRING_ARCH_unaligned   1
sysdeps/x86/string_private.h:/* Define _STRING_ARCH_unaligned.
i486/x86-64 version.
sysdeps/x86/string_private.h:#define _STRING_ARCH_unaligned   1
[hjl@gnu-6 glibc]$

Tested on x86-64, x32 and i686.  OK for trunk?


-- 
H.J.

From 0d32fcb814a7ea7e857a967e96e5c76205bd729b Mon Sep 17 00:00:00 2001
From: "H.J. Lu" <hjl.tools@gmail.com>
Date: Fri, 15 Jan 2016 08:11:23 -0800
Subject: [PATCH] Add _STRING_INLINE_unaligned and string_private.h

As discussed in

https://sourceware.org/ml/libc-alpha/2015-10/msg00403.html

the setting of _STRING_ARCH_unaligned currently controls the external
GLIBC ABI as well as selecting the use of unaligned accesses withing
GLIBC.

Since _STRING_ARCH_unaligned was recently changed for AArch64, this
would potentially break the ABI in GLIBC 2.23, so split the uses and add
_STRING_INLINE_unaligned to select the string ABI. This setting must be
fixed for each target, while _STRING_ARCH_unaligned may be changed from
release to release.  _STRING_ARCH_unaligned is used unconditionally in
glibc.  But <bits/string.h>, which defines _STRING_ARCH_unaligned, isn't
included with -Os.  Since _STRING_ARCH_unaligned is internal to glibc and
may change between glibc releases, it should be made private to glibc.
_STRING_ARCH_unaligned should defined in the new string_private.h heade
file which is included unconditionally from internal <string.h> for glibc
build.

2016-01-15  H.J. Lu  <hongjiu.lu@intel.com>
	    Wilco Dijkstra  <wdijkstr@arm.com>

	[BZ #19462]
	* bits/string.h (_STRING_ARCH_unaligned): Renamed to ...
	(_STRING_INLINE_unaligned): This.
	* include/string.h: Include <string_private.h>.
	* string/bits/string2.h: Replace _STRING_ARCH_unaligned with
	_STRING_INLINE_unaligned.
	* sysdeps/aarch64/bits/string.h (_STRING_ARCH_unaligned): Removed.
	(_STRING_INLINE_unaligned): New.
	* sysdeps/aarch64/string_private.h: New file.
	* sysdeps/generic/string_private.h: Likewise.
	* sysdeps/m68k/m680x0/m68020/string_private.h: Likewise.
	* sysdeps/s390/string_private.h: Likewise.
	* sysdeps/x86/string_private.h: Likewise.
	* sysdeps/m68k/m680x0/m68020/bits/string.h
	(_STRING_ARCH_unaligned): Renamed to ...
	(_STRING_INLINE_unaligned): This.
	* sysdeps/s390/bits/string.h (_STRING_ARCH_unaligned): Renamed
	to ...
	(_STRING_INLINE_unaligned): This.
	* sysdeps/sparc/bits/string.h (_STRING_ARCH_unaligned): Renamed
	to ...
	(_STRING_INLINE_unaligned): This.
	* sysdeps/x86/bits/string.h (_STRING_ARCH_unaligned): Renamed
	to ...
	(_STRING_INLINE_unaligned): This.
---
 bits/string.h                               |  8 ++++++--
 include/string.h                            |  3 +++
 string/bits/string2.h                       | 10 +++++-----
 sysdeps/aarch64/bits/string.h               |  4 ++--
 sysdeps/aarch64/string_private.h            | 20 ++++++++++++++++++++
 sysdeps/generic/string_private.h            | 21 +++++++++++++++++++++
 sysdeps/m68k/m680x0/m68020/bits/string.h    |  5 ++---
 sysdeps/m68k/m680x0/m68020/string_private.h | 21 +++++++++++++++++++++
 sysdeps/s390/bits/string.h                  |  4 ++--
 sysdeps/s390/string_private.h               | 20 ++++++++++++++++++++
 sysdeps/sparc/bits/string.h                 |  4 ++--
 sysdeps/x86/bits/string.h                   |  4 ++--
 sysdeps/x86/string_private.h                | 20 ++++++++++++++++++++
 13 files changed, 126 insertions(+), 18 deletions(-)
 create mode 100644 sysdeps/aarch64/string_private.h
 create mode 100644 sysdeps/generic/string_private.h
 create mode 100644 sysdeps/m68k/m680x0/m68020/string_private.h
 create mode 100644 sysdeps/s390/string_private.h
 create mode 100644 sysdeps/x86/string_private.h

diff --git a/bits/string.h b/bits/string.h
index b88a6bc..89c627c 100644
--- a/bits/string.h
+++ b/bits/string.h
@@ -8,7 +8,11 @@
 #ifndef _BITS_STRING_H
 #define _BITS_STRING_H	1
 
-/* Define if architecture can access unaligned multi-byte variables.  */
-#define _STRING_ARCH_unaligned   0
+/* Define whether to use the unaligned string inline ABI.
+   The string inline functions are an external ABI, thus cannot be changed
+   after the first release of a new target (unlike _STRING_ARCH_unaligned
+   which may be changed from release to release).  Targets must support
+   unaligned accesses in hardware if either define is set to true.  */
+#define _STRING_INLINE_unaligned   0
 
 #endif /* bits/string.h */
diff --git a/include/string.h b/include/string.h
index a684fd9..e145bfd 100644
--- a/include/string.h
+++ b/include/string.h
@@ -46,6 +46,9 @@ extern int __ffs (int __i) __attribute__ ((const));
 extern char *__strerror_r (int __errnum, char *__buf, size_t __buflen);
 #endif
 
+/* Get _STRING_ARCH_unaligned.  */
+#include <string_private.h>
+
 /* Now the real definitions.  We do this here since some of the functions
    above are defined as macros in the headers.  */
 #include <string/string.h>
diff --git a/string/bits/string2.h b/string/bits/string2.h
index b93be06..8200ef1 100644
--- a/string/bits/string2.h
+++ b/string/bits/string2.h
@@ -46,7 +46,7 @@
 # endif
 #endif
 
-#if _STRING_ARCH_unaligned
+#if _STRING_INLINE_unaligned
 /* If we can do unaligned memory accesses we must know the endianess.  */
 # include <endian.h>
 # include <bits/types.h>
@@ -95,7 +95,7 @@ __STRING2_COPY_TYPE (8);
 /* Set N bytes of S to C.  */
 #if !defined _HAVE_STRING_ARCH_memset
 # if !__GNUC_PREREQ (3, 0)
-#  if _STRING_ARCH_unaligned
+#  if _STRING_INLINE_unaligned
 #   define memset(s, c, n) \
   (__extension__ (__builtin_constant_p (n) && (n) <= 16			      \
 		  ? ((n) == 1						      \
@@ -223,7 +223,7 @@ __STRING2_COPY_TYPE (8);
 #  endif
 
 #  if !__GNUC_PREREQ (3, 0) || defined _FORCE_INLINES
-#   if _STRING_ARCH_unaligned
+#   if _STRING_INLINE_unaligned
 #    ifndef _FORCE_INLINES
 #     define __mempcpy_args(src) \
      ((const char *) (src))[0], ((const char *) (src))[2],		      \
@@ -419,7 +419,7 @@ extern void *__rawmemchr (const void *__s, int __c);
 		  : strcpy (dest, src)))
 # endif
 
-# if _STRING_ARCH_unaligned
+# if _STRING_INLINE_unaligned
 #  ifndef _FORCE_INLINES
 #   define __strcpy_args(src) \
      __extension__ __STRING2_SMALL_GET16 (src, 0),			      \
@@ -598,7 +598,7 @@ __strcpy_small (char *__dest,
 #  endif
 
 #  if !__GNUC_PREREQ (3, 0) || defined _FORCE_INLINES
-#   if _STRING_ARCH_unaligned
+#   if _STRING_INLINE_unaligned
 #    ifndef _FORCE_INLINES
 #     define __stpcpy_args(src) \
      __extension__ __STRING2_SMALL_GET16 (src, 0),			      \
diff --git a/sysdeps/aarch64/bits/string.h b/sysdeps/aarch64/bits/string.h
index 3c2a50b..0a508f7 100644
--- a/sysdeps/aarch64/bits/string.h
+++ b/sysdeps/aarch64/bits/string.h
@@ -20,5 +20,5 @@
 # error "Never use <bits/string.h> directly; include <string.h> instead."
 #endif
 
-/* AArch64 implementations support efficient unaligned access.  */
-#define _STRING_ARCH_unaligned 1
+/* AArch64 uses the aligned string inline ABI.  */
+#define _STRING_INLINE_unaligned 0
diff --git a/sysdeps/aarch64/string_private.h b/sysdeps/aarch64/string_private.h
new file mode 100644
index 0000000..8b194a4
--- /dev/null
+++ b/sysdeps/aarch64/string_private.h
@@ -0,0 +1,20 @@
+/* Define _STRING_ARCH_unaligned.  AArch64 version.
+   Copyright (C) 2016 Free Software Foundation, Inc.
+   This file is part of the GNU C Library.
+
+   The GNU C Library is free software; you can redistribute it and/or
+   modify it under the terms of the GNU Lesser General Public
+   License as published by the Free Software Foundation; either
+   version 2.1 of the License, or (at your option) any later version.
+
+   The GNU C Library is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+   Lesser General Public License for more details.
+
+   You should have received a copy of the GNU Lesser General Public
+   License along with the GNU C Library; if not, see
+   <http://www.gnu.org/licenses/>.  */
+
+/* AArch64 implementations support efficient unaligned access.  */
+#define _STRING_ARCH_unaligned 1
diff --git a/sysdeps/generic/string_private.h b/sysdeps/generic/string_private.h
new file mode 100644
index 0000000..c880aae
--- /dev/null
+++ b/sysdeps/generic/string_private.h
@@ -0,0 +1,21 @@
+/* Define _STRING_ARCH_unaligned.  Generic version.
+   Copyright (C) 2016 Free Software Foundation, Inc.
+   This file is part of the GNU C Library.
+
+   The GNU C Library is free software; you can redistribute it and/or
+   modify it under the terms of the GNU Lesser General Public
+   License as published by the Free Software Foundation; either
+   version 2.1 of the License, or (at your option) any later version.
+
+   The GNU C Library is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+   Lesser General Public License for more details.
+
+   You should have received a copy of the GNU Lesser General Public
+   License along with the GNU C Library; if not, see
+   <http://www.gnu.org/licenses/>.  */
+
+/* Define to 1 if architecture can access unaligned multi-byte
+   variables.  */
+#define _STRING_ARCH_unaligned   0
diff --git a/sysdeps/m68k/m680x0/m68020/bits/string.h b/sysdeps/m68k/m680x0/m68020/bits/string.h
index 88505bb..78c9594 100644
--- a/sysdeps/m68k/m680x0/m68020/bits/string.h
+++ b/sysdeps/m68k/m680x0/m68020/bits/string.h
@@ -20,6 +20,5 @@
 # error "Never use <bits/string.h> directly; include <string.h> instead."
 #endif
 
-/* Currently the only purpose of this file is to tell the generic inline
-   macros that unaligned memory access is possible.  */
-#define _STRING_ARCH_unaligned	1
+/* Use the unaligned string inline ABI.  */
+#define _STRING_INLINE_unaligned 1
diff --git a/sysdeps/m68k/m680x0/m68020/string_private.h b/sysdeps/m68k/m680x0/m68020/string_private.h
new file mode 100644
index 0000000..6846663
--- /dev/null
+++ b/sysdeps/m68k/m680x0/m68020/string_private.h
@@ -0,0 +1,21 @@
+/* Define _STRING_ARCH_unaligned.  m680x0 version, x >= 2.
+   Copyright (C) 2016 Free Software Foundation, Inc.
+   This file is part of the GNU C Library.
+
+   The GNU C Library is free software; you can redistribute it and/or
+   modify it under the terms of the GNU Lesser General Public
+   License as published by the Free Software Foundation; either
+   version 2.1 of the License, or (at your option) any later version.
+
+   The GNU C Library is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+   Lesser General Public License for more details.
+
+   You should have received a copy of the GNU Lesser General Public
+   License along with the GNU C Library; if not, see
+   <http://www.gnu.org/licenses/>.  */
+
+/* Tell the generic inline macros that unaligned memory access is
+   possible.  */
+#define _STRING_ARCH_unaligned   1
diff --git a/sysdeps/s390/bits/string.h b/sysdeps/s390/bits/string.h
index 09201d9..39e0b7f 100644
--- a/sysdeps/s390/bits/string.h
+++ b/sysdeps/s390/bits/string.h
@@ -21,8 +21,8 @@
 # error "Never use <bits/string.h> directly; include <string.h> instead."
 #endif
 
-/* The s390 processors can access unaligned multi-byte variables.  */
-#define _STRING_ARCH_unaligned	1
+/* Use the unaligned string inline ABI.  */
+#define _STRING_INLINE_unaligned 1
 
 /* We only provide optimizations if the user selects them and if
    GNU CC is used.  */
diff --git a/sysdeps/s390/string_private.h b/sysdeps/s390/string_private.h
new file mode 100644
index 0000000..9e11eee
--- /dev/null
+++ b/sysdeps/s390/string_private.h
@@ -0,0 +1,20 @@
+/* Define _STRING_ARCH_unaligned.  S/390 version.
+   Copyright (C) 2016 Free Software Foundation, Inc.
+   This file is part of the GNU C Library.
+
+   The GNU C Library is free software; you can redistribute it and/or
+   modify it under the terms of the GNU Lesser General Public
+   License as published by the Free Software Foundation; either
+   version 2.1 of the License, or (at your option) any later version.
+
+   The GNU C Library is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+   Lesser General Public License for more details.
+
+   You should have received a copy of the GNU Lesser General Public
+   License along with the GNU C Library; if not, see
+   <http://www.gnu.org/licenses/>.  */
+
+/* The s390 processors can access unaligned multi-byte variables.  */
+#define _STRING_ARCH_unaligned   1
diff --git a/sysdeps/sparc/bits/string.h b/sysdeps/sparc/bits/string.h
index 3054f62..10beca6 100644
--- a/sysdeps/sparc/bits/string.h
+++ b/sysdeps/sparc/bits/string.h
@@ -20,8 +20,8 @@
 # error "Never use <bits/string.h> directly; include <string.h> instead."
 #endif
 
-/* Define if architecture can access unaligned multi-byte variables.  */
-#define _STRING_ARCH_unaligned   0
+/* sparc uses the aligned string inline ABI.  */
+#define _STRING_INLINE_unaligned 0
 
 /* sparc32 and sparc64 strchr(x, '\0') perform better than
    __rawmemchr(x, '\0').  */
diff --git a/sysdeps/x86/bits/string.h b/sysdeps/x86/bits/string.h
index 1a0682b..e4e019f 100644
--- a/sysdeps/x86/bits/string.h
+++ b/sysdeps/x86/bits/string.h
@@ -20,8 +20,8 @@
 # error "Never use <bits/string.h> directly; include <string.h> instead."
 #endif
 
-/* The ix86 processors can access unaligned multi-byte variables.  */
-#define _STRING_ARCH_unaligned	1
+/* Use the unaligned string inline ABI.  */
+#define _STRING_INLINE_unaligned 1
 
 /* Enable inline functions only for i486 or better when compiling for
    ia32.  */
diff --git a/sysdeps/x86/string_private.h b/sysdeps/x86/string_private.h
new file mode 100644
index 0000000..e7281eb
--- /dev/null
+++ b/sysdeps/x86/string_private.h
@@ -0,0 +1,20 @@
+/* Define _STRING_ARCH_unaligned.  i486/x86-64 version.
+   Copyright (C) 2016 Free Software Foundation, Inc.
+   This file is part of the GNU C Library.
+
+   The GNU C Library is free software; you can redistribute it and/or
+   modify it under the terms of the GNU Lesser General Public
+   License as published by the Free Software Foundation; either
+   version 2.1 of the License, or (at your option) any later version.
+
+   The GNU C Library is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+   Lesser General Public License for more details.
+
+   You should have received a copy of the GNU Lesser General Public
+   License along with the GNU C Library; if not, see
+   <http://www.gnu.org/licenses/>.  */
+
+/* The ix86 processors can access unaligned multi-byte variables.  */
+#define _STRING_ARCH_unaligned   1
-- 
2.5.0