This is an archive of the discontinued LLVM Phabricator instance.

[LibFuzzer] Disable building and running LSan tests on Apple platforms because LSan is not currently supported.
ClosedPublic

Authored by delcypher on Jun 2 2016, 9:22 PM.

Details

Summary

[LibFuzzer] Disable building and running LSan tests on Apple platforms because LSan is not currently supported.

Diff Detail

Repository
rL LLVM

Event Timeline

delcypher updated this revision to Diff 59489.Jun 2 2016, 9:22 PM
delcypher retitled this revision from to [LibFuzzer] Disable building and running LSan tests on Apple platforms because LSan is not currently supported..
delcypher updated this object.
delcypher added reviewers: kcc, aizatsky.
delcypher added subscribers: kcc, aizatsky, anna and 2 others.
delcypher added a subscriber: llvm-commits.
delcypher edited subscribers, added: zaks.anna; removed: anna.Jun 2 2016, 9:24 PM
kcc edited edge metadata.Jun 3 2016, 10:50 AM

Why do you need to disable building the tests?
I would expect that "REQUIRES: lsan" is enough. No?

You may find that OutOfMemoryTest also does not *fully* work on OSX after my recent changes -- let's fix it separately.
BTW, it would be lovely if someone can implement lsan support for OSX (i.e. implement StopTheWorld)

lib/Fuzzer/test/CMakeLists.txt
96 ↗(On Diff #59489)

LeakSanitizer

97 ↗(On Diff #59489)

LeakSanitizer

delcypher added a comment.EditedJun 3 2016, 11:06 AM

@kcc:

Why do you need to disable building the tests?
I would expect that "REQUIRES: lsan" is enough. No?

My original version (not uploaded to phabricator) of the patch actually didn't disable building the tests. I decided to disable building them because I thought it was odd to build tests that would never be executed. However if you prefer I can change the patch to always build the tests because they do build successfully.

@kcc:

Why do you need to disable building the tests?
I would expect that "REQUIRES: lsan" is enough. No?

My original version (not uploaded to phabricator) of the patch actually didn't disable building the tests. I decided to disable building them because I thought it was to build tests that would never be executed. However if you prefer I can change the patch to always build the tests because they do build successfully.

Sorry

s/because I thought it was to build/because I thought it was odd to build/

delcypher added inline comments.Jun 3 2016, 11:17 AM
lib/Fuzzer/test/CMakeLists.txt
97 ↗(On Diff #59489)

The error message for the "DataflowSanitizer" tests is similarly worded. Do you want that fixed too?

kcc added a comment.Jun 3 2016, 11:41 AM

@kcc:

Why do you need to disable building the tests?
I would expect that "REQUIRES: lsan" is enough. No?

My original version (not uploaded to phabricator) of the patch actually didn't disable building the tests. I decided to disable building them because I thought it was to build tests that would never be executed. However if you prefer I can change the patch to always build the tests because they do build successfully.

Sorry

s/because I thought it was to build/because I thought it was odd to build/

Building these tests is very cheap.
Not building them makes the cmake file more complex.
If we can trade a tiny bit of CPU for less complexity we should do it.

lib/Fuzzer/test/CMakeLists.txt
97 ↗(On Diff #59489)

Yes, would be great.

delcypher added inline comments.Jun 6 2016, 4:14 PM
lib/Fuzzer/test/CMakeLists.txt
97 ↗(On Diff #59489)

@kcc: I will this as part of a separate patch because that is unrelated to this change.

delcypher updated this revision to Diff 59802.Jun 6 2016, 4:16 PM
delcypher edited edge metadata.
  • Build all tests, even the ones we aren't going to run
  • Fix Sanitizer marketing s/leak sanitizer/LeakSanitizer/.
kcc accepted this revision.Jun 6 2016, 6:05 PM
kcc edited edge metadata.

LGTM

This revision is now accepted and ready to land.Jun 6 2016, 6:05 PM
This revision was automatically updated to reflect the committed changes.