Do not merge traps in functions annotated optnone

[hnrklssn]

Summary

This aligns the behaviour with that of disabling optimisations for the
translation unit entirely. Not merging the traps allows us to keep
separate debug information for each, improving the debugging experience
when finding the cause for a ubsan trap.

[delcypher]

@hnrklssn Could you add a regression test?

clang/lib/CodeGen/CodeGenModule.cpp

5321 ↗

(On Diff #474268)

I'm a little worried about this ordering change here. Could this have some unintended consequences?

[hnrklssn]

Added regression test

[hnrklssn]

clang/lib/CodeGen/CodeGenModule.cpp

5321 ↗

(On Diff #474268)

Yeah I was also a bit worried about that when making the change, since the functions are both quite broad and I'm not familiar with them from before. However it doesn't break any test cases, so I'm not sure what the consequences would be exactly.

For reference, also moving the call to setNonAliasAttributes so that it is still before the call to SetLLVMFunctionAttributesForDefinition breaks a ton of test cases so I'm somewhat hopeful that we have good test coverage for this type of change.

[fcloutier]

clang/lib/CodeGen/CodeGenModule.cpp

5321 ↗

(On Diff #474268)

Could you get it from CurCodeDecl->hasAttr<OptimizeNoneAttr>() in CGExpr instead? Then you wouldn't have to change this.

Caveat: am not sure that CurCodeDecl is always set. For instance, do you have a CurCodeDecl when you generate a C++ static initializer? On the upside, if it's NULL, you can just bail out.

[hnrklssn]

No longer reorders annotations to occur before codegen. Instead EmitTrapCheck checks the function declaration for OptimizeNoneAttr.

[hnrklssn]

Made changes in line with what @fcloutier suggested.

clang/lib/CodeGen/CodeGenModule.cpp

5321 ↗

(On Diff #474268)

Indeed that approach works as well. There are some test cases that result in CurCodeDecl being NULL, like you suspected.

[delcypher]

Other than minor issue in the test this LGTM

clang/test/CodeGen/ubsan-trap-debugloc.c

12

Could you add CHECK-LABEL: @foo and CHECK-LABEL: @bar to this test? This helps make it explicit which functions in the IR we are trying to match.

[hnrklssn]

Add explicit labels to FileCheck checks

[delcypher]

LGTM

clang/lib/CodeGen/CGExpr.cpp

void CodeGenFunction::EmitTrapCheck(llvm::Value *Checked,
   llvm::BasicBlock *Cont = createBasicBlock("cont");
 
   // If we're optimizing, collapse all calls to trap down to just one per
   // check-type per function to save on code size.
   if (TrapBBs.size() <= CheckHandlerID)
     TrapBBs.resize(CheckHandlerID + 1);
   llvm::BasicBlock *&TrapBB = TrapBBs[CheckHandlerID];
 
-  if (!CGM.getCodeGenOpts().OptimizationLevel || !TrapBB) {
+  if (!CGM.getCodeGenOpts().OptimizationLevel || !TrapBB ||
+      (CurCodeDecl && CurCodeDecl->hasAttr<OptimizeNoneAttr>())) {
     TrapBB = createBasicBlock("trap");
     Builder.CreateCondBr(Checked, Cont, TrapBB);
     EmitBlock(TrapBB);
 
     llvm::CallInst *TrapCall =
         Builder.CreateCall(CGM.getIntrinsic(llvm::Intrinsic::ubsantrap),
                            llvm::ConstantInt::get(CGM.Int8Ty, CheckHandlerID));
 

clang/test/CodeGen/ubsan-trap-debugloc.c

 // RUN: %clang_cc1 -emit-llvm -disable-llvm-passes -O -fsanitize=signed-integer-overflow -fsanitize-trap=signed-integer-overflow %s -o - -debug-info-kind=line-tables-only | FileCheck %s
 
 
 void foo(volatile int a) {
+  // CHECK-LABEL: @foo
   // CHECK: call void @llvm.ubsantrap(i8 0){{.*}} !dbg [[LOC:![0-9]+]]
   a = a + 1;
   a = a + 1;
 }
 
+void bar(volatile int a) __attribute__((optnone)) {
+  // CHECK-LABEL: @bar
+  // CHECK: call void @llvm.ubsantrap(i8 0){{.*}} !dbg [[LOC2:![0-9]+]]
+  // CHECK: call void @llvm.ubsantrap(i8 0){{.*}} !dbg [[LOC3:![0-9]+]]
+  a = a + 1;
+  a = a + 1;
+}
+
+// With optimisations enabled the traps are merged and need to share a debug location
 // CHECK: [[LOC]] = !DILocation(line: 0
+
+// With optimisations disabled the traps are not merged and retain accurate debug locations
+// CHECK: [[LOC2]] = !DILocation(line: 15, column: 9
+// CHECK: [[LOC3]] = !DILocation(line: 16, column: 9