-
Notifications
You must be signed in to change notification settings - Fork 15k
[LLDB] Use native PDB reader by default #165363
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
Conversation
|
@llvm/pr-subscribers-lldb Author: nerix (Nerixyz) ChangesAll PDB tests now pass when compiled without DIA on Windows, so they pass with the native reader. With this PR, the default reader changes to the native reader. For now, DIA can be used by setting Full diff: https://github.com/llvm/llvm-project/pull/165363.diff 4 Files Affected:
diff --git a/lldb/source/Plugins/SymbolFile/PDB/SymbolFilePDB.cpp b/lldb/source/Plugins/SymbolFile/PDB/SymbolFilePDB.cpp
index 3b936c06b1072..0ccb1804bb13a 100644
--- a/lldb/source/Plugins/SymbolFile/PDB/SymbolFilePDB.cpp
+++ b/lldb/source/Plugins/SymbolFile/PDB/SymbolFilePDB.cpp
@@ -83,8 +83,8 @@ constexpr OptionEnumValueElement g_pdb_reader_enums[] = {
{
ePDBReaderDefault,
"default",
- "Use DIA PDB reader unless LLDB_USE_NATIVE_PDB_READER environment "
- "variable is set",
+ "Use native PDB reader unless LLDB_USE_NATIVE_PDB_READER environment "
+ "is set to 0",
},
{
ePDBReaderDIA,
@@ -109,16 +109,10 @@ enum {
static const bool g_should_use_native_reader_by_default = [] {
llvm::StringRef env_value = ::getenv("LLDB_USE_NATIVE_PDB_READER");
-#if !LLVM_ENABLE_DIA_SDK || !defined(_WIN32)
- // if the environment value is unset, the native reader is requested
- if (env_value.empty())
- return true;
-#endif
-
- return env_value.equals_insensitive("on") ||
- env_value.equals_insensitive("yes") ||
- env_value.equals_insensitive("1") ||
- env_value.equals_insensitive("true");
+ return !env_value.equals_insensitive("off") &&
+ !env_value.equals_insensitive("no") &&
+ !env_value.equals_insensitive("0") &&
+ !env_value.equals_insensitive("false");
}();
class PluginProperties : public Properties {
diff --git a/lldb/test/Shell/SymbolFile/NativePDB/native-setting.cpp b/lldb/test/Shell/SymbolFile/NativePDB/native-setting.cpp
index dc26ec8d30cb4..91f451fd0dadc 100644
--- a/lldb/test/Shell/SymbolFile/NativePDB/native-setting.cpp
+++ b/lldb/test/Shell/SymbolFile/NativePDB/native-setting.cpp
@@ -8,9 +8,9 @@
// RUN: env LLDB_USE_NATIVE_PDB_READER=0 %lldb %t.exe -o 'target modules dump symfile' 2>&1 | FileCheck --check-prefix=ENV0 %s
// RUN: env LLDB_USE_NATIVE_PDB_READER=1 %lldb %t.exe -o 'target modules dump symfile' 2>&1 | FileCheck --check-prefix=ENV1 %s
-// RUN: env LLDB_USE_NATIVE_PDB_READER=foo %lldb %t.exe -o 'target modules dump symfile' 2>&1 | FileCheck --check-prefix=ENV0 %s
-// RUN: env LLDB_USE_NATIVE_PDB_READER=42 %lldb %t.exe -o 'target modules dump symfile' 2>&1 | FileCheck --check-prefix=ENV0 %s
-// RUN: env LLDB_USE_NATIVE_PDB_READER=-1 %lldb %t.exe -o 'target modules dump symfile' 2>&1 | FileCheck --check-prefix=ENV0 %s
+// RUN: env LLDB_USE_NATIVE_PDB_READER=foo %lldb %t.exe -o 'target modules dump symfile' 2>&1 | FileCheck --check-prefix=ENV1 %s
+// RUN: env LLDB_USE_NATIVE_PDB_READER=42 %lldb %t.exe -o 'target modules dump symfile' 2>&1 | FileCheck --check-prefix=ENV1 %s
+// RUN: env LLDB_USE_NATIVE_PDB_READER=-1 %lldb %t.exe -o 'target modules dump symfile' 2>&1 | FileCheck --check-prefix=ENV1 %s
// RUN: env LLDB_USE_NATIVE_PDB_READER=0 %lldb \
// RUN: -o 'settings set plugin.symbol-file.pdb.reader dia' \
diff --git a/lldb/test/Shell/SymbolFile/PDB/native-setting.cpp b/lldb/test/Shell/SymbolFile/PDB/native-setting.cpp
index f5e54592b0b31..54b7f28a71259 100644
--- a/lldb/test/Shell/SymbolFile/PDB/native-setting.cpp
+++ b/lldb/test/Shell/SymbolFile/PDB/native-setting.cpp
@@ -8,9 +8,9 @@
// RUN: env LLDB_USE_NATIVE_PDB_READER=0 %lldb %t.exe -o 'target modules dump symfile' 2>&1 | FileCheck --check-prefix=ENV0 %s
// RUN: env LLDB_USE_NATIVE_PDB_READER=1 %lldb %t.exe -o 'target modules dump symfile' 2>&1 | FileCheck --check-prefix=ENV1 %s
-// RUN: env LLDB_USE_NATIVE_PDB_READER=foo %lldb %t.exe -o 'target modules dump symfile' 2>&1 | FileCheck --check-prefix=ENV0 %s
-// RUN: env LLDB_USE_NATIVE_PDB_READER=42 %lldb %t.exe -o 'target modules dump symfile' 2>&1 | FileCheck --check-prefix=ENV0 %s
-// RUN: env LLDB_USE_NATIVE_PDB_READER=-1 %lldb %t.exe -o 'target modules dump symfile' 2>&1 | FileCheck --check-prefix=ENV0 %s
+// RUN: env LLDB_USE_NATIVE_PDB_READER=foo %lldb %t.exe -o 'target modules dump symfile' 2>&1 | FileCheck --check-prefix=ENV1 %s
+// RUN: env LLDB_USE_NATIVE_PDB_READER=42 %lldb %t.exe -o 'target modules dump symfile' 2>&1 | FileCheck --check-prefix=ENV1 %s
+// RUN: env LLDB_USE_NATIVE_PDB_READER=-1 %lldb %t.exe -o 'target modules dump symfile' 2>&1 | FileCheck --check-prefix=ENV1 %s
// RUN: env LLDB_USE_NATIVE_PDB_READER=0 %lldb \
// RUN: -o 'settings set plugin.symbol-file.pdb.reader dia' \
@@ -36,7 +36,7 @@
// NO-ENV-NOT: warning:
// NO-ENV: (lldb) target modules dump symfile
// NO-ENV: Dumping debug symbols for 1 modules.
-// NO-ENV: SymbolFile pdb
+// NO-ENV: SymbolFile native-pdb
// ENV0-NOT: warning:
// ENV0: (lldb) target modules dump symfile
diff --git a/llvm/docs/ReleaseNotes.md b/llvm/docs/ReleaseNotes.md
index 36383b12788f9..fa99fc0338c56 100644
--- a/llvm/docs/ReleaseNotes.md
+++ b/llvm/docs/ReleaseNotes.md
@@ -191,6 +191,10 @@ Changes to LLDB
* The `show-progress` setting, which became a NOOP with the introduction of the
statusline, now defaults to off and controls using OSC escape codes to show a
native progress bar in supporting terminals like Ghostty and ConEmu.
+* The default PDB reader on Windows was changed from DIA to native, which uses
+ LLVM's PDB and CodeView support. You can switch back to the DIA reader with
+ `settings set plugin.symbol-file.pdb.reader dia`. Note that support for the
+ DIA reader will be removed in future versions of LLDB.
Changes to BOLT
---------------------------------
|
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.
SGTM, thanks for pushing through! Excited to see one of these plugins go away.
I assume the PDB shell-tests all explicitly turn on DIA? And the native ones turn it off? I.e., all the test coverage remains the same after this patch?
Please let @DavidSpickett and @JDevlieghere also take a look before merging.
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 with a small comment on the release note.
I might have expected to see a lot of tests flip from opting in to opting out, but I think thanks to yours and others' hard work, all the tests run on both plugins now, correct?
|
I also forgot that when not built on Windows, we have always been using native. So this work has improved the remote debugging experience too! |
|
Thanks for this. |
Yes, they use the environment variable:
|
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.
This is a huge achievement. Congratulations and thank you to everyone that helped make this happen.
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.
Thank you for this!
I did not realise the setting hasn't been around that long. It was added in #151490:
So for as long as we do / might need to do this, the setting remains. Even if we don't mention it in release notes, it is something users can discover. I was wondering if we should remove it so we didn't have one more thing to keep around for another release cycle. I think not though. If all goes to plan:
You can merge this whenever you're ready. |
|
Thank you to everyone who helped with the native plugin and LLVM's PDB support! |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/211/builds/3312 Here is the relevant piece of the build log for the reference |
Fixes the failing DIA unit test (https://lab.llvm.org/buildbot/#/builders/197/builds/10342) after #165363. Now that the native plugin is the default, we need to set the symbol file plugin for DIA via the settings.
After the default PDB plugin changed to the native one (#165363), this test failed, because it uses the size of public symbols and the native plugin sets the size to 0 (as PDB doesn't include this information explicitly). A PDB was built because the final executable in that test was linked with `-gdwarf`.
The buildbots lldb-remote-linux-win and lldb-aarch64-windows are broken too. So, all lldb Windows buildbots are broken. I recommend to revert this patch for now and investigate the problem. |
They're no longer broken after the two PRs. |
|
Ok, thanks. |
All PDB tests now pass when compiled without DIA on Windows, so they pass with the native reader. With this PR, the default reader changes to the native reader. The plan is to eventually remove the DIA reader (see https://discourse.llvm.org/t/rfc-removing-the-dia-pdb-plugin-from-lldb/87827 and llvm#114906). For now, DIA can be used by setting `plugin.symbol-file.pdb.reader` to `dia` or by setting `LLDB_USE_NATIVE_PDB_READER=0` (mostly undocumented, but used in tests).
Fixes the failing DIA unit test (https://lab.llvm.org/buildbot/#/builders/197/builds/10342) after llvm#165363. Now that the native plugin is the default, we need to set the symbol file plugin for DIA via the settings.
After the default PDB plugin changed to the native one (llvm#165363), this test failed, because it uses the size of public symbols and the native plugin sets the size to 0 (as PDB doesn't include this information explicitly). A PDB was built because the final executable in that test was linked with `-gdwarf`.
All PDB tests now pass when compiled without DIA on Windows, so they pass with the native reader.
With this PR, the default reader changes to the native reader.
The plan is to eventually remove the DIA reader (see https://discourse.llvm.org/t/rfc-removing-the-dia-pdb-plugin-from-lldb/87827 and #114906).
For now, DIA can be used by setting
plugin.symbol-file.pdb.readertodiaor by settingLLDB_USE_NATIVE_PDB_READER=0(mostly undocumented, but used in tests).