[clang modules] Setting `DebugCompilationDir` when it is safe to ignore current working directory #128446

qiongsiwu · 2025-02-24T00:55:31Z

This PR explicitly sets DebugCompilationDir to the system's root directory if it is safe to ignore the current working directory.

This fixes a problem where a PCM file's embedded debug information can lead to compilation failure. The compiler may have decided it is indeed safe to ignore the current working directory. In this case, the PCM file's content is functionally correct regardless of the current working directory because no inputs use relative paths (see #124786). However, a PCM may contain debug info. If debug info is requested, the compiler uses the current working directory value to set DW_AT_comp_dir. This may lead to the following situation:

Two different compilations need the same PCM file.
The PCM file is compiled assuming a working directory, which is embedded in the debug info, but otherwise has no effect.
The second compilation assumes a different working directory, and expects an identically-sized pcm file. However, it cannot find such a PCM, because the existing PCM file has been compiled assuming a different DW_AT_comp_dir , which is embedded in the debug info.

This PR resets the DebugCompilationDir if it is functionally safe to ignore the working directory so the above situation is avoided, since all debug information will share the same working directory.

rdar://145249881

llvmbot · 2025-02-24T00:56:07Z

@llvm/pr-subscribers-clang

Author: Qiongsi Wu (qiongsiwu)

Changes

This PR explicitly sets DebugCompilationDir to the system's root directory if it is safe to ignore the current working directory.

This fixes a problem where a PCM file's embedded debug information can lead to compilation failure. The compiler may have decided it is indeed safe to ignore the current working directory. In this case, the PCM file's content is functionally correct regardless of the current working directory because no inputs use relative paths (see #124786). However, a PCM may contain debug info. If debug info is requested, the compiler uses the current working directory value to set DW_AT_comp_dir. This may lead to the following situation:

Two different compilations need the same PCM file.
The PCM file is compiled assuming a working directory, which is embedded in the debug info, but otherwise has no effect.
The second compilation assumes a different working directory, hence expects an identically-sized pcm file. However, it cannot find such a PCM, because the existing PCM file has been compiled assuming a different DW_AT_comp_dir , which is embedded in the debug info.

This PR resets the DebugCompilationDir if it is functionally safe to ignore the working directory so the above situation is avoided, since all debug information will share the same working directory.

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

2 Files Affected:

(modified) clang/lib/Tooling/DependencyScanning/ModuleDepCollector.cpp (+16-1)
(added) clang/test/ClangScanDeps/modules-debug-dir.c (+22)

diff --git a/clang/lib/Tooling/DependencyScanning/ModuleDepCollector.cpp b/clang/lib/Tooling/DependencyScanning/ModuleDepCollector.cpp
index 1c5f4c4b50ab6..8a94535d3806c 100644
--- a/clang/lib/Tooling/DependencyScanning/ModuleDepCollector.cpp
+++ b/clang/lib/Tooling/DependencyScanning/ModuleDepCollector.cpp
@@ -492,8 +492,23 @@ static std::string getModuleContextHash(const ModuleDeps &MD,
   auto &FSOpts = const_cast<FileSystemOptions &>(CI.getFileSystemOpts());
   if (CWD && !IgnoreCWD)
     HashBuilder.add(*CWD);
-  else
+  else {
     FSOpts.WorkingDir.clear();
+    auto &CGOpts = const_cast<CodeGenOptions &>(CI.getCodeGenOpts());
+    if (CGOpts.DwarfVersion && CWD) {
+      // It is necessary to explicitly set the DebugCompilationDir
+      // to a common directory (e.g. root) if IgnoreCWD is true.
+      // When IgnoreCWD is true, the module's content should not depend
+      // on the current working directory. However, if dwarf information
+      // is needed (when CGOpts.DwarfVersion is non-zero), and if
+      // CGOpts.DebugCompilationDir is not explicitly set,
+      // the current working directory will be automatically embedded
+      // in the dwarf information in the pcm, contradicting the assumption
+      // that it is safe to ignore the CWD. Thus in such cases,
+      // CGOpts.DebugCompilationDir is explicitly set to a common directory.
+      CGOpts.DebugCompilationDir = llvm::sys::path::root_path(*CWD);
+    }
+  }
 
   // Hash the BuildInvocation without any input files.
   SmallString<0> ArgVec;
diff --git a/clang/test/ClangScanDeps/modules-debug-dir.c b/clang/test/ClangScanDeps/modules-debug-dir.c
new file mode 100644
index 0000000000000..580f72205e68f
--- /dev/null
+++ b/clang/test/ClangScanDeps/modules-debug-dir.c
@@ -0,0 +1,22 @@
+// RUN: rm -rf %t
+// RUN: split-file %s %t
+// RUN: sed -e "s|DIR|%/t|g" %t/cdb.json.in > %t/cdb.json
+// RUN: clang-scan-deps -compilation-database %t/cdb.json -format \
+// RUN:   experimental-full > %t/result.json
+
+//--- cdb.json.in
+[{
+  "directory": "DIR",
+  "command": "clang -g -fdebug-info-for-profiling DIR/tu.c -fmodules -fmodules-cache-path=DIR/cache -IDIR/include/ -o DIR/tu.o",
+  "file": "DIR/tu.c"
+}]
+
+//--- include/module.modulemap
+module mod {
+  header "mod.h"
+}
+
+//--- include/mod.h
+
+//--- tu.c
+#include "mod.h"

qiongsiwu · 2025-02-24T01:00:52Z

Note to reviewers:
This fixes rdar://145249881, but I'd like to get some input on three things.

I am not sure if it is a good idea to set DebugCompilationDir to root. I chose it because I think it is safe and all systems should have the root directory. I am all ears for a different choice.
I have not figured out a good way to test this. For some reason, I cannot get the module compilation command to produce debug information in the test (through -g, or -dwarf-version options). I don't think we always want to hardcode DebugCompilationDir to root, so I think we want to set it only if dwarf info is needed. May I get some pointers to get the compiler to generate debug info when compiling the pcm? The test is fixed. I need -gmodules.
This fix feels very patchy to me. I worry that something else may somehow embed the CWD in the binary and we may have to play the cat and mouse game again. I think it may be fine for now, but I'd not want to leave it like this for too long and I'd love to get some suggestions.

Thanks!

cachemeifyoucan · 2025-02-24T17:45:03Z

@adrian-prantl for the debug directory option since that might interact how debugger working with the option.

I think we need a solution approved by debugger forks for how to avoid stamping CWD into debug info and when it is safe to do so (for example, we guarantee all the paths in the TU/module are absolute path).

qiongsiwu · 2025-02-24T21:20:55Z

Or maybe it makes more sense to set the working directory to SYSROOT, or to the input header's path, or the directory that contains the output pcm?

Update: setting DebugCompilationDir to the pcms' output directory works. But as @cachemeifyoucan pointed out earlier this may not be the right thing to do.

clang/lib/Tooling/DependencyScanning/ModuleDepCollector.cpp

jansvoboda11

Leaving the correctness of the change up for other reviewers. My only gripe is the const_cast.

kastiglione

It makes sense on the surface. I'll pull this change and run lldb tests.

kastiglione · 2025-02-25T17:54:43Z

clang/lib/Tooling/DependencyScanning/ModuleDepCollector.cpp

+      // on the current working directory. However, if dwarf information
+      // is needed (when CGOpts.DwarfVersion is non-zero), and if
+      // CGOpts.DebugCompilationDir is not explicitly set,
+      // the current working directory will be automatically embedded
+      // in the dwarf information in the pcm, contradicting the assumption
+      // that it is safe to ignore the CWD. Thus in such cases,


When reading this comment, I was tripped up on "if CGOpts.DebugCompilationDir is not explicitly set", thinking that this should mean a if (!CGOpts.DebugCompilationDir) somewhere in this function.

But I understand now. What do you think of this wording?

Suggested change

// on the current working directory. However, if dwarf information

// is needed (when CGOpts.DwarfVersion is non-zero), and if

// CGOpts.DebugCompilationDir is not explicitly set,

// the current working directory will be automatically embedded

// in the dwarf information in the pcm, contradicting the assumption

// that it is safe to ignore the CWD. Thus in such cases,

// on the current working directory. However, if dwarf information

// is needed (when CGOpts.DwarfVersion is non-zero), then

// CGOpts.DebugCompilationDir must be populated, because otherwise

// the current working directory will be automatically embedded

// in the dwarf information in the pcm, contradicting the assumption

// that it is safe to ignore the CWD. Thus in such cases,

With this updated wording (before I didn't understand the intention) the patch looks safe to me.

How does lldb use the CWD encoded in the PCM? ~~Would it better/possible to avoid setting CGOpts.DebugCompilationDir when IgnoreCWD == true ?~~

I'm not as concerned now. The important factors seem to be that

the CWD in explicitly built PCM's shouldn't need to be directly referenced in the debugger.

Even if CGOpts.DebugCompilationDir was empty here, it would still be written later which may incur more side effects.

Thanks for the explanation folks.

But I understand now. What do you think of this wording?

Thanks @kastiglione ! The comment modification looks great. It is in the PR now.

clang/lib/Tooling/DependencyScanning/ModuleDepCollector.cpp

This reverts commit a9274ea.

qiongsiwu · 2025-02-26T17:02:06Z

Ping for review. Thanks so much!

cyndyishida

LGTM, @jansvoboda11 should also sign off.

jansvoboda11

LGTM, thanks!

…re current working directory (llvm#128446) This PR explicitly sets `DebugCompilationDir` to the system's root directory if it is safe to ignore the current working directory. This fixes a problem where a PCM file's embedded debug information can lead to compilation failure. The compiler may have decided it is indeed safe to ignore the current working directory. In this case, the PCM file's content is functionally correct regardless of the current working directory because no inputs use relative paths (see llvm#124786). However, a PCM may contain debug info. If debug info is requested, the compiler uses the current working directory value to set `DW_AT_comp_dir`. This may lead to the following situation: 1. Two different compilations need the same PCM file. 2. The PCM file is compiled assuming a working directory, which is embedded in the debug info, but otherwise has no effect. 3. The second compilation assumes a different working directory, and expects an identically-sized pcm file. However, it cannot find such a PCM, because the existing PCM file has been compiled assuming a different `DW_AT_comp_dir `, which is embedded in the debug info. This PR resets the `DebugCompilationDir` if it is functionally safe to ignore the working directory so the above situation is avoided, since all debug information will share the same working directory. rdar://145249881 (cherry picked from commit 7f482aa) Conflicts: clang/lib/Tooling/DependencyScanning/ModuleDepCollector.cpp

[clang modules] Setting `DebugCompilationDir` when it is safe to ignore current working directory (llvm#128446)

Initial commit

c8eda8b

qiongsiwu marked this pull request as draft February 24, 2025 00:55

llvmbot added the clang Clang issues not falling into any other category label Feb 24, 2025

Fix the test.

4b29c19

qiongsiwu requested review from Bigcheese, cyndyishida, jansvoboda11 and cachemeifyoucan February 24, 2025 17:37

qiongsiwu self-assigned this Feb 24, 2025

qiongsiwu marked this pull request as ready for review February 24, 2025 17:37

cachemeifyoucan requested a review from adrian-prantl February 24, 2025 17:41

Try to fix the test on windows.

5c53c84

cyndyishida requested a review from kastiglione February 24, 2025 19:22

jansvoboda11 reviewed Feb 25, 2025

View reviewed changes

clang/lib/Tooling/DependencyScanning/ModuleDepCollector.cpp Outdated Show resolved Hide resolved

jansvoboda11 reviewed Feb 25, 2025

View reviewed changes

clang/lib/Tooling/DependencyScanning/ModuleDepCollector.cpp Outdated Show resolved Hide resolved

jansvoboda11 requested changes Feb 25, 2025

View reviewed changes

kastiglione reviewed Feb 25, 2025

View reviewed changes

Address code review and try fixing the test on windows.

7021180

qiongsiwu requested a review from jansvoboda11 February 25, 2025 20:25

jansvoboda11 reviewed Feb 25, 2025

View reviewed changes

clang/lib/Tooling/DependencyScanning/ModuleDepCollector.cpp Outdated Show resolved Hide resolved

jansvoboda11 reviewed Feb 25, 2025

View reviewed changes

clang/lib/Tooling/DependencyScanning/ModuleDepCollector.cpp Outdated Show resolved Hide resolved

Address code review and try fixing the test on windows.

75b4ff6

qiongsiwu requested a review from jansvoboda11 February 25, 2025 23:57

qiongsiwu added 2 commits February 25, 2025 19:45

Try one more time to fix the test on windows.

a9274ea

Revert "Try one more time to fix the test on windows."

6b4435c

This reverts commit a9274ea.

cyndyishida approved these changes Feb 26, 2025

View reviewed changes

jansvoboda11 approved these changes Feb 26, 2025

View reviewed changes

qiongsiwu merged commit 7f482aa into llvm:main Feb 26, 2025
11 checks passed

qiongsiwu deleted the fix_145249881 branch February 26, 2025 21:21

qiongsiwu added a commit to swiftlang/llvm-project that referenced this pull request Feb 27, 2025

Merge pull request #10124 from qiongsiwu/cherry_pick_fix_145249881

01f9dd6

[clang modules] Setting `DebugCompilationDir` when it is safe to ignore current working directory (llvm#128446)

huaatian mentioned this pull request Feb 28, 2025

fix live interval empty issue huaatian/llvm-project#1

Open

[clang modules] Setting DebugCompilationDir when it is safe to ignore current working directory #128446

[clang modules] Setting DebugCompilationDir when it is safe to ignore current working directory #128446

Uh oh!

Conversation

qiongsiwu commented Feb 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

llvmbot commented Feb 24, 2025

Uh oh!

qiongsiwu commented Feb 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cachemeifyoucan commented Feb 24, 2025

Uh oh!

qiongsiwu commented Feb 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jansvoboda11 left a comment

Choose a reason for hiding this comment

Uh oh!

kastiglione left a comment

Choose a reason for hiding this comment

Uh oh!

kastiglione Feb 25, 2025

Choose a reason for hiding this comment

Uh oh!

adrian-prantl Feb 25, 2025

Choose a reason for hiding this comment

Uh oh!

cyndyishida Feb 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

qiongsiwu Feb 25, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

qiongsiwu commented Feb 26, 2025

Uh oh!

cyndyishida left a comment

Choose a reason for hiding this comment

Uh oh!

jansvoboda11 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

[clang modules] Setting `DebugCompilationDir` when it is safe to ignore current working directory #128446

[clang modules] Setting `DebugCompilationDir` when it is safe to ignore current working directory #128446

qiongsiwu commented Feb 24, 2025 •

edited

Loading

qiongsiwu commented Feb 24, 2025 •

edited

Loading

qiongsiwu commented Feb 24, 2025 •

edited

Loading

cyndyishida Feb 25, 2025 •

edited

Loading