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

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/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,103 @@ namespace clang {
 
 namespace driver {
 
+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
+  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();
+    DiagnoseDisabledBoundsSafetyChecks(Result, Diags,
+                                       IndividualChecksExplicitlyDisabled);
+    return Result;
   }
 
   // If flags are present then start with BS_CHK_None as the initial mask and
@@ -35,6 +127,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 +148,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 +177,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 +188,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"
@@ -7722,6 +7723,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

@@ -4575,9 +4575,13 @@ 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
+  // 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);
+      ParseBoundsSafetyNewChecksMaskFromArgs(Args, &Diags,
+                                             /*DiagnoseMissingChecks=*/false);
 
   // -fbounds-safety should be automatically marshalled into `Opts` in
   // GenerateFrontendArgs() via `LangOpts<"BoundsSafety">` on