Re: [PATCH] covoar: Fix DWARF reading

[Chris Johns]

On 14/3/21 8:43 am, Joel Sherrill wrote:
> On Fri, Mar 12, 2021 at 5:47 PM Chris Johns  > wrote:
> 
> These are design question and not review issues 
> 
> On 12/3/21 5:33 am, Alex White wrote:
> > +  // Create data based on target.
> > +  TargetInfo = Target::TargetFactory( buildTarget );
> 
> Any pointers in this object given there is a copy operator at work here?
> 
> There shouldn't be.
> 
> > +  // Create the set of desired symbols.
> > +  SymbolsToAnalyze = new Coverage::DesiredSymbols();
> 
> How hard would it be to convert these types of allocations in covoar to
> std::shared_ptr<> or std::unique_ptr<> (if that is suitable or possible)?
> 
> We probably need to have a data flow discussion as part of evaluating this.

Sure happy too, but I currently only considering the code in front of me.

> When (if) this code is parallellized like Alex and I would love to see done, 
> the base information gathering will happen upfront and then the analysis
> and report generation can happen in separate threads for each symbol
> set. This reduces the objdump for sure. I thinkTargetinfo is a shared 
> read-only 
> class once created and all threads can share it.

I am wondering about using references or shared pointers and not using any
pointers to objects. This is not related to const or read only attributes.

> SymbolSet would become a thread local variable since a thread would be
> created per SymbolSet.

I would discourage TLS and do not support using it in this case. If you add
threading you can support thread bodies as methods of instances of a class and
that is a better model than TLS.

Chris
___
devel mailing list
devel@rtems.org
http://lists.rtems.org/mailman/listinfo/devel

Re: [PATCH] covoar: Fix DWARF reading

[Joel Sherrill]

On Fri, Mar 12, 2021 at 5:47 PM Chris Johns  wrote:

> These are design question and not review issues 
>
> On 12/3/21 5:33 am, Alex White wrote:
> > +  // Create data based on target.
> > +  TargetInfo = Target::TargetFactory( buildTarget );
>
> Any pointers in this object given there is a copy operator at work here?
>

There shouldn't be.


>
> > +  // Create the set of desired symbols.
> > +  SymbolsToAnalyze = new Coverage::DesiredSymbols();
>
> How hard would it be to convert these types of allocations in covoar to
> std::shared_ptr<> or std::unique_ptr<> (if that is suitable or possible)?
>

We probably need to have a data flow discussion as part of evaluating this.
When (if) this code is parallellized like Alex and I would love to see
done,
the base information gathering will happen upfront and then the analysis
and report generation can happen in separate threads for each symbol
set. This reduces the objdump for sure. I thinkTargetinfo is a shared
read-only
class once created and all threads can share it.

SymbolSet would become a thread local variable since a thread would be
created per SymbolSet.


>
> Chris
> ___
> 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

Re: [PATCH] covoar: Fix DWARF reading

[Chris Johns]

These are design question and not review issues 

On 12/3/21 5:33 am, Alex White wrote:
> +  // Create data based on target.
> +  TargetInfo = Target::TargetFactory( buildTarget );

Any pointers in this object given there is a copy operator at work here?

> +  // Create the set of desired symbols.
> +  SymbolsToAnalyze = new Coverage::DesiredSymbols();

How hard would it be to convert these types of allocations in covoar to
std::shared_ptr<> or std::unique_ptr<> (if that is suitable or possible)?

Chris
___
devel mailing list
devel@rtems.org
http://lists.rtems.org/mailman/listinfo/devel

[PATCH] covoar: Fix DWARF reading

[Alex White]

There were a couple of issues with the way the DWARF info was being
read. The first issue was that it inefficiently included all symbols,
even symbols that were not desired. The second issue is that it did
not handle inline functions correctly. These have been fixed.
---
 tester/covoar/ExecutableInfo.cc |  30 -
 tester/covoar/covoar.cc | 110 
 2 files changed, 82 insertions(+), 58 deletions(-)

diff --git a/tester/covoar/ExecutableInfo.cc b/tester/covoar/ExecutableInfo.cc
index c593e1d..c4257f0 100644
--- a/tester/covoar/ExecutableInfo.cc
+++ b/tester/covoar/ExecutableInfo.cc
@@ -45,10 +45,34 @@ namespace Coverage {
 try {
   for (auto& cu : debug.get_cus()) {
 for (auto& func : cu.get_functions()) {
-  if (func.has_machine_code() && (!func.is_inlined() || 
func.is_external())) {
-createCoverageMap (cu.name(), func.name(),
-   func.pc_low(), func.pc_high() - 1);
+  if (!func.has_machine_code()) {
+continue;
   }
+
+  if (!SymbolsToAnalyze->isDesired(func.name())) {
+continue;
+  }
+
+  if (func.is_inlined()) {
+if (func.is_external()) {
+  // Flag it
+  std::cerr << "Function is both external and inlined: "
+<< func.name() << std::endl;
+}
+
+if (func.has_entry_pc()) {
+  continue;
+}
+
+// If the low PC address is zero, the symbol does not appear in
+// this executable.
+if (func.pc_low() == 0) {
+  continue;
+}
+  }
+
+  createCoverageMap (cu.name(), func.name(),
+  func.pc_low(), func.pc_high() - 1);
 }
   }
 } catch (...) {
diff --git a/tester/covoar/covoar.cc b/tester/covoar/covoar.cc
index cbb0e4f..84d883a 100644
--- a/tester/covoar/covoar.cc
+++ b/tester/covoar/covoar.cc
@@ -222,6 +222,61 @@ int covoar(
   if ( !projectName )
 throw option_error( "project name -p" );
 
+  //
+  // Find the top of the BSP's build tree and if we have found the top
+  // check the executable is under the same path and BSP.
+  //
+  std::string buildPath;
+  std::string buildTarget;
+  std::string buildBSP;
+  createBuildPath(executablesToAnalyze,
+  buildPath,
+  buildTarget,
+  buildBSP);
+
+  //
+  // Use a command line target if provided.
+  //
+  if (!target.empty()) {
+buildTarget = target;
+  }
+
+  if (Verbose) {
+if (singleExecutable) {
+  std::cerr << "Processing a single executable and multiple coverage files"
+<< std::endl;
+} else {
+  std::cerr << "Processing multiple executable/coverage file pairs" << 
std::endl;
+}
+std::cerr << "Coverage Format : " << format << std::endl
+  << "Target  : " << buildTarget.c_str() << std::endl
+  << std::endl;
+
+// Process each executable/coverage file pair.
+Executables::iterator eitr = executablesToAnalyze.begin();
+for (const auto& cname : coverageFileNames) {
+  std::cerr << "Coverage file " << cname
+<< " for executable: " << (*eitr)->getFileName() << std::endl;
+  if (!singleExecutable)
+eitr++;
+}
+  }
+
+  //
+  // Create data to support analysis.
+  //
+
+  // Create data based on target.
+  TargetInfo = Target::TargetFactory( buildTarget );
+
+  // Create the set of desired symbols.
+  SymbolsToAnalyze = new Coverage::DesiredSymbols();
+
+  //
+  // Read symbol configuration file and load needed symbols.
+  //
+  SymbolsToAnalyze->load( symbolSet, buildTarget, buildBSP, Verbose );
+
   // If a single executable was specified, process the remaining
   // arguments as coverage file names.
   if (singleExecutable) {
@@ -294,61 +349,6 @@ int covoar(
   if (executablesToAnalyze.size() != coverageFileNames.size())
 throw rld::error( "executables and coverage name size mismatch", "covoar" 
);
 
-  //
-  // Find the top of the BSP's build tree and if we have found the top
-  // check the executable is under the same path and BSP.
-  //
-  std::string buildPath;
-  std::string buildTarget;
-  std::string buildBSP;
-  createBuildPath(executablesToAnalyze,
-  buildPath,
-  buildTarget,
-  buildBSP);
-
-  //
-  // Use a command line target if provided.
-  //
-  if (!target.empty()) {
-buildTarget = target;
-  }
-
-  if (Verbose) {
-if (singleExecutable) {
-  std::cerr << "Processing a single executable and multiple coverage files"
-<< std::endl;
-} else {
-  std::cerr << "Processing multiple executable/coverage file pairs" << 
std::endl;
-}
-std::cerr << "Coverage Format : " << format << std::endl
-  << "Target  : " << buildTarget.c_str() << std::endl
-  << std::endl;
-
-//