The current version of SymbolFilePDB::ParseVariableForPDBData function always initializes variables with an empty location. This patch adds the converter of a location information from PDB to a DWARF expression, so it becomes possible to watch values of variables of primitive data types. At the moment the converter supports only Static, TLS, RegRel, Enregistered and Constant PDB location types, but it seems that it's enough for most cases. There are still some problems with retrieving values of variables (e.g. we can't watch variables of composite types), but they look not relevant to the conversion to DWARF.
Details
Details
Diff Detail
Diff Detail
Event Timeline
Comment Actions
Excuse me, I have forgot to add lldb-commits as a subscriber, so I'll repeat initial message.
The current version of SymbolFilePDB::ParseVariableForPDBData function always initializes variables with an empty location. This patch adds the converter of a location information from PDB to a DWARF expression, so it becomes possible to watch values of variables of primitive data types. At the moment the converter supports only Static, TLS, RegRel, Enregistered and Constant PDB location types, but it seems that it's enough for most cases. There are still some problems with retrieving values of variables (e.g. we can't watch variables of composite types), but they look not relevant to the conversion to DWARF.
Comment Actions
Ping!
Can you review this, please? I have just implemented the UDT types completion for PDB symbol files and I am preparing a patch for that, but it makes no sense without the part implemented in this one :)
| source/Plugins/SymbolFile/PDB/PDBLocationToDWARFExpression.h | ||
|---|---|---|
| 27 | Yes, I think that /// should be there, it's a Doxygen comment (e.g. Visual Studio highlights info about a function and its parameters after that). | |
Comment Actions
Thanks a lot, I have updated the patch!
I also have added a test case for a non-zeroth frame (it became possible after D49111).
If it's all right, can you commit this for me, please? I have no commit access.
Comment Actions
I am not 100% sure that this is the cause yet, but the test variables.test is now failing on Windows. @aleksandr.urakov and @JDevlieghere, which tests did you run on Windows? Did they all pass?
Comment Actions
This doesn't look like the cause, the test fails for me without this patch... Here is my tests output for PDB folder:
-- Testing: 10 tests, 8 threads --
FAIL: lldb :: SymbolFile/PDB/enums-layout.test (1 of 10)
FAIL: lldb :: SymbolFile/PDB/typedefs.test (2 of 10)
FAIL: lldb :: SymbolFile/PDB/type-quals.test (3 of 10)
PASS: lldb :: SymbolFile/PDB/function-nested-block.test (4 of 10)
PASS: lldb :: SymbolFile/PDB/function-level-linking.test (5 of 10)
FAIL: lldb :: SymbolFile/PDB/func-symbols.test (6 of 10)
FAIL: lldb :: SymbolFile/PDB/variables.test (7 of 10)
FAIL: lldb :: SymbolFile/PDB/variables-locations.test (8 of 10)
FAIL: lldb :: SymbolFile/PDB/udt-completion.test (9 of 10)
PASS: lldb :: SymbolFile/PDB/compilands.test (10 of 10)
Testing Time: 23.51s
********************
Failing Tests (7):
lldb :: SymbolFile/PDB/enums-layout.test
lldb :: SymbolFile/PDB/func-symbols.test
lldb :: SymbolFile/PDB/type-quals.test
lldb :: SymbolFile/PDB/typedefs.test
lldb :: SymbolFile/PDB/udt-completion.test
lldb :: SymbolFile/PDB/variables-locations.test
lldb :: SymbolFile/PDB/variables.test
Expected Passes : 3
Unexpected Failures: 7Comment Actions
I'll spend some time looking into this today, but with commit 0fa537f42f1af238c74bf41998dc1af31195839a variables.test passes. Then with commit d9899ad86e0a9b05781015cacced1438fcf70343, the test fails. There are clearly a couple of other commits in that range, but they are a lot less likely to have caused the failure.
Comment Actions
Fwiw I’ve seen cases where tests have passed even though they shouldn’t
have — the functionality being tested was broken. The one that comes to
mind was where we were doing a backtrace and then checking that it matched
the regex “main\(argc=3” to make sure the local variable argc had the
correct value. But the actual backtrace was more like mian(argc=3751589203,
...). I.e. a garbage value.
This test passed for months this way until an unrelated change caused argc
to change to a different junk value in the backtrace
This isn’t necessarily the case here, but something to keep in mind.
Comment Actions
The CHECK-SAME expression on line 10 can no longer find the expected string in the output. This is due to an extra location = DW_OP_addr(0000000140004114) , in the output between the two expected strings CHECK-SAME: scope = global, external, so it looks like it is this change that is causing the failure. This can be fixed by updating the CHECK-SAME expression, but I will leave it up to you to decide if that is the correct fix. I also noticed that location = DW_OP_addr(0000000140004114) , has an extra space before the comma which other values do not. Here is the log:
# command stderr:
##[error]llvm\tools\lldb\lit\SymbolFile\PDB\variables.test(10,13): Error GBE9F3850: CHECK-SAME: expected string not found in input
3>C:\agent1\_work\15\s\llvm\tools\lldb\lit\SymbolFile\PDB\variables.test(10,13): error GBE9F3850: CHECK-SAME: expected string not found in input [C:\agent1\_work\15\b\LLVMBuild\tools\lldb\lit\check-lldb-lit.vcxproj]
CHECK-SAME: scope = global, external
^
<stdin>:117:58: note: scanning from here
0000022CA2072050: Variable{0x00000002}, name = "g_IntVar", type = {0000000000000004} 0x0000022ca20c2390 (int), scope = global, location = DW_OP_addr(0000000140004114) , external
^
<stdin>:117:112: note: possible intended match here
0000022CA2072050: Variable{0x00000002}, name = "g_IntVar", type = {0000000000000004} 0x0000022ca20c2390 (int), scope = global, location = DW_OP_addr(0000000140004114) , externalComment Actions
Yes, you are right, extra location = DW_OP_addr(0000000140004114) appeared after this commit. But I hadn't noticed that, because the test was already broken for me on 0fa537f42f1af238c74bf41998dc1af31195839a. Now I have checked one out and ran the test to reveal the cause of the failure. I think it's because of different name mangling. I have attached the output of lldb-test symbols to the message. For tests I use the last version of clang-cl (which I build along with lldb), what version do you use (and what version must be used in such cases)?
I think, that updating CHECK-SAME expression to allow location = {{.*}} is a good fix for this, because this patch actually adds a location information.
Comment Actions
When you send a review for the fix, please add me to it. Could you also take care of the extra space in the output? Thanks!
Comment Actions
The expression location = {{.*}}, should process that space correctly. It is possible to remove that space in DWARFExpression::DumpLocation function, but I don't know how necessary is it.
But what about a compiler version? I want to fix the issue if something wrong with my testing environment...
I've already added you to D49368. But it's not clear enough yet what patch will be applied, D49368 or D49410 :) I'll add you to D49410 too.
Comment Actions
Please fix the space. The test will process it fine, but that doesn't mean we shouldn't fix it. I'll email you with the setup we use for tests.
Comment Actions
I've removed spaces in such places, but i'm not sure if this won't break anything (may be something relies on that spaces). @clayborg, are these spaces have some special purpose? I think it's a decor and not really important.
Comment Actions
Can you explain me, please, why do you think that we should remove these spaces?
I have the following considerations:
- For eight years (the spaces were commited in 2010) some tests may have relied on these spaces. It's a good case - we can run tests and see it;
- Some formatting may rely on these spaces and it will be broken. It's a worse case - if it wasn't covered with tests, then we will not see it;
- Several people in several places, not only in DWARFExpressions, use such a formatting style. May be it's an approved way of formatting such things?
- Format-only commits make the use of git blame more difficult. It's not very horrible, but still adds some inconvenients;
- The only thing I can find to justify such a commit is the beauty. But the beauty is the matter of opinion and what some peoples find beautiful, other find not.
So peoples who commited such spaces found them beautiful (or used the approved formatting style, or used them for some special formatting cases), and I can't find the reason to fix that. Please, correct me if I am missing something?
Please remove the return