<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">
<div style="font-family: Calibri, Arial, Helvetica, sans-serif; font-size: 12pt; color: rgb(0, 0, 0);">
<div class="Am aO9 Al editable LW-avf tS-tW tS-tY" tabindex="1" style="min-height:85px" spellcheck="false">
<br>
On Thu, Jul 15, 2021 at 1:20 AM Chris Johns <chrisj@rtems.org> wrote:<br>
><br>
> On 15/7/21 12:45 pm, Alex White wrote:<br>
> > On Tue, Jul 6, 2021 at 7:55 PM Chris Johns <chrisj@rtems.org> wrote:<br>
> >><br>
> >> On 3/7/21 5:56 am, Alex White wrote:<br>
> >> > On Wed, Jun 30, 2021 at 11:40 PM <chrisj@rtems.org> wrote:<br>
> >> >><br>
> >> >> From: Chris Johns <chrisj@rtems.org><br>
> >> >><br>
> >> >> - The member variable `path_` cannot be a reference and initialised to<br>
> >> >> a const char* type input. To do so would require there is a temporary with<br>
> >> >> an unspecified life time.<br>
> >> >> ---<br>
> >> >> tester/covoar/AddressToLineMapper.h | 2 +-<br>
> >> >> tester/covoar/Target_aarch64.h | 2 +-<br>
> >> >> 2 files changed, 2 insertions(+), 2 deletions(-)<br>
> >> >><br>
> >> >> diff --git a/tester/covoar/AddressToLineMapper.h<br>
> >> > b/tester/covoar/AddressToLineMapper.h<br>
> >> >> index 88bf475..c78aef7 100644<br>
> >> >> --- a/tester/covoar/AddressToLineMapper.h<br>
> >> >> +++ b/tester/covoar/AddressToLineMapper.h<br>
> >> >> @@ -84,7 +84,7 @@ namespace Coverage {<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>
> >> >> + const std::string path_;<br>
> >> ><br>
> >> > Ryan alerted me about this issue a couple weeks back. This patch would fix the<br>
> >> > problem, but it would lead to a second copy of every file path string being<br>
> >> > stored in memory. This would also take away the usefulness of the set of file<br>
> >> > path strings maintained by the AddressLineRange class.<br>
> >> ><br>
> >> > Instead, I propose we change the empty SourceLine constructor to take a `const<br>
> >> > std::string&`. This would allow the addition of something like this to the top<br>
> >> > of AddressToLineMapper::getSource():<br>
> >> > const std::string unknown = "unknown";<br>
> >> > const SourceLine default_sourceline = SourceLine(unknown);<br>
> >> ><br>
> >> > That should eliminate the issue and preserve the memory conservation efforts of<br>
> >> > the original design.<br>
> >><br>
> >> Yes it would but for some reason, that I cannot put my finger on, it seems like<br>
> >> a breach of boundaries between classes.<br>
> >><br>
> >> How much data are we talking about? Are you able to see the memory foot print<br>
> >> with the strings being contained vs what you have now?<br>
> ><br>
> > Sorry for the late reply.<br>
> ><br>
> > What I have now yields a peak memory usage for covoar when run on all ARM tests<br>
> > of 219 MB.<br>
> > Changing `SourceLine::path_` to plain `std::string` results in an increase to<br>
> > 523 MB.<br>
> ><br>
> > So it is a significant increase.<br>
><br>
> Yes and thank you for the test results. This makes sharing a worth while exercise.<br>
><br>
> >> If the figures show the strings need to be shared to avoid a memory blow out<br>
> >> would a std::shared_ptr<std::string> be something to look at where all<br>
> >> references are a shared pointer? A shared pointer means any changes do not flow<br>
> >> from one class to other.<br>
> ><br>
> > I don't think that `std::shared_ptr` would help here. Wouldn't that handle the<br>
> > case of an unknown path the same way as a raw pointer solution? Wouldn't we<br>
> > still have to check the return value of `SourceLine::path()` to make sure that<br>
> > it is not null?<br>
> > The only other way I see to make it work would be to store some "unknown" string<br>
> > object and hand pointers to it to all `SourceLine` objects with unknown paths.<br>
> > But that seems mostly equivalent to what I propose in my original reply to this<br>
> > patch.<br>
><br>
> I have not checked all the detail but a raw pointer will always be fragile<br>
> compared to a shared_ptr. You may get it working but there is nothing making<br>
> sure any references are still pointing to valid memory. Given the difference in<br>
> sizes you found there is a lot of sharing and so a lot of references.<br>
><br>
> > Now that I think about it, maybe making `SourceLine::_path` into an<br>
> > `std::string*` and doing a `nullptr` check in the `SourceLine::path()` would be<br>
> > most elegant?<br>
><br>
> All I see are potential leaks, maybe not now but may be after the next person<br>
> makes changes.
<div><br>
</div>
<div>I don't see how potential leaks could happen with this. The issue of a possible dangling pointer, which I can see, is eliminated by design.</div>
<div>`AddressLineRange` owns the `std::string` vector.</div>
<div>`AddressLineRange` also owns the `SourceLine` vector. `SourceLine` owns the `path_` that references a string in the above vector.</div>
<div>This means that the strings will only be deleted when `AddressLineRange` is deleted. At the same time, all of the `SourceLine` objects will be deleted. So, unless I'm missing something (which I could be), the `SourceLine::_path` as an `std::string*` should
be fine since both the `std::string` it points to and the owner of the `std::string*` will be cleaned up as part of the same teardown of `AddressLineRange`.</div>
<div><br>
</div>
<div>On top of that, the shared_ptr solution looks like it would be much less elegant as far as I can tell.</div>
<div>I think the set of strings would have to become an `std::set<std::shared_ptr<std::string>>` and that wouldn't really work because multiple `shared_ptr` to the same string don't compare as equal.</div>
<div><br>
</div>
<div>I just don't see a good, clean solution using `std::shared_ptr`. Maybe I'm missing something?</div>
<div><br>
</div>
<div>Thanks,</div>
<div><br>
</div>
<div>Alex</div>
<div><br>
><br>
> > That way we don't have to do any `nullptr` checks outside of the `SourceLine`<br>
> > implementation. We could just have `SourceLine::path()` return a plain old<br>
> > `std::string` and do something like this:<br>
> ><br>
> > if (_path == nullptr) {<br>
> > return "unknown";<br>
> > else {<br>
> > return *_path;<br>
> > }<br>
><br>
> A std::shared_ptr has the bool operator to check if the pointer is the nullptr<br>
> so it can store a nullptr. Why not use:<br>
><br>
> if (!_path) {<br>
> return static_const_unknown_string;<br>
> } else {<br>
> return *(_path.get());<br>
> }<br>
><br>
> ?<br>
><br>
> Chris<br>
> _______________________________________________<br>
> devel mailing list<br>
> devel@rtems.org<br>
> http://lists.rtems.org/mailman/listinfo/devel</div>
</div>
<br>
</div>
</body>
</html>