This is an archive of the discontinued LLVM Phabricator instance.

[LSAN] Fix test swapcontext.cc on MIPS
ClosedPublic

Authored by slthakur on Apr 1 2016, 4:22 AM.

Details

Summary

There is no frame validity check in the slow unwinder like there is in the fast unwinder due to which lsan reports a leak even for heap allocated coroutine in the test swapcontext.cc. Since mips/linux uses slow unwindwer instead of fast unwinder, the test fails for mips/linux. Therefore adding the checks before unwinding fixes the test for mips/linux.

Diff Detail

Repository
rL LLVM

Event Timeline

slthakur updated this revision to Diff 52346.Apr 1 2016, 4:22 AM
slthakur retitled this revision from to [LSAN] Fix test swapcontext.cc on MIPS.
slthakur updated this object.
slthakur added reviewers: samsonov, kcc, earthdok.
slthakur set the repository for this revision to rL LLVM.

Hi @kcc, @samsonov, @earthdok,

Could you all please review this patch?

Gentle reminder!

kcc edited edge metadata.Apr 14 2016, 1:09 PM

Is a test (that works on Linux x86_64) possible here?

lib/lsan/lsan.h
23

Why did you remove this check (fast &&)?

Is a test (that works on Linux x86_64) possible here?

The test swapcontext.cc itself serves as a test for invalid frame on Linux x86_64 too. The difference between x86_64 and mips in case of unwinding is that x86 uses fast unwinder which checks if the frame is valid before unwinding and mips uses slow unwinder which does not check if the frame is valid before unwinding.

lib/lsan/lsan.h
23

Before this patch frame validity check was there only for fast unwinder. So stack top and bottom were fetched only in case of fast unwinder as per my understanding. Now we need it for both unwinders irrespective of whether we use the fast or slow unwinder. Thats the reason I removed this check. Is there any other significance to this check?

kcc added a comment.Apr 15 2016, 9:22 AM

Is a test (that works on Linux x86_64) possible here?

The test swapcontext.cc itself serves as a test for invalid frame on Linux x86_64 too. The difference between x86_64 and mips in case of unwinding is that x86 uses fast unwinder which checks if the frame is valid before unwinding and mips uses slow unwinder which does not check if the frame is valid before unwinding.

I am not sure I understand this.
If the existing test is enough for Linux x86_64 then why we would need this patch?
I want a test that will fail with the current code and get fixed with your patch (on Linux x86_64). Is such test possible?

lib/lsan/lsan.h
23

I don't know.
This code is extremely fragile and extremely hard to test,
so any changes in this code are scary, unless you can properly explain them and test them.

Hi,

The test swapconext.cc fails only for MIPS64 and this patch is intended to fix the test swapcontext.cc only for Linux/MIPS64. This patch is not intended to fix any test/bug on Linux x86_64. The test swapcontext.cc was failing only on MIPS64 because for unwinding, MIPS uses the slow unwinder. And given an invalid frame (as given in test swapcontext.cc) the slow unwinder does not detect the invalid frame and continues to unwind even though the frame is invalid which results in a false positive report.

I have tested this patch on Linux x86_64 also. It does not cause any regressions.

kcc accepted this revision.Apr 18 2016, 11:33 AM
kcc edited edge metadata.

LGTM

This revision is now accepted and ready to land.Apr 18 2016, 11:33 AM
slthakur edited edge metadata.Apr 18 2016, 11:08 PM
slthakur added a project: Restricted Project.
slthakur added a subscriber: tstellarAMD.
slthakur closed this revision.Apr 18 2016, 11:13 PM

Committed in revision 266716.

@kcc, @tstellarAMD: Is it okay to merge this patch to 3.8.1 branch?

kcc added a subscriber: kcc.Apr 19 2016, 4:17 PM

I have no objection as long as all the tests pass

pcc added a subscriber: pcc.Apr 20 2016, 10:41 AM

I suspect that this change has caused test failures on ARM:
http://lab.llvm.org:8011/builders/clang-cmake-thumbv7-a15-full-sh/builds/3417

Sagar, can you please take a look and revert if the fix is non-trivial?

Hi Peter,

I am unable to reproduce this failure on ARMv7 Processor.
Here is the output I got for all optimization levels (O0, O1, O2, O3):

root@debian-armhf:~# arm-linux-gnueabihf-clang/bin/clang -g -O3 -fsanitize=address null_deref.cc 
root@debian-armhf:~# ./a.out 
ASAN:DEADLYSIGNAL
=================================================================
==856==ERROR: AddressSanitizer: SEGV on unknown address 0x00000028 (pc 0x000c8fa8 bp 0x7ed69bc0 sp 0x7ed69bc0 T0)
==856==The signal is caused by a READ memory access.
==856==Hint: address points to the zero page.
    #0 0xc8fa7 in NullDeref(int*) /root/null_deref.cc:15:10
    #1 0xc8f7b in main /root/null_deref.cc:21:3
    #2 0x76d8b631 in __libc_start_main (/lib/arm-linux-gnueabihf/libc.so.6+0x17631)
AddressSanitizer can not provide additional info.
SUMMARY: AddressSanitizer: SEGV /root/null_deref.cc:15:10 in NullDeref(int*)
==856==ABORTING

The build bot failure is:

error: expected string not found in input // CHECK: {{ #0 0x.* in .*NullDeref.*null_deref.cc:}}[[@LINE-3]]

This CL was reverted due to a regression in arm build bot. We were thinking of putting this code under #if defined(mips) and then recommitting it so that it does not affect the arm build bot. Is this fine with everyone?

slthakur updated this revision to Diff 56255.May 5 2016, 3:19 AM

Putting this code under #if defined(mips).