[PATCH] D113238: [llvm-profgen] Fix index out of bounds error while using ip.advance

[Wenlei He via Phabricator via llvm-commits]

wenlei added inline comments.


================
Comment at: llvm/tools/llvm-profgen/ProfileGenerator.cpp:388
     // can be Addr1+1 to Addr2-1. We should ignore such range.
     while (IP.Address <= RangeEnd) {
       uint64_t Offset = Binary->virtualAddrToOffset(IP.Address);
----------------
wlei wrote:
> wenlei wrote:
> > wlei wrote:
> > > wenlei wrote:
> > > > Before the loop, `RangeBegin <= RangeEnd` should be guaranteed, right? Would this look cleaner?
> > > > 
> > > > ```
> > > > do {
> > > > 
> > > >   ...
> > > > } while (IP.advance() && IP.Address <= RangeEnd);
> > > > 
> > > > ```
> > > Seems not guaranteed, see the comments: 
> > > ```
> > >     // Disjoint ranges may have range in the middle of two instr,
> > >     // e.g. If Instr1 at Addr1, and Instr2 at Addr2, disjoint range
> > >     // can be Addr1+1 to Addr2-1. We should ignore such range.
> > >     if (IP.Address > RangeEnd)
> > >       continue;
> > > ```
> > > or change to ?
> > > ```
> > > if (IP.Address > RangeEnd)
> > >      continue;
> > > do {
> > > 
> > >   ...
> > > } while (IP.advance() && IP.Address <= RangeEnd);
> > > 
> > > ```
> > Ok, I see. Yeah, that works. We could have `if (RangeStart > RangeEnd)` check before constructing the `IP`, and together with that comment.
> This is tricky. In this case, `RangeStart` is actually < `RangeEnd` but `IP.Address` is > `RangeEnd`, because `IP.Address` is round to next valid instruction. 
> 
> On a second thought, I'm wondering if we can remove the code with `InstructionPointer`. you see here it convert `Offset` --> `Address` then convert back to `Offset`. this seems redundant. I'm trying to work on `Offset` directly.
> Please take a look.
> This is tricky. In this case, RangeStart is actually < RangeEnd but IP.Address is > RangeEnd, because IP.Address is round to next valid instruction. 

Ah I see.. thanks for clarification. 

> On a second thought, I'm wondering if we can remove the code with InstructionPointer. you see here it convert Offset --> Address then convert back to Offset. this seems redundant. I'm trying to work on Offset directly. 

I'm still leaning towards keep such indexing details under some abstraction, and in this case, IP. Not too worried about the Addr -> offset conversion. Or if the conversion is a concern, perhaps we can change IP to store Offset by default instead of virtual address, since majority of the use is with Offset.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113238