Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1758618AbcCCTBs (ORCPT ); Thu, 3 Mar 2016 14:01:48 -0500 Received: from torg.zytor.com ([198.137.202.12]:50886 "EHLO mail.zytor.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1758384AbcCCTBq (ORCPT ); Thu, 3 Mar 2016 14:01:46 -0500 User-Agent: K-9 Mail for Android In-Reply-To: References: <9dd4161719b20e6def9564646d68bfbe498c549f.1456962210.git.jpoimboe@redhat.com> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit Content-Type: text/plain; charset=UTF-8 Subject: Re: [tip:core/objtool] x86/asm/decoder: Use explicitly signed chars From: "H. Peter Anvin" Date: Thu, 03 Mar 2016 11:00:26 -0800 To: linux-tip-commits@vger.kernel.org, tip-bot for Josh Poimboeuf CC: mingo@kernel.org, sfr@canb.auug.org.au, linux-kernel@vger.kernel.org, tglx@linutronix.de, mpe@ellerman.id.au, masami.hiramatsu.pt@hitachi.com, adrian.hunter@intel.com, jpoimboe@redhat.com, torvalds@linux-foundation.org, peterz@infradead.org Message-ID: Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 6151 Lines: 160 On March 3, 2016 8:51:39 AM PST, tip-bot for Josh Poimboeuf wrote: >Commit-ID: 19072f23d1d785c093b7f81cb1fb161e7a13ecc0 >Gitweb: >http://git.kernel.org/tip/19072f23d1d785c093b7f81cb1fb161e7a13ecc0 >Author: Josh Poimboeuf >AuthorDate: Wed, 2 Mar 2016 18:39:36 -0600 >Committer: Ingo Molnar >CommitDate: Thu, 3 Mar 2016 16:13:00 +0100 > >x86/asm/decoder: Use explicitly signed chars > >When running objtool on a ppc64le host to analyze x86 binaries, it >reports a lot of false warnings like: > >ipc/compat_mq.o: warning: objtool: compat_SyS_mq_open()+0x91: can't >find jump dest instruction at .text+0x3a5 > >The warnings are caused by the x86 instruction decoder setting the >wrong >value for the jump instruction's immediate field because it assumes >that >"char == signed char", which isn't true for all architectures. When >converting char to int, gcc sign-extends on x86 but doesn't sign-extend >on ppc64le. > >According to the gcc man page, that's a feature, not a bug: > > > Each kind of machine has a default for what "char" should be. It is > > either like "unsigned char" by default or like "signed char" by > > default. > > > > Ideally, a portable program should always use "signed char" or > > "unsigned char" when it depends on the signedness of an object. > >Conform to the "standards" by changing the "char" casts to "signed >char". This results in no actual changes to the object code on x86. > >Note: the x86 decoder now lives in three different locations in the >kernel tree, which are all kept in sync via makefile checks and >warnings: in-kernel, perf, and objtool. This fixes all three >locations. >Eventually we should probably try to at least converge the two separate >"tools" locations into a single shared location. > >Signed-off-by: Josh Poimboeuf >Cc: Adrian Hunter >Cc: Linus Torvalds >Cc: Masami Hiramatsu >Cc: Michael Ellerman >Cc: Peter Zijlstra >Cc: Stephen Rothwell >Cc: Thomas Gleixner >Link: >http://lkml.kernel.org/r/9dd4161719b20e6def9564646d68bfbe498c549f.1456962210.git.jpoimboe@redhat.com >Signed-off-by: Ingo Molnar >--- > arch/x86/lib/insn.c | 6 +++--- > tools/objtool/arch/x86/insn/insn.c | 6 +++--- > tools/perf/util/intel-pt-decoder/insn.c | 6 +++--- > 3 files changed, 9 insertions(+), 9 deletions(-) > >diff --git a/arch/x86/lib/insn.c b/arch/x86/lib/insn.c >index 8f72b33..1a41693 100644 >--- a/arch/x86/lib/insn.c >+++ b/arch/x86/lib/insn.c >@@ -374,7 +374,7 @@ void insn_get_displacement(struct insn *insn) > if (mod == 3) > goto out; > if (mod == 1) { >- insn->displacement.value = get_next(char, insn); >+ insn->displacement.value = get_next(signed char, insn); > insn->displacement.nbytes = 1; > } else if (insn->addr_bytes == 2) { > if ((mod == 0 && rm == 6) || mod == 2) { >@@ -532,7 +532,7 @@ void insn_get_immediate(struct insn *insn) > > switch (inat_immediate_size(insn->attr)) { > case INAT_IMM_BYTE: >- insn->immediate.value = get_next(char, insn); >+ insn->immediate.value = get_next(signed char, insn); > insn->immediate.nbytes = 1; > break; > case INAT_IMM_WORD: >@@ -566,7 +566,7 @@ void insn_get_immediate(struct insn *insn) > goto err_out; > } > if (inat_has_second_immediate(insn->attr)) { >- insn->immediate2.value = get_next(char, insn); >+ insn->immediate2.value = get_next(signed char, insn); > insn->immediate2.nbytes = 1; > } > done: >diff --git a/tools/objtool/arch/x86/insn/insn.c >b/tools/objtool/arch/x86/insn/insn.c >index 47314a6..9f26eae 100644 >--- a/tools/objtool/arch/x86/insn/insn.c >+++ b/tools/objtool/arch/x86/insn/insn.c >@@ -374,7 +374,7 @@ void insn_get_displacement(struct insn *insn) > if (mod == 3) > goto out; > if (mod == 1) { >- insn->displacement.value = get_next(char, insn); >+ insn->displacement.value = get_next(signed char, insn); > insn->displacement.nbytes = 1; > } else if (insn->addr_bytes == 2) { > if ((mod == 0 && rm == 6) || mod == 2) { >@@ -532,7 +532,7 @@ void insn_get_immediate(struct insn *insn) > > switch (inat_immediate_size(insn->attr)) { > case INAT_IMM_BYTE: >- insn->immediate.value = get_next(char, insn); >+ insn->immediate.value = get_next(signed char, insn); > insn->immediate.nbytes = 1; > break; > case INAT_IMM_WORD: >@@ -566,7 +566,7 @@ void insn_get_immediate(struct insn *insn) > goto err_out; > } > if (inat_has_second_immediate(insn->attr)) { >- insn->immediate2.value = get_next(char, insn); >+ insn->immediate2.value = get_next(signed char, insn); > insn->immediate2.nbytes = 1; > } > done: >diff --git a/tools/perf/util/intel-pt-decoder/insn.c >b/tools/perf/util/intel-pt-decoder/insn.c >index 47314a6..9f26eae 100644 >--- a/tools/perf/util/intel-pt-decoder/insn.c >+++ b/tools/perf/util/intel-pt-decoder/insn.c >@@ -374,7 +374,7 @@ void insn_get_displacement(struct insn *insn) > if (mod == 3) > goto out; > if (mod == 1) { >- insn->displacement.value = get_next(char, insn); >+ insn->displacement.value = get_next(signed char, insn); > insn->displacement.nbytes = 1; > } else if (insn->addr_bytes == 2) { > if ((mod == 0 && rm == 6) || mod == 2) { >@@ -532,7 +532,7 @@ void insn_get_immediate(struct insn *insn) > > switch (inat_immediate_size(insn->attr)) { > case INAT_IMM_BYTE: >- insn->immediate.value = get_next(char, insn); >+ insn->immediate.value = get_next(signed char, insn); > insn->immediate.nbytes = 1; > break; > case INAT_IMM_WORD: >@@ -566,7 +566,7 @@ void insn_get_immediate(struct insn *insn) > goto err_out; > } > if (inat_has_second_immediate(insn->attr)) { >- insn->immediate2.value = get_next(char, insn); >+ insn->immediate2.value = get_next(signed char, insn); > insn->immediate2.nbytes = 1; > } > done: It ought to be made specific as __s8 (or int8_t) really... -- Sent from my Android device with K-9 Mail. Please excuse brevity and formatting.