This is the mail archive of the
binutils@sourceware.org
mailing list for the binutils project.
Re: [Patch][AArch64] - Error on load pair to same register
- From: Richard Earnshaw <rearnsha at arm dot com>
- To: Ryan Mansfield <rmansfield at qnx dot com>, Jiong Wang <Jiong dot Wang at arm dot com>, "binutils at sourceware dot org" <binutils at sourceware dot org>
- Date: Tue, 18 Nov 2014 10:14:37 +0000
- Subject: Re: [Patch][AArch64] - Error on load pair to same register
- Authentication-results: sourceware.org; auth=none
- References: <5463BE02 dot 4000109 at qnx dot com> <546485FB dot 1030600 at arm dot com> <546A0CEC dot 3050205 at qnx dot com>
On 17/11/14 14:57, Ryan Mansfield wrote:
> On 14-11-13 05:20 AM, Jiong Wang wrote:
>> On 12/11/14 20:07, Ryan Mansfield wrote:
>>> I doubt that any one would intentionally do this operation since the
>>> behaviour is unpredictable so it just seems safer to reject the code.
>>
>> Hi Ryan,
>>
>> thanks for reporting this.
>>
>> I think the encoding of same register is allowed in ISA manual, while
>> the behavior is unpredictable. In principle, all allowed encoding need
>> to be supported, while if its behavior is unpredictable, then we need
>> to give warning instead of error which abort the assembling.
>
> Hi Jiong,
>
> Thanks for your comments. If it's a warning that should be issued, then
> operand_general_constraint_met_p in libopcodes probably isn't the place
> to be warning from, and better to handle it in md_assemble after the
> operands have already been parsed.
>
> I added some warnings about unpredictable writebacks as well. I realize
> there's many more unpredictable conditions in the ARMv8 ARM. If anyone
> feels like these are valuable to catch at assemble time, I can go and
> add more checks.
>
> 2014-11-17 Ryan Mansfield <rmansfield@qnx.com>
>
> * config/tc-aarch64.c (md_assemble): Call warn_unpredictable.
> (warn_unpredictable): New.
>
> 2014-11-17 Ryan Mansfield <rmansfield@qnx.com>
>
> * gas/aarch64/diagnostic.s: Add new warnings test patterns.
> * gas/aarch64/diagnostic.l: Update expected diagnostic output.
Ryan,
Thanks for the patch.
While I can see merit in handling common cases like loads/stores in one
function, I'm not entirely convinced that having one common function to
handle all unpredictable insns is a good idea - the function could get
very big over time with no real gain to be had from the single
interface. As such, I'd rename the function to
warn_unpredictable_ldst() or something like that.
OK with that change.
R.
>
> Regards,
>
> Ryan Mansfield
>
>
> unpredictable.diff
>
>
> diff --git a/gas/config/tc-aarch64.c b/gas/config/tc-aarch64.c
> index 4f3fe47..a858bd1 100644
> --- a/gas/config/tc-aarch64.c
> +++ b/gas/config/tc-aarch64.c
> @@ -5488,6 +5488,40 @@ programmer_friendly_fixup (aarch64_instruction *instr)
> return TRUE;
> }
>
> +/* Check for encodings that will cause unpredictable behavior */
> +
> +static void
> +warn_unpredictable (aarch64_instruction *instr, char *str)
> +{
> + aarch64_inst *base = &instr->base;
> + const aarch64_opcode *opcode = base->opcode;
> + const aarch64_opnd_info *opnds = base->operands;
> + switch (opcode->iclass)
> + {
> + case ldst_pos:
> + case ldst_imm9:
> + case ldst_unscaled:
> + case ldst_unpriv:
> + if (opnds[0].reg.regno == opnds[1].reg.regno
> + && opnds[1].addr.writeback)
> + as_warn (_("unpredictable register after writeback -- `%s'"), str);
> + break;
> + case ldstpair_off:
> + case ldstnapair_offs:
> + case ldstpair_indexed:
> + if ((opnds[0].reg.regno == opnds[2].reg.regno
> + || opnds[1].reg.regno == opnds[2].reg.regno)
> + && opnds[2].addr.writeback)
> + as_warn (_("unpredictable register after writeback -- `%s'"), str);
> + if ((opcode->opcode & (1 << 22))
> + && opnds[0].reg.regno == opnds[1].reg.regno)
> + as_warn (_("unpredictable load of register pair -- `%s'"), str);
> + break;
> + default:
> + break;
> + }
> +}
> +
> /* A wrapper function to interface with libopcodes on encoding and
> record the error message if there is any.
>
> @@ -5620,6 +5654,8 @@ md_assemble (char *str)
> return;
> }
>
> + warn_unpredictable (&inst, str);
> +
> if (inst.reloc.type == BFD_RELOC_UNUSED
> || !inst.reloc.need_libopcodes_p)
> output_inst (NULL);
> diff --git a/gas/testsuite/gas/aarch64/diagnostic.l b/gas/testsuite/gas/aarch64/diagnostic.l
> index 322e3c0..d5fdba8 100644
> --- a/gas/testsuite/gas/aarch64/diagnostic.l
> +++ b/gas/testsuite/gas/aarch64/diagnostic.l
> @@ -105,3 +105,12 @@
> [^:]*:109: Error: operand 5 should be an integer register -- `sys #0,c0,c0,#0,kk'
> [^:]*:110: Error: unexpected comma before the omitted optional operand at operand 5 -- `sys #0,c0,c0,#0,'
> [^:]*:112: Error: selected processor does not support `casp w0,w1,w2,w3,\[x4\]'
> +[^:]*:115: Warning: unpredictable load of register pair -- `ldp x0,x0,\[sp\]'
> +[^:]*:116: Warning: unpredictable load of register pair -- `ldp d0,d0,\[sp\]'
> +[^:]*:117: Warning: unpredictable load of register pair -- `ldp x0,x0,\[sp\],#16'
> +[^:]*:118: Warning: unpredictable load of register pair -- `ldnp x0,x0,\[sp\]'
> +[^:]*:121: Warning: unpredictable register after writeback -- `ldr x0,\[x0,#8\]!'
> +[^:]*:122: Warning: unpredictable register after writeback -- `str x0,\[x0,#8\]!'
> +[^:]*:123: Warning: unpredictable register after writeback -- `str x1,\[x1\],#8'
> +[^:]*:124: Warning: unpredictable register after writeback -- `stp x0,x1,\[x0,#16\]!'
> +[^:]*:125: Warning: unpredictable register after writeback -- `ldp x0,x1,\[x1\],#16'
> diff --git a/gas/testsuite/gas/aarch64/diagnostic.s b/gas/testsuite/gas/aarch64/diagnostic.s
> index b82ac8b..88001da 100644
> --- a/gas/testsuite/gas/aarch64/diagnostic.s
> +++ b/gas/testsuite/gas/aarch64/diagnostic.s
> @@ -110,3 +110,16 @@
> sys #0, c0, c0, #0,
>
> casp w0,w1,w2,w3,[x4]
> +
> + # test warning of unpredictable load pairs
> + ldp x0, x0, [sp]
> + ldp d0, d0, [sp]
> + ldp x0, x0, [sp], #16
> + ldnp x0, x0, [sp]
> +
> + # test warning of unpredictable writeback
> + ldr x0, [x0, #8]!
> + str x0, [x0, #8]!
> + str x1, [x1], #8
> + stp x0, x1, [x0, #16]!
> + ldp x0, x1, [x1], #16
>