diff mbox series

Enable libsanitizer build on riscv64

Message ID mvmzhh2d4cn.fsf@suse.de
State New
Headers show
Series Enable libsanitizer build on riscv64 | expand

Commit Message

Andreas Schwab Nov. 11, 2019, 11:44 a.m. UTC
Only ubsan is supported so far.  This has been tested on openSUSE
Tumbleweed, there are no testsuite failures.

	* configure.tgt (riscv64-*-linux*): Enable build.
---
 libsanitizer/configure.tgt | 2 ++
 1 file changed, 2 insertions(+)

Comments

Jim Wilson Nov. 12, 2019, 5:06 a.m. UTC | #1
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
Andreas Schwab Nov. 12, 2019, 9:56 a.m. UTC | #2
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.
Jakub Jelinek Nov. 12, 2019, 10:32 a.m. UTC | #3
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
Jakub Jelinek Nov. 12, 2019, 11:27 a.m. UTC | #4
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
Jim Wilson Nov. 12, 2019, 9:50 p.m. UTC | #5
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 mbox series

Patch

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
 	;;