[PATCH] D58848: [DebugInfo] follow up for "add SectionedAddress to DebugInfo interfaces"

[David Blaikie via Phabricator via llvm-commits]

dblaikie accepted this revision.
dblaikie added a comment.
This revision is now accepted and ready to land.

In D58848#1440161 <https://reviews.llvm.org/D58848#1440161>, @avl wrote:

> > Please move this down at least far enough that it doesn't involve separately 
> > computing the file name and opening the file. Where it's a "well, if you didn't 
> > specify a section index, we'll use the first section that covers this address".
>
> It looks like this version of patch does exactly this. 
>  It does not open file separately and does not parse file name. 
>  The only thing which is additionally done is for already opened file :
>  "well, if you didn't specify a section index, we'll use the first section that covers this address" :
>
> uint64_t SymbolizableObjectFile::getModuleSectionIndexForAddress(
>
>     uint64_t Address) const {
>   
>   for (SectionRef Sec : Module->sections()) {
>     if (!Sec.isText() || Sec.isVirtual())
>       continue;
>   
>     if (Address >= Sec.getAddress() &&
>         Address <= Sec.getAddress() + Sec.getSize()) {
>       return Sec.getIndex();
>     }
>   }
>   
>   return object::SectionedAddress::UndefSection;
>
> }
>
> I think it looks very close to above description.


Ah, so it does - thanks for correcting me/ walking me through that. Perhaps I was just reading poorly, or Phab was showing a different delta or something.

This looks good, thanks!

(I guess perhaps another/future change could be for these APIs to return an error if it is ambiguous (the symbolize APIs have an Error return type already, so they could change semantics here without changing the syntax - and that'd probably give a pretty good user experience of "hey, you're trying to symbolize an ambiguous address, we can't currently give you a clear answer to this query") - and, as discussed, perhaps one day they return multiple results, etc)


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D58848/new/

https://reviews.llvm.org/D58848