* [PATCH 0/3] Minor refactorings in arm-tdep.c instruction decoding @ 2016-02-10 16:17 Simon Marchi 2016-02-10 16:17 ` [PATCH 3/3] arm-tdep.c: Refactor arm_decode_media Simon Marchi ` (3 more replies) 0 siblings, 4 replies; 13+ messages in thread From: Simon Marchi @ 2016-02-10 16:17 UTC (permalink / raw) To: gdb-patches; +Cc: Simon Marchi I am currently working on extracting the instruction decoding from the displaced stepping support in arm-tdep.c, in order to share the functionality with the upcoming fast tracepoint support. I did a few refactors that helped me correlate the code with the ARM Architecture Reference Manual. I think the change helps readability in general, and especially when you have the manual open on the side. The idea is to follow the the order of the manual, use the same names and do the same "checks" (avoid using unnecessary shortcuts that make the code more cryptic). Simon Marchi (3): arm-tdep.c: Refactor arm_process_displaced_insn arm-tdep.c: Refactor arm_decode_dp_misc arm-tdep.c: Refactor arm_decode_media gdb/arm-tdep.c | 171 ++++++++++++++++++++++++++++++++++++++++----------------- 1 file changed, 121 insertions(+), 50 deletions(-) -- 2.5.1 ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 3/3] arm-tdep.c: Refactor arm_decode_media 2016-02-10 16:17 [PATCH 0/3] Minor refactorings in arm-tdep.c instruction decoding Simon Marchi @ 2016-02-10 16:17 ` Simon Marchi 2016-02-11 11:58 ` Yao Qi 2016-02-10 16:17 ` [PATCH 1/3] arm-tdep.c: Refactor arm_process_displaced_insn Simon Marchi ` (2 subsequent siblings) 3 siblings, 1 reply; 13+ messages in thread From: Simon Marchi @ 2016-02-10 16:17 UTC (permalink / raw) To: gdb-patches; +Cc: Simon Marchi Refactor arm_decode_media to make it more readable. The new layout matches very closely the description in the ARM Architecture Reference Manual. It uses the same order and same nomenclature. gdb/ChangeLog: * arm-tdep.c (arm_decode_media): Refactor instruction decoding. --- gdb/arm-tdep.c | 30 ++++++++++++++++++++++-------- 1 file changed, 22 insertions(+), 8 deletions(-) diff --git a/gdb/arm-tdep.c b/gdb/arm-tdep.c index e17a1a4..bf5bc49 100644 --- a/gdb/arm-tdep.c +++ b/gdb/arm-tdep.c @@ -6625,49 +6625,63 @@ static int arm_decode_media (struct gdbarch *gdbarch, uint32_t insn, struct displaced_step_closure *dsc) { - switch (bits (insn, 20, 24)) + uint8_t op1 = bits (insn, 20, 24); + uint8_t rd = bits (insn, 12, 15); + uint8_t op2 = bits (insn, 5, 7); + uint8_t rn = bits (insn, 0, 3); + + switch (op1) { case 0x00: case 0x01: case 0x02: case 0x03: + /* Parallel addition and subtraction, signed */ return arm_copy_unmodified (gdbarch, insn, "parallel add/sub signed", dsc); case 0x04: case 0x05: case 0x06: case 0x07: + /* Parallel addition and subtraction, unsigned */ return arm_copy_unmodified (gdbarch, insn, "parallel add/sub unsigned", dsc); case 0x08: case 0x09: case 0x0a: case 0x0b: case 0x0c: case 0x0d: case 0x0e: case 0x0f: + /* Packing, unpacking, saturation, and reversal */ return arm_copy_unmodified (gdbarch, insn, "decode/pack/unpack/saturate/reverse", dsc); case 0x18: - if (bits (insn, 5, 7) == 0) /* op2. */ + if (op2 == 0) { - if (bits (insn, 12, 15) == 0xf) + if (rd == 0xf) + /* USAD8 */ return arm_copy_unmodified (gdbarch, insn, "usad8", dsc); else + /* USADA8 */ return arm_copy_unmodified (gdbarch, insn, "usada8", dsc); } else - return arm_copy_undef (gdbarch, insn, dsc); + return arm_copy_undef (gdbarch, insn, dsc); case 0x1a: case 0x1b: - if (bits (insn, 5, 6) == 0x2) /* op2[1:0]. */ + if ((op2 & 0x3) == 0x2) + /* SBFX */ return arm_copy_unmodified (gdbarch, insn, "sbfx", dsc); else return arm_copy_undef (gdbarch, insn, dsc); case 0x1c: case 0x1d: - if (bits (insn, 5, 6) == 0x0) /* op2[1:0]. */ + if ((op2 & 0x3) == 0x0) { - if (bits (insn, 0, 3) == 0xf) + if (rn == 0xf) + /* BFC */ return arm_copy_unmodified (gdbarch, insn, "bfc", dsc); else + /* BFI */ return arm_copy_unmodified (gdbarch, insn, "bfi", dsc); } else return arm_copy_undef (gdbarch, insn, dsc); case 0x1e: case 0x1f: - if (bits (insn, 5, 6) == 0x2) /* op2[1:0]. */ + if ((op2 & 0x3) == 0x2) + /* UBFX */ return arm_copy_unmodified (gdbarch, insn, "ubfx", dsc); else return arm_copy_undef (gdbarch, insn, dsc); -- 2.5.1 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 3/3] arm-tdep.c: Refactor arm_decode_media 2016-02-10 16:17 ` [PATCH 3/3] arm-tdep.c: Refactor arm_decode_media Simon Marchi @ 2016-02-11 11:58 ` Yao Qi 0 siblings, 0 replies; 13+ messages in thread From: Yao Qi @ 2016-02-11 11:58 UTC (permalink / raw) To: Simon Marchi; +Cc: gdb-patches Simon Marchi <simon.marchi@ericsson.com> writes: > - switch (bits (insn, 20, 24)) > + uint8_t op1 = bits (insn, 20, 24); > + uint8_t rd = bits (insn, 12, 15); > + uint8_t op2 = bits (insn, 5, 7); > + uint8_t rn = bits (insn, 0, 3); > + > + switch (op1) op1 is only used once, I prefer using bits (insn, 20, 24) rather than defining a new variable. Other variables, like rd and rn, can be defined where they are used. -- Yao (齐尧) ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 1/3] arm-tdep.c: Refactor arm_process_displaced_insn 2016-02-10 16:17 [PATCH 0/3] Minor refactorings in arm-tdep.c instruction decoding Simon Marchi 2016-02-10 16:17 ` [PATCH 3/3] arm-tdep.c: Refactor arm_decode_media Simon Marchi @ 2016-02-10 16:17 ` Simon Marchi 2016-02-11 11:22 ` Yao Qi 2016-02-10 16:17 ` [PATCH 2/3] arm-tdep.c: Refactor arm_decode_dp_misc Simon Marchi 2016-02-11 11:17 ` [PATCH 0/3] Minor refactorings in arm-tdep.c instruction decoding Yao Qi 3 siblings, 1 reply; 13+ messages in thread From: Simon Marchi @ 2016-02-10 16:17 UTC (permalink / raw) To: gdb-patches; +Cc: Simon Marchi Refactor arm_process_displaced_insn to make it more readable. The new layout matches very closely the description in the ARM Architecture Reference Manual. It uses the same order and same nomenclature. gdb/ChangeLog: * arm-tdep.c (arm_process_displaced_insn): Refactor instruction decoding. --- gdb/arm-tdep.c | 68 ++++++++++++++++++++++++++++++++++++++++++---------------- 1 file changed, 50 insertions(+), 18 deletions(-) diff --git a/gdb/arm-tdep.c b/gdb/arm-tdep.c index 6ac05f0..0a9c0f6 100644 --- a/gdb/arm-tdep.c +++ b/gdb/arm-tdep.c @@ -7495,6 +7495,7 @@ arm_process_displaced_insn (struct gdbarch *gdbarch, CORE_ADDR from, int err = 0; enum bfd_endian byte_order_for_code = gdbarch_byte_order_for_code (gdbarch); uint32_t insn; + uint8_t cond, op, op1; /* Most displaced instructions use a 1-instruction scratch space, so set this here and override below if/when necessary. */ @@ -7515,29 +7516,60 @@ arm_process_displaced_insn (struct gdbarch *gdbarch, CORE_ADDR from, "at %.8lx\n", (unsigned long) insn, (unsigned long) from); - if ((insn & 0xf0000000) == 0xf0000000) - err = arm_decode_unconditional (gdbarch, insn, regs, dsc); - else switch (((insn & 0x10) >> 4) | ((insn & 0xe000000) >> 24)) + cond = bits (insn, 28, 31); + op1 = bits (insn, 25, 27); + op = bit (insn, 4); + + if (cond != 0xf) { - case 0x0: case 0x1: case 0x2: case 0x3: - err = arm_decode_dp_misc (gdbarch, insn, regs, dsc); - break; + switch (op1) + { + case 0x0: + case 0x1: + /* Data-processing and miscellaneous instructions */ + err = arm_decode_dp_misc (gdbarch, insn, regs, dsc); + break; - case 0x4: case 0x5: case 0x6: - err = arm_decode_ld_st_word_ubyte (gdbarch, insn, regs, dsc); - break; + case 0x2: + /* Load/store word and unsigned byte */ + err = arm_decode_ld_st_word_ubyte (gdbarch, insn, regs, dsc); + break; - case 0x7: - err = arm_decode_media (gdbarch, insn, dsc); - break; + case 0x3: + if (op == 0) + { + /* Load/store word and unsigned byte */ + err = arm_decode_ld_st_word_ubyte (gdbarch, insn, regs, dsc); + } + else + { + /* Media instructions */ + err = arm_decode_media (gdbarch, insn, dsc); + } + break; - case 0x8: case 0x9: case 0xa: case 0xb: - err = arm_decode_b_bl_ldmstm (gdbarch, insn, regs, dsc); - break; + case 0x4: + case 0x5: + /* Branch, branch with link, and block data transfer */ + err = arm_decode_b_bl_ldmstm (gdbarch, insn, regs, dsc); + break; - case 0xc: case 0xd: case 0xe: case 0xf: - err = arm_decode_svc_copro (gdbarch, insn, to, regs, dsc); - break; + case 0x6: + case 0x7: + /* Coprocessor instructions, and Supervisor Call */ + err = arm_decode_svc_copro (gdbarch, insn, to, regs, dsc); + break; + + default: + internal_error (__FILE__, __LINE__, + _("arm_process_displaced_insn: Missing case")); + break; + } + } + else + { + /* Unconditional instructions */ + err = arm_decode_unconditional (gdbarch, insn, regs, dsc); } if (err) -- 2.5.1 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/3] arm-tdep.c: Refactor arm_process_displaced_insn 2016-02-10 16:17 ` [PATCH 1/3] arm-tdep.c: Refactor arm_process_displaced_insn Simon Marchi @ 2016-02-11 11:22 ` Yao Qi 2016-02-11 16:59 ` Simon Marchi 0 siblings, 1 reply; 13+ messages in thread From: Yao Qi @ 2016-02-11 11:22 UTC (permalink / raw) To: Simon Marchi; +Cc: gdb-patches Simon Marchi <simon.marchi@ericsson.com> writes: > - if ((insn & 0xf0000000) == 0xf0000000) > - err = arm_decode_unconditional (gdbarch, insn, regs, dsc); > - else switch (((insn & 0x10) >> 4) | ((insn & 0xe000000) >> 24)) > + cond = bits (insn, 28, 31); Variable 'cond' is only used once, so don't need to define it. This is my personal flavour. > + op1 = bits (insn, 25, 27); > + op = bit (insn, 4); > + > + if (cond != 0xf) if (bits (insn, 28, 31) != INST_NV) this is consistent with other places in arm-tdep.c > { > - case 0x0: case 0x1: case 0x2: case 0x3: > - err = arm_decode_dp_misc (gdbarch, insn, regs, dsc); > - break; > + switch (op1) > + { > + case 0x0: > + case 0x1: > + /* Data-processing and miscellaneous instructions */ > + err = arm_decode_dp_misc (gdbarch, insn, regs, dsc); > + break; > > - case 0x4: case 0x5: case 0x6: > - err = arm_decode_ld_st_word_ubyte (gdbarch, insn, regs, dsc); > - break; > + case 0x2: > + /* Load/store word and unsigned byte */ > + err = arm_decode_ld_st_word_ubyte (gdbarch, insn, regs, dsc); > + break; > > - case 0x7: > - err = arm_decode_media (gdbarch, insn, dsc); > - break; > + case 0x3: > + if (op == 0) 'op' is only used here, let us define it in this block, or use 'bit (insn, 4)' instead. -- Yao (齐尧) ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/3] arm-tdep.c: Refactor arm_process_displaced_insn 2016-02-11 11:22 ` Yao Qi @ 2016-02-11 16:59 ` Simon Marchi 2016-02-12 16:56 ` Yao Qi 0 siblings, 1 reply; 13+ messages in thread From: Simon Marchi @ 2016-02-11 16:59 UTC (permalink / raw) To: Yao Qi; +Cc: gdb-patches On 16-02-11 06:21 AM, Yao Qi wrote: > Simon Marchi <simon.marchi@ericsson.com> writes: > >> - if ((insn & 0xf0000000) == 0xf0000000) >> - err = arm_decode_unconditional (gdbarch, insn, regs, dsc); >> - else switch (((insn & 0x10) >> 4) | ((insn & 0xe000000) >> 24)) >> + cond = bits (insn, 28, 31); > > Variable 'cond' is only used once, so don't need to define it. This is > my personal flavour. Well, my goal was to use variables with names that refer to these tables: http://nova.polymtl.ca/~simark/ss/fileJVxJNx.png (ARM Architecture Reference Manual, section A5.1) If you only use the bits (insn, 28, 31) notation, I think you lose readability, because the you have to do one more indirection in the doc, to go see what those bits mean. >> + op1 = bits (insn, 25, 27); >> + op = bit (insn, 4); >> + >> + if (cond != 0xf) > > if (bits (insn, 28, 31) != INST_NV) > > this is consistent with other places in arm-tdep.c I agree, if there is a define for that it should be used. What does _NV stand for though? >> { >> - case 0x0: case 0x1: case 0x2: case 0x3: >> - err = arm_decode_dp_misc (gdbarch, insn, regs, dsc); >> - break; >> + switch (op1) >> + { >> + case 0x0: >> + case 0x1: >> + -/* Data-processing and miscellaneous instructions */ >> + err = arm_decode_dp_misc (gdbarch, insn, regs, dsc); >> + break; >> >> - case 0x4: case 0x5: case 0x6: >> - err = arm_decode_ld_st_word_ubyte (gdbarch, insn, regs, dsc); >> - break; >> + case 0x2: >> + /* Load/store word and unsigned byte */ >> + err = arm_decode_ld_st_word_ubyte (gdbarch, insn, regs, dsc); >> + break; >> >> - case 0x7: >> - err = arm_decode_media (gdbarch, insn, dsc); >> - break; >> + case 0x3: >> + if (op == 0) > > 'op' is only used here, let us define it in this block, or use > 'bit (insn, 4)' instead. Ok for moving it, but I would suggest keeping the variable op, for the same reason as cond mentioned above. Thanks, Simon ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/3] arm-tdep.c: Refactor arm_process_displaced_insn 2016-02-11 16:59 ` Simon Marchi @ 2016-02-12 16:56 ` Yao Qi 0 siblings, 0 replies; 13+ messages in thread From: Yao Qi @ 2016-02-12 16:56 UTC (permalink / raw) To: Simon Marchi; +Cc: Yao Qi, gdb-patches Simon Marchi <simon.marchi@ericsson.com> writes: > Well, my goal was to use variables with names that refer to these tables: > > http://nova.polymtl.ca/~simark/ss/fileJVxJNx.png > (ARM Architecture Reference Manual, section A5.1) Yes, I clearly understand your goal, but I don't think the change is necessary. However, I can't see anything harmful or negative in this patch, and looks the patch is useful in terms of helping you reference the doc easily, I am OK. > > If you only use the bits (insn, 28, 31) notation, I think you lose readability, > because the you have to do one more indirection in the doc, to go see what those > bits mean. but if you write code like "if (bits (insn, 28, 31) != INST_NV)", people do understand what those bits mean. >>> + op1 = bits (insn, 25, 27); >>> + op = bit (insn, 4); >>> + >>> + if (cond != 0xf) >> >> if (bits (insn, 28, 31) != INST_NV) >> >> this is consistent with other places in arm-tdep.c > > I agree, if there is a define for that it should be used. What does _NV stand > for though? NV means Never. >> >> 'op' is only used here, let us define it in this block, or use >> 'bit (insn, 4)' instead. > > Ok for moving it, but I would suggest keeping the variable op, for > the same reason as cond mentioned above. OK, that is fine, since this is the personal flavour of writing code. -- Yao (齐尧) ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 2/3] arm-tdep.c: Refactor arm_decode_dp_misc 2016-02-10 16:17 [PATCH 0/3] Minor refactorings in arm-tdep.c instruction decoding Simon Marchi 2016-02-10 16:17 ` [PATCH 3/3] arm-tdep.c: Refactor arm_decode_media Simon Marchi 2016-02-10 16:17 ` [PATCH 1/3] arm-tdep.c: Refactor arm_process_displaced_insn Simon Marchi @ 2016-02-10 16:17 ` Simon Marchi 2016-02-11 11:52 ` Yao Qi 2016-02-11 11:17 ` [PATCH 0/3] Minor refactorings in arm-tdep.c instruction decoding Yao Qi 3 siblings, 1 reply; 13+ messages in thread From: Simon Marchi @ 2016-02-10 16:17 UTC (permalink / raw) To: gdb-patches; +Cc: Simon Marchi Refactor arm_decode_dp_misc to make it more readable. The new layout matches very closely the description in the ARM Architecture Reference Manual. It uses the same order and same nomenclature. gdb/ChangeLog: * arm-tdep.c (arm_decode_dp_misc): Refactor instruction decoding. --- gdb/arm-tdep.c | 73 +++++++++++++++++++++++++++++++++++++++------------------- 1 file changed, 49 insertions(+), 24 deletions(-) diff --git a/gdb/arm-tdep.c b/gdb/arm-tdep.c index 0a9c0f6..e17a1a4 100644 --- a/gdb/arm-tdep.c +++ b/gdb/arm-tdep.c @@ -6517,45 +6517,70 @@ arm_decode_dp_misc (struct gdbarch *gdbarch, uint32_t insn, struct regcache *regs, struct displaced_step_closure *dsc) { - if (bit (insn, 25)) - switch (bits (insn, 20, 24)) - { - case 0x10: - return arm_copy_unmodified (gdbarch, insn, "movw", dsc); - - case 0x14: - return arm_copy_unmodified (gdbarch, insn, "movt", dsc); + uint8_t op = bit (insn, 25); + uint8_t op1 = bits (insn, 20, 24); + uint8_t op2 = bits (insn, 4, 7); - case 0x12: case 0x16: - return arm_copy_unmodified (gdbarch, insn, "msr imm", dsc); - - default: - return arm_copy_alu_imm (gdbarch, insn, regs, dsc); - } - else + if (op == 0) { - uint32_t op1 = bits (insn, 20, 24), op2 = bits (insn, 4, 7); - if ((op1 & 0x19) != 0x10 && (op2 & 0x1) == 0x0) + /* Data-processing (register) */ return arm_copy_alu_reg (gdbarch, insn, regs, dsc); else if ((op1 & 0x19) != 0x10 && (op2 & 0x9) == 0x1) + /* Data-processing (register-shifted register) */ return arm_copy_alu_shifted_reg (gdbarch, insn, regs, dsc); else if ((op1 & 0x19) == 0x10 && (op2 & 0x8) == 0x0) + /* Miscellaneous instructions */ return arm_decode_miscellaneous (gdbarch, insn, regs, dsc); else if ((op1 & 0x19) == 0x10 && (op2 & 0x9) == 0x8) + /* Halfword multiply and multiply accumulate */ return arm_copy_unmodified (gdbarch, insn, "halfword mul/mla", dsc); else if ((op1 & 0x10) == 0x00 && op2 == 0x9) + /* Multiply and multiply accumulate */ return arm_copy_unmodified (gdbarch, insn, "mul/mla", dsc); else if ((op1 & 0x10) == 0x10 && op2 == 0x9) + /* Synchronization primitives */ return arm_copy_unmodified (gdbarch, insn, "synch", dsc); - else if (op2 == 0xb || (op2 & 0xd) == 0xd) - /* 2nd arg means "unprivileged". */ - return arm_copy_extra_ld_st (gdbarch, insn, (op1 & 0x12) == 0x02, regs, - dsc); + else if ((op1 & 0x12) != 0x2 && op2 == 0xb) + /* Extra load/store instructions */ + return arm_copy_extra_ld_st (gdbarch, insn, 0, regs, dsc); + else if ((op1 & 0x12) != 0x2 && (op2 & 0xd) == 0xd) + /* Extra load/store instructions */ + return arm_copy_extra_ld_st (gdbarch, insn, 0, regs, dsc); + else if ((op1 & 0x13) == 0x2 && (op2 & 0xd) == 0xd) + /* Extra load/store instructions */ + return arm_copy_extra_ld_st (gdbarch, insn, 0, regs, dsc); + else if ((op1 & 0x12) == 0x2 && op2 == 0xd) + /* Extra load/store instructions, unprivileged */ + return arm_copy_extra_ld_st (gdbarch, insn, 1, regs, dsc); + else if ((op1 & 0x13) == 0x3 && (op2 & 0xd) == 0xd) + /* Extra load/store instructions, unprivileged */ + return arm_copy_extra_ld_st (gdbarch, insn, 1, regs, dsc); + else + return 1; + } + else + { + switch (op1) + { + default: + /* Data-processing (immediate) */ + return arm_copy_alu_imm (gdbarch, insn, regs, dsc); + + case 0x10: + /* 16-bit immediate load, MOV (immediate) */ + return arm_copy_unmodified (gdbarch, insn, "movw", dsc); + + case 0x14: + /* High halfword 16-bit immediate load, MOVT */ + return arm_copy_unmodified (gdbarch, insn, "movt", dsc); + + case 0x12: + case 0x16: + /* MSR (immediate), and hints */ + return arm_copy_unmodified (gdbarch, insn, "msr imm", dsc); + } } - - /* Should be unreachable. */ - return 1; } static int -- 2.5.1 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/3] arm-tdep.c: Refactor arm_decode_dp_misc 2016-02-10 16:17 ` [PATCH 2/3] arm-tdep.c: Refactor arm_decode_dp_misc Simon Marchi @ 2016-02-11 11:52 ` Yao Qi 2016-02-11 17:10 ` Yao Qi 2016-02-11 17:18 ` Simon Marchi 0 siblings, 2 replies; 13+ messages in thread From: Yao Qi @ 2016-02-11 11:52 UTC (permalink / raw) To: Simon Marchi; +Cc: gdb-patches Simon Marchi <simon.marchi@ericsson.com> writes: > Refactor arm_decode_dp_misc to make it more readable. The new layout > matches very closely the description in the ARM Architecture Reference > Manual. It uses the same order and same nomenclature. As I mentioned in the reply to the patch cover letter, the manual may adjust the layout in the future. For example, the manual lists instructions whose OP is 0 first, but it may change to list instructions whose OP is 1 first in the future. IMO, we don't have to 100% match the code to the manual. > > gdb/ChangeLog: > > * arm-tdep.c (arm_decode_dp_misc): Refactor instruction decoding. > --- > gdb/arm-tdep.c | 73 +++++++++++++++++++++++++++++++++++++++------------------- > 1 file changed, 49 insertions(+), 24 deletions(-) > > diff --git a/gdb/arm-tdep.c b/gdb/arm-tdep.c > index 0a9c0f6..e17a1a4 100644 > --- a/gdb/arm-tdep.c > +++ b/gdb/arm-tdep.c > @@ -6517,45 +6517,70 @@ arm_decode_dp_misc (struct gdbarch *gdbarch, uint32_t insn, > struct regcache *regs, > struct displaced_step_closure *dsc) > { > - if (bit (insn, 25)) > - switch (bits (insn, 20, 24)) > - { > - case 0x10: > - return arm_copy_unmodified (gdbarch, insn, "movw", dsc); > - > - case 0x14: > - return arm_copy_unmodified (gdbarch, insn, "movt", dsc); > + uint8_t op = bit (insn, 25); > + uint8_t op1 = bits (insn, 20, 24); > + uint8_t op2 = bits (insn, 4, 7); > > - case 0x12: case 0x16: > - return arm_copy_unmodified (gdbarch, insn, "msr imm", dsc); > - > - default: > - return arm_copy_alu_imm (gdbarch, insn, regs, dsc); > - } > - else > + if (op == 0) > { > - uint32_t op1 = bits (insn, 20, 24), op2 = bits (insn, 4, 7); > - > if ((op1 & 0x19) != 0x10 && (op2 & 0x1) == 0x0) > + /* Data-processing (register) */ > return arm_copy_alu_reg (gdbarch, insn, regs, dsc); > else if ((op1 & 0x19) != 0x10 && (op2 & 0x9) == 0x1) > + /* Data-processing (register-shifted register) */ > return arm_copy_alu_shifted_reg (gdbarch, insn, regs, dsc); > else if ((op1 & 0x19) == 0x10 && (op2 & 0x8) == 0x0) > + /* Miscellaneous instructions */ > return arm_decode_miscellaneous (gdbarch, insn, regs, dsc); > else if ((op1 & 0x19) == 0x10 && (op2 & 0x9) == 0x8) > + /* Halfword multiply and multiply accumulate */ > return arm_copy_unmodified (gdbarch, insn, "halfword mul/mla", dsc); > else if ((op1 & 0x10) == 0x00 && op2 == 0x9) > + /* Multiply and multiply accumulate */ > return arm_copy_unmodified (gdbarch, insn, "mul/mla", dsc); > else if ((op1 & 0x10) == 0x10 && op2 == 0x9) > + /* Synchronization primitives */ > return arm_copy_unmodified (gdbarch, insn, "synch", dsc); These added comments are helpful. > - else if (op2 == 0xb || (op2 & 0xd) == 0xd) > - /* 2nd arg means "unprivileged". */ > - return arm_copy_extra_ld_st (gdbarch, insn, (op1 & 0x12) == 0x02, regs, > - dsc); > + else if ((op1 & 0x12) != 0x2 && op2 == 0xb) > + /* Extra load/store instructions */ > + return arm_copy_extra_ld_st (gdbarch, insn, 0, regs, dsc); > + else if ((op1 & 0x12) != 0x2 && (op2 & 0xd) == 0xd) > + /* Extra load/store instructions */ > + return arm_copy_extra_ld_st (gdbarch, insn, 0, regs, dsc); > + else if ((op1 & 0x13) == 0x2 && (op2 & 0xd) == 0xd) > + /* Extra load/store instructions */ > + return arm_copy_extra_ld_st (gdbarch, insn, 0, regs, dsc); > + else if ((op1 & 0x12) == 0x2 && op2 == 0xd) > + /* Extra load/store instructions, unprivileged */ > + return arm_copy_extra_ld_st (gdbarch, insn, 1, regs, dsc); > + else if ((op1 & 0x13) == 0x3 && (op2 & 0xd) == 0xd) > + /* Extra load/store instructions, unprivileged */ > + return arm_copy_extra_ld_st (gdbarch, insn, 1, regs, dsc); > + else > + return 1; However, I don't see how helpful or useful the changes above are. > + } > + else > + { > + switch (op1) > + { > + default: > + /* Data-processing (immediate) */ > + return arm_copy_alu_imm (gdbarch, insn, regs, dsc); > + > + case 0x10: > + /* 16-bit immediate load, MOV (immediate) */ > + return arm_copy_unmodified (gdbarch, insn, "movw", dsc); > + > + case 0x14: > + /* High halfword 16-bit immediate load, MOVT */ > + return arm_copy_unmodified (gdbarch, insn, "movt", dsc); > + > + case 0x12: > + case 0x16: > + /* MSR (immediate), and hints */ > + return arm_copy_unmodified (gdbarch, insn, "msr imm", dsc); > + } > } > - > - /* Should be unreachable. */ > - return 1; > } In short, I don't see how this patch improve the readability of the code, and I feel hard mapping the code to the manual. -- Yao (齐尧) ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/3] arm-tdep.c: Refactor arm_decode_dp_misc 2016-02-11 11:52 ` Yao Qi @ 2016-02-11 17:10 ` Yao Qi 2016-02-11 17:18 ` Simon Marchi 1 sibling, 0 replies; 13+ messages in thread From: Yao Qi @ 2016-02-11 17:10 UTC (permalink / raw) To: Simon Marchi; +Cc: gdb-patches On 11/02/16 11:52, Yao Qi wrote: > In short, I don't see how this patch improve the readability of the > code, and I feel hard mapping the code to the manual. s/ feel/ don't feel/ -- Yao (é½å°§) ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/3] arm-tdep.c: Refactor arm_decode_dp_misc 2016-02-11 11:52 ` Yao Qi 2016-02-11 17:10 ` Yao Qi @ 2016-02-11 17:18 ` Simon Marchi 1 sibling, 0 replies; 13+ messages in thread From: Simon Marchi @ 2016-02-11 17:18 UTC (permalink / raw) To: Yao Qi; +Cc: gdb-patches On 16-02-11 06:52 AM, Yao Qi wrote: > Simon Marchi <simon.marchi@ericsson.com> writes: > >> Refactor arm_decode_dp_misc to make it more readable. The new layout >> matches very closely the description in the ARM Architecture Reference >> Manual. It uses the same order and same nomenclature. > > As I mentioned in the reply to the patch cover letter, the manual may > adjust the layout in the future. For example, the manual lists > instructions whose OP is 0 first, but it may change to list instructions > whose OP is 1 first in the future. IMO, we don't have to 100% match the > code to the manual. Indeed, but I think there is very little chance that they change the order just for fun. And in the mean time, I think it can help people who want to read the code to learn or verify it. >> >> gdb/ChangeLog: >> >> * arm-tdep.c (arm_decode_dp_misc): Refactor instruction decoding. >> --- >> gdb/arm-tdep.c | 73 +++++++++++++++++++++++++++++++++++++++------------------- >> 1 file changed, 49 insertions(+), 24 deletions(-) >> >> diff --git a/gdb/arm-tdep.c b/gdb/arm-tdep.c >> index 0a9c0f6..e17a1a4 100644 >> --- a/gdb/arm-tdep.c >> +++ b/gdb/arm-tdep.c >> @@ -6517,45 +6517,70 @@ arm_decode_dp_misc (struct gdbarch *gdbarch, uint32_t insn, >> struct regcache *regs, >> struct displaced_step_closure *dsc) >> { >> - if (bit (insn, 25)) >> - switch (bits (insn, 20, 24)) >> - { >> - case 0x10: >> - return arm_copy_unmodified (gdbarch, insn, "movw", dsc); >> - >> - case 0x14: >> - return arm_copy_unmodified (gdbarch, insn, "movt", dsc); >> + uint8_t op = bit (insn, 25); >> + uint8_t op1 = bits (insn, 20, 24); >> + uint8_t op2 = bits (insn, 4, 7); >> >> - case 0x12: case 0x16: >> - return arm_copy_unmodified (gdbarch, insn, "msr imm", dsc); >> - >> - default: >> - return arm_copy_alu_imm (gdbarch, insn, regs, dsc); >> - } >> - else >> + if (op == 0) >> { >> - uint32_t op1 = bits (insn, 20, 24), op2 = bits (insn, 4, 7); >> - >> if ((op1 & 0x19) != 0x10 && (op2 & 0x1) == 0x0) >> + /* Data-processing (register) */ >> return arm_copy_alu_reg (gdbarch, insn, regs, dsc); >> else if ((op1 & 0x19) != 0x10 && (op2 & 0x9) == 0x1) >> + /* Data-processing (register-shifted register) */ >> return arm_copy_alu_shifted_reg (gdbarch, insn, regs, dsc); >> else if ((op1 & 0x19) == 0x10 && (op2 & 0x8) == 0x0) >> + /* Miscellaneous instructions */ >> return arm_decode_miscellaneous (gdbarch, insn, regs, dsc); >> else if ((op1 & 0x19) == 0x10 && (op2 & 0x9) == 0x8) >> + /* Halfword multiply and multiply accumulate */ >> return arm_copy_unmodified (gdbarch, insn, "halfword mul/mla", dsc); >> else if ((op1 & 0x10) == 0x00 && op2 == 0x9) >> + /* Multiply and multiply accumulate */ >> return arm_copy_unmodified (gdbarch, insn, "mul/mla", dsc); >> else if ((op1 & 0x10) == 0x10 && op2 == 0x9) >> + /* Synchronization primitives */ >> return arm_copy_unmodified (gdbarch, insn, "synch", dsc); > > These added comments are helpful. > >> - else if (op2 == 0xb || (op2 & 0xd) == 0xd) >> - /* 2nd arg means "unprivileged". */ >> - return arm_copy_extra_ld_st (gdbarch, insn, (op1 & 0x12) == 0x02, regs, >> - dsc); >> + else if ((op1 & 0x12) != 0x2 && op2 == 0xb) >> + /* Extra load/store instructions */ >> + return arm_copy_extra_ld_st (gdbarch, insn, 0, regs, dsc); >> + else if ((op1 & 0x12) != 0x2 && (op2 & 0xd) == 0xd) >> + /* Extra load/store instructions */ >> + return arm_copy_extra_ld_st (gdbarch, insn, 0, regs, dsc); >> + else if ((op1 & 0x13) == 0x2 && (op2 & 0xd) == 0xd) >> + /* Extra load/store instructions */ >> + return arm_copy_extra_ld_st (gdbarch, insn, 0, regs, dsc); >> + else if ((op1 & 0x12) == 0x2 && op2 == 0xd) >> + /* Extra load/store instructions, unprivileged */ >> + return arm_copy_extra_ld_st (gdbarch, insn, 1, regs, dsc); >> + else if ((op1 & 0x13) == 0x3 && (op2 & 0xd) == 0xd) >> + /* Extra load/store instructions, unprivileged */ >> + return arm_copy_extra_ld_st (gdbarch, insn, 1, regs, dsc); >> + else >> + return 1; > > However, I don't see how helpful or useful the changes above are. > >> + } >> + else >> + { >> + switch (op1) >> + { >> + default: >> + /* Data-processing (immediate) */ >> + return arm_copy_alu_imm (gdbarch, insn, regs, dsc); >> + >> + case 0x10: >> + /* 16-bit immediate load, MOV (immediate) */ >> + return arm_copy_unmodified (gdbarch, insn, "movw", dsc); >> + >> + case 0x14: >> + /* High halfword 16-bit immediate load, MOVT */ >> + return arm_copy_unmodified (gdbarch, insn, "movt", dsc); >> + >> + case 0x12: >> + case 0x16: >> + /* MSR (immediate), and hints */ >> + return arm_copy_unmodified (gdbarch, insn, "msr imm", dsc); >> + } >> } >> - >> - /* Should be unreachable. */ >> - return 1; >> } > > In short, I don't see how this patch improve the readability of the > code, and I feel hard mapping the code to the manual. Just to be sure that we are referring to the same manual, I am using this: http://nova.polymtl.ca/~simark/ss/fileWKxYqt.png Section A5.2 If we take the current code for when op == 0: uint32_t op1 = bits (insn, 20, 24), op2 = bits (insn, 4, 7); if ((op1 & 0x19) != 0x10 && (op2 & 0x1) == 0x0) return arm_copy_alu_reg (gdbarch, insn, regs, dsc); else if ((op1 & 0x19) != 0x10 && (op2 & 0x9) == 0x1) return arm_copy_alu_shifted_reg (gdbarch, insn, regs, dsc); else if ((op1 & 0x19) == 0x10 && (op2 & 0x8) == 0x0) return arm_decode_miscellaneous (gdbarch, insn, regs, dsc); else if ((op1 & 0x19) == 0x10 && (op2 & 0x9) == 0x8) return arm_copy_unmodified (gdbarch, insn, "halfword mul/mla", dsc); else if ((op1 & 0x10) == 0x00 && op2 == 0x9) return arm_copy_unmodified (gdbarch, insn, "mul/mla", dsc); else if ((op1 & 0x10) == 0x10 && op2 == 0x9) return arm_copy_unmodified (gdbarch, insn, "synch", dsc); else if (op2 == 0xb || (op2 & 0xd) == 0xd) /* 2nd arg means "unpriveleged". */ return arm_copy_extra_ld_st (gdbarch, insn, (op1 & 0x12) == 0x02, regs, dsc); Maybe it's just me being a bit slow with bitwise operations, but it's not obvious at all that "(op2 == 0xb || (op2 & 0xd) == 0xd)" matches exactly (not less and no more) the possibles values listed for "Extra load/store instructions". It's also not obvious that "(op1 & 0x12) == 0x02" manage to differentiate between the op1 values for privileged vs unprivileged. I mean, I could spend time doing those computations on paper and would probably realize that it's equivalent. But if each condition in the code matches exactly each contidion in the manual, it's easy to see follow. For example, I think it's relatively easy to verify that else if ((op1 & 0x13) == 0x3 && (op2 & 0xd) == 0xd) matches the condition from the manual op1 op2 Instruction 0xx11 11x1 Extra load/store instructions, unprivileged ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 0/3] Minor refactorings in arm-tdep.c instruction decoding 2016-02-10 16:17 [PATCH 0/3] Minor refactorings in arm-tdep.c instruction decoding Simon Marchi ` (2 preceding siblings ...) 2016-02-10 16:17 ` [PATCH 2/3] arm-tdep.c: Refactor arm_decode_dp_misc Simon Marchi @ 2016-02-11 11:17 ` Yao Qi 2016-02-16 15:26 ` Simon Marchi 3 siblings, 1 reply; 13+ messages in thread From: Yao Qi @ 2016-02-11 11:17 UTC (permalink / raw) To: Simon Marchi; +Cc: gdb-patches Simon Marchi <simon.marchi@ericsson.com> writes: > I am currently working on extracting the instruction decoding from the > displaced stepping support in arm-tdep.c, in order to share the functionality > with the upcoming fast tracepoint support. I did a few refactors that helped > me correlate the code with the ARM Architecture Reference Manual. I think the > change helps readability in general, and especially when you have the manual > open on the side. > > The idea is to follow the the order of the manual, use the same names and do > the same "checks" (avoid using unnecessary shortcuts that make the code more > cryptic). It is good to match the code to the manual. The instruction encoding can't change, but the order or the category of instruction _may_ change in the manual in the future. I am not the manual writer, but writers may want to refactor the doc in the future too :) Although your change is code refactor, better to run tests. -- Yao (齐尧) ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 0/3] Minor refactorings in arm-tdep.c instruction decoding 2016-02-11 11:17 ` [PATCH 0/3] Minor refactorings in arm-tdep.c instruction decoding Yao Qi @ 2016-02-16 15:26 ` Simon Marchi 0 siblings, 0 replies; 13+ messages in thread From: Simon Marchi @ 2016-02-16 15:26 UTC (permalink / raw) To: Yao Qi; +Cc: gdb-patches On 16-02-11 06:17 AM, Yao Qi wrote: > Simon Marchi <simon.marchi@ericsson.com> writes: > >> I am currently working on extracting the instruction decoding from the >> displaced stepping support in arm-tdep.c, in order to share the functionality >> with the upcoming fast tracepoint support. I did a few refactors that helped >> me correlate the code with the ARM Architecture Reference Manual. I think the >> change helps readability in general, and especially when you have the manual >> open on the side. >> >> The idea is to follow the the order of the manual, use the same names and do >> the same "checks" (avoid using unnecessary shortcuts that make the code more >> cryptic). > > It is good to match the code to the manual. The instruction encoding > can't change, but the order or the category of instruction _may_ change in > the manual in the future. I am not the manual writer, but writers may > want to refactor the doc in the future too :) > > Although your change is code refactor, better to run tests. Yes, that is the part that is worrying me. Since we don't have unit tests, I am too afraid to break things. I'll forget this patch set for the time being. When the code is extracted to its own file and decoupled from the core of GDB, maybe there will be some way to write some kind of unit tests that ensure that the refactor is safe. Thanks for reviewing anyway! ^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2016-02-16 15:26 UTC | newest] Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2016-02-10 16:17 [PATCH 0/3] Minor refactorings in arm-tdep.c instruction decoding Simon Marchi 2016-02-10 16:17 ` [PATCH 3/3] arm-tdep.c: Refactor arm_decode_media Simon Marchi 2016-02-11 11:58 ` Yao Qi 2016-02-10 16:17 ` [PATCH 1/3] arm-tdep.c: Refactor arm_process_displaced_insn Simon Marchi 2016-02-11 11:22 ` Yao Qi 2016-02-11 16:59 ` Simon Marchi 2016-02-12 16:56 ` Yao Qi 2016-02-10 16:17 ` [PATCH 2/3] arm-tdep.c: Refactor arm_decode_dp_misc Simon Marchi 2016-02-11 11:52 ` Yao Qi 2016-02-11 17:10 ` Yao Qi 2016-02-11 17:18 ` Simon Marchi 2016-02-11 11:17 ` [PATCH 0/3] Minor refactorings in arm-tdep.c instruction decoding Yao Qi 2016-02-16 15:26 ` Simon Marchi
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).