[PATCH] sim: riscv: Fix PC at gdb breakpoints

[PATCH] sim: riscv: Fix PC at gdb breakpoints

[Bernd Edlinger]

The uncompressed EBREAK instruction does not work
correctly this way, and the comment saying that
GDB expects us to step over EBREAK is just wrong.
The PC was always 4 bytes too high, which skips one
instruction at break and step over commands, and
causes complete chaos.  The compressed EBREAK was
already implemented correctly.

Tested by using gdb's "target sim" and single-stepping.
---
 sim/riscv/sim-main.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/sim/riscv/sim-main.c b/sim/riscv/sim-main.c
index 0876d455570..66b99e40314 100644
--- a/sim/riscv/sim-main.c
+++ b/sim/riscv/sim-main.c
@@ -624,9 +624,7 @@ execute_i (SIM_CPU *cpu, unsigned_word iw, const struct riscv_opcode *op)
       break;
     case MATCH_EBREAK:
       TRACE_INSN (cpu, "ebreak;");
-      /* GDB expects us to step over EBREAK.  */
-      sim_engine_halt (sd, cpu, NULL, riscv_cpu->pc + 4, sim_stopped,
-		       SIM_SIGTRAP);
+      sim_engine_halt (sd, cpu, NULL, riscv_cpu->pc, sim_stopped, SIM_SIGTRAP);
       break;
     case MATCH_ECALL:
       TRACE_INSN (cpu, "ecall;");
-- 
2.25.1

Re: [PATCH] sim: riscv: Fix PC at gdb breakpoints

[Andrew Burgess]

Bernd Edlinger <bernd.edlinger@hotmail.de> writes:

> The uncompressed EBREAK instruction does not work
> correctly this way, and the comment saying that
> GDB expects us to step over EBREAK is just wrong.
> The PC was always 4 bytes too high, which skips one
> instruction at break and step over commands, and
> causes complete chaos.  The compressed EBREAK was
> already implemented correctly.
>
> Tested by using gdb's "target sim" and single-stepping.

Thanks for fixing this.

For the record, in v1.12 of the RISC-V privileged architecture
specification, section 3.3.1: Environment Call and Breakpoint documents
that the $pc value should be the address of the EBREAK instruction, not
the address of the following instruction.

Approved-By: Andrew Burgess <aburgess@redhat.com>

Thanks,
Andrew

> ---
>  sim/riscv/sim-main.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
>
> diff --git a/sim/riscv/sim-main.c b/sim/riscv/sim-main.c
> index 0876d455570..66b99e40314 100644
> --- a/sim/riscv/sim-main.c
> +++ b/sim/riscv/sim-main.c
> @@ -624,9 +624,7 @@ execute_i (SIM_CPU *cpu, unsigned_word iw, const struct riscv_opcode *op)
>        break;
>      case MATCH_EBREAK:
>        TRACE_INSN (cpu, "ebreak;");
> -      /* GDB expects us to step over EBREAK.  */
> -      sim_engine_halt (sd, cpu, NULL, riscv_cpu->pc + 4, sim_stopped,
> -		       SIM_SIGTRAP);
> +      sim_engine_halt (sd, cpu, NULL, riscv_cpu->pc, sim_stopped, SIM_SIGTRAP);
>        break;
>      case MATCH_ECALL:
>        TRACE_INSN (cpu, "ecall;");
> -- 
> 2.25.1

Re: [PATCH] sim: riscv: Fix PC at gdb breakpoints

[Bernd Edlinger]

On 4/12/24 11:44, Andrew Burgess wrote:
> Bernd Edlinger <bernd.edlinger@hotmail.de> writes:
> 
>> The uncompressed EBREAK instruction does not work
>> correctly this way, and the comment saying that
>> GDB expects us to step over EBREAK is just wrong.
>> The PC was always 4 bytes too high, which skips one
>> instruction at break and step over commands, and
>> causes complete chaos.  The compressed EBREAK was
>> already implemented correctly.
>>
>> Tested by using gdb's "target sim" and single-stepping.
> 
> Thanks for fixing this.
> 
> For the record, in v1.12 of the RISC-V privileged architecture
> specification, section 3.3.1: Environment Call and Breakpoint documents
> that the $pc value should be the address of the EBREAK instruction, not
> the address of the following instruction.
> 
> Approved-By: Andrew Burgess <aburgess@redhat.com>
> 
> Thanks,
> Andrew
> 

Pushed.

Thanks
Bernd.