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

[Zachary Turner via llvm-commits]

Weird, I don’t remember it :)

In any case, can you make a common function called findSymbolByVAOrRVA?
Then have every caller use it
On Tue, Mar 13, 2018 at 8:43 PM Aaron Smith via Phabricator <
reviews at reviews.llvm.org> wrote:

> 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
>
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20180314/f918770b/attachment.html>