[clang] Use File Location for debug info resolution.

[SergejSalnikov]

To improve debuggability, the macro arguments should be resolved to their original location rather than macro expansion location.

Macro.debug.webm

fixes #160667

[github-actions]

Thank you for submitting a Pull Request (PR) to the LLVM Project!

This PR will be automatically labeled and the relevant teams will be notified.

If you wish to, you can add reviewers by using the "Reviewers" section on this page.

If this is not working for you, it is probably because you do not have write permissions for the repository. In which case you can instead tag reviewers by name in a comment by using @ followed by their GitHub username.

If you have received no comments on your PR for a week, you can request a review by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate is once a week. Please remember that you are asking for valuable time from other developers.

If you have further questions, they may be answered by the LLVM GitHub User Guide.

You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums.

[llvmbot]

@llvm/pr-subscribers-debuginfo
@llvm/pr-subscribers-clang

@llvm/pr-subscribers-clang-codegen

Author: SKill (SergejSalnikov)

Changes

To improve deniability, the macro arguments should be resolved to their original location rather than macro expansion location.

Macro.debug.webm

fixes #160667

---

Full diff: https://github.com/llvm/llvm-project/pull/163982.diff

1 Files Affected:

• (modified) clang/lib/CodeGen/CGDebugInfo.cpp (+9-7)

diff --git a/clang/lib/CodeGen/CGDebugInfo.cpp b/clang/lib/CodeGen/CGDebugInfo.cpp
index 12e2813ef2ec7..224a699784303 100644
--- a/clang/lib/CodeGen/CGDebugInfo.cpp
+++ b/clang/lib/CodeGen/CGDebugInfo.cpp
@@ -318,7 +318,7 @@ void CGDebugInfo::setLocation(SourceLocation Loc) {
   if (Loc.isInvalid())
     return;
 
-  CurLoc = CGM.getContext().getSourceManager().getExpansionLoc(Loc);
+  CurLoc = CGM.getContext().getSourceManager().getFileLoc(Loc);
 
   // If we've changed files in the middle of a lexical scope go ahead
   // and create a new lexical scope with file node if it's different
@@ -545,7 +545,7 @@ llvm::DIFile *CGDebugInfo::getOrCreateFile(SourceLocation Loc) {
     FileName = TheCU->getFile()->getFilename();
     CSInfo = TheCU->getFile()->getChecksum();
   } else {
-    PresumedLoc PLoc = SM.getPresumedLoc(Loc);
+    PresumedLoc PLoc = SM.getPresumedLoc(SM.getFileLoc(Loc));
     FileName = PLoc.getFilename();
 
     if (FileName.empty()) {
@@ -572,7 +572,8 @@ llvm::DIFile *CGDebugInfo::getOrCreateFile(SourceLocation Loc) {
     if (CSKind)
       CSInfo.emplace(*CSKind, Checksum);
   }
-  return createFile(FileName, CSInfo, getSource(SM, SM.getFileID(Loc)));
+  return createFile(FileName, CSInfo,
+                    getSource(SM, SM.getFileID(SM.getExpansionLoc(Loc))));
 }
 
 llvm::DIFile *CGDebugInfo::createFile(
@@ -627,7 +628,7 @@ unsigned CGDebugInfo::getLineNumber(SourceLocation Loc) {
   if (Loc.isInvalid())
     return 0;
   SourceManager &SM = CGM.getContext().getSourceManager();
-  return SM.getPresumedLoc(Loc).getLine();
+  return SM.getPresumedLoc(SM.getFileLoc(Loc)).getLine();
 }
 
 unsigned CGDebugInfo::getColumnNumber(SourceLocation Loc, bool Force) {
@@ -639,7 +640,7 @@ unsigned CGDebugInfo::getColumnNumber(SourceLocation Loc, bool Force) {
   if (Loc.isInvalid() && CurLoc.isInvalid())
     return 0;
   SourceManager &SM = CGM.getContext().getSourceManager();
-  PresumedLoc PLoc = SM.getPresumedLoc(Loc.isValid() ? Loc : CurLoc);
+  PresumedLoc PLoc = SM.getPresumedLoc(Loc.isValid() ? SM.getFileLoc(Loc) : CurLoc);
   return PLoc.isValid() ? PLoc.getColumn() : 0;
 }
 
@@ -4973,7 +4974,7 @@ void CGDebugInfo::EmitLocation(CGBuilderTy &Builder, SourceLocation Loc) {
   // Update our current location
   setLocation(Loc);
 
-  if (CurLoc.isInvalid() || CurLoc.isMacroID() || LexicalBlockStack.empty())
+  if (CurLoc.isInvalid() || LexicalBlockStack.empty())
     return;
 
   llvm::MDNode *Scope = LexicalBlockStack.back();
@@ -6244,7 +6245,8 @@ void CGDebugInfo::EmitGlobalAlias(const llvm::GlobalValue *GV,
 void CGDebugInfo::AddStringLiteralDebugInfo(llvm::GlobalVariable *GV,
                                             const StringLiteral *S) {
   SourceLocation Loc = S->getStrTokenLoc(0);
-  PresumedLoc PLoc = CGM.getContext().getSourceManager().getPresumedLoc(Loc);
+  SourceManager &SM = CGM.getContext().getSourceManager();
+  PresumedLoc PLoc = SM.getPresumedLoc(SM.getFileLoc(Loc));
   if (!PLoc.isValid())
     return;
 

[github-actions]

✅ With the latest revision this PR passed the C/C++ code formatter.

[dwblaikie]

Seems worth a go - @adrian-prantl @JDevlieghere @jmorse you folks on board with this, or want some more time to experiment (maybe have it behind a flag, etc)?

[OCHyams]

To improve debuggability

Possibly a silly question but can you please explain why this improves debuggability?

[SergejSalnikov]

Because you can step through and put breakpoints in macro arguments.
The huge benefit is when you have lambdas as part of macro argument.

Without that change the entire macro is expanded on a single line.

[SergejSalnikov]

I'm requesting the performance test run on this PR.

[OCHyams]

Sorry I hadn't followed the bug thread, I think I see now. It seems fairly reasonable to associate the macro arguments with their source locations rather than the overall macro expansion point. However, I have a few high level questions/concerns:

• I think this might be a departure from GCC's behaviour (@dblaikie in the issue thread you mentioned the Clang behaviour might be a debug info bug but this suggests maybe it's intentional?).
• The .debug_line size will inevitably increase. Does this noticeably impact file size of say, a clang self host build?

I don't think either of those are necessarily blockers.

maybe have it behind a flag

I like the safety of a flag / chicken-bit (are there situations where this makes macro-heavy code a pain to step through?), but can see the argument for avoiding adding more flags...

Tentative SGTM from over here

[SergejSalnikov]

I can tackle the

The .debug_line size will inevitably increase. Does this noticeably impact file size of say, a clang self host build?

When compiling in without full debug info the size difference of binary is under 0.01%
When using -g option, the binary size is impacted by less than 0.5% while the debug symbols size is bigger by 8% for macro heavy code-bases.

But usually the reason to compile the binary with -g option is to have move debug info, not less. Why do we treat macro any different from other code.
If the goal is to save on symbols table size, we can fold all source locations to the start of an enclosing scope for each symbol.

[OCHyams]

But usually the reason to compile the binary with -g option is to have move debug info, not less. Why do we treat macro any different from other code.

There are plenty of flags to use alongside -g that limit debug info emission in some way, e.g. -gno-column-info.

If the goal is to save on symbols table size, we can fold all source locations to the start of an enclosing scope for each symbol.

The goal is to understand and discuss the trade-offs, which is a common part of the review process.

My SGTM still stands, and David already approved this, but IMO it would be good to wait a little longer to see if one of the Apple/LLDB folks have any additional opinions on this.

[Michael137]

Seems simple enough, LGTM too.

I tested the issue that we saw internally with __builtin_verbose_trap inside an Apple block, and LLDB now stops at a more sensible location.

No opinion on size impact, but I guess we can put the change behind a flag retroactively.

[Michael137]

@SergejSalnikov do you need someone to merge this for you?

[SergejSalnikov]

I was told to hold off with merging to let all reviewers to take a look. Feel free to merge it in if you think it's time.

[github-actions]

@SergejSalnikov Congratulations on having your first Pull Request (PR) merged into the LLVM Project!

Your changes will be combined with recent changes from other authors, then tested by our build bots. If there is a problem with a build, you may receive a report in an email or a comment on this PR.

Please check whether problems have been caused by your change specifically, as the builds can include changes from many authors. It is not uncommon for your change to be included in a build that fails due to someone else's changes, or infrastructure issues.

How to do this, and the rest of the post-merge process, is covered in detail here.

If your change does cause a problem, it may be reverted, or you can revert it yourself. This is a normal part of LLVM development. You can fix your changes and open a new PR to merge them again.

If you don't get any reports, no action is required from you. Your changes are working as expected, well done!

[dwblaikie]

When using -g option, the binary size is impacted by less than 0.5% while the debug symbols size is bigger by 8% for macro heavy code-bases.

8% "debug symbols size" (what do you mean by that/how did you measure it?) is pretty severe perhaps a bloaty ( https://github.com/google/bloaty ) comparison for this macro heavy code you mentioned?

[SergejSalnikov]

I was comparing the size of .dwp files before and after the change.