[PATCH] D51695: [MC] [AsmParser]: Ensure a new CFI frame is not opened within an existing one when using integrated-as.

[Kristina Brooks via Phabricator via llvm-commits]

kristina created this revision.
kristina added reviewers: echristo, grosbach.
Herald added a subscriber: llvm-commits.

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.

  <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.


Repository:
  rL LLVM

https://reviews.llvm.org/D51695

Files:
  lib/MC/MCParser/AsmParser.cpp


Index: lib/MC/MCParser/AsmParser.cpp
===================================================================
--- lib/MC/MCParser/AsmParser.cpp
+++ lib/MC/MCParser/AsmParser.cpp
@@ -3888,12 +3888,19 @@
 /// ::= .cfi_startproc [simple]
 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");
   }
+  
+  // TODO(kristina): Deal with PP macro expansion so the error line
+  // matches the source chunk within the diagnostic when in a PP macro.
+  if (getStreamer().hasUnfinishedDwarfFrameInfo())
+    return Error(Lexer.getLoc(),
+                 ".cfi_startproc cannot be used in an existing frame");
 
   getStreamer().EmitCFIStartProc(!Simple.empty());
   return false;


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D51695.164063.patch
Type: text/x-patch
Size: 951 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20180905/1c0126b8/attachment.bin>