Hi Robert,
On Mon, 1 Oct 2018 at 14:17, Mathieu Poirier mathieu.poirier@linaro.org wrote:
On Mon, 1 Oct 2018 at 13:24, Robert Walker robert.walker@arm.com wrote:
Hi Mathieu,
On 01/10/18 19:00, Mathieu Poirier wrote:
Hi Robert and thanks for this second revision.
On Thu, 27 Sep 2018 at 04:24, 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.0 of OpenCSD. A check for the new version member has been added to the feature check for OpenCSD. Where only the older version of OpenCSD is available, the previous behaviour of assuming 4 byte instruction size is used.
Signed-off-by: Robert Walker robert.walker@arm.com
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 | 58 ++++++++++++++++++++ tools/perf/util/cs-etm-decoder/cs-etm-decoder.h | 10 ++++ tools/perf/util/cs-etm.c | 71 +++++++++++-------------- 7 files changed, 123 insertions(+), 41 deletions(-) create mode 100644 tools/build/feature/test-libopencsd-numinstr.c
diff --git a/tools/build/Makefile.feature b/tools/build/Makefile.feature index f216b2f..5e8d108 100644 --- a/tools/build/Makefile.feature +++ b/tools/build/Makefile.feature @@ -68,7 +68,8 @@ FEATURE_TESTS_BASIC := \ sched_getcpu \ sdt \ setns \
libopencsd
libopencsd \libopencsd-numinstrI understand what you're doing here but it won't fly with the upstream guy. The new #define in the openCSD library needs to come before this patchset. That way functionality that needs the newest version of the library can be conditionally compiled without this extra step and a new HAVE_CSTRACE_INSTR_INFO.
So just to be clear:
- We do a new release of OpenCSD (0.9.3?) that exports some macros
describing its version. You previously suggested including of the ocsd_if_version.h header in the OpenCSD install - we can't do a '#include "opencsd/ocsd_if_version.h"' directly from the perf code without breaking compilation when using the older library versions without the header (0.9.2 or older), so maybe opencsd/c_api/opencsd_c_api.h would be a good place to put the macros (or include ocsd_if_version.h from).
Yes, that makes sense to me.
- We don't need to make any changes to the feature detection in perf
Correct.
- util/cs-etm-decoder/cs-etm-decoder.c can use the version macros to
determine whether to set HAVE_CSTRACE_INSTR_INFO within that file and hence whether to use the extra info available from the decoder.
I was hoping for something like this instead of a new define:
# if OCSD_VER_MINOR >=9 && OCSD_VER_PATCH > 2 ....
#else ....
#endif
In light of more discussion on IRC and Leo Yan's work on perf sampling, the consensus is that too many features are still being added to the openCSD library to be concerned with backward compatibility. We can revisit that line of thought at a later time but for the moment asking the perf tools be compiled with the latest code should be the way to go.
As such your original patchset was close to the final solution. Just checking for the right library version in test-libopencsd.c should do the trick.
Apologies for the change in direction, doing the right thing isn't always an exact science.
Regards, Mathieu