[Backend][AsmPrinter][CFI] Generate unwind table entries in .eh_frame when the uwtable attribute is present.

[jmmartinez]

Summary

This patch forces the emission of CFI information when the uwtable attribute is present and the ExceptionModel is None.

The uwtable attribute is set by the the frontend when, for example, the address sanitizer is used.

I think few things are missing in this patch:

• a test on a target different from amdgpu that also uses cfi for the unwind tables, but that doesn't use DwarfCFI as exception model
• a test on clang's side to ensure that -fsanitize=address adds the -unwind-tables=2 attribute on amdgpu (this should probably go o a separate patch)

[jmmartinez]

Removed change that shouldn't have been commmited.

[MaskRay]

Can you describe the clang command line?

If AMDGPU doesn't use ExceptionHandling::DwarfCFI, why suppress .cfi_sections .debug_frame? .eh_frame seems inappropriate.
The change modifies the ExceptionHandling::None behavior for AMDGPU's own use case, which may not be desired by other ExceptionHandling::None targets.

[RamNalamothu]

@jmmartinez

http://logan.tw/llvm/nounwind.html might be helpful in throwing some light on the problem being solved here.

[jmmartinez]

Hi ! Thanks for the comment (and sorry for the delay).

In D139024#3969663, @MaskRay wrote:

Can you describe the clang command line?

The objective is to emit an entry in the .eh_frame section when the function is marked as uwtable (and CFI is supported by the target).
This is used by sanitizers that use async unwind tables (address, hwaddress, thread, memory and dataflow according to https://github.com/llvm/llvm-project/blob/main/clang/lib/Driver/SanitizerArgs.cpp#L42, and used here https://github.com/llvm/llvm-project/blob/main/clang/lib/Driver/ToolChains/Clang.cpp#L5395).

Then, when -fsanitize=address is used, clang would pass the -funwind-tables=2 flag to the clang -cc1 process. Then, every function definition coming clang's code-generation should have the uwtable attribute.

In D139024#3969663, @MaskRay wrote:

The change modifies the ExceptionHandling::None behavior for AMDGPU's own use case, which may not be desired by other ExceptionHandling::None targets.

I do agree that this may affect other targets, but I thought that it could affect them in a positive way (the uwtable forces the emission of an entry in the uwtable as it says it does). I might be misinterpreting the uwtable attribute.

[MaskRay]

In D139024#3988513, @jmmartinez wrote:

Hi ! Thanks for the comment (and sorry for the delay).

In D139024#3969663, @MaskRay wrote:

Can you describe the clang command line?

The objective is to emit an entry in the .eh_frame section when the function is marked as uwtable (and CFI is supported by the target).
This is used by sanitizers that use async unwind tables (address, hwaddress, thread, memory and dataflow according to https://github.com/llvm/llvm-project/blob/main/clang/lib/Driver/SanitizerArgs.cpp#L42, and used here https://github.com/llvm/llvm-project/blob/main/clang/lib/Driver/ToolChains/Clang.cpp#L5395).

Then, when -fsanitize=address is used, clang would pass the -funwind-tables=2 flag to the clang -cc1 process. Then, every function definition coming clang's code-generation should have the uwtable attribute.

-fsanitize=address used to need unwind tables for error reporting. It no longer does after https://reviews.llvm.org/D102046

In D139024#3969663, @MaskRay wrote:

The change modifies the ExceptionHandling::None behavior for AMDGPU's own use case, which may not be desired by other ExceptionHandling::None targets.

I do agree that this may affect other targets, but I thought that it could affect them in a positive way (the uwtable forces the emission of an entry in the uwtable as it says it does). I might be misinterpreting the uwtable attribute.

I am still unclear what you want to do. A .c source with a command line in the commit message will help, though I guess this patch may not be needed at all.

[jmmartinez]

I'm coming back to this issue after a long time.

---

I am still unclear what you want to do. A .c source with a command line in the commit message will help, though I guess this patch may not be needed at all.

Here's an example of what I would like to achieve:

Consider the vadd_hip.cpp example
but with the bounds check at line 18 removed.

The user can compile this code using the command line below:

hipcc -fsanitize=address vec_add.cpp

The previous command triggers two clang invocations:

• For the device: clang-15 -cc1 -triple amdgcn-amd-amdhsa -target-cpu gfx90a ... vadd_hip.cpp -funwind-tables=2 -fsanitize=address
• For the host: clang-15 -cc1 -triple x86_64_unknown-linux-gnu ... -fsanitize=address

Ideally, during the execution the out-of-bounds memory access would be detected by asan on the device side and a stacktrace reported.

At the moment, unwinding is not supported on the device side (neither fast nor slow), but having the CFI entries in the .eh_frame would remove a block to advance towards it.

---

When I started addressing the problem I realized two things:

• On clang's driver side, -fsanitize=address seems to imply that asynchronous unwind tables are used. Which sets all function with the attribute uwtable
• On targets that support DwarfCFI, unless the EH-type is DwarfCFI, CFI entries in are not generated for functions marked as uwtable.

I would have expectedt that functions marked with the uwtable attribute would get their entry regardles of the target's EH-type (doing this is the solution I propose).

An alternative solution could be to make the EH-type of AMDGPU be DwarfCFI (this could work, but I don't know if there are other consequences).

---

-fsanitize=address used to need unwind tables for error reporting. It no longer does after https://reviews.llvm.org/D102046

Finally, since I hace many blindspots on this part of LLVM, and for my curiosity, would you mind explaining the tradeoff between the slow and fast unwind?
If I understand, there is a difference in performance; and that there slow-unwind uses libunwind, while fast-unwind is implemented directly in compiler-rt. But I'm not sure if there are other implications from this tradeoff.

[jmmartinez]

@respindola & @rengolin, I've added you as reviewers since I've read your discussion from 2014 about Unwind behaviour in Clang/LLVM and I've though this issue is related.

In this patch I propose to generate entries using DwarfCFI in the .eh_frame region for functions with the uwtable attribute; when the target's EH type is either None or DwarfCFI.
I have the feeling that this last condition could be relaxed and these entries could be generated for all targets that support DwarfCFI (I ran some quick tests on my development setup and it didn't break any test).

What's my motivation for this? In my particular case, to support unwinding with the sanitizers on AMDGPU in the future.

Do you have some advice?

[jmmartinez]

llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp

1295

Could this condition be relaxed to any EHType as long as DwarfCFI is supported?

llvm/include/llvm/CodeGen/AsmPrinter.h

public:
   bool needsSEHMoves();
 
   /// Since emitting CFI unwind information is entangled with supporting the
   /// exceptions, this returns true for platforms which use CFI unwind
   /// information for debugging purpose when
   /// `MCAsmInfo::ExceptionsType == ExceptionHandling::None`.
   bool needsCFIForDebug() const;
 
+  /// Emitting CFI unwind information is also entangled with supporting
+  /// unwinding used by sanitizers. This returns true for platforms which use
+  /// CFI for the unwind tables or exception handling.
+  bool needsCFIForUnwindTables(const Function &F) const;
+
+  /// MachineFunction version of @ref needsCFIForUnwindTables(const Function &F)
+  bool needsCFIForUnwindTables(const MachineFunction &F) const;
+
   /// Print to the current output stream assembly representations of the
   /// constants in the constant pool MCP. This is used to print out constants
   /// which have been "spilled to memory" by the code generator.
   virtual void emitConstantPool();
 
   /// Print assembly representations of the jump tables used by the current
   /// function to the current output stream.
   virtual void emitJumpTableInfo();

llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp

void setPtr(BasicBlock *BB) {
     ValueHandleBase::operator=(BB);
   }
 
   void setMap(AddrLabelMap *map) { Map = map; }
 
   void deleted() override;
   void allUsesReplacedWith(Value *V2) override;
 };
+
+static bool anyFunctionNeedsCFIForUnwindTables(const AsmPrinter &Printer,
+                                               const Module &M) {
+  return any_of(M, [&Printer](const Function &F) {
+    return Printer.needsCFIForUnwindTables(F);
+  });
+}
 } // namespace
 
 class llvm::AddrLabelMap {
   MCContext &Context;
   struct AddrLabelSymEntry {
     /// The symbols for the label.
     TinyPtrVector<MCSymbol *> Symbols;
 
#endif
   }
 
   if (M.getNamedMetadata(PseudoProbeDescMetadataName)) {
     PP = new PseudoProbeHandler(this);
     Handlers.emplace_back(std::unique_ptr<PseudoProbeHandler>(PP), PPTimerName,
                           PPTimerDescription, PPGroupName, PPGroupDescription);
   }
 
-  switch (MAI->getExceptionHandlingType()) {
+  auto const ExceptionHandlingType = MAI->getExceptionHandlingType();
+  switch (ExceptionHandlingType) {
   case ExceptionHandling::None:
     // We may want to emit CFI for debug.
     [[fallthrough]];
   case ExceptionHandling::SjLj:
   case ExceptionHandling::DwarfCFI:
   case ExceptionHandling::ARM:
     for (auto &F : M.getFunctionList()) {
-      if (getFunctionCFISectionType(F) != CFISection::None)
-        ModuleCFISection = getFunctionCFISectionType(F);
+      auto FunctionCFISection = getFunctionCFISectionType(F);
+      if (FunctionCFISection != CFISection::None)
+        ModuleCFISection = FunctionCFISection;
       // If any function needsUnwindTableEntry(), it needs .eh_frame and hence
       // the module needs .eh_frame. If we have found that case, we are done.
       if (ModuleCFISection == CFISection::EH)
         break;
     }
-    assert(MAI->getExceptionHandlingType() == ExceptionHandling::DwarfCFI ||
-           ModuleCFISection != CFISection::EH);
     break;
   default:
     break;
   }
 
   EHStreamer *ES = nullptr;
-  switch (MAI->getExceptionHandlingType()) {
+  switch (ExceptionHandlingType) {
   case ExceptionHandling::None:
-    if (!needsCFIForDebug())
+    if (!needsCFIForDebug() && !anyFunctionNeedsCFIForUnwindTables(*this, M))
       break;
     [[fallthrough]];
   case ExceptionHandling::SjLj:
   case ExceptionHandling::DwarfCFI:
     ES = new DwarfCFIException(this);
     break;
   case ExceptionHandling::ARM:
     ES = new ARMException(this);

 }
 
 AsmPrinter::CFISection
 AsmPrinter::getFunctionCFISectionType(const Function &F) const {
   // Ignore functions that won't get emitted.
   if (F.isDeclarationForLinker())
     return CFISection::None;
 
-  if (MAI->getExceptionHandlingType() == ExceptionHandling::DwarfCFI &&
-      F.needsUnwindTableEntry())
+  if (needsCFIForUnwindTables(F))
     return CFISection::EH;
 
   if (MMI->hasDebugInfo() || TM.Options.ForceDwarfFrameSection)
     return CFISection::Debug;
 
   return CFISection::None;
 }
 
 AsmPrinter::CFISection
 AsmPrinter::getFunctionCFISectionType(const MachineFunction &MF) const {
   return getFunctionCFISectionType(MF.getFunction());
 }
 
 bool AsmPrinter::needsSEHMoves() {
   return MAI->usesWindowsCFI() && MF->getFunction().needsUnwindTableEntry();
 }
 
+bool AsmPrinter::needsCFIForUnwindTables(const MachineFunction &F) const {
+  return needsCFIForUnwindTables(F.getFunction());
+}
+
+bool AsmPrinter::needsCFIForUnwindTables(const Function &F) const {
+  // If the unwind table entries are used for eh, emit them.
+  // If the unwind table entires are explicitely needed via the corresponding
+  // attribute, and cfi is supported by the target, emit unwind table entries
+  // This last case is used by sanitizers (e.g. asan) that force the use of
+  // unwind tables
+  if (F.isDeclarationForLinker())
+    return false;
+
+  const auto EHType = MAI->getExceptionHandlingType();
+  if (EHType == ExceptionHandling::DwarfCFI && F.needsUnwindTableEntry())
+    return true;
+
+  if (EHType == ExceptionHandling::None && MAI->doesUseCFIForDebug() &&
+      F.hasUWTable())
+    return true;
+
+  return false;
+}
+
 bool AsmPrinter::needsCFIForDebug() const {
   return MAI->getExceptionHandlingType() == ExceptionHandling::None &&
          MAI->doesUseCFIForDebug() && ModuleCFISection == CFISection::Debug;
 }
 
 void AsmPrinter::emitCFIInstruction(const MachineInstr &MI) {
   ExceptionHandling ExceptionHandlingType = MAI->getExceptionHandlingType();
-  if (!needsCFIForDebug() &&
-      ExceptionHandlingType != ExceptionHandling::DwarfCFI &&
+  bool NeedsCFI = needsCFIForDebug() || needsCFIForUnwindTables(*MI.getMF());
+  if (!NeedsCFI && ExceptionHandlingType != ExceptionHandling::DwarfCFI &&
       ExceptionHandlingType != ExceptionHandling::ARM)
     return;
 
   if (getFunctionCFISectionType(*MF) == CFISection::None)
     return;
 
   // If there is no "real" instruction following this CFI instruction, skip
   // emitting it; it would be beyond the end of the function's FDE range.

llvm/lib/CodeGen/AsmPrinter/DwarfCFIException.cpp

void DwarfCFIException::beginFunction(const MachineFunction *MF) {
   shouldEmitLSDA = shouldEmitPersonality &&
     LSDAEncoding != dwarf::DW_EH_PE_omit;
 
   const MCAsmInfo &MAI = *MF->getMMI().getContext().getAsmInfo();
   if (MAI.getExceptionHandlingType() != ExceptionHandling::None)
     shouldEmitCFI =
         MAI.usesCFIForEH() && (shouldEmitPersonality || shouldEmitMoves);
   else
-    shouldEmitCFI = Asm->needsCFIForDebug() && shouldEmitMoves;
+    shouldEmitCFI =
+        (Asm->needsCFIForDebug() || Asm->needsCFIForUnwindTables(*MF)) &&
+        shouldEmitMoves;
 }
 
 void DwarfCFIException::beginBasicBlockSection(const MachineBasicBlock &MBB) {
   if (!shouldEmitCFI)
     return;
 
   if (!hasEmittedCFISections) {
     AsmPrinter::CFISection CFISecType = Asm->getModuleCFISectionType();

llvm/test/CodeGen/AMDGPU/eh_frame.ll

+; RUN: llc -mtriple=amdgcn-amd-amdhsa -mcpu=gfx908 -o - < %s | FileCheck %s --check-prefix=EH
+; RUN: llc -mtriple=amdgcn-amd-amdhsa --force-dwarf-frame-section -o - < %s | FileCheck %s --check-prefix=BOTH
+; RUN: llc -mtriple=amdgcn-amd-amdhsa --exception-model=dwarf -o - < %s | FileCheck %s --check-prefix=EH
+; RUN: llc -mtriple=amdgcn-amd-amdhsa --force-dwarf-frame-section --exception-model=dwarf -o - < %s | FileCheck %s --check-prefix=BOTH
+
+; EH: f:
+; EH-NOT: .cfi_sections
+; EH: .cfi_startproc
+
+; BOTH: f:
+; BOTH: .cfi_sections .eh_frame, .debug_frame
+; BOTH: .cfi_startproc
+
+define void @f() nounwind uwtable !dbg !0 {
+entry:
+  ret void
+}
+
+!llvm.dbg.cu = !{!2}
+!llvm.module.flags = !{!7}
+!5 = !{!0}
+
+!0 = distinct !DISubprogram(name: "f", line: 1, isLocal: false, isDefinition: true, virtualIndex: 6, flags: DIFlagPrototyped, isOptimized: true, unit: !2, scopeLine: 1, file: !6, scope: !1, type: !3)
+!1 = !DIFile(filename: "/home/llvm/test.c", directory: "/home/llvm/build")
+!2 = distinct !DICompileUnit(language: DW_LANG_C99, producer: "clang", isOptimized: true, emissionKind: FullDebug, file: !6, enums: !{}, retainedTypes: !{})
+!3 = !DISubroutineType(types: !4)
+!4 = !{null}
+!6 = !DIFile(filename: "/home/llvm/test.c", directory: "/home/llvm/build")
+!7 = !{i32 1, !"Debug Info Version", i32 3}

llvm/test/tools/UpdateTestChecks/update_llc_test_checks/Inputs/amdgpu_generated_funcs.ll.generated.expected

define dso_local i32 @main() #0 {
   store i32 4, i32 addrspace(5)* %5, align 4
   ret i32 0
 }
 
 attributes #0 = { noredzone nounwind ssp uwtable "frame-pointer"="all" }
 ; CHECK-LABEL: check_boundaries:
 ; CHECK:       check_boundaries$local:
 ; CHECK-NEXT:    .type check_boundaries$local,@function
+; CHECK-NEXT:    .cfi_startproc
 ; CHECK-NEXT:  ; %bb.0:
 ; CHECK-NEXT:    s_waitcnt vmcnt(0) expcnt(0) lgkmcnt(0)
 ; CHECK-NEXT:    s_mov_b32 s4, s33
 ; CHECK-NEXT:    s_mov_b32 s33, s32
 ; CHECK-NEXT:    v_mov_b32_e32 v0, 0
 ; CHECK-NEXT:    s_mov_b32 s33, s4
 ; CHECK-NEXT:    s_setpc_b64 s[30:31]
 ;
 ; CHECK-LABEL: main:
 ; CHECK:       main$local:
 ; CHECK-NEXT:    .type main$local,@function
+; CHECK-NEXT:    .cfi_startproc
 ; CHECK-NEXT:  ; %bb.0:
 ; CHECK-NEXT:    s_waitcnt vmcnt(0) expcnt(0) lgkmcnt(0)
 ; CHECK-NEXT:    s_mov_b32 s6, s33
 ; CHECK-NEXT:    s_mov_b32 s33, s32
 ; CHECK-NEXT:    s_getpc_b64 s[4:5]
 ; CHECK-NEXT:    s_add_u32 s4, s4, x@rel32@lo+4
 ; CHECK-NEXT:    s_addc_u32 s5, s5, x@rel32@hi+12
 ; CHECK-NEXT:    v_mov_b32_e32 v2, 1

llvm/test/tools/UpdateTestChecks/update_llc_test_checks/Inputs/amdgpu_generated_funcs.ll.nogenerated.expected

 ; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
 ; RUN: llc -enable-machine-outliner -mtriple=amdgcn-adm-amdhsa < %s | FileCheck %s
 
 ; NOTE: Machine outliner doesn't run.
 @x = dso_local global i32 0, align 4
 
 define dso_local i32 @check_boundaries() #0 {
 ; CHECK-LABEL: check_boundaries:
 ; CHECK:       check_boundaries$local:
 ; CHECK-NEXT:    .type check_boundaries$local,@function
+; CHECK-NEXT:    .cfi_startproc
 ; CHECK-NEXT:  ; %bb.0:
 ; CHECK-NEXT:    s_waitcnt vmcnt(0) expcnt(0) lgkmcnt(0)
 ; CHECK-NEXT:    s_mov_b32 s4, s33
 ; CHECK-NEXT:    s_mov_b32 s33, s32
 ; CHECK-NEXT:    v_mov_b32_e32 v0, 0
 ; CHECK-NEXT:    s_mov_b32 s33, s4
 ; CHECK-NEXT:    s_setpc_b64 s[30:31]
   %1 = alloca i32, align 4, addrspace(5)
; CHECK-NEXT:    s_setpc_b64 s[30:31]
 
   ret i32 0
 }
 
 define dso_local i32 @main() #0 {
 ; CHECK-LABEL: main:
 ; CHECK:       main$local:
 ; CHECK-NEXT:    .type main$local,@function
+; CHECK-NEXT:    .cfi_startproc
 ; CHECK-NEXT:  ; %bb.0:
 ; CHECK-NEXT:    s_waitcnt vmcnt(0) expcnt(0) lgkmcnt(0)
 ; CHECK-NEXT:    s_mov_b32 s6, s33
 ; CHECK-NEXT:    s_mov_b32 s33, s32
 ; CHECK-NEXT:    s_getpc_b64 s[4:5]
 ; CHECK-NEXT:    s_add_u32 s4, s4, x@rel32@lo+4
 ; CHECK-NEXT:    s_addc_u32 s5, s5, x@rel32@hi+12
 ; CHECK-NEXT:    v_mov_b32_e32 v2, 1