libsanitizer: Check assembler support for symbol assignment [PR112563]
Checks
Commit Message
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
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
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
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
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
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
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
# HG changeset patch
# Parent 1f757467f1bed35373c55b65cde4f9b0506172f5
libsanitizer: Require assembler support for sanitizer_redefine_builtins.h [PR112563]
@@ -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.
@@ -12,6 +12,7 @@
#ifndef SANITIZER_DEFS_H
#define SANITIZER_DEFS_H
+#include "config.h"
#include "sanitizer_platform.h"
#include "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