[PATCH] D44407: [DebugInfo] Add a new method IPDBSession::findLineNumbersBySectOffset

[Aaron Smith via Phabricator via llvm-commits]

asmith added inline comments.


================
Comment at: lib/DebugInfo/PDB/DIA/DIASession.cpp:214
   CComPtr<IDiaEnumLineNumbers> LineNumbers;
-  if (S_OK != Session->findLinesByVA(Address, Length, &LineNumbers))
+  if (S_OK != Session->findLinesByVA(Address, Length, &LineNumbers)) {
+    ULONGLONG LoadAddr = 0;
----------------
asmith wrote:
> zturner wrote:
> > 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?
> Do you also want to delete this code from DIASession::findSymbolByAddress?
Looks like your commit added the logic to search by VA then RVA.

commit 7c69a5821492050ce95eb489a6411b8320277405
Author: Zachary Turner <zturner at 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;
+  }


Repository:
  rL LLVM

https://reviews.llvm.org/D44407