Re: [PATCH 0/4] x86/insn: perf tools: Add a few new x86 instructions

[Arnaldo Carvalho de Melo <acme@kernel.org>]

Em Tue, Sep 01, 2015 at 03:16:52PM +0300, Adrian Hunter escreveu:
> On 01/09/15 11:54, Ingo Molnar wrote:
> > * Adrian Hunter <adrian.hunter@intel.com> wrote:
> >> perf tools has a copy of the x86 instruction decoder for decoding
> >> Intel PT. [...]

> > So that's the arch/x86/lib/insn.c instruction length decoder that the kernel uses 
> > for kprobes et al - and the two versions already forked slightly:

> > -#include "inat.h"
> > -#include "insn.h"
> > +#include <asm/inat.h>
> > +#include <asm/insn.h>

> > it would be nice to add a diff check to the perf build, and (non-fatally) warn 
> > during the build if the two versions depart from each other?
 
> I had a go and came up with this.  Arnaldo, Jiri any comments?

It looks ok, but then, if the people doing the original work, i.e.
Masami, IIRC, manages to make these files something shared, then this
becomes moot, right?

We would go back to sharing stuff with the kernel, but this time around
we would be using something that everybody knows is being shared, which
doesn't elliminates the possibility that at some point changes made with
the kernel in mind would break the tools/ using code.

Perhaps it is better to keep copying what we want and introduce
infrastructure to check for differences and warn us as soon as possible
and do the copy, test if it doesn't break what we use, etc.

I.e. we wouldn't be putting any new burden on the "kernel people", i.e.
the burden of having to check that changed they made doesn't break
tools/ living code, nor any out of the blue breakage on tools/
developers when changes are made on the kernel "side".

I.e. have something like what you did, but not limited to these intel-pt
decoder bits, we share more than that :-)

So, I would apply your patch and move forward, at least these intel-pt
bits would be covered, Ingo?

- Arnaldo
 
> diff --git a/tools/perf/util/intel-pt-decoder/Build b/tools/perf/util/intel-pt-decoder/Build
> index 240730d682c1..1b8a32de8504 100644
> --- a/tools/perf/util/intel-pt-decoder/Build
> +++ b/tools/perf/util/intel-pt-decoder/Build
> @@ -6,6 +6,17 @@ inat_tables_maps = util/intel-pt-decoder/x86-opcode-map.txt
>  $(OUTPUT)util/intel-pt-decoder/inat-tables.c: $(inat_tables_script) $(inat_tables_maps)
>  	@$(call echo-cmd,gen)$(AWK) -f $(inat_tables_script) $(inat_tables_maps) > $@ || rm -f $@
>  
> -$(OUTPUT)util/intel-pt-decoder/intel-pt-insn-decoder.o: util/intel-pt-decoder/inat.c $(OUTPUT)util/intel-pt-decoder/inat-tables.c
> +$(OUTPUT)util/intel-pt-decoder/intel-pt-insn-decoder.o: util/intel-pt-decoder/intel-pt-insn-decoder.c util/intel-pt-decoder/inat.c $(OUTPUT)util/intel-pt-decoder/inat-tables.c
> +	@test -d ../../arch/x86 && (( \
> +	diff -B -I'^#include' util/intel-pt-decoder/insn.c ../../arch/x86/lib/insn.c >/dev/null && \
> +	diff -B -I'^#include' util/intel-pt-decoder/inat.c ../../arch/x86/lib/inat.c >/dev/null && \
> +	diff -B util/intel-pt-decoder/x86-opcode-map.txt ../../arch/x86/lib/x86-opcode-map.txt >/dev/null && \
> +	diff -B util/intel-pt-decoder/gen-insn-attr-x86.awk ../../arch/x86/tools/gen-insn-attr-x86.awk >/dev/null && \
> +	diff -B -I'^#include' util/intel-pt-decoder/insn.h ../../arch/x86/include/asm/insn.h >/dev/null && \
> +	diff -B -I'^#include' util/intel-pt-decoder/inat.h ../../arch/x86/include/asm/inat.h >/dev/null && \
> +	diff -B -I'^#include' util/intel-pt-decoder/inat_types.h ../../arch/x86/include/asm/inat_types.h >/dev/null) \
> +	|| echo "Warning: Intel PT: x86 instruction decoder differs from kernel" >&2 )
> +	$(call rule_mkdir)
> +	$(call if_changed_dep,cc_o_c)
>  
>  CFLAGS_intel-pt-insn-decoder.o += -I$(OUTPUT)util/intel-pt-decoder -Wno-override-init
>