[BoundsSafety] Move traps reasons for indexing to new trap reasons infrastructure

This patch moves generation of the trap reason strings for indexing into
pointers into the new trap reason infrastructure. We will need to move
many other trap reasons over to the new infrastructure but this is the
first we are moving over.

Previously array indexing emitted these very unspecifc trap reason
messages:

- `Dereferencing above bounds`
- `Deferencing below bounds`

Now we emit trap reasons that look like

- `indexing above upper bound in '<expr>'`
- `indexing below lower bound in '<expr>'`
- `indexing overflows address space in '<expr>'`

where `<expr>` is the ArraySubscriptExpr printed as a string (see test
cases for example).

There are several improvements here:

1. We say indexing rather than dereferencing which is more specific.
2. We emit a specific trap reason for address space overflow. Previously
   there was no distinction between the upper bound trap and the address
   space overflow trap.
3. We emit the textual representation of the ArraySubscriptExpr that
   triggered the bounds check failed. This makes the message very
   specific.

This new approach to emitting trap reasons will likely increase the size
of debug info. To give users control a new flag
`-fbounds-safety-debug-trap-trap-reasons` has been added which is
analogous to `-fsanitize-debug-trap-reasons` for UBSan. The flag takes
three values:

* `none` - Dont' emit any trap reasons
* `detailed` - Emit the new more detailed trap reasons (the default)
* `basic` - Emit the less descriptive trap reasons using the legacy
  infrastructure.

While working on this it became clear that emission of trap diagnostics
is more complicated than for UBSan become some of the context for where
we are emitting the trap exists in a different stackframe than the
location where we can actually emit the trap diagnostic. In
`EmitWidePtrArraySubscriptExpr` we don't know if we are emitting a
lower/upper bound/address space check. In `EmitBoundsSafetyBoundsCheck`
we don't know we are emitting a check for an ArraySubscriptExpr so there
isn't a function where we can emit the trap diagnostic that has all the
necessary information. The solution used in this patch is to re-use the
`PartialDiagnostic` class which essentially lets us partially construct
a diagnostic in one function and then pass it along to another function
to the actual where the trap diagnostic can be fully constructed.

Note in this implementation the "detailed" trap reason is always
constructed even if it later gets thrown away. There are several reasons
for doing this:

* For clang's diagnostics normally we typically don't write guards
  around them to check they are enabled (e.g. the warning might be
  actually disabled).
* While technically we could write guards around all the code that
  builds the TrapReason objects this will become repetitive very
  quickly. It's cleaner to just put the guard in this function.
* I'm also planning to use these TrapReason objects for the upcoming
  soft trap mode and I didn't want to put guards around their creation
  until I've figured out exactly how this is going to be implemented.
* This is also how its implemented for UBSan's trapping diagnostics
  right now. This is not a particularly strong argument because I'm the
  one who implemented that but at least upstream didn't object to me
  doing it this way.

rdar://158623471
(cherry picked from commit a4898c2)

Author: delcypher

clang/include/clang/Basic/CodeGenOptions.def

@@ -337,6 +337,8 @@ CODEGENOPT(BoundsSafetyUniqueTraps, 1, 0, Benign) ///< When true, merging of
                                                   /// -fbounds-safety trap
                                                   /// instructions will be
                                                   /// prevented.
+
+ENUM_CODEGENOPT(BoundsSafetyDebugTrapReasons, SanitizeDebugTrapReasonKind, 2, SanitizeDebugTrapReasonKind::Detailed, Benign) ///< Control how "trap reasons" are emitted in debug info
 /* TO_UPSTREAM(BoundsSafety) OFF*/
 
 /// Treat loops as finite: language, always, never.

clang/include/clang/Basic/DiagnosticBoundsSafetyTrapKinds.td

@@ -15,5 +15,18 @@ let CategoryName = "Bounds check failed" in {
 // trap reason strings.
 def trap_bs_fallback : Trap<"%0">;
 
+// TODO: Split these up once we know what the diagnostics strings needs to be.
+// Ideally we want to be able to generate trap codes and opt-remark strings
+// from these definitions too so we'll need to split them up when we do that.
+def trap_bs_upper_lower_overflow_bound : Trap<
+  "%enum_select<BoundsCheckContextKind>{"
+    "%Indexing{indexing}|"
+    "%TODO{TODO}"
+  "}0 %enum_select<BoundsSafetyPtrCheckKind>{"
+    "%Upper{above upper bound}|"
+    "%Overflow{overflows address space}|"
+    "%Lower{below lower bound"
+  "}}2 in %1">;
+
 }
 }

clang/include/clang/Driver/Options.td

@@ -2045,6 +2045,20 @@ defm bounds_safety_legacy_unique_traps : BoolOptionWithoutMarshalling<"f", "uniq
   NegFlag<SetFalse, [], [ClangOption],
     "legacy alias for -fno-bounds-safety-unique-traps">>;
 
+def fbounds_safety_debug_trap_reasons_EQ
+  : Joined<["-"], "fbounds-safety-debug-trap-reasons=">, Group<f_Group>,
+    Visibility<[ClangOption, CC1Option]>,
+    HelpText<"Set how trap reasons are emitted. "
+      "`none` - Not emitted. This gives the smallest debug info; "
+      "`basic` - Emit a fixed trap message per check type. This increases the "
+      "debug info size but not as much as `detailed`; "
+      "`detailed` - Emit a more detailed trap message. This increases the "
+      "debug info size the most. Default is `detailed`.">,
+    Values<"none,basic,detailed">,
+    NormalizedValuesScope<"CodeGenOptions::SanitizeDebugTrapReasonKind">,
+    NormalizedValues<["None", "Basic", "Detailed"]>,
+    MarshallingInfoEnum<CodeGenOpts<"BoundsSafetyDebugTrapReasons">, "Detailed">;
+
 // TO_UPSTREAM(BoundsSafety) OFF
 
 defm lifetime_safety : BoolFOption<

clang/lib/CodeGen/CGExpr.cpp

@@ -121,8 +121,8 @@ void CodeGenFunction::EmitPtrCastLECheck(llvm::Value *LHS, llvm::Value *RHS,
 
 void CodeGenFunction::EmitBoundsSafetyBoundsCheck(
     llvm::Type *ElemTy, llvm::Value *Ptr, llvm::Value *Upper,
-    llvm::Value *Lower, bool AcceptNullPtr,
-    BoundsSafetyTrapCtx::Kind TrapCtx) {
+    llvm::Value *Lower, bool AcceptNullPtr, BoundsSafetyTrapCtx::Kind TrapCtx,
+    PartialDiagnostic *PD) {
   if (!Upper && !Lower)
     return;
   assert(TrapCtx != BoundsSafetyTrapCtx::UNKNOWN);
@@ -157,16 +157,30 @@ void CodeGenFunction::EmitBoundsSafetyBoundsCheck(
       BoundsSafetyOptRemarkScope Scope(this, BNS_OR_PTR_GT_UPPER_BOUND);
       llvm::Value *OnePastTheEndPtr =
           Builder.CreateGEP(ElemTy, Ptr, llvm::ConstantInt::get(SizeTy, 1));
+
+      TrapReason OverflowTR;
+      TrapReason UpperBoundTR;
+      if (PD) {
+        CGM.BuildTrapReason(diag::trap_bs_upper_lower_overflow_bound,
+                            UpperBoundTR)
+            << *PD << diag::BoundsSafetyPtrCheckKind::Upper;
+        CGM.BuildTrapReason(diag::trap_bs_upper_lower_overflow_bound,
+                            OverflowTR)
+            << *PD << diag::BoundsSafetyPtrCheckKind::Overflow;
+      }
+
       // Emitting the upper bound check first since it's more
       // optimization-friendly. This is because the upper bound calculation (ptr
       // + size) is often marked 'inbounds' if 'ptr' is '__counted_by' or an
       // array decay of a sized array. This allows ConstraintElimination to use
       // this information to infer subsequent pointer arithmetic (ptr + i; where
       // 'i <= size') doesn't wrap.
       EmitBoundsSafetyTrapCheck(Builder.CreateICmpULE(OnePastTheEndPtr, Upper),
-                              BNS_TRAP_PTR_GT_UPPER_BOUND, TrapCtx);
+                                BNS_TRAP_PTR_GT_UPPER_BOUND, TrapCtx,
+                                PD ? &UpperBoundTR : nullptr);
       EmitBoundsSafetyTrapCheck(Builder.CreateICmpULE(Ptr, OnePastTheEndPtr),
-                              BNS_TRAP_PTR_GT_UPPER_BOUND, TrapCtx);
+                                BNS_TRAP_PTR_GT_UPPER_BOUND, TrapCtx,
+                                PD ? &OverflowTR : nullptr);
     } else {
       // Path where the size of the access is assumed to be 1 byte. This is used
       // for
@@ -188,14 +202,31 @@ void CodeGenFunction::EmitBoundsSafetyBoundsCheck(
       //
       BoundsSafetyOptRemarkScope Scope(this, BNS_OR_PTR_GE_UPPER_BOUND);
       llvm::Value *Check = Builder.CreateICmpULT(Ptr, Upper);
-      EmitBoundsSafetyTrapCheck(Check, BNS_TRAP_PTR_GE_UPPER_BOUND, TrapCtx);
+
+      TrapReason UpperTR;
+      if (PD) {
+        CGM.BuildTrapReason(diag::trap_bs_upper_lower_overflow_bound, UpperTR)
+            << *PD << diag::BoundsSafetyPtrCheckKind::Upper;
+      }
+
+      EmitBoundsSafetyTrapCheck(Check, BNS_TRAP_PTR_GE_UPPER_BOUND, TrapCtx,
+                                PD ? &UpperTR : nullptr);
     }
   }
 
   if (Lower) {
     BoundsSafetyOptRemarkScope Scope(this, BNS_OR_PTR_LT_LOWER_BOUND);
     llvm::Value *Check = Builder.CreateICmpUGE(Ptr, Lower);
-    EmitBoundsSafetyTrapCheck(Check, BNS_TRAP_PTR_LT_LOWER_BOUND, TrapCtx);
+
+    TrapReason LowerBoundTR;
+    if (PD) {
+      CGM.BuildTrapReason(diag::trap_bs_upper_lower_overflow_bound,
+                          LowerBoundTR)
+          << *PD << diag::BoundsSafetyPtrCheckKind::Lower;
+    }
+
+    EmitBoundsSafetyTrapCheck(Check, BNS_TRAP_PTR_LT_LOWER_BOUND, TrapCtx,
+                              PD ? &LowerBoundTR : nullptr);
   }
   if (NullCheckBranch)
     NullCheckBranch->setSuccessor(1, Builder.GetInsertBlock());
@@ -4591,9 +4622,15 @@ void CodeGenFunction::EmitTrapCheck(llvm::Value *Checked,
   llvm::DILocation *TrapLocation = Builder.getCurrentDebugLocation();
   llvm::StringRef TrapMessage;
   llvm::StringRef TrapCategory;
-  auto DebugTrapReasonKind = CGM.getCodeGenOpts().getSanitizeDebugTrapReasons();
-
   /* TO_UPSTREAM(BoundsSafety) ON*/
+  CodeGenOptions::SanitizeDebugTrapReasonKind DebugTrapReasonKind;
+  if (CheckHandlerID == SanitizerHandler::BoundsSafety) {
+    DebugTrapReasonKind =
+        CGM.getCodeGenOpts().getBoundsSafetyDebugTrapReasons();
+  } else {
+    DebugTrapReasonKind = CGM.getCodeGenOpts().getSanitizeDebugTrapReasons();
+  }
+
   if (TR && !TR->isEmpty() &&
       (DebugTrapReasonKind ==
            CodeGenOptions::SanitizeDebugTrapReasonKind::Detailed ||
@@ -4607,9 +4644,8 @@ void CodeGenFunction::EmitTrapCheck(llvm::Value *Checked,
   }
 
   if (getDebugInfo() && !(TrapMessage.empty() && TrapCategory.empty()) &&
-      (DebugTrapReasonKind !=
-           CodeGenOptions::SanitizeDebugTrapReasonKind::None ||
-       CheckHandlerID == SanitizerHandler::BoundsSafety) &&
+      DebugTrapReasonKind !=
+          CodeGenOptions::SanitizeDebugTrapReasonKind::None &&
       TrapLocation) {
     TrapLocation = getDebugInfo()->CreateTrapFailureMessageFor(
         TrapLocation, TrapCategory, TrapMessage);
@@ -4705,10 +4741,11 @@ void CodeGenFunction::EmitBoundsSafetyTrapCheck(
   auto OptRemark = GetBoundsSafetyOptRemarkForTrap(kind);
   assert(BoundsSafetyOptRemarkScope::InScope(this, OptRemark));
 
-  // Fallback: If a TrapReason object isn't provided use the legacy approach
-  // for constructing
+  // Fallback: If a TrapReason object isn't provided or we are asked to provide
+  // a "basic" description.
   TrapReason TempTR;
-  if (!TR) {
+  if (!TR || CGM.getCodeGenOpts().getBoundsSafetyDebugTrapReasons() ==
+                 CodeGenOptions::SanitizeDebugTrapReasonKind::Basic) {
     CGM.BuildTrapReason(diag::trap_bs_fallback, TempTR)
         << GetBoundsSafetyTrapMessageSuffix(kind, TrapCtx);
   }
@@ -4718,7 +4755,8 @@ void CodeGenFunction::EmitBoundsSafetyTrapCheck(
   // caches basic blocks that contain instructions that need annotating.
   EmitTrapCheck(Checked, SanitizerHandler::BoundsSafety,
                 /*NoMerge=*/CGM.getCodeGenOpts().BoundsSafetyUniqueTraps,
-                TR ? TR : &TempTR, GetBoundsSafetyOptRemarkString(OptRemark));
+                TempTR.isEmpty() ? TR : &TempTR,
+                GetBoundsSafetyOptRemarkString(OptRemark));
 }
 
 llvm::CallInst *CodeGenFunction::EmitTrapCall(llvm::Intrinsic::ID IntrID) {
@@ -5065,9 +5103,11 @@ CodeGenFunction::EmitWidePtrArraySubscriptExpr(const ArraySubscriptExpr *E,
     }
     assert(!!Upper && !!Lower);
     llvm::Type *ElemTy = ConvertTypeForMem(E->getType());
+    auto PD = CGM.BuildPartialTrapReason();
+    PD << diag::BoundsCheckContextKind::Indexing << E;
     EmitBoundsSafetyBoundsCheck(ElemTy, Addr.getBasePointer(), Upper, Lower,
-                             /*AcceptNullPtr=*/false,
-                             /*TrapCtx=*/BoundsSafetyTrapCtx::DEREF);
+                                /*AcceptNullPtr=*/false,
+                                /*TrapCtx=*/BoundsSafetyTrapCtx::DEREF, &PD);
   }
   return MakeAddrLValue(Addr, E->getType(), BaseInfo, TBAAInfo);
 }

clang/lib/CodeGen/CodeGenFunction.h

@@ -2966,7 +2966,8 @@ class CodeGenFunction : public CodeGenTypeCache {
   void EmitBoundsSafetyBoundsCheck(
       llvm::Type *ElemTy, llvm::Value *Ptr, llvm::Value *Upper,
       llvm::Value *Lower, bool AcceptNullPt = false,
-      BoundsSafetyTrapCtx::Kind TrapCtx = BoundsSafetyTrapCtx::UNKNOWN);
+      BoundsSafetyTrapCtx::Kind TrapCtx = BoundsSafetyTrapCtx::UNKNOWN,
+      PartialDiagnostic *PD = nullptr);
   void EmitBoundsSafetyRangeCheck(
       llvm::Value *LowerBound, llvm::Value *LowerAccess,
       llvm::Value *UpperAccess, llvm::Value *UpperBound,

clang/lib/CodeGen/CodeGenModule.h

@@ -1839,6 +1839,15 @@ class CodeGenModule : public CodeGenTypeCache {
     return TrapReasonBuilder(&getDiags(), DiagID, TR);
   }
 
+  /* TO_UPSTREAM(BoundsSafety) ON*/
+  PartialDiagnostic BuildPartialTrapReason() {
+    // When building trap reasons we sometimes don't know exactly
+    // which diagnostic is going to be emitted so we just specify
+    // `0` as place holder.
+    return PartialDiagnostic(0, getContext().getDiagAllocator());
+  }
+  /* TO_UPSTREAM(BoundsSafety) OFF*/
+
 private:
   bool shouldDropDLLAttribute(const Decl *D, const llvm::GlobalValue *GV) const;
 

clang/lib/Driver/ToolChains/Clang.cpp

@@ -7136,6 +7136,8 @@ void Clang::ConstructJob(Compilation &C, const JobAction &Job,
       CmdArgs.push_back("-fbounds-safety-unique-traps");
     }
   }
+
+  Args.AddLastArg(CmdArgs, options::OPT_fbounds_safety_debug_trap_reasons_EQ);
   /* TO_UPSTREAM(BoundsSafety) OFF*/
 
   // Handle -f[no-]wrapv and -f[no-]strict-overflow, which are used by both

clang/test/BoundsSafety-legacy-checks/CodeGen/opt-remarks/ptr-bounds-argc-O0.c

@@ -63,9 +63,9 @@ int main(int argc, char **argv) {
 
 // IR-DAG: ![[LOC_10_16]] = !DILocation(line: 10, column: 16{{.*}})
 // IR-DAG: ![[LT_TRAP_LOC_10_16]] = !DILocation(line: 0, scope: ![[LT_TRAP_INFO_10_16:[0-9]+]], inlinedAt: ![[LOC_10_16]])
-// IR-DAG: ![[LT_TRAP_INFO_10_16]] = distinct !DISubprogram(name: "__clang_trap_msg$Bounds check failed$Dereferencing above bounds"
+// IR-DAG: ![[LT_TRAP_INFO_10_16]] = distinct !DISubprogram(name: "__clang_trap_msg$Bounds check failed$indexing above upper bound in 'array[idx]'"
 // IR-DAG: ![[GE_TRAP_LOC_10_16]] = !DILocation(line: 0, scope: ![[GE_TRAP_INFO_10_16:[0-9]+]], inlinedAt: ![[LOC_10_16]])
-// IR-DAG: ![[GE_TRAP_INFO_10_16]] = distinct !DISubprogram(name: "__clang_trap_msg$Bounds check failed$Dereferencing below bounds"
+// IR-DAG: ![[GE_TRAP_INFO_10_16]] = distinct !DISubprogram(name: "__clang_trap_msg$Bounds check failed$indexing below lower bound in 'array[idx]'"
 //
 // IR-DAG: ![[LOC_16_5]] = !DILocation(line: 16, column: 5
 // IR-DAG: ![[TRAP_LOC_16_5]] = !DILocation(line: 0, scope: ![[TRAP_INFO_16_5:[0-9]+]], inlinedAt: ![[LOC_16_5]])

clang/test/BoundsSafety-legacy-checks/CodeGen/opt-remarks/ptr-bounds-const-O0.c

@@ -63,9 +63,9 @@ int main() {
 
 // IR-DAG: ![[LOC_10_14]] = !DILocation(line: 10, column: 14{{.*}})
 // IR-DAG: ![[LT_TRAP_LOC_10_14]] = !DILocation(line: 0, scope: ![[LT_TRAP_INFO_10_14:[0-9]+]], inlinedAt: ![[LOC_10_14]])
-// IR-DAG: ![[LT_TRAP_INFO_10_14]] = distinct !DISubprogram(name: "__clang_trap_msg$Bounds check failed$Dereferencing above bounds"
+// IR-DAG: ![[LT_TRAP_INFO_10_14]] = distinct !DISubprogram(name: "__clang_trap_msg$Bounds check failed$indexing above upper bound in 'array[6]'"
 // IR-DAG: ![[GT_TRAP_LOC_10_14]] = !DILocation(line: 0, scope: ![[GT_TRAP_INFO_10_14:[0-9]+]], inlinedAt: ![[LOC_10_14]])
-// IR-DAG: ![[GT_TRAP_INFO_10_14]] = distinct !DISubprogram(name: "__clang_trap_msg$Bounds check failed$Dereferencing below bounds"
+// IR-DAG: ![[GT_TRAP_INFO_10_14]] = distinct !DISubprogram(name: "__clang_trap_msg$Bounds check failed$indexing below lower bound in 'array[6]'"
 // IR-DAG: ![[LOC_16_5]] = !DILocation(line: 16, column: 5
 // IR-DAG: ![[TRAP_LOC_16_5]] = !DILocation(line: 0, scope: ![[TRAP_INFO_16_5:[0-9]+]], inlinedAt: ![[LOC_16_5]])
 // IR-DAG: ![[TRAP_INFO_16_5]] = distinct !DISubprogram(name: "__clang_trap_msg$Bounds check failed$"

clang/test/BoundsSafety-legacy-checks/CodeGen/trap-reasons/ptr_ge_upper_bound-deref-array_subscript-O0.c

@@ -1,5 +1,11 @@
 
-// RUN: %clang_cc1 -O0 -debug-info-kind=standalone -dwarf-version=5 -fbounds-safety -emit-llvm %s -o - | FileCheck %s
+// RUN: %clang_cc1 -O0 -debug-info-kind=standalone -dwarf-version=5 \
+// RUN:   -fbounds-safety -fbounds-safety-debug-trap-reasons=basic \
+// RUN:   -emit-llvm %s -o - | FileCheck --check-prefixes=CHECK,BASIC %s
+
+// RUN: %clang_cc1 -O0 -debug-info-kind=standalone -dwarf-version=5 \
+// RUN:   -fbounds-safety -fbounds-safety-debug-trap-reasons=detailed \
+// RUN:   -emit-llvm %s -o - | FileCheck --check-prefixes=CHECK,DETAILED %s
 
 int operation(int index) {
   int array[] = {0, 1, 2};
@@ -15,5 +21,8 @@ int operation(int index) {
 // CHECK-NEXT: call void @llvm.ubsantrap(i8 25) {{.*}}!dbg [[TRAP_LOC:![0-9]+]]
 
 // CHECK-DAG: [[TRAP_LOC]] = !DILocation(line: 0, scope: [[TRAP_SCOPE:![0-9]+]], inlinedAt: [[LOC]])
-// CHECK-DAG: [[TRAP_SCOPE]] = distinct !DISubprogram(name: "__clang_trap_msg$Bounds check failed$Dereferencing above bounds", scope: [[FILE_SCOPE:![0-9]+]], file: [[FILE_SCOPE]], type: {{![0-9]+}}, flags: DIFlagArtificial, spFlags: DISPFlagDefinition, unit: {{![0-9]+}}
-// CHECK-DAG: [[LOC]] = !DILocation(line: 6, column: 10, scope: {{![0-9]+}})
+
+// BASIC-DAG: [[TRAP_SCOPE]] = distinct !DISubprogram(name: "__clang_trap_msg$Bounds check failed$Dereferencing above bounds", scope: [[FILE_SCOPE:![0-9]+]], file: [[FILE_SCOPE]], type: {{![0-9]+}}, flags: DIFlagArtificial, spFlags: DISPFlagDefinition, unit: {{![0-9]+}}
+// DETAILED-DAG: [[TRAP_SCOPE]] = distinct !DISubprogram(name: "__clang_trap_msg$Bounds check failed$indexing above upper bound in 'array[index]'", scope: [[FILE_SCOPE:![0-9]+]], file: [[FILE_SCOPE]], type: {{![0-9]+}}, flags: DIFlagArtificial, spFlags: DISPFlagDefinition, unit: {{![0-9]+}}
+
+// CHECK-DAG: [[LOC]] = !DILocation(line: 12, column: 10, scope: {{![0-9]+}})