Skip to content
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 16 additions & 1 deletion clang/lib/Tooling/DependencyScanning/ModuleDepCollector.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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,

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

@cyndyishida cyndyishida Feb 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

// 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;
Expand Down
30 changes: 30 additions & 0 deletions clang/test/ClangScanDeps/modules-debug-dir.c
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
// 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
// RUN: cat %t/result.json | FileCheck %s

//--- cdb.json.in
[{
"directory": "DIR",
"command": "clang -target x86_64-apple-darwin -c -g -gmodules DIR/tu.c -fmodules -fmodules-cache-path=DIR/cache -IDIR/include/ -fdebug-compilation-dir=DIR -o DIR/tu.o",
"file": "DIR/tu.c"
}]

//--- include/module.modulemap
module mod {
header "mod.h"
}

//--- include/mod.h

//--- tu.c
#include "mod.h"

// Check the -fdebug-compilation-dir used for the module is the root
// directory when current working directory optimization is in effect.
// CHECK: "modules": [
// CHECK: "command-line": [
// CHECK: "-fdebug-compilation-dir=/",
// CHECK: "translation-units": [
Loading