Skip to contentSkip to conversation

objtool warning fixes

PATCH New Josh Poimboeuf 2 participants 7 messages Jul 29, 2016 objtool

lore
Share & tools
lore
#0 lore raw

cover objtool warning fixes

Fixes for some objtool-related warnings reported by Linus.

Josh Poimboeuf (3):
objtool: support new gcc 6 switch jump table pattern
objtool: resync x86 instruction decoder with the kernel's
objtool: un-capitalize "Warning" for out-of-sync instruction decoder

 tools/objtool/Makefile                            |   2 +-
 tools/objtool/arch/x86/insn/gen-insn-attr-x86.awk |  11 +-
 tools/objtool/arch/x86/insn/inat.h                |  17 +-
 tools/objtool/arch/x86/insn/insn.c                |  18 +-
 tools/objtool/arch/x86/insn/insn.h                |  12 +-
 tools/objtool/arch/x86/insn/x86-opcode-map.txt    | 263 ++++++++++++++--------
 tools/objtool/builtin-check.c                     | 140 +++++++-----
 7 files changed, 309 insertions(+), 154 deletions(-)
Signature · 1 line
2.7.4
#1 lore raw

PATCH 1/3 objtool: support new gcc 6 switch jump table pattern

This fixes some false positive objtool warnings seen with gcc 6.1.1:

kernel/trace/ring_buffer.o: warning: objtool: ring_buffer_read_page()+0x36c: sibling call from callable instruction with changed frame pointer arch/x86/kernel/reboot.o: warning: objtool: native_machine_emergency_restart()+0x139: sibling call from callable instruction with changed frame pointer lib/xz/xz_dec_stream.o: warning: objtool: xz_dec_run()+0xc2: sibling call from callable instruction with changed frame pointer

With gcc 6, a new code pattern is sometimes used to access a switch statement jump table in .rodata, which objtool doesn't yet recognize:

mov [rodata addr],%reg1
... some instructions ...
jmpq *(%reg1,%reg2,8)

Add support for detecting that pattern. The detection code is rather crude, but it's still effective at weeding out false positives and catching real warnings. It can be refined later once objtool starts reading DWARF CFI.

---
 tools/objtool/builtin-check.c | 140 ++++++++++++++++++++++++++----------------
 1 file changed, 88 insertions(+), 52 deletions(-)
tools/objtool/builtin-check.c +88 -52
index 17fa7fc..8d1296a 100644
@@ -107,6 +107,12 @@ static struct instruction *next_insn_same_sec(struct objtool_file *file,
 		insn->offset < func->offset + func->len;		\
 	     insn = list_next_entry(insn, list))
 
+#define func_for_each_insn_continue_reverse(file, func, insn)		\
+	for (insn = list_prev_entry(insn, list);			\
+	     &insn->list != &file->insn_list &&				\
+		insn->sec == func->sec && insn->offset >= func->offset;	\
+	     insn = list_prev_entry(insn, list))
+
 #define sec_for_each_insn_from(file, insn)				\
 	for (; insn; insn = next_insn_same_sec(file, insn))
 
@@ -664,65 +670,95 @@ static int add_switch_table(struct objtool_file *file, struct symbol *func,
 	return 0;
 }
 
-static int add_func_switch_tables(struct objtool_file *file,
-				  struct symbol *func)
+/*
+ * find_switch_table() - Given a dynamic jump, find the switch jump table in
+ * .rodata associated with it.
+ *
+ * There are 3 basic patterns:
+ *
+ * 1. jmpq *[rodata addr](,%reg,8)
+ *
+ *    This is the most common case by far.  It jumps to an address in a simple
+ *    jump table which is stored in .rodata.
+ *
+ * 2. jmpq *[rodata addr](%rip)
+ *
+ *    This is caused by a rare gcc quirk, currently only seen in three driver
+ *    functions in the kernel, only with certain obscure non-distro configs.
+ *
+ *    As part of an optimization, gcc makes a copy of an existing switch jump
+ *    table, modifies it, and then hard-codes the jump (albeit with an indirect
+ *    jump) to use a single entry in the table.  The rest of the jump table and
+ *    some of its jump targets remain as dead code.
+ *
+ *    In such a case we can just crudely ignore all unreachable instruction
+ *    warnings for the entire object file.  Ideally we would just ignore them
+ *    for the function, but that would require redesigning the code quite a
+ *    bit.  And honestly that's just not worth doing: unreachable instruction
+ *    warnings are of questionable value anyway, and this is such a rare issue.
+ *
+ * 3. mov [rodata addr],%reg1
+ *    ... some instructions ...
+ *    jmpq *(%reg1,%reg2,8)
+ *
+ *    This is a fairly uncommon pattern which is new for gcc 6.  As of this
+ *    writing, there are 11 occurrences of it in the allmodconfig kernel.
+ *
+ *    TODO: Once we have DWARF CFI and smarter instruction decoding logic,
+ *    ensure the same register is used in the mov and jump instructions.
+ */
+static struct rela *find_switch_table(struct objtool_file *file,
+				      struct symbol *func,
+				      struct instruction *insn)
 {
-	struct instruction *insn, *prev_jump;
-	struct rela *text_rela, *rodata_rela, *prev_rela = NULL;
-	int ret;
+	struct rela *text_rela, *rodata_rela;
 
-	prev_jump = NULL;
+	text_rela = find_rela_by_dest_range(insn->sec, insn->offset, insn->len);
+	if (text_rela && text_rela->sym == file->rodata->sym) {
+		/* case 1 */
+		rodata_rela = find_rela_by_dest(file->rodata,
+						text_rela->addend);
+		if (rodata_rela)
+			return rodata_rela;
 
-	func_for_each_insn(file, func, insn) {
-		if (insn->type != INSN_JUMP_DYNAMIC)
-			continue;
+		/* case 2 */
+		rodata_rela = find_rela_by_dest(file->rodata,
+						text_rela->addend + 4);
+		if (!rodata_rela)
+			return NULL;
+		file->ignore_unreachables = true;
+		return rodata_rela;
+	}
+
+	/* case 3 */
+	func_for_each_insn_continue_reverse(file, func, insn) {
+		if (insn->type == INSN_JUMP_UNCONDITIONAL ||
+		    insn->type == INSN_JUMP_DYNAMIC)
+			break;
 
 		text_rela = find_rela_by_dest_range(insn->sec, insn->offset,
 						    insn->len);
-		if (!text_rela || text_rela->sym != file->rodata->sym)
-			continue;
+		if (text_rela && text_rela->sym == file->rodata->sym)
+			return find_rela_by_dest(file->rodata,
+						 text_rela->addend);
+	}
 
-		/* common case: jmpq *[addr](,%rax,8) */
-		rodata_rela = find_rela_by_dest(file->rodata,
-						text_rela->addend);
+	return NULL;
+}
 
-		/*
-		 * rare case:   jmpq *[addr](%rip)
-		 *
-		 * This check is for a rare gcc quirk, currently only seen in
-		 * three driver functions in the kernel, only with certain
-		 * obscure non-distro configs.
-		 *
-		 * As part of an optimization, gcc makes a copy of an existing
-		 * switch jump table, modifies it, and then hard-codes the jump
-		 * (albeit with an indirect jump) to use a single entry in the
-		 * table.  The rest of the jump table and some of its jump
-		 * targets remain as dead code.
-		 *
-		 * In such a case we can just crudely ignore all unreachable
-		 * instruction warnings for the entire object file.  Ideally we
-		 * would just ignore them for the function, but that would
-		 * require redesigning the code quite a bit.  And honestly
-		 * that's just not worth doing: unreachable instruction
-		 * warnings are of questionable value anyway, and this is such
-		 * a rare issue.
-		 *
-		 * kbuild reports:
-		 * - https://lkml.kernel.org/r/201603231906.LWcVUpxm%[email protected]
-		 * - https://lkml.kernel.org/r/201603271114.K9i45biy%[email protected]
-		 * - https://lkml.kernel.org/r/201603291058.zuJ6ben1%[email protected]
-		 *
-		 * gcc bug:
-		 * - https://gcc.gnu.org/bugzilla/show_bug.cgi?id=70604
-		 */
-		if (!rodata_rela) {
-			rodata_rela = find_rela_by_dest(file->rodata,
-							text_rela->addend + 4);
-			if (rodata_rela)
-				file->ignore_unreachables = true;
-		}
+static int add_func_switch_tables(struct objtool_file *file,
+				  struct symbol *func)
+{
+	struct instruction *insn, *prev_jump = NULL;
+	struct rela *rela, *prev_rela = NULL;
+	int ret;
 
-		if (!rodata_rela)
+	func_for_each_insn(file, func, insn) {
+		if (insn->type != INSN_JUMP_DYNAMIC)
+			continue;
+
+		rela = find_switch_table(file, func, insn);
+		if (!rela)
 			continue;
 
 		/*
@@ -732,13 +768,13 @@ static int add_func_switch_tables(struct objtool_file *file,
 		 */
 		if (prev_jump) {
 			ret = add_switch_table(file, func, prev_jump, prev_rela,
-					       rodata_rela);
+					       rela);
 			if (ret)
 				return ret;
 		}
 
 		prev_jump = insn;
-		prev_rela = rodata_rela;
+		prev_rela = rela;
 	}
 
 	if (prev_jump) {
Signature · 1 line
2.7.4
#2 lore raw

[tip:core/urgent] objtool: Support new GCC 6 switch jump table pattern

objtool: Support new GCC 6 switch jump table pattern
Commit-ID:  6d01f28bc08814abc7b8f8973cc71195537f4c80
Gitweb:     http://git.kernel.org/tip/6d01f28bc08814abc7b8f8973cc71195537f4c80
Author:     Josh Poimboeuf <[email protected]>
AuthorDate: Thu, 28 Jul 2016 19:14:58 -0500
Committer:  Ingo Molnar <[email protected]>
CommitDate: Fri, 29 Jul 2016 16:56:21 +0200

objtool: Support new GCC 6 switch jump table pattern

This fixes some false positive objtool warnings seen with gcc 6.1.1:

kernel/trace/ring_buffer.o: warning: objtool: ring_buffer_read_page()+0x36c: sibling call from callable instruction with changed frame pointer arch/x86/kernel/reboot.o: warning: objtool: native_machine_emergency_restart()+0x139: sibling call from callable instruction with changed frame pointer lib/xz/xz_dec_stream.o: warning: objtool: xz_dec_run()+0xc2: sibling call from callable instruction with changed frame pointer

With GCC 6, a new code pattern is sometimes used to access a switch statement jump table in .rodata, which objtool doesn't yet recognize:

mov [rodata addr],%reg1
... some instructions ...
jmpq *(%reg1,%reg2,8)

Add support for detecting that pattern. The detection code is rather crude, but it's still effective at weeding out false positives and catching real warnings. It can be refined later once objtool starts reading DWARF CFI.

---
 tools/objtool/builtin-check.c | 140 ++++++++++++++++++++++++++----------------
 1 file changed, 88 insertions(+), 52 deletions(-)
tools/objtool/builtin-check.c +88 -52
index 17fa7fc..bd09d0e 100644
@@ -107,6 +107,12 @@ static struct instruction *next_insn_same_sec(struct objtool_file *file,
 		insn->offset < func->offset + func->len;		\
 	     insn = list_next_entry(insn, list))
 
+#define func_for_each_insn_continue_reverse(file, func, insn)		\
+	for (insn = list_prev_entry(insn, list);			\
+	     &insn->list != &file->insn_list &&				\
+		insn->sec == func->sec && insn->offset >= func->offset;	\
+	     insn = list_prev_entry(insn, list))
+
 #define sec_for_each_insn_from(file, insn)				\
 	for (; insn; insn = next_insn_same_sec(file, insn))
 
@@ -664,65 +670,95 @@ static int add_switch_table(struct objtool_file *file, struct symbol *func,
 	return 0;
 }
 
-static int add_func_switch_tables(struct objtool_file *file,
-				  struct symbol *func)
+/*
+ * find_switch_table() - Given a dynamic jump, find the switch jump table in
+ * .rodata associated with it.
+ *
+ * There are 3 basic patterns:
+ *
+ * 1. jmpq *[rodata addr](,%reg,8)
+ *
+ *    This is the most common case by far.  It jumps to an address in a simple
+ *    jump table which is stored in .rodata.
+ *
+ * 2. jmpq *[rodata addr](%rip)
+ *
+ *    This is caused by a rare GCC quirk, currently only seen in three driver
+ *    functions in the kernel, only with certain obscure non-distro configs.
+ *
+ *    As part of an optimization, GCC makes a copy of an existing switch jump
+ *    table, modifies it, and then hard-codes the jump (albeit with an indirect
+ *    jump) to use a single entry in the table.  The rest of the jump table and
+ *    some of its jump targets remain as dead code.
+ *
+ *    In such a case we can just crudely ignore all unreachable instruction
+ *    warnings for the entire object file.  Ideally we would just ignore them
+ *    for the function, but that would require redesigning the code quite a
+ *    bit.  And honestly that's just not worth doing: unreachable instruction
+ *    warnings are of questionable value anyway, and this is such a rare issue.
+ *
+ * 3. mov [rodata addr],%reg1
+ *    ... some instructions ...
+ *    jmpq *(%reg1,%reg2,8)
+ *
+ *    This is a fairly uncommon pattern which is new for GCC 6.  As of this
+ *    writing, there are 11 occurrences of it in the allmodconfig kernel.
+ *
+ *    TODO: Once we have DWARF CFI and smarter instruction decoding logic,
+ *    ensure the same register is used in the mov and jump instructions.
+ */
+static struct rela *find_switch_table(struct objtool_file *file,
+				      struct symbol *func,
+				      struct instruction *insn)
 {
-	struct instruction *insn, *prev_jump;
-	struct rela *text_rela, *rodata_rela, *prev_rela = NULL;
-	int ret;
+	struct rela *text_rela, *rodata_rela;
 
-	prev_jump = NULL;
+	text_rela = find_rela_by_dest_range(insn->sec, insn->offset, insn->len);
+	if (text_rela && text_rela->sym == file->rodata->sym) {
+		/* case 1 */
+		rodata_rela = find_rela_by_dest(file->rodata,
+						text_rela->addend);
+		if (rodata_rela)
+			return rodata_rela;
 
-	func_for_each_insn(file, func, insn) {
-		if (insn->type != INSN_JUMP_DYNAMIC)
-			continue;
+		/* case 2 */
+		rodata_rela = find_rela_by_dest(file->rodata,
+						text_rela->addend + 4);
+		if (!rodata_rela)
+			return NULL;
+		file->ignore_unreachables = true;
+		return rodata_rela;
+	}
+
+	/* case 3 */
+	func_for_each_insn_continue_reverse(file, func, insn) {
+		if (insn->type == INSN_JUMP_UNCONDITIONAL ||
+		    insn->type == INSN_JUMP_DYNAMIC)
+			break;
 
 		text_rela = find_rela_by_dest_range(insn->sec, insn->offset,
 						    insn->len);
-		if (!text_rela || text_rela->sym != file->rodata->sym)
-			continue;
+		if (text_rela && text_rela->sym == file->rodata->sym)
+			return find_rela_by_dest(file->rodata,
+						 text_rela->addend);
+	}
 
-		/* common case: jmpq *[addr](,%rax,8) */
-		rodata_rela = find_rela_by_dest(file->rodata,
-						text_rela->addend);
+	return NULL;
+}
 
-		/*
-		 * rare case:   jmpq *[addr](%rip)
-		 *
-		 * This check is for a rare gcc quirk, currently only seen in
-		 * three driver functions in the kernel, only with certain
-		 * obscure non-distro configs.
-		 *
-		 * As part of an optimization, gcc makes a copy of an existing
-		 * switch jump table, modifies it, and then hard-codes the jump
-		 * (albeit with an indirect jump) to use a single entry in the
-		 * table.  The rest of the jump table and some of its jump
-		 * targets remain as dead code.
-		 *
-		 * In such a case we can just crudely ignore all unreachable
-		 * instruction warnings for the entire object file.  Ideally we
-		 * would just ignore them for the function, but that would
-		 * require redesigning the code quite a bit.  And honestly
-		 * that's just not worth doing: unreachable instruction
-		 * warnings are of questionable value anyway, and this is such
-		 * a rare issue.
-		 *
-		 * kbuild reports:
-		 * - https://lkml.kernel.org/r/201603231906.LWcVUpxm%[email protected]
-		 * - https://lkml.kernel.org/r/201603271114.K9i45biy%[email protected]
-		 * - https://lkml.kernel.org/r/201603291058.zuJ6ben1%[email protected]
-		 *
-		 * gcc bug:
-		 * - https://gcc.gnu.org/bugzilla/show_bug.cgi?id=70604
-		 */
-		if (!rodata_rela) {
-			rodata_rela = find_rela_by_dest(file->rodata,
-							text_rela->addend + 4);
-			if (rodata_rela)
-				file->ignore_unreachables = true;
-		}
+static int add_func_switch_tables(struct objtool_file *file,
+				  struct symbol *func)
+{
+	struct instruction *insn, *prev_jump = NULL;
+	struct rela *rela, *prev_rela = NULL;
+	int ret;
 
-		if (!rodata_rela)
+	func_for_each_insn(file, func, insn) {
+		if (insn->type != INSN_JUMP_DYNAMIC)
+			continue;
+
+		rela = find_switch_table(file, func, insn);
+		if (!rela)
 			continue;
 
 		/*
@@ -732,13 +768,13 @@ static int add_func_switch_tables(struct objtool_file *file,
 		 */
 		if (prev_jump) {
 			ret = add_switch_table(file, func, prev_jump, prev_rela,
-					       rodata_rela);
+					       rela);
 			if (ret)
 				return ret;
 		}
 
 		prev_jump = insn;
-		prev_rela = rodata_rela;
+		prev_rela = rela;
 	}
 
 	if (prev_jump) {
#3 lore raw

PATCH 2/3 objtool: resync x86 instruction decoder with the kernel's

In reply to Josh Poimboeuf

This fixes the following warning:

Warning: objtool: x86 instruction decoder differs from kernel

Unfortunately we have three identical copies of the x86 instruction decoder in the kernel tree that have to be manually kept in sync.

It's on my TODO list to at least library-ize the ones in the tools subdir so we'd only have two of them instead of three. In the meantime, here's another manual sync.

Fixes c61f4d5ebaf0 ("perf tools: Add AVX-512 support to the instruction decoder used by Intel PT")
---
 tools/objtool/arch/x86/insn/gen-insn-attr-x86.awk |  11 +-
 tools/objtool/arch/x86/insn/inat.h                |  17 +-
 tools/objtool/arch/x86/insn/insn.c                |  18 +-
 tools/objtool/arch/x86/insn/insn.h                |  12 +-
 tools/objtool/arch/x86/insn/x86-opcode-map.txt    | 263 ++++++++++++++--------
 5 files changed, 220 insertions(+), 101 deletions(-)
tools/objtool/arch/x86/insn/gen-insn-attr-x86.awk +8 -3
index 093a892..a3d2c62 100644
@@ -72,12 +72,14 @@ BEGIN {
 	lprefix_expr = "\\((66|F2|F3)\\)"
 	max_lprefix = 4
 
-	# All opcodes starting with lower-case 'v' or with (v1) superscript
+	# All opcodes starting with lower-case 'v', 'k' or with (v1) superscript
 	# accepts VEX prefix
-	vexok_opcode_expr = "^v.*"
+	vexok_opcode_expr = "^[vk].*"
 	vexok_expr = "\\(v1\\)"
 	# All opcodes with (v) superscript supports *only* VEX prefix
 	vexonly_expr = "\\(v\\)"
+	# All opcodes with (ev) superscript supports *only* EVEX prefix
+	evexonly_expr = "\\(ev\\)"
 
 	prefix_expr = "\\(Prefix\\)"
 	prefix_num["Operand-Size"] = "INAT_PFX_OPNDSZ"
@@ -95,6 +97,7 @@ BEGIN {
 	prefix_num["Address-Size"] = "INAT_PFX_ADDRSZ"
 	prefix_num["VEX+1byte"] = "INAT_PFX_VEX2"
 	prefix_num["VEX+2byte"] = "INAT_PFX_VEX3"
+	prefix_num["EVEX"] = "INAT_PFX_EVEX"
 
 	clear_vars()
 }
@@ -319,7 +322,9 @@ function convert_operands(count,opnd,       i,j,imm,mod)
 			flags = add_flags(flags, "INAT_MODRM")
 
 		# check VEX codes
-		if (match(ext, vexonly_expr))
+		if (match(ext, evexonly_expr))
+			flags = add_flags(flags, "INAT_VEXOK | INAT_EVEXONLY")
+		else if (match(ext, vexonly_expr))
 			flags = add_flags(flags, "INAT_VEXOK | INAT_VEXONLY")
 		else if (match(ext, vexok_expr) || match(opcode, vexok_opcode_expr))
 			flags = add_flags(flags, "INAT_VEXOK")
tools/objtool/arch/x86/insn/inat.h +15 -2
index 611645e..125ecd2 100644
@@ -48,6 +48,7 @@
 /* AVX VEX prefixes */
 #define INAT_PFX_VEX2	13	/* 2-bytes VEX prefix */
 #define INAT_PFX_VEX3	14	/* 3-bytes VEX prefix */
+#define INAT_PFX_EVEX	15	/* EVEX prefix */
 
 #define INAT_LSTPFX_MAX	3
 #define INAT_LGCPFX_MAX	11
@@ -89,6 +90,7 @@
 #define INAT_VARIANT	(1 << (INAT_FLAG_OFFS + 4))
 #define INAT_VEXOK	(1 << (INAT_FLAG_OFFS + 5))
 #define INAT_VEXONLY	(1 << (INAT_FLAG_OFFS + 6))
+#define INAT_EVEXONLY	(1 << (INAT_FLAG_OFFS + 7))
 /* Attribute making macros for attribute tables */
 #define INAT_MAKE_PREFIX(pfx)	(pfx << INAT_PFX_OFFS)
 #define INAT_MAKE_ESCAPE(esc)	(esc << INAT_ESC_OFFS)
@@ -141,7 +143,13 @@ static inline int inat_last_prefix_id(insn_attr_t attr)
 static inline int inat_is_vex_prefix(insn_attr_t attr)
 {
 	attr &= INAT_PFX_MASK;
-	return attr == INAT_PFX_VEX2 || attr == INAT_PFX_VEX3;
+	return attr == INAT_PFX_VEX2 || attr == INAT_PFX_VEX3 ||
+	       attr == INAT_PFX_EVEX;
+}
+
+static inline int inat_is_evex_prefix(insn_attr_t attr)
+{
+	return (attr & INAT_PFX_MASK) == INAT_PFX_EVEX;
 }
 
 static inline int inat_is_vex3_prefix(insn_attr_t attr)
@@ -216,6 +224,11 @@ static inline int inat_accept_vex(insn_attr_t attr)
 
 static inline int inat_must_vex(insn_attr_t attr)
 {
-	return attr & INAT_VEXONLY;
+	return attr & (INAT_VEXONLY | INAT_EVEXONLY);
+}
+
+static inline int inat_must_evex(insn_attr_t attr)
+{
+	return attr & INAT_EVEXONLY;
 }
 #endif
tools/objtool/arch/x86/insn/insn.c +15 -3
index 9f26eae..ca983e2 100644
@@ -155,14 +155,24 @@ found:
 			/*
 			 * In 32-bits mode, if the [7:6] bits (mod bits of
 			 * ModRM) on the second byte are not 11b, it is
-			 * LDS or LES.
+			 * LDS or LES or BOUND.
 			 */
 			if (X86_MODRM_MOD(b2) != 3)
 				goto vex_end;
 		}
 		insn->vex_prefix.bytes[0] = b;
 		insn->vex_prefix.bytes[1] = b2;
-		if (inat_is_vex3_prefix(attr)) {
+		if (inat_is_evex_prefix(attr)) {
+			b2 = peek_nbyte_next(insn_byte_t, insn, 2);
+			insn->vex_prefix.bytes[2] = b2;
+			b2 = peek_nbyte_next(insn_byte_t, insn, 3);
+			insn->vex_prefix.bytes[3] = b2;
+			insn->vex_prefix.nbytes = 4;
+			insn->next_byte += 4;
+			if (insn->x86_64 && X86_VEX_W(b2))
+				/* VEX.W overrides opnd_size */
+				insn->opnd_bytes = 8;
+		} else if (inat_is_vex3_prefix(attr)) {
 			b2 = peek_nbyte_next(insn_byte_t, insn, 2);
 			insn->vex_prefix.bytes[2] = b2;
 			insn->vex_prefix.nbytes = 3;
@@ -221,7 +231,9 @@ void insn_get_opcode(struct insn *insn)
 		m = insn_vex_m_bits(insn);
 		p = insn_vex_p_bits(insn);
 		insn->attr = inat_get_avx_attribute(op, m, p);
-		if (!inat_accept_vex(insn->attr) && !inat_is_group(insn->attr))
+		if ((inat_must_evex(insn->attr) && !insn_is_evex(insn)) ||
+		    (!inat_accept_vex(insn->attr) &&
+		     !inat_is_group(insn->attr)))
 			insn->attr = 0;	/* This instruction is bad */
 		goto end;	/* VEX has only 1 byte for opcode */
 	}
tools/objtool/arch/x86/insn/insn.h +11 -1
index dd12da0..e23578c 100644
@@ -91,6 +91,7 @@ struct insn {
 #define X86_VEX_B(vex)	((vex) & 0x20)	/* VEX3 Byte1 */
 #define X86_VEX_L(vex)	((vex) & 0x04)	/* VEX3 Byte2, VEX2 Byte1 */
 /* VEX bit fields */
+#define X86_EVEX_M(vex)	((vex) & 0x03)		/* EVEX Byte1 */
 #define X86_VEX3_M(vex)	((vex) & 0x1f)		/* VEX3 Byte1 */
 #define X86_VEX2_M	1			/* VEX2.M always 1 */
 #define X86_VEX_V(vex)	(((vex) & 0x78) >> 3)	/* VEX3 Byte2, VEX2 Byte1 */
@@ -133,6 +134,13 @@ static inline int insn_is_avx(struct insn *insn)
 	return (insn->vex_prefix.value != 0);
 }
 
+static inline int insn_is_evex(struct insn *insn)
+{
+	if (!insn->prefixes.got)
+		insn_get_prefixes(insn);
+	return (insn->vex_prefix.nbytes == 4);
+}
+
 /* Ensure this instruction is decoded completely */
 static inline int insn_complete(struct insn *insn)
 {
@@ -144,8 +152,10 @@ static inline insn_byte_t insn_vex_m_bits(struct insn *insn)
 {
 	if (insn->vex_prefix.nbytes == 2)	/* 2 bytes VEX */
 		return X86_VEX2_M;
-	else
+	else if (insn->vex_prefix.nbytes == 3)	/* 3 bytes VEX */
 		return X86_VEX3_M(insn->vex_prefix.bytes[1]);
+	else					/* EVEX */
+		return X86_EVEX_M(insn->vex_prefix.bytes[1]);
 }
 
 static inline insn_byte_t insn_vex_p_bits(struct insn *insn)
tools/objtool/arch/x86/insn/x86-opcode-map.txt +171 -92

438 lines not on this page · raw message

Signature · 1 line
2.7.4
#4 lore raw

[tip:core/urgent] objtool: Resync x86 instruction decoder with the kernel's

objtool: Resync x86 instruction decoder with the kernel's
Commit-ID:  8d94c2f9198301bd78feb1d9ad6c08a8ee7b5d1c
Gitweb:     http://git.kernel.org/tip/8d94c2f9198301bd78feb1d9ad6c08a8ee7b5d1c
Author:     Josh Poimboeuf <[email protected]>
AuthorDate: Thu, 28 Jul 2016 19:14:59 -0500
Committer:  Ingo Molnar <[email protected]>
CommitDate: Fri, 29 Jul 2016 16:57:16 +0200

objtool: Resync x86 instruction decoder with the kernel's

This fixes the following warning:

Warning: objtool: x86 instruction decoder differs from kernel

Unfortunately we have three identical copies of the x86 instruction decoder in the kernel tree that have to be manually kept in sync.

It's on my TODO list to at least library-ize the ones in the tools subdir so we'd only have two of them instead of three. In the meantime, here's another manual sync.

Cc Andy Lutomirski <[email protected]>
Cc Borislav Petkov <[email protected]>
Cc Brian Gerst <[email protected]>
Cc Denys Vlasenko <[email protected]>
Cc H. Peter Anvin <[email protected]>
Cc Peter Zijlstra <[email protected]>
Cc Thomas Gleixner <[email protected]>
Fixes c61f4d5ebaf0 ("perf tools: Add AVX-512 support to the instruction decoder used by Intel PT")
---
 tools/objtool/arch/x86/insn/gen-insn-attr-x86.awk |  11 +-
 tools/objtool/arch/x86/insn/inat.h                |  17 +-
 tools/objtool/arch/x86/insn/insn.c                |  18 +-
 tools/objtool/arch/x86/insn/insn.h                |  12 +-
 tools/objtool/arch/x86/insn/x86-opcode-map.txt    | 263 ++++++++++++++--------
 5 files changed, 220 insertions(+), 101 deletions(-)
tools/objtool/arch/x86/insn/gen-insn-attr-x86.awk +8 -3
index 093a892..a3d2c62 100644
@@ -72,12 +72,14 @@ BEGIN {
 	lprefix_expr = "\\((66|F2|F3)\\)"
 	max_lprefix = 4
 
-	# All opcodes starting with lower-case 'v' or with (v1) superscript
+	# All opcodes starting with lower-case 'v', 'k' or with (v1) superscript
 	# accepts VEX prefix
-	vexok_opcode_expr = "^v.*"
+	vexok_opcode_expr = "^[vk].*"
 	vexok_expr = "\\(v1\\)"
 	# All opcodes with (v) superscript supports *only* VEX prefix
 	vexonly_expr = "\\(v\\)"
+	# All opcodes with (ev) superscript supports *only* EVEX prefix
+	evexonly_expr = "\\(ev\\)"
 
 	prefix_expr = "\\(Prefix\\)"
 	prefix_num["Operand-Size"] = "INAT_PFX_OPNDSZ"
@@ -95,6 +97,7 @@ BEGIN {
 	prefix_num["Address-Size"] = "INAT_PFX_ADDRSZ"
 	prefix_num["VEX+1byte"] = "INAT_PFX_VEX2"
 	prefix_num["VEX+2byte"] = "INAT_PFX_VEX3"
+	prefix_num["EVEX"] = "INAT_PFX_EVEX"
 
 	clear_vars()
 }
@@ -319,7 +322,9 @@ function convert_operands(count,opnd,       i,j,imm,mod)
 			flags = add_flags(flags, "INAT_MODRM")
 
 		# check VEX codes
-		if (match(ext, vexonly_expr))
+		if (match(ext, evexonly_expr))
+			flags = add_flags(flags, "INAT_VEXOK | INAT_EVEXONLY")
+		else if (match(ext, vexonly_expr))
 			flags = add_flags(flags, "INAT_VEXOK | INAT_VEXONLY")
 		else if (match(ext, vexok_expr) || match(opcode, vexok_opcode_expr))
 			flags = add_flags(flags, "INAT_VEXOK")
tools/objtool/arch/x86/insn/inat.h +15 -2
index 611645e..125ecd2 100644
@@ -48,6 +48,7 @@
 /* AVX VEX prefixes */
 #define INAT_PFX_VEX2	13	/* 2-bytes VEX prefix */
 #define INAT_PFX_VEX3	14	/* 3-bytes VEX prefix */
+#define INAT_PFX_EVEX	15	/* EVEX prefix */
 
 #define INAT_LSTPFX_MAX	3
 #define INAT_LGCPFX_MAX	11
@@ -89,6 +90,7 @@
 #define INAT_VARIANT	(1 << (INAT_FLAG_OFFS + 4))
 #define INAT_VEXOK	(1 << (INAT_FLAG_OFFS + 5))
 #define INAT_VEXONLY	(1 << (INAT_FLAG_OFFS + 6))
+#define INAT_EVEXONLY	(1 << (INAT_FLAG_OFFS + 7))
 /* Attribute making macros for attribute tables */
 #define INAT_MAKE_PREFIX(pfx)	(pfx << INAT_PFX_OFFS)
 #define INAT_MAKE_ESCAPE(esc)	(esc << INAT_ESC_OFFS)
@@ -141,7 +143,13 @@ static inline int inat_last_prefix_id(insn_attr_t attr)
 static inline int inat_is_vex_prefix(insn_attr_t attr)
 {
 	attr &= INAT_PFX_MASK;
-	return attr == INAT_PFX_VEX2 || attr == INAT_PFX_VEX3;
+	return attr == INAT_PFX_VEX2 || attr == INAT_PFX_VEX3 ||
+	       attr == INAT_PFX_EVEX;
+}
+
+static inline int inat_is_evex_prefix(insn_attr_t attr)
+{
+	return (attr & INAT_PFX_MASK) == INAT_PFX_EVEX;
 }
 
 static inline int inat_is_vex3_prefix(insn_attr_t attr)
@@ -216,6 +224,11 @@ static inline int inat_accept_vex(insn_attr_t attr)
 
 static inline int inat_must_vex(insn_attr_t attr)
 {
-	return attr & INAT_VEXONLY;
+	return attr & (INAT_VEXONLY | INAT_EVEXONLY);
+}
+
+static inline int inat_must_evex(insn_attr_t attr)
+{
+	return attr & INAT_EVEXONLY;
 }
 #endif
tools/objtool/arch/x86/insn/insn.c +15 -3
index 9f26eae..ca983e2 100644
@@ -155,14 +155,24 @@ found:
 			/*
 			 * In 32-bits mode, if the [7:6] bits (mod bits of
 			 * ModRM) on the second byte are not 11b, it is
-			 * LDS or LES.
+			 * LDS or LES or BOUND.
 			 */
 			if (X86_MODRM_MOD(b2) != 3)
 				goto vex_end;
 		}
 		insn->vex_prefix.bytes[0] = b;
 		insn->vex_prefix.bytes[1] = b2;
-		if (inat_is_vex3_prefix(attr)) {
+		if (inat_is_evex_prefix(attr)) {
+			b2 = peek_nbyte_next(insn_byte_t, insn, 2);
+			insn->vex_prefix.bytes[2] = b2;
+			b2 = peek_nbyte_next(insn_byte_t, insn, 3);
+			insn->vex_prefix.bytes[3] = b2;
+			insn->vex_prefix.nbytes = 4;
+			insn->next_byte += 4;
+			if (insn->x86_64 && X86_VEX_W(b2))
+				/* VEX.W overrides opnd_size */
+				insn->opnd_bytes = 8;
+		} else if (inat_is_vex3_prefix(attr)) {
 			b2 = peek_nbyte_next(insn_byte_t, insn, 2);
 			insn->vex_prefix.bytes[2] = b2;
 			insn->vex_prefix.nbytes = 3;
@@ -221,7 +231,9 @@ void insn_get_opcode(struct insn *insn)
 		m = insn_vex_m_bits(insn);
 		p = insn_vex_p_bits(insn);
 		insn->attr = inat_get_avx_attribute(op, m, p);
-		if (!inat_accept_vex(insn->attr) && !inat_is_group(insn->attr))
+		if ((inat_must_evex(insn->attr) && !insn_is_evex(insn)) ||
+		    (!inat_accept_vex(insn->attr) &&
+		     !inat_is_group(insn->attr)))
 			insn->attr = 0;	/* This instruction is bad */
 		goto end;	/* VEX has only 1 byte for opcode */
 	}
tools/objtool/arch/x86/insn/insn.h +11 -1
index dd12da0..e23578c 100644
@@ -91,6 +91,7 @@ struct insn {
 #define X86_VEX_B(vex)	((vex) & 0x20)	/* VEX3 Byte1 */
 #define X86_VEX_L(vex)	((vex) & 0x04)	/* VEX3 Byte2, VEX2 Byte1 */
 /* VEX bit fields */
+#define X86_EVEX_M(vex)	((vex) & 0x03)		/* EVEX Byte1 */
 #define X86_VEX3_M(vex)	((vex) & 0x1f)		/* VEX3 Byte1 */
 #define X86_VEX2_M	1			/* VEX2.M always 1 */
 #define X86_VEX_V(vex)	(((vex) & 0x78) >> 3)	/* VEX3 Byte2, VEX2 Byte1 */
@@ -133,6 +134,13 @@ static inline int insn_is_avx(struct insn *insn)
 	return (insn->vex_prefix.value != 0);
 }
 
+static inline int insn_is_evex(struct insn *insn)
+{
+	if (!insn->prefixes.got)
+		insn_get_prefixes(insn);
+	return (insn->vex_prefix.nbytes == 4);
+}
+
 /* Ensure this instruction is decoded completely */
 static inline int insn_complete(struct insn *insn)
 {
@@ -144,8 +152,10 @@ static inline insn_byte_t insn_vex_m_bits(struct insn *insn)
 {
 	if (insn->vex_prefix.nbytes == 2)	/* 2 bytes VEX */
 		return X86_VEX2_M;
-	else
+	else if (insn->vex_prefix.nbytes == 3)	/* 3 bytes VEX */
 		return X86_VEX3_M(insn->vex_prefix.bytes[1]);
+	else					/* EVEX */
+		return X86_EVEX_M(insn->vex_prefix.bytes[1]);
 }
 
 static inline insn_byte_t insn_vex_p_bits(struct insn *insn)
tools/objtool/arch/x86/insn/x86-opcode-map.txt +171 -92

438 lines not on this page · raw message

#5 lore raw

PATCH 3/3 objtool: un-capitalize "Warning" for out-of-sync instruction decoder

In reply to Josh Poimboeuf

Change "Warning" to "warning" to make it look more like a gcc warning. Hopefully that will be enough to help the 0-day bot or other automated tools catch this warning earlier before it ends up in Linus's tree.

---
 tools/objtool/Makefile | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
tools/objtool/Makefile +1 -1
index 0b43770..041b493 100644
@@ -51,7 +51,7 @@ $(OBJTOOL): $(LIBSUBCMD) $(OBJTOOL_IN)
 	diff -I'^#include' arch/x86/insn/insn.h ../../arch/x86/include/asm/insn.h >/dev/null && \
 	diff -I'^#include' arch/x86/insn/inat.h ../../arch/x86/include/asm/inat.h >/dev/null && \
 	diff -I'^#include' arch/x86/insn/inat_types.h ../../arch/x86/include/asm/inat_types.h >/dev/null) \
-	|| echo "Warning: objtool: x86 instruction decoder differs from kernel" >&2 )) || true
+	|| echo "warning: objtool: x86 instruction decoder differs from kernel" >&2 )) || true
 	$(QUIET_LINK)$(CC) $(OBJTOOL_IN) $(LDFLAGS) -o $@
 
 
Signature · 1 line
2.7.4
#6 lore raw

[tip:core/urgent] objtool: Un-capitalize "Warning" for out-of-sync instruction decoder

objtool: Un-capitalize "Warning" for out-of-sync instruction decoder
Commit-ID:  2af04ea545f49bc4ac59b6aaa644312ecd09a78e
Gitweb:     http://git.kernel.org/tip/2af04ea545f49bc4ac59b6aaa644312ecd09a78e
Author:     Josh Poimboeuf <[email protected]>
AuthorDate: Thu, 28 Jul 2016 19:15:00 -0500
Committer:  Ingo Molnar <[email protected]>
CommitDate: Fri, 29 Jul 2016 16:57:16 +0200

objtool: Un-capitalize "Warning" for out-of-sync instruction decoder

Change "Warning" to "warning" to make it look more like a GCC warning. Hopefully that will be enough to help the 0-day bot or other automated tools catch this warning earlier before it ends up in Linus's tree.

---
 tools/objtool/Makefile | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
tools/objtool/Makefile +1 -1
index 0b43770..041b493 100644
@@ -51,7 +51,7 @@ $(OBJTOOL): $(LIBSUBCMD) $(OBJTOOL_IN)
 	diff -I'^#include' arch/x86/insn/insn.h ../../arch/x86/include/asm/insn.h >/dev/null && \
 	diff -I'^#include' arch/x86/insn/inat.h ../../arch/x86/include/asm/inat.h >/dev/null && \
 	diff -I'^#include' arch/x86/insn/inat_types.h ../../arch/x86/include/asm/inat_types.h >/dev/null) \
-	|| echo "Warning: objtool: x86 instruction decoder differs from kernel" >&2 )) || true
+	|| echo "warning: objtool: x86 instruction decoder differs from kernel" >&2 )) || true
 	$(QUIET_LINK)$(CC) $(OBJTOOL_IN) $(LDFLAGS) -o $@