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

[Zachary Turner via Phabricator via llvm-commits]

zturner added inline comments.


================
Comment at: lib/DebugInfo/PDB/DIA/DIASession.cpp:185
       return nullptr;
+    if (Address < LoadAddr)
+      return nullptr;
----------------
Why is this needed?  This seems like the user has passed in an invalid address.  Can we remove this?


================
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;
----------------
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?


Repository:
  rL LLVM

https://reviews.llvm.org/D44407