| Message ID | 20190816122403.14994-8-raphael.gault@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | objtool: Add support for arm64 | expand |
On Fri, Aug 16, 2019 at 01:23:52PM +0100, Raphael Gault wrote: > On arm64 some object files contain data stored in the .text section. > This data is interpreted by objtool as instruction but can't be > identified as a valid one. In order to keep analysing those files we > introduce INSN_UNKNOWN type. The "unknown instruction" warning will thus > only be raised if such instructions are uncountered while validating an > execution branch. > > This change doesn't impact the x86 decoding logic since 0 is still used > as a way to specify an unknown type, raising the "unknown instruction" > warning during the decoding phase still. > > Signed-off-by: Raphael Gault <raphael.gault@arm.com> Is there a reason such data can't be moved to .rodata? That would seem like the proper fix.
Hi Josh, On 22/08/19 21:04, Josh Poimboeuf wrote: > On Fri, Aug 16, 2019 at 01:23:52PM +0100, Raphael Gault wrote: >> On arm64 some object files contain data stored in the .text section. >> This data is interpreted by objtool as instruction but can't be >> identified as a valid one. In order to keep analysing those files we >> introduce INSN_UNKNOWN type. The "unknown instruction" warning will thus >> only be raised if such instructions are uncountered while validating an >> execution branch. >> >> This change doesn't impact the x86 decoding logic since 0 is still used >> as a way to specify an unknown type, raising the "unknown instruction" >> warning during the decoding phase still. >> >> Signed-off-by: Raphael Gault <raphael.gault@arm.com> > > Is there a reason such data can't be moved to .rodata? That would seem > like the proper fix. > Raphaël can confirm, if I remember correctly, that issue was encountered on assembly files implementing crypto algorithms were some words/double-words of data were in the middle of the .text. I think it is done this way to make sure the data can be loaded in a single instruction. So moving it to another section could impact the crypto performance depending on the relocations. That was my understanding at least. Cheers,
On Thu, Aug 22, 2019 at 09:45:00PM +0100, Julien wrote: > Hi Josh, > > On 22/08/19 21:04, Josh Poimboeuf wrote: > > On Fri, Aug 16, 2019 at 01:23:52PM +0100, Raphael Gault wrote: > > > On arm64 some object files contain data stored in the .text section. > > > This data is interpreted by objtool as instruction but can't be > > > identified as a valid one. In order to keep analysing those files we > > > introduce INSN_UNKNOWN type. The "unknown instruction" warning will thus > > > only be raised if such instructions are uncountered while validating an > > > execution branch. > > > > > > This change doesn't impact the x86 decoding logic since 0 is still used > > > as a way to specify an unknown type, raising the "unknown instruction" > > > warning during the decoding phase still. > > > > > > Signed-off-by: Raphael Gault <raphael.gault@arm.com> > > > > Is there a reason such data can't be moved to .rodata? That would seem > > like the proper fix. > > > > Raphaël can confirm, if I remember correctly, that issue was encountered on > assembly files implementing crypto algorithms were some words/double-words > of data were in the middle of the .text. I think it is done this way to make > sure the data can be loaded in a single instruction. So moving it to another > section could impact the crypto performance depending on the relocations. > > That was my understanding at least. Thanks. If that's the case then that would be useful information to put in the patch description. A code excerpt of an example code site would be useful too. I'm not sure INSN_UNKNOWN is the right name though, since the decoder does actually know about it. Maybe INSN_DATA or something?
diff --git a/tools/objtool/arch.h b/tools/objtool/arch.h index 68d6371a24a2..9f2590e1df79 100644 --- a/tools/objtool/arch.h +++ b/tools/objtool/arch.h @@ -28,6 +28,7 @@ enum insn_type { INSN_CLAC, INSN_STD, INSN_CLD, + INSN_UNKNOWN, INSN_OTHER, }; diff --git a/tools/objtool/arch/arm64/decode.c b/tools/objtool/arch/arm64/decode.c index be3d2eb10227..4cb9402d6fe1 100644 --- a/tools/objtool/arch/arm64/decode.c +++ b/tools/objtool/arch/arm64/decode.c @@ -37,9 +37,9 @@ */ static arm_decode_class aarch64_insn_class_decode_table[] = { [INSN_RESERVED] = arm_decode_reserved, - [INSN_UNKNOWN] = arm_decode_unknown, + [INSN_UNALLOC_1] = arm_decode_unknown, [INSN_SVE_ENC] = arm_decode_sve_encoding, - [INSN_UNALLOC] = arm_decode_unknown, + [INSN_UNALLOC_2] = arm_decode_unknown, [INSN_LD_ST_4] = arm_decode_ld_st, [INSN_DP_REG_5] = arm_decode_dp_reg, [INSN_LD_ST_6] = arm_decode_ld_st, @@ -191,7 +191,7 @@ int arch_decode_instruction(struct elf *elf, struct section *sec, int arm_decode_unknown(u32 instr, unsigned char *type, unsigned long *immediate, struct stack_op *op) { - *type = 0; + *type = INSN_UNKNOWN; return 0; } @@ -206,7 +206,7 @@ int arm_decode_reserved(u32 instr, unsigned char *type, unsigned long *immediate, struct stack_op *op) { *immediate = instr & ONES(16); - *type = INSN_BUG; + *type = INSN_UNKNOWN; return 0; } diff --git a/tools/objtool/arch/arm64/include/insn_decode.h b/tools/objtool/arch/arm64/include/insn_decode.h index eb54fc39dca5..a01d76306749 100644 --- a/tools/objtool/arch/arm64/include/insn_decode.h +++ b/tools/objtool/arch/arm64/include/insn_decode.h @@ -20,9 +20,9 @@ #include "../../../arch.h" #define INSN_RESERVED 0b0000 -#define INSN_UNKNOWN 0b0001 +#define INSN_UNALLOC_1 0b0001 #define INSN_SVE_ENC 0b0010 -#define INSN_UNALLOC 0b0011 +#define INSN_UNALLOC_2 0b0011 #define INSN_DP_IMM 0b1001 //0x100x #define INSN_BRANCH 0b1011 //0x101x #define INSN_LD_ST_4 0b0100 //0bx1x0 diff --git a/tools/objtool/check.c b/tools/objtool/check.c index 519569b0329f..baa6a93f37cd 100644 --- a/tools/objtool/check.c +++ b/tools/objtool/check.c @@ -1981,6 +1981,13 @@ static int validate_branch(struct objtool_file *file, struct symbol *func, while (1) { next_insn = next_insn_same_sec(file, insn); + if (insn->type == INSN_UNKNOWN) { + WARN("%s+0x%lx unknown instruction type, should never be reached", + insn->sec->name, + insn->offset); + return 1; + } + if (file->c_file && func && insn->func && func != insn->func->pfunc) { WARN("%s() falls through to next function %s()", func->name, insn->func->name); @@ -2414,7 +2421,8 @@ static int validate_reachable_instructions(struct objtool_file *file) return 0; for_each_insn(file, insn) { - if (insn->visited || ignore_unreachable_insn(insn)) + if (insn->visited || ignore_unreachable_insn(insn) || + insn->type == INSN_UNKNOWN) continue; WARN_FUNC("unreachable instruction", insn->sec, insn->offset);
On arm64 some object files contain data stored in the .text section. This data is interpreted by objtool as instruction but can't be identified as a valid one. In order to keep analysing those files we introduce INSN_UNKNOWN type. The "unknown instruction" warning will thus only be raised if such instructions are uncountered while validating an execution branch. This change doesn't impact the x86 decoding logic since 0 is still used as a way to specify an unknown type, raising the "unknown instruction" warning during the decoding phase still. Signed-off-by: Raphael Gault <raphael.gault@arm.com> --- tools/objtool/arch.h | 1 + tools/objtool/arch/arm64/decode.c | 8 ++++---- tools/objtool/arch/arm64/include/insn_decode.h | 4 ++-- tools/objtool/check.c | 10 +++++++++- 4 files changed, 16 insertions(+), 7 deletions(-)