On 27/09/18 14:39, Mike Leach wrote:
Hi Rob, On Thu, 27 Sep 2018 at 11:24, Robert Walker robert.walker@arm.com wrote:
Hi,
I'm taking this back to the linaro coresight list so we can get the OpenCSD library versioning sorted out.
The first patch splits the OpenCSD feature check into two parts. The original check is left as is - this just checks for the presence of an OpenCSD library. A new check (libopencsd-numinstr) is added that checks for the new OpenCSD (>0.9.0) that has the num_instr_range member in the ocsd_generic_trace_elem struct. This feature is then used to set a flag used in cs-etm-decoder.c to select which versions of 2 functions are used to get the instruction count / last instruction size of each instruction block - if the flag is not set, then the previous assumptions of a 4 byte instruction size are used. It was suggested that OpenCSD should export a version header - I agree this is a good idea, but this will require a new release of the library, so we would miss support for the instruction sizes when OpenCSD 0.9.{0,1,2} is installed - hence why I've kept the version check using the presence of num_instr_range.
I agree with the version check using the num_instr range for now. But my view is that we should simply fail to build if the version of the library is insufficient for the current set of perf features - with an appropriate warning of course.
The second patch adds support for finding the T32 instruction counts when the OpenCSD library doesn't report the instruction counts. As this involves iterating through the block of instructions and examining each instruction, there is a significant peformance hit (about 5x slower than using the OpenCSD library to report the instruction counts), so I'm not sure this patch should go into upstream.
I don't think that is should - what is the advantage of building a new version of perf against an old version of the OpenCSD library?
If a user builds a version of perf without these patches against the new library then it will work - that's fine. With the patches then we should require the correct library version.
The problem we cannot solve at this point is a user taking a perf built against one version (e.g. 0.8.x,) and running it against the other (0.9.x). The executable links against libopencsd.0
Once we up grade the version to 1.0.0 and beyond, then we will have to be more careful about breaking changes - these will have to rev the major version number in future.
Mike
The previous version of this patch did fail the build if OpenCSD 0.9.x wasn't available - feedback at the time was that we should try to support those users with only the older version of OpenCSD available (although I agree updating OpenCSD is the better option). The configuration checks added here would allow us to fail the build with a more detailed error message ("Old version of OpenCSD detected, but 0.9.x is required")
Regards
Rob
Regards
Rob
Robert Walker (2): perf: Support for Arm A32/T32 instruction sets in CoreSight trace perf: Full support for Arm T32 instructions with older version of OpenCSD
tools/build/Makefile.feature | 3 +- tools/build/feature/Makefile | 4 + tools/build/feature/test-libopencsd-numinstr.c | 15 ++++ tools/perf/Makefile.config | 3 + tools/perf/util/cs-etm-decoder/cs-etm-decoder.c | 106 ++++++++++++++++++++++++ tools/perf/util/cs-etm-decoder/cs-etm-decoder.h | 10 +++ tools/perf/util/cs-etm.c | 71 +++++++--------- 7 files changed, 171 insertions(+), 41 deletions(-) create mode 100644 tools/build/feature/test-libopencsd-numinstr.c
-- 2.7.4
CoreSight mailing list CoreSight@lists.linaro.org https://lists.linaro.org/mailman/listinfo/coresight