[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