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

[Alex White]

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?

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