On Wed, Mar 31, 2021 at 11:22 AM Gedare Bloom <ged...@rtems.org> wrote:
>
> On Wed, Mar 31, 2021 at 10:05 AM Alex White <alex.wh...@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@rtems.org
> > http://lists.rtems.org/mailman/listinfo/devel
> _______________________________________________
> devel mailing list
> devel@rtems.org
> http://lists.rtems.org/mailman/listinfo/devel
_______________________________________________
devel mailing list
devel@rtems.org
http://lists.rtems.org/mailman/listinfo/devel
