[MC][AsmParser] Diagnose improperly nested .cfi frames

[jroelofs]

Summary

This showed up when simplifying some large testcase, where the cfi directives became out of sync with the proc's they enclose.

rdar://111459507

[iains]

on the subject of is there any way to diagnose:

• if we encounter either a linker-visible or global symbol after a cfi_startproc but before the next cfi_endproc [in the same section], that would seem to indicate bad nesting?
• I suppose that there could be some weird code-gen that multiplexed the output between sections, so seeing a section switch before the closing cfi_endproc is not sufficient

[jroelofs]

• I suppose that there could be some weird code-gen that multiplexed the output between sections, so seeing a section switch before the closing cfi_endproc is not sufficient

I tried this one and found it broke a lot of JIT tests, as well as some cases with inline asm.

• if we encounter either a linker-visible or global symbol after a cfi_startproc but before the next cfi_endproc [in the same section], that would seem to indicate bad nesting?

I'll give this one a shot. I suspect it may also trip up on the same sorts of issues.

[MaskRay]

D153167 is related. Due to Mach-O's .subsections_via_symbols mechanism, non-private labels cannot appear between .cfi_startproc/.cfi_endproc. I think AArch64/cfi-bad-nesting.s deserves a getContext().reportError.

[jroelofs]

Address review feedback from @iains and @MaskRay

[MaskRay]

Looks great!

llvm/lib/MC/MCParser/AsmParser.cpp

1953

Add a test for isWeakExternal?

llvm/test/MC/MachO/AArch64/cfi-bad-nesting.s

1 ↗

(On Diff #558108)

We could also use test/MC/MachO/ since this is generic, not specific to AArch64/

[MaskRay]

[MC][AsmParse] Diagnose improperly nested .cfi frames

Fixed a typo in the title.

[jroelofs]

llvm/lib/MC/MCParser/AsmParser.cpp

1953

I realize now that that is a COFF feature, and .subsections_via_symbols is a MachO thing. I'll leave this out instead.

llvm/test/MC/MachO/AArch64/cfi-bad-nesting.s

1 ↗

(On Diff #558108)

Sounds good. I'll add the appropriate REQUIRES: too.

[aeubanks]

this is breaking runtimes builds of aarch64 builtins, e.g.

[294/404] Building ASM object CMakeFiles\clang_rt.builtins-aarch64.dir\outline_atomic_helpers.dir\outline_atomic_cas16_1.S.obj
FAILED: CMakeFiles/clang_rt.builtins-aarch64.dir/outline_atomic_helpers.dir/outline_atomic_cas16_1.S.obj 
C:\b\s\w\ir\cache\builder\src\third_party\llvm-build\Release+Asserts\.\bin\clang-cl.exe -target aarch64-pc-windows-msvc  /nologo -DHAS_ASM_LSE -DL_cas -DMODEL=1 -DSIZE=16 -IC:\b\s\w\ir\cache\builder\src\third_party\llvm\compiler-rt\lib\builtins -fno-lto /Zl -fno-builtin -DCOMPILER_RT_HAS_FLOAT16 /showIncludes /FoCMakeFiles\clang_rt.builtins-aarch64.dir\outline_atomic_helpers.dir\outline_atomic_cas16_1.S.obj /FdCMakeFiles\clang_rt.builtins-aarch64.dir\clang_rt.builtins-aarch64.pdb -c -- C:\b\s\w\ir\cache\builder\src\third_party\llvm-build\Release+Asserts\runtimes\builtins-aarch64-pc-windows-msvc-bins\outline_atomic_helpers.dir\outline_atomic_cas16_1.S
C:\\b\\s\\w\\ir\\cache\\builder\\src\\third_party\\llvm-build\\Release+Asserts\\runtimes\\builtins-aarch64-pc-windows-msvc-bins\\outline_atomic_helpers.dir\\outline_atomic_cas16_1.S:145:131: error: non-private labels cannot appear between .cfi_startproc / .cfi_endproc pairs
 .text ; .balign 16 ; .globl __aarch64_cas16_relax ; .def __aarch64_cas16_relax ; .scl 2 ; .type 32 ; .endef ; ; .cfi_startproc ; __aarch64_cas16_relax: ;
                                                                                                                                  ^
C:\\b\\s\\w\\ir\\cache\\builder\\src\\third_party\\llvm-build\\Release+Asserts\\runtimes\\builtins-aarch64-pc-windows-msvc-bins\\outline_atomic_helpers.dir\\outline_atomic_cas16_1.S:145:114: error: previous .cfi_startproc was here
 .text ; .balign 16 ; .globl __aarch64_cas16_relax ; .def __aarch64_cas16_relax ; .scl 2 ; .type 32 ; .endef ; ; .cfi_startproc ; __aarch64_cas16_relax: ;

this is at https://github.com/llvm/llvm-project/blob/6a126e279dedc9fb8c204d29cfc227b0652ffe6c/compiler-rt/lib/builtins/aarch64/lse.S#L145 (need to follow a bunch of macros). the builtins should be cleaned up first before landing this

[jroelofs]

Ok. I'll revert.

[jroelofs]

Addressed in 7939ce39dac0, and re-landed in 4323da926f12

[aeubanks]

thanks!

[mstorsjo]

This breaks building libffi for 32 bit arm; the following snippet does fallthrough from one global function to another, while sharing one single CFI region for all of them: https://github.com/libffi/libffi/blob/master/src/arm/sysv.S#L139-L213

Based on this patch, the code in libffi looks like it would be invalid and intentionally should fail to compile.

However, I noticed a different surprising detail here; it looks like this check fails to trigger on Linux (ELF in general I presume?) targets, while it does trigger on MachO and COFF. This can be tested with something like this:

.globl ffi_call_VFP
ffi_call_VFP:
.cfi_startproc
nop
.globl ffi_call_SYSV
ffi_call_SYSV:
nop
.cfi_endproc

$ clang -target aarch64-linux-gnu -c fallthrough.s 
$ clang -target aarch64-apple-darwin -c fallthrough.s 
fallthrough.s:7:1: error: non-private labels cannot appear between .cfi_startproc / .cfi_endproc pairs 
ffi_call_SYSV:
^
fallthrough.s:3:2: error: previous .cfi_startproc was here
.cfi_startproc 
^
$ clang -target aarch64-windows-gnu -c fallthrough.s 
fallthrough.s:7:1: error: non-private labels cannot appear between .cfi_startproc / .cfi_endproc pairs
ffi_call_SYSV:
^
fallthrough.s:3:2: error: previous .cfi_startproc was here
.cfi_startproc
^

[mstorsjo]

For cross reference, I filed an issue with libffi at https://github.com/libffi/libffi/issues/807 about this.

[jroelofs]

In D155245#4657075, @mstorsjo wrote:

This breaks building libffi for 32 bit arm; the following snippet does fallthrough from one global function to another, while sharing one single CFI region for all of them: https://github.com/libffi/libffi/blob/master/src/arm/sysv.S#L139-L213

Based on this patch, the code in libffi looks like it would be invalid and intentionally should fail to compile.

I may need to restrict this to just MachO, and possibly only when .subsections_via_symbols is present. If this is blocking you, please feel free to revert.

However, I noticed a different surprising detail here; it looks like this check fails to trigger on Linux (ELF in general I presume?) targets, while it does trigger on MachO and COFF. This can be tested with something like this:

.globl ffi_call_VFP
ffi_call_VFP:
.cfi_startproc
nop
.globl ffi_call_SYSV
ffi_call_SYSV:
nop
.cfi_endproc

$ clang -target aarch64-linux-gnu -c fallthrough.s 
$ clang -target aarch64-apple-darwin -c fallthrough.s 
fallthrough.s:7:1: error: non-private labels cannot appear between .cfi_startproc / .cfi_endproc pairs 
ffi_call_SYSV:
^
fallthrough.s:3:2: error: previous .cfi_startproc was here
.cfi_startproc 
^
$ clang -target aarch64-windows-gnu -c fallthrough.s 
fallthrough.s:7:1: error: non-private labels cannot appear between .cfi_startproc / .cfi_endproc pairs
ffi_call_SYSV:
^
fallthrough.s:3:2: error: previous .cfi_startproc was here
.cfi_startproc
^

That is really weird. I'll investigate this week.

https://github.com/llvm/llvm-project/issues/72802

In D155245#4657076, @mstorsjo wrote:

For cross reference, I filed an issue with libffi at https://github.com/libffi/libffi/issues/807 about this.

Thank you!

[mstorsjo]

In D155245#4657083, @jroelofs wrote:

In D155245#4657075, @mstorsjo wrote:

This breaks building libffi for 32 bit arm; the following snippet does fallthrough from one global function to another, while sharing one single CFI region for all of them: https://github.com/libffi/libffi/blob/master/src/arm/sysv.S#L139-L213

Based on this patch, the code in libffi looks like it would be invalid and intentionally should fail to compile.

I may need to restrict this to just MachO, and possibly only when .subsections_via_symbols is present. If this is blocking you, please feel free to revert.

Thanks! I pushed a revert now, and thanks for considering restricting the scope of this error!

However, I noticed a different surprising detail here; it looks like this check fails to trigger on Linux (ELF in general I presume?) targets, while it does trigger on MachO and COFF.

That is really weird. I'll investigate this week.

https://github.com/llvm/llvm-project/issues/72802

Thanks, much appreciated!

[jroelofs]

re-re-re-landed in d506aa4edfa66074db3dc1fa84da9d9c80d71500

[hans]

We're still having issues (https://crbug.com/1504532) due to this, because we're building some compiler-rt code from llvm 17 with clang from head.

That's something for us to figure out, but it makes we wonder if perhaps this diagnostic should be a warning rather than a hard error by default? For example, consider someone using clang 18 to build older llvm version for bisection or similar.

[hans]

Ping on thoughts about this being a warning instead?

llvm/lib/MC/MCParser/AsmParser.cpp

private:
   MCContext &Ctx;
   MCStreamer &Out;
   const MCAsmInfo &MAI;
   SourceMgr &SrcMgr;
   SourceMgr::DiagHandlerTy SavedDiagHandler;
   void *SavedDiagContext;
   std::unique_ptr<MCAsmParserExtension> PlatformParser;
   SMLoc StartTokLoc;
+  std::optional<SMLoc> CFIStartProcLoc;
 
   /// This is the current buffer index we're lexing from as managed by the
   /// SourceMgr object.
   unsigned CurBuffer;
 
   AsmCond TheCondState;
   std::vector<AsmCond> TheCondStack;
 
if (Lexer.is(AsmToken::Colon) && getTargetParser().isLabel(ID)) {
     }
 
     // Consume any end of statement token, if present, to avoid spurious
     // addBlankLine calls().
     if (getTok().is(AsmToken::EndOfStatement)) {
       Lex();
     }
 
+    if (CFIStartProcLoc && Sym->isExternal())
+      return Error(StartTokLoc, "non-private labels cannot appear between "
+                                ".cfi_startproc / .cfi_endproc pairs") &&
+             Error(*CFIStartProcLoc, "previous .cfi_startproc was here");
+
     if (discardLTOSymbol(IDVal))
       return false;
 
     getTargetParser().doBeforeLabelEmit(Sym, IDLoc);
 
     // Emit the label.
     if (!getTargetParser().isParsingMSInlineAsm())
       Out.emitLabel(Sym, IDLoc);
bool AsmParser::parseDirectiveCFISections() {
   }
   getStreamer().emitCFISections(EH, Debug);
   return false;
 }
 
 /// parseDirectiveCFIStartProc
 /// ::= .cfi_startproc [simple]
 bool AsmParser::parseDirectiveCFIStartProc() {
+  CFIStartProcLoc = StartTokLoc;
+
   StringRef Simple;
   if (!parseOptionalToken(AsmToken::EndOfStatement)) {
     if (check(parseIdentifier(Simple) || Simple != "simple",
               "unexpected token") ||
         parseEOL())
       return true;
   }
 
   // TODO(kristina): Deal with a corner case of incorrect diagnostic context
   // being produced if this directive is emitted as part of preprocessor macro
   // expansion which can *ONLY* happen if Clang's cc1as is the API consumer.
   // Tools like llvm-mc on the other hand are not affected by it, and report
   // correct context information.
   getStreamer().emitCFIStartProc(!Simple.empty(), Lexer.getLoc());
   return false;
 }
 
 /// parseDirectiveCFIEndProc
 /// ::= .cfi_endproc
 bool AsmParser::parseDirectiveCFIEndProc() {
+  CFIStartProcLoc = std::nullopt;
+
   if (parseEOL())
     return true;
+
   getStreamer().emitCFIEndProc();
   return false;
 }
 
 /// parse register name or number.
 bool AsmParser::parseRegisterOrRegisterNumber(int64_t &Register,
                                               SMLoc DirectiveLoc) {
   MCRegister RegNo;

llvm/test/MC/AArch64/cfi-bad-nesting.s

+; RUN: not llvm-mc -triple arm64-apple-darwin %s -filetype=obj -o /dev/null 2>&1 | FileCheck %s
+
+; REQUIRES: aarch64-registered-target
+
+  .section  __TEXT,locomotive,regular,pure_instructions
+
+  .globl  _locomotive
+  .p2align  2
+_locomotive:
+  .cfi_startproc
+  ret
+
+  ; It is invalid to have a non-private label between .cfi_startproc / .cfi_endproc
+  .section  __TEXT,__text,regular,pure_instructions
+  .globl  _caboose
+  .p2align  2
+_caboose:
+; CHECK: [[#@LINE-1]]:1: error: non-private labels cannot appear between .cfi_startproc / .cfi_endproc pairs
+; CHECK: [[#@LINE-9]]:2: error: previous .cfi_startproc was here
+  ret
+  .cfi_endproc
+
+.subsections_via_symbols
 No newline at end of file