* [PATCH] gdb: set only inferior_ptid in sparc_{fetch, store}_inferior_registers
@ 2021-05-31 1:16 Simon Marchi
2021-06-06 14:52 ` Joel Brobecker
0 siblings, 1 reply; 4+ messages in thread
From: Simon Marchi @ 2021-05-31 1:16 UTC (permalink / raw)
To: gdb-patches
The past commit d1e93af64a6b ("gdb: set current thread in
sparc_{fetch,collect}_inferior_registers (PR gdb/27147)") changed
sparc_fetch_inferior_registers and sparc_store_inferior_registers to
look up the thread corresponding to the regcache's ptid and make it the
current thread. The reason being that down the call chain, some
functions (like sparc_supply_rwindow) can do some memory reads or write,
through target_read_memory/target_write_memory, and those rely on the
current global context.
There is one small problem with this approach: when debugging a
multi-threaded program, the regcache for a new thread is created just
before the corresponding thread_info is created. In fact, the regcache
is created somewhere during the call to thread_from_lwp, which is
responsible for creating the thread_info:
#8 0x0000010000ab9968 in internal_error (file=0x10000bfca20 "/home/simark/src/binutils-gdb/gdb/thread.c", line=1346, fmt=0x10000bfc918 "%s: Assertion `%s' failed.") at /home/simark/src/binutils-gdb/gdbsupport/errors.cc:55
#9 0x0000010000827f3c in switch_to_thread (thr=0x0) at /home/simark/src/binutils-gdb/gdb/thread.c:1346
#10 0x0000010000753444 in sparc_fetch_inferior_registers (proc_target=0x10000fa8cb0 <the_sparc64_linux_nat_target>, regcache=0x10000ff03c0, regnum=-1) at /home/simark/src/binutils-gdb/gdb/sparc-nat.c:175
#11 0x000001000075b908 in sparc64_linux_nat_target::fetch_registers (this=0x10000fa8cb0 <the_sparc64_linux_nat_target>, regcache=0x10000ff03c0, regnum=-1) at /home/simark/src/binutils-gdb/gdb/sparc64-linux-nat.c:38
#12 0x00000100007fe6f4 in target_ops::fetch_registers (this=0x10000f7feb0 <the_thread_db_target>, arg0=0x10000ff03c0, arg1=-1) at /home/simark/src/binutils-gdb/gdb/target-delegates.c:496
#13 0x00000100008162a0 in target_fetch_registers (regcache=0x10000ff03c0, regno=-1) at /home/simark/src/binutils-gdb/gdb/target.c:3287
#14 0x000001000060a4bc in ps_lgetregs (ph=0x10001264368, lwpid=458727, gregset=0x7feff97d388) at /home/simark/src/binutils-gdb/gdb/proc-service.c:158
#15 0xffff800103e32420 in __td_ta_lookup_th_unique (ta_arg=0x100012d7080, lwpid=<optimized out>, th=0x7feff97d7c8) at td_ta_map_lwp2thr.c:119
#16 0xffff800103e32604 in td_ta_map_lwp2thr (ta_arg=0x100012d7080, lwpid=<optimized out>, th=0x7feff97d7c8) at td_ta_map_lwp2thr.c:207
#17 0x000001000051fee8 in thread_from_lwp (stopped=0x100011a3650, ptid=...) at /home/simark/src/binutils-gdb/gdb/linux-thread-db.c:415
#18 0x0000010000520150 in thread_db_notice_clone (parent=..., child=...) at /home/simark/src/binutils-gdb/gdb/linux-thread-db.c:446
#19 0x00000100005068a8 in linux_handle_extended_wait (lp=0x10001230700, status=4479) at /home/simark/src/binutils-gdb/gdb/linux-nat.c:1978
#20 0x000001000050a278 in linux_nat_filter_event (lwpid=458724, status=198015) at /home/simark/src/binutils-gdb/gdb/linux-nat.c:2913
#21 0x000001000050b818 in linux_nat_wait_1 (ptid=..., ourstatus=0x7feff97e8d0, target_options=...) at /home/simark/src/binutils-gdb/gdb/linux-nat.c:3194
#22 0x000001000050ca4c in linux_nat_target::wait (this=0x10000fa8cb0 <the_sparc64_linux_nat_target>, ptid=..., ourstatus=0x7feff97e8d0, target_options=...) at /home/simark/src/binutils-gdb/gdb/linux-nat.c:3432
#23 0x00000100005237ec in thread_db_target::wait (this=0x10000f7feb0 <the_thread_db_target>, ptid=..., ourstatus=0x7feff97e8d0, options=...) at /home/simark/src/binutils-gdb/gdb/linux-thread-db.c:1379
#24 0x00000100007fa668 in target_wait (ptid=..., status=0x7feff97e8d0, options=...) at /home/simark/src/binutils-gdb/gdb/target.c:2000
#25 0x00000100004adb0c in do_target_wait_1 (inf=0x10001173170, ptid=..., status=0x7feff97e8d0, options=...) at /home/simark/src/binutils-gdb/gdb/infrun.c:3464
#26 0x00000100004add48 in operator() (__closure=0x7feff97e658, inf=0x10001173170) at /home/simark/src/binutils-gdb/gdb/infrun.c:3527
#27 0x00000100004ae15c in do_target_wait (wait_ptid=..., ecs=0x7feff97e8a8, options=...) at /home/simark/src/binutils-gdb/gdb/infrun.c:3540
#28 0x00000100004af254 in fetch_inferior_event () at /home/simark/src/binutils-gdb/gdb/infrun.c:3880
#29 0x0000010000486ef8 in inferior_event_handler (event_type=INF_REG_EVENT) at /home/simark/src/binutils-gdb/gdb/inf-loop.c:42
The problem is that while sparc_fetch_inferior_registers runs and is
asked to read the registers of a given ptid, there isn't a thread_info
with that ptid yet. So, find_thread_ptid returns nullptr, and
switch_to_thread gives an internal error.
Fix this by only setting inferior_ptid, instead of the whole global
context, as switch_to_thread does. This is sufficient for
target_read_memory / target_write_memory to work down the line.
Ideally, it would be nice to be able to pass the ptid down the whole
call chain and to target_read_memory / target_write_memory, so that this
setting of inferior_ptid would not be necessary. But this is not going
to happen soon.
This fixes running a multi-threaded program, which would hit the
internal error show in the call stack above.
gdb/ChangeLog:
PR gdb/27899
* sparc-nat.c (sparc_fetch_inferior_registers): Set
inferior_ptid instead of using switch_to_thread.
(sparc_store_inferior_registers): Likewise.
Change-Id: I0b6ddb3af9b11f67b10ee46a734fb82ecc6462d5
---
gdb/sparc-nat.c | 10 ++++------
1 file changed, 4 insertions(+), 6 deletions(-)
diff --git a/gdb/sparc-nat.c b/gdb/sparc-nat.c
index fa3b32cee184..7f09a60420db 100644
--- a/gdb/sparc-nat.c
+++ b/gdb/sparc-nat.c
@@ -170,9 +170,8 @@ sparc_fetch_inferior_registers (process_stratum_target *proc_target,
/* Deep down, sparc_supply_rwindow reads memory, so needs the global
thread context to be set. */
- thread_info *thread = find_thread_ptid (proc_target, ptid);
- scoped_restore_current_thread restore_thread;
- switch_to_thread (thread);
+ scoped_restore restore_inferior_ptid
+ = make_scoped_restore (&inferior_ptid, ptid);
sparc_supply_gregset (sparc_gregmap, regcache, -1, ®s);
if (regnum != -1)
@@ -219,9 +218,8 @@ sparc_store_inferior_registers (process_stratum_target *proc_target,
/* Deep down, sparc_collect_rwindow writes memory, so needs the global
thread context to be set. */
- thread_info *thread = find_thread_ptid (proc_target, ptid);
- scoped_restore_current_thread restore_thread;
- switch_to_thread (thread);
+ scoped_restore restore_inferior_ptid
+ = make_scoped_restore (&inferior_ptid, ptid);
sparc_collect_rwindow (regcache, sp, regnum);
}
--
2.31.1
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] gdb: set only inferior_ptid in sparc_{fetch, store}_inferior_registers
2021-05-31 1:16 [PATCH] gdb: set only inferior_ptid in sparc_{fetch, store}_inferior_registers Simon Marchi
@ 2021-06-06 14:52 ` Joel Brobecker
2021-06-06 15:02 ` Simon Marchi
0 siblings, 1 reply; 4+ messages in thread
From: Joel Brobecker @ 2021-06-06 14:52 UTC (permalink / raw)
To: Simon Marchi via Gdb-patches; +Cc: Joel Brobecker
Hi Simon,
> gdb/ChangeLog:
>
> PR gdb/27899
> * sparc-nat.c (sparc_fetch_inferior_registers): Set
> inferior_ptid instead of using switch_to_thread.
> (sparc_store_inferior_registers): Likewise.
Thanks for the patch. This looks good to me. Were you able to run
the testsuite on the patch? If not, I could probably run it against
AdaCore's testsuite on a LEON3 baremetal target -- not as good as
the full official testsuite on a SPARC/Linux machine, but better
than nothing...
Thanks!
>
> Change-Id: I0b6ddb3af9b11f67b10ee46a734fb82ecc6462d5
> ---
> gdb/sparc-nat.c | 10 ++++------
> 1 file changed, 4 insertions(+), 6 deletions(-)
>
> diff --git a/gdb/sparc-nat.c b/gdb/sparc-nat.c
> index fa3b32cee184..7f09a60420db 100644
> --- a/gdb/sparc-nat.c
> +++ b/gdb/sparc-nat.c
> @@ -170,9 +170,8 @@ sparc_fetch_inferior_registers (process_stratum_target *proc_target,
>
> /* Deep down, sparc_supply_rwindow reads memory, so needs the global
> thread context to be set. */
> - thread_info *thread = find_thread_ptid (proc_target, ptid);
> - scoped_restore_current_thread restore_thread;
> - switch_to_thread (thread);
> + scoped_restore restore_inferior_ptid
> + = make_scoped_restore (&inferior_ptid, ptid);
>
> sparc_supply_gregset (sparc_gregmap, regcache, -1, ®s);
> if (regnum != -1)
> @@ -219,9 +218,8 @@ sparc_store_inferior_registers (process_stratum_target *proc_target,
>
> /* Deep down, sparc_collect_rwindow writes memory, so needs the global
> thread context to be set. */
> - thread_info *thread = find_thread_ptid (proc_target, ptid);
> - scoped_restore_current_thread restore_thread;
> - switch_to_thread (thread);
> + scoped_restore restore_inferior_ptid
> + = make_scoped_restore (&inferior_ptid, ptid);
>
> sparc_collect_rwindow (regcache, sp, regnum);
> }
> --
> 2.31.1
>
--
Joel
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] gdb: set only inferior_ptid in sparc_{fetch, store}_inferior_registers
2021-06-06 14:52 ` Joel Brobecker
@ 2021-06-06 15:02 ` Simon Marchi
2021-06-07 15:02 ` Simon Marchi
0 siblings, 1 reply; 4+ messages in thread
From: Simon Marchi @ 2021-06-06 15:02 UTC (permalink / raw)
To: Joel Brobecker, Simon Marchi via Gdb-patches
On 2021-06-06 10:52 a.m., Joel Brobecker wrote:
> Thanks for the patch. This looks good to me. Were you able to run
> the testsuite on the patch? If not, I could probably run it against
> AdaCore's testsuite on a LEON3 baremetal target -- not as good as
> the full official testsuite on a SPARC/Linux machine, but better
> than nothing...
I will run it on the machine I used for testing this (gcc102). If it
looks good I'll merge the patch.
Simon
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] gdb: set only inferior_ptid in sparc_{fetch, store}_inferior_registers
2021-06-06 15:02 ` Simon Marchi
@ 2021-06-07 15:02 ` Simon Marchi
0 siblings, 0 replies; 4+ messages in thread
From: Simon Marchi @ 2021-06-07 15:02 UTC (permalink / raw)
To: Joel Brobecker, Simon Marchi via Gdb-patches
On 2021-06-06 11:02 a.m., Simon Marchi via Gdb-patches wrote:
> On 2021-06-06 10:52 a.m., Joel Brobecker wrote:
>> Thanks for the patch. This looks good to me. Were you able to run
>> the testsuite on the patch? If not, I could probably run it against
>> AdaCore's testsuite on a LEON3 baremetal target -- not as good as
>> the full official testsuite on a SPARC/Linux machine, but better
>> than nothing...
>
> I will run it on the machine I used for testing this (gcc102). If it
> looks good I'll merge the patch.
>
> Simon
>
Without the patch, the testsuite wouldn't finish in reasonable time
because of too many internal errors when it deals with threaded code.
With the patch, it finishes with:
=== gdb Summary ===
# of expected passes 73610
# of unexpected failures 1311
# of expected failures 67
# of unknown successes 1
# of known failures 111
# of unresolved testcases 8
# of untested testcases 64
# of unsupported tests 180
# of duplicate test names 87
So I consider that the patch helps, I'll merge it.
Simon
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2021-06-07 15:02 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-05-31 1:16 [PATCH] gdb: set only inferior_ptid in sparc_{fetch, store}_inferior_registers Simon Marchi
2021-06-06 14:52 ` Joel Brobecker
2021-06-06 15:02 ` Simon Marchi
2021-06-07 15:02 ` Simon Marchi
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).