libsanitizer: Check assembler support for symbol assignment [PR112563]

Message ID yddzfyzz75d.fsf@CeBiTec.Uni-Bielefeld.DE
State New
Headers
Series libsanitizer: Check assembler support for symbol assignment [PR112563] |

Checks

Context Check Description
linaro-tcwg-bot/tcwg_gcc_build--master-arm success Testing passed
linaro-tcwg-bot/tcwg_gcc_build--master-aarch64 success Testing passed
linaro-tcwg-bot/tcwg_gcc_check--master-arm success Testing passed
linaro-tcwg-bot/tcwg_gcc_check--master-aarch64 success Testing passed

Commit Message

Rainer Orth Nov. 27, 2023, 12:56 p.m. UTC
  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
  

Comments

Jakub Jelinek Nov. 27, 2023, 1:08 p.m. UTC | #1
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
  
Rainer Orth Nov. 27, 2023, 1:20 p.m. UTC | #2
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
  
Jakub Jelinek Nov. 27, 2023, 1:26 p.m. UTC | #3
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
  
Rainer Orth Nov. 27, 2023, 1:29 p.m. UTC | #4
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 Nov. 28, 2023, 1:16 p.m. UTC | #5
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
  
Jakub Jelinek Nov. 28, 2023, 1:40 p.m. UTC | #6
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
  

Patch

# 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