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

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

[Vineet Gupta]

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;
 
   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;
       /* Is this a pseudo-instruction and may we print it as such?  */
       if (op->pinfo & INSN_ALIAS)
-- 
2.30.2

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

[Vineet Gupta]

ping !


On 10/29/21 12:28 PM, Vineet Gupta wrote:
> 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;
>   
>     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;
>         /* Is this a pseudo-instruction and may we print it as such?  */
>         if (op->pinfo & INSN_ALIAS)
> 

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

[Vineet Gupta]

ping

On 10/29/21 12:28 PM, Vineet Gupta wrote:
> 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;
>   
>     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;
>         /* Is this a pseudo-instruction and may we print it as such?  */
>         if (op->pinfo & INSN_ALIAS)
> 

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

[Andrew Burgess]

* 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?

>  
>    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.

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'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.

Thanks,
Andrew

>        /* Is this a pseudo-instruction and may we print it as such?  */
>        if (op->pinfo & INSN_ALIAS)
> -- 
> 2.30.2
> 

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

[Vineet Gupta]

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

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

[Vineet Gupta]

On 10/28/21 7:59 PM, Nelson Chu wrote:
> Also send this patch to gdb mailing since there should be the right
> place for the issues of gdb and sim.

Ok will do.

> I'm going to move the rvv stuff from the integration branch back to
> the mainline, so we should need this in the mainline later.  But
> neither am I the developer or the maintainer of gdb, so we need the
> gdb experts' help.

Technically this change is simply matching the function prototype that 
got changed as part of that rvv update. Since this is not mainline I 
think gdb guys might hand wave as not their problem. Anyhow I'll send it 
over and see what comes back. I presume riscv binutils/gdb still work 
out of unified tree and branches ? So it should build off of integration 
branch too.

-Vineet

>
> Thanks
> Nelson
>
> On Fri, Oct 29, 2021 at 4:54 AM Vineet Gupta <vineetg@rivosinc.com> wrote:
>> The vector changes on binutils-integration-branch missed updates
>> to sim causing build failure when build sim/gdb.
>>
>> This patch is only for user/riscv/binutils-integration-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..420b1867913c 100644
>> --- a/sim/riscv/ChangeLog-2021
>> +++ b/sim/riscv/ChangeLog-2021
>> @@ -1,3 +1,7 @@
>> +2021-20-28  Vineet Gupta  <vineetg@revosinc.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;
>>
>>     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;
>>         /* Is this a pseudo-instruction and may we print it as such?  */
>>         if (op->pinfo & INSN_ALIAS)
>> --
>> 2.30.2
>>

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

[Nelson Chu]

Also send this patch to gdb mailing since there should be the right
place for the issues of gdb and sim.

I'm going to move the rvv stuff from the integration branch back to
the mainline, so we should need this in the mainline later.  But
neither am I the developer or the maintainer of gdb, so we need the
gdb experts' help.

Thanks
Nelson

On Fri, Oct 29, 2021 at 4:54 AM Vineet Gupta <vineetg@rivosinc.com> wrote:
>
> The vector changes on binutils-integration-branch missed updates
> to sim causing build failure when build sim/gdb.
>
> This patch is only for user/riscv/binutils-integration-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..420b1867913c 100644
> --- a/sim/riscv/ChangeLog-2021
> +++ b/sim/riscv/ChangeLog-2021
> @@ -1,3 +1,7 @@
> +2021-20-28  Vineet Gupta  <vineetg@revosinc.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;
>
>    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;
>        /* Is this a pseudo-instruction and may we print it as such?  */
>        if (op->pinfo & INSN_ALIAS)
> --
> 2.30.2
>