Attach attribute "trap-func-name" to IR function

[ahatanak]

Summary

This patch is part of the work to make LTO and function multi-versioning work correctly.

Currently, the trap function name users specify on the command line using -ftrap-function is passed to the backend via TargetMachine's TargetOptions. This patch attaches attribute "trap-func-name" to the IR function instead so that we can use -ftrap-function for LTO builds and change the trap function name on a per-function basis.

[echristo]

Not sure we necessarily need a "put these function attributes on the functions" command line and map? Rationale?

-eric

[ahatanak]

In D10831#196951, @echristo wrote:

Not sure we necessarily need a "put these function attributes on the functions" command line and map? Rationale?

That's a good point. I don't think we need a new cc1 option for trap-function at all.

[ahatanak]

Remove cc1 option and simplify patch.

[echristo]

Lgtm thanks.

[ahatanak]

Thanks, I'll send the corresponding llvm patch shortly.

[ahatanak]

The new patch attaches "trap-func-name" to the call sites instead of the calling functions, which simplifies the handling of the attribute when functions are merged and inlined.

The relevant discussion I had with Eric is here:
http://reviews.llvm.org/D10832

[echristo]

You'll want to also test a trap inside of a ctor/dtor, global constructor (if you can), etc. The general change looks fine though.

Thanks!

-eric

[ahatanak]

I wasn't sure how to test traps inside ctor/dtor or global constructor, so I went ahead and committed the reviewed patch. I can add the tests later if I can find a way to emit traps for those functions.

[pcc]

Maybe I'm missing something, but I don't see why we need this attribute. Couldn't clang have been changed to implement -ftrap-function by generating a call to the trap function instead of emitting an llvm.trap intrinsic call?

[ahatanak]

AMDGPUAnnotateKernelFeatures::addFeatureAttributes looks for an llvm.trap instruction and adds attribute "amdgpu-queue-ptr" to the function if it finds one.

Other than that, it looks like your idea would work too.

lib/CodeGen/BackendUtil.cpp

TargetMachine *EmitAssemblyHelper::CreateTargetMachine(bool MustCreateTM) {
   }
 
   Options.LessPreciseFPMADOption = CodeGenOpts.LessPreciseFPMAD;
   Options.NoInfsFPMath = CodeGenOpts.NoInfsFPMath;
   Options.NoNaNsFPMath = CodeGenOpts.NoNaNsFPMath;
   Options.NoZerosInBSS = CodeGenOpts.NoZeroInitializedInBSS;
   Options.UnsafeFPMath = CodeGenOpts.UnsafeFPMath;
   Options.StackAlignmentOverride = CodeGenOpts.StackAlignment;
-  Options.TrapFuncName = CodeGenOpts.TrapFuncName;
   Options.PositionIndependentExecutable = LangOpts.PIELevel != 0;
   Options.FunctionSections = CodeGenOpts.FunctionSections;
   Options.DataSections = CodeGenOpts.DataSections;
   Options.UniqueSectionNames = CodeGenOpts.UniqueSectionNames;
 
   Options.MCOptions.MCRelaxAll = CodeGenOpts.RelaxAll;
   Options.MCOptions.MCSaveTempLabels = CodeGenOpts.SaveTempLabels;
   Options.MCOptions.MCUseDwarfDirectory = !CodeGenOpts.NoDwarfDirectoryAsm;

lib/CodeGen/CGBuiltin.cpp

case Builtin::BI__builtin_readcyclecounter: {
     return RValue::get(Builder.CreateCall(F, {}));
   }
   case Builtin::BI__builtin___clear_cache: {
     Value *Begin = EmitScalarExpr(E->getArg(0));
     Value *End = EmitScalarExpr(E->getArg(1));
     Value *F = CGM.getIntrinsic(Intrinsic::clear_cache);
     return RValue::get(Builder.CreateCall(F, {Begin, End}));
   }
-  case Builtin::BI__builtin_trap: {
-    Value *F = CGM.getIntrinsic(Intrinsic::trap);
-    return RValue::get(Builder.CreateCall(F, {}));
-  }
-  case Builtin::BI__debugbreak: {
-    Value *F = CGM.getIntrinsic(Intrinsic::debugtrap);
-    return RValue::get(Builder.CreateCall(F, {}));
-  }
+  case Builtin::BI__builtin_trap:
+    return RValue::get(EmitTrapCall(Intrinsic::trap));
+  case Builtin::BI__debugbreak:
+    return RValue::get(EmitTrapCall(Intrinsic::debugtrap));
   case Builtin::BI__builtin_unreachable: {
     if (SanOpts.has(SanitizerKind::Unreachable)) {
       SanitizerScope SanScope(this);
       EmitCheck(std::make_pair(static_cast<llvm::Value *>(Builder.getFalse()),
                                SanitizerKind::Unreachable),
                 "builtin_unreachable", EmitCheckSourceLocation(E->getExprLoc()),
                 None);
     } else

lib/CodeGen/CGCall.cpp

void CodeGenModule::ConstructAttributeList(const CGFunctionInfo &FI,
   if (CodeGenOpts.EnableSegmentedStacks &&
       !(TargetDecl && TargetDecl->hasAttr<NoSplitStackAttr>()))
     FuncAttrs.addAttribute("split-stack");
 
   if (AttrOnCallSite) {
     // Attributes that should go on the call site only.
     if (!CodeGenOpts.SimplifyLibCalls)
       FuncAttrs.addAttribute(llvm::Attribute::NoBuiltin);
+    if (!CodeGenOpts.TrapFuncName.empty())
+      FuncAttrs.addAttribute("trap-func-name", CodeGenOpts.TrapFuncName);
   } else {
     // Attributes that should go on the function, but not the call site.
     if (!CodeGenOpts.DisableFPElim) {
       FuncAttrs.addAttribute("no-frame-pointer-elim", "false");
     } else if (CodeGenOpts.OmitLeafFramePointer) {
       FuncAttrs.addAttribute("no-frame-pointer-elim", "false");
       FuncAttrs.addAttribute("no-frame-pointer-elim-non-leaf");
     } else {

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
   // function to save on code size.
   if (!CGM.getCodeGenOpts().OptimizationLevel || !TrapBB) {
     TrapBB = createBasicBlock("trap");
     Builder.CreateCondBr(Checked, Cont, TrapBB);
     EmitBlock(TrapBB);
-    llvm::Value *F = CGM.getIntrinsic(llvm::Intrinsic::trap);
-    llvm::CallInst *TrapCall = Builder.CreateCall(F, {});
+    llvm::CallInst *TrapCall = EmitTrapCall(llvm::Intrinsic::trap);
     TrapCall->setDoesNotReturn();
     TrapCall->setDoesNotThrow();
     Builder.CreateUnreachable();
   } else {
     Builder.CreateCondBr(Checked, Cont, TrapBB);
   }
 
   EmitBlock(Cont);
 }
 
+llvm::CallInst *CodeGenFunction::EmitTrapCall(llvm::Intrinsic::ID IntrID) {
+  llvm::CallInst *TrapCall =
+      Builder.CreateCall(CGM.getIntrinsic(IntrID), {});
+
+  if (!CGM.getCodeGenOpts().TrapFuncName.empty())
+    TrapCall->addAttribute(llvm::AttributeSet::FunctionIndex,
+                           "trap-func-name",
+                           CGM.getCodeGenOpts().TrapFuncName);
+
+  return TrapCall;
+}
+
 /// isSimpleArrayDecayOperand - If the specified expr is a simple decay from an
 /// array to pointer, return the array subexpression.
 static const Expr *isSimpleArrayDecayOperand(const Expr *E) {
   // If this isn't just an array->pointer decay, bail out.
   const auto *CE = dyn_cast<CastExpr>(E);
   if (!CE || CE->getCastKind() != CK_ArrayToPointerDecay)
     return nullptr;
 

lib/CodeGen/CodeGenFunction.h

public:
   void EmitCheck(ArrayRef<std::pair<llvm::Value *, SanitizerMask>> Checked,
                  StringRef CheckName, ArrayRef<llvm::Constant *> StaticArgs,
                  ArrayRef<llvm::Value *> DynamicArgs);
 
   /// \brief Create a basic block that will call the trap intrinsic, and emit a
   /// conditional branch to it, for the -ftrapv checks.
   void EmitTrapCheck(llvm::Value *Checked);
 
+  /// \brief Emit a call to trap or debugtrap and attach function attribute
+  /// "trap-func-name" if specified.
+  llvm::CallInst *EmitTrapCall(llvm::Intrinsic::ID IntrID);
+
   /// \brief Create a check for a function parameter that may potentially be
   /// declared as non-null.
   void EmitNonNullArgCheck(RValue RV, QualType ArgType, SourceLocation ArgLoc,
                            const FunctionDecl *FD, unsigned ParmNum);
 
   /// EmitCallArg - Emit a single call argument.
   void EmitCallArg(CallArgList &args, const Expr *E, QualType ArgType);
 

lib/CodeGen/CodeGenFunction.cpp

void CodeGenFunction::GenerateCode(GlobalDecl GD, llvm::Function *Fn,
   if (getLangOpts().CPlusPlus && !FD->hasImplicitReturnZero() && !SawAsmBlock &&
       !FD->getReturnType()->isVoidType() && Builder.GetInsertBlock()) {
     if (SanOpts.has(SanitizerKind::Return)) {
       SanitizerScope SanScope(this);
       llvm::Value *IsFalse = Builder.getFalse();
       EmitCheck(std::make_pair(IsFalse, SanitizerKind::Return),
                 "missing_return", EmitCheckSourceLocation(FD->getLocation()),
                 None);
-    } else if (CGM.getCodeGenOpts().OptimizationLevel == 0)
-      Builder.CreateCall(CGM.getIntrinsic(llvm::Intrinsic::trap), {});
+    } else if (CGM.getCodeGenOpts().OptimizationLevel == 0) {
+      EmitTrapCall(llvm::Intrinsic::trap);
+    }
     Builder.CreateUnreachable();
     Builder.ClearInsertionPoint();
   }
 
   // Emit the standard function epilogue.
   FinishFunction(BodyRange.getEnd());
 
   // If we haven't marked the function nothrow through other means, do

test/CodeGenCXX/trap-fnattr.cpp

+// RUN: %clang_cc1 -O0 -emit-llvm -ftrapv -ftrap-function=mytrap %s -o - | FileCheck %s -check-prefix=TRAPFUNC
+// RUN: %clang_cc1 -O0 -emit-llvm -ftrapv %s -o - | FileCheck %s -check-prefix=NOOPTION
+
+// TRAPFUNC-LABEL: define void @_Z12test_builtinv
+// TRAPFUNC: call void @llvm.trap() [[ATTR0:#[0-9]+]]
+
+// NOOPTION-LABEL: define void @_Z12test_builtinv
+// NOOPTION: call void @llvm.trap(){{$}}
+
+void test_builtin(void) {
+  __builtin_trap();
+}
+
+// TRAPFUNC-LABEL: define i32 @_Z13test_noreturnv
+// TRAPFUNC: call void @llvm.trap() [[ATTR0]]
+
+// NOOPTION-LABEL: define i32 @_Z13test_noreturnv
+// NOOPTION: call void @llvm.trap(){{$}}
+
+int test_noreturn(void) {
+}
+
+// TRAPFUNC-LABEL: define i32 @_Z17test_add_overflowii
+// TRAPFUNC: call void @llvm.trap() [[ATTR1:#[0-9]+]]
+
+// NOOPTION-LABEL: define i32 @_Z17test_add_overflowii
+// NOOPTION: call void @llvm.trap() [[ATTR2:#[0-9]+]]
+
+int test_add_overflow(int a, int b) {
+  return a + b;
+}
+
+// TRAPFUNC: attributes [[ATTR0]] = { {{.*}}"trap-func-name"="mytrap" }
+// TRAPFUNC: attributes [[ATTR1]] = { {{.*}}"trap-func-name"="mytrap" }
+
+// NOOPTION-NOT: attributes [[ATTR2]] = { {{.*}}"trap-func-name"="mytrap" }