[PATCH] D137714: Do not merge traps in functions annotated optnone

[Henrik G Olsson via Phabricator via cfe-commits]

hnrklssn created this revision.
Herald added a project: All.
delcypher added a comment.
hnrklssn updated this revision to Diff 474504.
hnrklssn added reviewers: delcypher, rapidsna, fcloutier, t.p.northover, patrykstefanski.
hnrklssn published this revision for review.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

@hnrklssn Could you add a regression test?


hnrklssn added a comment.

Added regression test



================
Comment at: clang/lib/CodeGen/CodeGenModule.cpp:5321
 
+  SetLLVMFunctionAttributesForDefinition(D, Fn);
   CodeGenFunction(*this).GenerateCode(GD, Fn, FI);
----------------
I'm a little worried about this ordering change here. Could this have some unintended consequences?


================
Comment at: clang/lib/CodeGen/CodeGenModule.cpp:5321
 
+  SetLLVMFunctionAttributesForDefinition(D, Fn);
   CodeGenFunction(*this).GenerateCode(GD, Fn, FI);
----------------
delcypher wrote:
> I'm a little worried about this ordering change here. Could this have some unintended consequences?
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.


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.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D137714

Files:
  clang/lib/CodeGen/CGExpr.cpp
  clang/lib/CodeGen/CodeGenModule.cpp
  clang/test/CodeGen/ubsan-trap-debugloc.c


Index: clang/test/CodeGen/ubsan-trap-debugloc.c
===================================================================
--- clang/test/CodeGen/ubsan-trap-debugloc.c
+++ clang/test/CodeGen/ubsan-trap-debugloc.c
@@ -7,4 +7,16 @@
   a = a + 1;
 }
 
+void bar(volatile int a) __attribute__((optnone)) {
+  // 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: 13, column: 9
+// CHECK: [[LOC3]] = !DILocation(line: 14, column: 9
Index: clang/lib/CodeGen/CodeGenModule.cpp
===================================================================
--- clang/lib/CodeGen/CodeGenModule.cpp
+++ clang/lib/CodeGen/CodeGenModule.cpp
@@ -5318,10 +5318,10 @@
   // Set CodeGen attributes that represent floating point environment.
   setLLVMFunctionFEnvAttributes(D, Fn);
 
+  SetLLVMFunctionAttributesForDefinition(D, Fn);
   CodeGenFunction(*this).GenerateCode(GD, Fn, FI);
 
   setNonAliasAttributes(GD, Fn);
-  SetLLVMFunctionAttributesForDefinition(D, Fn);
 
   if (const ConstructorAttr *CA = D->getAttr<ConstructorAttr>())
     AddGlobalCtor(Fn, CA->getPriority());
Index: clang/lib/CodeGen/CGExpr.cpp
===================================================================
--- clang/lib/CodeGen/CGExpr.cpp
+++ clang/lib/CodeGen/CGExpr.cpp
@@ -3545,7 +3545,8 @@
     TrapBBs.resize(CheckHandlerID + 1);
   llvm::BasicBlock *&TrapBB = TrapBBs[CheckHandlerID];
 
-  if (!CGM.getCodeGenOpts().OptimizationLevel || !TrapBB) {
+  if (!CGM.getCodeGenOpts().OptimizationLevel || !TrapBB ||
+      CurFn->hasFnAttribute(llvm::Attribute::OptimizeNone)) {
     TrapBB = createBasicBlock("trap");
     Builder.CreateCondBr(Checked, Cont, TrapBB);
     EmitBlock(TrapBB);


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D137714.474504.patch
Type: text/x-patch
Size: 2024 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20221110/caa067cf/attachment.bin>