-
Notifications
You must be signed in to change notification settings - Fork 15.2k
[BOLT][DWARF] Get DWO file via relative path if the CompilationDir does not exist #154515
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[BOLT][DWARF] Get DWO file via relative path if the CompilationDir does not exist #154515
Conversation
@llvm/pr-subscribers-bolt Author: Jinjie Huang (Jinjie-Huang) ChangesFull diff: https://github.com/llvm/llvm-project/pull/154515.diff 2 Files Affected:
diff --git a/bolt/lib/Core/BinaryContext.cpp b/bolt/lib/Core/BinaryContext.cpp
index 84f1853469709..6a56c8aa7cfa5 100644
--- a/bolt/lib/Core/BinaryContext.cpp
+++ b/bolt/lib/Core/BinaryContext.cpp
@@ -33,6 +33,7 @@
#include "llvm/MC/MCSymbol.h"
#include "llvm/Support/CommandLine.h"
#include "llvm/Support/Error.h"
+#include "llvm/Support/FileSystem.h"
#include "llvm/Support/Regex.h"
#include <algorithm>
#include <functional>
@@ -1640,6 +1641,11 @@ void BinaryContext::preprocessDWODebugInfo() {
if (!opts::CompDirOverride.empty()) {
sys::path::append(AbsolutePath, opts::CompDirOverride);
sys::path::append(AbsolutePath, DWOName);
+ } else {
+ std::string CompDir = DwarfUnit->getCompilationDir();
+ sys::path::append(AbsolutePath, CompDir);
+ if (!sys::fs::exists(AbsolutePath) && sys::fs::exists(DWOName))
+ AbsolutePath = DWOName;
}
DWARFUnit *DWOCU =
DwarfUnit->getNonSkeletonUnitDIE(false, AbsolutePath).getDwarfUnit();
diff --git a/bolt/lib/Rewrite/DWARFRewriter.cpp b/bolt/lib/Rewrite/DWARFRewriter.cpp
index 0c1a1bac6c72e..80b24a5561e81 100644
--- a/bolt/lib/Rewrite/DWARFRewriter.cpp
+++ b/bolt/lib/Rewrite/DWARFRewriter.cpp
@@ -1851,6 +1851,8 @@ void DWARFRewriter::writeDWOFiles(
CompDir = opts::DwarfOutputPath.c_str();
else if (!opts::CompDirOverride.empty())
CompDir = opts::CompDirOverride;
+ else if (!sys::fs::exists(CompDir))
+ CompDir = ".";
SmallString<16> AbsolutePath;
sys::path::append(AbsolutePath, CompDir);
|
5d845ac
to
d4a2319
Compare
d4a2319
to
e08166c
Compare
|
Thank you for your reply. Yes, "-fdebug-compilation-dir=." for compiler or "CompDirOverride=." for llvm-bolt can perfectly resolve the issue here. But it seems that having the ability to get dwo from a relative path is also not bad? This consideration is that some other users might encounter this problem when launching distributed builds on local paths, and they may be faced with different build systems, or not familiar enough with BOLT-opts. And addressing this issue might also require some understanding of DWARF? which could require a bit of learning curve. BTW, the behaviors of GDB and llvm-dwp also involve searching using both absolute and relative paths—probably based on similar considerations? |
bolt/lib/Core/BinaryContext.cpp
Outdated
} | ||
} else if (!sys::fs::exists(DwarfUnit->getCompilationDir()) && | ||
sys::fs::exists(DWOName)) | ||
AbsolutePath = DWOName; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shouldn't this be appended to "."?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Umm, DWOName might be an absolute path... But in this case, using CompilationDir + DWOName would also be incorrect...
I guess if there is precedence... |
e08166c
to
8519343
Compare
✅ With the latest revision this PR passed the C/C++ code formatter. |
ba3785e
to
26c696e
Compare
26c696e
to
2d35fcb
Compare
@ayermolo Currently it's changed to use make_absolute() joining DWOCompDir and DWOName. This is helpful when DWOName is already an absolute path (it will prevent the concatenation). Otherwise, if a .dwp file is specified, directly concatenating DWOCompDir and DWOName could trigger an asserting crash when emitting dwos. PTAL, thanks. |
bolt/test/dwo-name-retrieving.test
Outdated
@@ -0,0 +1,13 @@ | |||
## This test checks retrieving dwo file diercetly with dwo name, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you clarify on why we are building main.exe twice.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
6cc4f88
to
144b6fb
Compare
144b6fb
to
b9bd7d2
Compare
@ayermolo Please have a look at this too. Thanks. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
…es not exist (llvm#154515) In distributed builds, the DWARF CompilationDir is often invalid, causing BOLT to fail when locating DWO files. If the default path does not exist, it seems better to consider the DWOName as a relative path in this case. The implementation of this patch will try to search for the DWO file in the following order: 1. CompDirOverride + DWOName (if CompDirOverride specified) 2. CompilationDir + DWOName (if CompilationDir exists) 3. **Current directory + DWOName (relative path as a fallback)** This patch also fixes a crash that occurs when DWOName is an absolute path and a DWP file is provided.
In distributed builds, the DWARF CompilationDir is often invalid, causing BOLT to fail when locating DWO files. If the default path does not exist, it seems better to consider the DWOName as a relative path in this case?
And I think this approach should be more intuitive and aligns with DWARF-related tools like llvm-dwp(see: https://reviews.llvm.org/D133480#).
The implementation of this patch will try to search for the DWO file in the following order: