[AsmParser] Add source location to all errors related to .cfi directives

[arichardson]

Summary

I was trying to add .cfi_ annotations to assembly code in the FreeBSD
kernel and changed a macro that then resulted in incorrectly nested
directives. However, clang's diagnostics said the error was happening at
<unknown>:0. This addresses one of the TODOs added in D51695.

[arichardson]

rebase

[MaskRay]

llvm/lib/CodeGen/AsmPrinter/AsmPrinterDwarf.cpp

245 ↗

(On Diff #301927)

These can be simplified by switching to default arguments in the declarations.

[arichardson]

llvm/lib/CodeGen/AsmPrinter/AsmPrinterDwarf.cpp

245 ↗

(On Diff #301927)

Yes. However, I chose to not add default arguments to ensure that all callers from the asmparser side are forced to pass the argument. I initially had default arguments but missed a lot of call sites before I removed the default.

[arichardson]

llvm/lib/CodeGen/AsmPrinter/AsmPrinterDwarf.cpp

245 ↗

(On Diff #301927)

To simplify the cases where there is no Loc, we could do something like void emitCFIEndProc(MCSTREAMER_SMLOC(Loc));

The files that don't can't to pass a SMLoc could then do
#define MCSTREAMER_SMLOC(name) SMLoc name = SMLoc()
but I'm not sure this is worth the effort.

[rnk]

I've been thinking about this problem of missing SMLocs for a while, and I think the right direction is probably for MCStreamer to have a way to get the location of the beginning of the directive being processed. That would suffice for the majority of these .cfi_* directives. It would make it much easier to get error reporting right by default or to add in error reporting later.

[MaskRay]

In D89787#2365791, @rnk wrote:

I've been thinking about this problem of missing SMLocs for a while, and I think the right direction is probably for MCStreamer to have a way to get the location of the beginning of the directive being processed. That would suffice for the majority of these .cfi_* directives. It would make it much easier to get error reporting right by default or to add in error reporting later.

How should MCStreamer get an access to the parser's SMLoc? The constructor of AsmParser has a non-const reference to MCStreamer. Add a SMLoc pointer in MCStreamer and make it aware of the parser's SMLoc in the constructor of AsmParser?

[rnk]

In D89787#2365823, @MaskRay wrote:

In D89787#2365791, @rnk wrote:

I've been thinking about this problem of missing SMLocs for a while, and I think the right direction is probably for MCStreamer to have a way to get the location of the beginning of the directive being processed. That would suffice for the majority of these .cfi_* directives. It would make it much easier to get error reporting right by default or to add in error reporting later.

How should MCStreamer get an access to the parser's SMLoc? The constructor of AsmParser has a non-const reference to MCStreamer. Add a SMLoc pointer in MCStreamer and make it aware of the parser's SMLoc in the constructor of AsmParser?

Sounds reasonable to me. Note, it's not the current parser location, it's the saved location of the start of the directive, but otherwise, that's the idea.

[MaskRay]

In D89787#2365826, @rnk wrote:

In D89787#2365823, @MaskRay wrote:

In D89787#2365791, @rnk wrote:

I've been thinking about this problem of missing SMLocs for a while, and I think the right direction is probably for MCStreamer to have a way to get the location of the beginning of the directive being processed. That would suffice for the majority of these .cfi_* directives. It would make it much easier to get error reporting right by default or to add in error reporting later.

How should MCStreamer get an access to the parser's SMLoc? The constructor of AsmParser has a non-const reference to MCStreamer. Add a SMLoc pointer in MCStreamer and make it aware of the parser's SMLoc in the constructor of AsmParser?

Sounds reasonable to me. Note, it's not the current parser location, it's the saved location of the start of the directive, but otherwise, that's the idea.

OK, D90511 for the idea.

(I confess I committed a symbol binding related SMLoc change abruptly yesterday.. Sorry)

[arichardson]

rebase after D90511

[MaskRay]

llvm/lib/MC/MCStreamer.cpp

970

Perhaps EndLoc is clearer.

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

15–16

[[@LINE+x]] is a deprecated form. Use [[#@LINE-1]]

[arichardson]

address review feedback

[MaskRay]

LGTM. One last comment

llvm/test/MC/X86/cfi-scope-unclosed.s

0

If cfi-scope-errors.s includes the coverage perhaps delete the file

[arichardson]

• Merge two test that don't need to be separate files

llvm/include/llvm/MC/MCStreamer.h

public:
   /// If this file is backed by a assembly streamer, this dumps the
   /// specified string in the output .s file.  This capability is indicated by
   /// the hasRawTextSupport() predicate.  By default this aborts.
   void emitRawText(const Twine &String);
 
   /// Streamer specific finalization.
   virtual void finishImpl();
   /// Finish emission of machine code.
-  void Finish();
+  void Finish(SMLoc EndLoc = SMLoc());
 
   virtual bool mayHaveInstructions(MCSection &Sec) const { return true; }
 };
 
 /// Create a dummy machine code streamer, which does nothing. This is useful for
 /// timing the assembler front end.
 MCStreamer *createNullStreamer(MCContext &Ctx);
 

llvm/lib/MC/MCParser/AsmParser.cpp

for (std::tuple<SMLoc, CppHashInfoTy, MCSymbol *> &LocSym : DirLabels) {
         printError(std::get<0>(LocSym), "directional label undefined");
       }
     }
   }
 
   // Finalize the output stream if there are no errors and if the client wants
   // us to.
   if (!HadError && !NoFinalize)
-    Out.Finish();
+    Out.Finish(Lexer.getLoc());
 
   return HadError || getContext().hadError();
 }
 
 bool AsmParser::checkForValidSection() {
   if (!ParsingMSInlineAsm && !getStreamer().getCurrentSectionOnly()) {
     Out.InitSections(false);
     return Error(getTok().getLoc(),

llvm/lib/MC/MCParser/MasmParser.cpp

for (std::tuple<SMLoc, CppHashInfoTy, MCSymbol *> &LocSym : DirLabels) {
         printError(std::get<0>(LocSym), "directional label undefined");
       }
     }
   }
 
   // Finalize the output stream if there are no errors and if the client wants
   // us to.
   if (!HadError && !NoFinalize)
-    Out.Finish();
+    Out.Finish(Lexer.getLoc());
 
   return HadError || getContext().hadError();
 }
 
 bool MasmParser::checkForValidSection() {
   if (!ParsingMSInlineAsm && !getStreamer().getCurrentSectionOnly()) {
     Out.InitSections(false);
     return Error(getTok().getLoc(),

llvm/lib/MC/MCStreamer.cpp

 }
 
 bool MCStreamer::hasUnfinishedDwarfFrameInfo() {
   return !DwarfFrameInfos.empty() && !DwarfFrameInfos.back().End;
 }
 
 MCDwarfFrameInfo *MCStreamer::getCurrentDwarfFrameInfo() {
   if (!hasUnfinishedDwarfFrameInfo()) {
-    getContext().reportError(SMLoc(), "this directive must appear between "
-                                      ".cfi_startproc and .cfi_endproc "
-                                      "directives");
+    getContext().reportError(getStartTokLoc(),
+                             "this directive must appear between "
+                             ".cfi_startproc and .cfi_endproc directives");
     return nullptr;
   }
   return &DwarfFrameInfos.back();
 }
 
 bool MCStreamer::EmitCVFileDirective(unsigned FileNo, StringRef Filename,
                                      ArrayRef<uint8_t> Checksum,
                                      unsigned ChecksumKind) {

 void MCStreamer::emitRawText(const Twine &T) {
   SmallString<128> Str;
   emitRawTextImpl(T.toStringRef(Str));
 }
 
 void MCStreamer::EmitWindowsUnwindTables() {
 }
 
-void MCStreamer::Finish() {
+void MCStreamer::Finish(SMLoc EndLoc) {
   if ((!DwarfFrameInfos.empty() && !DwarfFrameInfos.back().End) ||
       (!WinFrameInfos.empty() && !WinFrameInfos.back()->End)) {
-    getContext().reportError(SMLoc(), "Unfinished frame!");
+    getContext().reportError(EndLoc, "Unfinished frame!");
     return;
   }
 
   MCTargetStreamer *TS = getTargetStreamer();
   if (TS)
     TS->finish();
 
   finishImpl();

llvm/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
- 
+# CHECK: [[#@LINE]]:1: error: starting new .cfi frame before finishing the previous one
  .cfi_endproc
 
-# CHECK: error: starting new .cfi frame before finishing the previous one

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

 # 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
+# CHECK: [[#@LINE-1]]:1: 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
+## 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
 
 nop
 .cfi_endproc
 
 .cfi_def_cfa rsp, 8
-# CHECK: error: this directive must appear between .cfi_startproc and .cfi_endproc directives
+# CHECK: [[#@LINE-1]]:1: error: this directive must appear between .cfi_startproc and .cfi_endproc directives
+
+## Check we don't crash on unclosed frame scope.
+.globl foo
+foo:
+ .cfi_startproc
+# CHECK: [[#@LINE+1]]:1: error: Unfinished frame!

llvm/test/MC/X86/cfi-scope-unclosed.s

-# RUN: not llvm-mc %s -filetype=obj -triple=x86_64-unknown-linux \
-# RUN:   -o /dev/null 2>&1 | FileCheck %s
-
-## Check we don't crash on unclosed frame scope.
-# CHECK: error: Unfinished frame!
-
-.text
-.globl foo
-foo:
- .cfi_startproc