[PATCH] libsanitizer: Check assembler support for symbol assignment [PR112563]

[PATCH] libsanitizer: Check assembler support for symbol assignment [PR112563]

[Rainer Orth]

[-- Attachment #1: Type: text/plain, Size: 1316 bytes --]

The recent libsanitizer import broke the build on Solaris/SPARC with the
native as:

/usr/ccs/bin/as: ".libs/sanitizer_errno.s", line 4247: error: symbol "__sanitizer_internal_memset" is used but not defined
/usr/ccs/bin/as: ".libs/sanitizer_errno.s", line 4247: error: symbol "__sanitizer_internal_memcpy" is used but not defined
/usr/ccs/bin/as: ".libs/sanitizer_errno.s", line 4247: error: symbol "__sanitizer_internal_memmove" is used but not defined

Since none of the alternatives considered in the PR worked out, this
patch checks if the assembler does support symbol assignment, disabling
the code otherwise.  This returns the code to the way it was up to LLVM 16.

Bootstrapped without regressions on sparc-sun-solaris2.11 (as and gas),
i386-pc-solaris2.11, x86_64-pc-linux-gnu, and x86_64-apple-darwin21.6.0.

Ok for trunk?

	Rainer

-- 
-----------------------------------------------------------------------------
Rainer Orth, Center for Biotechnology, Bielefeld University


2023-11-23  Rainer Orth  <ro@CeBiTec.Uni-Bielefeld.DE>

	libsanitizer:
	PR libsanitizer/112563
	* configure.ac (libsanitizer_cv_as_sym_assign): Check for
	assembler symbol assignment support.
	* configure, config.h.in: Regenerate.
	* sanitizer_common/sanitizer_redefine_builtins.h: Include config.h.
	Check HAVE_AS_SYM_ASSIGN.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: sol2sparc-libsanitizer-sanitizer_redefine_builtins.patch --]
[-- Type: text/x-patch, Size: 2295 bytes --]

# HG changeset patch
# Parent  1f757467f1bed35373c55b65cde4f9b0506172f5
libsanitizer: Require assembler support for sanitizer_redefine_builtins.h [PR112563]

diff --git a/libsanitizer/configure.ac b/libsanitizer/configure.ac
--- a/libsanitizer/configure.ac
+++ b/libsanitizer/configure.ac
@@ -214,6 +214,19 @@ if test "$libsanitizer_cv_sys_atomic" = 
 	    [Define to 1 if you have the __atomic functions])
 fi
 
+# Check if assembler supports symbol assignment.
+AC_CACHE_CHECK([assembler symbol assignment],
+[libsanitizer_cv_as_sym_assign],
+[AC_COMPILE_IFELSE(
+  [AC_LANG_PROGRAM([],
+		   [asm("a = b");])],
+  [libsanitizer_cv_as_sym_assign=yes],
+  [libsanitizer_cv_as_sym_assign=no])])
+if test "$libsanitizer_cv_as_sym_assign" = "yes"; then
+  AC_DEFINE([HAVE_AS_SYM_ASSIGN], 1,
+  	    [Define to 1 if assembler supports symbol assignment])
+fi
+
 # The library needs to be able to read the executable itself.  Compile
 # a file to determine the executable format.  The awk script
 # filetype.awk prints out the file type.
diff --git a/libsanitizer/sanitizer_common/sanitizer_internal_defs.h b/libsanitizer/sanitizer_common/sanitizer_internal_defs.h
--- a/libsanitizer/sanitizer_common/sanitizer_internal_defs.h
+++ b/libsanitizer/sanitizer_common/sanitizer_internal_defs.h
@@ -12,6 +12,7 @@
 #ifndef SANITIZER_DEFS_H
 #define SANITIZER_DEFS_H
 
+#include "config.h"
 #include "sanitizer_platform.h"
 #include "sanitizer_redefine_builtins.h"
 
diff --git a/libsanitizer/sanitizer_common/sanitizer_redefine_builtins.h b/libsanitizer/sanitizer_common/sanitizer_redefine_builtins.h
--- a/libsanitizer/sanitizer_common/sanitizer_redefine_builtins.h
+++ b/libsanitizer/sanitizer_common/sanitizer_redefine_builtins.h
@@ -15,7 +15,7 @@
 #    define SANITIZER_REDEFINE_BUILTINS_H
 
 // The asm hack only works with GCC and Clang.
-#    if !defined(_WIN32)
+#    if !defined(_WIN32) && defined(HAVE_AS_SYM_ASSIGN)
 
 asm("memcpy = __sanitizer_internal_memcpy");
 asm("memmove = __sanitizer_internal_memmove");
@@ -50,7 +50,7 @@ using vector = Define_SANITIZER_COMMON_N
 }  // namespace std
 
 #      endif  // __cpluplus
-#    endif    // !_WIN32
+#    endif    // !_WIN32 && HAVE_AS_SYM_ASSIGN
 
 #  endif  // SANITIZER_REDEFINE_BUILTINS_H
 #endif    // SANITIZER_COMMON_NO_REDEFINE_BUILTINS

Re: [PATCH] libsanitizer: Check assembler support for symbol assignment [PR112563]

[Jakub Jelinek]

On Mon, Nov 27, 2023 at 01:56:46PM +0100, Rainer Orth wrote:
> The recent libsanitizer import broke the build on Solaris/SPARC with the
> native as:
> 
> /usr/ccs/bin/as: ".libs/sanitizer_errno.s", line 4247: error: symbol "__sanitizer_internal_memset" is used but not defined
> /usr/ccs/bin/as: ".libs/sanitizer_errno.s", line 4247: error: symbol "__sanitizer_internal_memcpy" is used but not defined
> /usr/ccs/bin/as: ".libs/sanitizer_errno.s", line 4247: error: symbol "__sanitizer_internal_memmove" is used but not defined
> 
> Since none of the alternatives considered in the PR worked out, this
> patch checks if the assembler does support symbol assignment, disabling
> the code otherwise.  This returns the code to the way it was up to LLVM 16.
> 
> Bootstrapped without regressions on sparc-sun-solaris2.11 (as and gas),
> i386-pc-solaris2.11, x86_64-pc-linux-gnu, and x86_64-apple-darwin21.6.0.
> 
> Ok for trunk?
> 
> 	Rainer
> 
> -- 
> -----------------------------------------------------------------------------
> Rainer Orth, Center for Biotechnology, Bielefeld University
> 
> 
> 2023-11-23  Rainer Orth  <ro@CeBiTec.Uni-Bielefeld.DE>
> 
> 	libsanitizer:
> 	PR libsanitizer/112563
> 	* configure.ac (libsanitizer_cv_as_sym_assign): Check for
> 	assembler symbol assignment support.
> 	* configure, config.h.in: Regenerate.
> 	* sanitizer_common/sanitizer_redefine_builtins.h: Include config.h.
> 	Check HAVE_AS_SYM_ASSIGN.

Can you please
1) split it into 2 patches, one touching config* which is owned by GCC (and
   Makefiles, see later), one just sanitizer_common/sanitizer_redefine_builtins.h
2) avoid using config.h in, instead use AC_SUBST and add @HAVE_AS_SYM_ASSIGN@
   to Makefile.am's DEFS where needed (either expanding to nothing or
   -DHAVE_AS_SYM_ASSIGN=1)?  The reason is to minimize changes to imported
   sources

Once the sanitizer_common/sanitizer_redefine_builtins.h change (just
the && defined(HAVE_AS_SYM_ASSIGN) addition) patch is committed and pushed
upstream, add its commit has LOCAL_PATCHES.

Thanks.

Note, your ChangeLog entry was pretending config.h include has been added
to one header, but it went to a different one instead.

> # HG changeset patch
> # Parent  1f757467f1bed35373c55b65cde4f9b0506172f5
> libsanitizer: Require assembler support for sanitizer_redefine_builtins.h [PR112563]
> 
> diff --git a/libsanitizer/configure.ac b/libsanitizer/configure.ac
> --- a/libsanitizer/configure.ac
> +++ b/libsanitizer/configure.ac
> @@ -214,6 +214,19 @@ if test "$libsanitizer_cv_sys_atomic" = 
>  	    [Define to 1 if you have the __atomic functions])
>  fi
>  
> +# Check if assembler supports symbol assignment.
> +AC_CACHE_CHECK([assembler symbol assignment],
> +[libsanitizer_cv_as_sym_assign],
> +[AC_COMPILE_IFELSE(
> +  [AC_LANG_PROGRAM([],
> +		   [asm("a = b");])],
> +  [libsanitizer_cv_as_sym_assign=yes],
> +  [libsanitizer_cv_as_sym_assign=no])])
> +if test "$libsanitizer_cv_as_sym_assign" = "yes"; then
> +  AC_DEFINE([HAVE_AS_SYM_ASSIGN], 1,
> +  	    [Define to 1 if assembler supports symbol assignment])
> +fi
> +
>  # The library needs to be able to read the executable itself.  Compile
>  # a file to determine the executable format.  The awk script
>  # filetype.awk prints out the file type.
> diff --git a/libsanitizer/sanitizer_common/sanitizer_internal_defs.h b/libsanitizer/sanitizer_common/sanitizer_internal_defs.h
> --- a/libsanitizer/sanitizer_common/sanitizer_internal_defs.h
> +++ b/libsanitizer/sanitizer_common/sanitizer_internal_defs.h
> @@ -12,6 +12,7 @@
>  #ifndef SANITIZER_DEFS_H
>  #define SANITIZER_DEFS_H
>  
> +#include "config.h"
>  #include "sanitizer_platform.h"
>  #include "sanitizer_redefine_builtins.h"
>  
> diff --git a/libsanitizer/sanitizer_common/sanitizer_redefine_builtins.h b/libsanitizer/sanitizer_common/sanitizer_redefine_builtins.h
> --- a/libsanitizer/sanitizer_common/sanitizer_redefine_builtins.h
> +++ b/libsanitizer/sanitizer_common/sanitizer_redefine_builtins.h
> @@ -15,7 +15,7 @@
>  #    define SANITIZER_REDEFINE_BUILTINS_H
>  
>  // The asm hack only works with GCC and Clang.
> -#    if !defined(_WIN32)
> +#    if !defined(_WIN32) && defined(HAVE_AS_SYM_ASSIGN)
>  
>  asm("memcpy = __sanitizer_internal_memcpy");
>  asm("memmove = __sanitizer_internal_memmove");
> @@ -50,7 +50,7 @@ using vector = Define_SANITIZER_COMMON_N
>  }  // namespace std
>  
>  #      endif  // __cpluplus
> -#    endif    // !_WIN32
> +#    endif    // !_WIN32 && HAVE_AS_SYM_ASSIGN
>  
>  #  endif  // SANITIZER_REDEFINE_BUILTINS_H
>  #endif    // SANITIZER_COMMON_NO_REDEFINE_BUILTINS


	Jakub

Re: [PATCH] libsanitizer: Check assembler support for symbol assignment [PR112563]

[Rainer Orth]

Hi Jakub,

>> 2023-11-23  Rainer Orth  <ro@CeBiTec.Uni-Bielefeld.DE>
>> 
>> 	libsanitizer:
>> 	PR libsanitizer/112563
>> 	* configure.ac (libsanitizer_cv_as_sym_assign): Check for
>> 	assembler symbol assignment support.
>> 	* configure, config.h.in: Regenerate.
>> 	* sanitizer_common/sanitizer_redefine_builtins.h: Include config.h.
>> 	Check HAVE_AS_SYM_ASSIGN.
>
> Can you please
> 1) split it into 2 patches, one touching config* which is owned by GCC (and
>    Makefiles, see later), one just sanitizer_common/sanitizer_redefine_builtins.h
> 2) avoid using config.h in, instead use AC_SUBST and add @HAVE_AS_SYM_ASSIGN@
>    to Makefile.am's DEFS where needed (either expanding to nothing or
>    -DHAVE_AS_SYM_ASSIGN=1)?  The reason is to minimize changes to imported
>    sources
>
> Once the sanitizer_common/sanitizer_redefine_builtins.h change (just
> the && defined(HAVE_AS_SYM_ASSIGN) addition) patch is committed and pushed
> upstream, add its commit has LOCAL_PATCHES.

But will they accept a patch to check a macro never set anywhere in and
irrelevant to LLVM?  That's why I kept all in one patch, to be GCC-local.

If we go (or at least try) this upstream route, should I wait for
approval there and than commit both parts to GCC, keeping it in my local
tree until then?

> Note, your ChangeLog entry was pretending config.h include has been added
> to one header, but it went to a different one instead.

Drats, that's what you get for starting one way and adjusting later ;-)

	Rainer

-- 
-----------------------------------------------------------------------------
Rainer Orth, Center for Biotechnology, Bielefeld University

Re: [PATCH] libsanitizer: Check assembler support for symbol assignment [PR112563]

[Jakub Jelinek]

On Mon, Nov 27, 2023 at 02:20:25PM +0100, Rainer Orth wrote:
> Hi Jakub,
> 
> >> 2023-11-23  Rainer Orth  <ro@CeBiTec.Uni-Bielefeld.DE>
> >> 
> >> 	libsanitizer:
> >> 	PR libsanitizer/112563
> >> 	* configure.ac (libsanitizer_cv_as_sym_assign): Check for
> >> 	assembler symbol assignment support.
> >> 	* configure, config.h.in: Regenerate.
> >> 	* sanitizer_common/sanitizer_redefine_builtins.h: Include config.h.
> >> 	Check HAVE_AS_SYM_ASSIGN.
> >
> > Can you please
> > 1) split it into 2 patches, one touching config* which is owned by GCC (and
> >    Makefiles, see later), one just sanitizer_common/sanitizer_redefine_builtins.h
> > 2) avoid using config.h in, instead use AC_SUBST and add @HAVE_AS_SYM_ASSIGN@
> >    to Makefile.am's DEFS where needed (either expanding to nothing or
> >    -DHAVE_AS_SYM_ASSIGN=1)?  The reason is to minimize changes to imported
> >    sources
> >
> > Once the sanitizer_common/sanitizer_redefine_builtins.h change (just
> > the && defined(HAVE_AS_SYM_ASSIGN) addition) patch is committed and pushed
> > upstream, add its commit has LOCAL_PATCHES.
> 
> But will they accept a patch to check a macro never set anywhere in and
> irrelevant to LLVM?  That's why I kept all in one patch, to be GCC-local.

I meant the patch would be gcc local.
But, for later we need only the changes to the imported files be in one
commit, not others, because merge.sh will not revert the GCC owned changes,
just the imported ones, and so that is what should be reapplied.
And, the preference of not using config.h is because we do it like that
for other stuff already (exactly to minimize amount of local changes).

> If we go (or at least try) this upstream route, should I wait for
> approval there and than commit both parts to GCC, keeping it in my local
> tree until then?
> 
> > Note, your ChangeLog entry was pretending config.h include has been added
> > to one header, but it went to a different one instead.
> 
> Drats, that's what you get for starting one way and adjusting later ;-)

	Jakub

Re: [PATCH] libsanitizer: Check assembler support for symbol assignment [PR112563]

[Rainer Orth]

Hi Jakub,

>> But will they accept a patch to check a macro never set anywhere in and
>> irrelevant to LLVM?  That's why I kept all in one patch, to be GCC-local.
>
> I meant the patch would be gcc local.
> But, for later we need only the changes to the imported files be in one
> commit, not others, because merge.sh will not revert the GCC owned changes,
> just the imported ones, and so that is what should be reapplied.
> And, the preference of not using config.h is because we do it like that
> for other stuff already (exactly to minimize amount of local changes).

ah, now I get it.  Will rework the patch accordingly.

	Rainer

-- 
-----------------------------------------------------------------------------
Rainer Orth, Center for Biotechnology, Bielefeld University

Re: [PATCH] libsanitizer: Check assembler support for symbol assignment [PR112563]

[Rainer Orth]

[-- Attachment #1: Type: text/plain, Size: 1539 bytes --]

Hi Jakub,

>>> But will they accept a patch to check a macro never set anywhere in and
>>> irrelevant to LLVM?  That's why I kept all in one patch, to be GCC-local.
>>
>> I meant the patch would be gcc local.
>> But, for later we need only the changes to the imported files be in one
>> commit, not others, because merge.sh will not revert the GCC owned changes,
>> just the imported ones, and so that is what should be reapplied.
>> And, the preference of not using config.h is because we do it like that
>> for other stuff already (exactly to minimize amount of local changes).
>
> ah, now I get it.  Will rework the patch accordingly.

here are both patches.

Bootstrapped on sparc-sun-solaris2.11 (as and gas) and
i386-pc-solaris2.11 (as and gas).

Ok for trunk?

Thanks.
	Rainer

-- 
-----------------------------------------------------------------------------
Rainer Orth, Center for Biotechnology, Bielefeld University


2023-11-23  Rainer Orth  <ro@CeBiTec.Uni-Bielefeld.DE>

	libsanitizer:
	PR libsanitizer/112563
	* configure.ac (libsanitizer_cv_as_sym_assign): Check for
	assembler symbol assignment support.
	* configure, config.h.in: Regenerate.
	* asan/Makefile.am (DEFS): Add @AS_SYM_ASSIGN_DEFS@.
	* Makefile.in, asan/Makefile.in, hwasan/Makefile.in,
	interception/Makefile.in, libbacktrace/Makefile.in,
	lsan/Makefile.in, sanitizer_common/Makefile.in, tsan/Makefile.in,
	ubsan/Makefile.in: Regenerate.

	libsanitizer:
	PR libsanitizer/112563
	* sanitizer_common/sanitizer_redefine_builtins.h: Check
	HAVE_AS_SYM_ASSIGN.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: sol2sparc-libsanitizer-as-symbol-assignment.patch --]
[-- Type: text/x-patch, Size: 1859 bytes --]

# HG changeset patch
# Parent  1f757467f1bed35373c55b65cde4f9b0506172f5
libsanitizer: Check assembler support for symbol assignment [PR112563]

diff --git a/libsanitizer/asan/Makefile.am b/libsanitizer/asan/Makefile.am
--- a/libsanitizer/asan/Makefile.am
+++ b/libsanitizer/asan/Makefile.am
@@ -3,7 +3,7 @@ AM_CPPFLAGS = -I $(top_srcdir)/include -
 # May be used by toolexeclibdir.
 gcc_version := $(shell @get_gcc_base_ver@ $(top_srcdir)/../gcc/BASE-VER)
 
-DEFS = -D_GNU_SOURCE -D_DEBUG -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -DASAN_HAS_EXCEPTIONS=1 -DASAN_NEEDS_SEGV=1 -DCAN_SANITIZE_UB=0 -DASAN_HAS_CXA_RETHROW_PRIMARY_EXCEPTION=0
+DEFS = -D_GNU_SOURCE -D_DEBUG -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -DASAN_HAS_EXCEPTIONS=1 -DASAN_NEEDS_SEGV=1 -DCAN_SANITIZE_UB=0 -DASAN_HAS_CXA_RETHROW_PRIMARY_EXCEPTION=0 @AS_SYM_ASSIGN_DEFS@
 if USING_MAC_INTERPOSE
 DEFS += -DMAC_INTERPOSE_FUNCTIONS -DMISSING_BLOCKS_SUPPORT
 endif
diff --git a/libsanitizer/configure.ac b/libsanitizer/configure.ac
--- a/libsanitizer/configure.ac
+++ b/libsanitizer/configure.ac
@@ -214,6 +214,19 @@ if test "$libsanitizer_cv_sys_atomic" = 
 	    [Define to 1 if you have the __atomic functions])
 fi
 
+# Check if assembler supports symbol assignment.
+AC_CACHE_CHECK([assembler symbol assignment],
+[libsanitizer_cv_as_sym_assign],
+[AC_COMPILE_IFELSE(
+  [AC_LANG_PROGRAM([],
+		   [asm("a = b");])],
+  [libsanitizer_cv_as_sym_assign=yes],
+  [libsanitizer_cv_as_sym_assign=no])])
+if test "$libsanitizer_cv_as_sym_assign" = "yes"; then
+  as_sym_assign_defs=-DHAVE_AS_SYM_ASSIGN=1
+fi
+AC_SUBST(AS_SYM_ASSIGN_DEFS, [$as_sym_assign_defs])
+
 # The library needs to be able to read the executable itself.  Compile
 # a file to determine the executable format.  The awk script
 # filetype.awk prints out the file type.

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #3: sol2sparc-libsanitizer-sanitizer_redefine_builtins.patch --]
[-- Type: text/x-patch, Size: 998 bytes --]

# HG changeset patch
# Parent  60ba51b7bcf476549e3d8165b7c90ec0a3cf0979
libsanitizer: Require assembler support for sanitizer_redefine_builtins.h [PR112563]

diff --git a/libsanitizer/sanitizer_common/sanitizer_redefine_builtins.h b/libsanitizer/sanitizer_common/sanitizer_redefine_builtins.h
--- a/libsanitizer/sanitizer_common/sanitizer_redefine_builtins.h
+++ b/libsanitizer/sanitizer_common/sanitizer_redefine_builtins.h
@@ -15,7 +15,7 @@
 #    define SANITIZER_REDEFINE_BUILTINS_H
 
 // The asm hack only works with GCC and Clang.
-#    if !defined(_WIN32)
+#    if !defined(_WIN32) && defined(HAVE_AS_SYM_ASSIGN)
 
 asm("memcpy = __sanitizer_internal_memcpy");
 asm("memmove = __sanitizer_internal_memmove");
@@ -50,7 +50,7 @@ using vector = Define_SANITIZER_COMMON_N
 }  // namespace std
 
 #      endif  // __cpluplus
-#    endif    // !_WIN32
+#    endif    // !_WIN32 && HAVE_AS_SYM_ASSIGN
 
 #  endif  // SANITIZER_REDEFINE_BUILTINS_H
 #endif    // SANITIZER_COMMON_NO_REDEFINE_BUILTINS

Re: [PATCH] libsanitizer: Check assembler support for symbol assignment [PR112563]

[Jakub Jelinek]

On Tue, Nov 28, 2023 at 02:16:11PM +0100, Rainer Orth wrote:
> Hi Jakub,
> 
> >>> But will they accept a patch to check a macro never set anywhere in and
> >>> irrelevant to LLVM?  That's why I kept all in one patch, to be GCC-local.
> >>
> >> I meant the patch would be gcc local.
> >> But, for later we need only the changes to the imported files be in one
> >> commit, not others, because merge.sh will not revert the GCC owned changes,
> >> just the imported ones, and so that is what should be reapplied.
> >> And, the preference of not using config.h is because we do it like that
> >> for other stuff already (exactly to minimize amount of local changes).
> >
> > ah, now I get it.  Will rework the patch accordingly.
> 
> here are both patches.
> 
> Bootstrapped on sparc-sun-solaris2.11 (as and gas) and
> i386-pc-solaris2.11 (as and gas).
> 
> Ok for trunk?

Yes, thanks.

	Jakub