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

[Lirong Yuan via Phabricator via llvm-commits]

yuanzi marked an inline comment as done.
yuanzi 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__)
----------------
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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72367