<div dir="ltr"><div dir="ltr"><br></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Thu, Mar 25, 2021 at 10:42 AM Gedare Bloom <<a href="mailto:gedare@rtems.org">gedare@rtems.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">On Thu, Mar 18, 2021 at 12:05 PM Alex White <<a href="mailto:alex.white@oarcorp.com" target="_blank">alex.white@oarcorp.com</a>> wrote:<br>
><br>
> Some NOP instructions were not being marked as executed because they<br>
> are located at the end of uncovered ranges. This has been fixed.<br>
> ---<br>
> tester/covoar/CoverageMapBase.cc | 10 +++<br>
> tester/covoar/CoverageMapBase.h | 4 ++<br>
> tester/covoar/DesiredSymbols.cc | 38 +++++++++--<br>
> tester/covoar/DesiredSymbols.h | 11 ++-<br>
> tester/covoar/ExecutableInfo.cc | 2 +-<br>
> tester/covoar/ExecutableInfo.h | 4 ++<br>
> tester/covoar/ObjdumpProcessor.cc | 109 +++++++++++++++++++++++-------<br>
> 7 files changed, 142 insertions(+), 36 deletions(-)<br>
><br>
> diff --git a/tester/covoar/CoverageMapBase.cc b/tester/covoar/CoverageMapBase.cc<br>
> index ad0080d..6ca5cf7 100644<br>
> --- a/tester/covoar/CoverageMapBase.cc<br>
> +++ b/tester/covoar/CoverageMapBase.cc<br>
> @@ -142,6 +142,11 @@ namespace Coverage {<br>
> return size;<br>
> }<br>
><br>
> + uint32_t CoverageMapBase::getSizeOfRange( size_t index ) const<br>
> + {<br>
> + return Ranges.at(index).size();<br>
> + }<br>
> +<br>
> bool CoverageMapBase::getBeginningOfInstruction(<br>
> uint32_t address,<br>
> uint32_t* beginning<br>
> @@ -178,6 +183,11 @@ namespace Coverage {<br>
> return Ranges.front().lowAddress;<br>
> }<br>
><br>
> + uint32_t CoverageMapBase::getLowAddressOfRange( size_t index ) const<br>
> + {<br>
> + return Ranges.at(index).lowAddress;<br>
> + }<br>
> +<br>
> bool CoverageMapBase::getRange( uint32_t address, AddressRange& range ) const<br>
> {<br>
> for ( auto r : Ranges ) {<br>
> diff --git a/tester/covoar/CoverageMapBase.h b/tester/covoar/CoverageMapBase.h<br>
> index 6ad76d3..a58c696 100644<br>
> --- a/tester/covoar/CoverageMapBase.h<br>
> +++ b/tester/covoar/CoverageMapBase.h<br>
> @@ -156,6 +156,8 @@ namespace Coverage {<br>
> */<br>
> int32_t getFirstLowAddress() const;<br>
><br>
> + uint32_t getLowAddressOfRange( size_t index ) const;<br>
> +<br>
> /*!<br>
> * This method returns true and sets the address range if<br>
> * the address falls with the bounds of an address range<br>
> @@ -177,6 +179,8 @@ namespace Coverage {<br>
> */<br>
> uint32_t getSize() const;<br>
><br>
> + uint32_t getSizeOfRange( size_t index ) const;<br>
> +<br>
> /*!<br>
> * This method returns the address of the beginning of the<br>
> * instruction that contains the specified address.<br>
> diff --git a/tester/covoar/DesiredSymbols.cc b/tester/covoar/DesiredSymbols.cc<br>
> index b9a5bb7..c97b25c 100644<br>
> --- a/tester/covoar/DesiredSymbols.cc<br>
> +++ b/tester/covoar/DesiredSymbols.cc<br>
> @@ -142,7 +142,7 @@ namespace Coverage {<br>
> CoverageMapBase* theCoverageMap = s.second.unifiedCoverageMap;<br>
> if (theCoverageMap)<br>
> {<br>
> - // Increment the total sizeInBytes byt the bytes in the symbol<br>
> + // Increment the total sizeInBytes by the bytes in the symbol<br>
> stats.sizeInBytes += s.second.stats.sizeInBytes;<br>
><br>
> // Now scan through the coverage map of this symbol.<br>
> @@ -202,6 +202,26 @@ namespace Coverage {<br>
> uint32_t count;<br>
><br>
> // Mark NOPs as executed<br>
> + a = s.second.stats.sizeInBytes - 1;<br>
> + count = 0;<br>
> + while (a > 0) {<br>
> + if (theCoverageMap->isStartOfInstruction( a )) {<br>
> + break;<br>
> + }<br>
> +<br>
> + count++;<br>
> +<br>
> + if (theCoverageMap->isNop( a )) {<br>
> + for (la = a; la < (a + count); la++) {<br>
> + theCoverageMap->setWasExecuted( la );<br>
> + }<br>
> +<br>
> + count = 0;<br>
> + }<br>
> +<br>
> + a--;<br>
> + }<br>
> +<br>
> endAddress = s.second.stats.sizeInBytes - 1;<br>
> a = 0;<br>
> while (a < endAddress) {<br>
> @@ -223,12 +243,13 @@ namespace Coverage {<br>
> ha++;<br>
> if ( ha >= endAddress )<br>
> break;<br>
> - } while ( !theCoverageMap->isStartOfInstruction( ha ) );<br>
> + } while ( !theCoverageMap->isStartOfInstruction( ha ) ||<br>
> + theCoverageMap->isNop( ha ) );<br>
> a = ha;<br>
> }<br>
><br>
> // Now scan through the coverage map of this symbol.<br>
> - endAddress = s.second.stats.sizeInBytes - 1;<br>
> + endAddress = s.second.stats.sizeInBytesWithoutNops - 1;<br>
> a = 0;<br>
> while (a <= endAddress) {<br>
> // If an address was NOT executed, find consecutive unexecuted<br>
> @@ -316,7 +337,8 @@ namespace Coverage {<br>
> void DesiredSymbols::createCoverageMap(<br>
> const std::string& exefileName,<br>
> const std::string& symbolName,<br>
> - uint32_t size<br>
> + uint32_t size,<br>
> + uint32_t sizeWithoutNops<br>
> )<br>
> {<br>
> CoverageMapBase* aCoverageMap;<br>
> @@ -354,9 +376,10 @@ namespace Coverage {<br>
> << '/' << size << ')'<br>
> << std::endl;<br>
><br>
> - if ( itr->second.stats.sizeInBytes < size )<br>
> + if ( itr->second.stats.sizeInBytes < size ) {<br>
> itr->second.stats.sizeInBytes = size;<br>
> - else<br>
> + itr->second.stats.sizeInBytesWithoutNops = sizeWithoutNops;<br>
> + } else<br>
> size = itr->second.stats.sizeInBytes;<br>
> }<br>
> }<br>
> @@ -376,6 +399,7 @@ namespace Coverage {<br>
> );<br>
> itr->second.unifiedCoverageMap = aCoverageMap;<br>
> itr->second.stats.sizeInBytes = size;<br>
> + itr->second.stats.sizeInBytesWithoutNops = sizeWithoutNops;<br>
> }<br>
> }<br>
><br>
> @@ -479,7 +503,7 @@ namespace Coverage {<br>
> // are the same size.<br>
> // Changed from ERROR msg to INFO, because size mismatch is not<br>
> // treated as error anymore. 2015-07-20<br>
> - uint32_t dMapSize = sinfo.stats.sizeInBytes;<br>
> + uint32_t dMapSize = sinfo.stats.sizeInBytesWithoutNops;<br>
> uint32_t sBaseAddress = sourceCoverageMap->getFirstLowAddress();<br>
> uint32_t sMapSize = sourceCoverageMap->getSize();<br>
> if (dMapSize != 0 && dMapSize != sMapSize) {<br>
> diff --git a/tester/covoar/DesiredSymbols.h b/tester/covoar/DesiredSymbols.h<br>
> index 5c45af8..367ca95 100644<br>
> --- a/tester/covoar/DesiredSymbols.h<br>
> +++ b/tester/covoar/DesiredSymbols.h<br>
> @@ -57,7 +57,12 @@ namespace Coverage {<br>
> uint32_t sizeInBytes;<br>
><br>
> /*!<br>
> - * This member contains the size in Bytes.<br>
> + * This member contains the size in Bytes not accounting for NOPs.<br>
> + */<br>
> + uint32_t sizeInBytesWithoutNops;<br>
> +<br>
> + /*!<br>
> + * This member contains the size in instructions.<br>
> */<br>
> uint32_t sizeInInstructions;<br>
><br>
> @@ -100,6 +105,7 @@ namespace Coverage {<br>
> branchesNeverTaken(0),<br>
> branchesNotExecuted(0),<br>
> sizeInBytes(0),<br>
> + sizeInBytesWithoutNops(0),<br>
> sizeInInstructions(0),<br>
> uncoveredBytes(0),<br>
> uncoveredInstructions(0),<br>
> @@ -227,7 +233,8 @@ namespace Coverage {<br>
> void createCoverageMap(<br>
> const std::string& exefileName,<br>
> const std::string& symbolName,<br>
> - uint32_t size<br>
> + uint32_t size,<br>
> + uint32_t sizeWithoutNops<br>
> );<br>
><br>
> /*!<br>
> diff --git a/tester/covoar/ExecutableInfo.cc b/tester/covoar/ExecutableInfo.cc<br>
> index c593e1d..ddd2987 100644<br>
> --- a/tester/covoar/ExecutableInfo.cc<br>
> +++ b/tester/covoar/ExecutableInfo.cc<br>
> @@ -127,7 +127,7 @@ namespace Coverage {<br>
> {<br>
> CoverageMaps::iterator cmi = coverageMaps.find( symbolName );<br>
> if ( cmi == coverageMaps.end() )<br>
> - throw rld::error (symbolName, "ExecutableInfo::findCoverageMap");<br>
> + throw CoverageMapNotFoundError(symbolName);<br>
> return *(cmi->second);<br>
> }<br>
><br>
> diff --git a/tester/covoar/ExecutableInfo.h b/tester/covoar/ExecutableInfo.h<br>
> index dcb4dcb..ab74b7f 100644<br>
> --- a/tester/covoar/ExecutableInfo.h<br>
> +++ b/tester/covoar/ExecutableInfo.h<br>
> @@ -29,6 +29,10 @@ namespace Coverage {<br>
><br>
> public:<br>
><br>
> + class CoverageMapNotFoundError : public std::runtime_error {<br>
> + using std::runtime_error::runtime_error;<br>
> + };<br>
> +<br>
> /*!<br>
> * This method constructs an ExecutableInfo instance.<br>
> *<br>
> diff --git a/tester/covoar/ObjdumpProcessor.cc b/tester/covoar/ObjdumpProcessor.cc<br>
> index 56ee219..0647ff9 100644<br>
> --- a/tester/covoar/ObjdumpProcessor.cc<br>
> +++ b/tester/covoar/ObjdumpProcessor.cc<br>
> @@ -32,35 +32,92 @@ namespace Coverage {<br>
> ObjdumpProcessor::objdumpLines_t instructions<br>
> ) {<br>
> // Find the symbol's coverage map.<br>
> - CoverageMapBase& coverageMap = executableInfo->findCoverageMap( symbolName );<br>
> -<br>
> - uint32_t lowAddress = coverageMap.getFirstLowAddress();<br>
> - uint32_t size = coverageMap.getSize();<br>
> - uint32_t highAddress = lowAddress + size - 1;<br>
> -<br>
> - // If there are NOT already saved instructions, save them.<br>
> - SymbolInformation* symbolInfo = SymbolsToAnalyze->find( symbolName );<br>
> - if (symbolInfo->instructions.empty()) {<br>
> - symbolInfo->sourceFile = executableInfo;<br>
> - symbolInfo->baseAddress = lowAddress;<br>
> - symbolInfo->instructions = instructions;<br>
> - }<br>
> + try {<br>
> + CoverageMapBase& coverageMap = executableInfo->findCoverageMap(symbolName);<br>
><br>
> - // Add the symbol to this executable's symbol table.<br>
> - SymbolTable* theSymbolTable = executableInfo->getSymbolTable();<br>
> - theSymbolTable->addSymbol(<br>
> - symbolName, lowAddress, highAddress - lowAddress + 1<br>
> - );<br>
> + uint32_t firstInstructionAddress = UINT32_MAX;<br>
><br>
> - // Mark the start of each instruction in the coverage map.<br>
> - for (auto& instruction : instructions) {<br>
> - coverageMap.setIsStartOfInstruction( instruction.address );<br>
> - }<br>
> + // Find the address of the first instruction.<br>
> + for (auto& line : instructions) {<br>
> + if (!line.isInstruction) {<br>
> + continue;<br>
> + }<br>
> +<br>
> + firstInstructionAddress = line.address;<br>
> + break;<br>
> + }<br>
This is a poorly written loop. Anytime you use continue/break to<br>
control the loop iteration, something is going awry. You can write<br>
this code equivalently without any continue or break.<br></blockquote><div><br></div><div>This loop is very deliberately constructed not to be a for loop because</div><div>you need the ability to break at different points to debug issues. Alex</div><div>and I spent hours in this loop debugging which I think would have</div><div>been more difficult to debug if all the conditions were in a for loop.</div><div>This loop was written to explicitly have breakpoints for various </div><div>conditions. </div><div><br></div><div>There have been multiple issues with the information reported with</div><div>size/length being at the top of that list. Before Alex found the source </div><div>of that (not covoar), it resulted in an infinite loop.</div><div><br></div><div>I suppose it can be changed to a for loop as purer academically but</div><div>it loses significantly on ability to set breakpoints and debug if there</div><div>ever is an issue again. </div><div><br></div><div>Are you willing to make that trade?</div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
> +<br>
> + if (firstInstructionAddress == UINT32_MAX) {<br>
> + std::ostringstream what;<br>
> + what << "Could not find first instruction address for symbol "<br>
> + << symbolName << " in " << executableInfo->getFileName();<br>
> + throw rld::error( what, "Coverage::finalizeSymbol" );<br>
> + }<br>
> +<br>
> + int rangeIndex;<br>
> + uint32_t lowAddress = UINT32_MAX;<br>
> + for (rangeIndex = 0; ; rangeIndex++) {<br>
> + lowAddress = coverageMap.getLowAddressOfRange(rangeIndex);<br>
> +<br>
> + if (firstInstructionAddress == lowAddress) {<br>
> + break;<br>
ditto... if you're goin to break the loop on this condition, why not<br>
use this in your for loop?<br>
for (rangeIndex = 0; firstInstructionAddress != lowAddress ; rangeIndex++) {<br>
<br>
> + }<br>
> + }<br>
><br>
> - // Create a unified coverage map for the symbol.<br>
> - SymbolsToAnalyze->createCoverageMap(<br>
> - executableInfo->getFileName().c_str(), symbolName, size<br>
> - );<br>
> + uint32_t sizeWithoutNops = coverageMap.getSizeOfRange(rangeIndex);<br>
> + uint32_t size = sizeWithoutNops;<br>
> + uint32_t highAddress = lowAddress + size - 1;<br>
> + uint32_t computedHighAddress = highAddress;<br>
> +<br>
> + // Find the high address as reported by the address of the last NOP<br>
> + // instruction. This ensures that NOPs get marked as executed later.<br>
> + for (auto instruction = instructions.rbegin();<br>
> + instruction != instructions.rend();<br>
> + instruction++) {<br>
> + if (instruction->isInstruction) {<br>
> + if (instruction->isNop) {<br>
> + computedHighAddress = instruction->address + instruction->nopSize;<br>
> + }<br>
> + break;<br>
Here too.<br>
<br>
> + }<br>
> + }<br>
> +<br>
> + if (highAddress != computedHighAddress) {<br>
> + std::cerr << "Function's high address differs between DWARF and objdump: "<br>
> + << symbolName << " (0x" << std::hex << highAddress << " and 0x"<br>
> + << computedHighAddress - 1 << ")" << std::dec << std::endl;<br>
> + size = computedHighAddress - lowAddress;<br>
> + }<br>
> +<br>
> + // If there are NOT already saved instructions, save them.<br>
> + SymbolInformation* symbolInfo = SymbolsToAnalyze->find( symbolName );<br>
> + if (symbolInfo->instructions.empty()) {<br>
> + symbolInfo->sourceFile = executableInfo;<br>
> + symbolInfo->baseAddress = lowAddress;<br>
> + symbolInfo->instructions = instructions;<br>
> + }<br>
> +<br>
> + // Add the symbol to this executable's symbol table.<br>
> + SymbolTable* theSymbolTable = executableInfo->getSymbolTable();<br>
> + theSymbolTable->addSymbol(<br>
> + symbolName, lowAddress, highAddress - lowAddress + 1<br>
> + );<br>
> +<br>
> + // Mark the start of each instruction in the coverage map.<br>
> + for (auto& instruction : instructions) {<br>
> + coverageMap.setIsStartOfInstruction( instruction.address );<br>
> + }<br>
> +<br>
> + // Create a unified coverage map for the symbol.<br>
> + SymbolsToAnalyze->createCoverageMap(<br>
> + executableInfo->getFileName().c_str(), symbolName, size, sizeWithoutNops<br>
> + );<br>
> + } catch (const ExecutableInfo::CoverageMapNotFoundError& e) {<br>
> + // Allow execution to continue even if a coverage map could not be<br>
> + // found.<br>
> + std::cerr << "Coverage map not found for symbol " << e.what()<br>
> + << std::endl;<br>
> + }<br>
> }<br>
><br>
> ObjdumpProcessor::ObjdumpProcessor()<br>
> --<br>
> 2.27.0<br>
><br>
> _______________________________________________<br>
> devel mailing list<br>
> <a href="mailto:devel@rtems.org" target="_blank">devel@rtems.org</a><br>
> <a href="http://lists.rtems.org/mailman/listinfo/devel" rel="noreferrer" target="_blank">http://lists.rtems.org/mailman/listinfo/devel</a><br>
_______________________________________________<br>
devel mailing list<br>
<a href="mailto:devel@rtems.org" target="_blank">devel@rtems.org</a><br>
<a href="http://lists.rtems.org/mailman/listinfo/devel" rel="noreferrer" target="_blank">http://lists.rtems.org/mailman/listinfo/devel</a><br>
</blockquote></div></div>