[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:358
       for (auto &R : FuncI.second.Ranges) {
-        Ranges[{R.first, R.second}] += 0;
+        Ranges[{R.first, R.second - 1}] += 0;
       }
----------------
wlei wrote:
> wenlei wrote:
> > So this is fixing a separate bug? And ranges end is actually `endOffset` + 1? If that's the case, some comments for `RangesTy Ranges` would be helpful.
> Yeah, a separate bug. Remembering that RangesTy's end is exclusive.
> 
> Still kind of related with this patch, because the extra one byte only can cause the out bounds issue.
> 
> Will add some comments.
This is indeed error prone. The end range in DWARF is exclusive but the input using by `findDisjointRanges` is required to be  inclusive.


================
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:
> > > 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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113238