Re: [PATCH v2] perf: Support for Arm A32/T32 instruction sets in CoreSight trace

[Kim Phillips <kim.phillips@arm.com>]

On Fri, 31 Aug 2018 14:42:00 +0100
Robert Walker <robert.walker@arm.com> wrote:

> Generally, I agree with you about breaking backward compatibility, but 
> in this case I don't think there is an actual problem.  As I understand 

I consider it a serious problem.

> it, you're worried that perf will break for people who are using an 
> older version (0.8.x) of the OpenCSD library for CoreSight trace decode 
> and this patch updates the requirement to a newer version (0.9.x) to 
> enable support for trace of 32-bit applications.

My problem is: every time a new feature is added, it shouldn't
force base CoreSight decode functionality to require a new version of
the library.

My second problem is: this patch implements a build-time requirement,
which is insufficient for running on machines with incompatible
versions of the library.

> There are only a few (4/5?) targets around with working support for 
> CoreSight trace (and of these only Juno is the only platform with a 
> devicetree in the mainline kernel), so only a few users of perf for Arm 
> trace decode - most of these are people working those directly involved 
> with Arm & Linaro or will be reading the coresight mailing list.  Anyone

Great, then share this feature with them, but don't send a patch to
break upstream, which, in turn, goes back to many things downstream
(future distro releases on newer targets, etc.).

> working with OpenCSD will have got it from github and compiled it 
> themselves, so they can update and build a new version.  It's only been 

No.  Updating the library - no matter where one gets it from - shouldn't
have to be necessary to avoid perf build regressions.

> packaged for debian so far and testing already has the 0.9.x version 
> (the 0.8.x version was only in debian for 8 days before being replaced 
> by 0.9.x).

What Debian does is immaterial to upstream submissions.

How is Debian testing the library, btw?  Functionality that worked in
0.8 will fail in 0.9 AFAICT.

> It would be possible to add conditional compilation flags to support 
> compiling with 0.8.x, but I feel this would add too much mess to the 
> code and I'd need some help in figuring out perf's feature detection 
> system to generate the flags.

No, we need run-time compatibility.  Build-time compatibility does not
satisfy the run-time requirements in this case.

>  Given the likely small number of people 
> affected and the easy upgrade path, I don't think this is worth it.

This is an upstream submission, and I wouldn't like for a single person
to ever have to experience such silently deceitful bugs.

For now, share the new feature in a personal git tree, for those that
need it.

Meanwhile, the library needs to be fixed with a run-time version
compatibility API ASAP.

Thanks,

Kim

> On 29/08/18 17:32, Kim Phillips wrote:
> > On Wed, 29 Aug 2018 15:34:16 +0100
> > Robert Walker <robert.walker@arm.com> wrote:
> >
> >> Hi Kim,
> > Hi Robert,
> >
> >> On 29/08/18 14:49, Kim Phillips wrote:
> >>> On Wed, 29 Aug 2018 10:44:23 +0100
> >>> Robert Walker <robert.walker@arm.com> wrote:
> >>>
> >>>> This patch adds support for generating instruction samples from trace of
> >>>> AArch32 programs using the A32 and T32 instruction sets.
> >>>>
> >>>> T32 has variable 2 or 4 byte instruction size, so the conversion between
> >>>> addresses and instruction counts requires extra information from the trace
> >>>> decoder, requiring version 0.9.1 of OpenCSD.  A check for the new struct
> >>>> member has been added to the feature check for OpenCSD.
> >>>>
> >>>> Signed-off-by: Robert Walker <robert.walker@arm.com>
> >>>> ---
> >>> ...
> >>>> +++ b/tools/build/feature/test-libopencsd.c
> >>>> @@ -3,6 +3,13 @@
> >>>>    
> >>>>    int main(void)
> >>>>    {
> >>>> +	/*
> >>>> +	 * Requires ocsd_generic_trace_elem.num_instr_range introduced in
> >>>> +	 * OpenCSD 0.9
> >>> 0.9 != 0.9.1 in the above commit text: which is it?
> >> I'll change it to 0.9.1 if there's another version of the patch (it was
> >> introduced in 0.9, but 0.9.1 has a necessary bug fix)
> >>>> +	 */
> >>>> +	ocsd_generic_trace_elem elem;
> >>>> +	(void)elem.num_instr_range;
> >>>> +
> >>> This breaks building against older versions of OpenCSD, right?
> >>>
> >>>>    	(void)ocsd_get_version();
> >>> Why don't we maintain building against older versions of the library,
> >>> and use the version information to make the decision on whether to use
> >>> the new feature being introduced in this patch?
> >> The intention is to fail the feature detection check if the older
> >> version is installed - perf will still compile, but without the
> >> CoreSight trace support.
> > It should still compile, and with CoreSight trace support, just
> > not support for A32/T32 instruction sets.  The user shouldn't be denied
> > CoreSight trace support if they don't care for A32/T32 ISA support.
> >
> >> OpenCSD is still in development, so new features like this are being
> >> added and it would add a lot of #ifdef mess to the perf code to continue
> >> to support any machines with the old library version installed - there
> > Even adding #ifdefs, that won't survive taking one perf executable
> > built on one machine and running it on another machine with a different
> > version of the OpenCSD library: it'll break inconspicuously, not
> > gracefully!
> 
> perf has a lot of other shared library dependencies (ELF , unwind 
> libraries etc), so moving builds between systems is already fragile.
> 
> > There needs to be a run-time means of working with older versions of
> > the library.
> >
> > Consider checking the sizeof some of the structs?  IIRC, some of the
> > structs sizes changed in the library.  See e.g., the 'size' field of
> > perf_event_attr:
> >
> > size
> >       The size of the perf_event_attr structure for forward/backward
> >       compatibility.  Set this using sizeof(struct perf_event_attr)
> >       to allow the kernel to see the struct size at the time
> >       of compilation.
> >
> > or, likely better, the 'version' and 'compat_version' of the
> > perf_event_mmap_page structure:
> >
> >             struct perf_event_mmap_page {
> >                 __u32 version;        /* version number of this structure */
> >                 __u32 compat_version; /* lowest version this is compat with */
> > 	       ...
> >
> >> will only be a handful of machines affected and it's trivial to upgrade
> >> them (the new Debian packages are available).
> > This is upstream linux, so I don't know how you know only a 'handful'
> > of machines affected, and I wouldn't assume everyone's using Debian.
> >
> > For one, I'd hate to see a single user affected if it isn't necessary,
> > as is in this case - not everyone wants A32/T32 ISA support, and
> > library compatibility needn't be broken.
> >
> > This 'screw compatibility' mentality needs to be dropped *now* if
> > CoreSight support is to have a successful future.
> >
> > Otherwise, I suggest keeping this feature in downstream trees for the
> > 'handful', until the library and perf code are rewritten in a state
> > where they properly interoperate, and do not break each other.
> >
> >> How long would we
> >> continue to support such an older version?
> > What do you mean such an older version?  The project's v0.9.0 commit
> > was on 20 June 2018, the one that's usable - v0.9.1 - has a July 27
> > 2018 commit date!  One month is *not* *old*!
> I mean the 0.8.x version as the old version.
> >>    I also don't see any
> >> precedent for supporting multiple dependent library versions in perf.
> > That's because perf doesn't have a precedent on depending on libraries
> > that flat-out break their own users compatibility across versions ;)
> This patch picks up a new feature that's been added - I notice the 
> feature detection checks for other libraries check a number of features 
> and emit warnings about required versions.
> 
> > Thanks,
> >
> > Kim
> 
> Regards
> 
> Rob
> 
>