[PATCH] covoar: Fix errors building on FreeBSD and clang

[Alex White]

On Thu, Jul 15, 2021 at 1:20 AM Chris Johns <chrisj at rtems.org> wrote:
>
> On 15/7/21 12:45 pm, Alex White wrote:
> > On Tue, Jul 6, 2021 at 7:55 PM Chris Johns <chrisj at rtems.org> wrote:
> >>
> >> On 3/7/21 5:56 am, Alex White wrote:
> >> > On Wed, Jun 30, 2021 at 11:40 PM <chrisj at rtems.org> wrote:
> >> >>
> >> >> From: Chris Johns <chrisj at rtems.org>
> >> >>
> >> >> - The member variable `path_` cannot be a reference and initialised to
> >> >>   a const char* type input. To do so would require there is a temporary with
> >> >>   an unspecified life time.
> >> >> ---
> >> >>  tester/covoar/AddressToLineMapper.h | 2 +-
> >> >>  tester/covoar/Target_aarch64.h      | 2 +-
> >> >>  2 files changed, 2 insertions(+), 2 deletions(-)
> >> >>
> >> >> diff --git a/tester/covoar/AddressToLineMapper.h
> >> > b/tester/covoar/AddressToLineMapper.h
> >> >> index 88bf475..c78aef7 100644
> >> >> --- a/tester/covoar/AddressToLineMapper.h
> >> >> +++ b/tester/covoar/AddressToLineMapper.h
> >> >> @@ -84,7 +84,7 @@ namespace Coverage {
> >> >>       *  An iterator pointing to the location in the set that contains the
> >> >>       *  source file path of the address.
> >> >>       */
> >> >> -    const std::string& path_;
> >> >> +    const std::string path_;
> >> >
> >> > Ryan alerted me about this issue a couple weeks back. This patch would fix the
> >> > problem, but it would lead to a second copy of every file path string being
> >> > stored in memory. This would also take away the usefulness of the set of file
> >> > path strings maintained by the AddressLineRange class.
> >> >
> >> > Instead, I propose we change the empty SourceLine constructor to take a `const
> >> > std::string&`. This would allow the addition of something like this to the top
> >> > of AddressToLineMapper::getSource():
> >> > const std::string unknown = "unknown";
> >> > const SourceLine default_sourceline = SourceLine(unknown);
> >> >
> >> > That should eliminate the issue and preserve the memory conservation efforts of
> >> > the original design.
> >>
> >> Yes it would but for some reason, that I cannot put my finger on, it seems like
> >> a breach of boundaries between classes.
> >>
> >> How much data are we talking about? Are you able to see the memory foot print
> >> with the strings being contained vs what you have now?
> >
> > Sorry for the late reply.
> >
> > What I have now yields a peak memory usage for covoar when run on all ARM tests
> > of 219 MB.
> > Changing `SourceLine::path_` to plain `std::string` results in an increase to
> > 523 MB.
> >
> > So it is a significant increase.
>
> Yes and thank you for the test results. This makes sharing a worth while exercise.
>
> >> If the figures show the strings need to be shared to avoid a memory blow out
> >> would a std::shared_ptr<std::string> be something to look at where all
> >> references are a shared pointer? A shared pointer means any changes do not flow
> >> from one class to other.
> >
> > I don't think that `std::shared_ptr` would help here. Wouldn't that handle the
> > case of an unknown path the same way as a raw pointer solution? Wouldn't we
> > still have to check the return value of `SourceLine::path()` to make sure that
> > it is not null?
> > The only other way I see to make it work would be to store some "unknown" string
> > object and hand pointers to it to all `SourceLine` objects with unknown paths.
> > But that seems mostly equivalent to what I propose in my original reply to this
> > patch.
>
> I have not checked all the detail but a raw pointer will always be fragile
> compared to a shared_ptr. You may get it working but there is nothing making
> sure any references are still pointing to valid memory. Given the difference in
> sizes you found there is a lot of sharing and so a lot of references.
>
> > Now that I think about it, maybe making `SourceLine::_path` into an
> > `std::string*` and doing a `nullptr` check in the `SourceLine::path()` would be
> > most elegant?
>
> All I see are potential leaks, maybe not now but may be after the next person
> makes changes.

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.
`AddressLineRange` owns the `std::string` vector.
`AddressLineRange` also owns the `SourceLine` vector. `SourceLine` owns the `path_` that references a string in the above vector.
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`.

On top of that, the shared_ptr solution looks like it would be much less elegant as far as I can tell.
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.

I just don't see a good, clean solution using `std::shared_ptr`. Maybe I'm missing something?

Thanks,

Alex

>
> > That way we don't have to do any `nullptr` checks outside of the `SourceLine`
> > implementation. We could just have `SourceLine::path()` return a plain old
> > `std::string` and do something like this:
> >
> > if (_path == nullptr) {
> >     return "unknown";
> > else {
> >     return *_path;
> > }
>
> A std::shared_ptr has the bool operator to check if the pointer is the nullptr
> so it can store a nullptr. Why not use:
>
>  if (!_path) {
>      return static_const_unknown_string;
>  } else {
>      return *(_path.get());
>  }
>
> ?
>
> Chris
> _______________________________________________
> devel mailing list
> devel at rtems.org
> http://lists.rtems.org/mailman/listinfo/devel

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.rtems.org/pipermail/devel/attachments/20210716/5ef9120d/attachment-0001.html>