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

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 current implementation supports there being multple batches of
checks. However, there is currently only one batch (`batch_0`).

Unfortunately I've discovered these diagnostics don't respect `-Werror`
or `-Wno-bounds-safety-legacy-checks-enabled`. Fixing that is
out-of-scope for this patch and is tracked by rdar://152730261.

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

@@ -1670,6 +1670,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

@@ -454,6 +454,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/lib/Driver/BoundsSafetyArgs.cpp

@@ -7,7 +7,9 @@
 //===----------------------------------------------------------------------===//
 #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"
 
 using namespace llvm::opt;
 using namespace clang::driver::options;
@@ -16,15 +18,101 @@ namespace clang {
 
 namespace driver {
 
+template <class AllocatorTy = llvm::MallocAllocator>
+static void DiagnoseDisabledBoundsSafetyChecks(
+    LangOptions::BoundsSafetyNewChecksMaskIntTy EnabledChecks,
+    DiagnosticsEngine *Diags,
+    LangOptions::BoundsSafetyNewChecksMaskIntTy
+        IndividualChecksExplicitlyDisabled) {
+  struct BoundsCheckBatch {
+    const char *Name;
+    LangOptionsBase::BoundsSafetyNewChecksMaskIntTy Checks;
+  };
+
+  // Batches of checks should be ordered with newest first
+  BoundsCheckBatch 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}};
+
+  ArrayRef<BoundsCheckBatch> BatchesAR(Batches);
+  LangOptionsBase::BoundsSafetyNewChecksMaskIntTy DiagnosedDisabledChecks =
+      LangOptions::BS_CHK_None;
+
+  // Loop over all batches except "none"
+  for (size_t BatchIdx = 0; BatchIdx < BatchesAR.size() - 1; ++BatchIdx) {
+    auto ChecksInCurrentBatch = BatchesAR[BatchIdx].Checks;
+    auto ChecksInOlderBatch = BatchesAR[BatchIdx + 1].Checks;
+    auto ChecksInCurrentBatchOnly = ChecksInCurrentBatch & ~ChecksInOlderBatch;
+    auto CurrentBatchName = BatchesAR[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 << __builtin_ctz(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 = __builtin_ctz(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) {
+  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();
+    DiagnoseDisabledBoundsSafetyChecks(Result, Diags,
+                                       IndividualChecksExplicitlyDisabled);
+    return Result;
   }
 
   // If flags are present then start with BS_CHK_None as the initial mask and
@@ -35,6 +123,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 +144,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 +173,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 +184,24 @@ ParseBoundsSafetyNewChecksMaskFromArgs(const llvm::opt::ArgList &Args,
             OPT_fno_bounds_safety_bringup_missing_checks_EQ));
         Result &= ~(*Mask);
       }
+
+      // Update which checks have been explicitly disabled
+      if (__builtin_popcount(*Mask) == 1) {
+        // 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 (Diags && !Errors)
+    DiagnoseDisabledBoundsSafetyChecks(Result, Diags,
+                                       IndividualChecksExplicitlyDisabled);
   return Result;
 }