[PATCH] covoar: Fix off-by-one in Coverage::finalizeSymbol()

[Gedare Bloom]

On Wed, Mar 31, 2021 at 10:42 AM Alex White <alex.white at oarcorp.com> wrote:
>
> On Wed, Mar 31, 2021 at 11:22 AM Gedare Bloom <gedare at rtems.org> wrote:
> >
> > On Wed, Mar 31, 2021 at 10:05 AM Alex White <alex.white at oarcorp.com> wrote:
> > >
> > > The `rangeIndex` variable is 1 higher than the index at which the first
> > > instruction address was found. This fixes the lookup after the loop to
> > > account for that fact.
> > > ---
> > >  tester/covoar/ObjdumpProcessor.cc | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/tester/covoar/ObjdumpProcessor.cc b/tester/covoar/ObjdumpProcessor.cc
> > > index 62a06c5..1cfa877 100644
> > > --- a/tester/covoar/ObjdumpProcessor.cc
> > > +++ b/tester/covoar/ObjdumpProcessor.cc
> > > @@ -60,7 +60,7 @@ namespace Coverage {
> > >          lowAddress = coverageMap.getLowAddressOfRange(rangeIndex);
> > >        }
> > >
> > > -      uint32_t sizeWithoutNops = coverageMap.getSizeOfRange(rangeIndex);
> > > +      uint32_t sizeWithoutNops = coverageMap.getSizeOfRange(rangeIndex - 1);
> >
> > I guess this is because of the for loop. Maybe, you should prefer to
> > exit the for loop with the proper index?
>
> That is what I had before this code was initially reviewed:
>
> for (rangeIndex = 0; ; rangeIndex++) {
>   lowAddress = coverageMap.getLowAddressOfRange(rangeIndex);
>
>   if (firstInstructionAddress == lowAddress) {
>     break;
>   }
> }
>
> The suggestion was made to move the condition into the loop rather than
> use `break`:
>
> for (rangeIndex = 0;
>      firstInstructionAddress != lowAddress;
>      rangeIndex++) {
>   lowAddress = coverageMap.getLowAddressOfRange(rangeIndex);
> }
>
> I did that without realizing it wasn't equivalent and didn't run enough
> tests to catch it.
>
> Am I missing something here? Could I go back to my first solution? Is
> there a more elegant way?
>

Yes, since the first iteration will always happen, and you want to end
with the 'right' value in the iterator, a do-while loop is more
appropriate.

> Your most tedious code reviewee,
>
> Alex
>
> :)
>
> >
> > Fixing an off-by-1 bug by subtracting one from the point of
> > consumption is a good way to ensure the bug occurs again if anyone
> > reuses the rangeIndex variable later.
>
> That is true.
>
> >
> > >        uint32_t size = sizeWithoutNops;
> > >        uint32_t highAddress = lowAddress + size - 1;
> > >        uint32_t computedHighAddress = highAddress;
> > > --
> > > 2.27.0
> > >
> > > _______________________________________________
> > > devel mailing list
> > > devel at rtems.org
> > > http://lists.rtems.org/mailman/listinfo/devel
> > _______________________________________________
> > devel mailing list
> > devel at rtems.org
> > http://lists.rtems.org/mailman/listinfo/devel