On Fri, 31 Aug 2018 at 08:43, Kim Phillips [off-list ref] wrote:

On Fri, 31 Aug 2018 14:42:00 +0100
Robert Walker [off-list ref] wrote:

quoted

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.

quoted

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.

While it is not realistic to expect eternal backward compatibility for
core features, I think in this case we can do better.

Looking at the code in the patch it seems possible to implement the
new functionality in functions that can be enabled based on the
library version we compile against.  If the library version is not
high enough, the functions simply get stubbed out and the feature
isn't available.

A prerequisite step would be to move openCSD's
"decoder/include/ocsd_if_version.h" under
"decoder/include/opencsd/ocsd_if_version.h" in order to make symbols
OCSD_VER_{ MAJOR | MINOW | PATH } accessible but that's not a big
deal.

Please try this approach first - we can consider more drastic measures
if that doesn't work.

quoted

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.).

quoted

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.

quoted

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.

quoted

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.

quoted

 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

quoted

On 29/08/18 17:32, Kim Phillips wrote:

quoted

On Wed, 29 Aug 2018 15:34:16 +0100
Robert Walker [off-list ref] wrote:

quoted

Hi Kim,

Hi Robert,

quoted

On 29/08/18 14:49, Kim Phillips wrote:

quoted

On Wed, 29 Aug 2018 10:44:23 +0100
Robert Walker [off-list ref] wrote:

quoted

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 <redacted>
---

...

quoted

+++ 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)

quoted

quoted

+         */
+        ocsd_generic_trace_elem elem;
+        (void)elem.num_instr_range;
+

This breaks building against older versions of OpenCSD, right?

quoted

         (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.

quoted

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.

quoted

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 */
           ...

quoted

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.

quoted

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.

quoted

quoted

   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.

quoted

Thanks,

Kim

Regards

Rob