<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=us-ascii">
<style type="text/css" style="display:none;"> P {margin-top:0;margin-bottom:0;} </style>
</head>
<body dir="ltr">
<div style="font-family: Calibri, Arial, Helvetica, sans-serif; font-size: 12pt; color: rgb(0, 0, 0);">
On Tue, May 25, 2021 at 7:24 PM Chris Johns <chrisj@rtems.org> wrote:
<div>></div>
<div>></div>
<div>></div>
<div>> On 1/5/21 8:01 am, Alex White wrote:</div>
<div>> > This adds the AddressToLineMapper class and supporting classes to</div>
<div>> > assume responsibility of tracking address-to-line information.</div>
<div>> ></div>
<div>> > This allows the DWARF library to properly cleanup all of its resources</div>
<div>> > and leads to significant memory savings.</div>
<div>> ></div>
<div>> > Closes #4383</div>
<div>> > ---</div>
<div>> > rtemstoolkit/rld-dwarf.cpp | 5 +</div>
<div>> > rtemstoolkit/rld-dwarf.h | 5 +</div>
<div>> > tester/covoar/AddressToLineMapper.cc | 105 +++++++++++++++</div>
<div>> > tester/covoar/AddressToLineMapper.h | 193 +++++++++++++++++++++++++++</div>
<div>> > tester/covoar/ExecutableInfo.cc | 73 +++++-----</div>
<div>> > tester/covoar/ExecutableInfo.h | 11 +-</div>
<div>> > tester/covoar/wscript | 1 +</div>
<div>> > 7 files changed, 351 insertions(+), 42 deletions(-)</div>
<div>> > create mode 100644 tester/covoar/AddressToLineMapper.cc</div>
<div>> > create mode 100644 tester/covoar/AddressToLineMapper.h</div>
<div>> ></div>
<div>> > diff --git a/rtemstoolkit/rld-dwarf.cpp b/rtemstoolkit/rld-dwarf.cpp</div>
<div>> > index 2fce0e4..1eae50c 100644</div>
<div>> > --- a/rtemstoolkit/rld-dwarf.cpp</div>
<div>> > +++ b/rtemstoolkit/rld-dwarf.cpp</div>
<div>> > @@ -1893,6 +1893,11 @@ namespace rld</div>
<div>> > return false;</div>
<div>> > }</div>
<div>> > </div>
<div>> > + const addresses& compilation_unit::get_addresses () const</div>
<div>> > + {</div>
<div>> > + return addr_lines_;</div>
<div>> > + }</div>
<div>> > +</div>
<div>> > functions&</div>
<div>> > compilation_unit::get_functions ()</div>
<div>> > {</div>
<div>> > diff --git a/rtemstoolkit/rld-dwarf.h b/rtemstoolkit/rld-dwarf.h</div>
<div>> > index 1210813..eadb50d 100644</div>
<div>> > --- a/rtemstoolkit/rld-dwarf.h</div>
<div>> > +++ b/rtemstoolkit/rld-dwarf.h</div>
<div>> > @@ -707,6 +707,11 @@ namespace rld</div>
<div>> > */</div>
<div>> > unsigned int pc_high () const;</div>
<div>> > </div>
<div>> > + /**</div>
<div>> > + * The addresses associated with this compilation unit.</div>
<div>> > + */</div>
<div>> > + const addresses& get_addresses () const;</div>
<div>> > +</div>
<div>> > /**</div>
<div>> > * Get the source and line for an address. If the address does not match</div>
<div>> > * false is returned the file is set to 'unknown' and the line is set to</div>
<div>> > diff --git a/tester/covoar/AddressToLineMapper.cc b/tester/covoar/AddressToLineMapper.cc</div>
<div>> > new file mode 100644</div>
<div>> > index 0000000..1062fd7</div>
<div>> > --- /dev/null</div>
<div>> > +++ b/tester/covoar/AddressToLineMapper.cc</div>
<div>> > @@ -0,0 +1,105 @@</div>
<div>> > +/*! @file AddressToLineMapper.cc</div>
<div>> > + * @brief AddressToLineMapper Implementation</div>
<div>> > + *</div>
<div>> > + * This file contains the implementation of the functionality</div>
<div>> > + * of the AddressToLineMapper class.</div>
<div>> > + */</div>
<div>> > +</div>
<div>> > +#include "AddressToLineMapper.h"</div>
<div>> > +</div>
<div>> > +namespace Coverage {</div>
<div>> > +</div>
<div>> > + uint64_t SourceLine::location() const</div>
<div>> > + {</div>
<div>> > + return address;</div>
<div>> > + }</div>
<div>> > +</div>
<div>> > + bool SourceLine::is_an_end_sequence() const</div>
<div>> > + {</div>
<div>> > + return is_end_sequence;</div>
<div>> > + }</div>
<div>> > +</div>
<div>> > + std::string SourceLine::path() const</div>
<div>> > + {</div>
<div>> > + return *sourceIterator;</div>
<div>> > + }</div>
<div>> > +</div>
<div>> > + uint64_t SourceLine::line() const</div>
<div>> > + {</div>
<div>> > + return line_num;</div>
<div>> > + }</div>
<div>> > +</div>
<div>> > + void AddressLineRange::addSourceLine(const rld::dwarf::address& address)</div>
<div>> > + {</div>
<div>> > + auto insertResult = sourcePaths.insert(address.path());</div>
<div>> > +</div>
<div>> > + sourceLines.emplace_back(</div>
<div>> > + SourceLine (</div>
<div>> > + address.location(),</div>
<div>> > + insertResult.first,</div>
<div>> > + address.line(),</div>
<div>> > + address.is_an_end_sequence()</div>
<div>> > + )</div>
<div>> > + );</div>
<div>> > + }</div>
<div>> > +</div>
<div>> > + const SourceLine* AddressLineRange::getSourceLine(uint32_t address) const</div>
<div>> > + {</div>
<div>> > + if (address < lowAddress || address > highAddress) {</div>
<div>> > + return nullptr;</div>
<div>> > + }</div>
<div>> > +</div>
<div>> > + const SourceLine* last_line = nullptr;</div>
<div>> > + for (const auto &line : sourceLines) {</div>
<div>> > + if (address <= line.location())</div>
<div>> > + {</div>
<div>> > + if (address == line.location())</div>
<div>> > + last_line = &line;</div>
<div>> > + break;</div>
<div>> > + }</div>
<div>> > + last_line = &line;</div>
<div>> > + }</div>
<div>> > +</div>
<div>> > + return last_line;</div>
<div>> > + }</div>
<div>> > +</div>
<div>> > + void AddressToLineMapper::getSource(</div>
<div>> > + uint32_t address,</div>
<div>> > + std::string& sourceFile,</div>
<div>> > + int& sourceLine</div>
<div>> > + ) const {</div>
<div>> > + sourceFile = "unknown";</div>
<div>> > + sourceLine = -1;</div>
<div>> > +</div>
<div>> > + const SourceLine* match = nullptr;</div>
<div>> > + for (const auto &range : addressLineRanges) {</div>
<div>> > + const SourceLine* potential_match = range.getSourceLine(address);</div>
<div>> > +</div>
<div>> > + if (potential_match != nullptr) {</div>
<div>> > + if (match == nullptr) {</div>
<div>> > + match = potential_match;</div>
<div>> > + } else if (match->is_an_end_sequence() || !potential_match->is_an_end_sequence()) {</div>
<div>> > + match = potential_match;</div>
<div>> > + }</div>
<div>> > + }</div>
<div>> > + }</div>
<div>> > +</div>
<div>> > + if (match != nullptr) {</div>
<div>> > + sourceFile = match->path();</div>
<div>> > + sourceLine = match->line();</div>
<div>> > + }</div>
<div>> > + }</div>
<div>> > +</div>
<div>> > + AddressLineRange& AddressToLineMapper::makeRange(</div>
<div>> > + uint32_t low,</div>
<div>> > + uint32_t high</div>
<div>> > + )</div>
<div>> > + {</div>
<div>> > + addressLineRanges.emplace_back(</div>
<div>> > + AddressLineRange(low, high)</div>
<div>> > + );</div>
<div>> > +</div>
<div>> > + return addressLineRanges.back();</div>
<div>> > + }</div>
<div>> > +</div>
<div>> > +}</div>
<div>> > diff --git a/tester/covoar/AddressToLineMapper.h b/tester/covoar/AddressToLineMapper.h</div>
<div>> > new file mode 100644</div>
<div>> > index 0000000..ee0a633</div>
<div>> > --- /dev/null</div>
<div>> > +++ b/tester/covoar/AddressToLineMapper.h</div>
<div>> > @@ -0,0 +1,193 @@</div>
<div>> > +/*! @file AddressToLineMapper.h</div>
<div>> > + * @brief AddressToLineMapper Specification</div>
<div>> > + *</div>
<div>> > + * This file contains the specification of the AddressToLineMapper class.</div>
<div>> > + */</div>
<div>> > +</div>
<div>> > +#ifndef __ADDRESS_TO_LINE_MAPPER_H__</div>
<div>> > +#define __ADDRESS_TO_LINE_MAPPER_H__</div>
<div>> > +</div>
<div>> > +#include <cstdint></div>
<div>> > +#include <set></div>
<div>> > +#include <string></div>
<div>> > +#include <vector></div>
<div>> > +</div>
<div>> > +#include <rld-dwarf.h></div>
<div>> > +</div>
<div>> > +namespace Coverage {</div>
<div>> > +</div>
<div>> > + /*! @class SourceLine</div>
<div>> > + *</div>
<div>> > + * This class stores source information for a specific address.</div>
<div>> > + */</div>
<div>> > + class SourceLine {</div>
<div>> > +</div>
<div>> > + public:</div>
<div>> > +</div>
<div>> > + SourceLine(</div>
<div>> > + uint64_t addr,> + std::set<std::string>::const_iterator src,</div>
<div>></div>
<div>> Why not just pass in a reference to the object the iterator references? You do</div>
<div>> not touch the iterator which is a good thing because the iterator held in the</div>
<div>> object is a copy of a copy of the outside iterator.</div>
<div><br>
</div>
<div>Good point. I will change it to accept a const reference to a std::string.</div>
<div><br>
</div>
<div>></div>
<div>> > + int64_t ln,</div>
<div>> > + bool end_sequence</div>
<div>> > + ) : address(addr),</div>
<div>> > + sourceIterator(src),</div>
<div>> > + line_num(ln),</div>
<div>> > + is_end_sequence(end_sequence)</div>
<div>> > + {</div>
<div>> > + }</div>
<div>> > +</div>
<div>> > + /*!</div>
<div>> > + * This method gets the address of this source information.</div>
<div>> > + *</div>
<div>> > + * @return Returns the address of this source information</div>
<div>> > + */</div>
<div>> > + uint64_t location() const;</div>
<div>> > +</div>
<div>> > + /*!</div>
<div>> > + * This method gets a value indicating whether this address represents an</div>
<div>> > + * end sequence.</div>
<div>> > + *</div>
<div>> > + * @return Returns whether this address represents an end sequence or not</div>
<div>> > + */</div>
<div>> > + bool is_an_end_sequence() const;</div>
<div>> > +</div>
<div>> > + /*!</div>
<div>> > + * This method gets the source file path of this address.</div>
<div>> > + *</div>
<div>> > + * @return Returns the source file path of this address</div>
<div>> > + */</div>
<div>> > + std::string path() const;</div>
<div>> > +</div>
<div>> > + /*!</div>
<div>> > + * This method gets the source line number of this address.</div>
<div>> > + *</div>
<div>> > + * @return Returns the source line number of this address</div>
<div>> > + */</div>
<div>> > + uint64_t line() const;</div>
<div>></div>
<div>> Why not `unsigned int` or `size_t`?</div>
<div><br>
</div>
<div>Actually I'm seeing now that the line is always set as the result of a call to</div>
<div>`rld::dwarf::address::line()` which returns an `int`. The `rld::dwarf::address`</div>
<div>class, however, stores the line number as an `int64_t`. So maybe this should use</div>
<div>`int64_t`, and `rld::dwarf::address::line()` should be modified to return</div>
<div>`int64_t` or the equivalent `rld::dwarf::dwarf_signed`. Would that be</div>
<div>acceptable?</div>
<div><br>
</div>
<div>></div>
<div>> > +</div>
<div>> > + private:</div>
<div>> > +</div>
<div>> > + /*!</div>
<div>> > + * The address of this source information.</div>
<div>> > + */</div>
<div>> > + uint64_t address;</div>
<div>> > +</div>
<div>> > + /*!</div>
<div>> > + * An iterator pointing to the location in the set that contains the</div>
<div>> > + * source file path of the address.</div>
<div>> > + */</div>
<div>> > + std::set<std::string>::const_iterator sourceIterator;</div>
<div>></div>
<div>> Would `const std::string& path_;` work ?</div>
<div><br>
</div>
<div>Yes. I will change it.</div>
<div><br>
</div>
<div>></div>
<div>> > +</div>
<div>> > + /*!</div>
<div>> > + * The source line number of the address.</div>
<div>> > + */</div>
<div>> > + int64_t line_num;</div>
<div>> > +</div>
<div>> > + /*!</div>
<div>> > + * Whether the address represents an end sequence or not.</div>
<div>> > + */</div>
<div>> > + bool is_end_sequence;</div>
<div>> > +</div>
<div>> > + };</div>
<div>> > +</div>
<div>> > + /*! @class AddressLineRange</div>
<div>> > + *</div>
<div>> > + * This class stores source information for an address range.</div>
<div>> > + */</div>
<div>> > + class AddressLineRange {</div>
<div>> > +</div>
<div>> > + public:</div>
<div>> > +</div>
<div>> > + AddressLineRange(</div>
<div>> > + uint32_t low,</div>
<div>> > + uint32_t high</div>
<div>> > + ) : lowAddress(low), highAddress(high)</div>
<div>> > + {</div>
<div>> > + }</div>
<div>> > +</div>
<div>> > + /*!</div>
<div>> > + * This method adds source and line information for a specified address.</div>
<div>> > + *</div>
<div>> > + * @param[in] address specifies the DWARF address information</div>
<div>> > + */</div>
<div>> > + void addSourceLine(const rld::dwarf::address& address);</div>
<div>> > +</div>
<div>> > + /*!</div>
<div>> > + * This method gets the source file name and line number for a given</div>
<div>> > + * address.</div>
<div>> > + *</div>
<div>> > + * @param[in] address specifies the address to look up</div>
<div>> > + *</div>
<div>> > + * @return Returns the source information for the specified address or</div>
<div>> > + * nullptr if not found.</div>
<div>> > + */</div>
<div>> > + const SourceLine* getSourceLine(uint32_t address) const;</div>
<div>></div>
<div>> Why a pointer? Returning `nullptr` is a potential source of bugs where the</div>
<div>> pointer is not checked. All you have done is encoded an address and a state in</div>
<div>> the one variable, something that is done in C.</div>
<div>></div>
<div>> An `operator[](uint32_t address)` could be used here that throws an exception on</div>
<div>> an invalid address. You just need to catch the exception in</div>
<div>> `AddressToLineMapper::getSource()` and make `match` be in instance. Just add a</div>
<div>> default constructor to SourceLine with the path set to "uknown" and line to</div>
<div>> "-1". This however means you need to make the change to not use an iterator as a</div>
<div>> reference. It seems the choice to use the iterator in the class has cascaded out</div>
<div>> into other areas.</div>
<div><br>
</div>
<div>You're right. The pointer can be avoided. I will make this change.</div>
<div><br>
</div>
<div>></div>
<div>> > +</div>
<div>> > + private:</div>
<div>> > +</div>
<div>> > + /*!</div>
<div>> > + * The low address for this range.</div>
<div>> > + */</div>
<div>> > + uint32_t lowAddress;</div>
<div>> > +</div>
<div>> > + /*!</div>
<div>> > + * The high address for this range.</div>
<div>> > + */</div>
<div>> > + uint32_t highAddress;</div>
<div>> > +</div>
<div>> > + /*!</div>
<div>> > + * The source information for addresses in this range.</div>
<div>> > + */</div>
<div>> > + std::vector<SourceLine> sourceLines;</div>
<div>> > +</div>
<div>> > + /*!</div>
<div>> > + * The set of source file names for this range.</div>
<div>> > + */</div>
<div>> > + std::set<std::string> sourcePaths;</div>
<div>></div>
<div>> I always add a local class/struct typedef for containers.</div>
<div><br>
</div>
<div>I will make this change.</div>
<div><br>
</div>
<div>></div>
<div>> > +</div>
<div>> > + };</div>
<div>> > +</div>
<div>> > + /*! @class AddressToLineMapper</div>
<div>> > + *</div>
<div>> > + * This class provides address-to-line capabilities.</div>
<div>> > + */</div>
<div>> > + class AddressToLineMapper {</div>
<div>> > +</div>
<div>> > + public:</div>
<div>> > +</div>
<div>> > + /*!</div>
<div>> > + * This method gets the source file name and line number for a given</div>
<div>> > + * address.</div>
<div>> > + *</div>
<div>> > + * @param[in] address specifies the address to look up</div>
<div>> > + * @param[out] sourceFile specifies the name of the source file</div>
<div>> > + * @param[out] sourceLine specifies the line number in the source file</div>
<div>> > + */</div>
<div>> > + void getSource(</div>
<div>> > + uint32_t address,</div>
<div>> > + std::string& sourceFile,</div>
<div>> > + int& sourceLine</div>
<div>> > + ) const;</div>
<div>> > +</div>
<div>> > + /*!</div>
<div>> > + * This method creates a new range with the specified addresses.</div>
<div>> > + *</div>
<div>> > + * @param[in] low specifies the low address of the range</div>
<div>> > + * @param[in] high specifies the high address of the range</div>
<div>> > + *</div>
<div>> > + * @return Returns a reference to the newly created range</div>
<div>> > + */</div>
<div>> > + AddressLineRange& makeRange(uint32_t low, uint32_t high);</div>
<div>> > +</div>
<div>> > + private:</div>
<div>> > +</div>
<div>> > + /*!</div>
<div>> > + * The address and line information ranges.</div>
<div>> > + */</div>
<div>> > + std::vector<AddressLineRange> addressLineRanges;</div>
<div>></div>
<div>> Why is this private? :)</div>
<div><br>
</div>
<div>It is private because it does not need to be public. :)</div>
<div><br>
</div>
<div>Alex</div>
<div><br>
</div>
<div>></div>
<div>> Chris</div>
<div>></div>
<div>> > +</div>
<div>> > + };</div>
<div>> > +</div>
<div>> > +}</div>
<div>> > +</div>
<div>> > +#endif</div>
<div>> > diff --git a/tester/covoar/ExecutableInfo.cc b/tester/covoar/ExecutableInfo.cc</div>
<div>> > index 861e60d..a6184e7 100644</div>
<div>> > --- a/tester/covoar/ExecutableInfo.cc</div>
<div>> > +++ b/tester/covoar/ExecutableInfo.cc</div>
<div>> > @@ -40,61 +40,60 @@ namespace Coverage {</div>
<div>> > executable.begin();</div>
<div>> > executable.load_symbols(symbols);</div>
<div>> > </div>
<div>> > + rld::dwarf::file debug;</div>
<div>> > +</div>
<div>> > debug.begin(executable.elf());</div>
<div>> > debug.load_debug();</div>
<div>> > debug.load_functions();</div>
<div>> > </div>
<div>> > - try {</div>
<div>> > - for (auto& cu : debug.get_cus()) {</div>
<div>> > - for (auto& func : cu.get_functions()) {</div>
<div>> > - if (!func.has_machine_code()) {</div>
<div>> > - continue;</div>
<div>> > - }</div>
<div>> > + for (auto& cu : debug.get_cus()) {</div>
<div>> > + AddressLineRange& range = mapper.makeRange(cu.pc_low(), cu.pc_high());</div>
<div>> > + // Does not filter on desired symbols under the assumption that the test</div>
<div>> > + // code and any support code is small relative to what is being tested.</div>
<div>> > + for (const auto &address : cu.get_addresses()) {</div>
<div>> > + range.addSourceLine(address);</div>
<div>> > + }</div>
<div>> > </div>
<div>> > - if (!SymbolsToAnalyze->isDesired(func.name())) {</div>
<div>> > - continue;</div>
<div>> > + for (auto& func : cu.get_functions()) {</div>
<div>> > + if (!func.has_machine_code()) {</div>
<div>> > + continue;</div>
<div>> > + }</div>
<div>> > +</div>
<div>> > + if (!SymbolsToAnalyze->isDesired(func.name())) {</div>
<div>> > + continue;</div>
<div>> > + }</div>
<div>> > +</div>
<div>> > + if (func.is_inlined()) {</div>
<div>> > + if (func.is_external()) {</div>
<div>> > + // Flag it</div>
<div>> > + std::cerr << "Function is both external and inlined: "</div>
<div>> > + << func.name() << std::endl;</div>
<div>> > }</div>
<div>> > </div>
<div>> > - if (func.is_inlined()) {</div>
<div>> > - if (func.is_external()) {</div>
<div>> > - // Flag it</div>
<div>> > - std::cerr << "Function is both external and inlined: "</div>
<div>> > - << func.name() << std::endl;</div>
<div>> > - }</div>
<div>> > -</div>
<div>> > - if (func.has_entry_pc()) {</div>
<div>> > - continue;</div>
<div>> > - }</div>
<div>> > -</div>
<div>> > - // If the low PC address is zero, the symbol does not appear in</div>
<div>> > - // this executable.</div>
<div>> > - if (func.pc_low() == 0) {</div>
<div>> > - continue;</div>
<div>> > - }</div>
<div>> > + if (func.has_entry_pc()) {</div>
<div>> > + continue;</div>
<div>> > }</div>
<div>> > </div>
<div>> > - // We can't process a zero size function.</div>
<div>> > - if (func.pc_high() == 0) {</div>
<div>> > + // If the low PC address is zero, the symbol does not appear in</div>
<div>> > + // this executable.</div>
<div>> > + if (func.pc_low() == 0) {</div>
<div>> > continue;</div>
<div>> > }</div>
<div>> > + }</div>
<div>> > </div>
<div>> > - createCoverageMap (cu.name(), func.name(),</div>
<div>> > - func.pc_low(), func.pc_high() - 1);</div>
<div>> > + // We can't process a zero size function.</div>
<div>> > + if (func.pc_high() == 0) {</div>
<div>> > + continue;</div>
<div>> > }</div>
<div>> > +</div>
<div>> > + createCoverageMap (cu.name(), func.name(),</div>
<div>> > + func.pc_low(), func.pc_high() - 1);</div>
<div>> > }</div>
<div>> > - } catch (...) {</div>
<div>> > - debug.end();</div>
<div>> > - throw;</div>
<div>> > }</div>
<div>> > -</div>
<div>> > - // Can't cleanup handles until the destructor because the information is</div>
<div>> > - // referenced elsewhere. NOTE: This could cause problems from too many open</div>
<div>> > - // file descriptors.</div>
<div>> > }</div>
<div>> > </div>
<div>> > ExecutableInfo::~ExecutableInfo()</div>
<div>> > {</div>
<div>> > - debug.end();</div>
<div>> > }</div>
<div>> > </div>
<div>> > void ExecutableInfo::dumpCoverageMaps( void ) {</div>
<div>> > @@ -197,7 +196,7 @@ namespace Coverage {</div>
<div>> > {</div>
<div>> > std::string file;</div>
<div>> > int lno;</div>
<div>> > - debug.get_source (address, file, lno);</div>
<div>> > + mapper.getSource (address, file, lno);</div>
<div>> > std::ostringstream ss;</div>
<div>> > ss << file << ':' << lno;</div>
<div>> > line = ss.str ();</div>
<div>> > diff --git a/tester/covoar/ExecutableInfo.h b/tester/covoar/ExecutableInfo.h</div>
<div>> > index 5d5a595..dfc71aa 100644</div>
<div>> > --- a/tester/covoar/ExecutableInfo.h</div>
<div>> > +++ b/tester/covoar/ExecutableInfo.h</div>
<div>> > @@ -15,6 +15,7 @@</div>
<div>> > #include <rld-files.h></div>
<div>> > #include <rld-symbols.h></div>
<div>> > </div>
<div>> > +#include "AddressToLineMapper.h"</div>
<div>> > #include "CoverageMapBase.h"</div>
<div>> > #include "SymbolTable.h"</div>
<div>> > </div>
<div>> > @@ -157,11 +158,6 @@ namespace Coverage {</div>
<div>> > uint32_t highAddress</div>
<div>> > );</div>
<div>> > </div>
<div>> > - /*!</div>
<div>> > - * The DWARF data to the ELF executable.</div>
<div>> > - */</div>
<div>> > - rld::dwarf::file debug;</div>
<div>> > -</div>
<div>> > /*!</div>
<div>> > * The executable's file name.</div>
<div>> > */</div>
<div>> > @@ -172,6 +168,11 @@ namespace Coverage {</div>
<div>> > */</div>
<div>> > rld::symbols::table symbols;</div>
<div>> > </div>
<div>> > + /*!</div>
<div>> > + * The address-to-line mapper for this executable.</div>
<div>> > + */</div>
<div>> > + AddressToLineMapper mapper;</div>
<div>> > +</div>
<div>> > /*!</div>
<div>> > * This map associates a symbol with its coverage map.</div>
<div>> > */</div>
<div>> > diff --git a/tester/covoar/wscript b/tester/covoar/wscript</div>
<div>> > index 82599b0..a928b1b 100644</div>
<div>> > --- a/tester/covoar/wscript</div>
<div>> > +++ b/tester/covoar/wscript</div>
<div>> > @@ -83,6 +83,7 @@ def build(bld):</div>
<div>> > </div>
<div>> > bld.stlib(target = 'ccovoar',</div>
<div>> > source = ['app_common.cc',</div>
<div>> > + 'AddressToLineMapper.cc',</div>
<div>> > 'CoverageFactory.cc',</div>
<div>> > 'CoverageMap.cc',</div>
<div>> > 'CoverageMapBase.cc',</div>
<div>> ></div>
<div>> _______________________________________________</div>
<div>> devel mailing list</div>
<div>> devel@rtems.org</div>
> <a href="http://lists.rtems.org/mailman/listinfo/devel" id="LPlnk151970">http://lists.rtems.org/mailman/listinfo/devel</a><br>
</div>
<div id="appendonsend"></div>
<hr style="display:inline-block;width:98%" tabindex="-1">
<div id="divRplyFwdMsg" dir="ltr"><font face="Calibri, sans-serif" style="font-size:11pt" color="#000000"><b>From:</b> Chris Johns <chrisj@rtems.org><br>
<b>Sent:</b> Tuesday, May 25, 2021 7:24 PM<br>
<b>To:</b> Alex White <alex.white@oarcorp.com>; devel@rtems.org <devel@rtems.org><br>
<b>Subject:</b> Re: [PATCH 2/2] covoar: Store address-to-line info outside of DWARF</font>
<div> </div>
</div>
<div class="BodyFragment"><font size="2"><span style="font-size:11pt;">
<div class="PlainText"><br>
<br>
On 1/5/21 8:01 am, Alex White wrote:<br>
> This adds the AddressToLineMapper class and supporting classes to<br>
> assume responsibility of tracking address-to-line information.<br>
> <br>
> This allows the DWARF library to properly cleanup all of its resources<br>
> and leads to significant memory savings.<br>
> <br>
> Closes #4383<br>
> ---<br>
> rtemstoolkit/rld-dwarf.cpp | 5 +<br>
> rtemstoolkit/rld-dwarf.h | 5 +<br>
> tester/covoar/AddressToLineMapper.cc | 105 +++++++++++++++<br>
> tester/covoar/AddressToLineMapper.h | 193 +++++++++++++++++++++++++++<br>
> tester/covoar/ExecutableInfo.cc | 73 +++++-----<br>
> tester/covoar/ExecutableInfo.h | 11 +-<br>
> tester/covoar/wscript | 1 +<br>
> 7 files changed, 351 insertions(+), 42 deletions(-)<br>
> create mode 100644 tester/covoar/AddressToLineMapper.cc<br>
> create mode 100644 tester/covoar/AddressToLineMapper.h<br>
> <br>
> diff --git a/rtemstoolkit/rld-dwarf.cpp b/rtemstoolkit/rld-dwarf.cpp<br>
> index 2fce0e4..1eae50c 100644<br>
> --- a/rtemstoolkit/rld-dwarf.cpp<br>
> +++ b/rtemstoolkit/rld-dwarf.cpp<br>
> @@ -1893,6 +1893,11 @@ namespace rld<br>
> return false;<br>
> }<br>
> <br>
> + const addresses& compilation_unit::get_addresses () const<br>
> + {<br>
> + return addr_lines_;<br>
> + }<br>
> +<br>
> functions&<br>
> compilation_unit::get_functions ()<br>
> {<br>
> diff --git a/rtemstoolkit/rld-dwarf.h b/rtemstoolkit/rld-dwarf.h<br>
> index 1210813..eadb50d 100644<br>
> --- a/rtemstoolkit/rld-dwarf.h<br>
> +++ b/rtemstoolkit/rld-dwarf.h<br>
> @@ -707,6 +707,11 @@ namespace rld<br>
> */<br>
> unsigned int pc_high () const;<br>
> <br>
> + /**<br>
> + * The addresses associated with this compilation unit.<br>
> + */<br>
> + const addresses& get_addresses () const;<br>
> +<br>
> /**<br>
> * Get the source and line for an address. If the address does not match<br>
> * false is returned the file is set to 'unknown' and the line is set to<br>
> diff --git a/tester/covoar/AddressToLineMapper.cc b/tester/covoar/AddressToLineMapper.cc<br>
> new file mode 100644<br>
> index 0000000..1062fd7<br>
> --- /dev/null<br>
> +++ b/tester/covoar/AddressToLineMapper.cc<br>
> @@ -0,0 +1,105 @@<br>
> +/*! @file AddressToLineMapper.cc<br>
> + * @brief AddressToLineMapper Implementation<br>
> + *<br>
> + * This file contains the implementation of the functionality<br>
> + * of the AddressToLineMapper class.<br>
> + */<br>
> +<br>
> +#include "AddressToLineMapper.h"<br>
> +<br>
> +namespace Coverage {<br>
> +<br>
> + uint64_t SourceLine::location() const<br>
> + {<br>
> + return address;<br>
> + }<br>
> +<br>
> + bool SourceLine::is_an_end_sequence() const<br>
> + {<br>
> + return is_end_sequence;<br>
> + }<br>
> +<br>
> + std::string SourceLine::path() const<br>
> + {<br>
> + return *sourceIterator;<br>
> + }<br>
> +<br>
> + uint64_t SourceLine::line() const<br>
> + {<br>
> + return line_num;<br>
> + }<br>
> +<br>
> + void AddressLineRange::addSourceLine(const rld::dwarf::address& address)<br>
> + {<br>
> + auto insertResult = sourcePaths.insert(address.path());<br>
> +<br>
> + sourceLines.emplace_back(<br>
> + SourceLine (<br>
> + address.location(),<br>
> + insertResult.first,<br>
> + address.line(),<br>
> + address.is_an_end_sequence()<br>
> + )<br>
> + );<br>
> + }<br>
> +<br>
> + const SourceLine* AddressLineRange::getSourceLine(uint32_t address) const<br>
> + {<br>
> + if (address < lowAddress || address > highAddress) {<br>
> + return nullptr;<br>
> + }<br>
> +<br>
> + const SourceLine* last_line = nullptr;<br>
> + for (const auto &line : sourceLines) {<br>
> + if (address <= line.location())<br>
> + {<br>
> + if (address == line.location())<br>
> + last_line = &line;<br>
> + break;<br>
> + }<br>
> + last_line = &line;<br>
> + }<br>
> +<br>
> + return last_line;<br>
> + }<br>
> +<br>
> + void AddressToLineMapper::getSource(<br>
> + uint32_t address,<br>
> + std::string& sourceFile,<br>
> + int& sourceLine<br>
> + ) const {<br>
> + sourceFile = "unknown";<br>
> + sourceLine = -1;<br>
> +<br>
> + const SourceLine* match = nullptr;<br>
> + for (const auto &range : addressLineRanges) {<br>
> + const SourceLine* potential_match = range.getSourceLine(address);<br>
> +<br>
> + if (potential_match != nullptr) {<br>
> + if (match == nullptr) {<br>
> + match = potential_match;<br>
> + } else if (match->is_an_end_sequence() || !potential_match->is_an_end_sequence()) {<br>
> + match = potential_match;<br>
> + }<br>
> + }<br>
> + }<br>
> +<br>
> + if (match != nullptr) {<br>
> + sourceFile = match->path();<br>
> + sourceLine = match->line();<br>
> + }<br>
> + }<br>
> +<br>
> + AddressLineRange& AddressToLineMapper::makeRange(<br>
> + uint32_t low,<br>
> + uint32_t high<br>
> + )<br>
> + {<br>
> + addressLineRanges.emplace_back(<br>
> + AddressLineRange(low, high)<br>
> + );<br>
> +<br>
> + return addressLineRanges.back();<br>
> + }<br>
> +<br>
> +}<br>
> diff --git a/tester/covoar/AddressToLineMapper.h b/tester/covoar/AddressToLineMapper.h<br>
> new file mode 100644<br>
> index 0000000..ee0a633<br>
> --- /dev/null<br>
> +++ b/tester/covoar/AddressToLineMapper.h<br>
> @@ -0,0 +1,193 @@<br>
> +/*! @file AddressToLineMapper.h<br>
> + * @brief AddressToLineMapper Specification<br>
> + *<br>
> + * This file contains the specification of the AddressToLineMapper class.<br>
> + */<br>
> +<br>
> +#ifndef __ADDRESS_TO_LINE_MAPPER_H__<br>
> +#define __ADDRESS_TO_LINE_MAPPER_H__<br>
> +<br>
> +#include <cstdint><br>
> +#include <set><br>
> +#include <string><br>
> +#include <vector><br>
> +<br>
> +#include <rld-dwarf.h><br>
> +<br>
> +namespace Coverage {<br>
> +<br>
> + /*! @class SourceLine<br>
> + *<br>
> + * This class stores source information for a specific address.<br>
> + */<br>
> + class SourceLine {<br>
> +<br>
> + public:<br>
> +<br>
> + SourceLine(<br>
> + uint64_t addr,> + std::set<std::string>::const_iterator src,<br>
<br>
Why not just pass in a reference to the object the iterator references? You do<br>
not touch the iterator which is a good thing because the iterator held in the<br>
object is a copy of a copy of the outside iterator.<br>
<br>
> + int64_t ln,<br>
> + bool end_sequence<br>
> + ) : address(addr),<br>
> + sourceIterator(src),<br>
> + line_num(ln),<br>
> + is_end_sequence(end_sequence)<br>
> + {<br>
> + }<br>
> +<br>
> + /*!<br>
> + * This method gets the address of this source information.<br>
> + *<br>
> + * @return Returns the address of this source information<br>
> + */<br>
> + uint64_t location() const;<br>
> +<br>
> + /*!<br>
> + * This method gets a value indicating whether this address represents an
<br>
> + * end sequence.<br>
> + *<br>
> + * @return Returns whether this address represents an end sequence or not<br>
> + */<br>
> + bool is_an_end_sequence() const;<br>
> +<br>
> + /*!<br>
> + * This method gets the source file path of this address.<br>
> + *<br>
> + * @return Returns the source file path of this address<br>
> + */<br>
> + std::string path() const;<br>
> +<br>
> + /*!<br>
> + * This method gets the source line number of this address.<br>
> + *<br>
> + * @return Returns the source line number of this address<br>
> + */<br>
> + uint64_t line() const;<br>
<br>
Why not `unsigned int` or `size_t`?<br>
<br>
> +<br>
> + private:<br>
> +<br>
> + /*!<br>
> + * The address of this source information.<br>
> + */<br>
> + uint64_t address;<br>
> +<br>
> + /*!<br>
> + * An iterator pointing to the location in the set that contains the<br>
> + * source file path of the address.<br>
> + */<br>
> + std::set<std::string>::const_iterator sourceIterator;<br>
<br>
Would `const std::string& path_;` work ?<br>
<br>
> +<br>
> + /*!<br>
> + * The source line number of the address.<br>
> + */<br>
> + int64_t line_num;<br>
> +<br>
> + /*!<br>
> + * Whether the address represents an end sequence or not.<br>
> + */<br>
> + bool is_end_sequence;<br>
> +<br>
> + };<br>
> +<br>
> + /*! @class AddressLineRange<br>
> + *<br>
> + * This class stores source information for an address range.<br>
> + */<br>
> + class AddressLineRange {<br>
> +<br>
> + public:<br>
> +<br>
> + AddressLineRange(<br>
> + uint32_t low,<br>
> + uint32_t high<br>
> + ) : lowAddress(low), highAddress(high)<br>
> + {<br>
> + }<br>
> +<br>
> + /*!<br>
> + * This method adds source and line information for a specified address.<br>
> + *<br>
> + * @param[in] address specifies the DWARF address information<br>
> + */<br>
> + void addSourceLine(const rld::dwarf::address& address);<br>
> +<br>
> + /*!<br>
> + * This method gets the source file name and line number for a given <br>
> + * address.<br>
> + *<br>
> + * @param[in] address specifies the address to look up<br>
> + * <br>
> + * @return Returns the source information for the specified address or<br>
> + * nullptr if not found.<br>
> + */<br>
> + const SourceLine* getSourceLine(uint32_t address) const;<br>
<br>
Why a pointer? Returning `nullptr` is a potential source of bugs where the<br>
pointer is not checked. All you have done is encoded an address and a state in<br>
the one variable, something that is done in C.<br>
<br>
An `operator[](uint32_t address)` could be used here that throws an exception on<br>
an invalid address. You just need to catch the exception in<br>
`AddressToLineMapper::getSource()` and make `match` be in instance. Just add a<br>
default constructor to SourceLine with the path set to "uknown" and line to<br>
"-1". This however means you need to make the change to not use an iterator as a<br>
reference. It seems the choice to use the iterator in the class has cascaded out<br>
into other areas.<br>
<br>
> +<br>
> + private:<br>
> +<br>
> + /*!<br>
> + * The low address for this range.<br>
> + */<br>
> + uint32_t lowAddress;<br>
> +<br>
> + /*!<br>
> + * The high address for this range.<br>
> + */<br>
> + uint32_t highAddress;<br>
> +<br>
> + /*!<br>
> + * The source information for addresses in this range.<br>
> + */<br>
> + std::vector<SourceLine> sourceLines;<br>
> +<br>
> + /*!<br>
> + * The set of source file names for this range.<br>
> + */<br>
> + std::set<std::string> sourcePaths;<br>
<br>
I always add a local class/struct typedef for containers.<br>
<br>
> +<br>
> + };<br>
> +<br>
> + /*! @class AddressToLineMapper<br>
> + *<br>
> + * This class provides address-to-line capabilities.<br>
> + */<br>
> + class AddressToLineMapper {<br>
> +<br>
> + public:<br>
> +<br>
> + /*!<br>
> + * This method gets the source file name and line number for a given <br>
> + * address.<br>
> + *<br>
> + * @param[in] address specifies the address to look up<br>
> + * @param[out] sourceFile specifies the name of the source file<br>
> + * @param[out] sourceLine specifies the line number in the source file<br>
> + */<br>
> + void getSource(<br>
> + uint32_t address,<br>
> + std::string& sourceFile,<br>
> + int& sourceLine<br>
> + ) const;<br>
> +<br>
> + /*!<br>
> + * This method creates a new range with the specified addresses.<br>
> + *<br>
> + * @param[in] low specifies the low address of the range<br>
> + * @param[in] high specifies the high address of the range<br>
> + * <br>
> + * @return Returns a reference to the newly created range<br>
> + */<br>
> + AddressLineRange& makeRange(uint32_t low, uint32_t high);<br>
> +<br>
> + private:<br>
> +<br>
> + /*!<br>
> + * The address and line information ranges.<br>
> + */<br>
> + std::vector<AddressLineRange> addressLineRanges;<br>
<br>
Why is this private? :)<br>
<br>
Chris<br>
<br>
> +<br>
> + };<br>
> +<br>
> +}<br>
> +<br>
> +#endif<br>
> diff --git a/tester/covoar/ExecutableInfo.cc b/tester/covoar/ExecutableInfo.cc<br>
> index 861e60d..a6184e7 100644<br>
> --- a/tester/covoar/ExecutableInfo.cc<br>
> +++ b/tester/covoar/ExecutableInfo.cc<br>
> @@ -40,61 +40,60 @@ namespace Coverage {<br>
> executable.begin();<br>
> executable.load_symbols(symbols);<br>
> <br>
> + rld::dwarf::file debug;<br>
> +<br>
> debug.begin(executable.elf());<br>
> debug.load_debug();<br>
> debug.load_functions();<br>
> <br>
> - try {<br>
> - for (auto& cu : debug.get_cus()) {<br>
> - for (auto& func : cu.get_functions()) {<br>
> - if (!func.has_machine_code()) {<br>
> - continue;<br>
> - }<br>
> + for (auto& cu : debug.get_cus()) {<br>
> + AddressLineRange& range = mapper.makeRange(cu.pc_low(), cu.pc_high());<br>
> + // Does not filter on desired symbols under the assumption that the test<br>
> + // code and any support code is small relative to what is being tested.<br>
> + for (const auto &address : cu.get_addresses()) {<br>
> + range.addSourceLine(address);<br>
> + }<br>
> <br>
> - if (!SymbolsToAnalyze->isDesired(func.name())) {<br>
> - continue;<br>
> + for (auto& func : cu.get_functions()) {<br>
> + if (!func.has_machine_code()) {<br>
> + continue;<br>
> + }<br>
> +<br>
> + if (!SymbolsToAnalyze->isDesired(func.name())) {<br>
> + continue;<br>
> + }<br>
> +<br>
> + if (func.is_inlined()) {<br>
> + if (func.is_external()) {<br>
> + // Flag it<br>
> + std::cerr << "Function is both external and inlined: "<br>
> + << func.name() << std::endl;<br>
> }<br>
> <br>
> - if (func.is_inlined()) {<br>
> - if (func.is_external()) {<br>
> - // Flag it<br>
> - std::cerr << "Function is both external and inlined: "<br>
> - << func.name() << std::endl;<br>
> - }<br>
> -<br>
> - if (func.has_entry_pc()) {<br>
> - continue;<br>
> - }<br>
> -<br>
> - // If the low PC address is zero, the symbol does not appear in<br>
> - // this executable.<br>
> - if (func.pc_low() == 0) {<br>
> - continue;<br>
> - }<br>
> + if (func.has_entry_pc()) {<br>
> + continue;<br>
> }<br>
> <br>
> - // We can't process a zero size function.<br>
> - if (func.pc_high() == 0) {<br>
> + // If the low PC address is zero, the symbol does not appear in<br>
> + // this executable.<br>
> + if (func.pc_low() == 0) {<br>
> continue;<br>
> }<br>
> + }<br>
> <br>
> - createCoverageMap (cu.name(), func.name(),<br>
> - func.pc_low(), func.pc_high() - 1);<br>
> + // We can't process a zero size function.<br>
> + if (func.pc_high() == 0) {<br>
> + continue;<br>
> }<br>
> +<br>
> + createCoverageMap (cu.name(), func.name(),<br>
> + func.pc_low(), func.pc_high() - 1);<br>
> }<br>
> - } catch (...) {<br>
> - debug.end();<br>
> - throw;<br>
> }<br>
> -<br>
> - // Can't cleanup handles until the destructor because the information is<br>
> - // referenced elsewhere. NOTE: This could cause problems from too many open<br>
> - // file descriptors.<br>
> }<br>
> <br>
> ExecutableInfo::~ExecutableInfo()<br>
> {<br>
> - debug.end();<br>
> }<br>
> <br>
> void ExecutableInfo::dumpCoverageMaps( void ) {<br>
> @@ -197,7 +196,7 @@ namespace Coverage {<br>
> {<br>
> std::string file;<br>
> int lno;<br>
> - debug.get_source (address, file, lno);<br>
> + mapper.getSource (address, file, lno);<br>
> std::ostringstream ss;<br>
> ss << file << ':' << lno;<br>
> line = ss.str ();<br>
> diff --git a/tester/covoar/ExecutableInfo.h b/tester/covoar/ExecutableInfo.h<br>
> index 5d5a595..dfc71aa 100644<br>
> --- a/tester/covoar/ExecutableInfo.h<br>
> +++ b/tester/covoar/ExecutableInfo.h<br>
> @@ -15,6 +15,7 @@<br>
> #include <rld-files.h><br>
> #include <rld-symbols.h><br>
> <br>
> +#include "AddressToLineMapper.h"<br>
> #include "CoverageMapBase.h"<br>
> #include "SymbolTable.h"<br>
> <br>
> @@ -157,11 +158,6 @@ namespace Coverage {<br>
> uint32_t highAddress<br>
> );<br>
> <br>
> - /*!<br>
> - * The DWARF data to the ELF executable.<br>
> - */<br>
> - rld::dwarf::file debug;<br>
> -<br>
> /*!<br>
> * The executable's file name.<br>
> */<br>
> @@ -172,6 +168,11 @@ namespace Coverage {<br>
> */<br>
> rld::symbols::table symbols;<br>
> <br>
> + /*!<br>
> + * The address-to-line mapper for this executable.<br>
> + */<br>
> + AddressToLineMapper mapper;<br>
> +<br>
> /*!<br>
> * This map associates a symbol with its coverage map.<br>
> */<br>
> diff --git a/tester/covoar/wscript b/tester/covoar/wscript<br>
> index 82599b0..a928b1b 100644<br>
> --- a/tester/covoar/wscript<br>
> +++ b/tester/covoar/wscript<br>
> @@ -83,6 +83,7 @@ def build(bld):<br>
> <br>
> bld.stlib(target = 'ccovoar',<br>
> source = ['app_common.cc',<br>
> + 'AddressToLineMapper.cc',<br>
> 'CoverageFactory.cc',<br>
> 'CoverageMap.cc',<br>
> 'CoverageMapBase.cc',<br>
> <br>
</div>
</span></font></div>
</body>
</html>