[Driver][sanitizers] Warn about ignored -fsanitize-trap or -fsanitize-recover

[jkorous]

Summary

We have a number of internal reports from UBSan users that use -fsanitize-trap=foo or -fsanitize-recover=bar expecting the respective -fsanitize= is automatically implied which isn't the case and this option is silently ignored.

I feel we should try harder to prevent our users from making this mistake.

[yln]

This sounds like a good improvement, thanks!

LGTM, with nits. We should also get one additional approval from an external/open source contributor.

clang/include/clang/Basic/DiagnosticDriverKinds.td

548–551

Can foo and bar here be individual UBSan checks or only "full" sanitizers?

If it's easy (i.e., there already exists a SanitizerKind->string mapping), can we include which foo or bar causes the warning. Might be helpful when (a long list of) individual UBSan checks are used.

Message wording nitpicking: active form, more aligned with other warnings.

Ignoring -fsanitize-trap=foo without corresponding -fsanitize=foo

clang/lib/Driver/SanitizerArgs.cpp

208–215

Are those good names?

My understanding is that we have a trap default behavior which can be overridden for specific checks. Previously we would directly return the computed set of "default + overrides", but now we want to print these new warnings which requires us to know individual overrides.

How about the following:

struct SanitizeTrapOverrides {
  SanitizerMask TrapsAdded;
  SanitizerMask TrapsRemoved;
};

Maybe even warrants renaming the TrappingKinds local var below.

[vsk]

Since this is a fairly impactful driver change, I'd suggest sending a heads-up about it to llvm-dev and adding a release note for extra visibility.

[jkorous]

clang/include/clang/Basic/DiagnosticDriverKinds.td

548–551

Yes, it responds to presence of any -fsanitize-trap/recover command line argument no matter its value. I changed the test so it includes a specific UBSan check.

I'm not sure we need to spell out the command line argument value here unless we start to be more aggressive with the warning.
Currently the warning is emitted only if there's ANY explicit -fsanitize-trap that wasn't disabled with -fno-sanitize-trap AND NO -fsanitize (explicit or implicit == no default sanitizer for the toolchain). Which means that all the -fsanitize-trap arguments are equally important to warn about and adding any -fsanitize argument will fix the warning.

Thanks to your question I just realized I should also add a test for -fsanitize-trap=foo -fno-sanitize-trap=foo case :)

I don't think we can start warning about a missing -fsanitize per sanitizer/check group/check found in -fsanitize-trap/recover as for UBSan it's not unreasonable to use for example:
-fsanitize=array-bounds,integer -fsanitize-trap=undefined
It simply means we want to trap for all UBSan checks but we don't want to repeat the definition of which checks to use.

I'm open to be persuaded we don't need to be this conservative with the warnings in which case spelling out the value of suspicious -fsanitize-trap/recover argument might be necessary.

Edit: I just realized there's a problem with UBSan groups of checks - for example shift = {shift-base, shift-exponent}. That means that for this command line the problematic check isn't even spelled out: -fsanitize-trap=shift -fno-sanitize-trap=shift-base (as this pretty much evaluates to -fsanitize-trap=shift-exponent).
I'm not sure if this is argument against your idea (potentially complex implementation) or for (current behavior might be confusing for users).

clang/lib/Driver/SanitizerArgs.cpp

208–215

TLDR: I don't feel strongly about these names. I'm willing to change them - just explaining how did I arrive at the current names. Let me know if you still consider these better.

Yes, you are pretty much correct - except we don't care about the individual overrides, just about the existence of any of these.
Also the fno-sanitize-trap "un-overrides" have a funny property of affecting only the state that have been processed before themselves (with default being processed at the start). In other words - the order on the command line matters and the semantics here isn't simple set operations.

> bin/clang -fsanitize=array-bounds -fno-sanitize-trap=array-bounds -fsanitize-trap=array-bounds ~/tmp/overflow.c -###

... "-fsanitize=array-bounds" "-fsanitize-trap=array-bounds" ...

> bin/clang -fsanitize=array-bounds -fsanitize-trap=array-bounds -fno-sanitize-trap=array-bounds ~/tmp/overflow.c -##
... "-fsanitize=array-bounds" "-fsanitize-recover=array-bounds" ...

I kept the names "low level" - with semantics close to command line option spelling to avoid interpreting them. If we were to rename these in the spirit of your suggestion it should probably be more like this:

struct SanitizeTrapOverrides {
  SanitizerMask TrapsAddedButNotRemoved;
  SanitizerMask TrapsToBeRemovedFromDefault;
};

The local var name seems sensible to me - it is the end result of processing the command line which says which checks will trap (if used; modulo some sanity checks of this set later).

[jkorous]

improved tests

[jkorous]

I just realized that for -fsanitize-recover the logic is inverted - it's actually -fno-sanitize-recover that's an explicit change from the default.
I will probably need to do the same thing I do for traps.

[jkorous]

I will send an email to llvm-dev@ and add a release note once I finish that.

clang/include/clang/Basic/DiagnosticDriverKinds.td

def err_aix_default_altivec_abi : Error<
   "The default Altivec ABI on AIX is not yet supported, use '-mabi=vec-extabi' for the extended Altivec ABI">;
 
 def note_cc1_round_trip_original : Note<"Original arguments in round-trip: %0">;
 def note_cc1_round_trip_generated : Note<"Generated arguments #%0 in round-trip: %1">;
 def remark_cc1_round_trip_generated : Remark<"Generated arguments #%0 in round-trip: %1">, InGroup<RoundTripCC1Args>;
 def err_cc1_round_trip_fail_then_ok : Error<"Original arguments parse failed, then succeeded in round-trip">;
 def err_cc1_round_trip_ok_then_fail : Error<"Generated arguments parse failed in round-trip">;
 def err_cc1_round_trip_mismatch : Error<"Generated arguments do not match in round-trip">;
+
+def warn_drv_fsanitize_trap_ignored : Warning<
+  "-fsanitize-trap used without -fsanitize is ignored">;
+def warn_drv_fsanitize_recover_ignored : Warning<
+  "-fsanitize-recover used without -fsanitize is ignored">;
 }

clang/lib/Driver/SanitizerArgs.cpp

 #define SANITIZER(NAME, ID)
 #define SANITIZER_GROUP(NAME, ID, ALIAS)                                       \
   if (Kinds & SanitizerKind::ID)                                               \
     Kinds |= SanitizerKind::ID##Group;
 #include "clang/Basic/Sanitizers.def"
   return Kinds;
 }
 
-static SanitizerMask parseSanitizeTrapArgs(const Driver &D,
-                                           const llvm::opt::ArgList &Args) {
+struct ParsedSanitizeTrapOpts {
+  SanitizerMask PrunedTrapKinds;
+  SanitizerMask NoTrapKinds;
+};
+
+static ParsedSanitizeTrapOpts
+parseSanitizeTrapArgs(const Driver &D, const llvm::opt::ArgList &Args) {
   SanitizerMask TrapRemove;     // During the loop below, the accumulated set of
                                 // sanitizers disabled by the current sanitizer
                                 // argument or any argument after it.
   SanitizerMask TrappingKinds;
   SanitizerMask TrappingSupportedWithGroups = setGroupBits(TrappingSupported);
 
   for (ArgList::const_reverse_iterator I = Args.rbegin(), E = Args.rend();
        I != E; ++I) {
     const auto *Arg = *I;
if (Arg->getOption().matches(options::OPT_fsanitize_trap_EQ)) {
       }
       TrappingKinds |= expandSanitizerGroups(Add) & ~TrapRemove;
     } else if (Arg->getOption().matches(options::OPT_fno_sanitize_trap_EQ)) {
       Arg->claim();
       TrapRemove |= expandSanitizerGroups(parseArgValues(D, Arg, true));
     }
   }
 
-  // Apply default trapping behavior.
-  TrappingKinds |= TrappingDefault & ~TrapRemove;
-
-  return TrappingKinds;
+  return ParsedSanitizeTrapOpts{TrappingKinds, TrapRemove};
 }
 
 bool SanitizerArgs::needsFuzzerInterceptors() const {
   return needsFuzzer() && !needsAsanRt() && !needsTsanRt() && !needsMsanRt();
 }
 
 bool SanitizerArgs::needsUbsanRt() const {
   // All of these include ubsan.
SanitizerArgs::SanitizerArgs(const ToolChain &TC,
   const SanitizerMask Supported = setGroupBits(TC.getSupportedSanitizers());
 
   CfiCrossDso = Args.hasFlag(options::OPT_fsanitize_cfi_cross_dso,
                              options::OPT_fno_sanitize_cfi_cross_dso, false);
 
   ToolChain::RTTIMode RTTIMode = TC.getRTTIMode();
 
   const Driver &D = TC.getDriver();
-  SanitizerMask TrappingKinds = parseSanitizeTrapArgs(D, Args);
+  const ParsedSanitizeTrapOpts ParsedKind = parseSanitizeTrapArgs(D, Args);
+  // Apply default trapping behavior.
+  SanitizerMask TrappingKinds =
+      ParsedKind.PrunedTrapKinds | (TrappingDefault & ~ParsedKind.NoTrapKinds);
+
   SanitizerMask InvalidTrappingKinds = TrappingKinds & NotAllowedWithTrap;
 
   MinimalRuntime =
       Args.hasFlag(options::OPT_fsanitize_minimal_runtime,
                    options::OPT_fno_sanitize_minimal_runtime, MinimalRuntime);
 
   // The object size sanitizer should not be enabled at -O0.
   Arg *OptLevel = Args.getLastArg(options::OPT_O_Group);
SanitizerArgs::SanitizerArgs(const ToolChain &TC,
   for (auto G : IncompatibleGroups) {
     SanitizerMask Group = G.first;
     if ((Default & Group) && (Kinds & G.second))
       Default &= ~Group;
   }
 
   Kinds |= Default;
 
+  if (ParsedKind.PrunedTrapKinds && !Kinds)
+    D.Diag(diag::warn_drv_fsanitize_trap_ignored);
+
   // We disable the vptr sanitizer if it was enabled by group expansion but RTTI
   // is disabled.
   if ((Kinds & SanitizerKind::Vptr) && (RTTIMode == ToolChain::RM_Disabled)) {
     Kinds &= ~SanitizerKind::Vptr;
   }
 
   // Check that LTO is enabled if we need it.
   if ((Kinds & NeedsLTO) && !D.isUsingLTO()) {
SanitizerArgs::SanitizerArgs(const ToolChain &TC,
   // -fsanitize=address. Perhaps it should print an error, or perhaps
   // -f(-no)sanitize=leak should change whether leak detection is enabled by
   // default in ASan?
 
   // Parse -f(no-)?sanitize-recover flags.
   SanitizerMask RecoverableKinds = RecoverableByDefault | AlwaysRecoverable;
   SanitizerMask DiagnosedUnrecoverableKinds;
   SanitizerMask DiagnosedAlwaysRecoverableKinds;
+  bool AnyExplicitRecoverableKind = false;
   for (const auto *Arg : Args) {
     if (Arg->getOption().matches(options::OPT_fsanitize_recover_EQ)) {
       SanitizerMask Add = parseArgValues(D, Arg, true);
       // Report error if user explicitly tries to recover from unrecoverable
       // sanitizer.
       if (SanitizerMask KindsToDiagnose =
               Add & Unrecoverable & ~DiagnosedUnrecoverableKinds) {
         SanitizerSet SetToDiagnose;
         SetToDiagnose.Mask |= KindsToDiagnose;
         D.Diag(diag::err_drv_unsupported_option_argument)
             << Arg->getOption().getName() << toString(SetToDiagnose);
         DiagnosedUnrecoverableKinds |= KindsToDiagnose;
       }
       RecoverableKinds |= expandSanitizerGroups(Add);
+      AnyExplicitRecoverableKind = true;
       Arg->claim();
     } else if (Arg->getOption().matches(options::OPT_fno_sanitize_recover_EQ)) {
       SanitizerMask Remove = parseArgValues(D, Arg, true);
       // Report error if user explicitly tries to disable recovery from
       // always recoverable sanitizer.
       if (SanitizerMask KindsToDiagnose =
               Remove & AlwaysRecoverable & ~DiagnosedAlwaysRecoverableKinds) {
         SanitizerSet SetToDiagnose;
         SetToDiagnose.Mask |= KindsToDiagnose;
         D.Diag(diag::err_drv_unsupported_option_argument)
             << Arg->getOption().getName() << toString(SetToDiagnose);
         DiagnosedAlwaysRecoverableKinds |= KindsToDiagnose;
       }
       RecoverableKinds &= ~expandSanitizerGroups(Remove);
       Arg->claim();
     }
   }
+
+  if (AnyExplicitRecoverableKind && !Kinds)
+    D.Diag(diag::warn_drv_fsanitize_recover_ignored);
+
   RecoverableKinds &= Kinds;
   RecoverableKinds &= ~Unrecoverable;
 
   TrappingKinds &= Kinds;
   RecoverableKinds &= ~TrappingKinds;
 
   // Setup blacklist files.
   // Add default blacklist from resource directory for activated sanitizers, and

clang/test/Driver/fsanitize-recover-ignored.c

+// RUN: %clang -fsanitize-recover=integer %s -### 2>&1 | FileCheck %s
+// RUN: %clang -fsanitize-recover=shift-base %s -### 2>&1 | FileCheck %s
+// RUN: %clang -fsanitize-recover=memory %s -### 2>&1 | FileCheck %s
+
+// CHECK:  warning: -fsanitize-recover used without -fsanitize is ignored

clang/test/Driver/fsanitize-trap-ignored.c

+// RUN: %clang -fsanitize-trap=integer %s -### 2>&1 | FileCheck %s
+// RUN: %clang -fsanitize-trap=shift-base %s -### 2>&1 | FileCheck %s
+// RUN: %clang -fsanitize-trap=memory %s -### 2>&1 | FileCheck %s
+
+// RUN: %clang -fno-sanitize-trap=shift-base -fsanitize-trap=shift-base %s -### 2>&1 | FileCheck %s
+// RUN: %clang -fsanitize-trap=shift-base -fno-sanitize-trap=shift-base %s -### 2>&1 | FileCheck %s -check-prefixes=NO
+
+// RUN: %clang -fsanitize-trap=shift -fno-sanitize-trap=shift-base %s -### 2>&1 | FileCheck %s
+
+// RUN: %clang -fsanitize-trap=shift-base -fno-sanitize-trap=shift %s -### 2>&1 | FileCheck %s -check-prefixes=NO
+
+// CHECK:  warning: -fsanitize-trap used without -fsanitize is ignored
+// NO-NOT:  warning: -fsanitize-trap used without -fsanitize is ignored