This is an archive of the discontinued LLVM Phabricator instance.

Introduce -fsanitize-trap= flag.
ClosedPublic

Authored by pcc on Jun 15 2015, 5:04 PM.

Details

Summary

This flag controls whether a given sanitizer traps upon detecting
an error. It currently only supports UBSan. The existing flag
-fsanitize-undefined-trap-on-error has been made an alias of
-fsanitize-trap=undefined.

This change also cleans up some awkward behavior around the combination
of -fsanitize-trap=undefined and -fsanitize=undefined. Previously we
would reject command lines containing the combination of these two flags,
as -fsanitize=vptr is not compatible with trapping. This required the
creation of -fsanitize=undefined-trap, which excluded -fsanitize=vptr
(and -fsanitize=function, but this seems like an oversight).

Now, -fsanitize=undefined is an alias for -fsanitize=undefined-trap,
and if -fsanitize-trap=undefined is specified, we treat -fsanitize=vptr
as an "unsupported" flag, which means that we error out if the flag is
specified explicitly, but implicitly disable it if the flag was implied
by -fsanitize=undefined.

Diff Detail

Event Timeline

pcc updated this revision to Diff 27731.Jun 15 2015, 5:04 PM
pcc retitled this revision from to Introduce -fsanitize-trap= flag..
pcc updated this object.
pcc edited the test plan for this revision. (Show Details)
pcc added a reviewer: samsonov.
pcc added subscribers: rsmith, Unknown Object (MLST).
rsmith added inline comments.Jun 15 2015, 5:57 PM
docs/UsersManual.rst
1123

Missing 'the' before -fsanitize=?

1124–1126

I think this description will leave people wondering why we have a per-sanitizer trap flag rather than a global flag. It'd be good to extend this description with the CFI case.

1129

It's probably better to refer to the group as "`undefined" rather than "UndefinedBehaviorSanitizer", for symmetry with "vptr`".

lib/CodeGen/CGExpr.cpp
2306–2315

The differing style in these two cases is strange. Please pick one or the other and apply it to both the Recover and the Trap cases.

lib/Driver/SanitizerArgs.cpp
223

I think this should be TrappingKinds & Vptr. If I write -fsanitize-trap=undefined -fsanitize=vptr -fno-sanitize-trap=vptr, I think you'll currently mark vptr as unavailable, but it seems reasonable to support that case.

samsonov added inline comments.Jun 15 2015, 6:09 PM
docs/UsersManual.rst
973–975

"deprecated alias"

1121

Please move it next to -fsanitize-recover=.

1128

Do you think we should allow other trapping sanitizers (like local-bounds) here? Or you plan to do it in subsequent patches?

include/clang/Driver/Options.td
526

Please move next to fsanitize_recover

lib/Driver/SanitizerArgs.cpp
37

Could we use smth. like getSanitizersWithNoRequiredRuntime() from http://reviews.llvm.org/D10467 instead (if we decide to allow -fsanitize-trap=` for other checks)?

37

I'd prefer to not use UndefinedGroup here, and instead add special case for Vptr to the loop iterating over -fsanitize-trap= arguments. E.g. see how we handle special-case for vptr+RTTI in the main loop in SanitizerArgs constructor (not that it's elegant, it's just... more literal).

542–544
if (!TrapSanitizers.empty())
pcc updated this revision to Diff 27747.Jun 15 2015, 7:56 PM
  • Address review comments
docs/UsersManual.rst
973–975

Done

1121

Done

1123

Added

1124–1126

The CFI case will be implemented in a separate patch, which will include a documentation update here. For now we do not support -fsanitize-trap=cfi.

1128

Added.

1129

Done

include/clang/Driver/Options.td
526

Done

lib/CodeGen/CGExpr.cpp
2306–2315

Done

lib/Driver/SanitizerArgs.cpp
37

Could we use smth. like getSanitizersWithNoRequiredRuntime() from http://reviews.llvm.org/D10467 instead (if we decide to allow -fsanitize-trap=` for other checks)?

Most likely, I'll do that once your change lands.

I'd prefer to not use UndefinedGroup here, and instead add special case for Vptr to the loop iterating over -fsanitize-trap= arguments.

I don't see how that would work. I think we'd always need groups in the thing we mask against, otherwise we'd incorrectly diagnose things like -fsanitize-trap=undefined due to the presence of the group. Seems like the setGroupBits thing you added in D10467 does what I want?

223

Done

542–544

Done

samsonov added inline comments.Jun 16 2015, 11:41 AM
lib/CodeGen/CGExpr.cpp
2306

Should we clarify this behavior in the docs?
We can also handle this case in the driver, so that frontend can assert that each sanitizer check is listed in at most one of -fsanitize-recover= and -fsanitize-trap= lists. (i.e. intersection of SanitizeTrap and SanitizeRecover is empty).

Another interesting question is handling the relative order of -fsanitize-trap= and -fsanitize-recover= flags. Now we parse them completely independently, but this may not be the best solution. Consider global

CFLAGS='-fsanitize=undefined -fsanitize-trap=undefined'

and now I want to compile a special program, and enable recovery for alignment issues. Using

clang++ $(CFLAGS) -fsanitize-recover=alignment a.cc

wouldn't work, I would have to write

clang++ $(CFLAGS) -fno-sanitize-trap=alignment -fsanitize-recover=alignment a.cc

Do you think we should instead parse -fsanitize-trap and -fsanitize-recover in a single pass, and maintain the recovery setting for each sanitizer kind to be one of

  • diagnosed, recoverable
  • diagnosed, unrecoverable/fatal
  • undiagnosed, trapping.

?

lib/Driver/SanitizerArgs.cpp
37

Yep, let's use setGroupBits. Specifying the groups here manually is error-prone - one can easily forget to change this part if Sanitizers.def is re-orrganized.

517

Note that you can also strip out sanitizers which were not enabled eventually from here

TrappingKinds &= Kinds;

as we do for recoverable sanitizers.

pcc updated this revision to Diff 27799.Jun 16 2015, 5:13 PM
  • Fix disablement logic
pcc updated this revision to Diff 27804.Jun 16 2015, 5:51 PM
  • Document -fsanitize-trap precedence over -fsanitize-recover
  • Filter TrappingKinds based on Kinds
lib/CodeGen/CGExpr.cpp
2306

Should we clarify this behavior in the docs?

Yes, done.

Do you think we should instead parse -fsanitize-trap and -fsanitize-recover in a single pass

The logic around parsing these flags is already too complicated and I would be against making it more complex than it already is. I would prefer to solve this problem with documentation.

lib/Driver/SanitizerArgs.cpp
517

Done

samsonov added inline comments.Jun 17 2015, 1:04 PM
include/clang/Driver/Options.td
565

Add some HelpText

lib/Driver/SanitizerArgs.cpp
37

If you're happy with http://reviews.llvm.org/D10467, we can submit that one first, and add setGroupBits to this patch. Or steal setGroupBits part from there if you want this patch to go in first.

223

We already have NotAllowedWithTrap mask. I think we should use it, and probably split diagnosing the sanitizers unsupported because they cannot trap, and those that are not supported by the toolchain. You already do some of the former in parseSanitizeTrapArgs above, I think we should handle vptr in that function as well somehow.

pcc updated this revision to Diff 27904.Jun 17 2015, 5:33 PM
  • Address reviewer comments
include/clang/Driver/Options.td
565

Done

lib/Driver/SanitizerArgs.cpp
37

Done. Per offline discussion I've moved setGroupBits to this patch.

223

We already have NotAllowedWithTrap mask. I think we should use it

Done.

and probably split diagnosing the sanitizers unsupported because they cannot trap, and those that are not supported by the toolchain. You already do some of the former in parseSanitizeTrapArgs above, I think we should handle vptr in that function as well somehow.

Not really sure how that would work. We only know which sanitizers to forbid once we have looked at all -fsanitize-trap arguments. At the very least it makes the code easier to read if all unsupported sanitizer diagnostics are done in a single place. (This would have fit much better into the original design from D1990 with an extra clause in filterUnsupportedKinds; oh well.)

samsonov accepted this revision.Jun 18 2015, 4:22 PM
samsonov edited edge metadata.

I'm mostly happy with this patch. Accepting it, so that you can land it without yet another round of review, but please consider addressing the comments below.

Thank you for doing this!

lib/Driver/SanitizerArgs.cpp
147
if (SanitizerMask InvalidValues = Add & ~TrappingSupportedWithGroups) {
  SanitizerSet S;
  S.Mask = InvalidValues;
  //....
}
165
TrapRemove |= expandSanitizerGroups(UndefinedGroup);
239

I don't think this is right. Suppose -fsanitize=vptr is not supported on target foo (by corresponding toolchain). Then

clang -target foo -fsanitize=vptr a.cc

would produce confusing

"-fsanitize=vptr" is not allowed with "-fsanitize-trap=undefined"

I think you would need separate clause for that

if (SanitizerMask KindsToDiagnose = Add & TrappingKinds & NotAllowedWithTrap & ~DiagnosedKinds) {
  std::string Desc = describeSanitizeArg(*I, KindsToDiagnose);
  D.Diag(diag::err_drv_argument_not_allowed_with) << Desc << lastTrapArgumentForMask(KindsToDiagnose);
  DiagnosedKinds |= KindsToDiagnose;
}
This revision is now accepted and ready to land.Jun 18 2015, 4:22 PM
pcc added inline comments.Jun 18 2015, 5:03 PM
lib/Driver/SanitizerArgs.cpp
147

Done

165

Done

239

Done. (We know that the second argument to the diagnostic can only ever be "-fsanitize-trap=undefined", so I've hard coded that for now.)

This revision was automatically updated to reflect the committed changes.