This is an archive of the discontinued LLVM Phabricator instance.

SanitizerArgs: add ability to filter/diagnose unsupported sanitizers.
ClosedPublic

Authored by pcc on Oct 21 2013, 5:57 PM.

Details

Summary

The sanitizers whose supported set of platforms I am sure of are now diagnosed
if enabled explicitly on an unsupported platform. Unsupported sanitizers which
are enabled implicitly (as part of a larger group) are silently disabled. As a
side effect, this makes SanitizerArgs parsing toolchain-dependent (and thus
essentially reverts r188058), and moves SanitizerArgs ownership to ToolChain.

Diff Detail

Event Timeline

+Richard, who reviewed the original patch

I support this change.

include/clang/Driver/SanitizerArgs.h
119

Can you add a comment for these functions?

lib/Driver/SanitizerArgs.cpp
230

This part:

if (Kinds & Foo) {
  Kinds &= ~Foo;
  <...>
}

can be factored out into a separate function.

Yes, moving ownership of the SanitizerArgs from Driver to ToolChain makes sense to me; the latter is parameterized by a set of command-line arguments, the former is not. I also support this change =)

pcc updated this revision to Unknown Object (????).Oct 22 2013, 4:33 PM
  • Address reviewer comments
samsonov added inline comments.Oct 23 2013, 9:13 PM
lib/Driver/SanitizerArgs.cpp
229

Do an early return here instead.

243

-fsanitize=thread is also unsupported in this case.

lib/Driver/ToolChains.cpp
301

Can you move this piece to filterUnsupportedKinds?

316

Can you move this piece to filterUnsupportedKinds?

pcc added inline comments.Oct 24 2013, 1:14 AM
lib/Driver/SanitizerArgs.cpp
229

Done.

243

Added.

lib/Driver/ToolChains.cpp
301

I think so. The logic behind isTargetIPhoneOS() and isTargetIOSSimulator() is in Darwin::AddDeploymentTarget and is rather more complex than my translation (e.g. it checks certain flags). This branch was untested though, so I'm inclined to change the logic and let Apple folks fix this if it causes a problem.

316

Likewise.

pcc updated this revision to Unknown Object (????).Oct 24 2013, 1:14 AM
  • Address reviewer comments
glider added inline comments.Oct 24 2013, 6:15 AM
lib/Driver/ToolChains.cpp
301

The reason behind the check for "isTargetIPhoneOS() && !isTargetIOSSimulator()" in ASan (added by me) was to prevent people from trying ASan on the real iOS devices where it doesn't work, but let them use ASan for iossim builds.
I'd love to keep this check, if possible.

samsonov added inline comments.Oct 24 2013, 6:35 AM
include/clang/Basic/DiagnosticDriverKinds.td
66 ↗(On Diff #5116)

separate commit?

include/clang/Driver/SanitizerArgs.h
136

Kind is actually a Mask. There is no FlagName parameter. Consider fixing parameter names and comment.

lib/Driver/SanitizerArgs.cpp
50

please remove blank lines in added block

233

ditto

247

I'd appreciate booleans like "IsLinux", "IsX86" etc. for brevity.

259

Make sure you agree with glider@ on this part. I think it would be fine to revert this piece to get the patch committed, leave a FIXME, and deal with/refactor this part later.

pcc updated this revision to Unknown Object (????).Oct 24 2013, 3:14 PM
  • Address reviewer comments
include/clang/Basic/DiagnosticDriverKinds.td
66 ↗(On Diff #5116)

Yeah, this would go with whichever commit removes the isTargetIOS checks.

include/clang/Driver/SanitizerArgs.h
136

Updated.

lib/Driver/SanitizerArgs.cpp
50

Done.

233

Done.

247

Done.

259

After investigating how Xcode builds iOS stuff on Darwin I think I've come to the conclusion that the driver sees the wrong triple, so my change wouldn't work. Reverted to be dealt with later.

rsmith added inline comments.Oct 24 2013, 3:23 PM
lib/Driver/SanitizerArgs.cpp
47–52

If I say:

clang -target triple -fsanitize=foo,bar -fno-sanitize=bar

and bar is not supported on triple, I don't think we should diagnose it.

pcc updated this revision to Unknown Object (????).Oct 24 2013, 5:10 PM
  • Avoid diagnosing unsupported sanitizers which are added then removed
lib/Driver/SanitizerArgs.cpp
47–52

Agree, done.

rsmith added inline comments.Oct 24 2013, 5:32 PM
lib/Driver/SanitizerArgs.cpp
51

Hmm, should we mask out AllAdd here too? We don't want to issue two diagnostics for -fsanitize=bad -fsanitize=bad. But on the other hand diagnosing -fsanitize=bad -fsanitize=group-including-bad might be nice. Ugh. Maybe what we want is a limit of one diagnostic per problem?

120–123

This looks a bit strange. We won't diagnose

-fsanitize=address -fno-sanitize=address -fsanitize=init-order

Following the principle of later arguments overriding earlier ones, I would have expected a diagnostic for that, but not for:

-fsanitize=address -fsanitize=init-order -fno-sanitize=address

Seems to be a pre-existing oddness, though.

pcc updated this revision to Unknown Object (????).Oct 24 2013, 7:13 PM
  • Address review comments
lib/Driver/SanitizerArgs.cpp
51

Yes, masking out AllAdd doesn't seem like it will work trivially. Deduplicating in filterUnsupportedMask by accumulating a mask of diagnosed kinds seems to work, although (as the new test shows) we don't give the best diagnostics in the world sometimes.

120–123

*shrug* I'll let Alexey comment on this, as he introduced this behavior in r173670.

The change looks good to me.

lib/Driver/SanitizerArgs.cpp
46

Please less blank lines here

120–123

Yes, that one is tricky :( I planned to remove this code together with -fsanitize=init-order option, so I'd rather not heavily modify it.

256

ditto

pcc updated this revision to Unknown Object (????).Oct 25 2013, 5:09 PM
  • Remove blank lines
pcc closed this revision.Nov 1 2013, 11:20 AM

Closed by commit rL193875 (authored by @pcc).