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

[Chris Johns]

On 16/7/21 12:22 pm, Alex White wrote:
> 
> 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.
Any place the string is passed out of the context of the AddressToLineMapper.h
you need to be careful. I insulated the code to some extent when I refactored
part of it here:

https://git.rtems.org/rtems-tools/tree/tester/covoar/ExecutableInfo.cc#n192

However assuming this is always the case for ever makes the code fragile. My
approach is to make sure classes in a specific area of code take care of themselves.

There are plenty of cases in this code of sharing being an issue ...

https://git.rtems.org/rtems-tools/tree/tester/covoar/ExecutableInfo.cc#n117

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

While the usage is constrained it is OK. I hope further clean ups do not become
a problem. When I pushed the DWARF support in I could only go some of the way to
cleaning things up and as a result there are cases that present themselves as
fragile.

> At the same time, all of the `SourceLine` objects will be deleted. 

Yes as it stands.

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

Up to you.

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

Do you need to compare the results of .get() and not the instances as they are
at different addresses?

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

I will leave this with you.

Chris

Chris