[PATCH 12/22] covoar: Fix NOP execution marking
Joel Sherrill
joel at rtems.org
Tue Mar 2 16:38:16 UTC 2021
On Tue, Mar 2, 2021 at 10:23 AM Gedare Bloom <gedare at rtems.org> wrote:
> 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
>
Yes. But harder to debug when something goes wrong. We spent a
surprising amount of time in this loop. :(
>
> > + 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?
>
There could be problems on a 64-bit target if the address space for the
program doesn't fall within the first 4GB. On the aarch64, even the
Couverture traces still output 32-bit addresses. We were surprised.
Change -1 to the max constant for uint32_t. which should be UINT32_MAX
Add changing uint32_t to a private target address type to the list
of things to do and then we can look at making it 64-bit easier.
I have a bad feeling this will be a sweep until itself which shouldn't
be mixed with this set.
>
> >
> > - // 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.
>
Alex .. Add this to the list of todo's. Not worth holding this up for.
We need a baseline working version to make improvements to. It had
bitrotted quite considerably.
> > +
> > + if (highAddress != computedHighAddress) {
> > + fprintf(
> why not using C++ way of printing?
>
Originally covoar used C IO exclusively. Apparently all hasn't been swept
out.
Go ahead and change this.
>
> > + 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
> _______________________________________________
> devel mailing list
> devel at rtems.org
> http://lists.rtems.org/mailman/listinfo/devel
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.rtems.org/pipermail/devel/attachments/20210302/6453581c/attachment-0001.html>
More information about the devel
mailing list