Page MenuHomePhabricator

Add sanitizer blacklists to the rules generated with -M/-MM/-MD/-MMD.
ClosedPublic

Authored by krasin on Aug 11 2015, 5:16 PM.

Details

Summary

Clang sanitizers, such as AddressSanitizer, ThreadSanitizer, MemorySanitizer,
Control Flow Integrity and others, use blacklists to specify which types / functions
should not be instrumented to avoid false positives or suppress known failures.

This change adds the blacklist filenames to the list of dependencies of the rules,
generated with -M/-MM/-MD/-MMD. This lets CMake/Ninja recognize that certain
C/C++/ObjC files need to be recompiled (if a blacklist is updated).

Diff Detail

Event Timeline

krasin updated this revision to Diff 31889.Aug 11 2015, 5:16 PM
krasin retitled this revision from to Add sanitizer blacklists to the rules generated with -M/-MM/-MD/-MMD..
krasin updated this object.
krasin added subscribers: cfe-commits, pcc.
pcc added a comment.Aug 11 2015, 5:35 PM

We should also make blacklists appear in the --show-includes output.

lib/Frontend/DependencyFile.cpp
418–422

If you move this code to the constructor you won't need to add an extra data member to this class.

test/Frontend/dependency-gen.c
26

Can we also test that the default blacklist appears in the -MD output if appropriate?

krasin updated this revision to Diff 31895.Aug 11 2015, 6:10 PM
krasin marked an inline comment as done.

Add more test cases.

In D11968#222338, @pcc wrote:

We should also make blacklists appear in the --show-includes output.

Is it about Windows compatibility? Do you mean that the output of

bin/clang-cl /Zs /showIncludes ~/lala.cc -fsanitize=address

should include the default blacklist (and any explicit blacklists, if they were specified)?

lib/Frontend/DependencyFile.cpp
418–422

I considered that. In this case, extra deps will appear even before the source file itself.

Right now, for my local test code, it generates:

lala.o: /usr/local/google/home/krasin/lala.cc \
  /usr/local/google/home/krasin/foo.h \
  /usr/local/google/home/krasin/bar.h \
  /usr/local/google/home/krasin/blacklist.txt

If moved to the constructor, it will become:

lala.o: /usr/local/google/home/krasin/blacklist.txt \
  /usr/local/google/home/krasin/lala.cc \
  /usr/local/google/home/krasin/foo.h \
  /usr/local/google/home/krasin/bar.h

This decreases the readability of the generated rules, as the main source becomes harder to discover.

Please, let me know, if you think it's not a problem; I will move the code to the constructor and eliminate the extra member.

pcc added a comment.Aug 11 2015, 6:17 PM

should include the default blacklist (and any explicit blacklists, if they were specified)?

Yes, exactly.

lib/Frontend/DependencyFile.cpp
418–422

These files are for machine consumption. If make and ninja accept this output, I don't think this should be a problem.

krasin updated this revision to Diff 31979.Aug 12 2015, 2:05 PM

Windows compat

krasin marked an inline comment as done.Aug 12 2015, 2:11 PM

Please, take another look. The test for --show-includes is on the way.

krasin updated this revision to Diff 31983.Aug 12 2015, 2:19 PM

More tests / fix tests.

pcc accepted this revision.Aug 12 2015, 3:44 PM
pcc added a reviewer: pcc.

LGTM

This revision is now accepted and ready to land.Aug 12 2015, 3:44 PM

Thank you, Peter.

I will commit once I have restored my password (the email to Chris is sent)

krasin closed this revision.Aug 12 2015, 9:05 PM