Re: [PATCH] RISC-V: fix build after "Add support for arbitrary immediate encoding formats"

Re: [PATCH] RISC-V: fix build after "Add support for arbitrary immediate encoding formats"

[Palmer Dabbelt]

On Fri, 30 Sep 2022 03:26:29 PDT (-0700), christoph.muellner@vrull.eu wrote:
> Hi Jan,
>
> Thanks for pointing that out!
>
> BR
> Christoph
>
> On Fri, Sep 30, 2022 at 11:41 AM Jan Beulich <jbeulich@suse.com> wrote:
>
>> Pre- and post-increment/decrement are side effects, the behavior of
>> which is undefined when combined with passing an address of the accessed
>> variable in the same function invocation. There's no need for the
>> increments here - simply adding 1 achieves the intended effect without
>> triggering compiler diagnostics (which are fatal with -Werror).
>> ---
>> Committing as obvious.

Thanks, sorry we missed this one.

That said: I'd argue that the sscanf() version of these routines were 
much less prone to these sorts of string parsing issues, I re-wrote 
these because I'd run into a handful of issues when trying to 
forward-port the original T-Head patches.  I don't remember exactly what 
the issues were, though (sorry if I missed something during the patch 
review, I've been pretty out of it over the last few weeks).

>>
>> --- a/gas/config/tc-riscv.c
>> +++ b/gas/config/tc-riscv.c
>> @@ -1287,10 +1287,10 @@ validate_riscv_insn (const struct riscv_
>>                 case 'u': /* 'XuN@S' ... N-bit unsigned immediate at bit
>> S.  */
>>                   goto use_imm;
>>                 use_imm:
>> -                 n = strtol (++oparg, (char **)&oparg, 10);
>> +                 n = strtol (oparg + 1, (char **)&oparg, 10);
>>                   if (*oparg != '@')
>>                     goto unknown_validate_operand;
>> -                 s = strtol (++oparg, (char **)&oparg, 10);
>> +                 s = strtol (oparg + 1, (char **)&oparg, 10);
>>                   oparg--;
>>
>>                   USE_IMM (n, s);
>> @@ -3327,10 +3327,10 @@ riscv_ip (char *str, struct riscv_cl_ins
>>                       sign = false;
>>                       goto parse_imm;
>>                     parse_imm:
>> -                     n = strtol (++oparg, (char **)&oparg, 10);
>> +                     n = strtol (oparg + 1, (char **)&oparg, 10);
>>                       if (*oparg != '@')
>>                         goto unknown_riscv_ip_operand;
>> -                     s = strtol (++oparg, (char **)&oparg, 10);
>> +                     s = strtol (oparg + 1, (char **)&oparg, 10);
>>                       oparg--;
>>
>>                       my_getExpression (imm_expr, asarg);
>> --- a/opcodes/riscv-dis.c
>> +++ b/opcodes/riscv-dis.c
>> @@ -586,10 +586,10 @@ print_insn_args (const char *oparg, insn
>>                   sign = false;
>>                   goto print_imm;
>>                 print_imm:
>> -                 n = strtol (++oparg, (char **)&oparg, 10);
>> +                 n = strtol (oparg + 1, (char **)&oparg, 10);
>>                   if (*oparg != '@')
>>                     goto undefined_modifier;
>> -                 s = strtol (++oparg, (char **)&oparg, 10);
>> +                 s = strtol (oparg + 1, (char **)&oparg, 10);
>>                   oparg--;
>>
>>                   if (!sign)
>>

[PATCH] RISC-V: fix build after "Add support for arbitrary immediate encoding formats"

[Jan Beulich]

Pre- and post-increment/decrement are side effects, the behavior of
which is undefined when combined with passing an address of the accessed
variable in the same function invocation. There's no need for the
increments here - simply adding 1 achieves the intended effect without
triggering compiler diagnostics (which are fatal with -Werror).
---
Committing as obvious.

--- a/gas/config/tc-riscv.c
+++ b/gas/config/tc-riscv.c
@@ -1287,10 +1287,10 @@ validate_riscv_insn (const struct riscv_
 		case 'u': /* 'XuN@S' ... N-bit unsigned immediate at bit S.  */
 		  goto use_imm;
 		use_imm:
-		  n = strtol (++oparg, (char **)&oparg, 10);
+		  n = strtol (oparg + 1, (char **)&oparg, 10);
 		  if (*oparg != '@')
 		    goto unknown_validate_operand;
-		  s = strtol (++oparg, (char **)&oparg, 10);
+		  s = strtol (oparg + 1, (char **)&oparg, 10);
 		  oparg--;
 
 		  USE_IMM (n, s);
@@ -3327,10 +3327,10 @@ riscv_ip (char *str, struct riscv_cl_ins
 		      sign = false;
 		      goto parse_imm;
 		    parse_imm:
-		      n = strtol (++oparg, (char **)&oparg, 10);
+		      n = strtol (oparg + 1, (char **)&oparg, 10);
 		      if (*oparg != '@')
 			goto unknown_riscv_ip_operand;
-		      s = strtol (++oparg, (char **)&oparg, 10);
+		      s = strtol (oparg + 1, (char **)&oparg, 10);
 		      oparg--;
 
 		      my_getExpression (imm_expr, asarg);
--- a/opcodes/riscv-dis.c
+++ b/opcodes/riscv-dis.c
@@ -586,10 +586,10 @@ print_insn_args (const char *oparg, insn
 		  sign = false;
 		  goto print_imm;
 		print_imm:
-		  n = strtol (++oparg, (char **)&oparg, 10);
+		  n = strtol (oparg + 1, (char **)&oparg, 10);
 		  if (*oparg != '@')
 		    goto undefined_modifier;
-		  s = strtol (++oparg, (char **)&oparg, 10);
+		  s = strtol (oparg + 1, (char **)&oparg, 10);
 		  oparg--;
 
 		  if (!sign)