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

[Chris Johns]

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.

> 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