Fix debug line info when line markers are present inside macros.

[leandrov]

Summary

Compiling assembly files when newlines are reduced to line markers within a .macro context will generate wrong information in .debug_line section.
This patch fixes this issue by evaluating line markers within the macro scope but not when they are used and evaluated.

[leandrov]

Moved the test case to AsmParser tests.

[thopre]

llvm/lib/MC/MCParser/AsmParser.cpp

1706

Is there code inside parseCppHashLineFilenameComment that could be hoisted out in a separate function that could be used here? If not, maybe you could create a function anyway which would make the code here smaller and more obvious.

Might be nice having in the heading comment for that function the format of a line marker: # linenum filename flags. I don't know if it's customary to refer to documentation for a format when it's an online page, but if it is you could mention https://gcc.gnu.org/onlinedocs/cpp/Preprocessor-Output.html in that heading comment.

[leandrov]

llvm/lib/MC/MCParser/AsmParser.cpp

1706

parseCppHashLineFilenameComment() also calls Lex() 3 times but saves the content in the process. In here, we just want to skip it. I felt it wasn't worth to hoist out in a separate function (that in this case would simply discard token contents) and it's unlikely for this to repeat elsewhere... I would say to keep it like this (maybe) but let us wait for other reviewers.

[compnerd]

llvm/lib/MC/MCParser/AsmParser.cpp

1706

I think that having a parameter to parseCppHashLineFilenameComment that indicates if the location should be preserved would be simple to add and avoid the duplication. We could default to to true and avoid altering the existing callers.

[leandrov]

Conditionally saving location information on parseCppHashLineFilenameComment.

[thopre]

llvm/lib/MC/MCParser/AsmParser.cpp

1702–1704

You shouldn't need to change this with the default value.

[thopre]

llvm/lib/MC/MCParser/AsmParser.cpp

1702–1704

Oh my bad, I misread what was happening. Please disregard.

[leandrov]

CI error is unrelated.

[leandrov]

Rebase.

[leandrov]

Shall we land this?

[probinson]

Without this patch, I'm seeing line 109 reported, which makes no sense at all. The test shows with the patch we get line 105, which in effect means that the line directive is processed as if it was not inside a macro definition; that makes some sense.
LGTM

llvm/lib/MC/MCParser/AsmParser.cpp

public:
   bool checkForValidSection() override;
 
   /// }
 
 private:
   bool parseStatement(ParseStatementInfo &Info,
                       MCAsmParserSemaCallback *SI);
   bool parseCurlyBlockScope(SmallVectorImpl<AsmRewrite>& AsmStrRewrites);
-  bool parseCppHashLineFilenameComment(SMLoc L);
+  bool parseCppHashLineFilenameComment(SMLoc L, bool SaveLocInfo = true);
 
   void checkForBadMacro(SMLoc DirectiveLoc, StringRef Name, StringRef Body,
                         ArrayRef<MCAsmMacroParameter> Parameters);
   bool expandMacro(raw_svector_ostream &OS, StringRef Body,
                    ArrayRef<MCAsmMacroParameter> Parameters,
                    ArrayRef<MCAsmMacroArgument> A, bool EnableAtPseudoVariable,
                    SMLoc L);
 
if (Lexer.is(AsmToken::EndOfStatement)) {
     return false;
   }
   // Statements always start with an identifier.
   AsmToken ID = getTok();
   SMLoc IDLoc = ID.getLoc();
   StringRef IDVal;
   int64_t LocalLabelVal = -1;
   if (Lexer.is(AsmToken::HashDirective))
-    return parseCppHashLineFilenameComment(IDLoc);
+    return parseCppHashLineFilenameComment(IDLoc,
+                                           !isInsideMacroInstantiation());
+
   // Allow an integer followed by a ':' as a directional local label.
   if (Lexer.is(AsmToken::Integer)) {
     LocalLabelVal = getTok().getIntVal();
     if (LocalLabelVal < 0) {
       if (!TheCondState.Ignore) {
         Lex(); // always eat a token
         return Error(IDLoc, "unexpected token at start of statement");
       }
       IDVal = "";
     } else {
AsmParser::parseCurlyBlockScope(SmallVectorImpl<AsmRewrite> &AsmStrRewrites) {
   // Erase the block start/end brace from the output asm string
   AsmStrRewrites.emplace_back(AOK_Skip, StartLoc, Lexer.getLoc().getPointer() -
                                                   StartLoc.getPointer());
   return true;
 }
 
 /// parseCppHashLineFilenameComment as this:
 ///   ::= # number "filename"
-bool AsmParser::parseCppHashLineFilenameComment(SMLoc L) {
+bool AsmParser::parseCppHashLineFilenameComment(SMLoc L, bool SaveLocInfo) {
   Lex(); // Eat the hash token.
   // Lexer only ever emits HashDirective if it fully formed if it's
   // done the checking already so this is an internal error.
   assert(getTok().is(AsmToken::Integer) &&
          "Lexing Cpp line comment: Expected Integer");
   int64_t LineNumber = getTok().getIntVal();
   Lex();
   assert(getTok().is(AsmToken::String) &&
          "Lexing Cpp line comment: Expected String");
   StringRef Filename = getTok().getString();
   Lex();
 
+  if (!SaveLocInfo)
+    return false;
+
   // Get rid of the enclosing quotes.
   Filename = Filename.substr(1, Filename.size() - 2);
 
   // Save the SMLoc, Filename and LineNumber for later use by diagnostics
   // and possibly DWARF file info.
   CppHashInfo.Loc = L;
   CppHashInfo.Filename = Filename;
   CppHashInfo.LineNumber = LineNumber;
while (true) {
     while (Lexer.is(AsmToken::Error)) {
       Lexer.Lex();
     }
 
     // Check whether we have reached the end of the file.
     if (getLexer().is(AsmToken::Eof))
       return Error(DirectiveLoc, "no matching '.endmacro' in definition");
 
-    // Otherwise, check whether we have reach the .endmacro.
+    // Otherwise, check whether we have reach the .endmacro or the start of a
+    // preprocessor line marker.
     if (getLexer().is(AsmToken::Identifier)) {
       if (getTok().getIdentifier() == ".endm" ||
           getTok().getIdentifier() == ".endmacro") {
         if (MacroDepth == 0) { // Outermost macro.
           EndToken = getTok();
           Lexer.Lex();
           if (getLexer().isNot(AsmToken::EndOfStatement))
             return TokError("unexpected token in '" + EndToken.getIdentifier() +
                             "' directive");
           break;
         } else {
           // Otherwise we just found the end of an inner macro.
           --MacroDepth;
         }
       } else if (getTok().getIdentifier() == ".macro") {
         // We allow nested macros. Those aren't instantiated until the outermost
         // macro is expanded so just ignore them for now.
         ++MacroDepth;
       }
+    } else if (Lexer.is(AsmToken::HashDirective)) {
+      (void)parseCppHashLineFilenameComment(getLexer().getLoc());
     }
 
     // Otherwise, scan til the end of the statement.
     eatToEndOfStatement();
   }
 
   if (getContext().lookupMacro(Name)) {
     return Error(DirectiveLoc, "macro '" + Name + "' is already defined");

llvm/test/MC/AsmParser/line-marker-inside-macro.s

+// RUN: llvm-mc -triple i386-unknown-unknown --filetype=obj -g %s -o %t
+// RUN: llvm-dwarfdump -a %t | FileCheck %s
+
+.macro FOO
+# 100 "./line-marker-inside-macro.s"
+.endm
+
+
+
+FOO
+  mov %eax, 0
+
+// CHECK:      0x0000000000000000 105 0 1 0 0 is_stmt
+// CHECK-NEXT: 0x0000000000000005 105 0 1 0 0 is_stmt end_sequence