On 16/7/21 12:22 pm, Alex White wrote:
>
> On Thu, Jul 15, 2021 at 1:20 AM Chris Johns <chr...@rtems.org> wrote:
>>
>> On 15/7/21 12:45 pm, Alex White wrote:
>> > On Tue, Jul 6, 2021 at 7:55 PM Chris Johns <chr...@rtems.org> wrote:
>> >>
>> >> On 3/7/21 5:56 am, Alex White wrote:
>> >> > On Wed, Jun 30, 2021 at 11:40 PM <chr...@rtems.org> wrote:
>> >> >>
>> >> >> From: Chris Johns <chr...@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
_______________________________________________
devel mailing list
devel@rtems.org
http://lists.rtems.org/mailman/listinfo/devel
