[PATCH 1/2] covoar: Fix NOP execution marking
Chris Johns
chrisj at rtems.org
Thu Mar 11 01:18:07 UTC 2021
On 11/3/21 10:33 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 | 108 +++++++++++++++++++++++-------
> 5 files changed, 136 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..1d8c1d0 100644
> --- a/tester/covoar/ObjdumpProcessor.cc
> +++ b/tester/covoar/ObjdumpProcessor.cc
> @@ -32,35 +32,91 @@ 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" );
> + }
> +
> + int rangeIndex;
> + uint32_t lowAddress = UINT32_MAX;
> + for (rangeIndex = 0; ; rangeIndex++) {
> + lowAddress = coverageMap.getLowAddressOfRange(rangeIndex);
> +
> + if (firstInstructionAddress == lowAddress) {
> + break;
> + }
> +
> + 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;
> + }
> + }
> +
> + 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) {
> + throw rld::error( e.what, "Coverage::finalizeSymbol" );
What does this error look like when an exception is thrown? My concern is this
changes hides the original source. Do you think this is important to do?
Chris
More information about the devel
mailing list