This is an archive of the discontinued LLVM Phabricator instance.

Tread TSan LLVM flags to driver: add TSan controlling flags to clang.
ClosedPublic

Authored by alekseyshl on Nov 9 2016, 9:53 AM.

Event Timeline

alekseyshl updated this revision to Diff 77366.Nov 9 2016, 9:53 AM
alekseyshl retitled this revision from to Tread TSan LLVM flags to driver: add TSan controlling flags to clang..
alekseyshl updated this object.
alekseyshl added a reviewer: eugenis.
alekseyshl added a subscriber: cfe-commits.
eugenis added inline comments.Nov 9 2016, 11:46 AM
lib/Frontend/CompilerInvocation.cpp
735

It seems common to hardcode the default option value here.
The same in SanitizerArgs.cpp.

eugenis edited edge metadata.Nov 9 2016, 11:52 AM

Oh, and this needs a test. See test/Driver/fsanitize.c (search for -fsanitize-address-use-after-scope as an example).

alekseyshl added inline comments.Nov 9 2016, 12:00 PM
lib/Frontend/CompilerInvocation.cpp
735

True, but the default value for these flags is already mentioned in more than one place, why not to try to reduce the complexity?

eugenis added inline comments.Nov 9 2016, 1:49 PM
lib/Frontend/CompilerInvocation.cpp
735

I guess I don't mind either way.

alekseyshl updated this revision to Diff 77398.Nov 9 2016, 2:57 PM
alekseyshl edited edge metadata.
  • Added ust test for the new command line args.
eugenis added inline comments.Nov 9 2016, 3:00 PM
test/Driver/fsanitize.c
288

did you mean CHECK-TSAN-DATA-RACES-BOTH-NOT?

dvyukov added inline comments.Nov 9 2016, 3:13 PM
include/clang/Driver/Options.td
733

These descriptions may be confusing for users.
This does not disable data race detection in tsan. Even if all files are compiled with this flag, tsan can still report races. Same for traces and atomics.
Please make it clear that it only enables/disables source _instrumentation_.

Does user see what's the default value? If not, then I guess some users will add flags just because they do want data races detection and stack traces. We need to make it clear that all flags are enabled by default.

alekseyshl updated this revision to Diff 77432.Nov 9 2016, 6:04 PM
alekseyshl marked 2 inline comments as done.
  • Rename new options to better reflect their functionality.
alekseyshl added inline comments.Nov 9 2016, 6:08 PM
test/Driver/fsanitize.c
288

I missed -BOTH on the FileCheck line above and in couple of other places.

dvyukov accepted this revision.Nov 10 2016, 9:19 PM
dvyukov edited edge metadata.
This revision is now accepted and ready to land.Nov 10 2016, 9:19 PM
alekseyshl updated this revision to Diff 77631.Nov 11 2016, 9:50 AM
alekseyshl edited edge metadata.
  • Translate new driver flags to legacy -llvm ones.
pcc accepted this revision.Nov 11 2016, 10:58 AM
pcc added a reviewer: pcc.
pcc added a subscriber: pcc.

Seems like a reasonable enough first step. Please add a FIXME to pass these flags as attributes.

eugenis added inline comments.Nov 11 2016, 11:32 AM
lib/Frontend/CompilerInvocation.cpp
732

It looks like lib/Frontend changes are no longer necessary.

alekseyshl edited edge metadata.
  • Remove unnecessary frontend options.
alekseyshl marked an inline comment as done.Nov 11 2016, 12:04 PM

Frontend options removed.

eugenis accepted this revision.Nov 11 2016, 12:04 PM
eugenis edited edge metadata.

LGTM

pcc added inline comments.Nov 11 2016, 12:06 PM
include/clang/Driver/Options.td
732

You can remove the Flags<[CC1Option]> part in each of these now.

alekseyshl edited edge metadata.
  • Add FIXME to address the rest of the suggestions later.
alekseyshl updated this revision to Diff 77667.Nov 11 2016, 1:58 PM
  • Remove unnecessary flag qualifications.
alekseyshl marked an inline comment as done.Nov 11 2016, 1:58 PM

.

This revision was automatically updated to reflect the committed changes.