[PATCH 1/2] covoar/CoverageReaderQEMU: Fix infinite loop

[Gedare Bloom]

On Mon, Mar 15, 2021 at 3:34 PM Alex White <alex.white at oarcorp.com> wrote:

> I honestly can't remember why I changed 1024 to 20,000.
>
> I've looked back at that code and changed it back to 1024 without any
> issues. I think I might have missed that this is all happening in a loop,
> and at one point during a (long) debugging session I convinced myself that
> it wasn't reading all of the entries.
>
> At least that's the most rational explanation I can think up for that
> particular change. 😊
>
> If I revert ENTRIES from 20000 back to 1024, are we satisfied to leave the
> "entries" array as-is?
>
>
I think that Chris' main points here are that, as you get covoar working
again and cleaning it up, it should be made more C++ (and less C).


> Alex
>
> -----Original Message-----
> From: Chris Johns <chrisj at rtems.org>
> Sent: Sunday, March 14, 2021 7:27 PM
> To: Alex White <alex.white at oarcorp.com>; devel at rtems.org
> Subject: Re: [PATCH 1/2] covoar/CoverageReaderQEMU: Fix infinite loop
>
> On 12/3/21 5:30 am, Alex White wrote:
> > There was a potential that the branch info loop never terminated.
> > This has been fixed by adding a more reliable termination condition
> > and logging an error if it cannot find the branch target.
> > ---
> >  tester/covoar/CoverageReaderQEMU.cc | 11 +++++++++--
> >  1 file changed, 9 insertions(+), 2 deletions(-)
> >
> > diff --git a/tester/covoar/CoverageReaderQEMU.cc
> > b/tester/covoar/CoverageReaderQEMU.cc
> > index 7c344e4..fb1709d 100644
> > --- a/tester/covoar/CoverageReaderQEMU.cc
> > +++ b/tester/covoar/CoverageReaderQEMU.cc
> > @@ -76,7 +76,7 @@ namespace Coverage {
> >      //
> >      // Read ENTRIES number of trace entries.
> >      //
> > -#define ENTRIES 1024
> > +#define ENTRIES 20000
>
> 1024 sure, 20,000 ... hmmm ... I am not so sure. If you need more would is
> the change 200,000? Maybe a better solution exists.
>
> >      while (true) {
> >        CoverageMapBase     *aCoverageMap = NULL;
> >        struct trace_entry  entries[ENTRIES];
>
> Can an array or resized vector be used here?
>
> > @@ -118,8 +118,15 @@ namespace Coverage {
> >          // Determine if additional branch information is available.
> >          if ( (entry->op & branchInfo) != 0 ) {
> >            uint32_t  a = entry->pc + entry->size - 1;
>
> An aside ... more pointers being used.
>
> Chris
>
> > -            while (!aCoverageMap->isStartOfInstruction(a))
> > +            while (a > entry->pc &&
> > + !aCoverageMap->isStartOfInstruction(a))
> >                a--;
> > +            if (a == entry->pc &&
> !aCoverageMap->isStartOfInstruction(a)) {
> > +              // Something went wrong parsing the objdump.
> > +              std::ostringstream what;
> > +              what << "Reached beginning of range in " << file
> > +                << " at " << entry->pc << " with no start of
> instruction.";
> > +              throw rld::error( what, "CoverageReaderQEMU::processFile"
> );
> > +            }
> >              if (entry->op & taken) {
> >                aCoverageMap->setWasTaken( a );
> >              } else if (entry->op & notTaken) {
> >
> _______________________________________________
> devel mailing list
> devel at rtems.org
> http://lists.rtems.org/mailman/listinfo/devel
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.rtems.org/pipermail/devel/attachments/20210315/0822ffd3/attachment.html>