[PATCH 3/6] x86: harmonize disp with imm handling
H.J. Lu
hjl.tools@gmail.com
Thu Jun 17 14:46:07 GMT 2021
On Mon, Jun 14, 2021 at 3:25 AM Jan Beulich <jbeulich@suse.com> wrote:
>
> Certain disp values may trigger "... shortened to ..." warnings when
> equivalent imm ones don't. In some of the cases there are also
> differences (for non-64-bit code) between BFD64 and !BFD64 builds. The
> resulting encodings (i.e. use [or not] of the shorter disp8 / imm8
> forms) are also different in some cases. Make this handling consistent.
>
> Note that using equivalent 16-bit mode displacements / immediates
> continues to expose entirely different behavior (see the disp-imm-16
> testcase added by an earlier patch). This may want to be the subject of
> further changes, but it'll then quickly become obvious that e.g. keying
> use of extend_to_32bit_address() to non-64-bit mode isn't appropriate
> either: Once we allow wrapping operands, we would better do so
> consistently, in which case all of this would need to become dependent
> upon address or operand size instead of mode.
>
> gas/
> 2021-06-XX Jan Beulich <jbeulich@suse.com>
>
> * config/tc-i386.c (optimize_disp): Generalize disp32 part of
> the BFD64-only logic to also apply to non-64-bit code.
> (i386_finalize_displacement): Use extend_to_32bit_address for
> non-64-bit code. Drop now redundant O_constant checks.
> * testsuite/gas/i386/disp-imm-32.s,
> testsuite/gas/i386/disp-imm-32.d: New.
> * testsuite/gas/i386/i386.exp: Run new test.
>
> ---
> It may remain a point for discussion whether immediates/displacements
> which are obviously exceeding 32 bit (like present in the new test case)
> wouldn't better trigger minimally a warning, to at least sort of match
> !BFD64's "missing or invalid {displacement,immediate} expression" errors
> in such cases. But telling apart expressions which have overflowed (in
> 32 bits) from ones where larger-than-32-bit constants were present may
> end up being difficult.
>
> --- a/gas/config/tc-i386.c
> +++ b/gas/config/tc-i386.c
> @@ -5905,26 +5905,24 @@ optimize_disp (void)
> }
>
> #ifdef BFD64
> - if (flag_code == CODE_64BIT)
> + /* Optimize 64-bit displacement to 32-bit for 64-bit BFD. */
> + if ((i.types[op].bitfield.disp32
> + || (flag_code == CODE_64BIT
> + && want_disp32 (current_templates->start)))
> + && fits_in_unsigned_long (op_disp))
> {
> - /* Optimize 64-bit displacement to 32-bit for 64-bit BFD. */
> - if ((i.types[op].bitfield.disp32
> - || want_disp32 (current_templates->start))
> - && fits_in_unsigned_long (op_disp))
> - {
> - /* If this operand is at most 32 bits, convert
> - to a signed 32 bit number and don't use 64bit
> - displacement. */
> - op_disp = (op_disp ^ ((offsetT) 1 << 31)) - ((addressT) 1 << 31);
> - i.types[op].bitfield.disp64 = 0;
> - i.types[op].bitfield.disp32 = 1;
> - }
> + /* If this operand is at most 32 bits, convert
> + to a signed 32 bit number and don't use 64bit
> + displacement. */
> + op_disp = (op_disp ^ ((offsetT) 1 << 31)) - ((addressT) 1 << 31);
> + i.types[op].bitfield.disp64 = 0;
> + i.types[op].bitfield.disp32 = 1;
> + }
>
> - if (fits_in_signed_long (op_disp))
> - {
> - i.types[op].bitfield.disp64 = 0;
> - i.types[op].bitfield.disp32s = 1;
> - }
> + if (flag_code == CODE_64BIT && fits_in_signed_long (op_disp))
> + {
> + i.types[op].bitfield.disp64 = 0;
> + i.types[op].bitfield.disp32s = 1;
> }
> #endif
> if ((i.types[op].bitfield.disp32
> @@ -11019,9 +11017,18 @@ i386_finalize_displacement (segT exp_seg
> ret = 0;
> }
>
> + else if (exp->X_op == O_constant)
> + {
> + /* Sizing gets taken care of by optimize_disp().
> +
> + If not 64bit, sign/zero extend val, to account for wraparound
> + when !BFD64. */
> + if (flag_code != CODE_64BIT)
> + exp->X_add_number = extend_to_32bit_address (exp->X_add_number);
> + }
> +
> #if (defined (OBJ_AOUT) || defined (OBJ_MAYBE_AOUT))
> - else if (exp->X_op != O_constant
> - && OUTPUT_FLAVOR == bfd_target_aout_flavour
> + else if (OUTPUT_FLAVOR == bfd_target_aout_flavour
> && exp_seg != absolute_section
> && exp_seg != text_section
> && exp_seg != data_section
> @@ -11034,9 +11041,7 @@ i386_finalize_displacement (segT exp_seg
> }
> #endif
>
> - if (current_templates->start->opcode_modifier.jump == JUMP_BYTE
> - /* Constants get taken care of by optimize_disp(). */
> - && exp->X_op != O_constant)
> + else if (current_templates->start->opcode_modifier.jump == JUMP_BYTE)
> i.types[this_operand].bitfield.disp8 = 1;
>
> /* Check if this is a displacement only operand. */
> --- /dev/null
> +++ b/gas/testsuite/gas/i386/disp-imm-32.d
> @@ -0,0 +1,21 @@
> +#objdump: -dw
> +#name: i386 displacements / immediates (32-bit)
> +
> +.*: +file format .*
> +
> +Disassembly of section .text:
> +
> +0+ <disp_imm>:
> +[ ]*[a-f0-9]+: 8b 40 01 mov 0x1\(%eax\),%eax
> +[ ]*[a-f0-9]+: 62 f1 7c 48 28 40 01 vmovaps 0x40\(%eax\),%zmm0
> +[ ]*[a-f0-9]+: 83 c1 01 add \$0x1,%ecx
> +[ ]*[a-f0-9]+: 8b 00 mov \(%eax\),%eax
> +[ ]*[a-f0-9]+: 62 f1 7c 48 28 00 vmovaps \(%eax\),%zmm0
> +[ ]*[a-f0-9]+: 83 c1 00 add \$0x0,%ecx
> +[ ]*[a-f0-9]+: 8b 40 ff mov -0x1\(%eax\),%eax
> +[ ]*[a-f0-9]+: 62 f1 7c 48 28 40 ff vmovaps -0x40\(%eax\),%zmm0
> +[ ]*[a-f0-9]+: 83 c1 ff add \$0xffffffff,%ecx
> +[ ]*[a-f0-9]+: 8b 40 01 mov 0x1\(%eax\),%eax
> +[ ]*[a-f0-9]+: 62 f1 7c 48 28 40 01 vmovaps 0x40\(%eax\),%zmm0
> +[ ]*[a-f0-9]+: 83 c1 01 add \$0x1,%ecx
> +#pass
> --- /dev/null
> +++ b/gas/testsuite/gas/i386/disp-imm-32.s
> @@ -0,0 +1,17 @@
> + .text
> +disp_imm:
> + mov -0xffffffff(%eax), %eax
I don't think we should treat -0xffffffff(%eax) as 1(%eax).
We allow addresses to wraparound. I don't see a need for
displacements to wraparound.
> + vmovaps -0xffffffc0(%eax), %zmm0
> + add $-0xffffffff, %ecx
> +
> + mov -0xffffffff-1(%eax), %eax
> + vmovaps -0xffffffc0-0x40(%eax), %zmm0
> + add $-0xffffffff-1, %ecx
> +
> + mov -0xffffffff-2(%eax), %eax
> + vmovaps -0xffffffc0-0x80(%eax), %zmm0
> + add $-0xffffffff-2, %ecx
> +
> + mov -0x1ffffffff(%eax), %eax
> + vmovaps -0x1ffffffc0(%eax), %zmm0
> + add $-0x1ffffffff, %ecx
> --- a/gas/testsuite/gas/i386/i386.exp
> +++ b/gas/testsuite/gas/i386/i386.exp
> @@ -88,6 +88,9 @@ if [gas_32_check] then {
> run_dump_test "disp-intel"
> run_dump_test "disp32"
> run_list_test "disp-imm-16"
> + if { [gas_bfd64_check] } {
> + run_dump_test "disp-imm-32"
> + }
> run_dump_test "vmx"
> run_dump_test "vmfunc"
> run_dump_test "smx"
>
--
H.J.
More information about the Binutils
mailing list