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

[vitalybuka]

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]

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]

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]

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]

compiler-rt/lib/sanitizer_common/symbolizer/sanitizer_wrappers.cpp

21–22

SGTM

[yuanzi]

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

[MaskRay]

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]

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]

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]

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?

[eugenis]

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 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.

[yuanzi]

Tested that it works internally. https://critique.corp.google.com/#review/288921325

[eugenis]

Please update all the other implementations of internal_mmap

[yuanzi]

update all the other implementations of internal_mmap

[yuanzi]

updated, PTAL!

[eugenis]

LGTM

[MaskRay]

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

compiler-rt/lib/sanitizer_common/sanitizer_linux.cpp

 #else
 #include "sanitizer_syscall_generic.inc"
 #endif
 
 // --------------- sanitizer_libc.h
 #if !SANITIZER_SOLARIS && !SANITIZER_NETBSD
 #if !SANITIZER_S390 && !SANITIZER_OPENBSD
 uptr internal_mmap(void *addr, uptr length, int prot, int flags, int fd,
-                   OFF_T offset) {
+                   u64 offset) {
 #if SANITIZER_FREEBSD || SANITIZER_LINUX_USES_64BIT_SYSCALLS
   return internal_syscall(SYSCALL(mmap), (uptr)addr, length, prot, flags, fd,
                           offset);
 #else
   // mmap2 specifies file offset in 4096-byte units.
   CHECK(IsAligned(offset, 4096));
   return internal_syscall(SYSCALL(mmap2), addr, length, prot, flags, fd,
                           offset / 4096);

compiler-rt/lib/sanitizer_common/sanitizer_linux_s390.cpp

 #include <sys/syscall.h>
 #include <sys/utsname.h>
 #include <unistd.h>
 
 namespace __sanitizer {
 
 // --------------- sanitizer_libc.h
 uptr internal_mmap(void *addr, uptr length, int prot, int flags, int fd,
-                   OFF_T offset) {
+                   u64 offset) {
   struct s390_mmap_params {
     unsigned long addr;
     unsigned long length;
     unsigned long prot;
     unsigned long flags;
     unsigned long fd;
     unsigned long offset;
   } params = {

compiler-rt/lib/sanitizer_common/sanitizer_netbsd.cpp

#define DEFINE__REAL(ret_type, func, ...)                              \
   static ret_type (*real_##func)(__VA_ARGS__) = NULL;                  \
   if (!real_##func) {                                                  \
     real_##func = (ret_type(*)(__VA_ARGS__))GetRealLibcAddress(#func); \
   }                                                                    \
   CHECK(real_##func);
 
 // --------------- sanitizer_libc.h
 uptr internal_mmap(void *addr, uptr length, int prot, int flags, int fd,
-                   OFF_T offset) {
+                   u64 offset) {
   CHECK(&__mmap);
   return (uptr)__mmap(addr, length, prot, flags, fd, 0, offset);
 }
 
 uptr internal_munmap(void *addr, uptr length) {
   DEFINE__REAL(int, munmap, void *a, uptr b);
   return _REAL(munmap, addr, length);
 }

compiler-rt/lib/sanitizer_common/sanitizer_posix.h

 uptr internal_open(const char *filename, int flags, u32 mode);
 uptr internal_close(fd_t fd);
 
 uptr internal_read(fd_t fd, void *buf, uptr count);
 uptr internal_write(fd_t fd, const void *buf, uptr count);
 
 // Memory
 uptr internal_mmap(void *addr, uptr length, int prot, int flags,
-                   int fd, OFF_T offset);
+                   int fd, u64 offset);
 uptr internal_munmap(void *addr, uptr length);
 int internal_mprotect(void *addr, uptr length, int prot);
 
 // OS
 uptr internal_filesize(fd_t fd);  // -1 on error.
 uptr internal_stat(const char *path, void *buf);
 uptr internal_lstat(const char *path, void *buf);
 uptr internal_fstat(fd_t fd, void *buf);

compiler-rt/lib/sanitizer_common/symbolizer/sanitizer_wrappers.cpp

 #include <dlfcn.h>
 #include <errno.h>
 #include <fcntl.h>
 #include <stdarg.h>
 #include <stdio.h>
 #include <unistd.h>
 
 #include <tuple>
 
-// Need to match ../sanitizer_common/sanitizer_internal_defs.h
-#if defined(ARCH_PPC)
-#define OFF_T unsigned long
-#else
-#define OFF_T unsigned long long
-#endif
-
 namespace __sanitizer {
 unsigned long internal_open(const char *filename, int flags);
 unsigned long internal_open(const char *filename, int flags, unsigned mode);
 unsigned long internal_close(int fd);
 unsigned long internal_stat(const char *path, void *buf);
 unsigned long internal_lstat(const char *path, void *buf);
 unsigned long internal_fstat(int fd, void *buf);
 size_t internal_strlen(const char *s);
 unsigned long internal_mmap(void *addr, unsigned long length, int prot,
-                            int flags, int fd, OFF_T offset);
+                            int flags, int fd, unsigned long long offset);
 void *internal_memcpy(void *dest, const void *src, unsigned long n);
 // Used to propagate errno.
 bool internal_iserror(unsigned long retval, int *rverrno = 0);
 }  // namespace __sanitizer
 
 namespace {
 
 template <typename T>