From a7904b257c062baba944286fc55f3e682d6eae08 Mon Sep 17 00:00:00 2001 From: Sterling Augustine Date: Wed, 24 Sep 2025 16:43:37 -0700 Subject: [PATCH 1/7] [Sframe] Support cfi_escape directives compatibly with gnu-gas Some cfi_escape directives don't affect sframe unwind info, some do. Detect those cases appropriately, following gnu-gas for most cases. Using a full-blown dwarf expression parser allows for somewhat more precise error detection than other sframe implementations. So this code is less conservative for long and more involved expressions. It could be made even more permissive. --- llvm/lib/MC/CMakeLists.txt | 3 +- llvm/lib/MC/MCSFrame.cpp | 149 +++++++++++++++++- .../MC/ELF/cfi-sframe-cfi-escape-errors.s | 37 +++++ llvm/test/MC/ELF/cfi-sframe-cfi-escape.s | 43 +++++ .../llvm-project-overlay/llvm/BUILD.bazel | 1 + 5 files changed, 230 insertions(+), 3 deletions(-) create mode 100644 llvm/test/MC/ELF/cfi-sframe-cfi-escape-errors.s create mode 100644 llvm/test/MC/ELF/cfi-sframe-cfi-escape.s diff --git a/llvm/lib/MC/CMakeLists.txt b/llvm/lib/MC/CMakeLists.txt index 1e1d0a6d00601..70c4577aeec0a 100644 --- a/llvm/lib/MC/CMakeLists.txt +++ b/llvm/lib/MC/CMakeLists.txt @@ -73,9 +73,10 @@ add_llvm_component_library(LLVMMC ${LLVM_MAIN_INCLUDE_DIR}/llvm/MC LINK_COMPONENTS + BinaryFormat + DebugInfoDWARFLowLevel Support TargetParser - BinaryFormat DEPENDS intrinsics_gen diff --git a/llvm/lib/MC/MCSFrame.cpp b/llvm/lib/MC/MCSFrame.cpp index d6fa54c087ca3..c00262471f123 100644 --- a/llvm/lib/MC/MCSFrame.cpp +++ b/llvm/lib/MC/MCSFrame.cpp @@ -8,6 +8,8 @@ #include "llvm/MC/MCSFrame.h" #include "llvm/BinaryFormat/SFrame.h" +#include "llvm/DebugInfo/DWARF/LowLevel/DWARFCFIProgram.h" +#include "llvm/DebugInfo/DWARF/LowLevel/DWARFDataExtractorSimple.h" #include "llvm/MC/MCAsmInfo.h" #include "llvm/MC/MCContext.h" #include "llvm/MC/MCObjectFileInfo.h" @@ -211,6 +213,148 @@ class SFrameEmitterImpl { return true; } + // Technically, the escape data could be anything, but it is commonly a dwarf + // CFI program. Even then, it could contain an arbitrarily complicated Dwarf + // expression. Following gnu-gas, look for certain common cases that could + // invalidate an FDE, emit a warning for those sequences, and don't generate + // an FDE in those cases. Allow any that are known safe. It is likely that + // more thorough test cases could refine this code, but it handles the most + // important ones compatibly with gas. + bool IsCFIEscapeSafe(SFrameFDE &FDE, const SFrameFRE &FRE, + const MCCFIInstruction &CFI) { + const MCAsmInfo *AI = Streamer.getContext().getAsmInfo(); + DWARFDataExtractorSimple data(CFI.getValues(), AI->isLittleEndian(), + AI->getCodePointerSize()); + + // Normally, both alignment factors are extracted from the enclosing Dwarf + // FDE or CIE. We don't have one here. Alignments are used for scaling + // factors for ops like CFA_def_cfa_offset_sf. But this particular function + // is only interested in registers. + dwarf::CFIProgram P(/* CodeAlignmentFactor */ 1, + /* DataAlignmentFactor*/ 1, + Streamer.getContext().getTargetTriple().getArch()); + uint64_t Offset = 0; + if (P.parse(data, &Offset, CFI.getValues().size())) { + // Not a parsable dwarf expression. Assume the worst. + Streamer.getContext().reportWarning( + CFI.getLoc(), + "skipping SFrame FDE; .cfi_escape with unknown effects"); + return false; + } + + // This loop deals with are dwarf::CFIProgram::Instructions. Everywhere + // else, This file deals with MCCFIInstructions.. + for (const dwarf::CFIProgram::Instruction &I : P) { + switch (I.Opcode) { + case dwarf::DW_CFA_nop: + break; + case dwarf::DW_CFA_val_offset: { + // First argument is a register. Anything that touches CFA, FP, or RA is + // a problem, but allow others through. As an even more special case, + // allow SP + 0. + auto Reg = I.getOperandAsUnsigned(P, 0); + if (!Reg) { + Streamer.getContext().reportWarning( + CFI.getLoc(), + "skipping SFrame FDE; .cfi_escape with unknown effects"); + } + bool SPOk = true; + if (*Reg == SPReg) { + auto Opnd = I.getOperandAsSigned(P, 1); + if (!Opnd || *Opnd != 0) + SPOk = false; + } + if (!SPOk || *Reg == RAReg || *Reg == FPReg) { + StringRef RN = *Reg == SPReg + ? "SP reg " + : (*Reg == FPReg ? "FP reg " : "RA reg "); + Streamer.getContext().reportWarning( + CFI.getLoc(), + Twine( + "skipping SFrame FDE; .cfi_escape DW_CFA_val_offset with ") + + RN + Twine(*Reg)); + return false; + } + } break; + case dwarf::DW_CFA_expression: { + // First argument is a register. Anything that touches CFA, FP, or RA is + // a problem, but allow others through. + auto Reg = I.getOperandAsUnsigned(P, 0); + if (!Reg) { + Streamer.getContext().reportWarning( + CFI.getLoc(), + "skipping SFrame FDE; .cfi_escape with unknown effects"); + return false; + } + if (*Reg == SPReg || *Reg == RAReg || *Reg == FPReg) { + StringRef RN = *Reg == SPReg + ? "SP reg " + : (*Reg == FPReg ? "FP reg " : "RA reg "); + Streamer.getContext().reportWarning( + CFI.getLoc(), + Twine( + "skipping SFrame FDE; .cfi_escape DW_CFA_expression with ") + + RN + Twine(*Reg)); + return false; + } + } break; + case dwarf::DW_CFA_GNU_args_size: + if (FRE.Info.getBaseRegister() != BaseReg::FP) { + Streamer.getContext().reportWarning( + CFI.getLoc(), + Twine("skipping SFrame FDE; .cfi_escape DW_CFA_GNU_args_size " + "with non frame-pointer CFA")); + return false; + } + break; + // Cases that gas doesn't specially handle. TODO: Some of these could be + // analyzed and handled instead of just punting. But these are uncommon, + // or should be written as normal cfi directives. Some will need fixes to + // the scaling factor. + case dwarf::DW_CFA_advance_loc: + case dwarf::DW_CFA_offset: + case dwarf::DW_CFA_restore: + case dwarf::DW_CFA_set_loc: + case dwarf::DW_CFA_advance_loc1: + case dwarf::DW_CFA_advance_loc2: + case dwarf::DW_CFA_advance_loc4: + case dwarf::DW_CFA_offset_extended: + case dwarf::DW_CFA_restore_extended: + case dwarf::DW_CFA_undefined: + case dwarf::DW_CFA_same_value: + case dwarf::DW_CFA_register: + case dwarf::DW_CFA_remember_state: + case dwarf::DW_CFA_restore_state: + case dwarf::DW_CFA_def_cfa: + case dwarf::DW_CFA_def_cfa_register: + case dwarf::DW_CFA_def_cfa_offset: + case dwarf::DW_CFA_def_cfa_expression: + case dwarf::DW_CFA_offset_extended_sf: + case dwarf::DW_CFA_def_cfa_sf: + case dwarf::DW_CFA_def_cfa_offset_sf: + case dwarf::DW_CFA_val_offset_sf: + case dwarf::DW_CFA_val_expression: + case dwarf::DW_CFA_MIPS_advance_loc8: + case dwarf::DW_CFA_AARCH64_negate_ra_state_with_pc: + case dwarf::DW_CFA_AARCH64_negate_ra_state: + case dwarf::DW_CFA_LLVM_def_aspace_cfa: + case dwarf::DW_CFA_LLVM_def_aspace_cfa_sf: + Streamer.getContext().reportWarning( + CFI.getLoc(), "skipping SFrame FDE; .cfi_escape " + "CFA expression with unknown side effects"); + return false; + default: + // Dwarf expression was only partially valid, and user could have + // written anything. + Streamer.getContext().reportWarning( + CFI.getLoc(), + "skipping SFrame FDE; .cfi_escape with unknown effects"); + return false; + } + } + return true; + } + // Add the effects of CFI to the current FDE, creating a new FRE when // necessary. bool handleCFI(SFrameFDE &FDE, SFrameFRE &FRE, const MCCFIInstruction &CFI) { @@ -265,8 +409,9 @@ class SFrameEmitterImpl { FRE = FDE.SaveState.pop_back_val(); return true; case MCCFIInstruction::OpEscape: - // TODO: Implement. Will use FDE. - return true; + // This is a string of bytes that contains an aribtrary dwarf-expression + // that may or may not affect uwnind info. + return IsCFIEscapeSafe(FDE, FRE, CFI); default: // Instructions that don't affect the CFA, RA, and SP can be safely // ignored. diff --git a/llvm/test/MC/ELF/cfi-sframe-cfi-escape-errors.s b/llvm/test/MC/ELF/cfi-sframe-cfi-escape-errors.s new file mode 100644 index 0000000000000..1304d5b7d1f57 --- /dev/null +++ b/llvm/test/MC/ELF/cfi-sframe-cfi-escape-errors.s @@ -0,0 +1,37 @@ +# RUN: llvm-mc --filetype=obj --gsframe -triple x86_64 %s -o %t.o 2>&1 | FileCheck %s +# RUN: llvm-readelf --sframe %t.o | FileCheck %s --check-prefix=NOFDES + +# Tests that .cfi_escape sequences that are unrepresentable in sframe warn +# and do not produce FDEs. + + .align 1024 +cfi_escape_sp: + .cfi_startproc + .long 0 +# Setting SP via other registers makes it unrepresentable in sframe +# DW_CFA_expression,reg 0x7,length 2,DW_OP_breg6,SLEB(-8) +# CHECK: skipping SFrame FDE; .cfi_escape DW_CFA_expression with SP reg 7 + .cfi_escape 0x10, 0x7, 0x2, 0x76, 0x78 + .long 0 +.cfi_endproc + +cfi_escape_args_sp: + .cfi_startproc + .long 0 +# DW_CFA_GNU_args_size is not OK if cfa is SP +# CHECK: skipping SFrame FDE; .cfi_escape DW_CFA_GNU_args_size with non frame-pointer CFA + .cfi_escape 0x2e, 0x20 + .cfi_endproc + +cfi_escape_val_offset: + .cfi_startproc + .long 0 + .cfi_def_cfa_offset 16 +# DW_CFA_val_offset,rbp,ULEB scaled offset(16) +# CHECK: skipping SFrame FDE; .cfi_escape DW_CFA_val_offset with FP reg 6 + .cfi_escape 0x14,0x6,0x2 + .long 0 + .cfi_endproc + + +# NOFDES: Num FDEs: 0 diff --git a/llvm/test/MC/ELF/cfi-sframe-cfi-escape.s b/llvm/test/MC/ELF/cfi-sframe-cfi-escape.s new file mode 100644 index 0000000000000..73120157b46d5 --- /dev/null +++ b/llvm/test/MC/ELF/cfi-sframe-cfi-escape.s @@ -0,0 +1,43 @@ +# RUN: llvm-mc --filetype=obj --gsframe -triple x86_64 %s -o %t.o +# RUN: llvm-readelf --sframe %t.o | FileCheck %s + +# Tests that .cfi_escape sequences that are are ok to pass through work. + + .align 1024 +cfi_escape_ok: + .cfi_startproc + .long 0 + .cfi_def_cfa_offset 16 + # Uninteresting register +# DW_CFA_expression,reg 0xc,length 2,DW_OP_breg6,SLEB(-8) + .cfi_escape 0x10,0xc,0x2,0x76,0x78 +# DW_CFA_nop + .cfi_escape 0x0 + .cfi_escape 0x0,0x0,0x0,0x0 + # Uninteresting register +# DW_CFA_val_offset,reg 0xc,ULEB scaled offset + .cfi_escape 0x14,0xc,0x4 + .long 0 + .cfi_endproc + +cfi_escape_gnu_args_fp: + .cfi_startproc + .long 0 + .cfi_def_cfa_register 6 + .long 0 +# DW_CFA_GNU_args_size is OK if cfa is FP + .cfi_escape 0x2e, 0x20 + .cfi_endproc + +cfi_escape_long_expr: + .cfi_startproc + .long 0 + .cfi_def_cfa_offset 16 +# This is a long, but valid, dwarf expression without sframe +# implications. An FDE can still be created. +# DW_CFA_val_offset,rcx,ULEB scaled offset(16), DW_CFA_expr,r10,length,DW_OP_deref,SLEB(-8) + .cfi_escape 0x14,0x2,0x2,0x10,0xa,0x2,0x76,0x78 + .long 0 + .cfi_endproc + +# CHECK: Num FDEs: 3 diff --git a/utils/bazel/llvm-project-overlay/llvm/BUILD.bazel b/utils/bazel/llvm-project-overlay/llvm/BUILD.bazel index 8f607c7ce087e..04bd20d69d94e 100644 --- a/utils/bazel/llvm-project-overlay/llvm/BUILD.bazel +++ b/utils/bazel/llvm-project-overlay/llvm/BUILD.bazel @@ -577,6 +577,7 @@ cc_library( deps = [ ":BinaryFormat", ":DebugInfoCodeView", + ":DebugInfoDWARFLowLevel", ":Support", ":TargetParser", ":config", From 38c36496e19fad2a04b5caa430a431fc6feb6b82 Mon Sep 17 00:00:00 2001 From: Sterling Augustine Date: Mon, 6 Oct 2025 10:28:33 -0700 Subject: [PATCH 2/7] Improve test case, fix capitalization. --- llvm/lib/MC/MCSFrame.cpp | 4 ++-- llvm/test/MC/ELF/cfi-sframe-cfi-escape.s | 3 +++ 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/llvm/lib/MC/MCSFrame.cpp b/llvm/lib/MC/MCSFrame.cpp index c00262471f123..dcfaa6b375244 100644 --- a/llvm/lib/MC/MCSFrame.cpp +++ b/llvm/lib/MC/MCSFrame.cpp @@ -220,7 +220,7 @@ class SFrameEmitterImpl { // an FDE in those cases. Allow any that are known safe. It is likely that // more thorough test cases could refine this code, but it handles the most // important ones compatibly with gas. - bool IsCFIEscapeSafe(SFrameFDE &FDE, const SFrameFRE &FRE, + bool isCFIEscapeSafe(SFrameFDE &FDE, const SFrameFRE &FRE, const MCCFIInstruction &CFI) { const MCAsmInfo *AI = Streamer.getContext().getAsmInfo(); DWARFDataExtractorSimple data(CFI.getValues(), AI->isLittleEndian(), @@ -411,7 +411,7 @@ class SFrameEmitterImpl { case MCCFIInstruction::OpEscape: // This is a string of bytes that contains an aribtrary dwarf-expression // that may or may not affect uwnind info. - return IsCFIEscapeSafe(FDE, FRE, CFI); + return isCFIEscapeSafe(FDE, FRE, CFI); default: // Instructions that don't affect the CFA, RA, and SP can be safely // ignored. diff --git a/llvm/test/MC/ELF/cfi-sframe-cfi-escape.s b/llvm/test/MC/ELF/cfi-sframe-cfi-escape.s index 73120157b46d5..92e2e9417ae90 100644 --- a/llvm/test/MC/ELF/cfi-sframe-cfi-escape.s +++ b/llvm/test/MC/ELF/cfi-sframe-cfi-escape.s @@ -23,6 +23,9 @@ cfi_escape_ok: cfi_escape_gnu_args_fp: .cfi_startproc .long 0 +# DW_CFA_GNU_args_size is OK arg size is zero + .cfi_escape 0x2e, 0x0 + .long 0 .cfi_def_cfa_register 6 .long 0 # DW_CFA_GNU_args_size is OK if cfa is FP From 4e230d747d316a785750a4ef174462084d02603f Mon Sep 17 00:00:00 2001 From: Sterling Augustine Date: Mon, 6 Oct 2025 10:31:17 -0700 Subject: [PATCH 3/7] Fix comment --- llvm/lib/MC/MCSFrame.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/llvm/lib/MC/MCSFrame.cpp b/llvm/lib/MC/MCSFrame.cpp index dcfaa6b375244..ae78e0e0bddea 100644 --- a/llvm/lib/MC/MCSFrame.cpp +++ b/llvm/lib/MC/MCSFrame.cpp @@ -243,7 +243,7 @@ class SFrameEmitterImpl { } // This loop deals with are dwarf::CFIProgram::Instructions. Everywhere - // else, This file deals with MCCFIInstructions.. + // else this file deals with MCCFIInstructions.. for (const dwarf::CFIProgram::Instruction &I : P) { switch (I.Opcode) { case dwarf::DW_CFA_nop: From 3946b79f6eb3ff8a2ec59c6334017917455b46ab Mon Sep 17 00:00:00 2001 From: Sterling Augustine Date: Mon, 6 Oct 2025 11:44:20 -0700 Subject: [PATCH 4/7] Properly handle zero-sized DW_CFA_GNU_args_size --- llvm/lib/MC/MCSFrame.cpp | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/llvm/lib/MC/MCSFrame.cpp b/llvm/lib/MC/MCSFrame.cpp index ae78e0e0bddea..df916cf26731d 100644 --- a/llvm/lib/MC/MCSFrame.cpp +++ b/llvm/lib/MC/MCSFrame.cpp @@ -298,7 +298,11 @@ class SFrameEmitterImpl { return false; } } break; - case dwarf::DW_CFA_GNU_args_size: + case dwarf::DW_CFA_GNU_args_size: { + auto Size = I.getOperandAsSigned(P, 0); + // Zero size doesn't affect the cfa. + if (Size && *Size == 0) + break; if (FRE.Info.getBaseRegister() != BaseReg::FP) { Streamer.getContext().reportWarning( CFI.getLoc(), @@ -306,7 +310,7 @@ class SFrameEmitterImpl { "with non frame-pointer CFA")); return false; } - break; + } break; // Cases that gas doesn't specially handle. TODO: Some of these could be // analyzed and handled instead of just punting. But these are uncommon, // or should be written as normal cfi directives. Some will need fixes to From 45f8eee80581ab057010ba511532a71f7127f3aa Mon Sep 17 00:00:00 2001 From: Sterling Augustine Date: Mon, 6 Oct 2025 16:44:26 -0700 Subject: [PATCH 5/7] Fix some typos. Add some comments. --- llvm/lib/MC/MCSFrame.cpp | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/llvm/lib/MC/MCSFrame.cpp b/llvm/lib/MC/MCSFrame.cpp index df916cf26731d..4b47549dc0b87 100644 --- a/llvm/lib/MC/MCSFrame.cpp +++ b/llvm/lib/MC/MCSFrame.cpp @@ -220,6 +220,7 @@ class SFrameEmitterImpl { // an FDE in those cases. Allow any that are known safe. It is likely that // more thorough test cases could refine this code, but it handles the most // important ones compatibly with gas. + // Returns true if the CFI escape sequence is safe for sframes. bool isCFIEscapeSafe(SFrameFDE &FDE, const SFrameFRE &FRE, const MCCFIInstruction &CFI) { const MCAsmInfo *AI = Streamer.getContext().getAsmInfo(); @@ -242,8 +243,8 @@ class SFrameEmitterImpl { return false; } - // This loop deals with are dwarf::CFIProgram::Instructions. Everywhere - // else this file deals with MCCFIInstructions.. + // This loop deals with dwarf::CFIProgram::Instructions. Everywhere else + // this file deals with MCCFIInstructions. for (const dwarf::CFIProgram::Instruction &I : P) { switch (I.Opcode) { case dwarf::DW_CFA_nop: @@ -360,7 +361,7 @@ class SFrameEmitterImpl { } // Add the effects of CFI to the current FDE, creating a new FRE when - // necessary. + // necessary. Return true if the CFI is representable in the sframe format. bool handleCFI(SFrameFDE &FDE, SFrameFRE &FRE, const MCCFIInstruction &CFI) { switch (CFI.getOperation()) { case MCCFIInstruction::OpDefCfaRegister: @@ -413,11 +414,11 @@ class SFrameEmitterImpl { FRE = FDE.SaveState.pop_back_val(); return true; case MCCFIInstruction::OpEscape: - // This is a string of bytes that contains an aribtrary dwarf-expression - // that may or may not affect uwnind info. + // This is a string of bytes that contains an arbitrary dwarf-expression + // that may or may not affect unwind info. return isCFIEscapeSafe(FDE, FRE, CFI); default: - // Instructions that don't affect the CFA, RA, and SP can be safely + // Instructions that don't affect the CFA, RA, and FP can be safely // ignored. return true; } From 529700bab88745af7fa42e026f5d47b5e9ab352a Mon Sep 17 00:00:00 2001 From: Sterling Augustine Date: Wed, 8 Oct 2025 13:21:29 -0700 Subject: [PATCH 6/7] Switch assertion because the parser should have failed. Fix comment. --- llvm/lib/MC/MCSFrame.cpp | 7 ++----- llvm/test/MC/ELF/cfi-sframe-cfi-escape.s | 2 +- 2 files changed, 3 insertions(+), 6 deletions(-) diff --git a/llvm/lib/MC/MCSFrame.cpp b/llvm/lib/MC/MCSFrame.cpp index 4b47549dc0b87..f5956c574c1b1 100644 --- a/llvm/lib/MC/MCSFrame.cpp +++ b/llvm/lib/MC/MCSFrame.cpp @@ -254,11 +254,8 @@ class SFrameEmitterImpl { // a problem, but allow others through. As an even more special case, // allow SP + 0. auto Reg = I.getOperandAsUnsigned(P, 0); - if (!Reg) { - Streamer.getContext().reportWarning( - CFI.getLoc(), - "skipping SFrame FDE; .cfi_escape with unknown effects"); - } + // The parser should have failed in this case. + assert(Reg && "DW_CFA_val_offset with no register."); bool SPOk = true; if (*Reg == SPReg) { auto Opnd = I.getOperandAsSigned(P, 1); diff --git a/llvm/test/MC/ELF/cfi-sframe-cfi-escape.s b/llvm/test/MC/ELF/cfi-sframe-cfi-escape.s index 92e2e9417ae90..1e571587a132d 100644 --- a/llvm/test/MC/ELF/cfi-sframe-cfi-escape.s +++ b/llvm/test/MC/ELF/cfi-sframe-cfi-escape.s @@ -23,7 +23,7 @@ cfi_escape_ok: cfi_escape_gnu_args_fp: .cfi_startproc .long 0 -# DW_CFA_GNU_args_size is OK arg size is zero +# DW_CFA_GNU_args_size is OK if arg size is zero .cfi_escape 0x2e, 0x0 .long 0 .cfi_def_cfa_register 6 From 08df48e4b23e5d379973471c9dfe8ea0954437c9 Mon Sep 17 00:00:00 2001 From: Sterling Augustine Date: Tue, 14 Oct 2025 11:39:56 -0700 Subject: [PATCH 7/7] Fix typo --- llvm/test/MC/ELF/cfi-sframe-cfi-escape.s | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/llvm/test/MC/ELF/cfi-sframe-cfi-escape.s b/llvm/test/MC/ELF/cfi-sframe-cfi-escape.s index 1e571587a132d..a39d6c2124d13 100644 --- a/llvm/test/MC/ELF/cfi-sframe-cfi-escape.s +++ b/llvm/test/MC/ELF/cfi-sframe-cfi-escape.s @@ -1,7 +1,7 @@ # RUN: llvm-mc --filetype=obj --gsframe -triple x86_64 %s -o %t.o # RUN: llvm-readelf --sframe %t.o | FileCheck %s -# Tests that .cfi_escape sequences that are are ok to pass through work. +# Tests that .cfi_escape sequences that are ok to pass through work. .align 1024 cfi_escape_ok: