[PATCH] Enable libsanitizer build on riscv64

classic Classic list List threaded Threaded
7 messages Options
Reply | Threaded
Open this post in threaded view
|

[PATCH] Enable libsanitizer build on riscv64

Andreas Schwab
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(+)

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


--
Andreas Schwab, SUSE Labs, [hidden email]
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Enable libsanitizer build on riscv64

Jim Wilson-2
On Mon, Nov 11, 2019 at 3:45 AM Andreas Schwab <[hidden email]> 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
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Enable libsanitizer build on riscv64

Andreas Schwab
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.

--
Andreas Schwab, SUSE Labs, [hidden email]
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Enable libsanitizer build on riscv64

Jakub Jelinek
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

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Enable libsanitizer build on riscv64

Jakub Jelinek
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

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Enable libsanitizer build on riscv64

Jim Wilson-2
In reply to this post by Andreas Schwab
On Mon, Nov 11, 2019 at 3:45 AM Andreas Schwab <[hidden email]> 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
Reply | Threaded
Open this post in threaded view
|

[committed] Fix up libsanitizer build with master glibc

Jakub Jelinek
In reply to this post by Jakub Jelinek
Hi!

On Tue, Nov 12, 2019 at 12:27:26PM +0100, Jakub Jelinek wrote:
> 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.

I have committed the following change cherry-picked from upstream,
before submitting there tested on
{x86_64,i686,ppc64le,s390x,armv7hl,aarch64}-linux with both older glibc and
with master glibc (in that case the patch tweaked to have __GLIBC_PREREQ (2,
30) instead of 2, 31, so that even before the 2.31 release on a glibc known
to have the new layout it would verify what will be verified in glibc 2.31.

arm big endian and s390 31-bit is still broken for shmctl and I'm afraid
fixing that is going to be extremely hard.

2019-11-26  Jakub Jelinek  <[hidden email]>

        PR sanitizer/92154
        * sanitizer_common/sanitizer_platform_limits_posix.h: Cherry-pick
        llvm-project revision 947f9692440836dcb8d88b74b69dd379d85974ce.
        * sanitizer_common/sanitizer_platform_limits_posix.cpp: Likewise.

--- libsanitizer/sanitizer_common/sanitizer_platform_limits_posix.cpp (revision 278721)
+++ libsanitizer/sanitizer_common/sanitizer_platform_limits_posix.cpp (working copy)
@@ -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 many architectures.  */
 CHECK_SIZE_AND_OFFSET(ipc_perm, mode);
 #endif
 
--- libsanitizer/sanitizer_common/sanitizer_platform_limits_posix.h (revision 278721)
+++ libsanitizer/sanitizer_common/sanitizer_platform_limits_posix.h (working copy)
@@ -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)


        Jakub