diff mbox series

Add sanitizer support for AArch64 ILP32

Message ID 1519396039-22827-1-git-send-email-adhemerval.zanella@linaro.org
State New
Headers show
Series Add sanitizer support for AArch64 ILP32 | expand

Commit Message

Adhemerval Zanella Feb. 23, 2018, 2:27 p.m. UTC 
This patch adds asan support for aarch64 ilp32.  It is based on 'official'
glibc support branch [1] (which is in turn based on Linux branch [2]).

Main changes for libsanitizer is the kernel ABI support for internal
syscalls. Different than x32, ILP32 tries to leverage 32-bits syscalls
with kernel ABI for 64 bit argument passing (ftruncate for instance)
are passed using the new Linux generic way for 32 bits (by splitting
high and low in two registers).

So instead of adding more adhoc defines to ILP32 this patch extends the
SANITIZER_USES_CANONICAL_LINUX_SYSCALLS to represent both 64 bits
argument syscalls (value of 1) and 32 bits (value of 2).

The shadow offset used is similar to the arm one (29).

I did a sanity check on aarch64-linux-gnu and x86_64-linux-gnu without
any regressions.

PS: I sent this change llvm-commit, but it got stalled because llvm
lack aarch64 ilp32 support and noone could confirm no breakage due
missing buildbot.  Also the idea is not sync it back to libsanitizer.

gcc/

	* gcc/config/aarch64/aarch64.c (aarch64_asan_shadow_offset): Add
	TARGET_ILP32 support.

libsanitizer/

	* libsanitizer/sanitizer_common/sanitizer_internal_defs.h
	(uhwptr, OFF_T, operator_new_size_type): Define for
	SANITIZER_AARCH64_ILP32.
	* libsanitizer/sanitizer_common/sanitizer_linux.cc (SYSCALL_LL64):
	New define: wrap a 64-bit argument for syscall.
	(internal_ftruncate, internal_stat, internal_lstat, internal_unlink,
	internal_rename, internal_lseek): Add support for
	SANITIZER_USES_CANONICAL_LINUX_SYSCALLS equal to 2.
	(FileExists): Use internal_stat regardless.
	* libsanitizer/sanitizer_common/sanitizer_platform.h
	(SANITIZER_AARCH64_ILP32): Define.
	(SANITIZER_USES_CANONICAL_LINUX_SYSCALLS): Define to 1 or 2 depending
	of the expected architecture ABI.
	* libsanitizer/sanitizer_common/sanitizer_platform_limits_posix.cc
	(CHECK_SIZE_AND_OFFSET(ipc_perm,mode): Define for AArch64 ILP32.
	* libsanitizer/sanitizer_common/sanitizer_platform_limits_posix.h
	(struct_kernel_stat_sz, struct_kernel_stat64_sz,
	__sanitizer___kernel_uid_t, __sanitizer___kernel_gid_t): Likewise.
	[__sanitizer_dirent] (d_ino, d_off): Likewise.

[1] https://sourceware.org/git/?p=glibc.git;a=shortlog;h=refs/heads/arm/ilp32
[2] git://git.kernel.org/pub/scm/linux/kernel/git/arm64/linux.git
---
 gcc/config/aarch64/aarch64.c                       |  5 ++-
 .../sanitizer_common/sanitizer_internal_defs.h     | 12 +++----
 libsanitizer/sanitizer_common/sanitizer_linux.cc   | 40 ++++++++++++++++++----
 libsanitizer/sanitizer_common/sanitizer_platform.h | 15 ++++++--
 .../sanitizer_platform_limits_posix.cc             |  3 +-
 .../sanitizer_platform_limits_posix.h              | 13 +++++--
 6 files changed, 69 insertions(+), 19 deletions(-)

Comments

Jakub Jelinek Feb. 23, 2018, 2:39 p.m. UTC | #1 
On Fri, Feb 23, 2018 at 11:27:19AM -0300, Adhemerval Zanella wrote:
> This patch adds asan support for aarch64 ilp32.  It is based on 'official'
> glibc support branch [1] (which is in turn based on Linux branch [2]).
> 
> Main changes for libsanitizer is the kernel ABI support for internal
> syscalls. Different than x32, ILP32 tries to leverage 32-bits syscalls
> with kernel ABI for 64 bit argument passing (ftruncate for instance)
> are passed using the new Linux generic way for 32 bits (by splitting
> high and low in two registers).
> 
> So instead of adding more adhoc defines to ILP32 this patch extends the
> SANITIZER_USES_CANONICAL_LINUX_SYSCALLS to represent both 64 bits
> argument syscalls (value of 1) and 32 bits (value of 2).
> 
> The shadow offset used is similar to the arm one (29).
> 
> I did a sanity check on aarch64-linux-gnu and x86_64-linux-gnu without
> any regressions.
> 
> PS: I sent this change llvm-commit, but it got stalled because llvm
> lack aarch64 ilp32 support and noone could confirm no breakage due
> missing buildbot.  Also the idea is not sync it back to libsanitizer.

It will be a nightmare for merges from upstream, but because the upstream is
so uncooperative probably there is no other way.

> gcc/
> 
> 	* gcc/config/aarch64/aarch64.c (aarch64_asan_shadow_offset): Add

No gcc/ prefixes in gcc/ChangeLog.

> 	TARGET_ILP32 support.
> 
> libsanitizer/

Nor libsanitizer/ prefixes in libsaniter/ChangeLog.
> --- a/libsanitizer/sanitizer_common/sanitizer_internal_defs.h
> +++ b/libsanitizer/sanitizer_common/sanitizer_internal_defs.h
> @@ -117,9 +117,9 @@ typedef signed   long long sptr;  // NOLINT
>  typedef unsigned long uptr;  // NOLINT
>  typedef signed   long sptr;  // NOLINT
>  #endif  // defined(_WIN64)
> -#if defined(__x86_64__)
> -// Since x32 uses ILP32 data model in 64-bit hardware mode, we must use
> -// 64-bit pointer to unwind stack frame.
> +#if defined(__x86_64__) || SANITIZER_AARCH64_ILP32
> +// Since x32 adn AArch64 ILP32 use ILP32 data model in 64-bit hardware mode, 

s/adn/and/

> @@ -445,11 +467,7 @@ uptr internal_execve(const char *filename, char *const argv[],
>  // ----------------- sanitizer_common.h
>  bool FileExists(const char *filename) {
>    struct stat st;
> -#if SANITIZER_USES_CANONICAL_LINUX_SYSCALLS
> -  if (internal_syscall(SYSCALL(newfstatat), AT_FDCWD, filename, &st, 0))
> -#else
>    if (internal_stat(filename, &st))
> -#endif
>      return false;
>    // Sanity check: filename is a regular file.
>    return S_ISREG(st.st_mode);

I'm uneasy about this hunk, you change behavior not just on aarch64 ilp32,
but for many other targets too.  Please don't.

Do you really want it for GCC8?  We are in stage4 and this isn't a
regression...

	Jakub
Adhemerval Zanella Feb. 23, 2018, 6:37 p.m. UTC | #2 
On 23/02/2018 11:39, Jakub Jelinek wrote:
> On Fri, Feb 23, 2018 at 11:27:19AM -0300, Adhemerval Zanella wrote:
>> This patch adds asan support for aarch64 ilp32.  It is based on 'official'
>> glibc support branch [1] (which is in turn based on Linux branch [2]).
>>
>> Main changes for libsanitizer is the kernel ABI support for internal
>> syscalls. Different than x32, ILP32 tries to leverage 32-bits syscalls
>> with kernel ABI for 64 bit argument passing (ftruncate for instance)
>> are passed using the new Linux generic way for 32 bits (by splitting
>> high and low in two registers).
>>
>> So instead of adding more adhoc defines to ILP32 this patch extends the
>> SANITIZER_USES_CANONICAL_LINUX_SYSCALLS to represent both 64 bits
>> argument syscalls (value of 1) and 32 bits (value of 2).
>>
>> The shadow offset used is similar to the arm one (29).
>>
>> I did a sanity check on aarch64-linux-gnu and x86_64-linux-gnu without
>> any regressions.
>>
>> PS: I sent this change llvm-commit, but it got stalled because llvm
>> lack aarch64 ilp32 support and noone could confirm no breakage due
>> missing buildbot.  Also the idea is not sync it back to libsanitizer.
> 
> It will be a nightmare for merges from upstream, but because the upstream is
> so uncooperative probably there is no other way.

What I meant it the idea is to sync it back to libsanitizer, sorry for the
misunderstanding. I can try to push it again for compiler-rt, but it took
a long way to actually get any reply.

> 
>> gcc/
>>
>> 	* gcc/config/aarch64/aarch64.c (aarch64_asan_shadow_offset): Add
> 
> No gcc/ prefixes in gcc/ChangeLog.

Right, fixed locally.

> 
>> 	TARGET_ILP32 support.
>>
>> libsanitizer/
> 
> Nor libsanitizer/ prefixes in libsaniter/ChangeLog.
>> --- a/libsanitizer/sanitizer_common/sanitizer_internal_defs.h
>> +++ b/libsanitizer/sanitizer_common/sanitizer_internal_defs.h
>> @@ -117,9 +117,9 @@ typedef signed   long long sptr;  // NOLINT
>>  typedef unsigned long uptr;  // NOLINT
>>  typedef signed   long sptr;  // NOLINT
>>  #endif  // defined(_WIN64)
>> -#if defined(__x86_64__)
>> -// Since x32 uses ILP32 data model in 64-bit hardware mode, we must use
>> -// 64-bit pointer to unwind stack frame.
>> +#if defined(__x86_64__) || SANITIZER_AARCH64_ILP32
>> +// Since x32 adn AArch64 ILP32 use ILP32 data model in 64-bit hardware mode, 
> 
> s/adn/and/

Ack.

> 
>> @@ -445,11 +467,7 @@ uptr internal_execve(const char *filename, char *const argv[],
>>  // ----------------- sanitizer_common.h
>>  bool FileExists(const char *filename) {
>>    struct stat st;
>> -#if SANITIZER_USES_CANONICAL_LINUX_SYSCALLS
>> -  if (internal_syscall(SYSCALL(newfstatat), AT_FDCWD, filename, &st, 0))
>> -#else
>>    if (internal_stat(filename, &st))
>> -#endif
>>      return false;
>>    // Sanity check: filename is a regular file.
>>    return S_ISREG(st.st_mode);
> 
> I'm uneasy about this hunk, you change behavior not just on aarch64 ilp32,
> but for many other targets too.  Please don't.

My understanding it a noop since for SANITIZER_USES_CANONICAL_LINUX_SYSCALLS
internal_stat would be a be a internal_syscall(newfstatat, ...) (also
the internal_* names are exactly to abstract this kind of constructions).

> 
> Do you really want it for GCC8?  We are in stage4 and this isn't a
> regression...
> 
> 	Jakub
> 

I don't have a strong opinion about that, it would to good to have on GCC8,
but I think I can wait.
diff mbox series

Patch

diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c
index 33c90ef..eebf85b 100644
--- a/gcc/config/aarch64/aarch64.c
+++ b/gcc/config/aarch64/aarch64.c
@@ -16171,7 +16171,10 @@  aarch64_split_dimode_const_store (rtx dst, rtx src)
 static unsigned HOST_WIDE_INT
 aarch64_asan_shadow_offset (void)
 {
-  return (HOST_WIDE_INT_1 << 36);
+  if (TARGET_ILP32)
+    return (HOST_WIDE_INT_1 << 29);
+  else
+    return (HOST_WIDE_INT_1 << 36);
 }
 
 static rtx
diff --git a/libsanitizer/sanitizer_common/sanitizer_internal_defs.h b/libsanitizer/sanitizer_common/sanitizer_internal_defs.h
index edd6a21..aef83c2 100644
--- a/libsanitizer/sanitizer_common/sanitizer_internal_defs.h
+++ b/libsanitizer/sanitizer_common/sanitizer_internal_defs.h
@@ -117,9 +117,9 @@  typedef signed   long long sptr;  // NOLINT
 typedef unsigned long uptr;  // NOLINT
 typedef signed   long sptr;  // NOLINT
 #endif  // defined(_WIN64)
-#if defined(__x86_64__)
-// Since x32 uses ILP32 data model in 64-bit hardware mode, we must use
-// 64-bit pointer to unwind stack frame.
+#if defined(__x86_64__) || SANITIZER_AARCH64_ILP32
+// Since x32 adn AArch64 ILP32 use ILP32 data model in 64-bit hardware mode, 
+// we must use 64-bit pointer to unwind stack frame.
 typedef unsigned long long uhwptr;  // NOLINT
 #else
 typedef uptr uhwptr;   // NOLINT
@@ -144,7 +144,7 @@  typedef int error_t;
 typedef int pid_t;
 
 #if SANITIZER_FREEBSD || SANITIZER_NETBSD || SANITIZER_MAC || \
-    (SANITIZER_LINUX && defined(__x86_64__))
+    (SANITIZER_LINUX && (defined(__x86_64__) || SANITIZER_AARCH64_ILP32))
 typedef u64 OFF_T;
 #else
 typedef uptr OFF_T;
@@ -154,8 +154,8 @@  typedef u64  OFF64_T;
 #if (SANITIZER_WORDSIZE == 64) || SANITIZER_MAC
 typedef uptr operator_new_size_type;
 #else
-# if defined(__s390__) && !defined(__s390x__)
-// Special case: 31-bit s390 has unsigned long as size_t.
+# if (defined(__s390__) && !defined(__s390x__)) || SANITIZER_AARCH64_ILP32
+// Special case: 31-bit s390 and AArch64 ILP32 have unsigned long as size_t.
 typedef unsigned long operator_new_size_type;
 # else
 typedef u32 operator_new_size_type;
diff --git a/libsanitizer/sanitizer_common/sanitizer_linux.cc b/libsanitizer/sanitizer_common/sanitizer_linux.cc
index 2826cc8..31f60ff 100644
--- a/libsanitizer/sanitizer_common/sanitizer_linux.cc
+++ b/libsanitizer/sanitizer_common/sanitizer_linux.cc
@@ -123,8 +123,16 @@  const int FUTEX_WAKE = 1;
 #if SANITIZER_LINUX && (defined(__x86_64__) || defined(__powerpc64__) || \
     SANITIZER_WORDSIZE == 64)
 # define SANITIZER_LINUX_USES_64BIT_SYSCALLS 1
+# define SYSCALL_LL64(val) (val)
 #else
 # define SANITIZER_LINUX_USES_64BIT_SYSCALLS 0
+# if __BYTE_ORDER == __LITTLE_ENDIAN
+#  define SYSCALL_LL64(val) \
+  (long) ((OFF64_T)(val) & 0xffffffff), (long) ((OFF64_T)(val) >> 32)
+# else
+#  define SYSCALL_LL64(val) \
+  (long) ((OFF64_T)val >> 32), (long) ((OFF64_T)(val) & 0xffffffff)
+# endif
 #endif
 
 #if defined(__x86_64__) || SANITIZER_MIPS64
@@ -227,8 +235,13 @@  uptr internal_ftruncate(fd_t fd, uptr size) {
 #if SANITIZER_NETBSD
   HANDLE_EINTR(res, internal_syscall(SYSCALL(ftruncate), fd, 0, (s64)size));
 #else
+# if SANITIZER_USES_CANONICAL_LINUX_SYSCALLS == 2
+  HANDLE_EINTR(res, (sptr)internal_syscall(SYSCALL(ftruncate64), fd,
+               SYSCALL_LL64((OFF_T)size)));
+# else
   HANDLE_EINTR(res, (sptr)internal_syscall(SYSCALL(ftruncate), fd,
                (OFF_T)size));
+# endif
 #endif
   return res;
 }
@@ -304,9 +317,12 @@  uptr internal_stat(const char *path, void *buf) {
 #if SANITIZER_FREEBSD || SANITIZER_NETBSD
   return internal_syscall(SYSCALL(fstatat), AT_FDCWD, (uptr)path,
                           (uptr)buf, 0);
-#elif SANITIZER_USES_CANONICAL_LINUX_SYSCALLS
+#elif SANITIZER_USES_CANONICAL_LINUX_SYSCALLS == 1
   return internal_syscall(SYSCALL(newfstatat), AT_FDCWD, (uptr)path,
                           (uptr)buf, 0);
+#elif SANITIZER_USES_CANONICAL_LINUX_SYSCALLS == 2
+  return internal_syscall(SYSCALL(fstatat64), AT_FDCWD, (uptr)path,
+                          (uptr)buf, 0);
 #elif SANITIZER_LINUX_USES_64BIT_SYSCALLS
 # if defined(__mips64)
   // For mips64, stat syscall fills buffer in the format of kernel_stat
@@ -331,9 +347,12 @@  uptr internal_lstat(const char *path, void *buf) {
 #elif SANITIZER_FREEBSD
   return internal_syscall(SYSCALL(fstatat), AT_FDCWD, (uptr)path,
                           (uptr)buf, AT_SYMLINK_NOFOLLOW);
-#elif SANITIZER_USES_CANONICAL_LINUX_SYSCALLS
+#elif SANITIZER_USES_CANONICAL_LINUX_SYSCALLS == 1
   return internal_syscall(SYSCALL(newfstatat), AT_FDCWD, (uptr)path,
                          (uptr)buf, AT_SYMLINK_NOFOLLOW);
+#elif SANITIZER_USES_CANONICAL_LINUX_SYSCALLS == 2
+  return internal_syscall(SYSCALL(fstatat64), AT_FDCWD, (uptr)path,
+                          (uptr)buf, AT_SYMLINK_NOFOLLOW);
 #elif SANITIZER_LINUX_USES_64BIT_SYSCALLS
 # if SANITIZER_MIPS64
   // For mips64, lstat syscall fills buffer in the format of kernel_stat
@@ -406,9 +425,12 @@  uptr internal_unlink(const char *path) {
 }
 
 uptr internal_rename(const char *oldpath, const char *newpath) {
-#if SANITIZER_USES_CANONICAL_LINUX_SYSCALLS
+#if SANITIZER_USES_CANONICAL_LINUX_SYSCALLS == 1
   return internal_syscall(SYSCALL(renameat), AT_FDCWD, (uptr)oldpath, AT_FDCWD,
                           (uptr)newpath);
+#elif SANITIZER_USES_CANONICAL_LINUX_SYSCALLS == 2
+  return internal_syscall(SYSCALL(renameat2), AT_FDCWD, (uptr)oldpath, AT_FDCWD,
+                          (uptr)newpath, 0);
 #else
   return internal_syscall(SYSCALL(rename), (uptr)oldpath, (uptr)newpath);
 #endif
@@ -445,11 +467,7 @@  uptr internal_execve(const char *filename, char *const argv[],
 // ----------------- sanitizer_common.h
 bool FileExists(const char *filename) {
   struct stat st;
-#if SANITIZER_USES_CANONICAL_LINUX_SYSCALLS
-  if (internal_syscall(SYSCALL(newfstatat), AT_FDCWD, filename, &st, 0))
-#else
   if (internal_stat(filename, &st))
-#endif
     return false;
   // Sanity check: filename is a regular file.
   return S_ISREG(st.st_mode);
@@ -729,7 +747,15 @@  uptr internal_lseek(fd_t fd, OFF_T offset, int whence) {
 #if SANITIZER_NETBSD
   return internal_syscall64(SYSCALL(lseek), fd, 0, offset, whence);
 #else
+# if SANITIZER_USES_CANONICAL_LINUX_SYSCALLS == 2
+  int64_t res;
+  int rc = internal_syscall(SYSCALL(llseek), fd,
+                                    (long) (((u64)(offset)) >> 32),
+                                    (long) offset, &res, whence);
+  return rc ? rc : res;
+# else
   return internal_syscall(SYSCALL(lseek), fd, offset, whence);
+# endif
 #endif
 }
 
diff --git a/libsanitizer/sanitizer_common/sanitizer_platform.h b/libsanitizer/sanitizer_common/sanitizer_platform.h
index 1eb4d0c..3963d09 100644
--- a/libsanitizer/sanitizer_common/sanitizer_platform.h
+++ b/libsanitizer/sanitizer_common/sanitizer_platform.h
@@ -179,6 +179,12 @@ 
 # define SANITIZER_ARM 0
 #endif
 
+#if defined(__aarch64__) && defined(__ILP32__)
+# define SANITIZER_AARCH64_ILP32 1
+#else
+# define SANITIZER_AARCH64_ILP32 0
+#endif
+
 // By default we allow to use SizeClassAllocator64 on 64-bit platform.
 // But in some cases (e.g. AArch64's 39-bit address space) SizeClassAllocator64
 // does not work well and we need to fallback to SizeClassAllocator32.
@@ -207,10 +213,15 @@ 
 
 // The AArch64 linux port uses the canonical syscall set as mandated by
 // the upstream linux community for all new ports. Other ports may still
-// use legacy syscalls.
+// use legacy syscalls.  Also AArch64 ILP32 uses canonical syscall set
+// 32 bits ports.
 #ifndef SANITIZER_USES_CANONICAL_LINUX_SYSCALLS
 # if defined(__aarch64__) && SANITIZER_LINUX
-# define SANITIZER_USES_CANONICAL_LINUX_SYSCALLS 1
+#  if defined(__ILP32__)
+#   define SANITIZER_USES_CANONICAL_LINUX_SYSCALLS 2
+#  else
+#   define SANITIZER_USES_CANONICAL_LINUX_SYSCALLS 1
+#  endif
 # else
 # define SANITIZER_USES_CANONICAL_LINUX_SYSCALLS 0
 # endif
diff --git a/libsanitizer/sanitizer_common/sanitizer_platform_limits_posix.cc b/libsanitizer/sanitizer_common/sanitizer_platform_limits_posix.cc
index 858bb21..ee4f74f 100644
--- a/libsanitizer/sanitizer_common/sanitizer_platform_limits_posix.cc
+++ b/libsanitizer/sanitizer_common/sanitizer_platform_limits_posix.cc
@@ -1136,7 +1136,8 @@  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)
+#if !defined(__aarch64__) || !SANITIZER_LINUX || __GLIBC_PREREQ (2, 21) \
+    && !defined(__ILP32__)
 /* On aarch64 glibc 2.20 and earlier provided incorrect mode field.  */
 CHECK_SIZE_AND_OFFSET(ipc_perm, mode);
 #endif
diff --git a/libsanitizer/sanitizer_common/sanitizer_platform_limits_posix.h b/libsanitizer/sanitizer_common/sanitizer_platform_limits_posix.h
index 4d11d07..fbdf3d5 100644
--- a/libsanitizer/sanitizer_common/sanitizer_platform_limits_posix.h
+++ b/libsanitizer/sanitizer_common/sanitizer_platform_limits_posix.h
@@ -74,7 +74,10 @@  namespace __sanitizer {
 #elif defined(__arm__)
   const unsigned struct_kernel_stat_sz = 64;
   const unsigned struct_kernel_stat64_sz = 104;
-#elif defined(__aarch64__)
+#elif defined(__aarch64__) && defined(__ILP32__)
+  const unsigned struct_kernel_stat_sz = 80;
+  const unsigned struct_kernel_stat64_sz = 104;
+#elif defined(__aarch64__) && defined(__LP64__)
   const unsigned struct_kernel_stat_sz = 128;
   const unsigned struct_kernel_stat64_sz = 104;
 #elif defined(__powerpc__) && !defined(__powerpc64__)
@@ -506,8 +509,13 @@  namespace __sanitizer {
   };
 #else
   struct __sanitizer_dirent {
+# if SANITIZER_AARCH64_ILP32
+    unsigned long long d_ino;
+    unsigned long long d_off;
+# else
     uptr d_ino;
     uptr d_off;
+# endif
     unsigned short d_reclen;
     // more fields that we don't care about
   };
@@ -537,7 +545,8 @@  namespace __sanitizer {
 
 #if SANITIZER_LINUX || SANITIZER_FREEBSD
 #if defined(_LP64) || defined(__x86_64__) || defined(__powerpc__)\
-                   || defined(__mips__)
+                   || defined(__mips__) \
+                   || (defined(__aarch64__) && defined(__ILP32__))
   typedef unsigned __sanitizer___kernel_uid_t;
   typedef unsigned __sanitizer___kernel_gid_t;
 #else