<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=iso-8859-1">
<style type="text/css" style="display:none;"> P {margin-top:0;margin-bottom:0;} </style>
</head>
<body dir="ltr">
<br>
On Wed, Jun 2, 2021 at 7:37 PM Chris Johns <chrisj@rtems.org> wrote:<br>
><br>
> Looking good with a single comment below ...<br>
><br>
> On 3/6/21 6:08 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 | 104 +++++++++++++<br>
> > tester/covoar/AddressToLineMapper.h | 211 +++++++++++++++++++++++++++<br>
> > tester/covoar/ExecutableInfo.cc | 73 +++++----<br>
> > tester/covoar/ExecutableInfo.h | 11 +-<br>
> > tester/covoar/wscript | 1 +<br>
> > 7 files changed, 368 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..c305e3b<br>
> > --- /dev/null<br>
> > +++ b/tester/covoar/AddressToLineMapper.cc<br>
> > @@ -0,0 +1,104 @@<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>
> > + const std::string& SourceLine::path() const<br>
> > + {<br>
> > + return path_;<br>
> > + }<br>
> > +<br>
> > + int 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>
> > + throw SourceNotFoundError(std::to_string(address));<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>
> > + if (last_line == nullptr) {<br>
> > + throw SourceNotFoundError(std::to_string(address));<br>
> > + }<br>
> > +<br>
> > + return *last_line;<br>
> > + }<br>
><br>
> How often is this function being called? If it is a hot spot are there other<br>
> ways to search for the address? I suppose it depends on the number of lines in<br>
<div>> the vector.<br>
</div>
<div><br>
</div>
<div>According to a quick gprof run, this function is being called a decent amount, somewhere around 1.5 million times for a full ARM coverage run.</div>
<div><br>
</div>
<div>But it only takes up a very small amount (less than 0.02%) of cumulative execution time so I don't believe it is an issue.</div>
<div><br>
</div>
<div>Alex</div>
<div><br>
</div>
><br>
> If the sourcesLines vector was sorted can this loop be replaced with some form<br>
> of std::upper_bound? That is ...<br>
><br>
> auto last_line =<br>
> std::upper_bound(sourceLines.begin(),<br>
> sourceLines.end(),<br>
> address,<br>
> [&address](const SourceLine& sl) {<br>
> return address < sl.location();<br>
> });<br>
><br>
> [ not sure if the code I posted would work and if I got the lamda right :) ]<br>
><br>
> Just wondering.<br>
><br>
> Chris<br>
><br>
> > +<br>
> > + void AddressToLineMapper::getSource(<br>
> > + uint32_t address,<br>
> > + std::string& sourceFile,<br>
> > + int& sourceLine<br>
> > + ) const {<br>
> > + const SourceLine default_sourceline = SourceLine();<br>
> > + const SourceLine* match = &default_sourceline;<br>
> > +<br>
> > + for (const auto &range : addressLineRanges) {<br>
> > + try {<br>
> > + const SourceLine& potential_match = range.getSourceLine(address);<br>
> > +<br>
> > + if (match->is_an_end_sequence() || !potential_match.is_an_end_sequence()) {<br>
> > + match = &potential_match;<br>
> > + }<br>
> > + } catch (const AddressLineRange::SourceNotFoundError&) {}<br>
> > + }<br>
> > +<br>
> > + sourceFile = match->path();<br>
> > + sourceLine = match->line();<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..e6ab4a8<br>
> > --- /dev/null<br>
> > +++ b/tester/covoar/AddressToLineMapper.h<br>
> > @@ -0,0 +1,211 @@<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>
> > + : address(0),<br>
> > + path_("unknown"),<br>
> > + line_num(-1),<br>
> > + is_end_sequence(true)<br>
> > + {<br>
> > + }<br>
> > +<br>
> > + SourceLine(<br>
> > + uint64_t addr,<br>
> > + const std::string& src,<br>
> > + int line,<br>
> > + bool end_sequence<br>
> > + ) : address(addr),<br>
> > + path_(src),<br>
> > + line_num(line),<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>
> > + const 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>
> > + int line() const;<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>
> > + const std::string& path_;<br>
> > +<br>
> > + /*!<br>
> > + * The source line number of the address.<br>
> > + */<br>
> > + int line_num;<br>
> > +<br>
> > + /*!<br>
> > + * Whether the address represents an end sequence or not.<br>
> > + */<br>
> > + bool is_end_sequence;<br>
> > +<br>
> > + };<br>
> > +<br>
> > + typedef std::vector<SourceLine> SourceLines;<br>
> > +<br>
> > + typedef std::set<std::string> SourcePaths;<br>
> > +<br>
> > + /*! @class AddressLineRange<br>
> > + *<br>
> > + * This class stores source information for an address range.<br>
> > + */<br>
> > + class AddressLineRange {<br>
> > +<br>
> > + public:<br>
> > +<br>
> > + class SourceNotFoundError : public std::runtime_error {<br>
> > + /* Use the base class constructors. */<br>
> > + using std::runtime_error::runtime_error;<br>
> > + };<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.<br>
> > + */<br>
> > + const SourceLine& getSourceLine(uint32_t address) const;<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>
> > + SourceLines sourceLines;<br>
> > +<br>
> > + /*!<br>
> > + * The set of source file names for this range.<br>
> > + */<br>
> > + SourcePaths sourcePaths;<br>
> > +<br>
> > + };<br>
> > +<br>
> > + typedef std::vector<AddressLineRange> AddressLineRanges;<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>
> > + AddressLineRanges addressLineRanges;<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>
> _______________________________________________<br>
> devel mailing list<br>
> devel@rtems.org<br>
> <a href="http://lists.rtems.org/mailman/listinfo/devel" id="LPlnk271137">http://lists.rtems.org/mailman/listinfo/devel</a>
</body>
</html>