[BoundsSafety] Fix missing bounds check when indexing into a buffer of aggregates

Previously bounds checks where missing for a case like:

```
struct F {int x;};
struct F access(struct F* __bidi_indexable f, size_t idx) {
  // the index operation should be bounds checked.
  return f[idx];
```

Note this only happened specifically for ArraySubscriptExpr and **only**
when the whole aggregate was being loaded with no further operations.
E.g. `f[idx].x` was already correctly bounds checked.

The missing bounds check is guarded using
`-fbounds-safety-bringup-missing-checks=array_subscript_agg` to avoid
breaking existing users.

The bug occured because when `AggExprEmitter::VisitArraySubscriptExpr`
calls `EmitAggLoadOfLValue`, `EmitAggLoadOfLValue` didn't call
`EmitCheckedLValue` and instead called `EmitLValue`.

In this patch the value of `Checked` even when the `array_subscript_agg`
is enabled it still does

```
Checked |= E->getType()->isPointerTypeWithBounds();
```

which was the previous condition for calling `EmitCheckedLValue`. This
is because there's an interaction with UBSan there that we might
accidently change if setting `Checked` in this way was removed.  We
should investigate this in the future and this is tracked by
rdar://145257962.

rdar://145020583

Author: delcypher

clang/include/clang/Basic/LangOptions.def

@@ -548,7 +548,7 @@ LANGOPT(BoundsAttributesCXXExperimental, 1, 0, "Experimental bounds attributes f
 LANGOPT(BoundsAttributesObjCExperimental, 1, 0, "Experimental bounds attributes for Objective-C")
 LANGOPT(BoundsSafetyRelaxedSystemHeaders, 1, 1,
         "Relax bounds safety assignment rules in system headers")
-ENUM_LANGOPT(BoundsSafetyBringUpMissingChecks, BoundsSafetyNewChecksMask, 6, BS_CHK_Default,
+ENUM_LANGOPT(BoundsSafetyBringUpMissingChecks, BoundsSafetyNewChecksMask, 7, BS_CHK_Default,
         "Bring up new -fbounds-safety run-time checks")
 /* TO_UPSTREAM(BoundsSafety) OFF*/
 

clang/include/clang/Basic/LangOptions.h

@@ -454,13 +454,16 @@ class LangOptionsBase {
     BS_CHK_EndedByLowerBound = 1 << 3,   // rdar://91596663
     BS_CHK_CompoundLiteralInit = 1 << 4, // rdar://110871666
     BS_CHK_LibCAttributes = 1 << 5,      // rdar://84733153
+    BS_CHK_ArraySubscriptAgg = 1 << 6,   // rdar://145020583
 
     BS_CHK_All = BS_CHK_AccessSize | BS_CHK_IndirectCountUpdate |
                  BS_CHK_ReturnSize | BS_CHK_EndedByLowerBound |
-                 BS_CHK_CompoundLiteralInit | BS_CHK_LibCAttributes,
+                 BS_CHK_CompoundLiteralInit | BS_CHK_LibCAttributes |
+                 BS_CHK_ArraySubscriptAgg,
 
     // This sets the default value assumed by clang if no
-    // `-fbounds-safety-bringup-missing-checks` flags are passed to clang.
+    // `-fbounds-safety-bringup-missing-checks` flags are
+    // passed to clang.
     BS_CHK_Default = BS_CHK_None,
   };
 

clang/include/clang/Driver/Options.td

@@ -1964,12 +1964,12 @@ defm bounds_safety_adoption_mode : BoolFOption<"bounds-safety-adoption-mode",
 def fbounds_safety_bringup_missing_checks_EQ : CommaJoined<["-"], "fbounds-safety-bringup-missing-checks=">, Group<f_Group>,
   MetaVarName<"<check>">,
   Visibility<[ClangOption, CC1Option]>,
-  HelpText<"Enable a set of new -fbounds-safety run-time checks, (option: access_size, indirect_count_update, return_size, ended_by_lower_bound, compound_literal_init, libc_attributes, all)">;
+  HelpText<"Enable a set of new -fbounds-safety run-time checks, (option: access_size, indirect_count_update, return_size, ended_by_lower_bound, compound_literal_init, libc_attributes, array_subscript_agg, all)">;
 
 def fno_bounds_safety_bringup_missing_checks_EQ : CommaJoined<["-"], "fno-bounds-safety-bringup-missing-checks=">, Group<f_Group>,
   MetaVarName<"<check>">,
   Visibility<[ClangOption, CC1Option]>,
-  HelpText<"Disable a set of new -fbounds-safety run-time checks, (option: access_size, indirect_count_update, return_size, ended_by_lower_bound, compound_literal_init, libc_attributes, all)">;
+  HelpText<"Disable a set of new -fbounds-safety run-time checks, (option: access_size, indirect_count_update, return_size, ended_by_lower_bound, compound_literal_init, libc_attributes, array_subscript_agg, all)">;
 
 def fbounds_safety_bringup_missing_checks : Flag<["-"], "fbounds-safety-bringup-missing-checks">, Group<f_Group>,
   Visibility<[ClangOption]>,

clang/lib/CodeGen/CGExprAgg.cpp

@@ -81,7 +81,9 @@ class AggExprEmitter : public StmtVisitor<AggExprEmitter> {
   /// EmitAggLoadOfLValue - Given an expression with aggregate type that
   /// represents a value lvalue, this method emits the address of the lvalue,
   /// then loads the result into DestPtr.
-  void EmitAggLoadOfLValue(const Expr *E);
+  /* TO_UPSTREAM(BoundsSafety) ON */
+  void EmitAggLoadOfLValue(const Expr *E, bool Checked = false);
+  /* TO_UPSTREAM(BoundsSafety) OFF */
 
   /// EmitFinalDestCopy - Perform the final copy to DestPtr, if desired.
   /// SrcIsRValue is true if source comes from an RValue.
@@ -171,7 +173,9 @@ class AggExprEmitter : public StmtVisitor<AggExprEmitter> {
   void VisitStringLiteral(StringLiteral *E) { EmitAggLoadOfLValue(E); }
   void VisitCompoundLiteralExpr(CompoundLiteralExpr *E);
   void VisitArraySubscriptExpr(ArraySubscriptExpr *E) {
-    EmitAggLoadOfLValue(E);
+    /* TO_UPSTREAM(BoundsSafety) ON */
+    EmitAggLoadOfLValue(E, E->getBase()->getType()->isPointerTypeWithBounds());
+    /* TO_UPSTREAM(BoundsSafety) OFF */
   }
   void VisitPredefinedExpr(const PredefinedExpr *E) {
     EmitAggLoadOfLValue(E);
@@ -372,10 +376,30 @@ AggExprEmitter::WidePointerElemCallback AggExprEmitter::DefaultElemCallback =
 /// EmitAggLoadOfLValue - Given an expression with aggregate type that
 /// represents a value lvalue, this method emits the address of the lvalue,
 /// then loads the result into DestPtr.
-void AggExprEmitter::EmitAggLoadOfLValue(const Expr *E) {
+void AggExprEmitter::EmitAggLoadOfLValue(const Expr *E, bool Checked) {
   /*TO_UPSTREAM(BoundsSafety) ON*/
-  LValue LV = E->getType()->isPointerTypeWithBounds() ?
-      CGF.EmitCheckedLValue(E, CodeGenFunction::TCK_Load) : CGF.EmitLValue(E);
+  if (CGF.getLangOpts().hasNewBoundsSafetyCheck(
+          clang::LangOptionsBase::BS_CHK_ArraySubscriptAgg)) {
+    // TODO(dliew): Modifying `Checked` should probably be removed.
+    // Calling `EmitCheckedLValue` does two things:
+    //
+    // 1. If `E` is an ArraySubscriptExpr then emits bound checks if UBSan is
+    //    on or if the base pointer has bounds information
+    // 2. Calls `CodeGenFunction::EmitTypeCheck()` in some cases.
+    //
+    // (1.) is already handled by `AggExprEmitter::VisitArraySubscriptExpr()` so
+    // modifying `Checked` isn't need for that. However, (2.) adds UBSan type
+    // checks.
+    //
+    // We should audit this interaction with UBSan to see if its safe to remove
+    // setting `Checked`. (rdar://145257962).
+    Checked |= E->getType()->isPointerTypeWithBounds();
+  } else {
+    // Preserve old buggy behavior
+    Checked = E->getType()->isPointerTypeWithBounds();
+  }
+  LValue LV = Checked ? CGF.EmitCheckedLValue(E, CodeGenFunction::TCK_Load)
+                      : CGF.EmitLValue(E);
   /*TO_UPSTREAM(BoundsSafety) OFF*/
 
   // If the type of the l-value is atomic, then do an atomic load.

clang/lib/Driver/BoundsSafetyArgs.cpp

@@ -49,6 +49,8 @@ ParseBoundsSafetyNewChecksMaskFromArgs(const llvm::opt::ArgList &Args,
               .Case("compound_literal_init",
                     LangOptions::BS_CHK_CompoundLiteralInit)
               .Case("libc_attributes", LangOptions::BS_CHK_LibCAttributes)
+              .Case("array_subscript_agg",
+                    LangOptions::BS_CHK_ArraySubscriptAgg)
               .Case("all", LangOptions::BS_CHK_All)
               .Case("none", LangOptions::BS_CHK_None)
               .Default(std::nullopt);

clang/lib/Frontend/CompilerInvocation.cpp

@@ -4056,6 +4056,8 @@ void CompilerInvocationBase::GenerateLangArgs(const LangOptions &Opts,
         Checks += "compound_literal_init,";
       if (Opts.hasNewBoundsSafetyCheck(LangOptions::BS_CHK_LibCAttributes))
         Checks += "libc_attributes,";
+      if (Opts.hasNewBoundsSafetyCheck(LangOptions::BS_CHK_ArraySubscriptAgg))
+        Checks += "array_subscript_agg";
       if (StringRef(Checks).ends_with(","))
         Checks.pop_back();
     }