[MC][DWARF][AsmParser] Ensure nested CFI frames are diagnosed to avoid a crash or bad codegen in Dwarf streamer later on.

[kristina]

Summary

Integrated assembler currently does not check whether there is an an existing CFI frame when .cfi_startproc is used, forwarding it down to MC layer. This patch makes integrated assembler check for it so it can report it more gracefully and avoid letting this leak into the MCStreamer.

The actual error reporting code in MCStreamer seems to be missing a return which allows this to trickle down, but this revision only addresses it in integrated assembler as location information is guaranteed to be available and is the part exposed to the user as opposed to MCStreamer itself. I'm not sure if the missing bailout after reportError is intentional or not, and I think a separate diff would be better for that if it's not.

This causes the following assert to fail down the line in Dwarf streamer since it shouldn't have made it that far:

<unknown>:0: error: starting new .cfi frame before finishing the previous one
clang-8: /SourceCache/llvm-trunk-8.0.4104/lib/MC/MCExpr.cpp:176: llvm::MCSymbolRefExpr::MCSymbolRefExpr(const llvm::MCSymbol *, llvm::MCSymbolRefExpr::VariantKind, const llvm::MCAsmInfo *, llvm::SMLoc): Assertion `Symbol' failed.

This patch makes it evaluate the conditional twice in successful cases (second time in MCStreamer) but only for AsmParser for that directive so that shouldn't lead to major performance regressions. This fixes the assert and presents a diagnostic error with accurate context/line information.

However if this happens within a PP evaluation, the correct line is reported but the contextual information is not correct (hence the comment), like this:

os/os_thread_x86_64.S:269:2: error: .cfi_startproc cannot be used in an existing frame
 pushq %rbp; movq %rsp, %rbp;

This is still better than an assertion down the line, and I don't want to violate MCStreamer/AsmParser layering, though MCStreamer with optional SMLoc could be a better place for this? If anyone can suggest a way to deal with PP macros in a more friendly way I'll amend the revision.

Have tested it on ELF/Linux/x86_64, with patch, instead of a hard assertion and a crash it emits the a diagnostic.

[lebedev.ri]

I don't know anything about this code, but there should probably be a test for this change, somewhere.

[kristina]

Yes, though I specifically tried to avoid touching MCStreamer itself for that reason since regressions there would be much worse than in AsmParser, and I'm not entirely sure how to test this either, since it's an error right down the line in MCStreamer from which it doesn't bail out after calling reportError. Would rather wait for some input from people familiar with the MC layer, as since this mostly tries to address this bug being triggered from -cc1as and this seems like the appropriate layer.

[mgrang]

lib/MC/MCParser/AsmParser.cpp

3891 ↗

(On Diff #164063)

Please remove the extra newline.

[kristina]

Removed extra newline.

[rnk]

I'd suggest fixing this in MCStreamer::EmitCFIStartProc by having it recover in some way that doesn't lead to future assertions. Also, it should be possible to write a standalone .s test case for this.

[kristina]

In D51695#1227779, @rnk wrote:

I'd suggest fixing this in MCStreamer::EmitCFIStartProc by having it recover in some way that doesn't lead to future assertions. Also, it should be possible to write a standalone .s test case for this.

Oh I can definitely do a small reproduction of the bug in an cfi_nested_crash.s type thing and use that as a test case for any future regressions if that's what people keep saying.

The fix in MCStreamer would be to return after the reporting the error and to also add optional SMLoc to the signature so it could report the error when used as part of integrated as, which is what I wasn't sure about, since it's missing a return there, I don't know if it's on purpose or not, I really want someone familiar with this part to comment, the AsmParser is was a provisional source-location-aware fix to the consumer visible part of this stuff (as -cc1as or inline directives) that avoids crashes or bad DWARF tables as a band-aid.

The actual the questionable code below is what I really want an opinion on (lib/MC/MCStreamer.cpp):

void MCStreamer::EmitCFIStartProc(bool IsSimple) {
  if (hasUnfinishedDwarfFrameInfo())
    // TODO(kristina): Is this intentionally falling through or not? If so
    // it will assert down the line if used from cc1as in DWARF streamer.
    getContext().reportError(
        SMLoc(), "starting new .cfi frame before finishing the previous one");

  MCDwarfFrameInfo Frame;
  Frame.IsSimple = IsSimple;
  EmitCFIStartProcImpl(Frame);

  const MCAsmInfo* MAI = Context.getAsmInfo();
  if (MAI) {
    for (const MCCFIInstruction& Inst : MAI->getInitialFrameState()) {
      if (Inst.getOperation() == MCCFIInstruction::OpDefCfa ||
          Inst.getOperation() == MCCFIInstruction::OpDefCfaRegister) {
        Frame.CurrentCfaRegister = Inst.getRegister();
      }
    }
  }
  
  // XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX
  // This being let through will cause the crash down the line once
  // it reaches the DWARF streamer.
  DwarfFrameInfos.push_back(Frame);
}

[rnk]

Passing the source location through to MCStreamer seems like a good idea. Before rL315264, this code used to use report_fatal_error. My goal was to make it less crummy, and make sure errors get diagnosed when emitting assembly or emitting object files.

[kristina]

Alright, I'll handle it in MCStreamer directly instead, if SMLoc is provided will try to print an error with more context seeing as it could be from -cc1as. Would it be worth returning and bailing out instead of just reporting an error even if SMLoc isn't provided?

[kristina]

This is a testcase for the crash, patch fixes it, though I'll amend it to work on MCStreamer level as per what @rnk suggested. I'm going to sound really stupid asking but where should the testcase be, is it a FileCheck based one? I just made two targets, one with pre-patch toolchain and one with current (my working tree) toolchain which handles this just fine. First one crashes, second one doesn't, not sure where tests like that go.

#if !defined(__x86_64__)
	#error Unsupported architecture
#endif   

.align 2, 0x90
.type Cfi_level_0, @function
.global Cfi_level_0
.hidden Cfi_level_0
.cfi_startproc
Cfi_level_0:
	movq $123, %rax
	ret

.align 2, 0x90
.type Cfi_level_1, @function
.global Cfi_level_1
.hidden Cfi_level_1
.cfi_startproc
Cfi_level_1:
	movq $321, %rax
	int  $3

.cfi_endproc; .size Cfi_level_0, . - Cfi_level_0

[rnk]

In D51695#1228444, @kristina wrote:

This is a testcase for the crash, patch fixes it, though I'll amend it to work on MCStreamer level as per what @rnk suggested. I'm going to sound really stupid asking but where should the testcase be, is it a FileCheck based one? I just made two targets, one with pre-patch toolchain and one with current (my working tree) toolchain which handles this just fine. First one crashes, second one doesn't, not sure where tests like that go.

Yeah, we definitely want a lit test. I think the one we already have exercises this code path, but because it doesn't create an object file, it doesn't assert. I think if you apply this patch, the test will start failing, and your fix should make it pass again:

diff --git a/llvm/test/MC/X86/cfi-scope-errors.s b/llvm/test/MC/X86/cfi-scope-errors.s
index a61f817f741e..91d42b9662b6 100644
--- a/llvm/test/MC/X86/cfi-scope-errors.s
+++ b/llvm/test/MC/X86/cfi-scope-errors.s
@@ -1,4 +1,4 @@
-# RUN: not llvm-mc %s -triple x86_64-linux -o /dev/null 2>&1 | FileCheck %s --implicit-check-not=error:
+# RUN: not llvm-mc %s -triple x86_64-linux -filetype=obj -o /dev/null 2>&1 | FileCheck %s --implicit-check-not=error:

 # FIXME: Push source locations into diagnostics.

If you thread through the SMLoc you'll even address the FIXME.

[kristina]

Address comments by @rnk and redo this on MCStreamer level adding SMLoc information to the diagnostic as a bonus. Room for followup patches to propagate more source locations for other directives but for now this is out of scope of this patch as it deals with the assembler crash.

[kristina]

Seems to be fine (ignore the 8 failed tests, they're IPC related and fail because of the host environment constraints).

ninja check-llvm

-snip-

********************
Testing Time: 80.11s
********************
Failing Tests (8):
    LLVM :: ExecutionEngine/MCJIT/eh-lg-pic.ll
    LLVM :: ExecutionEngine/MCJIT/eh.ll
    LLVM :: ExecutionEngine/MCJIT/multi-module-eh-a.ll
    LLVM :: ExecutionEngine/MCJIT/remote/eh.ll
    LLVM :: ExecutionEngine/OrcMCJIT/eh-lg-pic.ll
    LLVM :: ExecutionEngine/OrcMCJIT/eh.ll
    LLVM :: ExecutionEngine/OrcMCJIT/multi-module-eh-a.ll
    LLVM :: ExecutionEngine/OrcMCJIT/remote/eh.ll

  Expected Passes    : 16602
  Expected Failures  : 50
  Unsupported Tests  : 11500
  Unexpected Failures: 8

[kristina]

Ping.

[rnk]

lgtm Thanks!

llvm/trunk/include/llvm/MC/MCStreamer.h

virtual void emitAbsoluteSymbolDiff(const MCSymbol *Hi, const MCSymbol *Lo,
                                       unsigned Size);
 
   /// Emit the absolute difference between two symbols encoded with ULEB128.
   virtual void emitAbsoluteSymbolDiffAsULEB128(const MCSymbol *Hi,
                                                const MCSymbol *Lo);
 
   virtual MCSymbol *getDwarfLineTableSymbol(unsigned CUID);
   virtual void EmitCFISections(bool EH, bool Debug);
-  void EmitCFIStartProc(bool IsSimple);
+  void EmitCFIStartProc(bool IsSimple, SMLoc Loc = SMLoc());
   void EmitCFIEndProc();
   virtual void EmitCFIDefCfa(int64_t Register, int64_t Offset);
   virtual void EmitCFIDefCfaOffset(int64_t Offset);
   virtual void EmitCFIDefCfaRegister(int64_t Register);
   virtual void EmitCFIOffset(int64_t Register, int64_t Offset);
   virtual void EmitCFIPersonality(const MCSymbol *Sym, unsigned Encoding);
   virtual void EmitCFILsda(const MCSymbol *Sym, unsigned Encoding);
   virtual void EmitCFIRememberState();

llvm/trunk/lib/MC/MCParser/AsmParser.cpp

 bool AsmParser::parseDirectiveCFIStartProc() {
   StringRef Simple;
   if (!parseOptionalToken(AsmToken::EndOfStatement)) {
     if (check(parseIdentifier(Simple) || Simple != "simple",
               "unexpected token") ||
         parseToken(AsmToken::EndOfStatement))
       return addErrorSuffix(" in '.cfi_startproc' directive");
   }
   
-  getStreamer().EmitCFIStartProc(!Simple.empty());
+  // 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() {
   getStreamer().EmitCFIEndProc();
   return false;

llvm/trunk/lib/MC/MCStreamer.cpp

void MCStreamer::EmitLabel(MCSymbol *Symbol, SMLoc Loc) {
   if (TS)
     TS->emitLabel(Symbol);
 }
 
 void MCStreamer::EmitCFISections(bool EH, bool Debug) {
   assert(EH || Debug);
 }
 
-void MCStreamer::EmitCFIStartProc(bool IsSimple) {
+void MCStreamer::EmitCFIStartProc(bool IsSimple, SMLoc Loc) {
   if (hasUnfinishedDwarfFrameInfo())
-    getContext().reportError(
-        SMLoc(), "starting new .cfi frame before finishing the previous one");
+    return getContext().reportError(
+        Loc, "starting new .cfi frame before finishing the previous one");
 
   MCDwarfFrameInfo Frame;
   Frame.IsSimple = IsSimple;
   EmitCFIStartProcImpl(Frame);
 
   const MCAsmInfo* MAI = Context.getAsmInfo();
   if (MAI) {
     for (const MCCFIInstruction& Inst : MAI->getInitialFrameState()) {

llvm/trunk/test/MC/X86/cfi-open-within-another-crash.s

+# Test for D51695 ensuring there is no crash when two .cfi_startproc are opened
+# without the first one being closed.
+
+# RUN: not llvm-mc %s -filetype=obj -triple=x86_64-unknown-linux -o /dev/null 2>&1 | FileCheck %s
+
+.text
+.globl proc_one
+proc_one:
+ .cfi_startproc
+ 
+.text
+.globl proc_two
+proc_two:
+ .cfi_startproc
+ 
+ .cfi_endproc
+
+# CHECK: error: starting new .cfi frame before finishing the previous one

llvm/trunk/test/MC/X86/cfi-scope-errors.s

-# RUN: not llvm-mc %s -triple x86_64-linux -o /dev/null 2>&1 | FileCheck %s --implicit-check-not=error:
-
-# FIXME: Push source locations into diagnostics.
+# RUN: not llvm-mc %s -triple x86_64-linux -o /dev/null 2>&1 | FileCheck %s
+# RUN: not llvm-mc %s -triple x86_64-linux -filetype=obj -o /dev/null 2>&1 | FileCheck %s
 
 .text
 .cfi_def_cfa rsp, 8
 # CHECK: error: this directive must appear between .cfi_startproc and .cfi_endproc directives
 
 .cfi_startproc
 nop
 
+# TODO(kristina): As Reid suggested, this now supports source locations as a side effect
+# of another patch aimed at fixing the crash that would occur here, however the other
+# ones do not unfortunately. Will address it in a further patch propogating SMLoc down to
+# other CFI directives at which point more LINE checks can be added to ensure proper source
+# location reporting.
+
+# This tests source location correctness as well as the error and it not crashing.
+# CHECK: [[@LINE+2]]:1: error: starting new .cfi frame before finishing the previous one
 .cfi_startproc
-# CHECK: error: starting new .cfi frame before finishing the previous one
+
 nop
 .cfi_endproc
 
 .cfi_def_cfa rsp, 8
 # CHECK: error: this directive must appear between .cfi_startproc and .cfi_endproc directives