[prev in list] [next in list] [prev in thread] [next in thread] 

List:       binutils
Subject:    Re: [PATCH] RISC-V: fix build after "Add support for arbitrary immediate encoding formats"
From:       Palmer Dabbelt <palmer () dabbelt ! com>
Date:       2022-09-30 22:17:35
Message-ID: mhng-1c72f11a-8424-45bd-b4c5-2588ef505222 () palmer-ri-x1c9
[Download RAW message or body]

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)
>>
[prev in list] [next in list] [prev in thread] [next in thread]