commit 9ace48f3d7d80ce09c5df60cccb433470410b11b Author: Andreas Arnez Date: Tue Aug 19 15:42:13 2014 +0100 This patch set mainly aims at improving the S/390 disassembler's readability and also fixes some minor issues. S/390: Split disassembler routine into smaller functions S/390: Fix disassembler's treatment of signed/unsigned operands S/390: Fix off-by-one error in disassembler initialization S/390: Simplify opcode search loop in disassembler S/390: Drop function pointer dereferences in disassembler S/390: Various minor simplifications in disassembler ### a/opcodes/ChangeLog ### b/opcodes/ChangeLog ## -1,3 +1,30 @@ +2014-08-19 Andreas Arnez + + * s390-dis.c (s390_insn_length, s390_insn_matches_opcode) + (s390_print_insn_with_opcode, opcode_mask_more_specific): New + static functions, code was moved from... + (print_insn_s390): ...here. + (s390_extract_operand): Adjust comment. Change type of first + parameter from 'unsigned char *' to 'const bfd_byte *'. + (union operand_value): New. + (s390_extract_operand): Change return type to union operand_value. + Also avoid integer overflow in sign-extension. + (s390_print_insn_with_opcode): Adjust to changed return value from + s390_extract_operand(). Change "%i" printf format to "%u" for + unsigned values. + (init_disasm): Simplify initialization of opc_index[]. This also + fixes an access after the last element of s390_opcodes[]. + (print_insn_s390): Simplify the opcode search loop. + Check architecture mask against all searched opcodes, not just the + first matching one. + (s390_print_insn_with_opcode): Drop function pointer dereferences + without effect. + (print_insn_s390): Likewise. + (s390_insn_length): Simplify formula for return value. + (s390_print_insn_with_opcode): Avoid special handling for the + separator before the first operand. Use new local variable + 'flags' in place of 'operand->flags'. + 2014-08-14 Mike Frysinger * bfin-dis.c (struct private): Change int's to bfd_boolean's. --- a/opcodes/s390-dis.c +++ b/opcodes/s390-dis.c @@ -35,19 +35,15 @@ static int current_arch_mask = 0; static void init_disasm (struct disassemble_info *info) { - const struct s390_opcode *opcode; - const struct s390_opcode *opcode_end; + int i; const char *p; memset (opc_index, 0, sizeof (opc_index)); - opcode_end = s390_opcodes + s390_num_opcodes; - for (opcode = s390_opcodes; opcode < opcode_end; opcode++) - { - opc_index[(int) opcode->opcode[0]] = opcode - s390_opcodes; - while ((opcode < opcode_end) && - (opcode[1].opcode[0] == opcode->opcode[0])) - opcode++; - } + + /* Reverse order, such that each opc_index ends up pointing to the + first matching entry instead of the last. */ + for (i = s390_num_opcodes; i--; ) + opc_index[s390_opcodes[i].opcode[0]] = i; for (p = info->disassembler_options; p != NULL; ) { @@ -69,15 +65,46 @@ init_disasm (struct disassemble_info *info) init_flag = 1; } +/* Derive the length of an instruction from its first byte. */ + +static inline int +s390_insn_length (const bfd_byte *buffer) +{ + /* 00xxxxxx -> 2, 01xxxxxx/10xxxxxx -> 4, 11xxxxxx -> 6. */ + return ((buffer[0] >> 6) + 3) & ~1U; +} + +/* Match the instruction in BUFFER against the given OPCODE, excluding + the first byte. */ + +static inline int +s390_insn_matches_opcode (const bfd_byte *buffer, + const struct s390_opcode *opcode) +{ + return (buffer[1] & opcode->mask[1]) == opcode->opcode[1] + && (buffer[2] & opcode->mask[2]) == opcode->opcode[2] + && (buffer[3] & opcode->mask[3]) == opcode->opcode[3] + && (buffer[4] & opcode->mask[4]) == opcode->opcode[4] + && (buffer[5] & opcode->mask[5]) == opcode->opcode[5]; +} + +union operand_value +{ + int i; + unsigned int u; +}; + /* Extracts an operand value from an instruction. */ /* We do not perform the shift operation for larl-type address operands here since that would lead to an overflow of the 32 bit integer value. Instead the shift operation is done when printing - the operand in print_insn_s390. */ + the operand. */ -static inline unsigned int -s390_extract_operand (unsigned char *insn, const struct s390_operand *operand) +static inline union operand_value +s390_extract_operand (const bfd_byte *insn, + const struct s390_operand *operand) { + union operand_value ret; unsigned int val; int bits; @@ -99,15 +126,97 @@ s390_extract_operand (unsigned char *insn, const struct s390_operand *operand) if (operand->bits == 20 && operand->shift == 20) val = (val & 0xff) << 12 | (val & 0xfff00) >> 8; - /* Sign extend value if the operand is signed or pc relative. */ - if ((operand->flags & (S390_OPERAND_SIGNED | S390_OPERAND_PCREL)) - && (val & (1U << (operand->bits - 1)))) - val |= (-1U << (operand->bits - 1)) << 1; + /* Sign extend value if the operand is signed or pc relative. Avoid + integer overflows. */ + if (operand->flags & (S390_OPERAND_SIGNED | S390_OPERAND_PCREL)) + { + unsigned int m = 1U << (operand->bits - 1); + + if (val >= m) + ret.i = (int) (val - m) - 1 - (int) (m - 1U); + else + ret.i = (int) val; + } + else if (operand->flags & S390_OPERAND_LENGTH) + /* Length x in an instruction has real length x + 1. */ + ret.u = val + 1; + else + ret.u = val; + + return ret; +} + +/* Print the S390 instruction in BUFFER, assuming that it matches the + given OPCODE. */ + +static void +s390_print_insn_with_opcode (bfd_vma memaddr, + struct disassemble_info *info, + const bfd_byte *buffer, + const struct s390_opcode *opcode) +{ + const unsigned char *opindex; + char separator; + + /* Mnemonic. */ + info->fprintf_func (info->stream, "%s", opcode->name); + + /* Operands. */ + separator = '\t'; + for (opindex = opcode->operands; *opindex != 0; opindex++) + { + const struct s390_operand *operand = s390_operands + *opindex; + union operand_value val = s390_extract_operand (buffer, operand); + unsigned long flags = operand->flags; + + if ((flags & S390_OPERAND_INDEX) && val.u == 0) + continue; + if ((flags & S390_OPERAND_BASE) && + val.u == 0 && separator == '(') + { + separator = ','; + continue; + } + + info->fprintf_func (info->stream, "%c", separator); + + if (flags & S390_OPERAND_GPR) + info->fprintf_func (info->stream, "%%r%u", val.u); + else if (flags & S390_OPERAND_FPR) + info->fprintf_func (info->stream, "%%f%u", val.u); + else if (flags & S390_OPERAND_AR) + info->fprintf_func (info->stream, "%%a%u", val.u); + else if (flags & S390_OPERAND_CR) + info->fprintf_func (info->stream, "%%c%u", val.u); + else if (flags & S390_OPERAND_PCREL) + info->print_address_func (memaddr + val.i + val.i, info); + else if (flags & S390_OPERAND_SIGNED) + info->fprintf_func (info->stream, "%i", val.i); + else + info->fprintf_func (info->stream, "%u", val.u); + + if (flags & S390_OPERAND_DISP) + separator = '('; + else if (flags & S390_OPERAND_BASE) + { + info->fprintf_func (info->stream, ")"); + separator = ','; + } + else + separator = ','; + } +} + +/* Check whether opcode A's mask is more specific than that of B. */ - /* Length x in an instructions has real length x + 1. */ - if (operand->flags & S390_OPERAND_LENGTH) - val++; - return val; +static int +opcode_mask_more_specific (const struct s390_opcode *a, + const struct s390_opcode *b) +{ + return (((int) a->mask[0] + a->mask[1] + a->mask[2] + + a->mask[3] + a->mask[4] + a->mask[5]) + > ((int) b->mask[0] + b->mask[1] + b->mask[2] + + b->mask[3] + b->mask[4] + b->mask[5])); } /* Print a S390 instruction. */ @@ -116,11 +225,9 @@ int print_insn_s390 (bfd_vma memaddr, struct disassemble_info *info) { bfd_byte buffer[6]; - const struct s390_opcode *opcode; - const struct s390_opcode *opcode_end; + const struct s390_opcode *opcode = NULL; unsigned int value; int status, opsize, bufsize; - char separator; if (init_flag == 0) init_disasm (info); @@ -130,156 +237,72 @@ print_insn_s390 (bfd_vma memaddr, struct disassemble_info *info) /* Every S390 instruction is max 6 bytes long. */ memset (buffer, 0, 6); - status = (*info->read_memory_func) (memaddr, buffer, 6, info); + status = info->read_memory_func (memaddr, buffer, 6, info); if (status != 0) { for (bufsize = 0; bufsize < 6; bufsize++) - if ((*info->read_memory_func) (memaddr, buffer, bufsize + 1, info) != 0) + if (info->read_memory_func (memaddr, buffer, bufsize + 1, info) != 0) break; if (bufsize <= 0) { - (*info->memory_error_func) (status, memaddr, info); + info->memory_error_func (status, memaddr, info); return -1; } - /* Opsize calculation looks strange but it works - 00xxxxxx -> 2 bytes, 01xxxxxx/10xxxxxx -> 4 bytes, - 11xxxxxx -> 6 bytes. */ - opsize = ((((buffer[0] >> 6) + 1) >> 1) + 1) << 1; + opsize = s390_insn_length (buffer); status = opsize > bufsize; } else { bufsize = 6; - opsize = ((((buffer[0] >> 6) + 1) >> 1) + 1) << 1; + opsize = s390_insn_length (buffer); } if (status == 0) { const struct s390_opcode *op; - /* Find the first match in the opcode table. */ - opcode_end = s390_opcodes + s390_num_opcodes; - for (opcode = s390_opcodes + opc_index[(int) buffer[0]]; - (opcode < opcode_end) && (buffer[0] == opcode->opcode[0]); - opcode++) + /* Find the "best match" in the opcode table. */ + for (op = s390_opcodes + opc_index[buffer[0]]; + op != s390_opcodes + s390_num_opcodes + && op->opcode[0] == buffer[0]; + op++) { - const struct s390_operand *operand; - const unsigned char *opindex; - - /* Check architecture. */ - if (!(opcode->modes & current_arch_mask)) - continue; - - /* Check signature of the opcode. */ - if ((buffer[1] & opcode->mask[1]) != opcode->opcode[1] - || (buffer[2] & opcode->mask[2]) != opcode->opcode[2] - || (buffer[3] & opcode->mask[3]) != opcode->opcode[3] - || (buffer[4] & opcode->mask[4]) != opcode->opcode[4] - || (buffer[5] & opcode->mask[5]) != opcode->opcode[5]) - continue; - - /* Advance to an opcode with a more specific mask. */ - for (op = opcode + 1; op < opcode_end; op++) - { - if ((buffer[0] & op->mask[0]) != op->opcode[0]) - break; - - if ((buffer[1] & op->mask[1]) != op->opcode[1] - || (buffer[2] & op->mask[2]) != op->opcode[2] - || (buffer[3] & op->mask[3]) != op->opcode[3] - || (buffer[4] & op->mask[4]) != op->opcode[4] - || (buffer[5] & op->mask[5]) != op->opcode[5]) - continue; - - if (((int)opcode->mask[0] + opcode->mask[1] + - opcode->mask[2] + opcode->mask[3] + - opcode->mask[4] + opcode->mask[5]) < - ((int)op->mask[0] + op->mask[1] + - op->mask[2] + op->mask[3] + - op->mask[4] + op->mask[5])) - opcode = op; - } - - /* The instruction is valid. */ - if (opcode->operands[0] != 0) - (*info->fprintf_func) (info->stream, "%s\t", opcode->name); - else - (*info->fprintf_func) (info->stream, "%s", opcode->name); - - /* Extract the operands. */ - separator = 0; - for (opindex = opcode->operands; *opindex != 0; opindex++) - { - operand = s390_operands + *opindex; - value = s390_extract_operand (buffer, operand); - - if ((operand->flags & S390_OPERAND_INDEX) && value == 0) - continue; - if ((operand->flags & S390_OPERAND_BASE) && - value == 0 && separator == '(') - { - separator = ','; - continue; - } - - if (separator) - (*info->fprintf_func) (info->stream, "%c", separator); - - if (operand->flags & S390_OPERAND_GPR) - (*info->fprintf_func) (info->stream, "%%r%i", value); - else if (operand->flags & S390_OPERAND_FPR) - (*info->fprintf_func) (info->stream, "%%f%i", value); - else if (operand->flags & S390_OPERAND_AR) - (*info->fprintf_func) (info->stream, "%%a%i", value); - else if (operand->flags & S390_OPERAND_CR) - (*info->fprintf_func) (info->stream, "%%c%i", value); - else if (operand->flags & S390_OPERAND_PCREL) - (*info->print_address_func) (memaddr + (int)value + (int)value, - info); - else if (operand->flags & S390_OPERAND_SIGNED) - (*info->fprintf_func) (info->stream, "%i", (int) value); - else - (*info->fprintf_func) (info->stream, "%u", value); - - if (operand->flags & S390_OPERAND_DISP) - { - separator = '('; - } - else if (operand->flags & S390_OPERAND_BASE) - { - (*info->fprintf_func) (info->stream, ")"); - separator = ','; - } - else - separator = ','; - } - - /* Found instruction, printed it, return its size. */ - return opsize; + if ((op->modes & current_arch_mask) + && s390_insn_matches_opcode (buffer, op) + && (opcode == NULL + || opcode_mask_more_specific (op, opcode))) + opcode = op; } - /* No matching instruction found, fall through to hex print. */ } + if (opcode != NULL) + { + /* The instruction is valid. Print it and return its size. */ + s390_print_insn_with_opcode (memaddr, info, buffer, opcode); + return opsize; + } + + /* Fall back to hex print. */ if (bufsize >= 4) { value = (unsigned int) buffer[0]; value = (value << 8) + (unsigned int) buffer[1]; value = (value << 8) + (unsigned int) buffer[2]; value = (value << 8) + (unsigned int) buffer[3]; - (*info->fprintf_func) (info->stream, ".long\t0x%08x", value); + info->fprintf_func (info->stream, ".long\t0x%08x", value); return 4; } else if (bufsize >= 2) { value = (unsigned int) buffer[0]; value = (value << 8) + (unsigned int) buffer[1]; - (*info->fprintf_func) (info->stream, ".short\t0x%04x", value); + info->fprintf_func (info->stream, ".short\t0x%04x", value); return 2; } else { value = (unsigned int) buffer[0]; - (*info->fprintf_func) (info->stream, ".byte\t0x%02x", value); + info->fprintf_func (info->stream, ".byte\t0x%02x", value); return 1; } }