[PATCH v5 1/2] covoar: Fix NOP execution marking

[Alex White]

On Thu, Mar 25, 2021 at 2:38 PM Gedare Bloom <gedare at rtems.org> wrote:
>
> On Thu, Mar 25, 2021 at 10:05 AM Joel Sherrill <joel at rtems.org> wrote:
> >
> >
> >
> > On Thu, Mar 25, 2021 at 10:42 AM Gedare Bloom <gedare at rtems.org> wrote:
> >>
> >> On Thu, Mar 18, 2021 at 12:05 PM Alex White <alex.white at oarcorp.com> wrote:
> >> > -    // Mark the start of each instruction in the coverage map.
> >> > -    for (auto& instruction : instructions) {
> >> > -      coverageMap.setIsStartOfInstruction( instruction.address );
> >> > -    }
> >> > +      // Find the address of the first instruction.
> >> > +      for (auto& line : instructions) {
> >> > +        if (!line.isInstruction) {
> >> > +          continue;
> >> > +        }
> >> > +
> >> > +        firstInstructionAddress = line.address;
> >> > +        break;
> >> > +      }
> >> This is a poorly written loop. Anytime you use continue/break to
> >> control the loop iteration, something is going awry. You can write
> >> this code equivalently without any continue or break.
> >
> >
> > This loop is very deliberately constructed not to be a for loop because
> > you need the ability to break at different points to debug issues. Alex
> > and I spent hours in this loop debugging which I think would have
> > been more difficult to debug if all the conditions were in a for loop.
> > This loop was written to explicitly have breakpoints for various
> > conditions.
> >
> > There have been multiple issues with the information reported with
> > size/length being at the top of that list. Before Alex found the source
> > of that (not covoar), it resulted in an infinite loop.
> >
> > I suppose it can be changed to a for loop as purer academically but
> > it loses significantly on ability to set breakpoints and debug if there
> > ever is an issue again.
> >
> > Are you willing to make that trade?
> >
>
> You know that what we debug is often much uglier than what we publish.
> At least, simplify the logic to let the loop work for you.
>
> for (auto& line : instructions) {
>   if ( line.isInstruction ) {
>     firstInstructionAddress = line.address;
>     break;
>   }
> }

I will make this change.

>
> Using continue/break to control loops is antithetical to using loops,
> and probably doesn't do the compiler any good.
>
> >> > +        if (!line.isInstruction) {
> >> > +          continue;
> >> > +        }
> >> > +
> >> > +
> >> > +        break;
> >> > +      }
> >>
> >> > +
> >> > +      if (firstInstructionAddress == UINT32_MAX) {
> >> > +        std::ostringstream what;
> >> > +        what << "Could not find first instruction address for symbol "
> >> > +          << symbolName << " in " << executableInfo->getFileName();
> >> > +        throw rld::error( what, "Coverage::finalizeSymbol" );
> >> > +      }
> >> > +
> >> > +      int rangeIndex;
> >> > +      uint32_t lowAddress = UINT32_MAX;
> >> > +      for (rangeIndex = 0; ; rangeIndex++) {
> >> > +        lowAddress = coverageMap.getLowAddressOfRange(rangeIndex);
> >> > +
> >> > +        if (firstInstructionAddress == lowAddress) {
> >> > +          break;
> >> ditto... if you're goin to break the loop on this condition, why not
> >> use this in your for loop?
> >> for (rangeIndex = 0; firstInstructionAddress != lowAddress ; rangeIndex++) {

I will make this change, too.

> >>
> >> > +        }
> >> > +      }
> >> >
> >> > -    // Create a unified coverage map for the symbol.
> >> > -    SymbolsToAnalyze->createCoverageMap(
> >> > -      executableInfo->getFileName().c_str(), symbolName, size
> >> > -    );
> >> > +      uint32_t sizeWithoutNops = coverageMap.getSizeOfRange(rangeIndex);
> >> > +      uint32_t size = sizeWithoutNops;
> >> > +      uint32_t highAddress = lowAddress + size - 1;
> >> > +      uint32_t computedHighAddress = highAddress;
> >> > +
> >> > +      // Find the high address as reported by the address of the last NOP
> >> > +      // instruction. This ensures that NOPs get marked as executed later.
> >> > +      for (auto instruction = instructions.rbegin();
> >> > +          instruction != instructions.rend();
> >> > +          instruction++) {
> >> > +        if (instruction->isInstruction) {
> >> > +          if (instruction->isNop) {
> >> > +            computedHighAddress = instruction->address + instruction->nopSize;
> >> > +          }
> >> > +          break;
> >> Here too.

I'm not sure I see a more clear way to write this loop while
eliminating the `break` statement.

Do you have a suggestion?

> >>
> >> > +        }
> >> > +      }
> >> > +
> >> > +      if (highAddress != computedHighAddress) {
> >> > +        std::cerr << "Function's high address differs between DWARF and objdump: "
> >> > +          << symbolName << " (0x" << std::hex << highAddress << " and 0x"
> >> > +          << computedHighAddress - 1 << ")" << std::dec << std::endl;
> >> > +        size = computedHighAddress - lowAddress;
> >> > +      }
> >> > +
> >> > +      // If there are NOT already saved instructions, save them.
> >> > +      SymbolInformation* symbolInfo = SymbolsToAnalyze->find( symbolName );
> >> > +      if (symbolInfo->instructions.empty()) {
> >> > +        symbolInfo->sourceFile = executableInfo;
> >> > +        symbolInfo->baseAddress = lowAddress;
> >> > +        symbolInfo->instructions = instructions;
> >> > +      }
> >> > +
> >> > +      // Add the symbol to this executable's symbol table.
> >> > +      SymbolTable* theSymbolTable = executableInfo->getSymbolTable();
> >> > +      theSymbolTable->addSymbol(
> >> > +        symbolName, lowAddress, highAddress - lowAddress + 1
> >> > +      );
> >> > +
> >> > +      // Mark the start of each instruction in the coverage map.
> >> > +      for (auto& instruction : instructions) {
> >> > +        coverageMap.setIsStartOfInstruction( instruction.address );
> >> > +      }
> >> > +
> >> > +      // Create a unified coverage map for the symbol.
> >> > +      SymbolsToAnalyze->createCoverageMap(
> >> > +        executableInfo->getFileName().c_str(), symbolName, size, sizeWithoutNops
> >> > +      );
> >> > +    } catch (const ExecutableInfo::CoverageMapNotFoundError& e) {
> >> > +      // Allow execution to continue even if a coverage map could not be
> >> > +      // found.
> >> > +      std::cerr << "Coverage map not found for symbol " << e.what()
> >> > +        << std::endl;
> >> > +    }
> >> >    }
> >> >
> >> >    ObjdumpProcessor::ObjdumpProcessor()
> >> > --
> >> > 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
> _______________________________________________
> devel mailing list
> devel at rtems.org
> http://lists.rtems.org/mailman/listinfo/devel