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

[Gedare Bloom]

On Thu, Mar 25, 2021 at 2:42 PM Alex White <alex.white at oarcorp.com> wrote:
>
> 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?
>
no, this one is alright, since you have a special action to take on
the last iteration.

fwiw, the isInstruction and isNop ought to be methods and not directly
accessing boolean variables, but that is just another C'ism nit ;)

> > >> > +
> > >> > +      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