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.
