[PATCH, RFC] PPC sync instruction accepts invalid and incompatible operands
Edmar
edmar@freescale.com
Mon Jun 22 15:44:00 GMT 2015
Hi Peter,
There is no particular reason to the current behaviour.
Thanks for noticing and fixing this.
Edmar
On 06/19/2015 09:50 PM, Peter Bergner wrote:
> Edmar, there is a question for you below...
>
> ISA 2.07 added a new category called Elemental Memory Barriers that modifies
> the sync instruction to accept an additional operand ESYNC. Edmar added
> support for this instruction variant here:
>
> https://sourceware.org/ml/binutils/2012-02/msg00221.html
>
> Looking at this closer, I see that the insert_ls() function is misnamed
> (since it's attached to the ESYNC operand, not the LS operand) but more
> importantly, it is silently modifying the LS operand value behind the
> users back when the LS operand is either invalid or is incompatible with
> the new ESYNC operand. The ISA 2.07 doc has an Assembler Note that clearly
> states that assemblers that support the ESYNC operand should report all
> invalid uses of LS and ESYNC.
>
> Edmar, is there a reason you need to silently modify the LS operand when
> it is invalid and/or incompatible with the ESYNC value, rather than just
> reporting the error like the following patch does?
>
> Clearly, the e6500.s "sync 2,0" is invalid for the E6500, since only
> server cpus implement LS==2 (ie, ptesync), but shouldn't we flag that
> as an error, rather than silently changing LS to 1? Maybe it was just
> a typo on the users part and they meant to type "sync 0,2", which is
> valid and now they don't know they actually got a "sync 1,0" instead.
> Ditto for the sync tests that pass LS==3, which is reserved on all cpus.
>
> Peter
>
>
> opcodes/
> * ppc-opc.c (insert_ls): Test for invalid LS operands.
> (insert_esync): New function.
> (LS, WC): Use insert_ls.
> (ESYNC): Use insert_esync.
>
> gas/testsuite/
> * gas/ppc/e6500.s<sync>: Fix invalid test.
> * gas/ppc/e6500.d: Likewise.
>
>
> diff --git a/opcodes/ppc-opc.c b/opcodes/ppc-opc.c
> index 9689a81..601ec2f 100644
> --- a/opcodes/ppc-opc.c
> +++ b/opcodes/ppc-opc.c
> @@ -53,6 +53,7 @@ static unsigned long insert_bo (unsigned long, long, ppc_cpu_t, const char **);
> static long extract_bo (unsigned long, ppc_cpu_t, int *);
> static unsigned long insert_boe (unsigned long, long, ppc_cpu_t, const char **);
> static long extract_boe (unsigned long, ppc_cpu_t, int *);
> +static unsigned long insert_esync (unsigned long, long, ppc_cpu_t, const char **);
> static unsigned long insert_fxm (unsigned long, long, ppc_cpu_t, const char **);
> static long extract_fxm (unsigned long, ppc_cpu_t, int *);
> static unsigned long insert_li20 (unsigned long, long, ppc_cpu_t, const char **);
> @@ -417,7 +418,7 @@ const struct powerpc_operand powerpc_operands[] =
> /* The LS or WC field in an X (sync or wait) form instruction. */
> #define LS LIA + 1
> #define WC LS
> - { 0x3, 21, NULL, NULL, PPC_OPERAND_OPTIONAL },
> + { 0x3, 21, insert_ls, NULL, PPC_OPERAND_OPTIONAL },
>
> /* The ME field in an M form instruction. */
> #define ME LS + 1
> @@ -635,7 +636,7 @@ const struct powerpc_operand powerpc_operands[] =
>
> /* The ESYNC field in an X (sync) form instruction. */
> #define ESYNC STRM + 1
> - { 0xf, 16, insert_ls, NULL, PPC_OPERAND_OPTIONAL },
> + { 0xf, 16, insert_esync, NULL, PPC_OPERAND_OPTIONAL },
>
> /* The SV field in a POWER SC form instruction. */
> #define SV ESYNC + 1
> @@ -1365,17 +1366,40 @@ extract_li20 (unsigned long insn,
> | (insn& 0x7ff);
> }
>
> -/* The LS field in a sync instruction that accepts 2 operands
> - Values 2 and 3 are reserved,
> - must be treated as 0 for future compatibility
> - Values 0 and 1 can be accepted, if field ESYNC is zero
> - Otherwise L = complement of ESYNC-bit2 (1<<18) */
> +/* The 2-bit L field in a SYNC or WC field in a WAIT instruction.
> + For SYNC, some L values are reserved:
> + * Value 3 is reserved on newer server cpus.
> + * Values 2 and 3 are reserved on all other cpus. */
>
> static unsigned long
> insert_ls (unsigned long insn,
> long value,
> - ppc_cpu_t dialect ATTRIBUTE_UNUSED,
> - const char **errmsg ATTRIBUTE_UNUSED)
> + ppc_cpu_t dialect,
> + const char **errmsg)
> +{
> + /* For SYNC, some L values are illegal. */
> + if (((insn>> 1)& 0x3ff) == 598)
> + {
> + long max_value = (dialect& PPC_OPCODE_POWER4) ? 2 : 1;
> + if (value> max_value)
> + {
> + *errmsg = _("illegal L operand value");
> + return insn;
> + }
> + }
> +
> + return insn | ((value& 0x3)<< 21);
> +}
> +
> +/* The 4-bit E field in a sync instruction that accepts 2 operands.
> + If ESYNC is non-zero, then the L field must be either 0 or 1 and
> + the complement of ESYNC-bit2. */
> +
> +static unsigned long
> +insert_esync (unsigned long insn,
> + long value,
> + ppc_cpu_t dialect ATTRIBUTE_UNUSED,
> + const char **errmsg)
> {
> unsigned long ls;
>
> @@ -1383,12 +1407,15 @@ insert_ls (unsigned long insn,
> if (value == 0)
> {
> if (ls> 1)
> - return insn& ~(0x3<< 21);
> + *errmsg = _("illegal L operand value");
> return insn;
> }
> - if ((value& 0x2) != 0)
> - return (insn& ~(0x3<< 21)) | ((value& 0xf)<< 16);
> - return (insn& ~(0x3<< 21)) | (0x1<< 21) | ((value& 0xf)<< 16);
> +
> + if ((ls& ~0x1)
> + || (((value>> 1)& 0x1) ^ ls) == 0)
> + *errmsg = _("incompatible L operand value");
> +
> + return insn | ((value& 0xf)<< 16);
> }
>
> /* The MB and ME fields in an M form instruction expressed as a single
> @@ -2024,6 +2051,7 @@ extract_dm (unsigned long insn,
> *invalid = 1;
> return (value) ? 1 : 0;
> }
> +
> /* The VLESIMM field in an I16A form instruction. This is split. */
>
> static unsigned long
> diff --git a/gas/testsuite/gas/ppc/e6500.s b/gas/testsuite/gas/ppc/e6500.s
> index ceee777..2167cc6 100644
> --- a/gas/testsuite/gas/ppc/e6500.s
> +++ b/gas/testsuite/gas/ppc/e6500.s
> @@ -56,9 +56,9 @@ start:
> sync
> sync 0,0
> sync 1,0
> - sync 2,0
> - sync 3,7
> - sync 3,8
> + sync 1,1
> + sync 0,7
> + sync 1,8
> dni 0,0
> dni 31,31
> dcblq. 2,0,1
> diff --git a/gas/testsuite/gas/ppc/e6500.d b/gas/testsuite/gas/ppc/e6500.d
> index 48b0001..c8d8f57 100644
> --- a/gas/testsuite/gas/ppc/e6500.d
> +++ b/gas/testsuite/gas/ppc/e6500.d
> @@ -62,7 +62,7 @@ Disassembly of section \.text:
> d0: (7c 00 04 ac|ac 04 00 7c) sync
> d4: (7c 00 04 ac|ac 04 00 7c) sync
> d8: (7c 20 04 ac|ac 04 20 7c) lwsync
> - dc: (7c 00 04 ac|ac 04 00 7c) sync
> + dc: (7c 21 04 ac|ac 04 21 7c) sync 1,1
> e0: (7c 07 04 ac|ac 04 07 7c) sync 0,7
> e4: (7c 28 04 ac|ac 04 28 7c) sync 1,8
> e8: (7c 00 00 c3|c3 00 00 7c) dni 0,0
>
>
> .
>
More information about the Binutils
mailing list