Re: [PATCH] Add sanitizer support for AArch64 ILP32
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, , 0))
>> -#else
>>if (internal_stat(filename, ))
>> -#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.
Re: [PATCH] Add sanitizer support for AArch64 ILP32
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, , 0))
> -#else
>if (internal_stat(filename, ))
> -#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
[PATCH] Add sanitizer support for AArch64 ILP32
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(-)
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
