[BoundsSafety][Attempt 2] Add warning diagnostics for uses of legacy bounds checks

** This is the second attempt landing this patch. This first (#10800
4c8f6e7) failed because there was a bug. **

The bug was in `ParseBoundsSafetyNewChecksMaskFromArgs` where the call to
`DiagnoseDisabledBoundsSafetyChecks` was not guarded with
`DiagnoseMissingChecks`. This missing check caused diagnostics to appear
when they shouldn't and caused crashes in cases where the `Diags`
pointer was nullptr. To fix this the missing guard has been added and
an assert that `Diags` is not null has been added.

Unfortunately it isn't possible to write a regression test for this
because the behavior depends on having at least some bounds checks
disabled by default, which is not desirable behavior. While techinically
we could add a hidden flag to change the default for the purposes of
testing the added complexity of doing this doesn't really justify the
added test coverage.

This version of the patch also guards assigning to
`Opts.BoundsSafetyBringUpMissingChecks` in
`CompilerInvocation::ParseLangArgs` so that it is only assigned to
when `-fbounds-safety` is enabled. This is done for several reasons

* It avoids unnecessarily parsing the `-fbounds-safety-bringup-missing-checks=` flags
  when `-fbounds-safety` is disabled.
* Isolates code built without `-fbounds-safety` from bugs in
  `ParseBoundsSafetyNewChecksMaskFromArgs`.
* It is more consistent with `CompilerInvocationBase::GenerateLangArgs`
  which only emits `-fbounds-safety-bring-checks=` when
  `-fbounds-safety` is enabled.

---

This adds warning diagnostics when any of the new bounds checks that can
be enabled with `-fbounds-safety-bringup-missing-checks=batch_0` are
disabled.

If all bounds checks in the batch are disabled a single diagnostic is
emitted. If only some of the bounds checks in the batch are disabled
then a diagnostic is emitted for each disabled bounds check. The
implementation will either suggest enabling a batch of checks (e.g.
`-fbounds-safety-bringup-missing-checks=batch_0`) or will suggest
removing a flag that is explicitly disabling a check (e.g.
`-fno-bounds-safety-bringup-missing-checks=access_size`).

The current implementation supports there being multple batches of
checks. However, there is currently only one batch (`batch_0`).

I originally tried to emit these warnings in the frontend. Unfortunately
it turns out warning suppression (i.e.
`-Wno-bounds-safety-legacy-checks-enabled`) and `-Werror` don't work
correctly if warnings are emitted from the frontend (rdar://152730261).
To workaround this the `-fbounds-safety-bringup-missing-checks=` flags
are now also parsed in the Driver and at this point (and only this
point) diagnostics for missing checks are emitted.

The intention is to make these warnings be errors eventually.

rdar://150805550

Author: delcypher

clang/include/clang/Basic/DiagnosticFrontendKinds.td

@@ -402,6 +402,36 @@ def warn_bounds_safety_adoption_mode_ignored : Warning<
   "-fbounds-safety-adoption-mode without -fbounds-safety is "
   "ignored">,
   InGroup<BoundsSafetyAdoptionModeIgnored>;
+
+def warn_bounds_safety_new_checks_none : Warning<
+  "compiling with legacy -fbounds-safety bounds checks is deprecated;"
+  " compile with -fbounds-safety-bringup-missing-checks=%0 to use the "
+  "new bound checks">,
+  InGroup<BoundsSafetyLegacyChecksEnabled>;
+def warn_bounds_safety_new_checks_mixed : Warning<
+  "compiling with \"%select{"
+    "access_size|" // BS_CHK_AccessSize
+    "indirect_count_update|" // BS_CHK_IndirectCountUpdate
+    "return_size|" // BS_CHK_ReturnSize
+    "ended_by_lower_bound|" // BS_CHK_EndedByLowerBound
+    "compound_literal_init|" // BS_CHK_CompoundLiteralInit
+    "libc_attributes|" // BS_CHK_LibCAttributes
+    "array_subscript_agg" // BS_CHK_ArraySubscriptAgg
+  "}0\" bounds check disabled is deprecated;"
+  " %select{"
+    "compile with -fbounds-safety-bringup-missing-checks=%2|"
+    "remove -fno-bounds-safety-bringup-missing-checks=%select{"
+      "access_size|" // BS_CHK_AccessSize
+      "indirect_count_update|" // BS_CHK_IndirectCountUpdate
+      "return_size|" // BS_CHK_ReturnSize
+      "ended_by_lower_bound|" // BS_CHK_EndedByLowerBound
+      "compound_literal_init|" // BS_CHK_CompoundLiteralInit
+      "libc_attributes|" // BS_CHK_LibCAttributes
+      "array_subscript_agg" // BS_CHK_ArraySubscriptAgg
+      "}0"
+    "|}1"
+  " to enable the new bound checks">,
+  InGroup<BoundsSafetyLegacyChecksEnabled>;
 // TO_UPSTREAM(BoundsSafety) OFF
 
 let CategoryName = "Instrumentation Issue" in {

clang/include/clang/Basic/DiagnosticGroups.td

@@ -1679,6 +1679,8 @@ def BoundsSafetyStrictTerminatedByCast
     : DiagGroup<"bounds-safety-strict-terminated-by-cast">;
 def BoundsSafetyCountAttrArithConstantCount :
   DiagGroup<"bounds-safety-externally-counted-ptr-arith-constant-count">;
+def BoundsSafetyLegacyChecksEnabled :
+  DiagGroup<"bounds-safety-legacy-checks-enabled">;
 // TO_UPSTREAM(BoundsSafety) OFF
 
 // -fbounds-safety and bounds annotation related warnings

clang/include/clang/Basic/LangOptions.h

@@ -471,6 +471,8 @@ class LangOptionsBase {
     BS_CHK_CompoundLiteralInit = 1 << 4, // rdar://110871666
     BS_CHK_LibCAttributes = 1 << 5,      // rdar://84733153
     BS_CHK_ArraySubscriptAgg = 1 << 6,   // rdar://145020583
+
+    BS_CHK_MaxMask = BS_CHK_ArraySubscriptAgg,
   };
   using BoundsSafetyNewChecksMaskIntTy =
       std::underlying_type_t<BoundsSafetyNewChecks>;

clang/include/clang/Driver/BoundsSafetyArgs.h

@@ -16,7 +16,8 @@ namespace driver {
 
 LangOptions::BoundsSafetyNewChecksMaskIntTy
 ParseBoundsSafetyNewChecksMaskFromArgs(const llvm::opt::ArgList &Args,
-                                       DiagnosticsEngine *Diags);
+                                       DiagnosticsEngine *Diags,
+                                       bool DiagnoseMissingChecks);
 } // namespace driver
 } // namespace clang
 

clang/lib/Driver/BoundsSafetyArgs.cpp

@@ -7,7 +7,11 @@
 //===----------------------------------------------------------------------===//
 #include "clang/Driver/BoundsSafetyArgs.h"
 #include "clang/Basic/DiagnosticDriver.h"
+#include "clang/Basic/DiagnosticFrontend.h"
 #include "clang/Driver/Options.h"
+#include "llvm/ADT/StringSet.h"
+#include "llvm/ADT/bit.h"
+#include <array>
 
 using namespace llvm::opt;
 using namespace clang::driver::options;
@@ -16,15 +20,105 @@ namespace clang {
 
 namespace driver {
 
+static void DiagnoseDisabledBoundsSafetyChecks(
+    LangOptions::BoundsSafetyNewChecksMaskIntTy EnabledChecks,
+    DiagnosticsEngine *Diags,
+    LangOptions::BoundsSafetyNewChecksMaskIntTy
+        IndividualChecksExplicitlyDisabled) {
+  assert(Diags);
+  struct BoundsCheckBatch {
+    const char *Name;
+    LangOptionsBase::BoundsSafetyNewChecksMaskIntTy Checks;
+  };
+
+  // Batches of checks should be ordered with newest first
+  std::array<BoundsCheckBatch, 2> Batches = {
+      {// We deliberately don't include `all` here because that batch
+       // isn't stable over time (unlike batches like `batch_0`) so we
+       // don't want to suggest users start using it.
+       {"batch_0",
+        LangOptions::getBoundsSafetyNewChecksMaskForGroup("batch_0")},
+       {"none", LangOptions::BS_CHK_None}}};
+
+  LangOptionsBase::BoundsSafetyNewChecksMaskIntTy DiagnosedDisabledChecks =
+      LangOptions::BS_CHK_None;
+
+  // Loop over all batches except "none"
+  for (size_t BatchIdx = 0; BatchIdx < Batches.size() - 1; ++BatchIdx) {
+    auto ChecksInCurrentBatch = Batches[BatchIdx].Checks;
+    auto ChecksInOlderBatch = Batches[BatchIdx + 1].Checks;
+    auto ChecksInCurrentBatchOnly = ChecksInCurrentBatch & ~ChecksInOlderBatch;
+    const auto *CurrentBatchName = Batches[BatchIdx].Name;
+
+    if ((EnabledChecks & ChecksInCurrentBatchOnly) == ChecksInCurrentBatchOnly)
+      continue; // All checks in batch are enabled. Nothing to diagnose.
+
+    // Diagnose disabled bounds checks
+
+    if ((EnabledChecks & ChecksInCurrentBatchOnly) == 0) {
+      // None of the checks in the current batch are enabled. Diagnose this
+      // once for all the checks in the batch.
+      if ((DiagnosedDisabledChecks & ChecksInCurrentBatchOnly) !=
+          ChecksInCurrentBatchOnly) {
+        Diags->Report(diag::warn_bounds_safety_new_checks_none)
+            << CurrentBatchName;
+        DiagnosedDisabledChecks |= ChecksInCurrentBatchOnly;
+      }
+      continue;
+    }
+
+    // Some (but not all) checks are disabled in the current batch. Iterate over
+    // each check in the batch and emit a diagnostic for each disabled check
+    // in the batch.
+    assert(ChecksInCurrentBatch > 0);
+    auto FirstCheckInBatch = 1 << llvm::countr_zero(ChecksInCurrentBatch);
+    for (size_t CheckBit = FirstCheckInBatch;
+         CheckBit <= LangOptions::BS_CHK_MaxMask; CheckBit <<= 1) {
+      assert(CheckBit != 0);
+      if ((CheckBit & ChecksInCurrentBatch) == 0)
+        continue; // Check not in batch
+
+      if (EnabledChecks & CheckBit)
+        continue; // Check is active
+
+      // Diagnose disabled check
+      if (!(DiagnosedDisabledChecks & CheckBit)) {
+        size_t CheckNumber = llvm::countr_zero(CheckBit);
+        // If we always suggested enabling the current batch that
+        // could be confusing if the user passed something like
+        // `-fbounds-safety-bringup-missing-checks=batch_0
+        // -fno-bounds-safety-bringup-missing-checks=access_size`. To avoid
+        // this we detect when the check corresponding to `CheckBit` has been
+        // explicitly disabled on the command line and in that case we suggeset
+        // removing the flag.
+        bool SuggestRemovingFlag =
+            CheckBit & IndividualChecksExplicitlyDisabled;
+        Diags->Report(diag::warn_bounds_safety_new_checks_mixed)
+            << CheckNumber << SuggestRemovingFlag << CurrentBatchName;
+        DiagnosedDisabledChecks |= CheckBit;
+      }
+    }
+  }
+}
+
 LangOptions::BoundsSafetyNewChecksMaskIntTy
 ParseBoundsSafetyNewChecksMaskFromArgs(const llvm::opt::ArgList &Args,
-                                       DiagnosticsEngine *Diags) {
+                                       DiagnosticsEngine *Diags,
+                                       bool DiagnoseMissingChecks) {
+  assert((!DiagnoseMissingChecks || Diags) &&
+         "Cannot diagnose missing checks when Diags is a nullptr");
+  LangOptions::BoundsSafetyNewChecksMaskIntTy
+      IndividualChecksExplicitlyDisabled = LangOptions::BS_CHK_None;
   auto FilteredArgs =
       Args.filtered(OPT_fbounds_safety_bringup_missing_checks_EQ,
                     OPT_fno_bounds_safety_bringup_missing_checks_EQ);
   if (FilteredArgs.empty()) {
     // No flags present. Use the default
-    return LangOptions::getDefaultBoundsSafetyNewChecksMask();
+    auto Result = LangOptions::getDefaultBoundsSafetyNewChecksMask();
+    if (DiagnoseMissingChecks && Diags)
+      DiagnoseDisabledBoundsSafetyChecks(Result, Diags,
+                                         IndividualChecksExplicitlyDisabled);
+    return Result;
   }
 
   // If flags are present then start with BS_CHK_None as the initial mask and
@@ -35,6 +129,11 @@ ParseBoundsSafetyNewChecksMaskFromArgs(const llvm::opt::ArgList &Args,
   // All the options will be applied as masks in the command line order, to make
   // it easier to enable all but certain checks (or disable all but certain
   // checks).
+  const auto Batch0Checks =
+      LangOptions::getBoundsSafetyNewChecksMaskForGroup("batch_0");
+  const auto AllChecks =
+      LangOptions::getBoundsSafetyNewChecksMaskForGroup("all");
+  bool Errors = false;
   for (const Arg *A : FilteredArgs) {
     for (const char *Value : A->getValues()) {
       std::optional<LangOptions::BoundsSafetyNewChecksMaskIntTy> Mask =
@@ -51,17 +150,16 @@ ParseBoundsSafetyNewChecksMaskFromArgs(const llvm::opt::ArgList &Args,
               .Case("libc_attributes", LangOptions::BS_CHK_LibCAttributes)
               .Case("array_subscript_agg",
                     LangOptions::BS_CHK_ArraySubscriptAgg)
-              .Case("all",
-                    LangOptions::getBoundsSafetyNewChecksMaskForGroup("all"))
-              .Case(
-                  "batch_0",
-                  LangOptions::getBoundsSafetyNewChecksMaskForGroup("batch_0"))
+              .Case("all", AllChecks)
+              .Case("batch_0", Batch0Checks)
               .Case("none", LangOptions::BS_CHK_None)
               .Default(std::nullopt);
+
       if (!Mask) {
         if (Diags)
           Diags->Report(diag::err_drv_invalid_value)
               << A->getSpelling() << Value;
+        Errors = true;
         break;
       }
 
@@ -81,6 +179,7 @@ ParseBoundsSafetyNewChecksMaskFromArgs(const llvm::opt::ArgList &Args,
               << A->getSpelling() << Value
               << (IsPosFlag ? "-fno-bounds-safety-bringup-missing-checks"
                             : "-fbounds-safety-bringup-missing-checks");
+        Errors = true;
         break;
       }
 
@@ -91,8 +190,25 @@ ParseBoundsSafetyNewChecksMaskFromArgs(const llvm::opt::ArgList &Args,
             OPT_fno_bounds_safety_bringup_missing_checks_EQ));
         Result &= ~(*Mask);
       }
+
+      // Update which checks have been explicitly disabled. E.g.
+      // `-fno-bounds-safety-bringup-missing-checks=access_size`.
+      if (llvm::has_single_bit(*Mask)) {
+        // A single check was enabled/disabled
+        if (IsPosFlag)
+          IndividualChecksExplicitlyDisabled &= ~(*Mask);
+        else
+          IndividualChecksExplicitlyDisabled |= *Mask;
+      } else {
+        // A batch of checks were enabled/disabled. Any checks in that batch
+        // are no longer explicitly set.
+        IndividualChecksExplicitlyDisabled &= ~(*Mask);
+      }
     }
   }
+  if (DiagnoseMissingChecks && Diags && !Errors)
+    DiagnoseDisabledBoundsSafetyChecks(Result, Diags,
+                                       IndividualChecksExplicitlyDisabled);
   return Result;
 }
 

clang/lib/Driver/ToolChains/Clang.cpp

@@ -25,6 +25,7 @@
 #include "clang/Basic/Version.h"
 #include "clang/Config/config.h"
 #include "clang/Driver/Action.h"
+#include "clang/Driver/BoundsSafetyArgs.h" // TO_UPSTREAM(BoundsSafety)
 #include "clang/Driver/CommonArgs.h"
 #include "clang/Driver/Distro.h"
 #include "clang/Driver/InputInfo.h"
@@ -7641,6 +7642,15 @@ void Clang::ConstructJob(Compilation &C, const JobAction &Job,
       Args.addAllArgs(
           CmdArgs, {options::OPT_fbounds_safety_bringup_missing_checks_EQ,
                     options::OPT_fno_bounds_safety_bringup_missing_checks_EQ});
+
+      // Validate the `-fbounds-safety-bringup-missing-checks` flags and emit
+      // warnings for disabled checks. We do this here because if we emit
+      // warnings in CC1 (where `ParseBoundsSafetyNewChecksMaskFromArgs` is also
+      // called) they cannot be suppressed and ignore `-Werror`
+      // (rdar://152730261).
+      (void)ParseBoundsSafetyNewChecksMaskFromArgs(
+          Args, &D.getDiags(),
+          /*DiagnoseMissingChecks=*/true);
     }
   }
 

clang/lib/Driver/ToolChains/Darwin.cpp

@@ -1336,7 +1336,8 @@ void DarwinClang::addClangTargetOptions(
                                 options::OPT_fno_bounds_safety, false)) {
     LangOptions::BoundsSafetyNewChecksMaskIntTy NewChecks =
         ParseBoundsSafetyNewChecksMaskFromArgs(DriverArgs,
-                                               /*DiagnosticsEngine=*/nullptr);
+                                               /*DiagnosticsEngine=*/nullptr,
+                                               /*DiagnoseMissingChecks=*/false);
     if (NewChecks & LangOptions::BS_CHK_LibCAttributes) {
       bool TargetWithoutUserspaceLibc = false;
       if (getTriple().isOSDarwin() && !TargetWithoutUserspaceLibc) {

clang/lib/Frontend/CompilerInvocation.cpp

@@ -4635,9 +4635,15 @@ bool CompilerInvocation::ParseLangArgs(LangOptions &Opts, ArgList &Args,
       Args.hasFlag(OPT_ftrigraphs, OPT_fno_trigraphs, Opts.Trigraphs);
 
   /*TO_UPSTREAM(BoundsSafety) ON*/
-  // Parse the enabled checks and emit any necessary diagnostics
-  Opts.BoundsSafetyBringUpMissingChecks =
-      ParseBoundsSafetyNewChecksMaskFromArgs(Args, &Diags);
+  if (Opts.BoundsSafety) {
+    // Parse the enabled checks and emit any necessary diagnostics.
+    // We do not diagnose missing checks here because warnings emitted in this
+    // context cannot be surpressed and don't respect `-Werror`
+    // (rdar://152730261). Instead we warn about missing checks in the driver.
+    Opts.BoundsSafetyBringUpMissingChecks =
+        ParseBoundsSafetyNewChecksMaskFromArgs(Args, &Diags,
+                                               /*DiagnoseMissingChecks=*/false);
+  }
 
   // -fbounds-safety should be automatically marshalled into `Opts` in
   // GenerateFrontendArgs() via `LangOpts<"BoundsSafety">` on