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

[Evgenii Stepanov via Phabricator via llvm-commits]

eugenis 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__)
----------------
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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72367