Page MenuHomePhabricator

Convert a location information from PDB to a DWARF expression
ClosedPublic

Authored by aleksandr.urakov on Jul 6 2018, 4:53 AM.

Details

Summary

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.

Diff Detail

Repository
rL LLVM

Event Timeline

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.

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 :)

asmith accepted this revision.Jul 12 2018, 4:42 AM

Except for minor formatting LGTM

lit/SymbolFile/PDB/Inputs/VariablesLocationsTest.cpp
7 ↗(On Diff #154386)

Please remove the return

source/Plugins/SymbolFile/PDB/PDBLocationToDWARFExpression.h
26 ↗(On Diff #154386)

Are you sure about the /// ?

Should these be // ?

This revision is now accepted and ready to land.Jul 12 2018, 4:42 AM
aleksandr.urakov marked an inline comment as done.Jul 12 2018, 5:14 AM
aleksandr.urakov added inline comments.
source/Plugins/SymbolFile/PDB/PDBLocationToDWARFExpression.h
26 ↗(On Diff #154386)

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).

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.

Sure, I will do it Monday if someone hasn’t done it already.

This revision was automatically updated to reflect the committed changes.

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?

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: 7

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.

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.

stella.stamenova added a comment.EditedJul 16 2018, 12:58 PM

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) , external

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.

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!

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.

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.

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.

Can you test this change and send it for review? Thanks!

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?