[PATCH 12/22] covoar: Fix NOP execution marking
Gedare Bloom
gedare at rtems.org
Tue Mar 2 16:23:41 UTC 2021
On Mon, Mar 1, 2021 at 1:02 PM Alex White <alexanderjwhite at gmail.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 | 10 ++-
> tester/covoar/ObjdumpProcessor.cc | 112 +++++++++++++++++++++++-------
> 5 files changed, 139 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 )) {
You can merge this if conditional into the while expression.
while (a > 0 && !theCoverageMap->isStartOfInstruction( a )) {
is equivalent
> + 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..fb3efd6 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;
>
> @@ -227,7 +232,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..bf39fe9 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 = -1;
I don't really like this with -1 in an unsigned number. Beside which,
will this stuff using uint32_t break in a 64-bit address space?
>
> - // 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;
> + }
There has to be a more elegant way to write this loop.
> +
> + if (firstInstructionAddress == (uint32_t)-1) {
> + std::ostringstream what;
> + what << "Could not find first instruction address for symbol "
> + << symbolName << " in " << executableInfo->getFileName();
> + throw rld::error( what, "Coverage::finalizeSymbol" );
> + }
> +
> + bool inRange = false;
> + int rangeIndex = 0;
> + uint32_t lowAddress = -1;
> + while (!inRange) {
inRange is always false.
> + 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;
> + }
> + }
The complexity of this method suggests it might be a good idea to refactor it.
> +
> + if (highAddress != computedHighAddress) {
> + fprintf(
why not using C++ way of printing?
> + stderr,
> + "Function's high address differs between DWARF and objdump: "
> + "%s (0x%x and 0x%x)\n",
> + symbolName.c_str(), highAddress, computedHighAddress - 1
> + );
> + 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" );
> + }
> }
>
> ObjdumpProcessor::ObjdumpProcessor()
> --
> 2.27.0
>
> _______________________________________________
> devel mailing list
> devel at rtems.org
> http://lists.rtems.org/mailman/listinfo/devel
More information about the devel
mailing list