From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754054AbbIAN4i (ORCPT ); Tue, 1 Sep 2015 09:56:38 -0400 Received: from mail.kernel.org ([198.145.29.136]:47628 "EHLO mail.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752886AbbIAN4h (ORCPT ); Tue, 1 Sep 2015 09:56:37 -0400 Date: Tue, 1 Sep 2015 10:56:32 -0300 From: Arnaldo Carvalho de Melo To: Adrian Hunter Cc: Ingo Molnar , Jiri Olsa , linux-kernel@vger.kernel.org, Andy Lutomirski , Masami Hiramatsu , Denys Vlasenko , Peter Zijlstra , Dave Hansen , Qiaowei Ren , "H. Peter Anvin" , Thomas Gleixner Subject: Re: [PATCH 0/4] x86/insn: perf tools: Add a few new x86 instructions Message-ID: <20150901135632.GB29821@kernel.org> References: <1441029522-20235-1-git-send-email-adrian.hunter@intel.com> <20150901085432.GE6315@gmail.com> <55E59734.4080205@intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <55E59734.4080205@intel.com> X-Url: http://acmel.wordpress.com User-Agent: Mutt/1.5.23 (2014-03-12) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.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 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 > > +#include > > 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 >