diff mbox series

sim: riscv: Fix PC at gdb breakpoints

Message ID AS8P193MB12851EAC96A002B34CB57EBBE4042@AS8P193MB1285.EURP193.PROD.OUTLOOK.COM
State New
Headers
Series sim: riscv: Fix PC at gdb breakpoints |

Checks

Context Check Description
linaro-tcwg-bot/tcwg_gdb_build--master-aarch64 success Testing passed
linaro-tcwg-bot/tcwg_gdb_build--master-arm success Testing passed
linaro-tcwg-bot/tcwg_gdb_check--master-arm fail Testing failed
linaro-tcwg-bot/tcwg_gdb_check--master-aarch64 success Testing passed

Commit Message

Bernd Edlinger April 12, 2024, 7:31 a.m. UTC 
  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(-)

Comments

Andrew Burgess April 12, 2024, 9:44 a.m. UTC | #1 
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
Bernd Edlinger April 15, 2024, 8:21 a.m. UTC | #2 
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.
diff mbox series

Patch

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;");