Re: [PATCH 05/11] perf parse-event: Fix memory leak in evsel->unit

From: Ian Rogers <irogers@google.com>
To: Namhyung Kim <namhyung@kernel.org>
Cc: David Malcolm <dmalcolm@redhat.com>,
	Arnaldo Carvalho de Melo <acme@kernel.org>,
	Jiri Olsa <jolsa@redhat.com>, Ingo Molnar <mingo@kernel.org>,
	Peter Zijlstra <a.p.zijlstra@chello.nl>,
	Mark Rutland <mark.rutland@arm.com>,
	Alexander Shishkin <alexander.shishkin@linux.intel.com>,
	Stephane Eranian <eranian@google.com>,
	LKML <linux-kernel@vger.kernel.org>,
	Andi Kleen <ak@linux.intel.com>
Subject: Re: [PATCH 05/11] perf parse-event: Fix memory leak in evsel->unit
Date: Wed, 16 Sep 2020 11:37:03 -0700	[thread overview]
Message-ID: <CAP-5=fWr0UVxp+JBcwzPV3=+EuUiBXuzHtaH5+1s1rCQb_dmdw@mail.gmail.com> (raw)
In-Reply-To: <CAM9d7cgtDq8yOFDEGEdTD9kN=Ko1gX=5o+tAB4+EDtN0WtGQPw@mail.gmail.com>

On Wed, Sep 16, 2020 at 12:12 AM Namhyung Kim <namhyung@kernel.org> wrote:
>
> Hello Ian and David,
>
> Thank you for the good suggestions!
>
> On Wed, Sep 16, 2020 at 4:56 AM David Malcolm <dmalcolm@redhat.com> wrote:
> > Some ideas (with the caveat that I'm a GCC developer, and not a regular
> > on LKML): can you capture the ownership status in the type system?
> > I'm brainstorming here but how about:
> >   typedef char *owned_string_t;
> >   typedef const char *borrowed_string_t;
> > This would at least capture the intent in human-readable form, and
> > *might* make things more amenable to checking by a machine.  It's also
> > less macro cruft.
> > I take it that capturing the ownership status with a runtime flag next
> > to the pointer in a struct is too expensive for your code?
>
> Adding more random thoughts..
>
> I think we can make it more generic like __attribute__((owned))
> so that it can be applied to any pointers.  And we can use a
> conventional macro like '__owned' in the declaration..
>
> __owned char *name;
> __owned char *strdup(const char *);
> ...
>
> Thanks
> Namhyung

I have to say I like the idea of a __owned like "modifier" before
these names more than introducing types. David, do you think a patch
with something like the following is reasonable? I'm also throwing
this out there to see if somebody on the linux code side screams and
thinks this is the worst idea ever in existence :-)

compiler.h:
/* In the future __owned and __unowned will be an attribute to allow
static analysis to perform certain correctness checks. For now they
are placeholders to provide documentation. */
#define __owned
#define __unowned
..
evsel.h:
..
struct evsel {
  ..
  __owned char *name;
  ..
  __unowned const char *unit;
  ..

Thanks,
Ian