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

[Alex White]

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.

>
> 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.

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?
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;
}

Alex

>
> 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/20210715/b061d686/attachment.html>