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

Chris Johns chrisj at rtems.org
Mon Mar 15 00:52:07 UTC 2021


This v3 set is blocked by the v2 questions I have raised.

Chris

On 13/3/21 2:48 am, Alex White 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/ObjdumpProcessor.cc | 112 +++++++++++++++++++++++-------
>  5 files changed, 140 insertions(+), 35 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/ObjdumpProcessor.cc b/tester/covoar/ObjdumpProcessor.cc
> index 56ee219..9d10a40 100644
> --- a/tester/covoar/ObjdumpProcessor.cc
> +++ b/tester/covoar/ObjdumpProcessor.cc
> @@ -32,35 +32,95 @@ 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;
> +      }
> +
> +      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" );
> +      }
>  
> -    // Create a unified coverage map for the symbol.
> -    SymbolsToAnalyze->createCoverageMap(
> -      executableInfo->getFileName().c_str(), symbolName, size
> -    );
> +      int rangeIndex;
> +      uint32_t lowAddress = UINT32_MAX;
> +      for (rangeIndex = 0; ; rangeIndex++) {
> +        lowAddress = coverageMap.getLowAddressOfRange(rangeIndex);
> +
> +        if (firstInstructionAddress == lowAddress) {
> +          break;
> +        }
> +      }
> +
> +      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;
> +        }
> +      }
> +
> +      if (highAddress != computedHighAddress) {
> +        std::cerr << "Function's high address differs between DWARF and objdump: "
> +          << symbolName << " (0x" << std::hex << highAddress << " and 0x"
> +          << computedHighAddress - 1 << ")" << 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 (rld::error& e) {
> +      if (e.where == "ExecutableInfo::findCoverageMap") {
> +        // Allow execution to continue even if a coverage map could not be
> +        // found.
> +        std::cerr << e.where << ": " << e.what << std::endl;
> +      } else {
> +        throw e;
> +      }
> +    }
>    }
>  
>    ObjdumpProcessor::ObjdumpProcessor()
> 


More information about the devel mailing list