-
Notifications
You must be signed in to change notification settings - Fork 15.2k
[LLDB][PDB] Run UDT layout test with native PDB too #159769
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) ChangesThis test was failing with the native plugin due to two reasons:
I updated the checks to permit outputs from both plugins. Full diff: https://github.com/llvm/llvm-project/pull/159769.diff 1 Files Affected:
diff --git a/lldb/test/Shell/SymbolFile/PDB/udt-layout.test b/lldb/test/Shell/SymbolFile/PDB/udt-layout.test
index bc68539e25ec1..ed943bc9fb513 100644
--- a/lldb/test/Shell/SymbolFile/PDB/udt-layout.test
+++ b/lldb/test/Shell/SymbolFile/PDB/udt-layout.test
@@ -1,51 +1,63 @@
REQUIRES: target-windows, lld
RUN: %build --compiler=clang-cl --output=%t.exe %S/Inputs/UdtLayoutTest.cpp
-RUN: %lldb -b -s %S/Inputs/UdtLayoutTest.script -- %t.exe | FileCheck %s
+RUN: env LLDB_USE_NATIVE_PDB_READER=0 %lldb -b -s %S/Inputs/UdtLayoutTest.script -- %t.exe | FileCheck %s
+RUN: env LLDB_USE_NATIVE_PDB_READER=1 %lldb -b -s %S/Inputs/UdtLayoutTest.script -- %t.exe | FileCheck %s
-CHECK:(int) C::abc = 123
+CHECK:(int) {{(::)?}}C::abc = 123
CHECK:(List[16]) ls = {
-CHECK: [15] = {
-CHECK: Prev = nullptr
-CHECK: Next = nullptr
-CHECK: Value = {
-CHECK: B<0> = {
-CHECK: A = {
-CHECK: _u = (_u1 = '\x02', _u2 = 2, _u3 = 2)
-CHECK: }
-CHECK: _a = '\x01'
-CHECK: _b = 2
-CHECK: _c = 3
-CHECK: }
-CHECK: B<1> = {
-CHECK: A = {
-CHECK: _u = (_u1 = '\x02', _u2 = 2, _u3 = 2)
-CHECK: }
-CHECK: _a = '\x02'
-CHECK: _b = 4
-CHECK: _c = 6
-CHECK: }
-CHECK: B<2> = {
-CHECK: A = {
-CHECK: _u = (_u1 = '\x02', _u2 = 2, _u3 = 2)
-CHECK: }
-CHECK: _a = '\x03'
-CHECK: _b = 6
-CHECK: _c = 9
-CHECK: }
-CHECK: B<3> = {
-CHECK: A = {
-CHECK: _u = (_u1 = '\x02', _u2 = 2, _u3 = 2)
-CHECK: }
-CHECK: _a = '\x04'
-CHECK: _b = 8
-CHECK: _c = 12
-CHECK: }
-CHECK: A = {
-CHECK: _u = (_u1 = '\x02', _u2 = 2, _u3 = 2)
-CHECK: }
-CHECK: _x = 5
-CHECK: _y = 10
-CHECK: _z = '\x0f'
-CHECK: }
-CHECK: }
-CHECK:}
+
+CHECK: [14] = {
+CHECK-NEXT: Prev = nullptr
+CHECK-NEXT: Next = nullptr
+CHECK-NEXT: Value = {
+CHECK: B<0> = {
+CHECK-NEXT: A = {
+CHECK-NEXT: _u = (_u1 = '\x02', _u2 = 2, _u3 = 2)
+CHECK-NEXT: }
+CHECK-NEXT: _a = '\x01'
+CHECK-NEXT: _b = 2
+CHECK-NEXT: _c = 3
+CHECK-NEXT: }
+CHECK-NEXT: B<1> = {
+CHECK-NEXT: A = {
+CHECK-NEXT: _u = (_u1 = '\x02', _u2 = 2, _u3 = 2)
+CHECK-NEXT: }
+CHECK-NEXT: _a = '\x02'
+CHECK-NEXT: _b = 4
+CHECK-NEXT: _c = 6
+CHECK-NEXT: }
+CHECK: _x = 5
+CHECK-NEXT: _y = 10
+CHECK-NEXT: _z = '\x0f'
+CHECK-NEXT: }
+CHECK-NEXT: }
+
+CHECK-NEXT: [15] = {
+CHECK-NEXT: Prev = nullptr
+CHECK-NEXT: Next = nullptr
+CHECK-NEXT: Value = {
+CHECK: B<2> = {
+CHECK-NEXT: A = {
+CHECK-NEXT: _u = (_u1 = '\x02', _u2 = 2, _u3 = 2)
+CHECK-NEXT: }
+CHECK-NEXT: _a = '\x03'
+CHECK-NEXT: _b = 6
+CHECK-NEXT: _c = 9
+CHECK-NEXT: }
+CHECK-NEXT: B<3> = {
+CHECK-NEXT: A = {
+CHECK-NEXT: _u = (_u1 = '\x02', _u2 = 2, _u3 = 2)
+CHECK-NEXT: }
+CHECK-NEXT: _a = '\x04'
+CHECK-NEXT: _b = 8
+CHECK-NEXT: _c = 12
+CHECK-NEXT: }
+CHECK-NEXT: A = {
+CHECK-NEXT: _u = (_u1 = '\x02', _u2 = 2, _u3 = 2)
+CHECK-NEXT: }
+CHECK: _x = 5
+CHECK-NEXT: _y = 10
+CHECK-NEXT: _z = '\x0f'
+CHECK-NEXT: }
+CHECK-NEXT: }
+CHECK-NEXT:}
|
| CHECK: } | ||
| CHECK:} | ||
|
|
||
| CHECK: [14] = { |
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 we just have two CHECK prefixes. One for the native and one for the DIA plugin. And check the respective layouts.
I find that less confusing to someone reading this file for the first time
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 would fail if LLDB is compiled without DIA support. In that case, the native plugin would always be used, and it would fail the check intended for DIA.
I could also mark the whole test as DIA-only and add a second udt-layout-native.test for the native plugin.
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.
Does LLDB complain if a user requests the DIA plugin but LLDB was compiled without DIA support? If not, I think it's worth investigating
I could also mark the whole test as DIA-only and add a second udt-layout-native.test for the native plugin.
Yea I would prefer that. It would be nice if as many of the NativePDB tests stayed in the NativePDB subdirectory as possible. That way we don't need to worry about losing coverage when we delete the PDB plugin
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.
I added a separate test to the native plugin now.
Does LLDB complain if a user requests the DIA plugin but LLDB was compiled without DIA support? If not, I think it's worth investigating
This isn't the case right now. I'll take a look at printing a warning.
ccf7987 to
d2c6336
Compare
Michael137
left a comment
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.
great thanks!
If LLDB was built without the DIA SDK and the DIA reader is explicitly requested (through `LLDB_USE_NATIVE_PDB_READER=0` or `settings set plugin.symbol-file.pdb.reader dia`), LLDB should print a warning, because it will use the native reader in any case (#159769 (comment)). This PR adds the warning and a test when LLDB is not built with the SDK on Windows. I don't think any builder runs this configuration, as there are still five failing tests. I tested this locally with and without the SDK.
…ble (#160067) If LLDB was built without the DIA SDK and the DIA reader is explicitly requested (through `LLDB_USE_NATIVE_PDB_READER=0` or `settings set plugin.symbol-file.pdb.reader dia`), LLDB should print a warning, because it will use the native reader in any case (llvm/llvm-project#159769 (comment)). This PR adds the warning and a test when LLDB is not built with the SDK on Windows. I don't think any builder runs this configuration, as there are still five failing tests. I tested this locally with and without the SDK.
This test was failing with the native plugin due to two reasons:
The static
C::abcwas printed as(int) ::C::abc = 123The order of the base classes of
C(List::Value) is different between DIA and the native plugin. I don't know how the order in the DIA plugin is determined - it printsB<0>,B<1>,B<2>,B<3>,A. The native plugin follows the order of the bases in memory and printsB<2>,B<3>,A,B<0>,B<1>(last three are the virtual bases).Class layout of C
I split the tests for the plugins for better readability.