This is an archive of the discontinued LLVM Phabricator instance.

[MSan][Clang][MIPS] Enabled memory, dataflow and thread options for MIPS platform
ClosedPublic

Authored by mohit.bhakkad on Nov 6 2014, 2:46 AM.

Diff Detail

Event Timeline

mohit.bhakkad retitled this revision from to [MSan][Clang][MIPS] Enabled memory, dataflow and thread options for MIPS platform.
mohit.bhakkad updated this object.
mohit.bhakkad edited the test plan for this revision. (Show Details)
mohit.bhakkad added reviewers: kcc, eugenis, samsonov, petarj.
mohit.bhakkad set the repository for this revision to rL LLVM.
mohit.bhakkad added subscribers: dsanders, sdkie, slthakur, Unknown Object (MLST).
samsonov edited edge metadata.Nov 6 2014, 10:58 AM

Does MSan/DFSan actually work and pass all the tests on mips32 and mips64? Does TSan work and pass all tests on mips64?

In D6147#5, @samsonov wrote:

Does MSan/DFSan actually work and pass all the tests on mips32 and mips64? Does TSan work and pass all tests on mips64?

I have submitted patches of MSan mips64[1] and mips32[2], for which majority of tests
are passing, and some failures are because of minor differences in printing, I am working on it.
@eugenis suggested me to submit patches for llvm and clang.

As we are also working on TSan and DFSan, I thought of enabling them in clang.

[ 1 ] http://reviews.llvm.org/D5906
[ 2 ] http://reviews.llvm.org/D5672

eugenis edited edge metadata.Nov 7 2014, 1:56 AM

I don't think 100% passing tests should be a requirement for this change to go in. On the opposite, driver support has value even if the tool barely works, and is a prerequisite for tests passing.

IIRC we had problems when we enabled the feature with a few tests still failing, and immediately got complaints from users, whose LLVM build started to fail "check-all" command.
I agree that it's hard to use and test the tool w/o Driver support. But I'd like to see the order of landing patches that would keep the MIPS build green.

E.g. I'm OK with landing the change to the driver before enabling building/testing of a specific sanitizer runtime.

kcc edited edge metadata.Nov 19 2014, 1:04 PM

is dfsan expected to work in 32-bit?

pcc added a subscriber: pcc.Nov 19 2014, 1:33 PM

I would be surprised if it did. Even if it were made to work, it would probably not be very useful because of its memory requirements.

mohit.bhakkad edited edge metadata.
mohit.bhakkad set the repository for this revision to rL LLVM.

Changes in this revision:
Removed DataFlow option for MIPS32

kcc added a comment.Nov 21 2014, 11:30 AM

Please do not enable msan on MIPS32 before enabling it on x86-32, as I've explained in another thread.

I can not speak for DFSan.

mohit.bhakkad set the repository for this revision to rL LLVM.

Changes in this revision:

  • removed memory for mips32
  • added function for mips32
kcc added a comment.Dec 4 2014, 1:31 PM

Looks ok. Alexey?

pcc added inline comments.Dec 4 2014, 2:09 PM
lib/Driver/SanitizerArgs.cpp
140

Does -fsanitize=function work on MIPS? I'd imagine you'd need an implementation of getUBSanFunctionSignature for MIPS.

mohit.bhakkad added inline comments.Dec 5 2014, 1:12 AM
lib/Driver/SanitizerArgs.cpp
140

@Sagar is going to submit a patch which implements this

Do you plan to setup the public buildbot that would run sanitizer test suite on MIPS machine?

Buildbots are on my todo list and I intend to start pushing this along again once we're past the 3.5.1 release. One thing I'm wondering about for the sanitizer buildbot is how important is the amount of RAM? The systems I have available are 2GB as standard. I might be able to do a bit better than that but I've been warned about the dragons down that path so I'd prefer not to take that route unless it's really important.

kcc added a comment.Dec 8 2014, 10:56 AM

*san uses more memory, so RAM is indeed important and you need to be careful not to overwhelm the bot.
Our Android bot has (afaict) 2Gb, so it should work for you too.

samsonov accepted this revision.Dec 8 2014, 5:40 PM
samsonov edited edge metadata.

I'm fine with the change, although I'd rather roll it gradually. First land the part for MSan/DFSan (which are already supported on MIPS), then TSan and -fsanitize=function when they passes the review.

This revision is now accepted and ready to land.Dec 8 2014, 5:40 PM

Let me know if you need some part of the patch committed

In D6147#28, @samsonov wrote:

Let me know if you need some part of the patch committed

Yes, Memory and DataFlow options should be there to build compiler-rt on mips. Between I just got "commit after approval" rights, I can commit this partial change if you agree.
One more thing, http://reviews.llvm.org/D6146 is also equally important for MSan, please let me know if any changes are required in it.

Alright, feel free to submit the MSan/DFSan piece once they are functional (i.e. when http://reviews.llvm.org/D6146 is landed).

Enabled memory, dataflow in rL226790

This revision was automatically updated to reflect the committed changes.