This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by leandrov on May 21 2020, 8:42 AM.

Details

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.

Diff Detail

Event Timeline

leandrov created this revision.May 21 2020, 8:42 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 21 2020, 8:42 AM
Herald added a subscriber: hiraditya. · View Herald Transcript
leandrov updated this revision to Diff 265513.May 21 2020, 8:48 AM

Moved the test case to AsmParser tests.

thopre added a subscriber: thopre.May 21 2020, 8:55 AM
thopre added inline comments.
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.

davidb added a subscriber: davidb.May 21 2020, 9:19 AM
Harbormaster completed remote builds in B57529: Diff 265512.
leandrov marked an inline comment as done.May 22 2020, 2:17 AM
leandrov added inline comments.
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 added inline comments.Jun 3 2020, 9:34 AM
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 updated this revision to Diff 268396.Jun 4 2020, 2:34 AM

Conditionally saving location information on parseCppHashLineFilenameComment.

thopre added inline comments.Jun 4 2020, 3:00 AM
llvm/lib/MC/MCParser/AsmParser.cpp
1701–1715

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

thopre added inline comments.Jun 4 2020, 3:21 AM
llvm/lib/MC/MCParser/AsmParser.cpp
1701–1715

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

leandrov marked 2 inline comments as done.Jun 4 2020, 3:23 AM
leandrov marked 2 inline comments as done.Jun 4 2020, 5:31 AM

CI error is unrelated.

Shall we land this?

probinson accepted this revision.Jun 15 2020, 12:26 PM

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

This revision is now accepted and ready to land.Jun 15 2020, 12:26 PM
This revision was automatically updated to reflect the committed changes.