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

[Fangrui Song via Phabricator via llvm-commits]

MaskRay 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__)
----------------
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.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72367