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

Joel Sherrill joel at rtems.org
Thu Mar 25 16:05:22 UTC 2021


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:
> >
> > Some NOP instructions were not being marked as executed because they
> > are located at the end of uncovered ranges. This has been fixed.
> > ---
> >  tester/covoar/CoverageMapBase.cc  |  10 +++
> >  tester/covoar/CoverageMapBase.h   |   4 ++
> >  tester/covoar/DesiredSymbols.cc   |  38 +++++++++--
> >  tester/covoar/DesiredSymbols.h    |  11 ++-
> >  tester/covoar/ExecutableInfo.cc   |   2 +-
> >  tester/covoar/ExecutableInfo.h    |   4 ++
> >  tester/covoar/ObjdumpProcessor.cc | 109 +++++++++++++++++++++++-------
> >  7 files changed, 142 insertions(+), 36 deletions(-)
> >
> > diff --git a/tester/covoar/CoverageMapBase.cc
> b/tester/covoar/CoverageMapBase.cc
> > index ad0080d..6ca5cf7 100644
> > --- a/tester/covoar/CoverageMapBase.cc
> > +++ b/tester/covoar/CoverageMapBase.cc
> > @@ -142,6 +142,11 @@ namespace Coverage {
> >      return size;
> >    }
> >
> > +  uint32_t CoverageMapBase::getSizeOfRange( size_t index ) const
> > +  {
> > +    return Ranges.at(index).size();
> > +  }
> > +
> >    bool CoverageMapBase::getBeginningOfInstruction(
> >      uint32_t  address,
> >      uint32_t* beginning
> > @@ -178,6 +183,11 @@ namespace Coverage {
> >      return Ranges.front().lowAddress;
> >    }
> >
> > +  uint32_t CoverageMapBase::getLowAddressOfRange( size_t index ) const
> > +  {
> > +    return Ranges.at(index).lowAddress;
> > +  }
> > +
> >    bool CoverageMapBase::getRange( uint32_t address, AddressRange& range
> ) const
> >    {
> >      for ( auto r : Ranges ) {
> > diff --git a/tester/covoar/CoverageMapBase.h
> b/tester/covoar/CoverageMapBase.h
> > index 6ad76d3..a58c696 100644
> > --- a/tester/covoar/CoverageMapBase.h
> > +++ b/tester/covoar/CoverageMapBase.h
> > @@ -156,6 +156,8 @@ namespace Coverage {
> >       */
> >      int32_t getFirstLowAddress() const;
> >
> > +    uint32_t getLowAddressOfRange( size_t index ) const;
> > +
> >      /*!
> >       *  This method returns true and sets the address range if
> >       *  the address falls with the bounds of an address range
> > @@ -177,6 +179,8 @@ namespace Coverage {
> >       */
> >      uint32_t getSize() const;
> >
> > +    uint32_t getSizeOfRange( size_t index ) const;
> > +
> >      /*!
> >       *  This method returns the address of the beginning of the
> >       *  instruction that contains the specified address.
> > diff --git a/tester/covoar/DesiredSymbols.cc
> b/tester/covoar/DesiredSymbols.cc
> > index b9a5bb7..c97b25c 100644
> > --- a/tester/covoar/DesiredSymbols.cc
> > +++ b/tester/covoar/DesiredSymbols.cc
> > @@ -142,7 +142,7 @@ namespace Coverage {
> >        CoverageMapBase* theCoverageMap = s.second.unifiedCoverageMap;
> >        if (theCoverageMap)
> >        {
> > -        // Increment the total sizeInBytes byt the bytes in the symbol
> > +        // Increment the total sizeInBytes by the bytes in the symbol
> >          stats.sizeInBytes += s.second.stats.sizeInBytes;
> >
> >          // Now scan through the coverage map of this symbol.
> > @@ -202,6 +202,26 @@ namespace Coverage {
> >          uint32_t count;
> >
> >          // Mark NOPs as executed
> > +        a = s.second.stats.sizeInBytes - 1;
> > +        count = 0;
> > +        while (a > 0) {
> > +          if (theCoverageMap->isStartOfInstruction( a )) {
> > +            break;
> > +          }
> > +
> > +          count++;
> > +
> > +          if (theCoverageMap->isNop( a )) {
> > +            for (la = a; la < (a + count); la++) {
> > +              theCoverageMap->setWasExecuted( la );
> > +            }
> > +
> > +            count = 0;
> > +          }
> > +
> > +          a--;
> > +        }
> > +
> >          endAddress = s.second.stats.sizeInBytes - 1;
> >          a = 0;
> >          while (a < endAddress) {
> > @@ -223,12 +243,13 @@ namespace Coverage {
> >                ha++;
> >                if ( ha >= endAddress )
> >                  break;
> > -            } while ( !theCoverageMap->isStartOfInstruction( ha ) );
> > +            } while ( !theCoverageMap->isStartOfInstruction( ha ) ||
> > +                      theCoverageMap->isNop( ha ) );
> >            a = ha;
> >          }
> >
> >          // Now scan through the coverage map of this symbol.
> > -        endAddress = s.second.stats.sizeInBytes - 1;
> > +        endAddress = s.second.stats.sizeInBytesWithoutNops - 1;
> >          a = 0;
> >          while (a <= endAddress) {
> >            // If an address was NOT executed, find consecutive unexecuted
> > @@ -316,7 +337,8 @@ namespace Coverage {
> >    void DesiredSymbols::createCoverageMap(
> >      const std::string& exefileName,
> >      const std::string& symbolName,
> > -    uint32_t           size
> > +    uint32_t           size,
> > +    uint32_t           sizeWithoutNops
> >    )
> >    {
> >      CoverageMapBase* aCoverageMap;
> > @@ -354,9 +376,10 @@ namespace Coverage {
> >                    << '/' << size << ')'
> >                    << std::endl;
> >
> > -        if ( itr->second.stats.sizeInBytes < size )
> > +        if ( itr->second.stats.sizeInBytes < size ) {
> >            itr->second.stats.sizeInBytes = size;
> > -        else
> > +          itr->second.stats.sizeInBytesWithoutNops = sizeWithoutNops;
> > +        } else
> >            size = itr->second.stats.sizeInBytes;
> >        }
> >      }
> > @@ -376,6 +399,7 @@ namespace Coverage {
> >          );
> >        itr->second.unifiedCoverageMap = aCoverageMap;
> >        itr->second.stats.sizeInBytes = size;
> > +      itr->second.stats.sizeInBytesWithoutNops = sizeWithoutNops;
> >      }
> >    }
> >
> > @@ -479,7 +503,7 @@ namespace Coverage {
> >      // are the same size.
> >      // Changed from ERROR msg to INFO, because size mismatch is not
> >      // treated as error anymore. 2015-07-20
> > -    uint32_t dMapSize = sinfo.stats.sizeInBytes;
> > +    uint32_t dMapSize = sinfo.stats.sizeInBytesWithoutNops;
> >      uint32_t sBaseAddress = sourceCoverageMap->getFirstLowAddress();
> >      uint32_t sMapSize = sourceCoverageMap->getSize();
> >      if (dMapSize != 0 && dMapSize != sMapSize) {
> > diff --git a/tester/covoar/DesiredSymbols.h
> b/tester/covoar/DesiredSymbols.h
> > index 5c45af8..367ca95 100644
> > --- a/tester/covoar/DesiredSymbols.h
> > +++ b/tester/covoar/DesiredSymbols.h
> > @@ -57,7 +57,12 @@ namespace Coverage {
> >      uint32_t sizeInBytes;
> >
> >      /*!
> > -     *  This member contains the size in Bytes.
> > +     *  This member contains the size in Bytes not accounting for NOPs.
> > +     */
> > +    uint32_t sizeInBytesWithoutNops;
> > +
> > +    /*!
> > +     *  This member contains the size in instructions.
> >       */
> >      uint32_t sizeInInstructions;
> >
> > @@ -100,6 +105,7 @@ namespace Coverage {
> >         branchesNeverTaken(0),
> >         branchesNotExecuted(0),
> >         sizeInBytes(0),
> > +       sizeInBytesWithoutNops(0),
> >         sizeInInstructions(0),
> >         uncoveredBytes(0),
> >         uncoveredInstructions(0),
> > @@ -227,7 +233,8 @@ namespace Coverage {
> >      void createCoverageMap(
> >        const std::string& exefileName,
> >        const std::string& symbolName,
> > -      uint32_t           size
> > +      uint32_t           size,
> > +      uint32_t           sizeWithoutNops
> >      );
> >
> >      /*!
> > diff --git a/tester/covoar/ExecutableInfo.cc
> b/tester/covoar/ExecutableInfo.cc
> > index c593e1d..ddd2987 100644
> > --- a/tester/covoar/ExecutableInfo.cc
> > +++ b/tester/covoar/ExecutableInfo.cc
> > @@ -127,7 +127,7 @@ namespace Coverage {
> >    {
> >      CoverageMaps::iterator cmi = coverageMaps.find( symbolName );
> >      if ( cmi == coverageMaps.end() )
> > -      throw rld::error (symbolName, "ExecutableInfo::findCoverageMap");
> > +      throw CoverageMapNotFoundError(symbolName);
> >      return *(cmi->second);
> >    }
> >
> > diff --git a/tester/covoar/ExecutableInfo.h
> b/tester/covoar/ExecutableInfo.h
> > index dcb4dcb..ab74b7f 100644
> > --- a/tester/covoar/ExecutableInfo.h
> > +++ b/tester/covoar/ExecutableInfo.h
> > @@ -29,6 +29,10 @@ namespace Coverage {
> >
> >    public:
> >
> > +    class CoverageMapNotFoundError : public std::runtime_error {
> > +      using std::runtime_error::runtime_error;
> > +    };
> > +
> >      /*!
> >       *  This method constructs an ExecutableInfo instance.
> >       *
> > diff --git a/tester/covoar/ObjdumpProcessor.cc
> b/tester/covoar/ObjdumpProcessor.cc
> > index 56ee219..0647ff9 100644
> > --- a/tester/covoar/ObjdumpProcessor.cc
> > +++ b/tester/covoar/ObjdumpProcessor.cc
> > @@ -32,35 +32,92 @@ namespace Coverage {
> >      ObjdumpProcessor::objdumpLines_t instructions
> >    ) {
> >      // Find the symbol's coverage map.
> > -    CoverageMapBase& coverageMap = executableInfo->findCoverageMap(
> symbolName );
> > -
> > -    uint32_t lowAddress = coverageMap.getFirstLowAddress();
> > -    uint32_t size = coverageMap.getSize();
> > -    uint32_t highAddress = lowAddress + size - 1;
> > -
> > -    // 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;
> > -    }
> > +    try {
> > +      CoverageMapBase& coverageMap =
> executableInfo->findCoverageMap(symbolName);
> >
> > -    // Add the symbol to this executable's symbol table.
> > -    SymbolTable* theSymbolTable = executableInfo->getSymbolTable();
> > -    theSymbolTable->addSymbol(
> > -      symbolName, lowAddress, highAddress - lowAddress + 1
> > -    );
> > +      uint32_t firstInstructionAddress = UINT32_MAX;
> >
> > -    // 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?


> > +
> > +      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++)
> {
>
> > +        }
> > +      }
> >
> > -    // 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.
>
> > +        }
> > +      }
> > +
> > +      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
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.rtems.org/pipermail/devel/attachments/20210325/8c49cee6/attachment-0001.html>


More information about the devel mailing list