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

1705

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

1705

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

1705

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

1701–1715

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

[thopre]

llvm/lib/MC/MCParser/AsmParser.cpp

1701–1715

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

if (Lexer.is(AsmToken::EndOfStatement)) {
     Lex();
     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))
+  if (Lexer.is(AsmToken::HashDirective)) {
+    // Do not evaluate line markers during instantiation.
+    if (!isInsideMacroInstantiation())
       return parseCppHashLineFilenameComment(IDLoc);
+    // Eat the line marker.
+    Lex();
+    assert(getTok().is(AsmToken::Integer) &&
+           "Lexing Cpp line comment: Expected Integer");
+    Lex();
+    assert(getTok().is(AsmToken::String) &&
+           "Lexing Cpp line comment: Expected String");
+    Lex();
+    return false;
+  }
   // 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 = "";
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