[PATCH] D72367: Summary: update macro for OFF_T so that sanitizer works on AARCH64.

[Shu-Chun Weng via Phabricator via llvm-commits]

scw added inline comments.


================
Comment at: compiler-rt/lib/sanitizer_common/symbolizer/sanitizer_wrappers.cpp:22
 
 // Need to match ../sanitizer_common/sanitizer_internal_defs.h
+#if defined(__powerpc64__) || defined(__aarch64__)
----------------
MaskRay wrote:
> eugenis wrote:
> > yuanzi wrote:
> > > scw wrote:
> > > > eugenis wrote:
> > > > > vitalybuka wrote:
> > > > > > Do we need to change sanitizer_internal_defs.h
> > > > > 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.
> > > > > 
> > > > `!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.
> > > 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?
> > SGTM
> Why are there `!defined(x86_64)` or `ARCH_PPC` special cases?
> 
> ELF platforms can exclusively use `{,unsigned} long`.
> 
> (`signed` can be omitted.)
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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D72367/new/

https://reviews.llvm.org/D72367