| Message ID | mvmzhh2d4cn.fsf@suse.de |
|---|---|
| State | New |
| Headers | show |
| Series | Enable libsanitizer build on riscv64 | expand |
On Mon, Nov 11, 2019 at 3:45 AM Andreas Schwab <schwab@suse.de> wrote: > Only ubsan is supported so far. This has been tested on openSUSE > Tumbleweed, there are no testsuite failures. > > * configure.tgt (riscv64-*-linux*): Enable build. I tried this on my riscv64 Fedora system, and I get a build error. In file included from ../../../../gcc-git/libsanitizer/sanitizer_common/sanitizer_platform_limits_posix.cpp:167: ../../../../gcc-git/libsanitizer/sanitizer_common/sanitizer_internal_defs.h:342:72: error: narrowing conversion of ‘-1’ from ‘int’ to ‘long unsigned int’ [-Wnarrowing] 342 | typedef char IMPL_PASTE(assertion_failed_##_, line)[2*(int)(pred)-1] | ^ ... ../../../../gcc-git/libsanitizer/sanitizer_common/sanitizer_platform_limits_posix.cpp:1136:1: note: in expansion of macro ‘CHECK_SIZE_AND_OFFSET’ 1136 | CHECK_SIZE_AND_OFFSET(ipc_perm, mode); | ^~~~~~~~~~~~~~~~~~~~~ It appears that the problem is that my system defines the icp_perm mode field as __mode_t whereas the sanitizer assumes it will be unsigned short, followed by an unsigned short pad field. I haven't looked at the full history yet, but there were apparently similar problems with the aarch64 port, so maybe we need some special code for riscv to make this work? I don't know why it worked for you. Maybe a different glibc or kernel version? Incidentally, the libsanitizer/sanitizer_common/sanitizer_platform_limits_posix.h file has an obvious problem in the struct __sanitizer_ipc_perm definition, because it has an ifdef for __aarch64__ inside an ifdef for __sparc__, and there is no way the __aarch64__ test can ever succeed there. There is a second __aarch64__ test a little farther down that looks OK. Maybe a patch merge error? Jim
On Nov 11 2019, Jim Wilson wrote: > ../../../../gcc-git/libsanitizer/sanitizer_common/sanitizer_platform_limits_posix.cpp:1136:1: > note: in expansion of macro ‘CHECK_SIZE_AND_OFFSET’ > 1136 | CHECK_SIZE_AND_OFFSET(ipc_perm, mode); > | ^~~~~~~~~~~~~~~~~~~~~ Looks like you are using an unreleased version of glibc. This works correctly with glibc 2.30. As you have noticed, this will need to be corrected for all architectures where the ipc_perm structure has been changed in commit 2f959dfe84, once glibc 2.31 has been released. Care to file an llvm issue about that? > Incidentally, the > libsanitizer/sanitizer_common/sanitizer_platform_limits_posix.h file > has an obvious problem in the struct __sanitizer_ipc_perm definition, > because it has an ifdef for __aarch64__ inside an ifdef for __sparc__, That's __arch64__, not __aarch64__. Andreas.
On Tue, Nov 12, 2019 at 10:56:21AM +0100, Andreas Schwab wrote: > On Nov 11 2019, Jim Wilson wrote: > > > ../../../../gcc-git/libsanitizer/sanitizer_common/sanitizer_platform_limits_posix.cpp:1136:1: > > note: in expansion of macro ‘CHECK_SIZE_AND_OFFSET’ > > 1136 | CHECK_SIZE_AND_OFFSET(ipc_perm, mode); > > | ^~~~~~~~~~~~~~~~~~~~~ > > Looks like you are using an unreleased version of glibc. This works > correctly with glibc 2.30. > > As you have noticed, this will need to be corrected for all > architectures where the ipc_perm structure has been changed in commit > 2f959dfe84, once glibc 2.31 has been released. Care to file an llvm > issue about that? We actually have a change cherry-picked from upstream for the https://sourceware.org/git/?p=glibc.git;a=commitdiff;h=2f959dfe849e0646e27403f2e4091536496ac0f0 glibc change - https://gcc.gnu.org/ml/gcc-patches/2019-10/msg01554.html , but only for arm, while it apparently broke either all or many other architectures (at least x86_64 and riscv64 are now reported). Jakub
On Tue, Nov 12, 2019 at 11:32:56AM +0100, Jakub Jelinek wrote: > On Tue, Nov 12, 2019 at 10:56:21AM +0100, Andreas Schwab wrote: > > On Nov 11 2019, Jim Wilson wrote: > > > > > ../../../../gcc-git/libsanitizer/sanitizer_common/sanitizer_platform_limits_posix.cpp:1136:1: > > > note: in expansion of macro ‘CHECK_SIZE_AND_OFFSET’ > > > 1136 | CHECK_SIZE_AND_OFFSET(ipc_perm, mode); > > > | ^~~~~~~~~~~~~~~~~~~~~ > > > > Looks like you are using an unreleased version of glibc. This works > > correctly with glibc 2.30. > > > > As you have noticed, this will need to be corrected for all > > architectures where the ipc_perm structure has been changed in commit > > 2f959dfe84, once glibc 2.31 has been released. Care to file an llvm > > issue about that? > > We actually have a change cherry-picked from upstream for the > https://sourceware.org/git/?p=glibc.git;a=commitdiff;h=2f959dfe849e0646e27403f2e4091536496ac0f0 > glibc change - > https://gcc.gnu.org/ml/gcc-patches/2019-10/msg01554.html > , but only for arm, while it apparently broke either all or many other > architectures (at least x86_64 and riscv64 are now reported). From the linux targets supported by GCC libsanitizer, I think affected are sparc 32-bit, s390 31-bit (this one is even an ABI change, as mode not only changed size, but on big endian didn't change offset and unfortunately libsanitizer intercepts shmctl), arm (again, on big endian an ABI change which libsanitizer interception will not cope with, as it uses dlsym rather than dlvsym and is not symbol versioned), x86_64, i?86, riscv64. So, either we go for something like untested: --- libsanitizer/sanitizer_common/sanitizer_platform_limits_posix.cpp 2019-11-07 17:56:23.551835239 +0100 +++ libsanitizer/sanitizer_common/sanitizer_platform_limits_posix.cpp 2019-11-12 12:14:24.216763190 +0100 @@ -1129,10 +1129,12 @@ CHECK_SIZE_AND_OFFSET(ipc_perm, gid); CHECK_SIZE_AND_OFFSET(ipc_perm, cuid); CHECK_SIZE_AND_OFFSET(ipc_perm, cgid); #if (!defined(__aarch64__) || !SANITIZER_LINUX || __GLIBC_PREREQ (2, 21)) && \ - !defined(__arm__) + (!SANITIZER_LINUX || !__GLIBC_PREREQ (2, 30) || \ + defined(__powerpc__) || (defined(__sparc__) && defined(__arch64__)) \ + defined(__mips__) || defined(__aarch64__) || defined(__s390x__)) /* On aarch64 glibc 2.20 and earlier provided incorrect mode field. */ -/* On Arm newer glibc provide a different mode field, it's hard to detect - so just disable the check. */ +/* glibc 2.30 and earlier provided 16-bit mode field instead of 32-bit + on most architectures. */ CHECK_SIZE_AND_OFFSET(ipc_perm, mode); #endif or perhaps better change sanitizer_platform_limits_posix.h to match the glibc 2.31 definition and similarly to aarch64 don't check mode for !__GLIBC_PREREQ (2, 31), that would be something like untested: --- libsanitizer/sanitizer_common/sanitizer_platform_limits_posix.h 2019-11-07 17:56:23.530835549 +0100 +++ libsanitizer/sanitizer_common/sanitizer_platform_limits_posix.h 2019-11-12 12:22:26.314511706 +0100 @@ -207,26 +207,13 @@ struct __sanitizer_ipc_perm { u64 __unused1; u64 __unused2; #elif defined(__sparc__) -#if defined(__arch64__) unsigned mode; - unsigned short __pad1; -#else - unsigned short __pad1; - unsigned short mode; unsigned short __pad2; -#endif unsigned short __seq; unsigned long long __unused1; unsigned long long __unused2; -#elif defined(__mips__) || defined(__aarch64__) || defined(__s390x__) - unsigned int mode; - unsigned short __seq; - unsigned short __pad1; - unsigned long __unused1; - unsigned long __unused2; #else - unsigned short mode; - unsigned short __pad1; + unsigned int mode; unsigned short __seq; unsigned short __pad2; #if defined(__x86_64__) && !defined(_LP64) --- libsanitizer/sanitizer_common/sanitizer_platform_limits_posix.cpp 2019-11-07 17:56:23.551835239 +0100 +++ libsanitizer/sanitizer_common/sanitizer_platform_limits_posix.cpp 2019-11-12 12:23:42.959358844 +0100 @@ -1128,11 +1128,9 @@ CHECK_SIZE_AND_OFFSET(ipc_perm, uid); CHECK_SIZE_AND_OFFSET(ipc_perm, gid); CHECK_SIZE_AND_OFFSET(ipc_perm, cuid); CHECK_SIZE_AND_OFFSET(ipc_perm, cgid); -#if (!defined(__aarch64__) || !SANITIZER_LINUX || __GLIBC_PREREQ (2, 21)) && \ - !defined(__arm__) -/* On aarch64 glibc 2.20 and earlier provided incorrect mode field. */ -/* On Arm newer glibc provide a different mode field, it's hard to detect - so just disable the check. */ +#if !SANITIZER_LINUX || __GLIBC_PREREQ (2, 31) +/* glibc 2.30 and earlier provided 16-bit mode field instead of 32-bit + on most architectures. */ CHECK_SIZE_AND_OFFSET(ipc_perm, mode); #endif But I'm afraid I don't really have the cycles to test this on all targets, nor does it fix the arm be or s390 31-bit problem with shmctl. Jakub
On Mon, Nov 11, 2019 at 3:45 AM Andreas Schwab <schwab@suse.de> wrote: > Only ubsan is supported so far. This has been tested on openSUSE > Tumbleweed, there are no testsuite failures. > > * configure.tgt (riscv64-*-linux*): Enable build. With a workaround for the ipc_perm/mode issue, I can reproduce the same result on my Fedora rawhide machine. This patch is OK. Jim
diff --git a/libsanitizer/configure.tgt b/libsanitizer/configure.tgt index 714f2923605..f7f3a6bd3ff 100644 --- a/libsanitizer/configure.tgt +++ b/libsanitizer/configure.tgt @@ -65,6 +65,8 @@ case "${target}" in ;; x86_64-*-solaris2.11* | i?86-*-solaris2.11*) ;; + riscv64-*-linux*) + ;; *) UNSUPPORTED=1 ;;