[Lsan] Use fp registers to search for pointers

[vitalybuka]

Summary

X86 can use xmm registers for pointers operations. e.g. for std::swap.
I don't know yet if it's possible on other platforms.

NT_X86_XSTATE includes all registers from NT_FPREGSET so
the latter used only if the former is not available. I am not sure how
reasonable to expect that but LLD has such fallback in
NativeRegisterContextLinux_x86_64::ReadFPR.

[morehouse]

LGTM once we have a test.

compiler-rt/lib/sanitizer_common/sanitizer_stoptheworld_linux_libcdep.cpp

488

Please add comments here explaining why we need these extra registers.

[vitalybuka]

test

[vitalybuka]

restore sched_yield()

compiler-rt/test/lsan/TestCases/use_registers.cpp

18 ↗

(On Diff #292381)

I moved this to avoid call after we stored pointer into reg
It's not very important for general registers, but important for the new test as xmm more likely is going to be corrupted.

[vitalybuka]

FYI @eugenis @oontvoo
Maybe we need this for Android as well

[vitalybuka]

restore VReport for PTRACE_GETREGS

[vitalybuka]

convert into lambda to avoid guarding unused function with ARCH_IOVEC_FOR_GETREGSET

[morehouse]

compiler-rt/lib/sanitizer_common/sanitizer_stoptheworld_linux_libcdep.cpp

571

constexpr

574

For 64 *bit* alignment, we want 8 here I think.

compiler-rt/test/lsan/TestCases/use_registers_extra.cpp

30 ↗

(On Diff #292384)

Are the extra moves necessary? Can we move p directly into xmm0?

[vitalybuka]

comments

compiler-rt/test/lsan/TestCases/use_registers_extra.cpp

30 ↗

(On Diff #292384)

movd %0, %%xmm0
here and in original test as well use temporarily register
for original test it does not matter as we test any general register can hold pointer
here we want to reset that scratch register

[vitalybuka]

compiler-rt/test/lsan/TestCases/use_registers_extra.cpp

30 ↗

(On Diff #292384)

movd %0, %%xmm0
here and in original test as well use temporarily register

for movd %0, %%xmm0 compiler will still use another register

[vitalybuka]

simplify asm

[oontvoo]

In D87754#2278243, @vitalybuka wrote:

FYI @eugenis @oontvoo
Maybe we need this for Android as well

Do you mean we'll need to handle the aarch64 case?
Otherwise what's being done here should just work?
Or is there some edge-case that needs to be done specifically for Android?

(Btw, this use_registers.cpp test is currently failing on Android-AArch64 ... it couldn't find the register for some reason ... )

[vitalybuka]

In D87754#2278416, @oontvoo wrote:

In D87754#2278243, @vitalybuka wrote:

FYI @eugenis @oontvoo
Maybe we need this for Android as well

Do you mean we'll need to handle the aarch64 case?

Yes.

Otherwise what's being done here should just work?

That's I don't know. I know that x86 use this trick. Maybe for aarch64 does not need this.

Or is there some edge-case that needs to be done specifically for Android?

This can be Android specific, but I have no evidence of that.

(Btw, this use_registers.cpp test is currently failing on Android-AArch64 ... it couldn't find the register for some reason ... )

Maybe this is a prove that not all registers fetched.
How exactly this fails?
You can try move print_address as that function call may write register.
I guess easiest thing to do is to gdb and see if test register is OK.

[oontvoo]

In D87754#2278447, @vitalybuka wrote:

Maybe this is a prove that not all registers fetched.
How exactly this fails?

I've changed use_registers.cpp to store the allocated 1337 bytes in x9 register for aarch64. (Picked x9 because it's used to store local variables, hence probably unlikely to be re-used.)

#elif defined(__aarch64__)
  asm ( "mov x9, %0"
      :
      : "r" (p)
      );

Even when you run it with use_registers=1, it cannot find the chunk in the register range.
(haven't had much success with gdb remotely to the phone, so I've been doing logging :-).

Test alloc: 0x004fd7120080 
==8466==Scanning GLOBAL range 0x00788fc0a000-0x00788fc10628.
==8466==Scanning GLOBAL range 0x00788fc11640-0x00788fc1ddb8.
==8466==Scanning GLOBAL range 0x005cd7126c60-0x005cd7126fe0.
==8466==Scanning GLOBAL range 0x005cd7127fe0-0x005cd71281b0.
==8466==Scanning GLOBAL range 0x00788674afe0-0x00788674dae0.
==8466==Scanning GLOBAL range 0x00788674eae0-0x0078868a2658.
==8466==Scanning GLOBAL range 0x0078868a2920-0x00788692d408.
==8466==Scanning GLOBAL range 0x00788e758750-0x00788e762818.
==8466==0x00788e760e40: found 0x003dd7120030 pointing into chunk 0x003dd7120030-0x003dd7120038 of size 8.
==8466==Scanning GLOBAL range 0x00788abf8000-0x00788abf82c8.
==8466==Scanning GLOBAL range 0x00788abf92d0-0x00788abf9360.
==8466==Scanning GLOBAL range 0x00788e7e6000-0x00788e7e6180.
==8466==Scanning GLOBAL range 0x00788e7e8000-0x00788e7e9000.
==8466==Scanning GLOBAL range 0x00788a703000-0x00788a706a90.
==8466==Scanning GLOBAL range 0x00788a707aa0-0x00788ab69608.
==8466==0x00788a709558: found 0x003ed7120010 pointing into chunk 0x003ed7120010-0x003ed7120040 of size 48.
==8466==Scanning GLOBAL range 0x00788e7b6000-0x00788e7b6578.
==8466==Scanning GLOBAL range 0x00788e7b7578-0x00788e7b76e4.
==8466==Scanning GLOBAL range 0x007885f69000-0x007885f6f290.
==8466==Scanning GLOBAL range 0x007885f70290-0x007885f733a0.
==8466==0x007885f72298: found 0x003dd7120010 pointing into chunk 0x003dd7120010-0x003dd7120018 of size 8.
==8466==Scanning GLOBAL range 0x007886650000-0x0078866503b0.
==8466==Scanning GLOBAL range 0x0078866513b0-0x007886651404.
==8466==Scanning REGISTERS range 0x00788e86b000-0x00788e86b110.    <<<<<<<<<<<<<<<<<<<<< not found here
==8466==Scanning TLS range 0x00788e9baff8-0x00788e9be000.
==8466==Scanning REGISTERS range 0x00788e86b000-0x00788e86b110.
==8466==Scanning TLS range 0x0078831fbff8-0x0078831ff000.
==8466==Scanning HEAP range 0x003dd7120010-0x003dd7120018.
==8466==Scanning HEAP range 0x003ed7120010-0x003ed7120040.
==8466==Scanning HEAP range 0x003dd7120030-0x003dd7120038.
==8466==Processing platform-specific allocations.
==8466==Scanning leaked chunks.
==8466==Scanning HEAP range 0x004fd7120080-0x004fd71205b9.

So two theories:

• I'm wrong about x9 being unused - it got overwritten with something else
• (like you said), not all registers, including x9 did not get collected ...

(Debugging this is probably derailing from your patch a bit - so happy to move to emails or start a thread in D85927 if you want)

[morehouse]

LGTM

[fhahn]

It appears that after this change, use_registers.cpp is failing on green dragon: http://green.lab.llvm.org/green/job/clang-stage1-RA/14618/consoleFull#-1417328700a1ca8a51-895e-46c6-af87-ce24fa4cd561

It would be great if you could take a look or revert if there is no quick fix.

[fhahn]

Output of failed test:

******************* TEST 'LeakSanitizer-AddressSanitizer-x86_64 :: TestCases/use_registers_extra.cpp' FAILED ********************
Script:
--
: 'RUN: at line 2';   LSAN_BASE="report_objects=1:use_stacks=0"
: 'RUN: at line 3';      /Users/buildslave/jenkins/workspace/clang-stage1-RA/clang-build/./bin/clang  --driver-mode=g++ -O0  -arch x86_64 -stdlib=libc++ -mmacosx-version-min=10.9 -isysroot /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.15.sdk  -gline-tables-only -fsanitize=address -I/Users/buildslave/jenkins/workspace/clang-stage1-RA/llvm-project/compiler-rt/test/lsan/../ -pthread /Users/buildslave/jenkins/workspace/clang-stage1-RA/llvm-project/compiler-rt/test/lsan/TestCases/use_registers_extra.cpp -o /Users/buildslave/jenkins/workspace/clang-stage1-RA/clang-build/tools/clang/runtime/compiler-rt-bins/test/lsan/X86_64AsanConfig/TestCases/Output/use_registers_extra.cpp.tmp
: 'RUN: at line 4';   env LSAN_OPTIONS=detect_leaks=1:abort_on_error=0:log_to_syslog=0:$LSAN_BASE:"use_registers=0" not  /Users/buildslave/jenkins/workspace/clang-stage1-RA/clang-build/tools/clang/runtime/compiler-rt-bins/test/lsan/X86_64AsanConfig/TestCases/Output/use_registers_extra.cpp.tmp 2>&1 | FileCheck /Users/buildslave/jenkins/workspace/clang-stage1-RA/llvm-project/compiler-rt/test/lsan/TestCases/use_registers_extra.cpp
: 'RUN: at line 5';   env LSAN_OPTIONS=detect_leaks=1:abort_on_error=0:log_to_syslog=0:$LSAN_BASE:"use_registers=1"  /Users/buildslave/jenkins/workspace/clang-stage1-RA/clang-build/tools/clang/runtime/compiler-rt-bins/test/lsan/X86_64AsanConfig/TestCases/Output/use_registers_extra.cpp.tmp 2>&1
: 'RUN: at line 6';   env LSAN_OPTIONS=detect_leaks=1:abort_on_error=0:log_to_syslog=0:""  /Users/buildslave/jenkins/workspace/clang-stage1-RA/clang-build/tools/clang/runtime/compiler-rt-bins/test/lsan/X86_64AsanConfig/TestCases/Output/use_registers_extra.cpp.tmp 2>&1
--
Exit Code: 1
Hide Details


Command Output (stdout):
--
Test alloc: 0x61a000000080 
Hide Details


=================================================================
==40682==ERROR: LeakSanitizer: detected memory leaks
Hide Details


Direct leak of 1337 byte(s) in 1 object(s) allocated from:
    #0 0x10444f960 in wrap_malloc sanitizer_malloc_mac.inc:140
    #1 0x1044036ed in registers_thread_func use_registers_extra.cpp:20
    #2 0x7fff69bc5108 in _pthread_start+0x93 (libsystem_pthread.dylib:x86_64+0x6108)
    #3 0x7fff69bc0b8a in thread_start+0xe (libsystem_pthread.dylib:x86_64+0x1b8a)
Hide Details


Objects leaked above:
0x61a000000080 (1337 bytes)
Hide Details


SUMMARY: AddressSanitizer: 1337 byte(s) leaked in 1 allocation(s).

[vitalybuka]

Thanks! Should be fixed now 034781f7f33634918025206427e6ee912ef3985b

In D87754#2281695, @fhahn wrote:

Output of failed test:

******************* TEST 'LeakSanitizer-AddressSanitizer-x86_64 :: TestCases/use_registers_extra.cpp' FAILED ********************
Script:
--
: 'RUN: at line 2';   LSAN_BASE="report_objects=1:use_stacks=0"
: 'RUN: at line 3';      /Users/buildslave/jenkins/workspace/clang-stage1-RA/clang-build/./bin/clang  --driver-mode=g++ -O0  -arch x86_64 -stdlib=libc++ -mmacosx-version-min=10.9 -isysroot /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.15.sdk  -gline-tables-only -fsanitize=address -I/Users/buildslave/jenkins/workspace/clang-stage1-RA/llvm-project/compiler-rt/test/lsan/../ -pthread /Users/buildslave/jenkins/workspace/clang-stage1-RA/llvm-project/compiler-rt/test/lsan/TestCases/use_registers_extra.cpp -o /Users/buildslave/jenkins/workspace/clang-stage1-RA/clang-build/tools/clang/runtime/compiler-rt-bins/test/lsan/X86_64AsanConfig/TestCases/Output/use_registers_extra.cpp.tmp
: 'RUN: at line 4';   env LSAN_OPTIONS=detect_leaks=1:abort_on_error=0:log_to_syslog=0:$LSAN_BASE:"use_registers=0" not  /Users/buildslave/jenkins/workspace/clang-stage1-RA/clang-build/tools/clang/runtime/compiler-rt-bins/test/lsan/X86_64AsanConfig/TestCases/Output/use_registers_extra.cpp.tmp 2>&1 | FileCheck /Users/buildslave/jenkins/workspace/clang-stage1-RA/llvm-project/compiler-rt/test/lsan/TestCases/use_registers_extra.cpp
: 'RUN: at line 5';   env LSAN_OPTIONS=detect_leaks=1:abort_on_error=0:log_to_syslog=0:$LSAN_BASE:"use_registers=1"  /Users/buildslave/jenkins/workspace/clang-stage1-RA/clang-build/tools/clang/runtime/compiler-rt-bins/test/lsan/X86_64AsanConfig/TestCases/Output/use_registers_extra.cpp.tmp 2>&1
: 'RUN: at line 6';   env LSAN_OPTIONS=detect_leaks=1:abort_on_error=0:log_to_syslog=0:""  /Users/buildslave/jenkins/workspace/clang-stage1-RA/clang-build/tools/clang/runtime/compiler-rt-bins/test/lsan/X86_64AsanConfig/TestCases/Output/use_registers_extra.cpp.tmp 2>&1
--
Exit Code: 1
Hide Details


Command Output (stdout):
--
Test alloc: 0x61a000000080 
Hide Details


=================================================================
==40682==ERROR: LeakSanitizer: detected memory leaks
Hide Details


Direct leak of 1337 byte(s) in 1 object(s) allocated from:
    #0 0x10444f960 in wrap_malloc sanitizer_malloc_mac.inc:140
    #1 0x1044036ed in registers_thread_func use_registers_extra.cpp:20
    #2 0x7fff69bc5108 in _pthread_start+0x93 (libsystem_pthread.dylib:x86_64+0x6108)
    #3 0x7fff69bc0b8a in thread_start+0xe (libsystem_pthread.dylib:x86_64+0x1b8a)
Hide Details


Objects leaked above:
0x61a000000080 (1337 bytes)
Hide Details


SUMMARY: AddressSanitizer: 1337 byte(s) leaked in 1 allocation(s).

In D87754#2281695, @fhahn wrote:

Output of failed test:

******************* TEST 'LeakSanitizer-AddressSanitizer-x86_64 :: TestCases/use_registers_extra.cpp' FAILED ********************
Script:
--
: 'RUN: at line 2';   LSAN_BASE="report_objects=1:use_stacks=0"
: 'RUN: at line 3';      /Users/buildslave/jenkins/workspace/clang-stage1-RA/clang-build/./bin/clang  --driver-mode=g++ -O0  -arch x86_64 -stdlib=libc++ -mmacosx-version-min=10.9 -isysroot /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.15.sdk  -gline-tables-only -fsanitize=address -I/Users/buildslave/jenkins/workspace/clang-stage1-RA/llvm-project/compiler-rt/test/lsan/../ -pthread /Users/buildslave/jenkins/workspace/clang-stage1-RA/llvm-project/compiler-rt/test/lsan/TestCases/use_registers_extra.cpp -o /Users/buildslave/jenkins/workspace/clang-stage1-RA/clang-build/tools/clang/runtime/compiler-rt-bins/test/lsan/X86_64AsanConfig/TestCases/Output/use_registers_extra.cpp.tmp
: 'RUN: at line 4';   env LSAN_OPTIONS=detect_leaks=1:abort_on_error=0:log_to_syslog=0:$LSAN_BASE:"use_registers=0" not  /Users/buildslave/jenkins/workspace/clang-stage1-RA/clang-build/tools/clang/runtime/compiler-rt-bins/test/lsan/X86_64AsanConfig/TestCases/Output/use_registers_extra.cpp.tmp 2>&1 | FileCheck /Users/buildslave/jenkins/workspace/clang-stage1-RA/llvm-project/compiler-rt/test/lsan/TestCases/use_registers_extra.cpp
: 'RUN: at line 5';   env LSAN_OPTIONS=detect_leaks=1:abort_on_error=0:log_to_syslog=0:$LSAN_BASE:"use_registers=1"  /Users/buildslave/jenkins/workspace/clang-stage1-RA/clang-build/tools/clang/runtime/compiler-rt-bins/test/lsan/X86_64AsanConfig/TestCases/Output/use_registers_extra.cpp.tmp 2>&1
: 'RUN: at line 6';   env LSAN_OPTIONS=detect_leaks=1:abort_on_error=0:log_to_syslog=0:""  /Users/buildslave/jenkins/workspace/clang-stage1-RA/clang-build/tools/clang/runtime/compiler-rt-bins/test/lsan/X86_64AsanConfig/TestCases/Output/use_registers_extra.cpp.tmp 2>&1
--
Exit Code: 1
Hide Details


Command Output (stdout):
--
Test alloc: 0x61a000000080 
Hide Details


=================================================================
==40682==ERROR: LeakSanitizer: detected memory leaks
Hide Details


Direct leak of 1337 byte(s) in 1 object(s) allocated from:
    #0 0x10444f960 in wrap_malloc sanitizer_malloc_mac.inc:140
    #1 0x1044036ed in registers_thread_func use_registers_extra.cpp:20
    #2 0x7fff69bc5108 in _pthread_start+0x93 (libsystem_pthread.dylib:x86_64+0x6108)
    #3 0x7fff69bc0b8a in thread_start+0xe (libsystem_pthread.dylib:x86_64+0x1b8a)
Hide Details


Objects leaked above:
0x61a000000080 (1337 bytes)
Hide Details


SUMMARY: AddressSanitizer: 1337 byte(s) leaked in 1 allocation(s).

[jeroen.dobbelaere]

compiler-rt/lib/sanitizer_common/sanitizer_stoptheworld_linux_libcdep.cpp

490

This triggers a build error on a RHES 6.6 version which seems not to have 'PTRACE_GETREGSET'.

Would putting this between:

#if defined(PTRACE_GETREGSET)
# define ARCH.....
static ...
#endif

make sense ?

[morehouse]

compiler-rt/lib/sanitizer_common/sanitizer_stoptheworld_linux_libcdep.cpp

490

Seems reasonable.

[vitalybuka]

compiler-rt/lib/sanitizer_common/sanitizer_stoptheworld_linux_libcdep.cpp

490

I guess in this will break use_registers_extra.cpp
Also users code also can report false leaks.

It's better then build error, but I am not sure if we can do more for old kernels.

Are you going to create patch?

[jeroen.dobbelaere]

compiler-rt/lib/sanitizer_common/sanitizer_stoptheworld_linux_libcdep.cpp

490

I have created D91686 for this.

compiler-rt/lib/sanitizer_common/sanitizer_stoptheworld_linux_libcdep.cpp

 #elif SANITIZER_LINUX && defined(__arm__)
 typedef user_regs regs_struct;
 #define REG_SP uregs[13]
 
 #elif defined(__i386__) || defined(__x86_64__)
 typedef user_regs_struct regs_struct;
 #if defined(__i386__)
 #define REG_SP esp
+static constexpr uptr kExtraRegs[] = {NT_X86_XSTATE, NT_PRFPXREG};
 #else
 #define REG_SP rsp
+static constexpr uptr kExtraRegs[] = {NT_X86_XSTATE, NT_FPREGSET};
 #endif
+#define ARCH_IOVEC_FOR_GETREGSET
 
 #elif defined(__powerpc__) || defined(__powerpc64__)
 typedef pt_regs regs_struct;
 #define REG_SP gpr[PT_R1]
 
 #elif defined(__mips__)
 typedef struct user regs_struct;
 # if SANITIZER_ANDROID
 #  define REG_SP regs[EF_R29]
 # else
 #  define REG_SP regs[EF_REG29]
 # endif
 
 #elif defined(__aarch64__)
 typedef struct user_pt_regs regs_struct;
 #define REG_SP sp
+static constexpr uptr kExtraRegs[] = {};
 #define ARCH_IOVEC_FOR_GETREGSET
 
 #elif defined(__s390__)
 typedef _user_regs_struct regs_struct;
 #define REG_SP gprs[15]
+static constexpr uptr kExtraRegs[] = {};
 #define ARCH_IOVEC_FOR_GETREGSET
 
 #else
 #error "Unsupported architecture"
 #endif // SANITIZER_ANDROID && defined(__arm__)
 
 tid_t SuspendedThreadsListLinux::GetThreadID(uptr index) const {
   CHECK_LT(index, thread_ids_.size());
bool SuspendedThreadsListLinux::ContainsTid(tid_t thread_id) const {
   }
   return false;
 }
 
 void SuspendedThreadsListLinux::Append(tid_t tid) {
   thread_ids_.push_back(tid);
 }
 
+static bool AppendRegisters(pid_t tid, uptr regset,
+                            InternalMmapVector<uptr> *buffer, int *pterrno) {
+  struct iovec regset_io;
+  uptr size = buffer->size();
+  for (;; buffer->resize(buffer->capacity() * 2)) {
+    buffer->resize(buffer->capacity());
+    regset_io.iov_base = buffer->data() + size;
+    regset_io.iov_len = (buffer->size() - size) * sizeof(uptr);
+    bool isErr =
+        internal_iserror(internal_ptrace(PTRACE_GETREGSET, tid, (void *)regset,
+                                         (void *)&regset_io),
+                         pterrno);
+    if (isErr) {
+      buffer->resize(size);
+      return false;
+    }
+
+    // Far enough from the buffer size, no need to resize and repeat.
+    if (regset_io.iov_len + 64 < buffer->size() * sizeof(uptr))
+      break;
+  }
+  buffer->resize(RoundUpTo(regset_io.iov_len, sizeof(uptr)) / sizeof(uptr));
+  return true;
+}
+
 PtraceRegistersStatus SuspendedThreadsListLinux::GetRegistersAndSP(
     uptr index, InternalMmapVector<uptr> *buffer, uptr *sp) const {
   pid_t tid = GetThreadID(index);
-  regs_struct regs;
   int pterrno;
 #ifdef ARCH_IOVEC_FOR_GETREGSET
-  struct iovec regset_io;
-  regset_io.iov_base = &regs;
-  regset_io.iov_len = sizeof(regs_struct);
-  bool isErr = internal_iserror(internal_ptrace(PTRACE_GETREGSET, tid,
-                                (void*)NT_PRSTATUS, (void*)&regset_io),
-                                &pterrno);
+  buffer->clear();
+  buffer->reserve(Max<uptr>(1024, buffer->capacity()));
+  bool isErr = !AppendRegisters(tid, NT_PRSTATUS, buffer, &pterrno);
+  if (!isErr) {
+    // Accept the first available and do not report errors.
+    for (uptr regs : kExtraRegs)
+      if (AppendRegisters(tid, regs, buffer, &pterrno))
+        break;
+  }
 #else
-  bool isErr = internal_iserror(internal_ptrace(PTRACE_GETREGS, tid, nullptr,
-                                &regs), &pterrno);
+  buffer->resize(RoundUpTo(sizeof(regs_struct), sizeof(uptr)) / sizeof(uptr));
+  bool isErr = internal_iserror(
+      internal_ptrace(PTRACE_GETREGS, tid, nullptr, buffer->data()), &pterrno);
 #endif
   if (isErr) {
-    VReport(1, "Could not get registers from thread %d (errno %d).\n", tid,
-            pterrno);
+    Report("Could not get registers from thread %d (errno %d).\n", tid,
     // ESRCH means that the given thread is not suspended or already dead.
     // Therefore it's unsafe to inspect its data (e.g. walk through stack) and
     // we should notify caller about this.
     return pterrno == ESRCH ? REGISTERS_UNAVAILABLE_FATAL
                             : REGISTERS_UNAVAILABLE;
   }
 
-  *sp = regs.REG_SP;
-  buffer->resize(RoundUpTo(sizeof(regs), sizeof(uptr)) / sizeof(uptr));
-  internal_memcpy(buffer->data(), &regs, sizeof(regs));
+  *sp = reinterpret_cast<regs_struct *>(buffer->data())[0].REG_SP;
   return REGISTERS_AVAILABLE;
 }
 
 } // namespace __sanitizer
 
 #endif  // SANITIZER_LINUX && (defined(__x86_64__) || defined(__mips__)
         // || defined(__aarch64__) || defined(__powerpc64__)
         // || defined(__s390__) || defined(__i386__) || defined(__arm__)