Re: [PATCH] sim: riscv: fix build breakage with rvv changes

[Vineet Gupta <vineetg@rivosinc.com>]

Hi Andrew,

On 11/10/21 1:40 AM, Andrew Burgess wrote:
> * Vineet Gupta <vineetg@rivosinc.com> [2021-10-29 12:28:56 -0700]:
>
>> changes to gas for riscv vector extensions need to be propagated to sim
>> otherwise gdb fails to build on users/riscv/binutils-integration-branch
>>
>> This patch currently applies to that branch.
>>
>> Fixes: 144cceb058e "(RISC-V/rvv: Add rvv v0.10 instructions.)"
>> Reported-by: Dylan Reid <dylan@rivosinc.com>
>> Signed-off-by: Vineet Gupta <vineetg@rivosinc.com>
>> ---
>>   sim/riscv/ChangeLog-2021 | 4 ++++
>>   sim/riscv/sim-main.c     | 3 ++-
>>   2 files changed, 6 insertions(+), 1 deletion(-)
>>
>> diff --git a/sim/riscv/ChangeLog-2021 b/sim/riscv/ChangeLog-2021
>> index e9aa74490f12..9ced6773bdd6 100644
>> --- a/sim/riscv/ChangeLog-2021
>> +++ b/sim/riscv/ChangeLog-2021
>> @@ -1,3 +1,7 @@
>> +2021-20-28  Vineet Gupta  <vineetg@rivosinc.com>
>> +
>> +	* sim-main.c (step_once): Fix match_func call per gas changes.
>> +
>>   2021-07-01  Mike Frysinger  <vapier@gentoo.org>
>>   
>>   	* configure: Regenerate.
>> diff --git a/sim/riscv/sim-main.c b/sim/riscv/sim-main.c
>> index 0faf9395ae52..9b4f7c6c5aad 100644
>> --- a/sim/riscv/sim-main.c
>> +++ b/sim/riscv/sim-main.c
>> @@ -956,6 +956,7 @@ void step_once (SIM_CPU *cpu)
>>     sim_cia pc = cpu->pc;
>>     const struct riscv_opcode *op;
>>     int xlen = RISCV_XLEN (cpu);
>> +  const char *error = NULL;
> Could this not be moved to the more inner scope?

Yep.

>
>>   
>>     if (TRACE_ANY_P (cpu))
>>       trace_prefix (sd, cpu, NULL_CIA, pc, TRACE_LINENUM_P (cpu),
>> @@ -985,7 +986,7 @@ void step_once (SIM_CPU *cpu)
>>     for (; op->name; op++)
>>       {
>>         /* Does the opcode match?  */
>> -      if (! op->match_func (op, iw))
>> +      if (! op->match_func (op, iw, 0, /* check_constraints */ &error))
>>   	continue;
> I've not looked at exactly what the purpose of error is here, does it
> just provide a reason why this function returns false?  i.e. is it
> always OK for us to ignore it like this?  Maybe a comment explaining
> briefly why we ignore something called error would be helpful.

op->match_op is shared between gas and sim. It was updated as part of 
some gas change (in the "Fixes" tag) but missed the corresponding change 
in sim - all I'm doing here is propagate that - w/o needing any extra 
semantics needed for gas change.

> Maybe this email wasn't really intended for me, but you only need
> approval from the branch owner before merging to a user branch, and
> that certainly isn't me in this case, so I can't approve this patch
> for the branch.

I thought so too :-) and mentioned this to Nelson at the time of 
submission on binutils mailing list.

> I've added Nelson and Jim to the CC list as, along
> with Kito, they wrote the original patch.
>
> And, as this patch doesn't currently apply to master, I can't approve
> this patch for master either.
>
> I assume this fix will be merged into the original patch (commit
> 144cceb058e59977f in the user branch) before the work is officially
> posted for inclusion in upstream master.

Yeah indeed that was the idea.

Thx for taking a look.
-Vineet