Re: [tip:core/objtool] x86/asm/decoder: Use explicitly signed chars

[H. Peter Anvin]

On March 3, 2016 8:51:39 AM PST, tip-bot for Josh Poimboeuf <tipbot@xxxxxxxxx> wrote:
>Commit-ID:  19072f23d1d785c093b7f81cb1fb161e7a13ecc0
>Gitweb:    
>http://git.kernel.org/tip/19072f23d1d785c093b7f81cb1fb161e7a13ecc0
>Author:     Josh Poimboeuf <jpoimboe@xxxxxxxxxx>
>AuthorDate: Wed, 2 Mar 2016 18:39:36 -0600
>Committer:  Ingo Molnar <mingo@xxxxxxxxxx>
>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 <jpoimboe@xxxxxxxxxx>
>Cc: Adrian Hunter <adrian.hunter@xxxxxxxxx>
>Cc: Linus Torvalds <torvalds@xxxxxxxxxxxxxxxxxxxx>
>Cc: Masami Hiramatsu <masami.hiramatsu.pt@xxxxxxxxxxx>
>Cc: Michael Ellerman <mpe@xxxxxxxxxxxxxx>
>Cc: Peter Zijlstra <peterz@xxxxxxxxxxxxx>
>Cc: Stephen Rothwell <sfr@xxxxxxxxxxxxxxxx>
>Cc: Thomas Gleixner <tglx@xxxxxxxxxxxxx>
>Link:
>http://lkml.kernel.org/r/9dd4161719b20e6def9564646d68bfbe498c549f.1456962210.git.jpoimboe@xxxxxxxxxx
>Signed-off-by: Ingo Molnar <mingo@xxxxxxxxxx>
>---
> 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.