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

[Alex White]

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?

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) {
>