diff mbox

RISC-V: Don't decrement pc after break.

Message ID 20180717001241.25908-1-jimw@sifive.com
State New, archived
Headers

Commit Message

Jim Wilson July 17, 2018, 12:12 a.m. UTC 
  This is the gdb patch for the RISC-V Linux kernel patch I just submitted.
    https://patchwork.kernel.org/patch/10524925/
This removes the code that decrements the pc after a break, now that we have
a patch to stop linux from pointlessly adding to the pc after a break.  It
would be nice if this goes in now, to avoid unnecessary divergence between
gdb and the linux kernel.  Palmer, the RISC-V linux kernel maintainer, has
already agreed to accept that patch.

This will also be needed by the FreeBSD gdb port which may be started soon.

This also fixes a bug in the code.  The fact that we have two different
mechanisms to decide breakpoint size, used_compressed_breakpoints and
has_compressed_isa, means that it is possible for gdb to emit a 4 byte
breakpoint and then subtract 2 from the pc, and vice versa.  Removing the
unnecessary pc decrement fixes that problem.

OK?

Jim

	gdb/
	* riscv-tdep.c (riscv_has_feature): Delete comment that refers to
	set_gdbarch_decr_pc_after_break.  Call riscv_read_misa_reg always.
	(riscv_gdbarch_init): Delete local has_compressed_isa.  Delete now
	unecessary braces after EF_RISCV_RVC test.  Delete call to
	set_gdbarch_decr_pc_after_break.
---
 gdb/riscv-tdep.c | 25 ++-----------------------
 1 file changed, 2 insertions(+), 23 deletions(-)

Comments

Andrew Burgess July 17, 2018, 3:37 p.m. UTC | #1 
* Jim Wilson <jimw@sifive.com> [2018-07-16 17:12:41 -0700]:

> This is the gdb patch for the RISC-V Linux kernel patch I just submitted.
>     https://patchwork.kernel.org/patch/10524925/
> This removes the code that decrements the pc after a break, now that we have
> a patch to stop linux from pointlessly adding to the pc after a break.  It
> would be nice if this goes in now, to avoid unnecessary divergence between
> gdb and the linux kernel.  Palmer, the RISC-V linux kernel maintainer, has
> already agreed to accept that patch.
> 
> This will also be needed by the FreeBSD gdb port which may be started soon.
> 
> This also fixes a bug in the code.  The fact that we have two different
> mechanisms to decide breakpoint size, used_compressed_breakpoints and
> has_compressed_isa, means that it is possible for gdb to emit a 4 byte
> breakpoint and then subtract 2 from the pc, and vice versa.  Removing the
> unnecessary pc decrement fixes that problem.
> 
> OK?
> 
> Jim
> 
> 	gdb/
> 	* riscv-tdep.c (riscv_has_feature): Delete comment that refers to
> 	set_gdbarch_decr_pc_after_break.  Call riscv_read_misa_reg always.
> 	(riscv_gdbarch_init): Delete local has_compressed_isa.  Delete now
> 	unecessary braces after EF_RISCV_RVC test.  Delete call to
> 	set_gdbarch_decr_pc_after_break.

This looks good to me.

Thanks,
Andrew


> ---
>  gdb/riscv-tdep.c | 25 ++-----------------------
>  1 file changed, 2 insertions(+), 23 deletions(-)
> 
> diff --git a/gdb/riscv-tdep.c b/gdb/riscv-tdep.c
> index 72dab0f897..f5d1af822c 100644
> --- a/gdb/riscv-tdep.c
> +++ b/gdb/riscv-tdep.c
> @@ -335,23 +335,7 @@ riscv_has_feature (struct gdbarch *gdbarch, char feature)
>  
>    gdb_assert (feature >= 'A' && feature <= 'Z');
>  
> -  /* It would be nice to always check with the real target where possible,
> -     however, for compressed instructions this is a bad idea.
> -
> -     The call to `set_gdbarch_decr_pc_after_break' is made just once per
> -     GDBARCH and we decide at that point if we should decrement by 2 or 4
> -     bytes based on whether the BFD has compressed instruction support or
> -     not.
> -
> -     If the BFD was not compiled with compressed instruction support, but we
> -     are running on a target with compressed instructions then we might
> -     place a 4-byte breakpoint, then decrement the $pc by 2 bytes leading to
> -     confusion.
> -
> -     It's safer if we just make decisions about compressed instruction
> -     support based on the BFD.  */
> -  if (feature != 'C')
> -    misa = riscv_read_misa_reg (&have_read_misa);
> +  misa = riscv_read_misa_reg (&have_read_misa);
>    if (!have_read_misa || misa == 0)
>      misa = gdbarch_tdep (gdbarch)->core_features;
>  
> @@ -2440,7 +2424,6 @@ riscv_gdbarch_init (struct gdbarch_info info,
>    struct gdbarch *gdbarch;
>    struct gdbarch_tdep *tdep;
>    struct gdbarch_tdep tmp_tdep;
> -  bool has_compressed_isa = false;
>    int i;
>  
>    /* Ideally, we'd like to get as much information from the target for
> @@ -2472,10 +2455,7 @@ riscv_gdbarch_init (struct gdbarch_info info,
>  			_("unknown ELF header class %d"), eclass);
>  
>        if (e_flags & EF_RISCV_RVC)
> -	{
> -	  has_compressed_isa = true;
> -	  tmp_tdep.core_features |= (1 << ('C' - 'A'));
> -	}
> +	tmp_tdep.core_features |= (1 << ('C' - 'A'));
>  
>        if (e_flags & EF_RISCV_FLOAT_ABI_DOUBLE)
>  	{
> @@ -2545,7 +2525,6 @@ riscv_gdbarch_init (struct gdbarch_info info,
>    set_gdbarch_register_reggroup_p (gdbarch, riscv_register_reggroup_p);
>  
>    /* Functions to analyze frames.  */
> -  set_gdbarch_decr_pc_after_break (gdbarch, (has_compressed_isa ? 2 : 4));
>    set_gdbarch_skip_prologue (gdbarch, riscv_skip_prologue);
>    set_gdbarch_inner_than (gdbarch, core_addr_lessthan);
>    set_gdbarch_frame_align (gdbarch, riscv_frame_align);
> -- 
> 2.17.1
>
Sebastian Huber July 23, 2018, 10:30 a.m. UTC | #2 
Hello Jim,

this patch broke debugging with Qemu (qemu-system-riscv64, Git commit 
6598f0cdad6acc6674c4f060fa46e537228c2c47).  The GDB error message is:

Register 3921 is not available

A backtrace of the GDB run with a break point set the throw_error() yields:

Breakpoint 1, throw_error (error=error@entry=NOT_AVAILABLE_ERROR, 
fmt=fmt@entry=0x87b0c0 "Register %d is not available") at 
/home/EB/sebastian_h/archive/binutils-git/gdb/common/common-exceptions.c:390
390     {
Missing separate debuginfos, use: zypper install 
libexpat1-debuginfo-2.1.0-24.1.x86_64 
libgmp10-debuginfo-5.1.3-7.15.x86_64 
libmpfr4-debuginfo-3.1.2-13.15.x86_64 
libncurses5-debuginfo-5.9-62.1.x86_64 
libpython2_7-1_0-debuginfo-2.7.13-27.3.1.x86_64 
python-base-debuginfo-2.7.13-27.3.1.x86_64
(gdb) bt
#0  throw_error (error=error@entry=NOT_AVAILABLE_ERROR, 
fmt=fmt@entry=0x87b0c0 "Register %d is not available") at 
/home/EB/sebastian_h/archive/binutils-git/gdb/common/common-exceptions.c:390
#1  0x0000000000578552 in frame_unwind_register_unsigned 
(next_frame=<optimized out>, regnum=regnum@entry=834) at 
/home/EB/sebastian_h/archive/binutils-git/gdb/frame.c:1308
#2  0x000000000057857c in get_frame_register_unsigned 
(frame=frame@entry=0xb933c0, regnum=regnum@entry=834) at 
/home/EB/sebastian_h/archive/binutils-git/gdb/frame.c:1322
#3  0x0000000000413d7d in riscv_read_misa_reg (read_p=<synthetic 
pointer>) at /home/EB/sebastian_h/archive/binutils-git/gdb/riscv-tdep.c:306
#4  riscv_has_feature (gdbarch=0xc50cc0, feature=<optimized out>) at 
/home/EB/sebastian_h/archive/binutils-git/gdb/riscv-tdep.c:338
#5  0x00000000004149de in riscv_breakpoint_kind_from_pc 
(gdbarch=<optimized out>, pcptr=<optimized out>) at 
/home/EB/sebastian_h/archive/binutils-git/gdb/riscv-tdep.c:416
#6  0x00000000004b3ef1 in default_breakpoint_from_pc (gdbarch=0xc50cc0, 
pcptr=<optimized out>, lenptr=0x7fffffffd014) at 
/home/EB/sebastian_h/archive/binutils-git/gdb/arch-utils.c:833
#7  0x00000000004ccbb1 in program_breakpoint_here_p (gdbarch=<optimized 
out>, address=2147498700) at 
/home/EB/sebastian_h/archive/binutils-git/gdb/breakpoint.c:8656
#8  0x00000000004ccdb6 in bp_loc_is_permanent (loc=0xd982a0) at 
/home/EB/sebastian_h/archive/binutils-git/gdb/breakpoint.c:8694
#9  add_location_to_breakpoint (b=b@entry=0xd7b5f0, 
sal=sal@entry=0x7fffffffd0f0) at 
/home/EB/sebastian_h/archive/binutils-git/gdb/breakpoint.c:8638
#10 0x00000000004cceab in init_raw_breakpoint (b=0xd7b5f0, 
gdbarch=<optimized out>, sal=..., bptype=bp_breakpoint, ops=<optimized 
out>) at /home/EB/sebastian_h/archive/binutils-git/gdb/breakpoint.c:7200
#11 0x00000000004d0fa8 in init_breakpoint_sal (b=0xd7b5f0, 
gdbarch=0xc50cc0, sals=..., location=..., filter=..., cond_string=..., 
extra_string=..., type=bp_breakpoint, disposition=disp_donttouch, 
thread=-1, task=0, ignore_count=0, ops=0xa555e0 <bkpt_breakpoint_ops>,
     from_tty=0, enabled=1, flags=0, display_canonical=0, 
internal=<optimized out>) at 
/home/EB/sebastian_h/archive/binutils-git/gdb/breakpoint.c:8828
#12 0x00000000004d7b12 in create_breakpoint_sal 
(display_canonical=<optimized out>, flags=<optimized out>, 
internal=<optimized out>, enabled=<optimized out>, from_tty=<optimized 
out>, ops=<optimized out>, ignore_count=<optimized out>, task=<optimized 
out>,
     thread=<optimized out>, disposition=<optimized out>, 
type=<optimized out>, extra_string=..., cond_string=..., filter=..., 
location=..., sals=..., gdbarch=<optimized out>) at 
/home/EB/sebastian_h/archive/binutils-git/gdb/breakpoint.c:8932
#13 create_breakpoints_sal (flags=<optimized out>, internal=<optimized 
out>, enabled=<optimized out>, from_tty=<optimized out>, ops=<optimized 
out>, ignore_count=<optimized out>, task=<optimized out>, 
thread=<optimized out>, disposition=<optimized out>,
     type=<optimized out>, extra_string=..., cond_string=..., 
canonical=0x7fffffffd3c0, gdbarch=<optimized out>) at 
/home/EB/sebastian_h/archive/binutils-git/gdb/breakpoint.c:8984
#14 create_breakpoints_sal_default (flags=<optimized out>, 
internal=<optimized out>, enabled=<optimized out>, from_tty=<optimized 
out>, ops=<optimized out>, ignore_count=<optimized out>, task=<optimized 
out>, thread=<optimized out>, disposition=<optimized out>,
     type_wanted=<optimized out>, extra_string=..., cond_string=..., 
canonical=0x7fffffffd3c0, gdbarch=<optimized out>) at 
/home/EB/sebastian_h/archive/binutils-git/gdb/breakpoint.c:13806
#15 bkpt_create_breakpoints_sal (gdbarch=<optimized out>, 
canonical=0x7fffffffd3c0, cond_string=..., extra_string=..., 
type_wanted=<optimized out>, disposition=<optimized out>, 
thread=<optimized out>, task=<optimized out>, ignore_count=<optimized out>,
     ops=<optimized out>, from_tty=<optimized out>, enabled=<optimized 
out>, internal=<optimized out>, flags=<optimized out>) at 
/home/EB/sebastian_h/archive/binutils-git/gdb/breakpoint.c:12608
#16 0x00000000004d8454 in create_breakpoint (gdbarch=0xc50cc0, 
location=location@entry=0xc76770, cond_string=cond_string@entry=0x0, 
thread=<optimized out>, thread@entry=0, extra_string=<optimized out>, 
extra_string@entry=0xc70bcb "", parse_extra=parse_extra@entry=1,
     tempflag=<optimized out>, type_wanted=<optimized out>, 
ignore_count=<optimized out>, pending_break_support=<optimized out>, 
ops=<optimized out>, from_tty=<optimized out>, enabled=<optimized out>, 
internal=<optimized out>, flags=<optimized out>)
     at /home/EB/sebastian_h/archive/binutils-git/gdb/breakpoint.c:9357
#17 0x00000000004d8901 in break_command_1 (arg=<optimized out>, 
flag=<optimized out>, from_tty=0) at 
/home/EB/sebastian_h/archive/binutils-git/gdb/breakpoint.c:9436
#18 0x00000000006bc292 in cmd_func (cmd=<optimized out>, args=<optimized 
out>, from_tty=<optimized out>) at 
/home/EB/sebastian_h/archive/binutils-git/gdb/cli/cli-decode.c:1857
#19 0x000000000067ae6e in execute_command (p=<optimized out>, 
p@entry=0xc70bc0 "b bsp_reset", from_tty=0) at 
/home/EB/sebastian_h/archive/binutils-git/gdb/top.c:630
#20 0x000000000056b26c in command_handler (command=0xc70bc0 "b 
bsp_reset") at /home/EB/sebastian_h/archive/binutils-git/gdb/event-top.c:583
#21 0x000000000067b981 in read_command_file 
(stream=stream@entry=0xc70960) at 
/home/EB/sebastian_h/archive/binutils-git/gdb/top.c:424
#22 0x00000000006bffae in script_from_file 
(stream=stream@entry=0xc70960, file=file@entry=0x7fffffffde4a 
"~/bin/riscv.gdb") at 
/home/EB/sebastian_h/archive/binutils-git/gdb/cli/cli-script.c:1551
#23 0x00000000006b9feb in source_script_from_stream 
(file_to_open=0x7fffffffde4a "~/bin/riscv.gdb", file=0x7fffffffde4a 
"~/bin/riscv.gdb", stream=0xc70960) at 
/home/EB/sebastian_h/archive/binutils-git/gdb/cli/cli-cmds.c:567
#24 source_script_with_search (file=0x7fffffffde4a "~/bin/riscv.gdb", 
from_tty=<optimized out>, search_path=<optimized out>) at 
/home/EB/sebastian_h/archive/binutils-git/gdb/cli/cli-cmds.c:603
#25 0x00000000005c6a18 in catch_command_errors (command=0x6ba0d0 
<source_script(char const*, int)>, arg=0x7fffffffde4a "~/bin/riscv.gdb", 
from_tty=1) at /home/EB/sebastian_h/archive/binutils-git/gdb/main.c:379
#26 0x00000000005c76c1 in captured_main_1 (context=0x7fffffffd6e0) at 
/home/EB/sebastian_h/archive/binutils-git/gdb/main.c:1121
#27 captured_main (data=0x7fffffffd6e0) at 
/home/EB/sebastian_h/archive/binutils-git/gdb/main.c:1147
#28 gdb_main (args=args@entry=0x7fffffffd810) at 
/home/EB/sebastian_h/archive/binutils-git/gdb/main.c:1173
#29 0x000000000040dba5 in main (argc=<optimized out>, argv=<optimized 
out>) at /home/EB/sebastian_h/archive/binutils-git/gdb/gdb.c:32
Jim Wilson July 23, 2018, 3:38 p.m. UTC | #3 
On Mon, Jul 23, 2018 at 3:30 AM, Sebastian Huber
<sebastian.huber@embedded-brains.de> wrote:
> Hello Jim,
>
> this patch broke debugging with Qemu (qemu-system-riscv64, Git commit
> 6598f0cdad6acc6674c4f060fa46e537228c2c47).  The GDB error message is:
>
> Register 3921 is not available

I don't understand how rtems debugging could have worked without this
patch.  3921 minus 65 in hex is 0xf10 which is the legacy misa.  The
legacy misa is checked only if a read of the v1.9.1 misa fails.  If
the read of both the v1.9.1 misa and the legacy misa fails, then you
get a confusing error stating that register 3921 is not available.
But the real problem is that both misa reads failed.  The ISA spec
says that misa must exist and be readable, although an implementation
is allowed to return 0 when it is read.  Gdb uses misa to determine
target features.  Gdb does handle the 0 read case by deducing info
from ELF header flags instead of the misa register.

If you have a rtems target support, then it must handle reading the
misa register.  If is OK to just return 0.  That is what my linux port
does for now.  At some point I may try adding a linux kernel patch to
add ptrace support for reading misa.  If rtems is running in machine
mode, you can probably read misa directly.  Otherwise, you would need
something like a linux ptrace to read it.  For embedded targets using
openocd, they can just read misa directly.

The problem with misa is easy to miss, as gdb only tries to read misa
if you execute a command that requires info about the target, such as
trying to use hardware floating point.  Actually, one of my other
patches, the one to remove the pc decrement after a break, modified
the code so that we try to read misa when checking to see if
compressed breakpoints could be used.  Before it was only checking ELF
headers for this, which wasn't right.  This is probably the patch that
exposed the bug in your rtems target support.

Jim
Sebastian Huber July 23, 2018, 5:24 p.m. UTC | #4 
----- Am 23. Jul 2018 um 17:38 schrieb Jim Wilson jimw@sifive.com:

> On Mon, Jul 23, 2018 at 3:30 AM, Sebastian Huber
> <sebastian.huber@embedded-brains.de> wrote:
>> Hello Jim,
>>
>> this patch broke debugging with Qemu (qemu-system-riscv64, Git commit
>> 6598f0cdad6acc6674c4f060fa46e537228c2c47).  The GDB error message is:
>>
>> Register 3921 is not available
> 
> I don't understand how rtems debugging could have worked without this
> patch.  3921 minus 65 in hex is 0xf10 which is the legacy misa.  The
> legacy misa is checked only if a read of the v1.9.1 misa fails.  If
> the read of both the v1.9.1 misa and the legacy misa fails, then you
> get a confusing error stating that register 3921 is not available.
> But the real problem is that both misa reads failed.  The ISA spec
> says that misa must exist and be readable, although an implementation
> is allowed to return 0 when it is read.  Gdb uses misa to determine
> target features.  Gdb does handle the 0 read case by deducing info
> from ELF header flags instead of the misa register.
> 
> If you have a rtems target support, then it must handle reading the
> misa register.  If is OK to just return 0.  That is what my linux port
> does for now.  At some point I may try adding a linux kernel patch to
> add ptrace support for reading misa.  If rtems is running in machine
> mode, you can probably read misa directly.  Otherwise, you would need
> something like a linux ptrace to read it.  For embedded targets using
> openocd, they can just read misa directly.

RTEMS is more or less just a bare metal ELF target with a special Newlib configuration. I think the question is, why it worked at all on bare metal ELF RISC-V targets before.

> 
> The problem with misa is easy to miss, as gdb only tries to read misa
> if you execute a command that requires info about the target, such as
> trying to use hardware floating point.  Actually, one of my other
> patches, the one to remove the pc decrement after a break, modified
> the code so that we try to read misa when checking to see if
> compressed breakpoints could be used.  Before it was only checking ELF
> headers for this, which wasn't right.  This is probably the patch that
> exposed the bug in your rtems target support.

This could be also a Qemu bug. I have to check this with the debugger tomorrow. The misa should be returned by Qemu in csr_read_helper() (target/riscv/op_helper.c). A "info registers" for example returns only the standard registers. For the CSR registers I get "not available".
Jim Wilson July 23, 2018, 11:27 p.m. UTC | #5 
On Mon, Jul 23, 2018 at 10:24 AM, Sebastian Huber
<sebastian.huber@embedded-brains.de> wrote:
>> The problem with misa is easy to miss, as gdb only tries to read misa
>> if you execute a command that requires info about the target, such as
>> trying to use hardware floating point.  Actually, one of my other
>> patches, the one to remove the pc decrement after a break, modified
>> the code so that we try to read misa when checking to see if
>> compressed breakpoints could be used.  Before it was only checking ELF
>> headers for this, which wasn't right.  This is probably the patch that
>> exposed the bug in your rtems target support.
>
> This could be also a Qemu bug. I have to check this with the debugger tomorrow. The misa should be returned by Qemu in csr_read_helper() (target/riscv/op_helper.c). A "info registers" for example returns only the standard registers. For the CSR registers I get "not available".

Sorry, I see that you did point at the right patch.  I'm on vacation,
and not able to give this my full attention at the moment.

What should happen here is that gdb calls fetch_registers, which then
uses some target specific way to access registers.  In the linux
native support, there is a function defined that uses ptrace to read
registers.  I don't know what happens in the openocd case.  If you are
using target remote, then you are using the fetch_registers call in
remote.c, which means the problem is in the target gdbstub.  Looking
at old qemu in riscv-gnu-toolchain, I see in target-riscv/gdbstub.c in
riscv_cpu_gdb_read_register that it only handles the first 65
registers, which are the int, pc, and fp registers.  Looking at new
qemu in freedom-u-sdk, I see in target/riscv/gdbstub.c that the
riscv_cpu_gdb_read_register function is handling the csrs, and looks
OK.  So maybe the problem is that the qemu you are using is too old?

Jim
Sebastian Huber July 24, 2018, 6:15 a.m. UTC | #6 
On 24/07/18 01:27, Jim Wilson wrote:
> On Mon, Jul 23, 2018 at 10:24 AM, Sebastian Huber
> <sebastian.huber@embedded-brains.de> wrote:
>>> The problem with misa is easy to miss, as gdb only tries to read misa
>>> if you execute a command that requires info about the target, such as
>>> trying to use hardware floating point.  Actually, one of my other
>>> patches, the one to remove the pc decrement after a break, modified
>>> the code so that we try to read misa when checking to see if
>>> compressed breakpoints could be used.  Before it was only checking ELF
>>> headers for this, which wasn't right.  This is probably the patch that
>>> exposed the bug in your rtems target support.
>> This could be also a Qemu bug. I have to check this with the debugger tomorrow. The misa should be returned by Qemu in csr_read_helper() (target/riscv/op_helper.c). A "info registers" for example returns only the standard registers. For the CSR registers I get "not available".
> Sorry, I see that you did point at the right patch.  I'm on vacation,
> and not able to give this my full attention at the moment.

No problem, enjoy your holidays.

>
> What should happen here is that gdb calls fetch_registers, which then
> uses some target specific way to access registers.  In the linux
> native support, there is a function defined that uses ptrace to read
> registers.  I don't know what happens in the openocd case.  If you are
> using target remote, then you are using the fetch_registers call in
> remote.c, which means the problem is in the target gdbstub.  Looking
> at old qemu in riscv-gnu-toolchain, I see in target-riscv/gdbstub.c in
> riscv_cpu_gdb_read_register that it only handles the first 65
> registers, which are the int, pc, and fp registers.  Looking at new
> qemu in freedom-u-sdk, I see in target/riscv/gdbstub.c that the
> riscv_cpu_gdb_read_register function is handling the csrs, and looks
> OK.  So maybe the problem is that the qemu you are using is too old?

It looks like an Qemu issue. I use the latest upstream Qemu:

https://git.qemu.org/?p=qemu.git;a=blob;f=target/riscv/gdbstub.c;h=4f919b6c3413dcda62b018e6d3863a9aed273828;hb=HEAD

I set a break point in Qemu to riscv_cpu_gdb_read_register(), this 
function is only called with n in {0, ..., 64}. In the GDB connected to 
Qemu I get this

(gdb) p $misa
$1 = <unavailable>

and the break point doesn't trigger. We have also in Qemu 
(target/riscv/gdbstub.c)

     cc->gdb_read_register = riscv_cpu_gdb_read_register;
     cc->gdb_write_register = riscv_cpu_gdb_write_register;
     cc->gdb_num_core_regs = 65;

If I change this to

     cc->gdb_num_core_regs = 4096 + 65;

then I end up in Qemu with (SIGSEGV)

Thread 1 "qemu-system-ris" hit Breakpoint 2, 0x00007ffff2c6c220 in 
____longjmp_chk () from /lib64/libc.so.6
(gdb) bt
#0  0x00007ffff2c6c220 in ____longjmp_chk () at /lib64/libc.so.6
#1  0x00007ffff2c6c1f9 in __longjmp_chk () at /lib64/libc.so.6
#2  0x00005555556f7a7f in cpu_loop_exit (cpu=cpu@entry=0x555555e16850) 
at /home/EB/sebastian_h/git-qemu/accel/tcg/cpu-exec-common.c:68
#3  0x00005555556f7ad5 in cpu_loop_exit_restore 
(cpu=cpu@entry=0x555555e16850, pc=pc@entry=93824994209915) at 
/home/EB/sebastian_h/git-qemu/accel/tcg/cpu-exec-common.c:76
#4  0x0000555555732e63 in do_raise_exception_err 
(env=env@entry=0x555555e1eaf8, exception=exception@entry=2, 
pc=93824994209915) at 
/home/EB/sebastian_h/git-qemu/target/riscv/op_helper.c:67
#5  0x0000555555733258 in csr_read_helper (env=0x555555e1eaf8, 
csrno=<optimized out>) at 
/home/EB/sebastian_h/git-qemu/target/riscv/op_helper.c:625
#6  0x000055555573707b in riscv_cpu_gdb_read_register (cs=<optimized 
out>, mem_buf=0x7fffffffa578 "", n=65) at 
/home/EB/sebastian_h/git-qemu/target/riscv/gdbstub.c:36
#7  0x00005555556c0de0 in gdb_handle_packet (s=s@entry=0x555556041e90, 
line_buf=line_buf@entry=0x555556041eac "g") at 
/home/EB/sebastian_h/git-qemu/gdbstub.c:1118
#8  0x00005555556c2369 in gdb_read_byte (ch=55, s=0x555556041e90) at 
/home/EB/sebastian_h/git-qemu/gdbstub.c:1720
#9  0x00005555556c2369 in gdb_chr_receive (opaque=<optimized out>, 
buf=<optimized out>, size=<optimized out>) at 
/home/EB/sebastian_h/git-qemu/gdbstub.c:1931
#10 0x00005555558c6200 in tcp_chr_read (chan=<optimized out>, 
cond=<optimized out>, opaque=<optimized out>) at 
/home/EB/sebastian_h/git-qemu/chardev/char-socket.c:473
#11 0x00007ffff5684015 in g_main_context_dispatch () at 
/usr/lib64/libglib-2.0.so.0
#12 0x0000555555928817 in glib_pollfds_poll () at 
/home/EB/sebastian_h/git-qemu/util/main-loop.c:215
#13 0x0000555555928817 in os_host_main_loop_wait (timeout=<optimized 
out>) at /home/EB/sebastian_h/git-qemu/util/main-loop.c:238
#14 0x0000555555928817 in main_loop_wait (nonblocking=<optimized out>) 
at /home/EB/sebastian_h/git-qemu/util/main-loop.c:497
#15 0x0000555555748ff2 in main_loop () at 
/home/EB/sebastian_h/git-qemu/vl.c:1866
#16 0x00005555556622ab in main (argc=<optimized out>, argv=<optimized 
out>, envp=<optimized out>) at /home/EB/sebastian_h/git-qemu/vl.c:4644

Then some locking issues with the iothread mutex occurred. Afterwards a 
stack overflow in

static int gdb_handle_packet(GDBState *s, const char *line_buf)
{
     CPUState *cpu;
     CPUClass *cc;
     const char *p;
     uint32_t thread;
     int ch, reg_size, type, res;
     uint8_t mem_buf[MAX_PACKET_LENGTH];

since 4096 + 64 registers seems to be quite a lot.

What is the right place for RISC-V Qemu bug reports?
Jim Wilson July 24, 2018, 11:41 a.m. UTC | #7 
On Mon, Jul 23, 2018 at 11:15 PM, Sebastian Huber
<sebastian.huber@embedded-brains.de> wrote:
> What is the right place for RISC-V Qemu bug reports?

You can file an issue in github riscv/riscv-qemu.  That is what I
would do.  There is probably also some upstream mailing list or bug
reporting system but I don't know it.

Jim
Sebastian Huber July 26, 2018, 6:30 a.m. UTC | #8 
On 24/07/18 13:41, Jim Wilson wrote:
> On Mon, Jul 23, 2018 at 11:15 PM, Sebastian Huber
> <sebastian.huber@embedded-brains.de>  wrote:
>> What is the right place for RISC-V Qemu bug reports?
> You can file an issue in github riscv/riscv-qemu.  That is what I
> would do.  There is probably also some upstream mailing list or bug
> reporting system but I don't know it.

Just for reference:

https://github.com/riscv/riscv-qemu/issues/156
Palmer Dabbelt Aug. 3, 2018, 10:05 p.m. UTC | #9 
On Mon, 16 Jul 2018 17:12:41 PDT (-0700), Jim Wilson wrote:
> This is the gdb patch for the RISC-V Linux kernel patch I just submitted.
>     https://patchwork.kernel.org/patch/10524925/
> This removes the code that decrements the pc after a break, now that we have
> a patch to stop linux from pointlessly adding to the pc after a break.  It
> would be nice if this goes in now, to avoid unnecessary divergence between
> gdb and the linux kernel.  Palmer, the RISC-V linux kernel maintainer, has
> already agreed to accept that patch.

It's in the queue: 
https://git.kernel.org/pub/scm/linux/kernel/git/palmer/linux.git/commit/?h=fix-noinc&id=4d788e22c5ed2f76cf5112122cde85993d055b78

>
> This will also be needed by the FreeBSD gdb port which may be started soon.
>
> This also fixes a bug in the code.  The fact that we have two different
> mechanisms to decide breakpoint size, used_compressed_breakpoints and
> has_compressed_isa, means that it is possible for gdb to emit a 4 byte
> breakpoint and then subtract 2 from the pc, and vice versa.  Removing the
> unnecessary pc decrement fixes that problem.
>
> OK?
>
> Jim
>
> 	gdb/
> 	* riscv-tdep.c (riscv_has_feature): Delete comment that refers to
> 	set_gdbarch_decr_pc_after_break.  Call riscv_read_misa_reg always.
> 	(riscv_gdbarch_init): Delete local has_compressed_isa.  Delete now
> 	unecessary braces after EF_RISCV_RVC test.  Delete call to
> 	set_gdbarch_decr_pc_after_break.
> ---
>  gdb/riscv-tdep.c | 25 ++-----------------------
>  1 file changed, 2 insertions(+), 23 deletions(-)
>
> diff --git a/gdb/riscv-tdep.c b/gdb/riscv-tdep.c
> index 72dab0f897..f5d1af822c 100644
> --- a/gdb/riscv-tdep.c
> +++ b/gdb/riscv-tdep.c
> @@ -335,23 +335,7 @@ riscv_has_feature (struct gdbarch *gdbarch, char feature)
>
>    gdb_assert (feature >= 'A' && feature <= 'Z');
>
> -  /* It would be nice to always check with the real target where possible,
> -     however, for compressed instructions this is a bad idea.
> -
> -     The call to `set_gdbarch_decr_pc_after_break' is made just once per
> -     GDBARCH and we decide at that point if we should decrement by 2 or 4
> -     bytes based on whether the BFD has compressed instruction support or
> -     not.
> -
> -     If the BFD was not compiled with compressed instruction support, but we
> -     are running on a target with compressed instructions then we might
> -     place a 4-byte breakpoint, then decrement the $pc by 2 bytes leading to
> -     confusion.
> -
> -     It's safer if we just make decisions about compressed instruction
> -     support based on the BFD.  */
> -  if (feature != 'C')
> -    misa = riscv_read_misa_reg (&have_read_misa);
> +  misa = riscv_read_misa_reg (&have_read_misa);
>    if (!have_read_misa || misa == 0)
>      misa = gdbarch_tdep (gdbarch)->core_features;
>
> @@ -2440,7 +2424,6 @@ riscv_gdbarch_init (struct gdbarch_info info,
>    struct gdbarch *gdbarch;
>    struct gdbarch_tdep *tdep;
>    struct gdbarch_tdep tmp_tdep;
> -  bool has_compressed_isa = false;
>    int i;
>
>    /* Ideally, we'd like to get as much information from the target for
> @@ -2472,10 +2455,7 @@ riscv_gdbarch_init (struct gdbarch_info info,
>  			_("unknown ELF header class %d"), eclass);
>
>        if (e_flags & EF_RISCV_RVC)
> -	{
> -	  has_compressed_isa = true;
> -	  tmp_tdep.core_features |= (1 << ('C' - 'A'));
> -	}
> +	tmp_tdep.core_features |= (1 << ('C' - 'A'));
>
>        if (e_flags & EF_RISCV_FLOAT_ABI_DOUBLE)
>  	{
> @@ -2545,7 +2525,6 @@ riscv_gdbarch_init (struct gdbarch_info info,
>    set_gdbarch_register_reggroup_p (gdbarch, riscv_register_reggroup_p);
>
>    /* Functions to analyze frames.  */
> -  set_gdbarch_decr_pc_after_break (gdbarch, (has_compressed_isa ? 2 : 4));
>    set_gdbarch_skip_prologue (gdbarch, riscv_skip_prologue);
>    set_gdbarch_inner_than (gdbarch, core_addr_lessthan);
>    set_gdbarch_frame_align (gdbarch, riscv_frame_align);
diff mbox

Patch

diff --git a/gdb/riscv-tdep.c b/gdb/riscv-tdep.c
index 72dab0f897..f5d1af822c 100644
--- a/gdb/riscv-tdep.c
+++ b/gdb/riscv-tdep.c
@@ -335,23 +335,7 @@  riscv_has_feature (struct gdbarch *gdbarch, char feature)
 
   gdb_assert (feature >= 'A' && feature <= 'Z');
 
-  /* It would be nice to always check with the real target where possible,
-     however, for compressed instructions this is a bad idea.
-
-     The call to `set_gdbarch_decr_pc_after_break' is made just once per
-     GDBARCH and we decide at that point if we should decrement by 2 or 4
-     bytes based on whether the BFD has compressed instruction support or
-     not.
-
-     If the BFD was not compiled with compressed instruction support, but we
-     are running on a target with compressed instructions then we might
-     place a 4-byte breakpoint, then decrement the $pc by 2 bytes leading to
-     confusion.
-
-     It's safer if we just make decisions about compressed instruction
-     support based on the BFD.  */
-  if (feature != 'C')
-    misa = riscv_read_misa_reg (&have_read_misa);
+  misa = riscv_read_misa_reg (&have_read_misa);
   if (!have_read_misa || misa == 0)
     misa = gdbarch_tdep (gdbarch)->core_features;
 
@@ -2440,7 +2424,6 @@  riscv_gdbarch_init (struct gdbarch_info info,
   struct gdbarch *gdbarch;
   struct gdbarch_tdep *tdep;
   struct gdbarch_tdep tmp_tdep;
-  bool has_compressed_isa = false;
   int i;
 
   /* Ideally, we'd like to get as much information from the target for
@@ -2472,10 +2455,7 @@  riscv_gdbarch_init (struct gdbarch_info info,
 			_("unknown ELF header class %d"), eclass);
 
       if (e_flags & EF_RISCV_RVC)
-	{
-	  has_compressed_isa = true;
-	  tmp_tdep.core_features |= (1 << ('C' - 'A'));
-	}
+	tmp_tdep.core_features |= (1 << ('C' - 'A'));
 
       if (e_flags & EF_RISCV_FLOAT_ABI_DOUBLE)
 	{
@@ -2545,7 +2525,6 @@  riscv_gdbarch_init (struct gdbarch_info info,
   set_gdbarch_register_reggroup_p (gdbarch, riscv_register_reggroup_p);
 
   /* Functions to analyze frames.  */
-  set_gdbarch_decr_pc_after_break (gdbarch, (has_compressed_isa ? 2 : 4));
   set_gdbarch_skip_prologue (gdbarch, riscv_skip_prologue);
   set_gdbarch_inner_than (gdbarch, core_addr_lessthan);
   set_gdbarch_frame_align (gdbarch, riscv_frame_align);