Page MenuHomePhabricator

[Lsan] Use fp registers to search for pointers
ClosedPublic

Authored by vitalybuka on Sep 16 2020, 3:30 AM.

Details

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.

Diff Detail

Event Timeline

vitalybuka created this revision.Sep 16 2020, 3:30 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 16 2020, 3:30 AM
Herald added a subscriber: Restricted Project. · View Herald Transcript
vitalybuka requested review of this revision.Sep 16 2020, 3:30 AM

LGTM once we have a test.

compiler-rt/lib/sanitizer_common/sanitizer_stoptheworld_linux_libcdep.cpp
487

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

vitalybuka edited the summary of this revision. (Show Details)Sep 16 2020, 5:42 PM
vitalybuka marked an inline comment as done.

restore sched_yield()

compiler-rt/test/lsan/TestCases/use_registers.cpp
18–21

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 edited the summary of this revision. (Show Details)Sep 16 2020, 5:52 PM
vitalybuka added subscribers: oontvoo, eugenis.

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

vitalybuka edited the summary of this revision. (Show Details)

restore VReport for PTRACE_GETREGS

convert into lambda to avoid guarding unused function with ARCH_IOVEC_FOR_GETREGSET

morehouse added inline comments.Sep 16 2020, 6:13 PM
compiler-rt/lib/sanitizer_common/sanitizer_stoptheworld_linux_libcdep.cpp
547

constexpr

550

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

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

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

vitalybuka marked 2 inline comments as done.

comments

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

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 added inline comments.Sep 16 2020, 6:40 PM
compiler-rt/test/lsan/TestCases/use_registers_extra.cpp
30

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

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

simplify asm

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

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.

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)

This revision is now accepted and ready to land.Sep 17 2020, 9:10 AM
This revision was landed with ongoing or failed builds.Sep 17 2020, 12:16 PM
This revision was automatically updated to reflect the committed changes.
fhahn added a subscriber: fhahn.Sep 18 2020, 6:38 AM

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 added a comment.Sep 18 2020, 6:42 AM

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

Thanks! Should be fixed now 034781f7f33634918025206427e6ee912ef3985b

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

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 added inline comments.
compiler-rt/lib/sanitizer_common/sanitizer_stoptheworld_linux_libcdep.cpp
488

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 added inline comments.Mon, Oct 12, 8:26 AM
compiler-rt/lib/sanitizer_common/sanitizer_stoptheworld_linux_libcdep.cpp
488

Seems reasonable.

vitalybuka added inline comments.Wed, Oct 14, 8:14 PM
compiler-rt/lib/sanitizer_common/sanitizer_stoptheworld_linux_libcdep.cpp
488

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?