[PATCH v2 0/7] RISC-V/gas: insn operand parsing

[PATCH v2 0/7] RISC-V/gas: insn operand parsing

[Jan Beulich]

(v1 series was "re-work register named symbols avoidance logic")

This addresses some of the anomalies I've observed. There continue to
be questions towards consistent overall behavior - see remarks in the
individual patches.

An assembler with the full series in place was used in a gcc 12.2.0
testsuite run (cross build on x86, so no execution tests), resulting
in no new test failures (there were a number of pre-existing ones,
though).

1: minor effort reduction in relocation specifier parsing
2: drop "percent_op" parameter from my_getOpcodeExpression()
3: avoid redundant and misleading/wrong error messages
4: don't recognize bogus relocations
5: relax post-relocation-operator separator expectation
6: test for expected / no unexpected symbols
7: adjust logic to avoid register name symbols

Jan

[PATCH v2 1/7] RISC-V: minor effort reduction in relocation specifier parsing

[Jan Beulich]

The sole caller of parse_relocation() has already checked for the %
prefix, so there's no need to check for it again in the strncasecmp()
and there's also no reason to make the involved string literals longer
than necessary.
---
v2: New.

--- a/gas/config/tc-riscv.c
+++ b/gas/config/tc-riscv.c
@@ -2135,34 +2135,34 @@ macro (struct riscv_cl_insn *ip, express
 
 static const struct percent_op_match percent_op_utype[] =
 {
-  {"%tprel_hi", BFD_RELOC_RISCV_TPREL_HI20},
-  {"%pcrel_hi", BFD_RELOC_RISCV_PCREL_HI20},
-  {"%got_pcrel_hi", BFD_RELOC_RISCV_GOT_HI20},
-  {"%tls_ie_pcrel_hi", BFD_RELOC_RISCV_TLS_GOT_HI20},
-  {"%tls_gd_pcrel_hi", BFD_RELOC_RISCV_TLS_GD_HI20},
-  {"%hi", BFD_RELOC_RISCV_HI20},
+  {"tprel_hi", BFD_RELOC_RISCV_TPREL_HI20},
+  {"pcrel_hi", BFD_RELOC_RISCV_PCREL_HI20},
+  {"got_pcrel_hi", BFD_RELOC_RISCV_GOT_HI20},
+  {"tls_ie_pcrel_hi", BFD_RELOC_RISCV_TLS_GOT_HI20},
+  {"tls_gd_pcrel_hi", BFD_RELOC_RISCV_TLS_GD_HI20},
+  {"hi", BFD_RELOC_RISCV_HI20},
   {0, 0}
 };
 
 static const struct percent_op_match percent_op_itype[] =
 {
-  {"%lo", BFD_RELOC_RISCV_LO12_I},
-  {"%tprel_lo", BFD_RELOC_RISCV_TPREL_LO12_I},
-  {"%pcrel_lo", BFD_RELOC_RISCV_PCREL_LO12_I},
+  {"lo", BFD_RELOC_RISCV_LO12_I},
+  {"tprel_lo", BFD_RELOC_RISCV_TPREL_LO12_I},
+  {"pcrel_lo", BFD_RELOC_RISCV_PCREL_LO12_I},
   {0, 0}
 };
 
 static const struct percent_op_match percent_op_stype[] =
 {
-  {"%lo", BFD_RELOC_RISCV_LO12_S},
-  {"%tprel_lo", BFD_RELOC_RISCV_TPREL_LO12_S},
-  {"%pcrel_lo", BFD_RELOC_RISCV_PCREL_LO12_S},
+  {"lo", BFD_RELOC_RISCV_LO12_S},
+  {"tprel_lo", BFD_RELOC_RISCV_TPREL_LO12_S},
+  {"pcrel_lo", BFD_RELOC_RISCV_PCREL_LO12_S},
   {0, 0}
 };
 
 static const struct percent_op_match percent_op_rtype[] =
 {
-  {"%tprel_add", BFD_RELOC_RISCV_TPREL_ADD},
+  {"tprel_add", BFD_RELOC_RISCV_TPREL_ADD},
   {0, 0}
 };
 
@@ -2180,14 +2180,14 @@ parse_relocation (char **str, bfd_reloc_
 		  const struct percent_op_match *percent_op)
 {
   for ( ; percent_op->str; percent_op++)
-    if (strncasecmp (*str, percent_op->str, strlen (percent_op->str)) == 0)
+    if (strncasecmp (*str + 1, percent_op->str, strlen (percent_op->str)) == 0)
       {
-	int len = strlen (percent_op->str);
+	size_t len = 1 + strlen (percent_op->str);
 
 	if (!ISSPACE ((*str)[len]) && (*str)[len] != '(')
 	  continue;
 
-	*str += strlen (percent_op->str);
+	*str += len;
 	*reloc = percent_op->reloc;
 
 	/* Check whether the output BFD supports this relocation.

Re: [PATCH v2 1/7] RISC-V: minor effort reduction in relocation specifier parsing

[Jan Beulich]

On 10.03.2023 10:25, Jan Beulich via Binutils wrote:
> The sole caller of parse_relocation() has already checked for the %
> prefix, so there's no need to check for it again in the strncasecmp()
> and there's also no reason to make the involved string literals longer
> than necessary.

Taking this together with the .altmacro observation in
https://sourceware.org/pipermail/binutils/2023-March/126601.html
it may be worthwhile to consider allowing a 2nd prefix character
for relocation specifiers (e.g. '@'), such that they could be
passed as macro arguments without conflicting with the % operator.
The change here is effectively laying the grounds for very easily
doing so (might then be as simple as a 1-line change).

Jan

> ---
> v2: New.
> 
> --- a/gas/config/tc-riscv.c
> +++ b/gas/config/tc-riscv.c
> @@ -2135,34 +2135,34 @@ macro (struct riscv_cl_insn *ip, express
>  
>  static const struct percent_op_match percent_op_utype[] =
>  {
> -  {"%tprel_hi", BFD_RELOC_RISCV_TPREL_HI20},
> -  {"%pcrel_hi", BFD_RELOC_RISCV_PCREL_HI20},
> -  {"%got_pcrel_hi", BFD_RELOC_RISCV_GOT_HI20},
> -  {"%tls_ie_pcrel_hi", BFD_RELOC_RISCV_TLS_GOT_HI20},
> -  {"%tls_gd_pcrel_hi", BFD_RELOC_RISCV_TLS_GD_HI20},
> -  {"%hi", BFD_RELOC_RISCV_HI20},
> +  {"tprel_hi", BFD_RELOC_RISCV_TPREL_HI20},
> +  {"pcrel_hi", BFD_RELOC_RISCV_PCREL_HI20},
> +  {"got_pcrel_hi", BFD_RELOC_RISCV_GOT_HI20},
> +  {"tls_ie_pcrel_hi", BFD_RELOC_RISCV_TLS_GOT_HI20},
> +  {"tls_gd_pcrel_hi", BFD_RELOC_RISCV_TLS_GD_HI20},
> +  {"hi", BFD_RELOC_RISCV_HI20},
>    {0, 0}
>  };
>  
>  static const struct percent_op_match percent_op_itype[] =
>  {
> -  {"%lo", BFD_RELOC_RISCV_LO12_I},
> -  {"%tprel_lo", BFD_RELOC_RISCV_TPREL_LO12_I},
> -  {"%pcrel_lo", BFD_RELOC_RISCV_PCREL_LO12_I},
> +  {"lo", BFD_RELOC_RISCV_LO12_I},
> +  {"tprel_lo", BFD_RELOC_RISCV_TPREL_LO12_I},
> +  {"pcrel_lo", BFD_RELOC_RISCV_PCREL_LO12_I},
>    {0, 0}
>  };
>  
>  static const struct percent_op_match percent_op_stype[] =
>  {
> -  {"%lo", BFD_RELOC_RISCV_LO12_S},
> -  {"%tprel_lo", BFD_RELOC_RISCV_TPREL_LO12_S},
> -  {"%pcrel_lo", BFD_RELOC_RISCV_PCREL_LO12_S},
> +  {"lo", BFD_RELOC_RISCV_LO12_S},
> +  {"tprel_lo", BFD_RELOC_RISCV_TPREL_LO12_S},
> +  {"pcrel_lo", BFD_RELOC_RISCV_PCREL_LO12_S},
>    {0, 0}
>  };
>  
>  static const struct percent_op_match percent_op_rtype[] =
>  {
> -  {"%tprel_add", BFD_RELOC_RISCV_TPREL_ADD},
> +  {"tprel_add", BFD_RELOC_RISCV_TPREL_ADD},
>    {0, 0}
>  };
>  
> @@ -2180,14 +2180,14 @@ parse_relocation (char **str, bfd_reloc_
>  		  const struct percent_op_match *percent_op)
>  {
>    for ( ; percent_op->str; percent_op++)
> -    if (strncasecmp (*str, percent_op->str, strlen (percent_op->str)) == 0)
> +    if (strncasecmp (*str + 1, percent_op->str, strlen (percent_op->str)) == 0)
>        {
> -	int len = strlen (percent_op->str);
> +	size_t len = 1 + strlen (percent_op->str);
>  
>  	if (!ISSPACE ((*str)[len]) && (*str)[len] != '(')
>  	  continue;
>  
> -	*str += strlen (percent_op->str);
> +	*str += len;
>  	*reloc = percent_op->reloc;
>  
>  	/* Check whether the output BFD supports this relocation.
> 

[PATCH v2 2/7] RISC-V: drop "percent_op" parameter from my_getOpcodeExpression()

[Jan Beulich]

Both callers check for no relocations, so there's no point parsing for
some. Have the function pass percent_op_null into
my_getSmallExpression(). Note that there's no point passing
percent_op_itype: Elsewhere, especially when processing compressed alias
insns ahead of non-alias ones, this has the effect of avoiding "bad
expression" errors when another parsing pass may follow (and succeed).
Here, however, all alternative forms of an insn type will again start
with the same O4 or O2, so avoiding errors earlier on doesn't really
help. Plus constructs with a relocation specifier (as percent_op_itype
would permit) can't be specified anyway, as the scrubber eats the
whitespace between .insn's type and the O4 or O2 expression when that
starts with % or ( - i.e. these will be seen as e.g. "i%lo(x)", and
riscv_ip() looks only for whitespace when finding the end of a mnemonic.
---
There are pre-existing anomalies with redundant errors being reported,
up to one per insn form tried for a single source .insn or "normal"
insn. The change here merely widens that issue a little bit; the issue
wants getting under control elsewhere (and centrally).
---
v2: New.

--- a/gas/config/tc-riscv.c
+++ b/gas/config/tc-riscv.c
@@ -2285,7 +2285,7 @@ my_getSmallExpression (expressionS *ep,
 
 static size_t
 my_getOpcodeExpression (expressionS *ep, bfd_reloc_code_real_type *reloc,
-			char *str, const struct percent_op_match *percent_op)
+			char *str)
 {
   const struct opcode_name_t *o = opcode_name_lookup (&str);
 
@@ -2296,7 +2296,7 @@ my_getOpcodeExpression (expressionS *ep,
       return 0;
     }
 
-  return my_getSmallExpression (ep, reloc, str, percent_op);
+  return my_getSmallExpression (ep, reloc, str, percent_op_null);
 }
 
 /* Parse string STR as a vsetvli operand.  Store the expression in *EP.
@@ -3300,7 +3300,7 @@ riscv_ip (char *str, struct riscv_cl_ins
 	      switch (*++oparg)
 		{
 		case '4':
-		  if (my_getOpcodeExpression (imm_expr, imm_reloc, asarg, p)
+		  if (my_getOpcodeExpression (imm_expr, imm_reloc, asarg)
 		      || imm_expr->X_op != O_constant
 		      || imm_expr->X_add_number < 0
 		      || imm_expr->X_add_number >= 128
@@ -3317,7 +3317,7 @@ riscv_ip (char *str, struct riscv_cl_ins
 		  continue;
 
 		case '2':
-		  if (my_getOpcodeExpression (imm_expr, imm_reloc, asarg, p)
+		  if (my_getOpcodeExpression (imm_expr, imm_reloc, asarg)
 		      || imm_expr->X_op != O_constant
 		      || imm_expr->X_add_number < 0
 		      || imm_expr->X_add_number >= 3)

[PATCH v2 3/7] RISC-V: avoid redundant and misleading/wrong error messages

[Jan Beulich]

The use of a wrong (for the insn) relocation operator (or a future one
which simply isn't recognized by older gas yet) doesn't render the (rest
of the) expression "bad". Furthermore alongside the error from
expression() in most cases the parser would emit another error then
anyway. Suppress the call to my_getExpression() in such a case,
arranging for a guaranteed subsequent error message by marking the
expression "illegal".
---
Of course superfluous "bad expression" errors remain for format
specifiers where my_getExpression() is used directly. I guess once the
GPR special casing has disappeared from my_getSmallExpression() (see
"RISC-V: adjust logic to avoid register name symbols"), both functions
could be folded (which would also eliminate the unclear split between
when which of the two is used).
---
v2: New.

--- a/gas/config/tc-riscv.c
+++ b/gas/config/tc-riscv.c
@@ -2265,6 +2265,15 @@ my_getSmallExpression (expressionS *ep,
 	 && reloc_index < 1
 	 && parse_relocation (&str, reloc, percent_op));
 
+  if (*str == '%')
+    {
+       /* expression() will choke on anything looking like an (unrecognized)
+	  relocation specifier.  Don't even call it, avoiding multiple (and
+	  perhaps redundant) error messages; our caller will issue one.  */
+       ep->X_op = O_illegal;
+       return 0;
+    }
+
   my_getExpression (ep, crux);
   str = expr_parse_end;
 
--- a/gas/testsuite/gas/riscv/tprel-add.l
+++ b/gas/testsuite/gas/riscv/tprel-add.l
@@ -1,4 +1,3 @@
 .*: Assembler messages:
-.*: Error: bad expression
 .*: Error: illegal operands `amoadd.w x8,x9,%tprel_add\(i\)\(x10\)'
 .*: Error: illegal operands `add a5,a5,tp,0'

[PATCH v2 4/7] RISC-V: don't recognize bogus relocations

[Jan Beulich]

With my_getSmallExpression() consistently and silently failing on
relocation operators not fitting an insn, it is no longer necessary to
hand it percent_op_itype[] "just in case" (i.e. to avoid errors when a
subsequent parsing attempt for another operand combination might
succeed). This also eliminates the latent problem of percent_op_itype[]
and percent_op_stype[] growing a non-identical set of recognized
relocation operators.
---
v2: New.

--- a/gas/config/tc-riscv.c
+++ b/gas/config/tc-riscv.c
@@ -2517,7 +2517,7 @@ riscv_ip (char *str, struct riscv_cl_ins
 
       imm_expr->X_op = O_absent;
       *imm_reloc = BFD_RELOC_UNUSED;
-      p = percent_op_itype;
+      p = percent_op_null;
 
       for (oparg = insn->args;; ++oparg)
 	{
@@ -3233,7 +3233,6 @@ riscv_ip (char *str, struct riscv_cl_ins
 	      p = percent_op_rtype;
 	      goto alu_op;
 	    case '0': /* AMO displacement, which must be zero.  */
-	      p = percent_op_null;
 	    load_store:
 	      if (riscv_handle_implicit_zero_offset (imm_expr, asarg))
 		continue;

[PATCH v2 5/7] RISC-V: relax post-relocation-operator separator expectation

[Jan Beulich]

With a blank being okay as a separator, constructs like

	lui	t0, %hi sym
	lui	t0, %hi 0x1000

are accepted. But then it makes little sense to not also accept e.g.

	lui	t0, %hi +sym
	lui	t0, %hi -0x1000

Therefore instead of looking for a blank or opening parenthesis, merely
check whether what follows wouldn't continue an identifier.

Note that we don't need to also check is_name_ender(), as LEX_END_NAME
isn't used for any character.
---
v2: New.

--- a/gas/config/tc-riscv.c
+++ b/gas/config/tc-riscv.c
@@ -2184,7 +2184,7 @@ parse_relocation (char **str, bfd_reloc_
       {
 	size_t len = 1 + strlen (percent_op->str);
 
-	if (!ISSPACE ((*str)[len]) && (*str)[len] != '(')
+	if (is_part_of_name ((*str)[len]))
 	  continue;
 
 	*str += len;
--- a/gas/testsuite/gas/riscv/auipc-parsing.d
+++ b/gas/testsuite/gas/riscv/auipc-parsing.d
@@ -1,3 +1,3 @@
-#as:
+#as: -al
 #source: auipc-parsing.s
 #error_output: auipc-parsing.l
--- a/gas/testsuite/gas/riscv/auipc-parsing.l
+++ b/gas/testsuite/gas/riscv/auipc-parsing.l
@@ -3,3 +3,7 @@
 .*: Error: illegal operands `lui x10,x11'
 .*: Error: illegal operands `auipc x12,symbol'
 .*: Error: illegal operands `lui x13,symbol'
+#...
+ *[0-9]+[ 	]+# Accept unary .*
+ *[0-9]+[ 	]+\?\?\?\? 17070000[ 	]+auipc	x14,%hi \+symbol
+ *[0-9]+[ 	]+\?\?\?\? B7B7CBED[ 	]+lui	x15,%hi -0x12345678
--- a/gas/testsuite/gas/riscv/auipc-parsing.s
+++ b/gas/testsuite/gas/riscv/auipc-parsing.s
@@ -4,3 +4,6 @@
 # Don't accept a symbol without %hi() for 'u' operands.
 	auipc	x12,symbol
 	lui	x13,symbol
+# Accept unary operators starting the subject expression.
+	auipc	x14,%hi +symbol
+	lui	x15,%hi -0x12345678

Re: [PATCH v2 5/7] RISC-V: relax post-relocation-operator separator expectation

[Nelson Chu]

[-- Attachment #1: Type: text/plain, Size: 2249 bytes --]

On Fri, Mar 10, 2023 at 5:27 PM Jan Beulich <jbeulich@suse.com> wrote:

> With a blank being okay as a separator, constructs like
>
>         lui     t0, %hi sym
>         lui     t0, %hi 0x1000
>

I never noticed this kind of usage.  I always thought we should have a ()
after the %hi/%pcrel_hi/..., so perhaps this is just a mistake that should
be fixed.  Maybe other experts can help to clarify this behavior.

Thanks
Nelson


> are accepted. But then it makes little sense to not also accept e.g.
>
>         lui     t0, %hi +sym
>         lui     t0, %hi -0x1000
>
> Therefore instead of looking for a blank or opening parenthesis, merely
> check whether what follows wouldn't continue an identifier.
>
> Note that we don't need to also check is_name_ender(), as LEX_END_NAME
> isn't used for any character.
> ---
> v2: New.
>
> --- a/gas/config/tc-riscv.c
> +++ b/gas/config/tc-riscv.c
> @@ -2184,7 +2184,7 @@ parse_relocation (char **str, bfd_reloc_
>        {
>         size_t len = 1 + strlen (percent_op->str);
>
> -       if (!ISSPACE ((*str)[len]) && (*str)[len] != '(')
> +       if (is_part_of_name ((*str)[len]))
>           continue;
>
>         *str += len;
> --- a/gas/testsuite/gas/riscv/auipc-parsing.d
> +++ b/gas/testsuite/gas/riscv/auipc-parsing.d
> @@ -1,3 +1,3 @@
> -#as:
> +#as: -al
>  #source: auipc-parsing.s
>  #error_output: auipc-parsing.l
> --- a/gas/testsuite/gas/riscv/auipc-parsing.l
> +++ b/gas/testsuite/gas/riscv/auipc-parsing.l
> @@ -3,3 +3,7 @@
>  .*: Error: illegal operands `lui x10,x11'
>  .*: Error: illegal operands `auipc x12,symbol'
>  .*: Error: illegal operands `lui x13,symbol'
> +#...
> + *[0-9]+[      ]+# Accept unary .*
> + *[0-9]+[      ]+\?\?\?\? 17070000[    ]+auipc x14,%hi \+symbol
> + *[0-9]+[      ]+\?\?\?\? B7B7CBED[    ]+lui   x15,%hi -0x12345678
> --- a/gas/testsuite/gas/riscv/auipc-parsing.s
> +++ b/gas/testsuite/gas/riscv/auipc-parsing.s
> @@ -4,3 +4,6 @@
>  # Don't accept a symbol without %hi() for 'u' operands.
>         auipc   x12,symbol
>         lui     x13,symbol
> +# Accept unary operators starting the subject expression.
> +       auipc   x14,%hi +symbol
> +       lui     x15,%hi -0x12345678
>
>

Re: [PATCH v2 5/7] RISC-V: relax post-relocation-operator separator expectation

[Andreas Schwab]

On Mär 10 2023, Jan Beulich via Binutils wrote:

> With a blank being okay as a separator, constructs like
>
> 	lui	t0, %hi sym
> 	lui	t0, %hi 0x1000
>
> are accepted. But then it makes little sense to not also accept e.g.
>
> 	lui	t0, %hi +sym
> 	lui	t0, %hi -0x1000

https://github.com/riscv-non-isa/riscv-asm-manual/blob/master/riscv-asm.md
always includes the parens around the expression, and I don't think it
should be accepted without them.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510  2552 DF73 E780 A9DA AEC1
"And now for something completely different."

Re: [PATCH v2 5/7] RISC-V: relax post-relocation-operator separator expectation

[Nelson Chu]

[-- Attachment #1: Type: text/plain, Size: 947 bytes --]

Thanks for the information, Andreas.  The riscv-asm should be the right
place to see the expected behaviors, and discuss the related stuff :-)

Nelson

On Tue, Apr 25, 2023 at 4:37 PM Andreas Schwab <schwab@linux-m68k.org>
wrote:

> On Mär 10 2023, Jan Beulich via Binutils wrote:
>
> > With a blank being okay as a separator, constructs like
> >
> >       lui     t0, %hi sym
> >       lui     t0, %hi 0x1000
> >
> > are accepted. But then it makes little sense to not also accept e.g.
> >
> >       lui     t0, %hi +sym
> >       lui     t0, %hi -0x1000
>
> https://github.com/riscv-non-isa/riscv-asm-manual/blob/master/riscv-asm.md
> always includes the parens around the expression, and I don't think it
> should be accepted without them.
>
> --
> Andreas Schwab, schwab@linux-m68k.org
> GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510  2552 DF73 E780 A9DA AEC1
> "And now for something completely different."
>

Re: [PATCH v2 5/7] RISC-V: relax post-relocation-operator separator expectation

[Jan Beulich]

On 25.04.2023 10:44, Nelson Chu wrote:
> Thanks for the information, Andreas.  The riscv-asm should be the right
> place to see the expected behaviors, and discuss the related stuff :-)

Just to clarify: Is this a request to replace this patch by one insisting
on the use of a parenthesis? If so, I can certainly do that, and commit
only the remaining patches (as per your other reply).

Jan

> On Tue, Apr 25, 2023 at 4:37 PM Andreas Schwab <schwab@linux-m68k.org>
> wrote:
> 
>> On Mär 10 2023, Jan Beulich via Binutils wrote:
>>
>>> With a blank being okay as a separator, constructs like
>>>
>>>       lui     t0, %hi sym
>>>       lui     t0, %hi 0x1000
>>>
>>> are accepted. But then it makes little sense to not also accept e.g.
>>>
>>>       lui     t0, %hi +sym
>>>       lui     t0, %hi -0x1000
>>
>> https://github.com/riscv-non-isa/riscv-asm-manual/blob/master/riscv-asm.md
>> always includes the parens around the expression, and I don't think it
>> should be accepted without them.
>>
>> --
>> Andreas Schwab, schwab@linux-m68k.org
>> GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510  2552 DF73 E780 A9DA AEC1
>> "And now for something completely different."
>>
> 

Re: [PATCH v2 5/7] RISC-V: relax post-relocation-operator separator expectation

[Nelson Chu]

[-- Attachment #1: Type: text/plain, Size: 1702 bytes --]

Sorry for missing this one.  No, not a request.  I'm fine with both, since
this patch won't break the risc-gnu-toolchain regression, so most of the
stuff already used looks good.  It seems that the risc v-asm doesn't say we
"must '' use a parenthesis after the modifiers, it just shows examples that
all use it.

Thanks
Nelson

On Tue, Apr 25, 2023 at 4:52 PM Jan Beulich <jbeulich@suse.com> wrote:

> On 25.04.2023 10:44, Nelson Chu wrote:
> > Thanks for the information, Andreas.  The riscv-asm should be the right
> > place to see the expected behaviors, and discuss the related stuff :-)
>
> Just to clarify: Is this a request to replace this patch by one insisting
> on the use of a parenthesis? If so, I can certainly do that, and commit
> only the remaining patches (as per your other reply).
>
> Jan
>
> > On Tue, Apr 25, 2023 at 4:37 PM Andreas Schwab <schwab@linux-m68k.org>
> > wrote:
> >
> >> On Mär 10 2023, Jan Beulich via Binutils wrote:
> >>
> >>> With a blank being okay as a separator, constructs like
> >>>
> >>>       lui     t0, %hi sym
> >>>       lui     t0, %hi 0x1000
> >>>
> >>> are accepted. But then it makes little sense to not also accept e.g.
> >>>
> >>>       lui     t0, %hi +sym
> >>>       lui     t0, %hi -0x1000
> >>
> >>
> https://github.com/riscv-non-isa/riscv-asm-manual/blob/master/riscv-asm.md
> >> always includes the parens around the expression, and I don't think it
> >> should be accepted without them.
> >>
> >> --
> >> Andreas Schwab, schwab@linux-m68k.org
> >> GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510  2552 DF73 E780 A9DA AEC1
> >> "And now for something completely different."
> >>
> >
>
>

Re: [PATCH v2 5/7] RISC-V: relax post-relocation-operator separator expectation

[Jan Beulich]

On 28.04.2023 03:29, Nelson Chu wrote:
> Sorry for missing this one.  No, not a request.  I'm fine with both, since
> this patch won't break the risc-gnu-toolchain regression, so most of the
> stuff already used looks good.  It seems that the risc v-asm doesn't say we
> "must '' use a parenthesis after the modifiers, it just shows examples that
> all use it.

Hmm, interesting. My interpretation of especially the table in the doc (not
so much the examples) would be that it pretty much mandates parentheses. One
might even go as far as taking it to mandate no blank between reloc specifier
and paren (but I wouldn't feel well going that far without it being said
explicitly). So my plan was to make the alternative patch, and then leave it
to you (incl other maintainers) to decide (with my personal preference being
the yet to be created alternative one, now that I've seen the doc).

Jan

Re: [PATCH v2 5/7] RISC-V: relax post-relocation-operator separator expectation

[Fangrui Song]

On Thu, Apr 27, 2023 at 11:05 PM Jan Beulich via Binutils
<binutils@sourceware.org> wrote:
>
> On 28.04.2023 03:29, Nelson Chu wrote:
> > Sorry for missing this one.  No, not a request.  I'm fine with both, since
> > this patch won't break the risc-gnu-toolchain regression, so most of the
> > stuff already used looks good.  It seems that the risc v-asm doesn't say we
> > "must '' use a parenthesis after the modifiers, it just shows examples that
> > all use it.
>
> Hmm, interesting. My interpretation of especially the table in the doc (not
> so much the examples) would be that it pretty much mandates parentheses. One
> might even go as far as taking it to mandate no blank between reloc specifier
> and paren (but I wouldn't feel well going that far without it being said
> explicitly). So my plan was to make the alternative patch, and then leave it
> to you (incl other maintainers) to decide (with my personal preference being
> the yet to be created alternative one, now that I've seen the doc).
>
> Jan

I agree that rejecting `%hi sym` shall be a reasonable change in gas.
The LLVM integrated assembler (used by Clang and llvm-mc) rejects the
syntax.

% cat a.s
 lui     t0, %hi sym
% clang --target=riscv64 -c a.s
a.s:1:18: error: expected '('
 lui     t0, %hi sym
                 ^
a.s:1:18: error: unknown operand
 lui     t0, %hi sym
                 ^

Re: [PATCH v2 5/7] RISC-V: relax post-relocation-operator separator expectation

[Jan Beulich]

On 25.04.2023 10:37, Andreas Schwab wrote:
> On Mär 10 2023, Jan Beulich via Binutils wrote:
> 
>> With a blank being okay as a separator, constructs like
>>
>> 	lui	t0, %hi sym
>> 	lui	t0, %hi 0x1000
>>
>> are accepted. But then it makes little sense to not also accept e.g.
>>
>> 	lui	t0, %hi +sym
>> 	lui	t0, %hi -0x1000
> 
> https://github.com/riscv-non-isa/riscv-asm-manual/blob/master/riscv-asm.md
> always includes the parens around the expression, and I don't think it
> should be accepted without them.

Just to mention it: Someone (validly) said that much of this code (and
the underlying syntax) was derived from MIPS. Just now it happened that
I had to look at the MIPS testsuite for a different reason, when it
caught my eye that they're actually testing cases like the earlier two
above (and I'm pretty sure they would similarly fail on the latter two).

Jan

[PATCH v2 6/7] RISC-V: test for expected / no unexpected symbols

[Jan Beulich]

Both the temporary workaround for PR/gas 29940 and the existing special
casing of GPRs in my_getSmallExpression() aren't really tested anywhere
(i.e. with the workarounds remove testing would still succeed). Nor is
there any test for uses of symbols with names matching GPRs, where such
is permitted. Before altering how this is to be dealt with, install two
testcases covering the expected behavior. (For now this includes only
known affected insns; re-ordering of entries in riscv_opcodes[] could,
however, yield more of them.)
---
v2: Also include LUI and LW.

--- /dev/null
+++ b/gas/testsuite/gas/riscv/reg-syms.d
@@ -0,0 +1,8 @@
+#as: -march=rv32i
+#nm: --
+
+0+ t start
+ +U x2
+ +U x4
+ +U x6
+ +U x8
--- /dev/null
+++ b/gas/testsuite/gas/riscv/reg-syms.s
@@ -0,0 +1,10 @@
+	.text
+start:
+	and	x8, x8, x1
+	j	x2
+	jal	x3, x4
+	lui	x5, %hi(x6)
+	lw	x7, %lo(x8)(x9)
+	sll	x1, x1, x10
+	sra	x8, x8, x11
+	srl	x8, x8, x12
--- /dev/null
+++ b/gas/testsuite/gas/riscv/reg-syms-C.d
@@ -0,0 +1,4 @@
+#as: -march=rv32ic
+#source: reg-syms.s
+#nm: --
+#dump: reg-syms.d

[PATCH v2 7/7] RISC-V: adjust logic to avoid register name symbols

[Jan Beulich]

Special casing GPR names in my_getSmallExpression() leads to a number of
inconsistencies. Generalize this by utilizing the md_parse_name() hook,
limited to when instruction operands are being parsed (really: probed).
Then both the GPR lookup there and the yet more ad hoc workaround for
PR/gas 29940 can be removed (including its extension needed for making
the compressed form JAL work again).
---
When a floating point extension (but not [hfdq]inx) is enabled, floating
point registers would perhaps better be recognized by riscv_parse_name()
as well. Same for vector registers and likely also CSRs. Otherwise
inconsistent behavior may (continue to) result when equates come into
play, afaict.

Considering equates, the new behavior isn't the only sensible one: We
could also omit the symbol_find() call, thus never honoring equates.
Then, however, even with the "i" insn suffix (or sometimes infix)
explicitly specified, equates with names matching a GPR name (or, see
above, potentially also other register names) wouldn't be accepted
anymore. I guess the primary question here is what the intended (and
consistent) behavior is to be.

Overall the uses of my_getSmallExpression() vs my_getExpression() look
pretty random to me: Is there any underlying principle when which of the
two is expected to be used?
---
v2: Re-work to not break things like %hi(<register-name>).

--- a/gas/config/tc-riscv.c
+++ b/gas/config/tc-riscv.c
@@ -171,6 +171,8 @@ static enum float_abi float_abi = FLOAT_
 
 static unsigned elf_flags = 0;
 
+static bool probing_insn_operands;
+
 /* Set the default_isa_spec.  Return 0 if the spec isn't supported.
    Otherwise, return 1.  */
 
@@ -2228,21 +2230,10 @@ my_getSmallExpression (expressionS *ep,
 		       char *str, const struct percent_op_match *percent_op)
 {
   size_t reloc_index;
-  unsigned crux_depth, str_depth, regno;
+  unsigned crux_depth, str_depth;
+  bool orig_probing = probing_insn_operands;
   char *crux;
 
-  /* First, check for integer registers.  No callers can accept a reg, but
-     we need to avoid accidentally creating a useless undefined symbol below,
-     if this is an instruction pattern that can't match.  A glibc build fails
-     if this is removed.  */
-  if (reg_lookup (&str, RCLASS_GPR, &regno))
-    {
-      ep->X_op = O_register;
-      ep->X_add_number = regno;
-      expr_parse_end = str;
-      return 0;
-    }
-
   /* Search for the start of the main expression.
 
      End the loop with CRUX pointing to the start of the main expression and
@@ -2274,9 +2265,17 @@ my_getSmallExpression (expressionS *ep,
        return 0;
     }
 
+  /* Anything inside parentheses or subject to a relocation operator cannot
+     be a register and hence can be treated the same as operands to
+     directives (other than .insn).  */
+  if (str_depth || reloc_index)
+    probing_insn_operands = false;
+
   my_getExpression (ep, crux);
   str = expr_parse_end;
 
+  probing_insn_operands = orig_probing;
+
   /* Match every open bracket.  */
   while (crux_depth > 0 && (*str == ')' || *str == ' ' || *str == '\t'))
     if (*str++ == ')')
@@ -2462,6 +2461,13 @@ riscv_is_priv_insn (insn_t insn)
 	  || ((insn ^ MATCH_SFENCE_VM) & MASK_SFENCE_VM) == 0);
 }
 
+static symbolS *deferred_sym_rootP;
+static symbolS *deferred_sym_lastP;
+/* Since symbols can't easily be freed, try to recycle ones which weren't
+   committed.  */
+static symbolS *orphan_sym_rootP;
+static symbolS *orphan_sym_lastP;
+
 /* This routine assembles an instruction into its binary format.  As a
    side effect, it sets the global variable imm_reloc to the type of
    relocation to do if one of the operands is an address expression.  */
@@ -2497,6 +2503,8 @@ riscv_ip (char *str, struct riscv_cl_ins
 
   insn = (struct riscv_opcode *) str_hash_find (hash, str);
 
+  probing_insn_operands = true;
+
   asargStart = asarg;
   for ( ; insn && insn->name && strcmp (insn->name, str) == 0; insn++)
     {
@@ -2513,6 +2521,17 @@ riscv_ip (char *str, struct riscv_cl_ins
       /* Reset error message of the previous round.  */
       error.msg = _("illegal operands");
       error.missing_ext = NULL;
+
+      /* Purge deferred symbols from the previous round, if any.  */
+      while (deferred_sym_rootP)
+	{
+	  symbolS *sym = deferred_sym_rootP;
+
+	  symbol_remove (sym, &deferred_sym_rootP, &deferred_sym_lastP);
+	  symbol_append (sym, orphan_sym_lastP, &orphan_sym_rootP,
+			 &orphan_sym_lastP);
+	}
+
       create_insn (ip, insn);
 
       imm_expr->X_op = O_absent;
@@ -2567,9 +2586,22 @@ riscv_ip (char *str, struct riscv_cl_ins
 		}
 	      if (*asarg != '\0')
 		break;
+
 	      /* Successful assembly.  */
 	      error.msg = NULL;
 	      insn_with_csr = false;
+
+	      /* Commit deferred symbols, if any.  */
+	      while (deferred_sym_rootP)
+		{
+		  symbolS *sym = deferred_sym_rootP;
+
+		  symbol_remove (sym, &deferred_sym_rootP,
+				 &deferred_sym_lastP);
+		  symbol_append (sym, symbol_lastP, &symbol_rootP,
+				 &symbol_lastP);
+		  symbol_table_insert (sym);
+		}
 	      goto out;
 
 	    case 'C': /* RVC */
@@ -2773,8 +2805,6 @@ riscv_ip (char *str, struct riscv_cl_ins
 		case 'p':
 		  goto branch;
 		case 'a':
-		  if (oparg == insn->args + 1)
-		    goto jump_check_gpr;
 		  goto jump;
 		case 'S': /* Floating-point RS1 x8-x15.  */
 		  if (!reg_lookup (&asarg, RCLASS_FPR, &regno)
@@ -3278,18 +3308,6 @@ riscv_ip (char *str, struct riscv_cl_ins
 	      continue;
 
 	    case 'a': /* 20-bit PC-relative offset.  */
-	      /* Like in my_getSmallExpression() we need to avoid emitting
-		 a stray undefined symbol if the 1st JAL entry doesn't match,
-		 but the 2nd (with 2 operands) might.  */
-	      if (oparg == insn->args)
-		{
-	    jump_check_gpr:
-		  asargStart = asarg;
-		  if (reg_lookup (&asarg, RCLASS_GPR, NULL)
-		      && (*asarg == ',' || (ISSPACE (*asarg) && asarg[1] == ',')))
-		    break;
-		  asarg = asargStart;
-		}
 	    jump:
 	      my_getExpression (imm_expr, asarg);
 	      asarg = expr_parse_end;
@@ -3512,6 +3530,8 @@ riscv_ip (char *str, struct riscv_cl_ins
   if (save_c)
     *(asargStart  - 1) = save_c;
 
+  probing_insn_operands = false;
+
   return error;
 }
 
@@ -3808,6 +3828,53 @@ riscv_after_parse_args (void)
     flag_dwarf_cie_version = 3;
 }
 
+bool riscv_parse_name (const char *name, struct expressionS *ep,
+		       enum expr_mode mode)
+{
+  unsigned int regno;
+  symbolS *sym;
+
+  if (!probing_insn_operands)
+    return false;
+
+  gas_assert (mode == expr_normal);
+
+  regno = reg_lookup_internal (name, RCLASS_GPR);
+  if (regno == (unsigned int)-1)
+    return false;
+
+  if (symbol_find (name) != NULL)
+    return false;
+
+  /* Create a symbol without adding it to the symbol table yet.
+     Insertion will happen only once we commit to using the insn
+     we're probing operands for.  */
+  for (sym = deferred_sym_rootP; sym; sym = symbol_next (sym))
+    if (strcmp (name, S_GET_NAME (sym)) == 0)
+      break;
+  if (!sym)
+    {
+      for (sym = orphan_sym_rootP; sym; sym = symbol_next (sym))
+	if (strcmp (name, S_GET_NAME (sym)) == 0)
+	  {
+	    symbol_remove (sym, &orphan_sym_rootP, &orphan_sym_lastP);
+	    break;
+	  }
+      if (!sym)
+	sym = symbol_create (name, undefined_section,
+			     &zero_address_frag, 0);
+
+      symbol_append (sym, deferred_sym_lastP, &deferred_sym_rootP,
+		     &deferred_sym_lastP);
+    }
+
+  ep->X_op = O_symbol;
+  ep->X_add_symbol = sym;
+  ep->X_add_number = 0;
+
+  return true;
+}
+
 long
 md_pcrel_from (fixS *fixP)
 {
--- a/gas/config/tc-riscv.h
+++ b/gas/config/tc-riscv.h
@@ -123,6 +123,10 @@ extern void riscv_elf_final_processing (
 /* Adjust debug_line after relaxation.  */
 #define DWARF2_USE_FIXED_ADVANCE_PC 1
 
+#define md_parse_name(name, exp, mode, c) \
+  riscv_parse_name (name, exp, mode)
+bool riscv_parse_name (const char *, struct expressionS *, enum expr_mode);
+
 #define md_finish riscv_md_finish
 #define CONVERT_SYMBOLIC_ATTRIBUTE riscv_convert_symbolic_attribute
 

Re: [PATCH v2 0/7] RISC-V/gas: insn operand parsing

[Nelson Chu]

[-- Attachment #1: Type: text/plain, Size: 1651 bytes --]

Hi Jan,

On Fri, Mar 10, 2023 at 5:24 PM Jan Beulich <jbeulich@suse.com> wrote:

> (v1 series was "re-work register named symbols avoidance logic")
>
> This addresses some of the anomalies I've observed. There continue to
> be questions towards consistent overall behavior - see remarks in the
> individual patches.
>
> An assembler with the full series in place was used in a gcc 12.2.0
> testsuite run (cross build on x86, so no execution tests), resulting
> in no new test failures (there were a number of pre-existing ones,
> though).
>
> 1: minor effort reduction in relocation specifier parsing
> 2: drop "percent_op" parameter from my_getOpcodeExpression()
> 3: avoid redundant and misleading/wrong error messages
> 4: don't recognize bogus relocations
> 5: relax post-relocation-operator separator expectation
>

Except this one I'm not sure if we should accept or not, others look good
to me.  The gcc/binutils regressions of riscv-gnu-toolchain looks good with
this whole series, including the fifth patch, so I guess that is because
the current codegen and inline assembly developers are all expect () after
the %hi/%pcrel_hi/... modifiers.  I don't know if we should relax the usage
of modifiers here, but at least the regressions prove that there should be
no effect for now, even if we commit the patch.  The only thing is that we
may have different behaviors as llvm, but since llvm only accepts the ()
after the modifiers, we have already behaved a little differently.

Thanks
Nelson


> 6: test for expected / no unexpected symbols
> 7: adjust logic to avoid register name symbols
>
> Jan
>