Re: [AArch64][SVE 12/32] Make more use of bfd_boolean

["Richard Earnshaw (lists)" <Richard dot Earnshaw at arm dot com>]

On 16/09/16 12:56, Richard Sandiford wrote:
> "Richard Earnshaw (lists)" <Richard.Earnshaw@arm.com> writes:
>> On 23/08/16 10:13, Richard Sandiford wrote:
>>> Following on from the previous patch, which converted the
>>> aarch64_reg_parse_32_64 parameters to bfd_booleans, this one
>>> does the same for parse_address_main and parse_address.
>>> It also documents the parameters.
>>>
>>> This isn't an attempt to convert the whole file to use bfd_booleans
>>> more often.  It's simply trying to avoid inconsistencies with new
>>> SVE parameters.
>>>
>>> OK to install?
>>>
>>> Thanks,
>>> Richard
>>>
>>>
>>> gas/
>>> 	* config/tc-aarch64.c (parse_address_main): Turn reloc and
>>> 	accept_reg_post_index into bfd_booleans.  Add commentary.
>>> 	(parse_address_reloc): Update accordingly.  Add commentary.
>>> 	(parse_address): Likewise.  Also change accept_reg_post_index
>>> 	into a bfd_boolean here.
>>> 	(parse_operands): Update calls accordingly.
>>
>> My comment on the previous patch applies somewhat here too, although the
>> two bools are not as closely related here.  In particular statements
>> such as
>>
>>   return parse_address_main (str, operand, TRUE, FALSE);
>>
>> are not intuitively obvious to the reader of the code.
> 
> Yeah...
> 
> I think here too we can just get rid of the parameters and leavve the
> callers to check the addressing modes.  As it happens, the handling of
> ADDR_SIMM9{,_2} already did this for relocation operators (i.e. it used
> parse_address_reloc and then rejected relocations).
> 
> The callers are already set up to reject invalid register post-indexed
> addressing, so we can simply remove the accept_reg_post_index parameter
> without adding any more checks.  This again creates a corner case where:
> 
> 	.equ	x2, 1
> 	ldr	w0, [x1], x2
> 
> was previously an acceptable way of writing "ldr w0, [x1], #1" but
> is now rejected.

IMO that's a good thing.  I presume users can still write

	ldr	w0, [x1], #x2

in this instance and make the nature of x2 being a constant explicit.


> 
> Removing the "reloc" parameter means that two cases need to check
> explicitly for relocation operators.
> 
> ADDR_SIMM9_2 appers to be unused.  I'll send a separate patch
> to remove it.
> 
> This patch makes parse_address temporarily equivalent to
> parse_address_main, but later patches in the series will need
> to keep the distinction.
> 
> Tested on aarch64-linux-gnu.  OK to install?
> 

OK.

R.

> Thanks,
> Richard
> 
> 
> gas/
> 	* config/tc-aarch64.c (parse_address_main): Remove reloc and
> 	accept_reg_post_index parameters.  Parse relocations and register
> 	post indexes unconditionally.
> 	(parse_address): Remove accept_reg_post_index parameter.
> 	Update call to parse_address_main.
> 	(parse_address_reloc): Delete.
> 	(parse_operands): Call parse_address instead of parse_address_main.
> 	Update existing callers of parse_address and make them check
> 	inst.reloc.type where appropriate.
> 	* testsuite/gas/aarch64/diagnostic.s: Add tests for relocations
> 	in ADDR_SIMPLE, SIMD_ADDR_SIMPLE, ADDR_SIMM7 and ADDR_SIMM9 addresses.
> 	Also test for invalid uses of post-index register addressing.
> 	* testsuite/gas/aarch64/diagnostic.l: Update accordingly.
> 
> diff --git a/gas/config/tc-aarch64.c b/gas/config/tc-aarch64.c
> index 7b5be8b..f82fdb9 100644
> --- a/gas/config/tc-aarch64.c
> +++ b/gas/config/tc-aarch64.c
> @@ -3173,8 +3173,7 @@ parse_shifter_operand_reloc (char **str, aarch64_opnd_info *operand,
>     supported by the instruction, and to set inst.reloc.type.  */
>  
>  static bfd_boolean
> -parse_address_main (char **str, aarch64_opnd_info *operand, int reloc,
> -		    int accept_reg_post_index)
> +parse_address_main (char **str, aarch64_opnd_info *operand)
>  {
>    char *p = *str;
>    const reg_entry *reg;
> @@ -3190,7 +3189,7 @@ parse_address_main (char **str, aarch64_opnd_info *operand, int reloc,
>  
>        /* #:<reloc_op>:<symbol>  */
>        skip_past_char (&p, '#');
> -      if (reloc && skip_past_char (&p, ':'))
> +      if (skip_past_char (&p, ':'))
>  	{
>  	  bfd_reloc_code_real_type ty;
>  	  struct reloc_table_entry *entry;
> @@ -3315,7 +3314,7 @@ parse_address_main (char **str, aarch64_opnd_info *operand, int reloc,
>  	{
>  	  /* [Xn,#:<reloc_op>:<symbol>  */
>  	  skip_past_char (&p, '#');
> -	  if (reloc && skip_past_char (&p, ':'))
> +	  if (skip_past_char (&p, ':'))
>  	    {
>  	      struct reloc_table_entry *entry;
>  
> @@ -3388,8 +3387,8 @@ parse_address_main (char **str, aarch64_opnd_info *operand, int reloc,
>  	  return FALSE;
>  	}
>  
> -      if (accept_reg_post_index
> -	  && (reg = aarch64_reg_parse_32_64 (&p, &offset_qualifier)))
> +      reg = aarch64_reg_parse_32_64 (&p, &offset_qualifier);
> +      if (reg)
>  	{
>  	  /* [Xn],Xm */
>  	  if (!aarch64_check_reg_type (reg, REG_TYPE_R_64))
> @@ -3428,19 +3427,12 @@ parse_address_main (char **str, aarch64_opnd_info *operand, int reloc,
>    return TRUE;
>  }
>  
> -/* Return TRUE on success; otherwise return FALSE.  */
> -static bfd_boolean
> -parse_address (char **str, aarch64_opnd_info *operand,
> -	       int accept_reg_post_index)
> -{
> -  return parse_address_main (str, operand, 0, accept_reg_post_index);
> -}
> -
> -/* Return TRUE on success; otherwise return FALSE.  */
> +/* Parse a base AArch64 address (as opposed to an SVE one).  Return TRUE
> +   on success.  */
>  static bfd_boolean
> -parse_address_reloc (char **str, aarch64_opnd_info *operand)
> +parse_address (char **str, aarch64_opnd_info *operand)
>  {
> -  return parse_address_main (str, operand, 1, 0);
> +  return parse_address_main (str, operand);
>  }
>  
>  /* Parse an operand for a MOVZ, MOVN or MOVK instruction.
> @@ -5419,7 +5411,7 @@ parse_operands (char *str, const aarch64_opcode *opcode)
>  	case AARCH64_OPND_ADDR_PCREL19:
>  	case AARCH64_OPND_ADDR_PCREL21:
>  	case AARCH64_OPND_ADDR_PCREL26:
> -	  po_misc_or_fail (parse_address_reloc (&str, info));
> +	  po_misc_or_fail (parse_address (&str, info));
>  	  if (!info->addr.pcrel)
>  	    {
>  	      set_syntax_error (_("invalid pc-relative address"));
> @@ -5490,7 +5482,7 @@ parse_operands (char *str, const aarch64_opcode *opcode)
>  	    char *start = str;
>  	    /* First use the normal address-parsing routines, to get
>  	       the usual syntax errors.  */
> -	    po_misc_or_fail (parse_address (&str, info, 0));
> +	    po_misc_or_fail (parse_address (&str, info));
>  	    if (info->addr.pcrel || info->addr.offset.is_reg
>  		|| !info->addr.preind || info->addr.postind
>  		|| info->addr.writeback)
> @@ -5521,7 +5513,7 @@ parse_operands (char *str, const aarch64_opcode *opcode)
>  
>  	case AARCH64_OPND_ADDR_REGOFF:
>  	  /* [<Xn|SP>, <R><m>{, <extend> {<amount>}}]  */
> -	  po_misc_or_fail (parse_address (&str, info, 0));
> +	  po_misc_or_fail (parse_address (&str, info));
>  	  if (info->addr.pcrel || !info->addr.offset.is_reg
>  	      || !info->addr.preind || info->addr.postind
>  	      || info->addr.writeback)
> @@ -5540,13 +5532,18 @@ parse_operands (char *str, const aarch64_opcode *opcode)
>  	  break;
>  
>  	case AARCH64_OPND_ADDR_SIMM7:
> -	  po_misc_or_fail (parse_address (&str, info, 0));
> +	  po_misc_or_fail (parse_address (&str, info));
>  	  if (info->addr.pcrel || info->addr.offset.is_reg
>  	      || (!info->addr.preind && !info->addr.postind))
>  	    {
>  	      set_syntax_error (_("invalid addressing mode"));
>  	      goto failure;
>  	    }
> +	  if (inst.reloc.type != BFD_RELOC_UNUSED)
> +	    {
> +	      set_syntax_error (_("relocation not allowed"));
> +	      goto failure;
> +	    }
>  	  assign_imm_if_const_or_fixup_later (&inst.reloc, info,
>  					      /* addr_off_p */ 1,
>  					      /* need_libopcodes_p */ 1,
> @@ -5555,7 +5552,7 @@ parse_operands (char *str, const aarch64_opcode *opcode)
>  
>  	case AARCH64_OPND_ADDR_SIMM9:
>  	case AARCH64_OPND_ADDR_SIMM9_2:
> -	  po_misc_or_fail (parse_address_reloc (&str, info));
> +	  po_misc_or_fail (parse_address (&str, info));
>  	  if (info->addr.pcrel || info->addr.offset.is_reg
>  	      || (!info->addr.preind && !info->addr.postind)
>  	      || (operands[i] == AARCH64_OPND_ADDR_SIMM9_2
> @@ -5576,7 +5573,7 @@ parse_operands (char *str, const aarch64_opcode *opcode)
>  	  break;
>  
>  	case AARCH64_OPND_ADDR_UIMM12:
> -	  po_misc_or_fail (parse_address_reloc (&str, info));
> +	  po_misc_or_fail (parse_address (&str, info));
>  	  if (info->addr.pcrel || info->addr.offset.is_reg
>  	      || !info->addr.preind || info->addr.writeback)
>  	    {
> @@ -5596,7 +5593,7 @@ parse_operands (char *str, const aarch64_opcode *opcode)
>  
>  	case AARCH64_OPND_SIMD_ADDR_POST:
>  	  /* [<Xn|SP>], <Xm|#<amount>>  */
> -	  po_misc_or_fail (parse_address (&str, info, 1));
> +	  po_misc_or_fail (parse_address (&str, info));
>  	  if (!info->addr.postind || !info->addr.writeback)
>  	    {
>  	      set_syntax_error (_("invalid addressing mode"));
> diff --git a/gas/testsuite/gas/aarch64/diagnostic.l b/gas/testsuite/gas/aarch64/diagnostic.l
> index ef23577..0fb4db9 100644
> --- a/gas/testsuite/gas/aarch64/diagnostic.l
> +++ b/gas/testsuite/gas/aarch64/diagnostic.l
> @@ -150,3 +150,11 @@
>  [^:]*:264: Error: invalid floating-point constant at operand 2 -- `fmov s0,-2'
>  [^:]*:266: Error: integer 64-bit register expected at operand 2 -- `st2 {v0.4s,v1.4s},\[sp\],xzr'
>  [^:]*:267: Error: integer or zero register expected at operand 2 -- `str x1,\[x2,sp\]'
> +[^:]*:270: Error: relocation not allowed at operand 3 -- `ldnp x1,x2,\[x3,#:lo12:foo\]'
> +[^:]*:271: Error: invalid addressing mode at operand 2 -- `ld1 {v0\.4s},\[x3,#:lo12:foo\]'
> +[^:]*:272: Error: the optional immediate offset can only be 0 at operand 2 -- `stuminl x0,\[x3,#:lo12:foo\]'
> +[^:]*:273: Error: relocation not allowed at operand 2 -- `prfum pldl1keep,\[x3,#:lo12:foo\]'
> +[^:]*:275: Error: invalid addressing mode at operand 2 -- `ldr x0,\[x3\],x4'
> +[^:]*:276: Error: invalid addressing mode at operand 3 -- `ldnp x1,x2,\[x3\],x4'
> +[^:]*:278: Error: invalid addressing mode at operand 2 -- `stuminl x0,\[x3\],x4'
> +[^:]*:279: Error: invalid addressing mode at operand 2 -- `prfum pldl1keep,\[x3\],x4'
> diff --git a/gas/testsuite/gas/aarch64/diagnostic.s b/gas/testsuite/gas/aarch64/diagnostic.s
> index 8dbb542..a9cd124 100644
> --- a/gas/testsuite/gas/aarch64/diagnostic.s
> +++ b/gas/testsuite/gas/aarch64/diagnostic.s
> @@ -265,3 +265,15 @@
>  
>  	st2	{v0.4s, v1.4s}, [sp], xzr
>  	str	x1, [x2, sp]
> +
> +	ldr	x0, [x1, #:lo12:foo] // OK
> +	ldnp	x1, x2, [x3, #:lo12:foo]
> +	ld1	{v0.4s}, [x3, #:lo12:foo]
> +	stuminl x0, [x3, #:lo12:foo]
> +	prfum	pldl1keep, [x3, #:lo12:foo]
> +
> +	ldr	x0, [x3], x4
> +	ldnp	x1, x2, [x3], x4
> +	ld1	{v0.4s}, [x3], x4 // OK
> +	stuminl x0, [x3], x4
> +	prfum	pldl1keep, [x3], x4
>