Page MenuHomePhabricator

[LSan] [MIPS] adding support of LSan for mips64/mips64el arch
ClosedPublic

Authored by slthakur on Jan 16 2015, 6:02 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

slthakur updated this revision to Diff 18298.Jan 16 2015, 6:02 AM
slthakur retitled this revision from to [LSan] [MIPS] adding support of LSan for mips64/mips64el arch.
slthakur updated this object.
slthakur edited the test plan for this revision. (Show Details)
slthakur added reviewers: petarj, earthdok, kcc.
slthakur set the repository for this revision to rL LLVM.
slthakur added subscribers: samsonov, dsanders, mohit.bhakkad and 2 others.
kcc added inline comments.Jan 16 2015, 10:28 AM
lib/sanitizer_common/sanitizer_linux_libcdep.cc
166

Please add a comment and/or a more descriptive name.

191

Can we avoid an ifdef here?

193

can you pull this logic inside TlsPreTcbSize?

earthdok added inline comments.Jan 16 2015, 1:12 PM
lib/lsan/lsan_allocator.cc
41

indent

lib/lsan/lsan_common.h
25

indent to match previous line

lib/sanitizer_common/sanitizer_linux_libcdep.cc
170

indent

I suggest to use clang-format (in Google mode) when working on sanitizer code

194

indent

230

indent

lib/sanitizer_common/tests/sanitizer_linux_test.cc
272

This entire test is equivalent to the following:

ASSERT_EQ(pthread_self(), ThreadSelf())

You don't even need to spawn a new thread, as this should hold in the main thread as well.

This relies on the assumption that pthread_self() will return exactly the value of THREAD_SELF. This is currently true for all platforms, and will likely remain so. As long as we rely on that fact, we can probably just use pthread_self() in the code and move the THREAD_SELF computation into the test. I can do that in a separate change.

slthakur updated this revision to Diff 18414.Jan 19 2015, 10:59 PM
  • Corrected indentation
  • Added comment for TlsPreTcbSize()
  • Shifted computation of g_tls_size into TlsPreTcbSize()
  • Reduced test case in sanitizer_linux_test.cc

Hi @kcc, @earthdok,

This patch is important for http://reviews.llvm.org/D6291 to land.
Are there any suggestions/comments on this current revision ?

earthdok accepted this revision.Jan 23 2015, 11:58 AM
earthdok edited edge metadata.

lgtm with nits

Please don't forget to implement clone().

lib/sanitizer_common/sanitizer_linux_libcdep.cc
168

static

lib/sanitizer_common/tests/sanitizer_linux_test.cc
260

This needs a comment:

// Effectively, this is a test for ThreadDescriptorSize() which is used to compute ThreadSelf().

This revision is now accepted and ready to land.Jan 23 2015, 11:58 AM
slthakur updated this revision to Diff 18810.Jan 27 2015, 5:07 AM
slthakur edited edge metadata.
  • Updated patch as per suggestions by @earthdok

Hi @kcc, @earthdok,

Should we go ahead and commit this patch now ?

sdkie added a subscriber: sdkie.Jan 31 2015, 1:21 AM
slthakur updated this revision to Diff 20144.Feb 17 2015, 10:11 PM

Fixed unused variable warning for MIPS.

Sorry, this fell off my radar. Do you need me to land this?

lib/lsan/lsan_common.cc
141

style nit: this should be "#if defined" since there's an "#eilf defined" below

slthakur updated this revision to Diff 20259.Feb 18 2015, 10:20 PM

Corrected #ifdef directive.

@earthdok : Thanks for the review. I have corrected the #ifdef directive. @mohit.bhakkad will commit this patch for me.

mohit.bhakkad closed this revision.Feb 19 2015, 1:02 AM

Just a reminder:

Please don't forget to implement clone().