Page MenuHomePhabricator

[lsan] [mips] adding support of lsan for mips64/mips64el arch
AbandonedPublic

Authored by sdkie on Oct 4 2014, 12:42 AM.

Details

Summary

Mostly changes are specific to MIPS architecture to support leak sanitizer

Diff Detail

Repository
rL LLVM

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
sdkie updated this revision to Diff 14910.Oct 14 2014, 11:59 PM
sdkie set the repository for this revision to rL LLVM.

made changes for 64-bit MIPS architectures

sdkie added inline comments.Oct 15 2014, 1:38 AM
lib/sanitizer_common/sanitizer_linux.cc
906

@earthdok: After the basic support for mips64 is merged, I will start working on implementing clone in assembly.
Btw using syscall(clone,..) is not a good option for this?

sdkie added a comment.Oct 15 2014, 6:03 AM

@earthdok: whether we should check for 'mips64' instead of 'mips' as we are anyways not supporting 32-bit mips for LSAN?

sdkie updated this revision to Diff 15167.Oct 20 2014, 11:50 PM
sdkie set the repository for this revision to rL LLVM.

minor change in CanBeAHeapPointer, the virtual addressable bits for mips are 40 and not 39

Sorry, I haven't had the time to review this yet. I'll take a look this week.

earthdok added a comment.EditedOct 24 2014, 1:41 PM

How did you test this patch?

In D5616#23, @kumarsukhani wrote:

@earthdok: whether we should check for 'mips64' instead of 'mips' as we are anyways not supporting 32-bit mips for LSAN?

That would be more consistent, yes (we do check for __x86_64__).
Is __mips64__ the right symbol? __mips64 seems to be more prevalent, and https://code.google.com/p/android/issues/detail?id=75904 suggests __mips64__ may not be defined when building with gcc.

lib/lsan/lsan_allocator.cc
28

(SANITIZER_WORDSIZE == 64) is unnecessary, please remove it.

lib/lsan/lsan_common.cc
147

please remove (SANITIZER_WORDSIZE == 64)

lib/sanitizer_common/sanitizer_linux.cc
906

Please leave a TODO, then.

Btw using syscall(clone,..) is not a good option for this?

It wouldn't work at all. We'd crash when returning from syscall() in the child process, because we're now in the beginning of a new stack and there isn't a previous stack frame to return to.

lib/sanitizer_common/sanitizer_linux_libcdep.cc
257

Why do you need this? Just write to descr_addr.

263

const uptr kTlsTcbOffset
const uptr kTlsPreTcbSize

303

Is this correct? I thought in variant 2 the static TLS was located before the TCB?

sdkie updated this revision to Diff 15518.Oct 28 2014, 2:37 AM
sdkie set the repository for this revision to rL LLVM.

minor fixes as per comments of earthdok

sdkie added a comment.Oct 28 2014, 2:50 AM

How did you test this patch?

I just ran the testcases of lsan on mips rpe-100 board, and majority of LeakSanitizer-Standalone test cases are working.

Is this correct? I thought in variant 2 the static TLS was located before the TCB?

Sorry, it was a typing mistake. MIPS uses variant 1

sdkie updated this revision to Diff 15521.Oct 28 2014, 5:26 AM

changed 'AppendPointer' function to print 40 bits for mips64 instead of 48 bits

samsonov added inline comments.
lib/lsan/lsan_common.cc
147

typo: shouldn't this be __mips64?

lib/sanitizer_common/sanitizer_printf.cc
118 ↗(On Diff #15521)

Please introduce a named constant for 10/12 instead with a comment.

In D5616#32, @kumarsukhani wrote:

How did you test this patch?

I just ran the testcases of lsan on mips rpe-100 board, and majority of LeakSanitizer-Standalone test cases are working.

Which tests are failing and why? And what about LeakSanitizer-AddressSanitizer?

(I imagine at least use_tls_pthread_specific_static.cc would be failing because of the incorrect value of kTlsPreTcbSize.)

lib/sanitizer_common/sanitizer_linux_libcdep.cc
257

Comment indentation should match code indentation.

259

This comment doesn't give the complete picture. How about:

"MIPS uses TLS variant I. The thread pointer (in hardware register $29) points to the end of the TCB + 0x7000. The pthread_descr structure is immediately in front of the TCB. kTlsPreTcbSize includes the size of the TCB and the size of pthread_descr."

262

This can't be correct. How did you obtain this constant?
In glibc sources we have:

95 # define TLS_PRE_TCB_SIZE \
96 (sizeof (struct pthread) \
97 + ((sizeof (tcbhead_t) + TLS_TCB_ALIGN - 1) & ~(TLS_TCB_ALIGN - 1)))

tcbhead_t alone is 16 bytes. struct pthread is much bigger.

This also needs a test, like the one we have for kThreadDescriptorSize. The size of struct pthread will change between glibc versions.

304

I don't think this gives the correct TLS size. Your kTlsPreTcbSize above is too small, so *addr as you're currently computing it is too big. So if *addr is incorrect, *size can't be correct - otherwise we would be accessing past the end of the TLS and LSan would crash.

Probably you need something like

*size = GetTlsSize() + kTlsPreTcbSize;

or

*size = GetTlsSize() + kTlsPreTcbSize + kTlsTcbOffset;

I can't say what exactly without digging into glibc sources. Could you please do that?

sdkie added a comment.Oct 29 2014, 2:14 AM

Which tests are failing and why?

Following test cases are failing:

  1. suppressions_default.cc
  2. suppressions_file.cc
  3. swapcontext.cc
  4. use_tls_pthread_specific_dynamic.cc
  5. use_tls_pthread_specific_static.cc

And what about LeakSanitizer-AddressSanitizer?

asan is not yet supported for mips64, I am simultaneously working on it.
I will submit its patch soon.

I can't say what exactly without digging into glibc sources. Could you please do that?

Thanks for your comments. I will try to fix the addr and size of TLS.
Actually, I have very limited knowledge of TLS, So I will be really thankful if anyone is ready to help me.

sdkie updated this revision to Diff 15920.Nov 7 2014, 6:00 AM
  • updated TLS addr and size
  • introduced SANITIZER_POINTER_FORMAT_LENGTH variable
sdkie edited the test plan for this revision. (Show Details)Nov 7 2014, 6:06 AM
sdkie planned changes to this revision.Nov 11 2014, 10:12 PM

@kcc @earthdok: any comments on current revision ?
I will soon update this revision to change SanitizerAllocator64 to SanitizerAllocator32.

sdkie updated this revision to Diff 16141.Nov 13 2014, 2:39 AM
sdkie edited the test plan for this revision. (Show Details)
sdkie set the repository for this revision to rL LLVM.

using SanitizerAllocator32 instead of SanitizerAllocator64 for mips64 as its address space is just 40-bit

sdkie edited the test plan for this revision. (Show Details)Nov 13 2014, 2:52 AM
earthdok added inline comments.Nov 13 2014, 12:46 PM
lib/sanitizer_common/sanitizer_linux.cc
900–907

Where did clone go?

lib/sanitizer_common/sanitizer_linux_libcdep.cc
206

please move this #ifdef into the if block at line 201:

#if defined(__x86_64__) || defined(__i386__)
if (..)
val = ...
else if (...)
val = ...
#elif defined(__mips__)
val = ...
#endif

208

How did you obtain those numbers?

We need some kind of test for those, similar to SanitizerLinux.ThreadDescriptorSize (but taking into account the actual TLS layout on MIPS).

257

Please remove the blank lines here and below.

261

s/"//

263

Only this constant is part of the ABI. kTcbHead and kTlsTcbAlign could theoretically change, so we need to test them as well.

Instead of ThreadDescriptorSize(), we probably want to expose TlsPreTcbSize(). But please figure out what the test will look like before making this change.

266

This naming style is used for constants. s/kThreadPointer/thread_pointer/ and please move this closer to the asm block.

268

const

lib/sanitizer_common/sanitizer_printf.cc
116 ↗(On Diff #15920)

Accidental change?

sdkie updated this revision to Diff 16202.Nov 14 2014, 2:35 AM

minor fixes as per comment of earthdok

sdkie added inline comments.Nov 14 2014, 2:51 AM
lib/sanitizer_common/sanitizer_linux.cc
900–907

It was a mistake

lib/sanitizer_common/sanitizer_linux_libcdep.cc
208

I obtained .i file by compiling GLIBC 2.19-13 (same which is available on my board), and calculated those values

263

can we use test of minor number, the way x86_64 has done ?

lib/sanitizer_common/sanitizer_printf.cc
116 ↗(On Diff #15920)

no that change is already merged in http://reviews.llvm.org/D6024 patch

earthdok added inline comments.Nov 14 2014, 7:54 AM
lib/sanitizer_common/sanitizer_linux_libcdep.cc
263

That's how you guess the size. But you still need a testcase to check that your guess is correct. Take a look at lib/sanitizer_common/tests/sanitizer_linux_test.cc, specifically TEST(SanitizerLinux, ThreadDescriptorSize).

Also, please take note of test/asan/lit.cfg and test/asan/Unit/lit.site.cfg.in. Those configs enable leak detection in ASan tests on x86_64. Please make sure that is the case for MIPS as well.

sdkie added a comment.Nov 18 2014, 3:19 AM

Also, please take note of test/asan/lit.cfg and test/asan/Unit/lit.site.cfg.in. Those configs enable leak detection in ASan tests on x86_64. Please make sure that is the case for MIPS as well.

Thanks, I will submit lsan testing patch separately once this patch is accepted.

lib/sanitizer_common/sanitizer_linux_libcdep.cc
263

how to run this tests on x86_64 ?
I think so this tests doesn't execute when I run

make check-all
earthdok added inline comments.Nov 18 2014, 6:30 AM
lib/sanitizer_common/sanitizer_linux_libcdep.cc
263

"check-all" includes this testcase. You can verify by adding an invalid assertion to the test - check-all will fail.

The smallest subset that includes this test case is "check-sanitizer"

Note that the test case itself is under #ifdef x86_64/i386

sdkie added a comment.Nov 19 2014, 5:28 AM

I doubt whether its really working.
Even after doing following change 'make check-lsan' is working fine on x86_64

TEST(SanitizerLinux, ThreadDescriptorSize) {
   pthread_t tid;
   void *result;
-  ASSERT_EQ(0, pthread_create(&tid, 0, thread_descriptor_size_test_func, 0));
-  ASSERT_EQ(0, pthread_join(tid, &result));
+  ASSERT_EQ(1, pthread_create(&tid, 0, thread_descriptor_size_test_func, 0));
+  ASSERT_EQ(1, pthread_join(tid, &result));
   EXPECT_EQ((uptr)result, ThreadDescriptorSize());
 }
In D5616#57, @kumarsukhani wrote:

I doubt whether its really working.
Even after doing following change 'make check-lsan' is working fine on x86_64

check-lsan doesn't run this test. check-sanitizer does.

sdkie added a comment.Nov 20 2014, 4:29 AM

@earthdok: with check-sanitizer also I see no failures because of above changes in assertion.

Can you please commit this current version of patch ? if no more changes are require
I will update lib/sanitizer_common/tests/sanitizer_linux_test.cc file so that same check works for mips.
What do you think ?

sdkie added a comment.Nov 20 2014, 5:00 AM

And compiler-rt master build is not clean, I am getting following failures on x86_64:

Failing Tests (14):

  AddressSanitizer32 :: TestCases/Linux/interface_symbols_linux.c
  AddressSanitizer32 :: TestCases/default_blacklist.cc
  AddressSanitizer32 :: TestCases/lsan_annotations.cc
  AddressSanitizer64 :: TestCases/Linux/interface_symbols_linux.c
  AddressSanitizer64 :: TestCases/default_blacklist.cc
  DataFlowSanitizer :: basic.c
  DataFlowSanitizer :: custom.cc
  DataFlowSanitizer :: dump_labels.c
  DataFlowSanitizer :: fncall.c
  DataFlowSanitizer :: label_count.c
  DataFlowSanitizer :: propagate.c
  DataFlowSanitizer :: vararg.c
  DataFlowSanitizer :: write_callback.c
  MemorySanitizer :: default_blacklist.cc

Expected Passes    : 708
Expected Failures  : 15
Unsupported Tests  : 194
Unexpected Failures: 14

Something must be wrong with your checkout. For me ninja check-sanitizer starts failing when I reverse the assertions in the test.

In D5616#59, @kumarsukhani wrote:

@earthdok: with check-sanitizer also I see no failures because of above changes in assertion.

Can you please commit this current version of patch ? if no more changes are require
I will update lib/sanitizer_common/tests/sanitizer_linux_test.cc file so that same check works for mips.
What do you think ?

I'm sorry, I'm not comfortable committing without this test. This has caused far too many issues in the past.

By the way, I looked at the Glibc sources and it turns out your TLS calculation is incorrect even if ThreadDescriptorSize() is correct. Consider _dl_allocate_tls_storage():

332 #if TLS_DTV_AT_TP
333   /* Memory layout is:
334      [ TLS_PRE_TCB_SIZE ] [ TLS_TCB_SIZE ] [ TLS blocks ]
335                           ^ This should be returned.  */
336   size += (TLS_PRE_TCB_SIZE + GL(dl_tls_static_align) - 1)
337           & ~(GL(dl_tls_static_align) - 1);
338 #endif

The size we get from _dl_get_tls_static_info is GL(dl_tls_static_align). This means that the upper TLS boundary as you compute it is off by TLS_PRE_TCB_SIZE.

We have very few tests for this stuff so please make sure you understand thoroughly how TLS is laid out - don't just rely on tests passing.

Something must be wrong with your checkout. For me ninja check-sanitizer starts failing when I reverse the assertions in the test.

I am actually running make check-sanitizer. I am not sure what is the difference between them.
Today I first build clang/llvm from master, and then did testing using compiler-rt master branch.

Thanks for you comment, it will surely help me to fix TLS calculation.

In D5616#62, @kumarsukhani wrote:

Something must be wrong with your checkout. For me ninja check-sanitizer starts failing when I reverse the assertions in the test.

I am actually running make check-sanitizer. I am not sure what is the difference between them.

Hmm. What commands do you use to configure and build?

Today I first build clang/llvm from master, and then did testing using compiler-rt master branch.

Thanks for you comment, it will surely help me to fix TLS calculation.

Please actually look at the source and don't rely on my comments, I may have missed something.

sdkie added a comment.Nov 20 2014, 9:55 PM

What commands do you use to configure and build?

CC=clang CXX=clang++  cmake ../compiler-rt -DLLVM_CONFIG_PATH=/home/kms/LLVM_x86/install/bin/llvm-config  -DCOMPILER_RT_INSTALL_PATH=/home/kms/LLVM_x86/install/lib/clang/3.6.0
make install
make check-all

Clang path : /home/kms/LLVM_x86/install/bin/clang
make check-sanitizer output: https://gist.github.com/SDkie/6d2013b72ea67d77d094

In D5616#64, @kumarsukhani wrote:

Yeah, this doesn't look right. In your log it runs only the LIT tests from compiler-rt/test/sanitizer_common/. But there are many more unit tests under compiler-rt/lib/sanitizer_common/test/ which check-sanitizer is also supposed to run.

samsonov@, do you have any idea about this?

sdkie added a comment.Nov 21 2014, 9:52 AM

Yeah, this doesn't look right. In your log it runs only the LIT tests from compiler-rt/test/sanitizer_common/.

Yes, make check-* runs only LIT tests

Right, you'd have to enable sanitizer_common unit tests by baking support for mips64 to lib/sanitizer_common/tests/CMakeLists.txt.

In D5616#67, @samsonov wrote:

Right, you'd have to enable sanitizer_common unit tests by baking support for mips64 to lib/sanitizer_common/tests/CMakeLists.txt.

We're talking about x86_64.

sdkie added a comment.Nov 22 2014, 8:52 AM

@earthdok: can you share your configure and build commands/scripts ?
So that I can get those test cases running.

I'm actually doing a bootstrap build so it's a bit complicated. But it shouldn't matter, something like this should work just as well (assuming you have g++ 4.8 installed):

cmake ../llvm/ -G Ninja -DCMAKE_BUILD_TYPE=Release -DLLVM_ENABLE_ASSERTIONS=True
ninja check-sanitizer

Which commands are you using?

sdkie added a comment.Nov 24 2014, 9:29 AM

I am using following commands:

CC=clang CXX=clang++  cmake ../compiler-rt -DLLVM_CONFIG_PATH=/home/kms/LLVM_x86/install/bin/llvm-config  -DCOMPILER_RT_INSTALL_PATH=/home/kms/LLVM_x86/install/lib/clang/3.6.0
make
make install
make check-sanitizer

Ah, I see. You're doing a standalone compiler-rt build, whereas I'm building from the LLVM tree. Apparently there's a bug in the test configuration for standalone builds. Alexey will take a look at it today - you can either wait for him to fix it, or checkout LLVM and do an in-tree build.

If you're running a standalone compiler-rt build, you'd also have to pass -DCOMPILER_RT_INCLUDE_TESTS=ON to enable unit tests.

sdkie updated this revision to Diff 17032.Dec 8 2014, 1:23 AM

updated TLS part after studying Glibc code

sdkie added a comment.Dec 8 2014, 2:55 AM

@earthdok: I am not sure if mips stores the value of pthread struct size anywhere the way x86 does to write a UNIT test for pthread struct size.
Btw why x86_64 is using the hard codes values, even though they have direct access to pthread structure size?

In D5616#76, @kumarsukhani wrote:

@earthdok: I am not sure if mips stores the value of pthread struct size anywhere the way x86 does to write a UNIT test for pthread struct size.
Btw why x86_64 is using the hard codes values, even though they have direct access to pthread structure size?

The test that we have for x86_64 doesn't read the size from the struct. Rather, it relies on assumptions regarding how the TCB is positioned relative to the beginning of the stack. We could use that method to actually determine the TCB size at runtime. However: 1) it requires spawning a thread, which is pretty awkward and intrusive to user code and 2) we would have no way of knowing if those assumptions no longer held true.

If the size was stored in the struct itself, that would certainly make things easier, but to the best of my knowledge it isn't.

lib/sanitizer_common/sanitizer_linux_libcdep.cc
200

No need to introduce a separate constant, just return this.

267

Let's drop "get", to be consistent with ThreadSelfOffset(), ThreadDescriptorSize().

334

There's already a "|| defined(mips)" above, so you will never hit this branch.

335

const uptr kDlTlsStaticAlign

336

Why not do this calculation once in InitTlsSize().

339

You're breaking compilation on every arch other than x86-64 and MIPS. Why?

sdkie updated this revision to Diff 17424.Dec 18 2014, 1:33 AM
sdkie set the repository for this revision to rL LLVM.

updated patch as per suggestions of earthdok

sdkie added a comment.Dec 22 2014, 5:54 AM

@earthdok: did you got time to look at my new patch?
Actually I am going on a long holiday after couple of day, @mohit.bhakkad and @sagar will continue my work.

Sorry about the long wait. I had a long vacation as well.

The changes in the last patch look ok. However, as we discussed, this CL still needs a test before I can accept it.

lib/sanitizer_common/sanitizer_linux_libcdep.cc
170

whitespace after "-", indent

Hi @earthdok,

As per my understanding of the TLS memory layout and the TLS part required for sanitizers, we need to get the range of static TLS area in GetTls ().

X86_64 uses tls variant 2 and TLS_TCB_AT_TP is set to 1 in tls.h. So the size we get from __dl_tls_get_static_info () includes the thread descriptor size along with size of static TLS area as per the following code in elf/dl-tls.c:

223  GL(dl_tls_static_used) = offset;
224  GL(dl_tls_static_size) = (roundup (offset + TLS_STATIC_SURPLUS, max_align)
225			    + TLS_TCB_SIZE);

Therefore we need the Thread Descriptor Size, so that we can exclude it from the size we get from __dl_tls_get_static_info ().

But MIPS uses TLS variant 1 and TLS_DTV_AT_TP is set to 1 in tls.h. So the size we get from __dl_tls_get_static_info () is only the size of static TLS area and does not include the thread descriptor size as per the following code in elf/dl-tls.c:

267  GL(dl_tls_static_used) = offset;
268  GL(dl_tls_static_size) = roundup (offset + TLS_STATIC_SURPLUS,
269				    TLS_TCB_ALIGN);

Also when we read from hardware register $29 and subtract 0x7000 from it we reach to the start of static TLS (which is also end of TCB for TLS variant 1).
Therefore we already have the start address and size of static TLS area so we don't require TlsPreTcbSize () and ThreadDescriptorSize () at all for MIPS.

Documents refered :

[1] http://www.akkadia.org/drepper/tls.pdf
[2] http://www.linux-mips.org/wiki/NPTL

Hi!

The point of confusion here is whether the TCB is considered a part of TLS or not. From LSan's point of view, we have to include the TCB in the rootset. One reason is because thread-specific storage (i.e. pointers stored using pthread_setspecific()) is only reachable through a static array which is a part of the TCB structure. So LSan includes the TCB into the TLS.

In D5616#109055, @sagar wrote:

X86_64 uses tls variant 2 and TLS_TCB_AT_TP is set to 1 in tls.h. So the size we get from __dl_tls_get_static_info () includes the thread descriptor size along with size of static TLS area as per the following code in elf/dl-tls.c:

223  GL(dl_tls_static_used) = offset;
224  GL(dl_tls_static_size) = (roundup (offset + TLS_STATIC_SURPLUS, max_align)
225			    + TLS_TCB_SIZE);

Therefore we need the Thread Descriptor Size, so that we can exclude it from the size we get from __dl_tls_get_static_info ().

This is not strictly true. The size we get is correct; it's the offset that is shifted by ThreadDescriptorSize. Take a look at the code in sanitizer_linux_libcdep.cc:

264 # if defined(x86_64) || defined(i386)
265 *addr = ThreadSelf();
266 *size = GetTlsSize();
267 *addr -= *size;
268 *addr += ThreadDescriptorSize();

The size we get is the TLS size, including the TCB. We do not adjust it. But ThreadSelf() points at the beginning of the TCB, which is located at the very end of the TLS.

This is of course not relevant to MIPS, just making sure you have a clear picture.

But MIPS uses TLS variant 1 and TLS_DTV_AT_TP is set to 1 in tls.h. So the size we get from __dl_tls_get_static_info () is only the size of static TLS area and does not include the thread descriptor size as per the following code in elf/dl-tls.c:

267  GL(dl_tls_static_used) = offset;
268  GL(dl_tls_static_size) = roundup (offset + TLS_STATIC_SURPLUS,
269				    TLS_TCB_ALIGN);

Also when we read from hardware register $29 and subtract 0x7000 from it we reach to the start of static TLS (which is also end of TCB for TLS variant 1).
Therefore we already have the start address and size of static TLS area so we don't require TlsPreTcbSize () and ThreadDescriptorSize () at all for MIPS.

As discussed above, we still need to adjust the address to point to the beginning of the TCB.

slthakur added a comment.EditedJan 16 2015, 6:06 AM

Hi @earthdok,

I have created a new patch http://reviews.llvm.org/D7013.
http://reviews.llvm.org/D7013 includes the changes in this patch and the test case required for it.

sdkie abandoned this revision.Jan 31 2015, 1:24 AM

please check http://reviews.llvm.org/D7013 for further updates on this patch