[PATCH v1 09/13] Remove SymbolsToAnalyze global variable
Ryan Long
ryan.long at oarcorp.com
Mon Aug 2 14:54:27 UTC 2021
On master, the struct is defined as objdumpLine_t. I'll go back and remove the "_t" from the end of the variables in another patch since that's unrelated to this one, and I'll remove that typedef.
-----Original Message-----
From: Chris Johns <chrisj at rtems.org>
Sent: Sunday, August 1, 2021 10:23 PM
To: joel at rtems.org
Cc: Ryan Long <ryan.long at oarcorp.com>; rtems-devel at rtems.org <devel at rtems.org>
Subject: Re: [PATCH v1 09/13] Remove SymbolsToAnalyze global variable
On 2/8/21 1:20 pm, Joel Sherrill wrote:
>
>
> On Sun, Aug 1, 2021, 10:11 PM Chris Johns <chrisj at rtems.org
> <mailto:chrisj at rtems.org>> wrote:
>
> On 2/8/21 9:23 am, Ryan Long wrote:
> > - Removed SymbolsToAnalyze from app_common and replaced it with the
> > symbolsToAnalyze_m member variable in DesiredSymbols, GcovData, and
> > ObjdumpProcessor
> > - Added a parameter to constructors to initialize symbolsToAnalyze_m
> > - Moved the definition of objdumpLine_t out of ObjdumpProcessor to make
> > it work with DesiredSymbols
> > ---
> > tester/covoar/DesiredSymbols.cc | 12 +++--
> > tester/covoar/DesiredSymbols.h | 17 +++++--
> > tester/covoar/ExecutableInfo.cc | 14 +++---
> > tester/covoar/ExecutableInfo.h | 14 +++++-
> > tester/covoar/GcovData.cc | 7 +--
> > tester/covoar/GcovData.h | 12 ++++-
> > tester/covoar/GcovFunctionData.cc | 14 ++++--
> > tester/covoar/GcovFunctionData.h | 6 ++-
> > tester/covoar/ObjdumpProcessor.cc | 22 +++++----
> > tester/covoar/ObjdumpProcessor.h | 90 +++++++++++++++++++----------------
> > tester/covoar/ReportsBase.cc | 98
> +++++++++++++++++++++------------------
> > tester/covoar/ReportsBase.h | 33 +++++++++----
> > tester/covoar/ReportsHtml.cc | 16 ++++---
> > tester/covoar/ReportsHtml.h | 11 +++--
> > tester/covoar/ReportsText.cc | 8 ++--
> > tester/covoar/ReportsText.h | 3 +-
> > tester/covoar/TraceConverter.cc | 17 +++++--
> > tester/covoar/app_common.cc | 1 -
> > tester/covoar/app_common.h | 1 -
> > tester/covoar/covoar.cc | 40 +++++++++-------
> > 20 files changed, 271 insertions(+), 165 deletions(-)
> >
> > diff --git a/tester/covoar/DesiredSymbols.cc b/tester/covoar/DesiredSymbols.cc
> > index 5d5e637..d76c5af 100644
> > --- a/tester/covoar/DesiredSymbols.cc
> > +++ b/tester/covoar/DesiredSymbols.cc
> > @@ -25,7 +25,6 @@
> > #include "DesiredSymbols.h"
> > #include "app_common.h"
> > #include "CoverageMap.h"
> > -#include "ObjdumpProcessor.h"
> >
> > namespace Coverage {
> >
> > @@ -116,10 +115,10 @@ namespace Coverage {
> > }
> > }
> >
> > - void DesiredSymbols::preprocess( void )
> > + void DesiredSymbols::preprocess( const DesiredSymbols& symbolsToAnalyze )
> > {
> > // Look at each symbol.
> > - for (auto& s : SymbolsToAnalyze->set) {
> > + for (auto& s : symbolsToAnalyze.set) {
> > // If the unified coverage map does not exist, the symbol was
> > // never referenced by any executable. Just skip it.
> > CoverageMapBase* theCoverageMap = s.second.unifiedCoverageMap;
> > @@ -441,10 +440,13 @@ namespace Coverage {
> > return &set[ symbolName ];
> > }
> >
> > - void DesiredSymbols::findSourceForUncovered( bool verbose )
> > + void DesiredSymbols::findSourceForUncovered(
> > + bool verbose,
> > + const DesiredSymbols& symbolsToAnalyze
> > + )
> > {
> > // Process uncovered ranges and/or branches for each symbol.
> > - for (auto& d : SymbolsToAnalyze->set) {
> > + for (auto& d : symbolsToAnalyze.set) {
> > // First the unexecuted ranges, ...
> > CoverageRanges* theRanges = d.second.uncoveredRanges;
> > if (theRanges != nullptr) {
> > diff --git a/tester/covoar/DesiredSymbols.h b/tester/covoar/DesiredSymbols.h
> > index 791b4ea..70bc39b 100644
> > --- a/tester/covoar/DesiredSymbols.h
> > +++ b/tester/covoar/DesiredSymbols.h
> > @@ -19,6 +19,11 @@
> >
> > namespace Coverage {
> >
> > + class ObjdumpProcessor;
> > + struct objdumpLine;
> > + using objdumpLine_t = objdumpLine;
> > + class ExecutableInfo;
> > +
> >
> > /*!
> > *
> > @@ -139,7 +144,7 @@ namespace Coverage {
> > /*!
> > * This member contains the disassembly associated with a symbol.
> > */
> > - std::list<ObjdumpProcessor::objdumpLine_t> instructions;
> > + std::list<objdumpLine_t> instructions;
> >
> > /*!
> > * This member contains the executable that was used to
> > @@ -263,8 +268,12 @@ namespace Coverage {
> > * uncovered ranges or branches.
> > *
> > * @param[in] verbose specifies whether to be verbose with output
> > + * @param[in] symbolsToAnalyze the symbols to be analyzed
> > */
> > - void findSourceForUncovered( bool verbose );
> > + void findSourceForUncovered(
> > + bool verbose,
> > + const DesiredSymbols& symbolsToAnalyze
> > + );
> >
> > /*!
> > * This method returns the total number of branches always taken
> > @@ -398,8 +407,10 @@ namespace Coverage {
> > /*!
> > * This method preprocesses each symbol's coverage map to mark nop
> > * and branch information.
> > + *
> > + * @param[in] symbolsToAnalyze the symbols to be analyzed
> > */
> > - void preprocess( void );
> > + void preprocess( const DesiredSymbols& symbolsToAnalyze );
> >
> > private:
> >
> > diff --git a/tester/covoar/ExecutableInfo.cc b/tester/covoar/ExecutableInfo.cc
> > index 9c3031e..328f970 100644
> > --- a/tester/covoar/ExecutableInfo.cc
> > +++ b/tester/covoar/ExecutableInfo.cc
> > @@ -10,9 +10,9 @@
> > #include <rld.h>
> >
> > #include "ExecutableInfo.h"
> > +#include "ObjdumpProcessor.h"
> > #include "app_common.h"
> > #include "CoverageMap.h"
> > -#include "DesiredSymbols.h"
> > #include "SymbolTable.h"
> >
> > namespace Coverage {
> > @@ -20,9 +20,11 @@ namespace Coverage {
> > ExecutableInfo::ExecutableInfo(
> > const char* const theExecutableName,
> > const std::string& theLibraryName,
> > - bool verbose
> > + bool verbose,
> > + DesiredSymbols& symbolsToAnalyze
> > ) : fileName(theExecutableName),
> > - loadAddress(0)
> > + loadAddress(0),
> > + symbolsToAnalyze_m(symbolsToAnalyze)
> > {
> > if ( !theLibraryName.empty() )
> > libraryName = theLibraryName;
> > @@ -59,7 +61,7 @@ namespace Coverage {
> > continue;
> > }
> >
> > - if (!SymbolsToAnalyze->isDesired(func.name <http://func.name>())) {
> > + if (!symbolsToAnalyze_m.isDesired(func.name <http://func.name>())) {
> > continue;
> > }
> >
> > @@ -209,8 +211,8 @@ namespace Coverage {
> >
> > void ExecutableInfo::mergeCoverage( void ) {
> > for (auto& cm : coverageMaps) {
> > - if (SymbolsToAnalyze->isDesired( cm.first ))
> > - SymbolsToAnalyze->mergeCoverageMap( cm.first, cm.second );
> > + if (symbolsToAnalyze_m.isDesired( cm.first ))
> > + symbolsToAnalyze_m.mergeCoverageMap( cm.first, cm.second );
> > }
> > }
> >
> > diff --git a/tester/covoar/ExecutableInfo.h b/tester/covoar/ExecutableInfo.h
> > index 851a59d..0adebcb 100644
> > --- a/tester/covoar/ExecutableInfo.h
> > +++ b/tester/covoar/ExecutableInfo.h
> > @@ -18,9 +18,12 @@
> > #include "AddressToLineMapper.h"
> > #include "CoverageMapBase.h"
> > #include "SymbolTable.h"
> > +#include "DesiredSymbols.h"
> >
> > namespace Coverage {
> >
> > +class DesiredSymbols;
> > +
> > /*! @class ExecutableInfo
> > *
> > * This class holds a collection of information for an executable
> > @@ -41,11 +44,13 @@ namespace Coverage {
> > * @param[in] theExecutableName specifies the name of the executable
> > * @param[in] theLibraryName specifies the name of the executable
> > * @param[in] verbose specifies whether to be verbose with output
> > + * @param[in] symbolsToAnalyze the symbols to be analyzed
> > */
> > ExecutableInfo(
> > const char* const theExecutableName,
> > - const std::string& theLibraryName = "",
> > - bool verbose = false
> > + const std::string& theLibraryName,
> > + bool verbose,
> > + DesiredSymbols& symbolsToAnalyze
> > );
> >
> > /*!
> > @@ -198,6 +203,11 @@ namespace Coverage {
> > */
> > SymbolTable theSymbolTable;
> >
> > + /*!
> > + * This member variable contains the symbols to be analyzed.
> > + */
> > + DesiredSymbols& symbolsToAnalyze_m;
> > +
> > };
> > }
> > #endif
> > diff --git a/tester/covoar/GcovData.cc b/tester/covoar/GcovData.cc
> > index e8b8573..11c26ce 100644
> > --- a/tester/covoar/GcovData.cc
> > +++ b/tester/covoar/GcovData.cc
> > @@ -25,9 +25,10 @@
> >
> > namespace Gcov {
> >
> > - GcovData::GcovData()
> > + GcovData::GcovData( Coverage::DesiredSymbols& symbolsToAnalyze ):
> > + numberOfFunctions( 0 ),
> > + symbolsToAnalyze_m( symbolsToAnalyze )
> > {
> > - numberOfFunctions = 0;
> > }
> >
> > GcovData::~GcovData()
> > @@ -416,7 +417,7 @@ namespace Gcov {
> > function->setChecksum( intBuffer[1] );
> >
> > header.length -= readString( buffer, gcovFile );
> > - function->setFunctionName( buffer );
> > + function->setFunctionName( buffer, symbolsToAnalyze_m );
> > header.length -= readString( buffer, gcovFile );
> > function->setFileName( buffer );
> > status = fread( &intBuffer, 4, header.length, gcovFile );
> > diff --git a/tester/covoar/GcovData.h b/tester/covoar/GcovData.h
> > index 0e74b02..b00b27a 100644
> > --- a/tester/covoar/GcovData.h
> > +++ b/tester/covoar/GcovData.h
> > @@ -11,6 +11,7 @@
> > #include <list>
> > #include <iostream>
> > #include "GcovFunctionData.h"
> > +#include "DesiredSymbols.h"
> >
> > namespace Gcov {
> >
> > @@ -56,6 +57,8 @@ struct gcov_statistics
> > uint64_t sumMax; // sum of individual runs max values
> > };
> >
> > +class DesiredSymbols;
> > +
> > /*! @class GcovData
> > *
> > * This is the specification of the GcovData class.
> > @@ -66,8 +69,10 @@ struct gcov_statistics
> >
> > /*!
> > * This method constructs a GcnoReader instance.
> > + *
> > + * @param[in] symbolsToAnalyze the symbols to be analyzed
> > */
> > - GcovData();
> > + GcovData( Coverage::DesiredSymbols& symbolsToAnalyze );
> >
> > /*!
> > * This method destructs a GcnoReader instance.
> > @@ -194,6 +199,11 @@ struct gcov_statistics
> > * to a specified report file
> > */
> > void printGcnoFileInfo( FILE * textFile );
> > +
> > + /*!
> > + * This member variable contains the symbols to be analyzed
> > + */
> > + Coverage::DesiredSymbols& symbolsToAnalyze_m;
> > };
> > }
> > #endif
> > diff --git a/tester/covoar/GcovFunctionData.cc
> b/tester/covoar/GcovFunctionData.cc
> > index 90b1be0..1767f18 100644
> > --- a/tester/covoar/GcovFunctionData.cc
> > +++ b/tester/covoar/GcovFunctionData.cc
> > @@ -13,6 +13,7 @@
> > #include "GcovFunctionData.h"
> > #include "ObjdumpProcessor.h"
> > #include "CoverageMapBase.h"
> > +#include "DesiredSymbols.h"
> >
> >
> > namespace Gcov {
> > @@ -44,7 +45,10 @@ namespace Gcov {
> > firstLineNumber = lineNo;
> > }
> >
> > - bool GcovFunctionData::setFunctionName( const char* fcnName )
> > + bool GcovFunctionData::setFunctionName(
> > + const char* fcnName,
> > + Coverage::DesiredSymbols& symbolsToAnalyze
> > + )
> > {
> > std::string symbolName;
> >
> > @@ -62,7 +66,7 @@ namespace Gcov {
> > strcpy (functionName, fcnName);
> >
> > // Tie function to its coverage map
> > - symbolInfo = SymbolsToAnalyze->find( symbolName );
> > + symbolInfo = symbolsToAnalyze.find( symbolName );
> > if ( symbolInfo != NULL )
> > coverageMap = symbolInfo->unifiedCoverageMap;
> >
> > @@ -237,7 +241,7 @@ namespace Gcov {
> > uint32_t baseAddress = 0;
> > uint32_t baseSize;
> > uint32_t currentAddress;
> > - std::list<Coverage::ObjdumpProcessor::objdumpLine_t>::iterator
> instruction;
> > + std::list<Coverage::objdumpLine_t>::iterator instruction;
> >
> > if ( coverageMap != NULL ) {
> >
> > @@ -399,7 +403,7 @@ namespace Gcov {
> >
> > uint32_t baseAddress = 0;
> > uint32_t currentAddress = 0;
> > - std::list<Coverage::ObjdumpProcessor::objdumpLine_t>::iterator
> instruction;
> > + std::list<Coverage::objdumpLine_t>::iterator instruction;
> > blocks_iterator_t blockIterator;
> > blocks_iterator_t blockIterator2;
> > arcs_iterator_t arcIterator;
> > @@ -567,7 +571,7 @@ namespace Gcov {
> > {
> > uint32_t baseAddress = 0;
> > uint32_t currentAddress;
> > - std::list<Coverage::ObjdumpProcessor::objdumpLine_t>::iterator
> instruction;
> > + std::list<Coverage::objdumpLine_t>::iterator instruction;
> >
> > if ( coverageMap == NULL )
> > return false;
> > diff --git a/tester/covoar/GcovFunctionData.h
> b/tester/covoar/GcovFunctionData.h
> > index 812b45c..7d87b55 100644
> > --- a/tester/covoar/GcovFunctionData.h
> > +++ b/tester/covoar/GcovFunctionData.h
> > @@ -44,6 +44,8 @@ typedef std::list<gcov_arc_info>::iterator
> arcs_iterator_t;
> > typedef std::list<gcov_block_info> blocks_t;
> > typedef std::list<gcov_block_info>::iterator blocks_iterator_t;
> >
> > +class DesiredSymbols;
> > +
> > /*! @class GcovFunctionData
> > *
> > * This is the specification of the GcovFunctionData class.
> > @@ -94,11 +96,13 @@ typedef std::list<gcov_block_info>::iterator
> blocks_iterator_t;
> > * unified coverage map.
> > *
> > * @param[in] functionName passes name of the the function
> > + * @param[in] symbolsToAnalyze the symbols to be analyzed
> > *
> > * @return Returns TRUE if the method succeeded and FALSE if it failed.
> > */
> > bool setFunctionName(
> > - const char* fcnName
> > + const char* fcnName,
> > + Coverage::DesiredSymbols& symbolsToAnalyze
> > );
> >
> > /*!
> > diff --git a/tester/covoar/ObjdumpProcessor.cc
> b/tester/covoar/ObjdumpProcessor.cc
> > index d324440..f590ece 100644
> > --- a/tester/covoar/ObjdumpProcessor.cc
> > +++ b/tester/covoar/ObjdumpProcessor.cc
> > @@ -32,7 +32,8 @@ namespace Coverage {
> > ExecutableInfo* const executableInfo,
> > std::string& symbolName,
> > ObjdumpProcessor::objdumpLines_t instructions,
> > - bool verbose
> > + bool verbose,
> > + DesiredSymbols& symbolsToAnalyze
> > ) {
> > // Find the symbol's coverage map.
> > try {
> > @@ -88,7 +89,7 @@ namespace Coverage {
> > }
> >
> > // If there are NOT already saved instructions, save them.
> > - SymbolInformation* symbolInfo = SymbolsToAnalyze->find( symbolName );
> > + SymbolInformation* symbolInfo = symbolsToAnalyze.find( symbolName );
> > if (symbolInfo->instructions.empty()) {
> > symbolInfo->sourceFile = executableInfo;
> > symbolInfo->baseAddress = lowAddress;
> > @@ -107,7 +108,7 @@ namespace Coverage {
> > }
> >
> > // Create a unified coverage map for the symbol.
> > - SymbolsToAnalyze->createCoverageMap(
> > + symbolsToAnalyze.createCoverageMap(
> > executableInfo->getFileName().c_str(),
> > symbolName,
> > size,
> > @@ -122,7 +123,9 @@ namespace Coverage {
> > }
> > }
> >
> > - ObjdumpProcessor::ObjdumpProcessor()
> > + ObjdumpProcessor::ObjdumpProcessor(
> > + DesiredSymbols& symbolsToAnalyze
> > + ): symbolsToAnalyze_m( symbolsToAnalyze )
> > {
> > }
> >
> > @@ -363,7 +366,8 @@ namespace Coverage {
> > executableInformation,
> > currentSymbol,
> > theInstructions,
> > - verbose
> > + verbose,
> > + symbolsToAnalyze_m
> > );
> > fprintf(
> > stderr,
> > @@ -419,7 +423,8 @@ namespace Coverage {
> > executableInformation,
> > currentSymbol,
> > theInstructions,
> > - verbose
> > + verbose,
> > + symbolsToAnalyze_m
> > );
> > }
> >
> > @@ -444,7 +449,7 @@ namespace Coverage {
> > }
> >
> > // See if the new symbol is one that we care about.
> > - if (SymbolsToAnalyze->isDesired( symbol )) {
> > + if (symbolsToAnalyze_m.isDesired( symbol )) {
> > currentSymbol = symbol;
> > processSymbol = true;
> > theInstructions.push_back( lineInfo );
> > @@ -462,7 +467,8 @@ namespace Coverage {
> > executableInformation,
> > currentSymbol,
> > theInstructions,
> > - verbose
> > + verbose,
> > + symbolsToAnalyze_m
> > );
> > }
> > processSymbol = false;
> > diff --git a/tester/covoar/ObjdumpProcessor.h
> b/tester/covoar/ObjdumpProcessor.h
> > index d60a768..c7fc8bb 100644
> > --- a/tester/covoar/ObjdumpProcessor.h
> > +++ b/tester/covoar/ObjdumpProcessor.h
> > @@ -12,11 +12,54 @@
> >
> > #include "ExecutableInfo.h"
> > #include "TargetBase.h"
> > +#include "DesiredSymbols.h"
> >
> > #include "rld-process.h"
> >
> > namespace Coverage {
> >
> > + class DesiredSymbols;
> > + class ExecutableInfo;
> > +
> > + /*!
> > + * This type defines the elements of an objdump line.
> > + */
> > + typedef struct objdumpLine {
>
> Typedef here? I thought it was automatic with C++?
>
> > + /*!
> > + * This member variable contains the actual line from the object dump.
> > + */
> > + std::string line;
> > +
> > + /*!
> > + * This member variable contains the address from the object dump line.
> > + */
> > + uint32_t address;
> > +
> > + /*!
> > + * This member variable contains an indication of whether the line
> > + * is an instruction.
> > + */
> > + bool isInstruction;
> > +
> > + /*!
> > + * This member variable contains an indication of whether the line
> > + * is a nop instruction.
> > + */
> > + bool isNop;
> > +
> > + /*!
> > + * This member variable contains the size of the nop instruction.
> > + */
> > + int nopSize;
> > +
> > + /*!
> > + * This member variable contains an indication of whether the line
> > + * is a branch instruction.
> > + */
> > + bool isBranch;
> > +
> > + } objdumpLine_t;
>
> Please do not append `_t`. I understand it is reserved for standards or
> something like that.
>
>
> POSIX reserves _t. C and C++ have more complicated rules that don't go this far.
> One of which is something like double leading underscores are reserved
> for the implementation.
>
> I'm guilty of violating the _t rule and use __ for header guards.
Guilty as well but reformed.
Chris
More information about the devel
mailing list