Messages in this thread | | | Date | Tue, 22 Jun 2021 14:42:04 -0300 | From | Arnaldo Carvalho de Melo <> | Subject | Re: [PATCH 2/2] perf script: delete evlist when deleting session |
| |
Em Tue, Jun 22, 2021 at 09:33:23AM -0700, Ian Rogers escreveu: > On Tue, Jun 22, 2021 at 12:44 AM Riccardo Mancini <rickyman7@gmail.com> wrote: > > > > Hi, > > > > thanks for your comments. > > > > On Mon, 2021-06-21 at 22:14 -0700, Ian Rogers wrote: > > > On Mon, Jun 21, 2021 at 4:44 PM Riccardo Mancini <rickyman7@gmail.com> wrote: > > > > > > > > ASan reports a memory leak related to session->evlist never being deleted. > > > > The evlist member is not deleted in perf_session__delete, so it should be > > > > deleted separately. > > > > This patch adds the missing deletion in perf-script. > > > > > > > > Signed-off-by: Riccardo Mancini <rickyman7@gmail.com> > > > > --- > > > > tools/perf/builtin-script.c | 2 +- > > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > > > diff --git a/tools/perf/builtin-script.c b/tools/perf/builtin-script.c > > > > index 1280cbfad4db..635a1d9cfc88 100644 > > > > --- a/tools/perf/builtin-script.c > > > > +++ b/tools/perf/builtin-script.c > > > > @@ -3991,7 +3991,7 @@ int cmd_script(int argc, const char **argv) > > > > zfree(&script.ptime_range); > > > > } > > > > > > > > - evlist__free_stats(session->evlist); > > > > > > Should this be removed? > > > > Probably not. I originally thought this was already taken care of by > > evlist__delete, but it's not. > > Oddly, this issue is not causing a memory leak in my simple test. > > > > > > > > > + evlist__delete(session->evlist);
This looks like a bug, if it is a 'session' member, its a session method that should delete it, probably perf_session__delete().
> > > If the perf session "owns" the evlist, would it be cleaner to add this > > > to perf_session__delete? > > > > I thought about that too, but that's not always true. > > E.g., in perf-record, __cmd_record calls perf_session__delete,then cmd_record > > calls evlist__delete on rec->evlist, which points to the same location to which > > session->evlist pointed. > > Agreed. I find it hard to understand the ownership properties in the > perf code. The missing delete is an example of the owner of the evlist > (the caller) not "knowing" it needed cleaning up. I'd like it if we > documented things like perf_sessions' evlist to say not owned, user > must clean up. The makes it unambiguous who has to take > responsibility. Having things clean up after themselves is of course > easiest, hence wanting this to be in perf_session__delete.
This specific case, from just reading the description on this message, looks just like a bug/thinko.
> Fwiw, I've been reading around things like sparse [1, 2] and Clang's > similar analysis [3] that people have looked to use like sparse [4]. I > don't see anything that handles memory allocation lifetimes, but > perhaps something will feed into C's standards by way of C++ [5]. > Perhaps people have ideas to rewrite in checked C or Rust :-) > > Some thoughts: > 1) we can't have C++ as we're trying to follow kernel conventions [6] > 2) we can't annotate code for things like sparse or thread safety > analysis, as checking for memory errors is out of scope for them, the > annotations don't exist, etc. > 3) we can add comments, document the rules around pointers, perhaps > even invent empty annotations that may one day help with automated > checking. > 4) we can try to clean up the ownership model to make bugs less likely. > > I've heard concerns on non-kernel projects about annotation litter and > comments adding to complexity. I think your patch is good, it follows > the existing conventions. I wonder if we can learn something from the > fact the code was wrong to make it less likely we have wrong code in > the future. I'd be interested to hear what others think. > > Thanks, > Ian > > [1] https://lore.kernel.org/lkml/Pine.LNX.4.58.0410302005270.28839@ppc970.osdl.org/ > [2] https://lwn.net/Articles/689907/ > [3] https://clang.llvm.org/docs/ThreadSafetyAnalysis.html > [4] https://www.openwall.com/lists/kernel-hardening/2019/05/20/3 > [5] https://github.com/isocpp/CppCoreGuidelines/blob/master/docs/Lifetime.pdf > [6] even concatenating a string is error prone in C :-( > https://lore.kernel.org/lkml/YMzOpgZPJeC2jGKf@kernel.org/ > > > Thanks, > > Riccardo > > > > > > > > Thanks, > > > Ian > > > > > > > perf_session__delete(session); > > > > > > > > if (script_started) > > > > -- > > > > 2.31.1 > > > > > > > >
--
- Arnaldo
| |