[PATCH] D51695: [MC] [AsmParser]: Ensure a new CFI frame is not opened within an existing one when using cc1as (causing a crash due to failed assert).

[Kristina Brooks via Phabricator via llvm-commits]

kristina added a comment.

In https://reviews.llvm.org/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();
        }
      }
    }
  
    DwarfFrameInfos.push_back(Frame);
  }


Repository:
  rL LLVM

https://reviews.llvm.org/D51695