On Thu, Mar 25, 2021 at 10:05 AM Joel Sherrill <j...@rtems.org> wrote:
>
>
>
> On Thu, Mar 25, 2021 at 10:42 AM Gedare Bloom <ged...@rtems.org> wrote:
>>
>> On Thu, Mar 18, 2021 at 12:05 PM Alex White <alex.wh...@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?
>
You know that what we debug is often much uglier than what we publish.
At least, simplify the logic to let the loop work for you.
for (auto& line : instructions) {
if ( line.isInstruction ) {
firstInstructionAddress = line.address;
break;
}
}
Using continue/break to control loops is antithetical to using loops,
and probably doesn't do the compiler any good.
>> > + if (!line.isInstruction) {
>> > + continue;
>> > + }
>> > +
>> > +
>> > + 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;
>> 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@rtems.org
>> > http://lists.rtems.org/mailman/listinfo/devel
>> _______________________________________________
>> devel mailing list
>> devel@rtems.org
>> http://lists.rtems.org/mailman/listinfo/devel
_______________________________________________
devel mailing list
devel@rtems.org
http://lists.rtems.org/mailman/listinfo/devel
