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

[Lei Wang via Phabricator via llvm-commits]

wlei 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);
----------------
wenlei wrote:
> 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.
I see. If we changing to use `Offset` by default, it seems a lot code need to change in the unwinder where it heavily uses `Address` . If the conversion is fine, I can just change back to use address like previous version. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113238