Page MenuHomePhabricator

Summary: update macro for OFF_T so that sanitizer works on AARCH64.
ClosedPublic

Authored by yuanzi on Jan 7 2020, 3:13 PM.

Diff Detail

Event Timeline

yuanzi created this revision.Jan 7 2020, 3:13 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptJan 7 2020, 3:13 PM
Herald added subscribers: llvm-commits, Restricted Project, kristof.beyls. · View Herald Transcript

Sanitizers should work for aarch64, we have build bot for that
http://lab.llvm.org:8011/builders/sanitizer-x86_64-linux-android

compiler-rt/lib/sanitizer_common/symbolizer/sanitizer_wrappers.cpp
21–22

Do we need to change sanitizer_internal_defs.h

eugenis added inline comments.Jan 7 2020, 4:33 PM
compiler-rt/lib/sanitizer_common/symbolizer/sanitizer_wrappers.cpp
21–22

If anything, we need to change this file.
But I don't think it can break aarch64, because unsigned long is same width as unsigned long long there.

This change only affects 32-bit PowerPC.
Should it say !defined(x86_64) instead? That would be closer to how OFF_T is defined in sanitizer_internal_defs.h.

scw added inline comments.Jan 7 2020, 4:51 PM
compiler-rt/lib/sanitizer_common/symbolizer/sanitizer_wrappers.cpp
21–22

!defined(x86_64) is reasonable. Though the exact condition in sanitizer_internal_defs.h is complicated: https://github.com/llvm/llvm-project/blob/5e2f4dc37b1bf72bd27e929a68fec18ae1f5cfa8/compiler-rt/lib/sanitizer_common/sanitizer_internal_defs.h#L175 plus https://github.com/llvm/llvm-project/blob/5e2f4dc37b1bf72bd27e929a68fec18ae1f5cfa8/compiler-rt/lib/sanitizer_common/sanitizer_internal_defs.h#L134 for uptr.

This change actually should only affect powerpc64 -- ARCH_PPC is an google internally defined macro so this is always false in the wild.

yuanzi marked an inline comment as done.Jan 7 2020, 4:59 PM
yuanzi added inline comments.
compiler-rt/lib/sanitizer_common/symbolizer/sanitizer_wrappers.cpp
21–22

That makes sense! The other branch of the definition for "OFF_T" in sanitizer_internal_defs.h is the following:
typedef uptr OFF_T;

And uptr is defined to be "unsigned long" for non-64-bit Windows:

#if defined(_WIN64) 
// 64-bit Windows uses LLP64 data model.
typedef unsigned long long uptr;
typedef signed long long sptr;
#else
typedef unsigned long uptr;
typedef signed long sptr;
#endif  // defined(_WIN64)

So how about updating to "!defined(x86_64) && !defined(_WIN64)"?

Verified that this is working for Diorite imc/acc. Are there other tests that you think would be necessary to run?

eugenis added inline comments.Jan 7 2020, 6:11 PM
compiler-rt/lib/sanitizer_common/symbolizer/sanitizer_wrappers.cpp
21–22

SGTM

yuanzi updated this revision to Diff 236738.Jan 7 2020, 6:20 PM

update
update the definition for OFF_T to align with sanitizer_internal_defs.h

MaskRay added inline comments.
compiler-rt/lib/sanitizer_common/symbolizer/sanitizer_wrappers.cpp
21–22

Why are there !defined(x86_64) or ARCH_PPC special cases?

ELF platforms can exclusively use {,unsigned} long.

(signed can be omitted.)

scw added inline comments.Jan 8 2020, 11:35 AM
compiler-rt/lib/sanitizer_common/symbolizer/sanitizer_wrappers.cpp
21–22

According to the original commit https://github.com/llvm/llvm-project/commit/f0b8f989e93fa4a07a320c7cf54ac859bf9c164e

WARNING: OFF_T may be different from OS type off_t, depending on the value of
_FILE_OFFSET_BITS. This definition of OFF_T matches the ABI of system calls
like pread and mmap, as opposed to pread64 and mmap64.
Mac and Linux/x86-64 are special.

MaskRay added inline comments.Jan 8 2020, 11:44 AM
compiler-rt/lib/sanitizer_common/symbolizer/sanitizer_wrappers.cpp
21–22

There are two different OFF_T definitions.

In sanitizer_wrapper.cpp, it is expected to be an 64-bit integer on ELF32 (regardless of _FILE_OFFSET_BITS).

In sanitizer_internal_defs.h, it is expected to be an 32-bit integer on ELF32 (regardless of _FILE_OFFSET_BITS).

We probably should use OFF64_T for internal_mmap and ideally unify the two OFF_T definitions. OFF64_T is because we want to make 32-bit -D_FILE_OFFSET_BITS=64 platforms work.

yuanzi added inline comments.Jan 9 2020, 11:18 AM
compiler-rt/lib/sanitizer_common/symbolizer/sanitizer_wrappers.cpp
21–22

Looks we now have two options for fixing the bug.

@vitalybuka @eugenis Do you have a preference on how to proceed?

OFF_T magic is about matching libc.so ABI in the interceptors.
internal_mmap does not have to deal with it, and it can simply use uptr for the offset argument, unless I'm missing something.

yuanzi updated this revision to Diff 237401.Jan 10 2020, 11:35 AM

updated definition in sanitizer_linux.cpp to use u64 as the type for offset and updated internal_mmap in sanitizer_wrappers.cpp to use "unsigned long long" directly.

Please update all the other implementations of internal_mmap

yuanzi updated this revision to Diff 237424.Jan 10 2020, 1:09 PM

update all the other implementations of internal_mmap

yuanzi marked 9 inline comments as done.Jan 10 2020, 1:10 PM

updated, PTAL!

eugenis accepted this revision.Jan 10 2020, 1:12 PM

LGTM

This revision is now accepted and ready to land.Jan 10 2020, 1:12 PM
MaskRay accepted this revision.Jan 10 2020, 1:17 PM

u64 is unconditionally typedef unsigned long long u64; so using unsigned long long in sanitizer_wrappers.cpp should be fine.

This revision was automatically updated to reflect the committed changes.