This is an archive of the discontinued LLVM Phabricator instance.

[DebugInfo] Add a new method IPDBSession::findLineNumbersBySectOffset
ClosedPublic

Authored by asmith on Mar 12 2018, 6:07 PM.

Details

Summary

Some PDB symbols do not have a valid VA or RVA but have Addr by Section and Offset. For example, a variable in thread-local storage has the following properties:

get_addressOffset: 0
get_addressSection: 5
get_lexicalParentId: 2
get_name: g_tls
get_symIndexId: 12
get_typeId: 4
get_dataKind: 6
get_symTag: 7
get_locationType: 2

This change provides a new method to locate line numbers by Section and Offset from those symbols.

Diff Detail

Event Timeline

asmith created this revision.Mar 12 2018, 6:07 PM
zturner added inline comments.Mar 13 2018, 10:22 AM
lib/DebugInfo/PDB/DIA/DIASession.cpp
185

Why is this needed? This seems like the user has passed in an invalid address. Can we remove this?

214

I'm not sure it's worth putting this extra complexity into this function. Now the function is not so simple to reason about anymore. For example:

  • Why would Address be less than LoadAddr? That suggests you're calling this function with an invalid argument.
  • If findByVA failed, why would findByRVA succeed? That seems like a violation of DIA's API guarantees.

I think this method should have the same semantics as the DIA method and the same expectations.

Also, whatever the reason for this, it seems orthogonal to the goal of adding a method to find lines by section and offset, which is entirely handled by the new findLineNumbersBySectOffset method. Can we undo these changes?

This comment was removed by asmith.
asmith added inline comments.Mar 13 2018, 8:25 PM
lib/DebugInfo/PDB/DIA/DIASession.cpp
214

Do you also want to delete this code from DIASession::findSymbolByAddress?

asmith added inline comments.Mar 13 2018, 8:43 PM
lib/DebugInfo/PDB/DIA/DIASession.cpp
214

Looks like your commit added the logic to search by VA then RVA.

commit 7c69a5821492050ce95eb489a6411b8320277405
Author: Zachary Turner <zturner@google.com>
Date: Fri May 1 20:24:26 2015 +0000

+ if (S_OK != Session->findSymbolByVA(Address, EnumVal, &Symbol)) {
+ ULONGLONG LoadAddr = 0;
+ if (S_OK != Session->get_loadAddress(&LoadAddr))
+ return nullptr;
+ DWORD RVA = static_cast<DWORD>(Address - LoadAddr);
+ if (S_OK != Session->findSymbolByRVA(RVA, EnumVal, &Symbol))
+ return nullptr;
+ }

zturner added a subscriber: asmith.Mar 13 2018, 8:52 PM

Weird, I don’t remember it :)

In any case, can you make a common function called findSymbolByVAOrRVA?
Then have every caller use it

findLineNumbersByAddress() uses findLinesByVA/RVA and findSymbolByAddress() uses findSymbolByVA/RVA.

Do you still want them factored out into two methods: findLinesByVAOrRVA and findSymbolByVAOrRVA?

Ahh i see, maybe there is no way to avoid the duplication then. Still wish
I remembered why I did that, because now the code looks odd.

Anyway, lgtm

asmith updated this revision to Diff 138494.Mar 14 2018, 11:00 PM
asmith edited the summary of this revision. (Show Details)
asmith marked 4 inline comments as done.
This revision was not accepted when it landed; it landed in state Needs Review.Mar 14 2018, 11:07 PM
This revision was automatically updated to reflect the committed changes.