* [PATCH 0/3] Fix crash if connection drops in scoped_restore_current_thread's ctor
@ 2020-07-08 23:31 Pedro Alves
2020-07-08 23:31 ` [PATCH 1/3] Fix crash if connection drops in scoped_restore_current_thread's ctor, part 1 Pedro Alves
` (3 more replies)
0 siblings, 4 replies; 31+ messages in thread
From: Pedro Alves @ 2020-07-08 23:31 UTC (permalink / raw)
To: gdb-patches
(I have internet again: found a sim card of a different operator that
works. This will do until the communications tower near me is
repaired and get I fiber back...)
This series fixes the crashes exposed by the
gdb.multi/multi-target.exp testcase when run against an Asan-enabled
GDB build, initially reported by Simon here:
https://sourceware.org/pipermail/gdb-patches/2020-July/170222.html
The first two patches fix the crashes, and we should probably put them
in GDB 10.
The last patch is a follow up that avoids swallowing exceptions in
scoped_restore_current_thread's dtor that I'm thinking would be a bit
too invasive to put in GDB 10, I think it could do with a longer
baking period in master.
Pedro Alves (3):
Fix crash if connection drops in scoped_restore_current_thread's ctor,
part 1
Fix crash if connection drops in scoped_restore_current_thread's ctor,
part 2
Make scoped_restore_current_thread's cdtors exception free (RFC)
gdb/blockframe.c | 6 +---
gdb/dwarf2/frame-tailcall.c | 18 +++++++++--
gdb/frame.c | 73 ++++++++++++++++++++++++++++++-------------
gdb/frame.h | 22 ++++++++++---
gdb/gdbthread.h | 4 +++
gdb/stack.c | 9 +++---
gdb/thread.c | 76 ++++++++++++++++-----------------------------
gdb/value.c | 13 +++++++-
8 files changed, 132 insertions(+), 89 deletions(-)
base-commit: ad8464f799a4c96c7ab8bdfec3f95846cf54f9b0
prerequisite-patch-id: 32ffdda7d7d774bc4df88bf848bcb796559b53ce
prerequisite-patch-id: 02021b74355b70debd344a6e445285c67dfef7d6
prerequisite-patch-id: c87fcf5a54f6805967cbf8ab107606c57d9ecf52
prerequisite-patch-id: ac7dee583d0ffa519c9d1cd89d27664bca68d8c1
prerequisite-patch-id: eac59ae2ea85d2d51e5be1b03e88a5641cc12c22
prerequisite-patch-id: 13da42ad04dc8e2e3bd6a556a0be0e17cf23669b
prerequisite-patch-id: fd3f09fdb58ddc1c595ea014716851f4c8fca48c
--
2.14.5
^ permalink raw reply [flat|nested] 31+ messages in thread* [PATCH 1/3] Fix crash if connection drops in scoped_restore_current_thread's ctor, part 1 2020-07-08 23:31 [PATCH 0/3] Fix crash if connection drops in scoped_restore_current_thread's ctor Pedro Alves @ 2020-07-08 23:31 ` Pedro Alves 2020-07-09 3:17 ` Simon Marchi 2020-07-08 23:31 ` [PATCH 2/3] Fix crash if connection drops in scoped_restore_current_thread's ctor, part 2 Pedro Alves ` (2 subsequent siblings) 3 siblings, 1 reply; 31+ messages in thread From: Pedro Alves @ 2020-07-08 23:31 UTC (permalink / raw) To: gdb-patches Running the testsuite against an Asan-enabled build of GDB makes gdb.base/multi-target.exp expose this bug. scoped_restore_current_thread's ctor calls get_frame_id to record the selected frame's ID to restore later. If the frame ID hasn't been computed yet, it will be computed on the spot, and that will usually require accessing the target's memory and registers, which requires remote accesses. If the remote connection closes while we're computing the frame ID, the remote target exits its inferiors, unpushes itself, and throws a TARGET_CLOSE_ERROR error. If that happens, GDB can currently crash, here: > ==18555==ERROR: AddressSanitizer: heap-use-after-free on address 0x621004670aa8 at pc 0x0000007ab125 bp 0x7ffdecaecd20 sp 0x7ffdecaecd10 > READ of size 4 at 0x621004670aa8 thread T0 > #0 0x7ab124 in dwarf2_frame_this_id src/binutils-gdb/gdb/dwarf2/frame.c:1228 > #1 0x983ec5 in compute_frame_id src/binutils-gdb/gdb/frame.c:550 > #2 0x9841ee in get_frame_id(frame_info*) src/binutils-gdb/gdb/frame.c:582 > #3 0x1093faa in scoped_restore_current_thread::scoped_restore_current_thread() src/binutils-gdb/gdb/thread.c:1462 > #4 0xaee5ba in fetch_inferior_event(void*) src/binutils-gdb/gdb/infrun.c:3968 > #5 0xaa990b in inferior_event_handler(inferior_event_type, void*) src/binutils-gdb/gdb/inf-loop.c:43 > #6 0xea61b6 in remote_async_serial_handler src/binutils-gdb/gdb/remote.c:14161 > #7 0xefca8a in run_async_handler_and_reschedule src/binutils-gdb/gdb/ser-base.c:137 > #8 0xefcd23 in fd_event src/binutils-gdb/gdb/ser-base.c:188 > #9 0x15a7416 in handle_file_event src/binutils-gdb/gdbsupport/event-loop.cc:548 > #10 0x15a7c36 in gdb_wait_for_event src/binutils-gdb/gdbsupport/event-loop.cc:673 > #11 0x15a5dbb in gdb_do_one_event() src/binutils-gdb/gdbsupport/event-loop.cc:215 > #12 0xbfe62d in start_event_loop src/binutils-gdb/gdb/main.c:356 > #13 0xbfe935 in captured_command_loop src/binutils-gdb/gdb/main.c:416 > #14 0xc01d39 in captured_main src/binutils-gdb/gdb/main.c:1253 > #15 0xc01dc9 in gdb_main(captured_main_args*) src/binutils-gdb/gdb/main.c:1268 > #16 0x414ddd in main src/binutils-gdb/gdb/gdb.c:32 > #17 0x7f590110b82f in __libc_start_main ../csu/libc-start.c:291 > #18 0x414bd8 in _start (build/binutils-gdb/gdb/gdb+0x414bd8) What happens is that above, we're in dwarf2_frame_this_id, just after the dwarf2_frame_cache call. The "cache" variable that the dwarf2_frame_cache function returned is already stale. It's been released here, from within the dwarf2_frame_cache: (top-gdb) bt #0 reinit_frame_cache () at src/gdb/frame.c:1855 #1 0x00000000014ff7b0 in switch_to_no_thread () at src/gdb/thread.c:1301 #2 0x0000000000f66d3e in switch_to_inferior_no_thread (inf=0x615000338180) at src/gdb/inferior.c:626 #3 0x00000000012f3826 in remote_unpush_target (target=0x6170000c5900) at src/gdb/remote.c:5521 #4 0x00000000013097e0 in remote_target::readchar (this=0x6170000c5900, timeout=2) at src/gdb/remote.c:9137 #5 0x000000000130be4d in remote_target::getpkt_or_notif_sane_1 (this=0x6170000c5900, buf=0x6170000c5918, forever=0, expecting_notif=0, is_notif=0x0) at src/gdb/remote.c:9683 #6 0x000000000130c8ab in remote_target::getpkt_sane (this=0x6170000c5900, buf=0x6170000c5918, forever=0) at src/gdb/remote.c:9790 #7 0x000000000130bc0d in remote_target::getpkt (this=0x6170000c5900, buf=0x6170000c5918, forever=0) at src/gdb/remote.c:9623 #8 0x000000000130838e in remote_target::remote_read_bytes_1 (this=0x6170000c5900, memaddr=0x7fffffffcdc0, myaddr=0x6080000ad3bc "", len_units=64, unit_size=1, xfered_len_units=0x7fff6a29b9a0) at src/gdb/remote.c:8860 #9 0x0000000001308bd2 in remote_target::remote_read_bytes (this=0x6170000c5900, memaddr=0x7fffffffcdc0, myaddr=0x6080000ad3bc "", len=64, unit_size=1, xfered_len=0x7fff6a29b9a0) at src/gdb/remote.c:8987 #10 0x0000000001311ed1 in remote_target::xfer_partial (this=0x6170000c5900, object=TARGET_OBJECT_MEMORY, annex=0x0, readbuf=0x6080000ad3bc "", writebuf=0x0, offset=140737488342464, len=64, xfered_len=0x7fff6a29b9a0) at src/gdb/remote.c:10988 #11 0x00000000014ba969 in raw_memory_xfer_partial (ops=0x6170000c5900, readbuf=0x6080000ad3bc "", writebuf=0x0, memaddr=140737488342464, len=64, xfered_len=0x7fff6a29b9a0) at src/gdb/target.c:918 #12 0x00000000014bb720 in target_xfer_partial (ops=0x6170000c5900, object=TARGET_OBJECT_RAW_MEMORY, annex=0x0, readbuf=0x6080000ad3bc "", writebuf=0x0, offset=140737488342464, len=64, xfered_len=0x7fff6a29b9a0) at src/gdb/target.c:1148 #13 0x00000000014bc3b5 in target_read_partial (ops=0x6170000c5900, object=TARGET_OBJECT_RAW_MEMORY, annex=0x0, buf=0x6080000ad3bc "", offset=140737488342464, len=64, xfered_len=0x7fff6a29b9a0) at src/gdb/target.c:1380 #14 0x00000000014bc593 in target_read (ops=0x6170000c5900, object=TARGET_OBJECT_RAW_MEMORY, annex=0x0, buf=0x6080000ad3bc "", offset=140737488342464, len=64) at src/gdb/target.c:1419 #15 0x00000000014bbd4d in target_read_raw_memory (memaddr=0x7fffffffcdc0, myaddr=0x6080000ad3bc "", len=64) at src/gdb/target.c:1252 #16 0x0000000000bf27df in dcache_read_line (dcache=0x6060001eddc0, db=0x6080000ad3a0) at src/gdb/dcache.c:336 #17 0x0000000000bf2b72 in dcache_peek_byte (dcache=0x6060001eddc0, addr=0x7fffffffcdd8, ptr=0x6020001231b0 "") at src/gdb/dcache.c:403 #18 0x0000000000bf3103 in dcache_read_memory_partial (ops=0x6170000c5900, dcache=0x6060001eddc0, memaddr=0x7fffffffcdd8, myaddr=0x6020001231b0 "", len=8, xfered_len=0x7fff6a29bf20) at src/gdb/dcache.c:484 #19 0x00000000014bafe9 in memory_xfer_partial_1 (ops=0x6170000c5900, object=TARGET_OBJECT_STACK_MEMORY, readbuf=0x6020001231b0 "", writebuf=0x0, memaddr=140737488342488, len=8, xfered_len=0x7fff6a29bf20) at src/gdb/target.c:1034 #20 0x00000000014bb212 in memory_xfer_partial (ops=0x6170000c5900, object=TARGET_OBJECT_STACK_MEMORY, readbuf=0x6020001231b0 "", writebuf=0x0, memaddr=140737488342488, len=8, xfered_len=0x7fff6a29bf20) at src/gdb/target.c:1076 #21 0x00000000014bb6b3 in target_xfer_partial (ops=0x6170000c5900, object=TARGET_OBJECT_STACK_MEMORY, annex=0x0, readbuf=0x6020001231b0 "", writebuf=0x0, offset=140737488342488, len=8, xfered_len=0x7fff6a29bf20) at src/gdb/target.c:1133 #22 0x000000000164564d in read_value_memory (val=0x60f000029440, bit_offset=0, stack=1, memaddr=0x7fffffffcdd8, buffer=0x6020001231b0 "", length=8) at src/gdb/valops.c:956 #23 0x0000000001680fff in value_fetch_lazy_memory (val=0x60f000029440) at src/gdb/value.c:3764 #24 0x0000000001681efd in value_fetch_lazy (val=0x60f000029440) at src/gdb/value.c:3910 #25 0x0000000001676143 in value_optimized_out (value=0x60f000029440) at src/gdb/value.c:1411 #26 0x0000000000e0fcb8 in frame_register_unwind (next_frame=0x6210066bfde0, regnum=16, optimizedp=0x7fff6a29c200, unavailablep=0x7fff6a29c240, lvalp=0x7fff6a29c2c0, addrp=0x7fff6a29c300, realnump=0x7fff6a29c280, bufferp=0x7fff6a29c3a0 "@\304)j\377\177") at src/gdb/frame.c:1144 #27 0x0000000000e10418 in frame_unwind_register (next_frame=0x6210066bfde0, regnum=16, buf=0x7fff6a29c3a0 "@\304)j\377\177") at src/gdb/frame.c:1196 #28 0x0000000000f00431 in i386_unwind_pc (gdbarch=0x6210043d0110, next_frame=0x6210066bfde0) at src/gdb/i386-tdep.c:1969 #29 0x0000000000e39724 in gdbarch_unwind_pc (gdbarch=0x6210043d0110, next_frame=0x6210066bfde0) at src/gdb/gdbarch.c:3056 #30 0x0000000000c2ea90 in dwarf2_tailcall_sniffer_first (this_frame=0x6210066bfde0, tailcall_cachep=0x6210066bfee0, entry_cfa_sp_offsetp=0x0) at src/gdb/dwarf2/frame-tailcall.c:423 #31 0x0000000000c36bdb in dwarf2_frame_cache (this_frame=0x6210066bfde0, this_cache=0x6210066bfdf8) at src/gdb/dwarf2/frame.c:1198 #32 0x0000000000c36eb3 in dwarf2_frame_this_id (this_frame=0x6210066bfde0, this_cache=0x6210066bfdf8, this_id=0x6210066bfe40) at src/gdb/dwarf2/frame.c:1226 Note that remote_target::readchar in frame #3 throws TARGET_CLOSE_ERROR after that remote_unpush_target in frame #3 returns. The problem is that that TARGET_CLOSE_ERROR is swallowed by value_optimized_out in frame #25. If we fix that one, then we run into dwarf2_tailcall_sniffer_first swallowing the exception in frame #30 too. The attached patch fixes it by making those spots swallow fewer kinds of errors. gdb/ChangeLog: * frame-tailcall.c (dwarf2_tailcall_sniffer_first): Only swallow NO_ENTRY_VALUE_ERROR. * value.c (value_optimized_out): Only swallow OPTIMIZED_OUT_ERROR. --- gdb/dwarf2/frame-tailcall.c | 18 ++++++++++++++++-- gdb/value.c | 13 ++++++++++++- 2 files changed, 28 insertions(+), 3 deletions(-) diff --git a/gdb/dwarf2/frame-tailcall.c b/gdb/dwarf2/frame-tailcall.c index 16dba2b201..ec6ed6bb00 100644 --- a/gdb/dwarf2/frame-tailcall.c +++ b/gdb/dwarf2/frame-tailcall.c @@ -377,7 +377,6 @@ dwarf2_tailcall_sniffer_first (struct frame_info *this_frame, get_frame_address_in_block will decrease it by 1 in such case. */ this_pc = get_frame_address_in_block (this_frame); - /* Catch any unwinding errors. */ try { int sp_regnum; @@ -439,7 +438,22 @@ dwarf2_tailcall_sniffer_first (struct frame_info *this_frame, { if (entry_values_debug) exception_print (gdb_stdout, except); - return; + + switch (except.error) + { + case NO_ENTRY_VALUE_ERROR: + /* Thrown by call_site_find_chain. */ + case MEMORY_ERROR: + case OPTIMIZED_OUT_ERROR: + case NOT_AVAILABLE_ERROR: + /* These can normally happen when we try to access an + optimized out or unavailable register, either in a + physical register or spilled to memory. */ + return; + } + + /* Let unexpected errors propagate. */ + throw; } /* Ambiguous unwind or unambiguous unwind verified as matching. */ diff --git a/gdb/value.c b/gdb/value.c index 97a099ddbd..00d8ded2ae 100644 --- a/gdb/value.c +++ b/gdb/value.c @@ -1412,7 +1412,18 @@ value_optimized_out (struct value *value) } catch (const gdb_exception_error &ex) { - /* Fall back to checking value->optimized_out. */ + switch (ex.error) + { + case MEMORY_ERROR: + case OPTIMIZED_OUT_ERROR: + case NOT_AVAILABLE_ERROR: + /* These can normally happen when we try to access an + optimized out or unavailable register, either in a + physical register or spilled to memory. */ + break; + default: + throw; + } } } -- 2.14.5 ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 1/3] Fix crash if connection drops in scoped_restore_current_thread's ctor, part 1 2020-07-08 23:31 ` [PATCH 1/3] Fix crash if connection drops in scoped_restore_current_thread's ctor, part 1 Pedro Alves @ 2020-07-09 3:17 ` Simon Marchi 2020-07-09 10:51 ` Pedro Alves 0 siblings, 1 reply; 31+ messages in thread From: Simon Marchi @ 2020-07-09 3:17 UTC (permalink / raw) To: Pedro Alves, gdb-patches On 2020-07-08 7:31 p.m., Pedro Alves wrote: > Running the testsuite against an Asan-enabled build of GDB makes > gdb.base/multi-target.exp expose this bug. > > scoped_restore_current_thread's ctor calls get_frame_id to record the > selected frame's ID to restore later. If the frame ID hasn't been > computed yet, it will be computed on the spot, and that will usually > require accessing the target's memory and registers, which requires > remote accesses. If the remote connection closes while we're > computing the frame ID, the remote target exits its inferiors, > unpushes itself, and throws a TARGET_CLOSE_ERROR error. > > If that happens, GDB can currently crash, here: > >> ==18555==ERROR: AddressSanitizer: heap-use-after-free on address 0x621004670aa8 at pc 0x0000007ab125 bp 0x7ffdecaecd20 sp 0x7ffdecaecd10 >> READ of size 4 at 0x621004670aa8 thread T0 >> #0 0x7ab124 in dwarf2_frame_this_id src/binutils-gdb/gdb/dwarf2/frame.c:1228 >> #1 0x983ec5 in compute_frame_id src/binutils-gdb/gdb/frame.c:550 >> #2 0x9841ee in get_frame_id(frame_info*) src/binutils-gdb/gdb/frame.c:582 >> #3 0x1093faa in scoped_restore_current_thread::scoped_restore_current_thread() src/binutils-gdb/gdb/thread.c:1462 >> #4 0xaee5ba in fetch_inferior_event(void*) src/binutils-gdb/gdb/infrun.c:3968 >> #5 0xaa990b in inferior_event_handler(inferior_event_type, void*) src/binutils-gdb/gdb/inf-loop.c:43 >> #6 0xea61b6 in remote_async_serial_handler src/binutils-gdb/gdb/remote.c:14161 >> #7 0xefca8a in run_async_handler_and_reschedule src/binutils-gdb/gdb/ser-base.c:137 >> #8 0xefcd23 in fd_event src/binutils-gdb/gdb/ser-base.c:188 >> #9 0x15a7416 in handle_file_event src/binutils-gdb/gdbsupport/event-loop.cc:548 >> #10 0x15a7c36 in gdb_wait_for_event src/binutils-gdb/gdbsupport/event-loop.cc:673 >> #11 0x15a5dbb in gdb_do_one_event() src/binutils-gdb/gdbsupport/event-loop.cc:215 >> #12 0xbfe62d in start_event_loop src/binutils-gdb/gdb/main.c:356 >> #13 0xbfe935 in captured_command_loop src/binutils-gdb/gdb/main.c:416 >> #14 0xc01d39 in captured_main src/binutils-gdb/gdb/main.c:1253 >> #15 0xc01dc9 in gdb_main(captured_main_args*) src/binutils-gdb/gdb/main.c:1268 >> #16 0x414ddd in main src/binutils-gdb/gdb/gdb.c:32 >> #17 0x7f590110b82f in __libc_start_main ../csu/libc-start.c:291 >> #18 0x414bd8 in _start (build/binutils-gdb/gdb/gdb+0x414bd8) > > What happens is that above, we're in dwarf2_frame_this_id, just after > the dwarf2_frame_cache call. The "cache" variable that the > dwarf2_frame_cache function returned is already stale. It's been > released here, from within the dwarf2_frame_cache: > > (top-gdb) bt > #0 reinit_frame_cache () at src/gdb/frame.c:1855 > #1 0x00000000014ff7b0 in switch_to_no_thread () at src/gdb/thread.c:1301 > #2 0x0000000000f66d3e in switch_to_inferior_no_thread (inf=0x615000338180) at src/gdb/inferior.c:626 > #3 0x00000000012f3826 in remote_unpush_target (target=0x6170000c5900) at src/gdb/remote.c:5521 > #4 0x00000000013097e0 in remote_target::readchar (this=0x6170000c5900, timeout=2) at src/gdb/remote.c:9137 > #5 0x000000000130be4d in remote_target::getpkt_or_notif_sane_1 (this=0x6170000c5900, buf=0x6170000c5918, forever=0, expecting_notif=0, is_notif=0x0) at src/gdb/remote.c:9683 > #6 0x000000000130c8ab in remote_target::getpkt_sane (this=0x6170000c5900, buf=0x6170000c5918, forever=0) at src/gdb/remote.c:9790 > #7 0x000000000130bc0d in remote_target::getpkt (this=0x6170000c5900, buf=0x6170000c5918, forever=0) at src/gdb/remote.c:9623 > #8 0x000000000130838e in remote_target::remote_read_bytes_1 (this=0x6170000c5900, memaddr=0x7fffffffcdc0, myaddr=0x6080000ad3bc "", len_units=64, unit_size=1, xfered_len_units=0x7fff6a29b9a0) at src/gdb/remote.c:8860 > #9 0x0000000001308bd2 in remote_target::remote_read_bytes (this=0x6170000c5900, memaddr=0x7fffffffcdc0, myaddr=0x6080000ad3bc "", len=64, unit_size=1, xfered_len=0x7fff6a29b9a0) at src/gdb/remote.c:8987 > #10 0x0000000001311ed1 in remote_target::xfer_partial (this=0x6170000c5900, object=TARGET_OBJECT_MEMORY, annex=0x0, readbuf=0x6080000ad3bc "", writebuf=0x0, offset=140737488342464, len=64, xfered_len=0x7fff6a29b9a0) at src/gdb/remote.c:10988 > #11 0x00000000014ba969 in raw_memory_xfer_partial (ops=0x6170000c5900, readbuf=0x6080000ad3bc "", writebuf=0x0, memaddr=140737488342464, len=64, xfered_len=0x7fff6a29b9a0) at src/gdb/target.c:918 > #12 0x00000000014bb720 in target_xfer_partial (ops=0x6170000c5900, object=TARGET_OBJECT_RAW_MEMORY, annex=0x0, readbuf=0x6080000ad3bc "", writebuf=0x0, offset=140737488342464, len=64, xfered_len=0x7fff6a29b9a0) at src/gdb/target.c:1148 > #13 0x00000000014bc3b5 in target_read_partial (ops=0x6170000c5900, object=TARGET_OBJECT_RAW_MEMORY, annex=0x0, buf=0x6080000ad3bc "", offset=140737488342464, len=64, xfered_len=0x7fff6a29b9a0) at src/gdb/target.c:1380 > #14 0x00000000014bc593 in target_read (ops=0x6170000c5900, object=TARGET_OBJECT_RAW_MEMORY, annex=0x0, buf=0x6080000ad3bc "", offset=140737488342464, len=64) at src/gdb/target.c:1419 > #15 0x00000000014bbd4d in target_read_raw_memory (memaddr=0x7fffffffcdc0, myaddr=0x6080000ad3bc "", len=64) at src/gdb/target.c:1252 > #16 0x0000000000bf27df in dcache_read_line (dcache=0x6060001eddc0, db=0x6080000ad3a0) at src/gdb/dcache.c:336 > #17 0x0000000000bf2b72 in dcache_peek_byte (dcache=0x6060001eddc0, addr=0x7fffffffcdd8, ptr=0x6020001231b0 "") at src/gdb/dcache.c:403 > #18 0x0000000000bf3103 in dcache_read_memory_partial (ops=0x6170000c5900, dcache=0x6060001eddc0, memaddr=0x7fffffffcdd8, myaddr=0x6020001231b0 "", len=8, xfered_len=0x7fff6a29bf20) at src/gdb/dcache.c:484 > #19 0x00000000014bafe9 in memory_xfer_partial_1 (ops=0x6170000c5900, object=TARGET_OBJECT_STACK_MEMORY, readbuf=0x6020001231b0 "", writebuf=0x0, memaddr=140737488342488, len=8, xfered_len=0x7fff6a29bf20) at src/gdb/target.c:1034 > #20 0x00000000014bb212 in memory_xfer_partial (ops=0x6170000c5900, object=TARGET_OBJECT_STACK_MEMORY, readbuf=0x6020001231b0 "", writebuf=0x0, memaddr=140737488342488, len=8, xfered_len=0x7fff6a29bf20) at src/gdb/target.c:1076 > #21 0x00000000014bb6b3 in target_xfer_partial (ops=0x6170000c5900, object=TARGET_OBJECT_STACK_MEMORY, annex=0x0, readbuf=0x6020001231b0 "", writebuf=0x0, offset=140737488342488, len=8, xfered_len=0x7fff6a29bf20) at src/gdb/target.c:1133 > #22 0x000000000164564d in read_value_memory (val=0x60f000029440, bit_offset=0, stack=1, memaddr=0x7fffffffcdd8, buffer=0x6020001231b0 "", length=8) at src/gdb/valops.c:956 > #23 0x0000000001680fff in value_fetch_lazy_memory (val=0x60f000029440) at src/gdb/value.c:3764 > #24 0x0000000001681efd in value_fetch_lazy (val=0x60f000029440) at src/gdb/value.c:3910 > #25 0x0000000001676143 in value_optimized_out (value=0x60f000029440) at src/gdb/value.c:1411 > #26 0x0000000000e0fcb8 in frame_register_unwind (next_frame=0x6210066bfde0, regnum=16, optimizedp=0x7fff6a29c200, unavailablep=0x7fff6a29c240, lvalp=0x7fff6a29c2c0, addrp=0x7fff6a29c300, realnump=0x7fff6a29c280, bufferp=0x7fff6a29c3a0 "@\304)j\377\177") at src/gdb/frame.c:1144 > #27 0x0000000000e10418 in frame_unwind_register (next_frame=0x6210066bfde0, regnum=16, buf=0x7fff6a29c3a0 "@\304)j\377\177") at src/gdb/frame.c:1196 > #28 0x0000000000f00431 in i386_unwind_pc (gdbarch=0x6210043d0110, next_frame=0x6210066bfde0) at src/gdb/i386-tdep.c:1969 > #29 0x0000000000e39724 in gdbarch_unwind_pc (gdbarch=0x6210043d0110, next_frame=0x6210066bfde0) at src/gdb/gdbarch.c:3056 > #30 0x0000000000c2ea90 in dwarf2_tailcall_sniffer_first (this_frame=0x6210066bfde0, tailcall_cachep=0x6210066bfee0, entry_cfa_sp_offsetp=0x0) at src/gdb/dwarf2/frame-tailcall.c:423 > #31 0x0000000000c36bdb in dwarf2_frame_cache (this_frame=0x6210066bfde0, this_cache=0x6210066bfdf8) at src/gdb/dwarf2/frame.c:1198 > #32 0x0000000000c36eb3 in dwarf2_frame_this_id (this_frame=0x6210066bfde0, this_cache=0x6210066bfdf8, this_id=0x6210066bfe40) at src/gdb/dwarf2/frame.c:1226 > > Note that remote_target::readchar in frame #3 throws > TARGET_CLOSE_ERROR after that remote_unpush_target in frame #3 > returns. > > The problem is that that TARGET_CLOSE_ERROR is swallowed by `that that` Otherwise, LGTM. Simon ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 1/3] Fix crash if connection drops in scoped_restore_current_thread's ctor, part 1 2020-07-09 3:17 ` Simon Marchi @ 2020-07-09 10:51 ` Pedro Alves 2020-07-09 14:13 ` Simon Marchi 0 siblings, 1 reply; 31+ messages in thread From: Pedro Alves @ 2020-07-09 10:51 UTC (permalink / raw) To: Simon Marchi, gdb-patches On 7/9/20 4:17 AM, Simon Marchi wrote: > On 2020-07-08 7:31 p.m., Pedro Alves wrote: >> Running the testsuite against an Asan-enabled build of GDB makes >> gdb.base/multi-target.exp expose this bug. >> >> scoped_restore_current_thread's ctor calls get_frame_id to record the >> selected frame's ID to restore later. If the frame ID hasn't been >> computed yet, it will be computed on the spot, and that will usually >> require accessing the target's memory and registers, which requires >> remote accesses. If the remote connection closes while we're >> computing the frame ID, the remote target exits its inferiors, >> unpushes itself, and throws a TARGET_CLOSE_ERROR error. >> >> If that happens, GDB can currently crash, here: >> >>> ==18555==ERROR: AddressSanitizer: heap-use-after-free on address 0x621004670aa8 at pc 0x0000007ab125 bp 0x7ffdecaecd20 sp 0x7ffdecaecd10 >>> READ of size 4 at 0x621004670aa8 thread T0 >>> #0 0x7ab124 in dwarf2_frame_this_id src/binutils-gdb/gdb/dwarf2/frame.c:1228 >>> #1 0x983ec5 in compute_frame_id src/binutils-gdb/gdb/frame.c:550 >>> #2 0x9841ee in get_frame_id(frame_info*) src/binutils-gdb/gdb/frame.c:582 >>> #3 0x1093faa in scoped_restore_current_thread::scoped_restore_current_thread() src/binutils-gdb/gdb/thread.c:1462 >>> #4 0xaee5ba in fetch_inferior_event(void*) src/binutils-gdb/gdb/infrun.c:3968 >>> #5 0xaa990b in inferior_event_handler(inferior_event_type, void*) src/binutils-gdb/gdb/inf-loop.c:43 >>> #6 0xea61b6 in remote_async_serial_handler src/binutils-gdb/gdb/remote.c:14161 >>> #7 0xefca8a in run_async_handler_and_reschedule src/binutils-gdb/gdb/ser-base.c:137 >>> #8 0xefcd23 in fd_event src/binutils-gdb/gdb/ser-base.c:188 >>> #9 0x15a7416 in handle_file_event src/binutils-gdb/gdbsupport/event-loop.cc:548 >>> #10 0x15a7c36 in gdb_wait_for_event src/binutils-gdb/gdbsupport/event-loop.cc:673 >>> #11 0x15a5dbb in gdb_do_one_event() src/binutils-gdb/gdbsupport/event-loop.cc:215 >>> #12 0xbfe62d in start_event_loop src/binutils-gdb/gdb/main.c:356 >>> #13 0xbfe935 in captured_command_loop src/binutils-gdb/gdb/main.c:416 >>> #14 0xc01d39 in captured_main src/binutils-gdb/gdb/main.c:1253 >>> #15 0xc01dc9 in gdb_main(captured_main_args*) src/binutils-gdb/gdb/main.c:1268 >>> #16 0x414ddd in main src/binutils-gdb/gdb/gdb.c:32 >>> #17 0x7f590110b82f in __libc_start_main ../csu/libc-start.c:291 >>> #18 0x414bd8 in _start (build/binutils-gdb/gdb/gdb+0x414bd8) >> >> What happens is that above, we're in dwarf2_frame_this_id, just after >> the dwarf2_frame_cache call. The "cache" variable that the >> dwarf2_frame_cache function returned is already stale. It's been >> released here, from within the dwarf2_frame_cache: >> >> (top-gdb) bt >> #0 reinit_frame_cache () at src/gdb/frame.c:1855 >> #1 0x00000000014ff7b0 in switch_to_no_thread () at src/gdb/thread.c:1301 >> #2 0x0000000000f66d3e in switch_to_inferior_no_thread (inf=0x615000338180) at src/gdb/inferior.c:626 >> #3 0x00000000012f3826 in remote_unpush_target (target=0x6170000c5900) at src/gdb/remote.c:5521 >> #4 0x00000000013097e0 in remote_target::readchar (this=0x6170000c5900, timeout=2) at src/gdb/remote.c:9137 >> #5 0x000000000130be4d in remote_target::getpkt_or_notif_sane_1 (this=0x6170000c5900, buf=0x6170000c5918, forever=0, expecting_notif=0, is_notif=0x0) at src/gdb/remote.c:9683 >> #6 0x000000000130c8ab in remote_target::getpkt_sane (this=0x6170000c5900, buf=0x6170000c5918, forever=0) at src/gdb/remote.c:9790 >> #7 0x000000000130bc0d in remote_target::getpkt (this=0x6170000c5900, buf=0x6170000c5918, forever=0) at src/gdb/remote.c:9623 >> #8 0x000000000130838e in remote_target::remote_read_bytes_1 (this=0x6170000c5900, memaddr=0x7fffffffcdc0, myaddr=0x6080000ad3bc "", len_units=64, unit_size=1, xfered_len_units=0x7fff6a29b9a0) at src/gdb/remote.c:8860 >> #9 0x0000000001308bd2 in remote_target::remote_read_bytes (this=0x6170000c5900, memaddr=0x7fffffffcdc0, myaddr=0x6080000ad3bc "", len=64, unit_size=1, xfered_len=0x7fff6a29b9a0) at src/gdb/remote.c:8987 >> #10 0x0000000001311ed1 in remote_target::xfer_partial (this=0x6170000c5900, object=TARGET_OBJECT_MEMORY, annex=0x0, readbuf=0x6080000ad3bc "", writebuf=0x0, offset=140737488342464, len=64, xfered_len=0x7fff6a29b9a0) at src/gdb/remote.c:10988 >> #11 0x00000000014ba969 in raw_memory_xfer_partial (ops=0x6170000c5900, readbuf=0x6080000ad3bc "", writebuf=0x0, memaddr=140737488342464, len=64, xfered_len=0x7fff6a29b9a0) at src/gdb/target.c:918 >> #12 0x00000000014bb720 in target_xfer_partial (ops=0x6170000c5900, object=TARGET_OBJECT_RAW_MEMORY, annex=0x0, readbuf=0x6080000ad3bc "", writebuf=0x0, offset=140737488342464, len=64, xfered_len=0x7fff6a29b9a0) at src/gdb/target.c:1148 >> #13 0x00000000014bc3b5 in target_read_partial (ops=0x6170000c5900, object=TARGET_OBJECT_RAW_MEMORY, annex=0x0, buf=0x6080000ad3bc "", offset=140737488342464, len=64, xfered_len=0x7fff6a29b9a0) at src/gdb/target.c:1380 >> #14 0x00000000014bc593 in target_read (ops=0x6170000c5900, object=TARGET_OBJECT_RAW_MEMORY, annex=0x0, buf=0x6080000ad3bc "", offset=140737488342464, len=64) at src/gdb/target.c:1419 >> #15 0x00000000014bbd4d in target_read_raw_memory (memaddr=0x7fffffffcdc0, myaddr=0x6080000ad3bc "", len=64) at src/gdb/target.c:1252 >> #16 0x0000000000bf27df in dcache_read_line (dcache=0x6060001eddc0, db=0x6080000ad3a0) at src/gdb/dcache.c:336 >> #17 0x0000000000bf2b72 in dcache_peek_byte (dcache=0x6060001eddc0, addr=0x7fffffffcdd8, ptr=0x6020001231b0 "") at src/gdb/dcache.c:403 >> #18 0x0000000000bf3103 in dcache_read_memory_partial (ops=0x6170000c5900, dcache=0x6060001eddc0, memaddr=0x7fffffffcdd8, myaddr=0x6020001231b0 "", len=8, xfered_len=0x7fff6a29bf20) at src/gdb/dcache.c:484 >> #19 0x00000000014bafe9 in memory_xfer_partial_1 (ops=0x6170000c5900, object=TARGET_OBJECT_STACK_MEMORY, readbuf=0x6020001231b0 "", writebuf=0x0, memaddr=140737488342488, len=8, xfered_len=0x7fff6a29bf20) at src/gdb/target.c:1034 >> #20 0x00000000014bb212 in memory_xfer_partial (ops=0x6170000c5900, object=TARGET_OBJECT_STACK_MEMORY, readbuf=0x6020001231b0 "", writebuf=0x0, memaddr=140737488342488, len=8, xfered_len=0x7fff6a29bf20) at src/gdb/target.c:1076 >> #21 0x00000000014bb6b3 in target_xfer_partial (ops=0x6170000c5900, object=TARGET_OBJECT_STACK_MEMORY, annex=0x0, readbuf=0x6020001231b0 "", writebuf=0x0, offset=140737488342488, len=8, xfered_len=0x7fff6a29bf20) at src/gdb/target.c:1133 >> #22 0x000000000164564d in read_value_memory (val=0x60f000029440, bit_offset=0, stack=1, memaddr=0x7fffffffcdd8, buffer=0x6020001231b0 "", length=8) at src/gdb/valops.c:956 >> #23 0x0000000001680fff in value_fetch_lazy_memory (val=0x60f000029440) at src/gdb/value.c:3764 >> #24 0x0000000001681efd in value_fetch_lazy (val=0x60f000029440) at src/gdb/value.c:3910 >> #25 0x0000000001676143 in value_optimized_out (value=0x60f000029440) at src/gdb/value.c:1411 >> #26 0x0000000000e0fcb8 in frame_register_unwind (next_frame=0x6210066bfde0, regnum=16, optimizedp=0x7fff6a29c200, unavailablep=0x7fff6a29c240, lvalp=0x7fff6a29c2c0, addrp=0x7fff6a29c300, realnump=0x7fff6a29c280, bufferp=0x7fff6a29c3a0 "@\304)j\377\177") at src/gdb/frame.c:1144 >> #27 0x0000000000e10418 in frame_unwind_register (next_frame=0x6210066bfde0, regnum=16, buf=0x7fff6a29c3a0 "@\304)j\377\177") at src/gdb/frame.c:1196 >> #28 0x0000000000f00431 in i386_unwind_pc (gdbarch=0x6210043d0110, next_frame=0x6210066bfde0) at src/gdb/i386-tdep.c:1969 >> #29 0x0000000000e39724 in gdbarch_unwind_pc (gdbarch=0x6210043d0110, next_frame=0x6210066bfde0) at src/gdb/gdbarch.c:3056 >> #30 0x0000000000c2ea90 in dwarf2_tailcall_sniffer_first (this_frame=0x6210066bfde0, tailcall_cachep=0x6210066bfee0, entry_cfa_sp_offsetp=0x0) at src/gdb/dwarf2/frame-tailcall.c:423 >> #31 0x0000000000c36bdb in dwarf2_frame_cache (this_frame=0x6210066bfde0, this_cache=0x6210066bfdf8) at src/gdb/dwarf2/frame.c:1198 >> #32 0x0000000000c36eb3 in dwarf2_frame_this_id (this_frame=0x6210066bfde0, this_cache=0x6210066bfdf8, this_id=0x6210066bfe40) at src/gdb/dwarf2/frame.c:1226 >> >> Note that remote_target::readchar in frame #3 throws >> TARGET_CLOSE_ERROR after that remote_unpush_target in frame #3 >> returns. >> >> The problem is that that TARGET_CLOSE_ERROR is swallowed by > > `that that` > That wasn't a typo, compare with: The problem is that this TARGET_CLOSE_ERROR The problem is that those TARGET_CLOSE_ERRORs The problem is that these TARGET_CLOSE_ERRORs https://english.stackexchange.com/questions/3418/how-do-you-handle-that-that-the-double-that-problem I'll say "that the" instead to avoid confusion. > Otherwise, LGTM. > > Simon ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 1/3] Fix crash if connection drops in scoped_restore_current_thread's ctor, part 1 2020-07-09 10:51 ` Pedro Alves @ 2020-07-09 14:13 ` Simon Marchi 0 siblings, 0 replies; 31+ messages in thread From: Simon Marchi @ 2020-07-09 14:13 UTC (permalink / raw) To: Pedro Alves, gdb-patches On 2020-07-09 6:51 a.m., Pedro Alves wrote: >> `that that` >> > > That wasn't a typo, compare with: > > The problem is that this TARGET_CLOSE_ERROR > The problem is that those TARGET_CLOSE_ERRORs > The problem is that these TARGET_CLOSE_ERRORs > > https://english.stackexchange.com/questions/3418/how-do-you-handle-that-that-the-double-that-problem > > I'll say "that the" instead to avoid confusion. Haha, I see now. Yeah I think any alternative to `that that` is clearer. Simon ^ permalink raw reply [flat|nested] 31+ messages in thread
* [PATCH 2/3] Fix crash if connection drops in scoped_restore_current_thread's ctor, part 2 2020-07-08 23:31 [PATCH 0/3] Fix crash if connection drops in scoped_restore_current_thread's ctor Pedro Alves 2020-07-08 23:31 ` [PATCH 1/3] Fix crash if connection drops in scoped_restore_current_thread's ctor, part 1 Pedro Alves @ 2020-07-08 23:31 ` Pedro Alves 2020-07-09 3:31 ` Simon Marchi 2020-07-08 23:31 ` [PATCH 3/3] Make scoped_restore_current_thread's cdtors exception free (RFC) Pedro Alves 2020-07-10 23:02 ` [PATCH 0/3] Fix crash if connection drops in scoped_restore_current_thread's ctor Pedro Alves 3 siblings, 1 reply; 31+ messages in thread From: Pedro Alves @ 2020-07-08 23:31 UTC (permalink / raw) To: gdb-patches Running the testsuite against an Asan-enabled build of GDB makes gdb.base/multi-target.exp expose this bug. scoped_restore_current_thread's ctor calls get_frame_id to record the selected frame's ID to restore later. If the frame ID hasn't been computed yet, it will be computed on the spot, and that will usually require accessing the target's memory and registers. If the remote connection closes, while we're computing the frame ID, the remote target exits its inferiors, unpushes itself, and throws a TARGET_CLOSE_ERROR error. Exiting the inferiors deletes the inferior's threads. scoped_restore_current_thread increments the current thread's refcount to prevent the thread from being deleted from under its feet. However, the code that does that isn't considering the case of the thread being deleted from within get_frame_id. It only increments the refcount _after_ get_frame_id returns. So if the current thread is indeed deleted, the tp->incref (); statement references a stale TP pointer. Incrementing the refcounts earlier fixes it. We should probably also let the TARGET_CLOSE_ERROR error propagate in this case. That alone would fix it, though it seems better to tweak the refcount handling too. gdb/ChangeLog: * thread.c (scoped_restore_current_thread::scoped_restore_current_thread): Incref the thread before calling get_frame_id instead of after. Let TARGET_CLOSE_ERROR propagate. --- gdb/thread.c | 19 +++++++++++-------- 1 file changed, 11 insertions(+), 8 deletions(-) diff --git a/gdb/thread.c b/gdb/thread.c index f0722d3588..1ec047e35b 100644 --- a/gdb/thread.c +++ b/gdb/thread.c @@ -1433,15 +1433,17 @@ scoped_restore_current_thread::~scoped_restore_current_thread () scoped_restore_current_thread::scoped_restore_current_thread () { - m_thread = NULL; m_inf = current_inferior (); + m_inf->incref (); if (inferior_ptid != null_ptid) { - thread_info *tp = inferior_thread (); + m_thread = inferior_thread (); + m_thread->incref (); + struct frame_info *frame; - m_was_stopped = tp->state == THREAD_STOPPED; + m_was_stopped = m_thread->state == THREAD_STOPPED; if (m_was_stopped && target_has_registers && target_has_stack @@ -1466,13 +1468,14 @@ scoped_restore_current_thread::scoped_restore_current_thread () { m_selected_frame_id = null_frame_id; m_selected_frame_level = -1; - } - tp->incref (); - m_thread = tp; + /* Better let this propagate. */ + if (ex.error == TARGET_CLOSE_ERROR) + throw; + } } - - m_inf->incref (); + else + m_thread = NULL; } /* See gdbthread.h. */ -- 2.14.5 ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 2/3] Fix crash if connection drops in scoped_restore_current_thread's ctor, part 2 2020-07-08 23:31 ` [PATCH 2/3] Fix crash if connection drops in scoped_restore_current_thread's ctor, part 2 Pedro Alves @ 2020-07-09 3:31 ` Simon Marchi 2020-07-09 11:12 ` Pedro Alves 0 siblings, 1 reply; 31+ messages in thread From: Simon Marchi @ 2020-07-09 3:31 UTC (permalink / raw) To: Pedro Alves, gdb-patches On 2020-07-08 7:31 p.m., Pedro Alves wrote: > Running the testsuite against an Asan-enabled build of GDB makes > gdb.base/multi-target.exp expose this bug. > > scoped_restore_current_thread's ctor calls get_frame_id to record the > selected frame's ID to restore later. If the frame ID hasn't been > computed yet, it will be computed on the spot, and that will usually > require accessing the target's memory and registers. If the remote > connection closes, while we're computing the frame ID, the remote > target exits its inferiors, unpushes itself, and throws a > TARGET_CLOSE_ERROR error. Exiting the inferiors deletes the > inferior's threads. > > scoped_restore_current_thread increments the current thread's refcount > to prevent the thread from being deleted from under its feet. > However, the code that does that isn't considering the case of the > thread being deleted from within get_frame_id. It only increments the > refcount _after_ get_frame_id returns. So if the current thread is > indeed deleted, the > > tp->incref (); > > statement references a stale TP pointer. > > Incrementing the refcounts earlier fixes it. > > We should probably also let the TARGET_CLOSE_ERROR error propagate in > this case. That alone would fix it, though it seems better to tweak > the refcount handling too. So, when the target closes while we (scoped_restore_current_thread) own a reference on the inferior and thread, the inferior and thread are still destroyed, and so we shouldn't decref them? Simon ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 2/3] Fix crash if connection drops in scoped_restore_current_thread's ctor, part 2 2020-07-09 3:31 ` Simon Marchi @ 2020-07-09 11:12 ` Pedro Alves 2020-07-09 14:16 ` Simon Marchi 0 siblings, 1 reply; 31+ messages in thread From: Pedro Alves @ 2020-07-09 11:12 UTC (permalink / raw) To: Simon Marchi, gdb-patches On 7/9/20 4:31 AM, Simon Marchi wrote: > On 2020-07-08 7:31 p.m., Pedro Alves wrote: >> Running the testsuite against an Asan-enabled build of GDB makes >> gdb.base/multi-target.exp expose this bug. >> >> scoped_restore_current_thread's ctor calls get_frame_id to record the >> selected frame's ID to restore later. If the frame ID hasn't been >> computed yet, it will be computed on the spot, and that will usually >> require accessing the target's memory and registers. If the remote >> connection closes, while we're computing the frame ID, the remote >> target exits its inferiors, unpushes itself, and throws a >> TARGET_CLOSE_ERROR error. Exiting the inferiors deletes the >> inferior's threads. >> >> scoped_restore_current_thread increments the current thread's refcount >> to prevent the thread from being deleted from under its feet. >> However, the code that does that isn't considering the case of the >> thread being deleted from within get_frame_id. It only increments the >> refcount _after_ get_frame_id returns. So if the current thread is >> indeed deleted, the >> >> tp->incref (); >> >> statement references a stale TP pointer. >> >> Incrementing the refcounts earlier fixes it. >> >> We should probably also let the TARGET_CLOSE_ERROR error propagate in >> this case. That alone would fix it, though it seems better to tweak >> the refcount handling too. > > So, when the target closes while we (scoped_restore_current_thread) own > a reference on the inferior and thread, the inferior and thread are still > destroyed, and so we shouldn't decref them? Aw, no, I got confused and misremembered how exceptions in ctors work. The dtor for scoped_restore_current_thread isn't called, and I assumed it was called. We need to decref the inferior and thread before letting the exception propagate, otherwise we leak them. I'm testing the updated version of the patch below, which does that: /* Better let this propagate. */ if (ex.error == TARGET_CLOSE_ERROR) { m_thread->decref (); m_inf->decref (); throw; } It passes multi-target.exp with Asan-enabled GDB. Running the full testsuite now. From 1ad36a4b892fc4425d6f24c298713eeafece7b04 Mon Sep 17 00:00:00 2001 From: Pedro Alves <pedro@palves.net> Date: Tue, 7 Jul 2020 01:50:10 +0100 Subject: [PATCH] Fix crash if connection drops in scoped_restore_current_thread's ctor, part 2 Running the testsuite against an Asan-enabled build of GDB makes gdb.base/multi-target.exp expose this bug. scoped_restore_current_thread's ctor calls get_frame_id to record the selected frame's ID to restore later. If the frame ID hasn't been computed yet, it will be computed on the spot, and that will usually require accessing the target's memory and registers. If the remote connection closes, while we're computing the frame ID, the remote target exits its inferiors, unpushes itself, and throws a TARGET_CLOSE_ERROR error. Exiting the inferiors deletes the inferior's threads. scoped_restore_current_thread increments the current thread's refcount to prevent the thread from being deleted from under its feet. However, the code that does that isn't considering the case of the thread being deleted from within get_frame_id. It only increments the refcount _after_ get_frame_id returns. So if the current thread is indeed deleted, the tp->incref (); statement references a stale TP pointer. Incrementing the refcounts earlier fixes it. We should probably also let the TARGET_CLOSE_ERROR error propagate in this case. That alone would fix it, though it seems better to tweak the refcount handling too. gdb/ChangeLog: * thread.c (scoped_restore_current_thread::scoped_restore_current_thread): Incref the thread before calling get_frame_id instead of after. Let TARGET_CLOSE_ERROR propagate. --- gdb/thread.c | 23 +++++++++++++++-------- 1 file changed, 15 insertions(+), 8 deletions(-) diff --git a/gdb/thread.c b/gdb/thread.c index f0722d3588..a3c2be7dd0 100644 --- a/gdb/thread.c +++ b/gdb/thread.c @@ -1433,15 +1433,17 @@ scoped_restore_current_thread::~scoped_restore_current_thread () scoped_restore_current_thread::scoped_restore_current_thread () { - m_thread = NULL; m_inf = current_inferior (); + m_inf->incref (); if (inferior_ptid != null_ptid) { - thread_info *tp = inferior_thread (); + m_thread = inferior_thread (); + m_thread->incref (); + struct frame_info *frame; - m_was_stopped = tp->state == THREAD_STOPPED; + m_was_stopped = m_thread->state == THREAD_STOPPED; if (m_was_stopped && target_has_registers && target_has_stack @@ -1466,13 +1468,18 @@ scoped_restore_current_thread::scoped_restore_current_thread () { m_selected_frame_id = null_frame_id; m_selected_frame_level = -1; - } - tp->incref (); - m_thread = tp; + /* Better let this propagate. */ + if (ex.error == TARGET_CLOSE_ERROR) + { + m_thread->decref (); + m_inf->decref (); + throw; + } + } } - - m_inf->incref (); + else + m_thread = NULL; } /* See gdbthread.h. */ base-commit: ad8464f799a4c96c7ab8bdfec3f95846cf54f9b0 prerequisite-patch-id: 32ffdda7d7d774bc4df88bf848bcb796559b53ce prerequisite-patch-id: 02021b74355b70debd344a6e445285c67dfef7d6 prerequisite-patch-id: c87fcf5a54f6805967cbf8ab107606c57d9ecf52 prerequisite-patch-id: ac7dee583d0ffa519c9d1cd89d27664bca68d8c1 prerequisite-patch-id: eac59ae2ea85d2d51e5be1b03e88a5641cc12c22 prerequisite-patch-id: 13da42ad04dc8e2e3bd6a556a0be0e17cf23669b prerequisite-patch-id: fd3f09fdb58ddc1c595ea014716851f4c8fca48c prerequisite-patch-id: 8230262cb24020a5b37e915843180e940807ba1f -- 2.14.5 ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 2/3] Fix crash if connection drops in scoped_restore_current_thread's ctor, part 2 2020-07-09 11:12 ` Pedro Alves @ 2020-07-09 14:16 ` Simon Marchi 2020-07-09 17:23 ` Pedro Alves 0 siblings, 1 reply; 31+ messages in thread From: Simon Marchi @ 2020-07-09 14:16 UTC (permalink / raw) To: Pedro Alves, gdb-patches On 2020-07-09 7:12 a.m., Pedro Alves wrote: > Aw, no, I got confused and misremembered how exceptions in ctors work. > The dtor for scoped_restore_current_thread isn't called, and I assumed > it was called. We need to decref the inferior and thread before letting > the exception propagate, otherwise we leak them. I'm testing the updated > version of the patch below, which does that: > > /* Better let this propagate. */ > if (ex.error == TARGET_CLOSE_ERROR) > { > m_thread->decref (); > m_inf->decref (); > throw; > } > > It passes multi-target.exp with Asan-enabled GDB. Running the full > testsuite now. Ok, I also had the reasoning about exceptions thrown by constructors, and thought it might be on purpose that it wasn't decref'ed. If the references were kept by fields of scoped_restore_current_thread, then it would work automatically, because their destructor would get called. Simon ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 2/3] Fix crash if connection drops in scoped_restore_current_thread's ctor, part 2 2020-07-09 14:16 ` Simon Marchi @ 2020-07-09 17:23 ` Pedro Alves 2020-07-09 17:28 ` Simon Marchi 0 siblings, 1 reply; 31+ messages in thread From: Pedro Alves @ 2020-07-09 17:23 UTC (permalink / raw) To: Simon Marchi, gdb-patches On 7/9/20 3:16 PM, Simon Marchi wrote: > On 2020-07-09 7:12 a.m., Pedro Alves wrote: >> Aw, no, I got confused and misremembered how exceptions in ctors work. >> The dtor for scoped_restore_current_thread isn't called, and I assumed >> it was called. We need to decref the inferior and thread before letting >> the exception propagate, otherwise we leak them. I'm testing the updated >> version of the patch below, which does that: >> >> /* Better let this propagate. */ >> if (ex.error == TARGET_CLOSE_ERROR) >> { >> m_thread->decref (); >> m_inf->decref (); >> throw; >> } >> >> It passes multi-target.exp with Asan-enabled GDB. Running the full >> testsuite now. > > Ok, I also had the reasoning about exceptions thrown by constructors, > and thought it might be on purpose that it wasn't decref'ed. > > If the references were kept by fields of scoped_restore_current_thread, > then it would work automatically, because their destructor would get > called. Yup. I wasn't proposing that due to a header dependency issue which I wasn't looking at addressing right now, but here's a version that works around it. From 55813e06b771787d3120f4a57f6e4f8ae25e5ebb Mon Sep 17 00:00:00 2001 From: Pedro Alves <pedro@palves.net> Date: Thu, 9 Jul 2020 18:14:09 +0100 Subject: [PATCH] Fix crash if connection drops in scoped_restore_current_thread's ctor, part 2 Running the testsuite against an Asan-enabled build of GDB makes gdb.base/multi-target.exp expose this bug. scoped_restore_current_thread's ctor calls get_frame_id to record the selected frame's ID to restore later. If the frame ID hasn't been computed yet, it will be computed on the spot, and that will usually require accessing the target's memory and registers. If the remote connection closes, while we're computing the frame ID, the remote target exits its inferiors, unpushes itself, and throws a TARGET_CLOSE_ERROR error. Exiting the inferiors deletes the inferior's threads. scoped_restore_current_thread increments the current thread's refcount to prevent the thread from being deleted from under its feet. However, the code that does that isn't considering the case of the thread being deleted from within get_frame_id. It only increments the refcount _after_ get_frame_id returns. So if the current thread is indeed deleted, the tp->incref (); statement references a stale TP pointer. Incrementing the refcounts earlier fixes it. We should probably also let the TARGET_CLOSE_ERROR error propagate in this case. That alone would fix it, though it seems better to tweak the refcount handling too. And to avoid having to manually decref before throwing, convert to use gdb::ref_ptr. Unfortunately, we can't define inferior_ref in inferior.h and then use it in scoped_restore_current_thread, because scoped_restore_current_thread is defined before inferior is (inferior.h includes gdbthread.h). To break that dependency, we would have to move scoped_restore_current_thread to its own header. I'm not doing that here. gdb/ChangeLog: * gdbthread.h (inferior_ref): Define. (scoped_restore_current_thread) <m_thread>: Now a thread_info_ref. (scoped_restore_current_thread) <m_inf>: Now an inferior_ref. * thread.c (scoped_restore_current_thread::restore): Adjust to gdb::ref_ptr. (scoped_restore_current_thread::~scoped_restore_current_thread): Remove manual decref handling. (scoped_restore_current_thread::scoped_restore_current_thread): Adjust to use inferior_ref::new_reference/thread_info_ref::new_reference. Incref the thread before calling get_frame_id instead of after. Let TARGET_CLOSE_ERROR propagate. --- gdb/gdbthread.h | 14 ++++++++++---- gdb/thread.c | 25 ++++++++++--------------- 2 files changed, 20 insertions(+), 19 deletions(-) diff --git a/gdb/gdbthread.h b/gdb/gdbthread.h index 0166b2000f..ab5771fdb4 100644 --- a/gdb/gdbthread.h +++ b/gdb/gdbthread.h @@ -395,6 +395,13 @@ class thread_info : public refcounted_object using thread_info_ref = gdb::ref_ptr<struct thread_info, refcounted_object_ref_policy>; +/* A gdb::ref_ptr pointer to an inferior. This would ideally be in + inferior.h, but it can't due to header dependencies (inferior.h + includes gdbthread.h). */ + +using inferior_ref + = gdb::ref_ptr<struct inferior, refcounted_object_ref_policy>; + /* Create an empty thread list, or empty the existing one. */ extern void init_thread_list (void); @@ -660,10 +667,9 @@ class scoped_restore_current_thread void restore (); bool m_dont_restore = false; - /* Use the "class" keyword here, because of a clash with a "thread_info" - function in the Darwin API. */ - class thread_info *m_thread; - inferior *m_inf; + thread_info_ref m_thread; + inferior_ref m_inf; + frame_id m_selected_frame_id; int m_selected_frame_level; bool m_was_stopped; diff --git a/gdb/thread.c b/gdb/thread.c index f0722d3588..4dce1ef82a 100644 --- a/gdb/thread.c +++ b/gdb/thread.c @@ -1396,9 +1396,9 @@ scoped_restore_current_thread::restore () in the mean time exited (or killed, detached, etc.), then don't revert back to it, but instead simply drop back to no thread selected. */ && m_inf->pid != 0) - switch_to_thread (m_thread); + switch_to_thread (m_thread.get ()); else - switch_to_inferior_no_thread (m_inf); + switch_to_inferior_no_thread (m_inf.get ()); /* The running state of the originally selected thread may have changed, so we have to recheck it here. */ @@ -1425,23 +1425,19 @@ scoped_restore_current_thread::~scoped_restore_current_thread () but swallow the exception. */ } } - - if (m_thread != NULL) - m_thread->decref (); - m_inf->decref (); } scoped_restore_current_thread::scoped_restore_current_thread () { - m_thread = NULL; - m_inf = current_inferior (); + m_inf = inferior_ref::new_reference (current_inferior ()); if (inferior_ptid != null_ptid) { - thread_info *tp = inferior_thread (); + m_thread = thread_info_ref::new_reference (inferior_thread ()); + struct frame_info *frame; - m_was_stopped = tp->state == THREAD_STOPPED; + m_was_stopped = m_thread->state == THREAD_STOPPED; if (m_was_stopped && target_has_registers && target_has_stack @@ -1466,13 +1462,12 @@ scoped_restore_current_thread::scoped_restore_current_thread () { m_selected_frame_id = null_frame_id; m_selected_frame_level = -1; - } - tp->incref (); - m_thread = tp; + /* Better let this propagate. */ + if (ex.error == TARGET_CLOSE_ERROR) + throw; + } } - - m_inf->incref (); } /* See gdbthread.h. */ base-commit: ad8464f799a4c96c7ab8bdfec3f95846cf54f9b0 prerequisite-patch-id: 32ffdda7d7d774bc4df88bf848bcb796559b53ce prerequisite-patch-id: 02021b74355b70debd344a6e445285c67dfef7d6 prerequisite-patch-id: c87fcf5a54f6805967cbf8ab107606c57d9ecf52 prerequisite-patch-id: ac7dee583d0ffa519c9d1cd89d27664bca68d8c1 prerequisite-patch-id: eac59ae2ea85d2d51e5be1b03e88a5641cc12c22 prerequisite-patch-id: 13da42ad04dc8e2e3bd6a556a0be0e17cf23669b prerequisite-patch-id: fd3f09fdb58ddc1c595ea014716851f4c8fca48c prerequisite-patch-id: 8230262cb24020a5b37e915843180e940807ba1f -- 2.14.5 ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 2/3] Fix crash if connection drops in scoped_restore_current_thread's ctor, part 2 2020-07-09 17:23 ` Pedro Alves @ 2020-07-09 17:28 ` Simon Marchi 0 siblings, 0 replies; 31+ messages in thread From: Simon Marchi @ 2020-07-09 17:28 UTC (permalink / raw) To: Pedro Alves, gdb-patches On 2020-07-09 1:23 p.m., Pedro Alves wrote: > Unfortunately, we can't define inferior_ref in inferior.h and then use > it in scoped_restore_current_thread, because > scoped_restore_current_thread is defined before inferior is > (inferior.h includes gdbthread.h). To break that dependency, we would > have to move scoped_restore_current_thread to its own header. I'm not > doing that here. Cool, that's fine with me. Simon ^ permalink raw reply [flat|nested] 31+ messages in thread
* [PATCH 3/3] Make scoped_restore_current_thread's cdtors exception free (RFC) 2020-07-08 23:31 [PATCH 0/3] Fix crash if connection drops in scoped_restore_current_thread's ctor Pedro Alves 2020-07-08 23:31 ` [PATCH 1/3] Fix crash if connection drops in scoped_restore_current_thread's ctor, part 1 Pedro Alves 2020-07-08 23:31 ` [PATCH 2/3] Fix crash if connection drops in scoped_restore_current_thread's ctor, part 2 Pedro Alves @ 2020-07-08 23:31 ` Pedro Alves 2020-07-09 3:49 ` Simon Marchi 2020-07-10 23:02 ` [PATCH 0/3] Fix crash if connection drops in scoped_restore_current_thread's ctor Pedro Alves 3 siblings, 1 reply; 31+ messages in thread From: Pedro Alves @ 2020-07-08 23:31 UTC (permalink / raw) To: gdb-patches If the remote target closes while we're reading registers/memory for restoring the selected frame in scoped_restore_current_thread's dtor, the corresponding TARGET_CLOSE_ERROR error is swallowed by the scoped_restore_current_thread's dtor, because letting exceptions escape from a dtor is bad. It isn't great to lose that errors like that, though. I've been thinking about how to avoid it, and I came up with this patch. The idea here is to make scoped_restore_current_thread's dtor do as little as possible, to avoid any work that might throw in the first place. And to do that, instead of having the dtor call restore_selected_frame, which re-finds the previously selected frame, just record the frame_id/level of the desired selected frame, and have get_selected_frame find the frame the next time it is called. In effect, this implements most of Cagney's suggestion, here: /* On demand, create the selected frame and then return it. If the selected frame can not be created, this function prints then throws an error. When MESSAGE is non-NULL, use it for the error message, otherwize use a generic error message. */ /* FIXME: cagney/2002-11-28: At present, when there is no selected frame, this function always returns the current (inner most) frame. It should instead, when a thread has previously had its frame selected (but not resumed) and the frame cache invalidated, find and then return that thread's previously selected frame. */ extern struct frame_info *get_selected_frame (const char *message); The only thing missing to fully implement that would be to make reinit_frame_cache just clear selected_frame instead of calling select_frame(NULL), and the call select_frame(NULL) explicitly in the places where we really wanted reinit_frame_cache to go back to the current frame too. That can done separately, though, I'm not proposing to do that in this patch. restore_selected_frame should really move from thread.c to frame.c, but I didn't do that here, just to avoid churn in the patch while it collects comments. I will do that as a preparatory patch if people agree with this approach. Incidentally, this patch alone would fix the crashes fixed by the previous patches in the series, because with this, scoped_restore_current_thread's constructor doesn't throw either. --- gdb/blockframe.c | 6 +---- gdb/frame.c | 73 ++++++++++++++++++++++++++++++++++++++++---------------- gdb/frame.h | 22 +++++++++++++---- gdb/gdbthread.h | 4 ++++ gdb/stack.c | 9 ++++--- gdb/thread.c | 67 ++++++++++++++++----------------------------------- 6 files changed, 98 insertions(+), 83 deletions(-) diff --git a/gdb/blockframe.c b/gdb/blockframe.c index 05c26bc2c2..706b6db92c 100644 --- a/gdb/blockframe.c +++ b/gdb/blockframe.c @@ -448,14 +448,10 @@ find_gnu_ifunc_target_type (CORE_ADDR resolver_funaddr) struct frame_info * block_innermost_frame (const struct block *block) { - struct frame_info *frame; - if (block == NULL) return NULL; - frame = get_selected_frame_if_set (); - if (frame == NULL) - frame = get_current_frame (); + frame_info *frame = get_selected_frame (NULL); while (frame != NULL) { const struct block *frame_block = get_frame_block (frame, NULL); diff --git a/gdb/frame.c b/gdb/frame.c index ff27b9f00e..e7dffd204b 100644 --- a/gdb/frame.c +++ b/gdb/frame.c @@ -297,17 +297,15 @@ frame_stash_invalidate (void) /* See frame.h */ scoped_restore_selected_frame::scoped_restore_selected_frame () { - m_fid = get_frame_id (get_selected_frame (NULL)); + get_selected_frame_info (&m_fid, &m_level); } /* See frame.h */ scoped_restore_selected_frame::~scoped_restore_selected_frame () { - frame_info *frame = frame_find_by_id (m_fid); - if (frame == NULL) - warning (_("Unable to restore previously selected frame.")); - else - select_frame (frame); + /* Use the lazy variant because we don't want to do any work here + that might throw, since we're in a dtor. */ + select_frame_lazy (m_fid, m_level); } /* Flag to control debugging. */ @@ -1641,9 +1639,24 @@ get_current_frame (void) } /* The "selected" stack frame is used by default for local and arg - access. May be zero, for no selected frame. */ - + access. */ + +/* If SELECTED_FRAME is NULL, then the selected frame is found in + SELECTED_FRAME_ID / SELECTED_FRAME_LEVEL. Those are used by + get_selected_frame to re-find the selected frame. If + SELECTED_FRAME_ID / SELECTED_FRAME_LEVEL are null_ptid / -1, there + is no selected frame, in which case the next time + get_selected_frame is called, it selects the current frame. */ static struct frame_info *selected_frame; +static frame_id selected_frame_id = null_frame_id; +static int selected_frame_level = -1; + +void +get_selected_frame_info (frame_id *frame_id, int *level) +{ + *frame_id = selected_frame_id; + *level = selected_frame_level; +} int has_stack_frames (void) @@ -1671,6 +1684,8 @@ has_stack_frames (void) return 1; } +void restore_selected_frame (struct frame_id a_frame_id, int frame_level); + /* Return the selected frame. Always non-NULL (unless there isn't an inferior sufficient for creating a frame) in which case an error is thrown. */ @@ -1682,24 +1697,14 @@ get_selected_frame (const char *message) { if (message != NULL && !has_stack_frames ()) error (("%s"), message); - /* Hey! Don't trust this. It should really be re-finding the - last selected frame of the currently selected thread. This, - though, is better than nothing. */ - select_frame (get_current_frame ()); + + restore_selected_frame (selected_frame_id, selected_frame_level); } /* There is always a frame. */ gdb_assert (selected_frame != NULL); return selected_frame; } -/* If there is a selected frame, return it. Otherwise, return NULL. */ - -struct frame_info * -get_selected_frame_if_set (void) -{ - return selected_frame; -} - /* This is a variant of get_selected_frame() which can be called when the inferior does not have a frame; in that case it will return NULL instead of calling error(). */ @@ -1712,12 +1717,26 @@ deprecated_safe_get_selected_frame (void) return get_selected_frame (NULL); } -/* Select frame FI (or NULL - to invalidate the current frame). */ +/* Select frame FI (or NULL - to invalidate the selected frame). */ void select_frame (struct frame_info *fi) { selected_frame = fi; + selected_frame_level = frame_relative_level (fi); + if (selected_frame_level == 0) + { + /* Treat the current frame especially -- we want to always + save/restore it without warning, even if the frame ID changes + (see restore_selected_frame). Also get_frame_id may access + the target's registers/memory, and thus skipping get_frame_id + optimizes the common case. */ + selected_frame_level = -1; + selected_frame_id = null_frame_id; + } + else + selected_frame_id = get_frame_id (fi); + /* NOTE: cagney/2002-05-04: FI can be NULL. This occurs when the frame is being invalidated. */ @@ -1756,6 +1775,18 @@ select_frame (struct frame_info *fi) } } +void +select_frame_lazy (frame_id a_frame_id, int frame_level) +{ + /* get_selected_frame_info never returns level == 0, so we shouldn't + see it here either. */ + gdb_assert (frame_level != 0); + + selected_frame = nullptr; + selected_frame_id = a_frame_id; + selected_frame_level = frame_level; +} + /* Create an arbitrary (i.e. address specified by user) or innermost frame. Always returns a non-NULL value. */ diff --git a/gdb/frame.h b/gdb/frame.h index e835d49f9c..48222c69d3 100644 --- a/gdb/frame.h +++ b/gdb/frame.h @@ -181,8 +181,9 @@ class scoped_restore_selected_frame private: - /* The ID of the previously selected frame. */ + /* The ID and level of the previously selected frame. */ struct frame_id m_fid; + int m_level; }; /* Methods for constructing and comparing Frame IDs. */ @@ -329,13 +330,24 @@ extern void reinit_frame_cache (void); and then return that thread's previously selected frame. */ extern struct frame_info *get_selected_frame (const char *message); -/* If there is a selected frame, return it. Otherwise, return NULL. */ -extern struct frame_info *get_selected_frame_if_set (void); +/* Return frame ID and frame level of the selected frame. If there's + no selected frame or the selected frame is the current frame, + return null_frame_id/-1. This is preferred over getting the same + info out of get_selected_frame directly because this function does + not create the selected-frame's frame_info object if it hasn't been + created yet. */ +extern void get_selected_frame_info (frame_id *frame_id, int *level); -/* Select a specific frame. NULL, apparently implies re-select the - inner most frame. */ +/* Select a specific frame. NULL implies re-select the inner most + frame. */ extern void select_frame (struct frame_info *); +/* Setup frame A_FRAME_ID, with level FRAME_LEVEL as the selected + frame, but don't actually try to find it right now. The next call + to get_selected_frame will look it up (and cache the result). + null_frame_id/-1 implies re-select the inner most frame. */ +extern void select_frame_lazy (frame_id a_frame_id, int frame_level); + /* Given a FRAME, return the next (more inner, younger) or previous (more outer, older) frame. */ extern struct frame_info *get_prev_frame (struct frame_info *); diff --git a/gdb/gdbthread.h b/gdb/gdbthread.h index 0166b2000f..edfdf98b3d 100644 --- a/gdb/gdbthread.h +++ b/gdb/gdbthread.h @@ -667,6 +667,10 @@ class scoped_restore_current_thread frame_id m_selected_frame_id; int m_selected_frame_level; bool m_was_stopped; + /* Save/restore the language as well, because selecting a frame + changes the current language to the frame's language if "set + language auto". */ + enum language m_lang; }; /* Returns a pointer into the thread_info corresponding to diff --git a/gdb/stack.c b/gdb/stack.c index 265e764dc2..93de451a12 100644 --- a/gdb/stack.c +++ b/gdb/stack.c @@ -1836,9 +1836,9 @@ trailing_outermost_frame (int count) static void select_frame_command_core (struct frame_info *fi, bool ignored) { - struct frame_info *prev_frame = get_selected_frame_if_set (); + frame_info *prev_frame = get_selected_frame (NULL); select_frame (fi); - if (get_selected_frame_if_set () != prev_frame) + if (get_selected_frame (NULL) != prev_frame) gdb::observers::user_selected_context_changed.notify (USER_SELECTED_FRAME); } @@ -1857,10 +1857,9 @@ select_frame_for_mi (struct frame_info *fi) static void frame_command_core (struct frame_info *fi, bool ignored) { - struct frame_info *prev_frame = get_selected_frame_if_set (); - + frame_info *prev_frame = get_selected_frame (nullptr); select_frame (fi); - if (get_selected_frame_if_set () != prev_frame) + if (get_selected_frame (nullptr) != prev_frame) gdb::observers::user_selected_context_changed.notify (USER_SELECTED_FRAME); else print_selected_thread_frame (current_uiout, USER_SELECTED_FRAME); diff --git a/gdb/thread.c b/gdb/thread.c index 1ec047e35b..8a5634eae3 100644 --- a/gdb/thread.c +++ b/gdb/thread.c @@ -1325,20 +1325,25 @@ switch_to_thread (process_stratum_target *proc_target, ptid_t ptid) switch_to_thread (thr); } -static void +void restore_selected_frame (struct frame_id a_frame_id, int frame_level); + +void restore_selected_frame (struct frame_id a_frame_id, int frame_level) { struct frame_info *frame = NULL; int count; - /* This means there was no selected frame. */ + /* This either means there was no selected frame, or the selected + frame was the inner most (the current frame). */ if (frame_level == -1) { - select_frame (NULL); + select_frame (get_current_frame ()); return; } - gdb_assert (frame_level >= 0); + /* select_frame never saves 0 in selected_frame_level, so we + shouldn't see it here. */ + gdb_assert (frame_level > 0); /* Restore by level first, check if the frame id is the same as expected. If that fails, try restoring by frame id. If that @@ -1408,23 +1413,19 @@ scoped_restore_current_thread::restore () && target_has_registers && target_has_stack && target_has_memory) - restore_selected_frame (m_selected_frame_id, m_selected_frame_level); + { + /* Use the lazy variant because we don't want to do any work + here that might throw, since we're in a dtor. */ + select_frame_lazy (m_selected_frame_id, m_selected_frame_level); + } + + set_language (m_lang); } scoped_restore_current_thread::~scoped_restore_current_thread () { if (!m_dont_restore) - { - try - { - restore (); - } - catch (const gdb_exception &ex) - { - /* We're in a dtor, there's really nothing else we can do - but swallow the exception. */ - } - } + restore (); if (m_thread != NULL) m_thread->decref (); @@ -1436,43 +1437,15 @@ scoped_restore_current_thread::scoped_restore_current_thread () m_inf = current_inferior (); m_inf->incref (); + m_lang = current_language->la_language; + if (inferior_ptid != null_ptid) { m_thread = inferior_thread (); m_thread->incref (); - struct frame_info *frame; - m_was_stopped = m_thread->state == THREAD_STOPPED; - if (m_was_stopped - && target_has_registers - && target_has_stack - && target_has_memory) - { - /* When processing internal events, there might not be a - selected frame. If we naively call get_selected_frame - here, then we can end up reading debuginfo for the - current frame, but we don't generally need the debuginfo - at this point. */ - frame = get_selected_frame_if_set (); - } - else - frame = NULL; - - try - { - m_selected_frame_id = get_frame_id (frame); - m_selected_frame_level = frame_relative_level (frame); - } - catch (const gdb_exception_error &ex) - { - m_selected_frame_id = null_frame_id; - m_selected_frame_level = -1; - - /* Better let this propagate. */ - if (ex.error == TARGET_CLOSE_ERROR) - throw; - } + get_selected_frame_info (&m_selected_frame_id, &m_selected_frame_level); } else m_thread = NULL; -- 2.14.5 ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 3/3] Make scoped_restore_current_thread's cdtors exception free (RFC) 2020-07-08 23:31 ` [PATCH 3/3] Make scoped_restore_current_thread's cdtors exception free (RFC) Pedro Alves @ 2020-07-09 3:49 ` Simon Marchi 2020-07-09 11:56 ` Pedro Alves 0 siblings, 1 reply; 31+ messages in thread From: Simon Marchi @ 2020-07-09 3:49 UTC (permalink / raw) To: Pedro Alves, gdb-patches On 2020-07-08 7:31 p.m., Pedro Alves wrote: > If the remote target closes while we're reading registers/memory for > restoring the selected frame in scoped_restore_current_thread's dtor, > the corresponding TARGET_CLOSE_ERROR error is swallowed by the > scoped_restore_current_thread's dtor, because letting exceptions > escape from a dtor is bad. It isn't great to lose that errors like > that, though. I've been thinking about how to avoid it, and I came up > with this patch. > > The idea here is to make scoped_restore_current_thread's dtor do as > little as possible, to avoid any work that might throw in the first > place. And to do that, instead of having the dtor call > restore_selected_frame, which re-finds the previously selected frame, > just record the frame_id/level of the desired selected frame, and have > get_selected_frame find the frame the next time it is called. In > effect, this implements most of Cagney's suggestion, here: > > /* On demand, create the selected frame and then return it. If the > selected frame can not be created, this function prints then throws > an error. When MESSAGE is non-NULL, use it for the error message, > otherwize use a generic error message. */ > /* FIXME: cagney/2002-11-28: At present, when there is no selected > frame, this function always returns the current (inner most) frame. > It should instead, when a thread has previously had its frame > selected (but not resumed) and the frame cache invalidated, find > and then return that thread's previously selected frame. */ > extern struct frame_info *get_selected_frame (const char *message); > > The only thing missing to fully implement that would be to make > reinit_frame_cache just clear selected_frame instead of calling > select_frame(NULL), and the call select_frame(NULL) explicitly in the > places where we really wanted reinit_frame_cache to go back to the > current frame too. That can done separately, though, I'm not > proposing to do that in this patch. > > restore_selected_frame should really move from thread.c to frame.c, > but I didn't do that here, just to avoid churn in the patch while it > collects comments. I will do that as a preparatory patch if people > agree with this approach. > > Incidentally, this patch alone would fix the crashes fixed by the > previous patches in the series, because with this, > scoped_restore_current_thread's constructor doesn't throw either. I don't understand all the code changes, the but the idea sounds fine to me. > -/* Select frame FI (or NULL - to invalidate the current frame). */ > +/* Select frame FI (or NULL - to invalidate the selected frame). */ > > void > select_frame (struct frame_info *fi) > { > selected_frame = fi; > + selected_frame_level = frame_relative_level (fi); > + if (selected_frame_level == 0) > + { > + /* Treat the current frame especially -- we want to always > + save/restore it without warning, even if the frame ID changes > + (see restore_selected_frame). Also get_frame_id may access > + the target's registers/memory, and thus skipping get_frame_id > + optimizes the common case. */ > + selected_frame_level = -1; > + selected_frame_id = null_frame_id; > + } > + else > + selected_frame_id = get_frame_id (fi); > + I don't really understand this part, why don't we want to set selected_frame_level and selected_frame_id when the level is 0. I'm more interested by why it wouldn't be correct or how it would break things, rather than the optimization aspect. Simon ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 3/3] Make scoped_restore_current_thread's cdtors exception free (RFC) 2020-07-09 3:49 ` Simon Marchi @ 2020-07-09 11:56 ` Pedro Alves 2020-07-09 12:09 ` Pedro Alves 2020-07-09 15:40 ` Simon Marchi 0 siblings, 2 replies; 31+ messages in thread From: Pedro Alves @ 2020-07-09 11:56 UTC (permalink / raw) To: Simon Marchi, gdb-patches On 7/9/20 4:49 AM, Simon Marchi wrote: > On 2020-07-08 7:31 p.m., Pedro Alves wrote: >> If the remote target closes while we're reading registers/memory for >> restoring the selected frame in scoped_restore_current_thread's dtor, >> the corresponding TARGET_CLOSE_ERROR error is swallowed by the >> scoped_restore_current_thread's dtor, because letting exceptions >> escape from a dtor is bad. It isn't great to lose that errors like >> that, though. I've been thinking about how to avoid it, and I came up >> with this patch. >> >> The idea here is to make scoped_restore_current_thread's dtor do as >> little as possible, to avoid any work that might throw in the first >> place. And to do that, instead of having the dtor call >> restore_selected_frame, which re-finds the previously selected frame, >> just record the frame_id/level of the desired selected frame, and have >> get_selected_frame find the frame the next time it is called. In >> effect, this implements most of Cagney's suggestion, here: >> >> /* On demand, create the selected frame and then return it. If the >> selected frame can not be created, this function prints then throws >> an error. When MESSAGE is non-NULL, use it for the error message, >> otherwize use a generic error message. */ >> /* FIXME: cagney/2002-11-28: At present, when there is no selected >> frame, this function always returns the current (inner most) frame. >> It should instead, when a thread has previously had its frame >> selected (but not resumed) and the frame cache invalidated, find >> and then return that thread's previously selected frame. */ >> extern struct frame_info *get_selected_frame (const char *message); >> >> The only thing missing to fully implement that would be to make >> reinit_frame_cache just clear selected_frame instead of calling >> select_frame(NULL), and the call select_frame(NULL) explicitly in the >> places where we really wanted reinit_frame_cache to go back to the >> current frame too. That can done separately, though, I'm not >> proposing to do that in this patch. >> >> restore_selected_frame should really move from thread.c to frame.c, >> but I didn't do that here, just to avoid churn in the patch while it >> collects comments. I will do that as a preparatory patch if people >> agree with this approach. >> >> Incidentally, this patch alone would fix the crashes fixed by the >> previous patches in the series, because with this, >> scoped_restore_current_thread's constructor doesn't throw either. > > I don't understand all the code changes, the but the idea sounds fine > to me. > >> -/* Select frame FI (or NULL - to invalidate the current frame). */ >> +/* Select frame FI (or NULL - to invalidate the selected frame). */ >> >> void >> select_frame (struct frame_info *fi) >> { >> selected_frame = fi; >> + selected_frame_level = frame_relative_level (fi); >> + if (selected_frame_level == 0) >> + { >> + /* Treat the current frame especially -- we want to always >> + save/restore it without warning, even if the frame ID changes >> + (see restore_selected_frame). Also get_frame_id may access >> + the target's registers/memory, and thus skipping get_frame_id >> + optimizes the common case. */ >> + selected_frame_level = -1; >> + selected_frame_id = null_frame_id; >> + } >> + else >> + selected_frame_id = get_frame_id (fi); >> + > > I don't really understand this part, why don't we want to set selected_frame_level > and selected_frame_id when the level is 0. I'm more interested by why it wouldn't > be correct or how it would break things, rather than the optimization aspect. > > Simon > At first, I was recording frame 0 normally, without that special case. But running the testsuite revealed regressions in a couple testcases: gdb.python/py-unwind-maint.exp gdb.server/bkpt-other-inferior.exp Both are related to the get_frame_id call. Before the patch, get_frame_id isn't called on the current frame until you try to backtrace from it. Adding the get_frame_id call makes the gdb.python/py-unwind-maint.exp testcase print the Python unwinder callbacks in a different order, unexpected by the testcase. I didn't look too deeply into this one, but I suspect it would just be a matter of adjusting the testcase's expectations. The gdb.server/bkpt-other-inferior.exp one though is what got me thinking. The testcase makes sure that setting a breakpoint in a function that doesn't exist in the remote inferior does not cause remote protocol traffic. After the patch, without the special casing, the testcase would fail because the get_frame_id call, coming from check_frame_language_change # called after every command -> get_selected_frame -> restore_selected_frame -> select_frame(get_current_frame()) -> get_frame_id would cause registers and memory to be read from the remote target (when restoring the selected frame). Those accesses aren't wrong, but they aren't the kind that the bug the testcase is looking for. Those were about spurious/incorrect remote protocol accesses when parsing the function's prologue. Neither of these cases were strictly incorrect, though they got me thinking, and I came to the conclusion that warning when we fail to re-find the current frame is pointless, and that avoids having unbreak the testcases mentioned, or even redo them differently in the gdb.server/bkpt-other-inferior.exp case. I've updated the comment to make it clearer with an example. I've also polished the patch some more. I now renamed the current restore_selected_frame to lookup_selected_frame, to give space to the new save_selected_frame/restore_selected_frame pair. select_frame_lazy is now restore_selected_frame. save_selected_frame/restore_selected_frame are now noexcept, and their intro comments explain why. I declared lookup_selected_frame in frame.h already, thinking that it's easier if I move lookup_selected_frame from thread.c to frame.c after this is in, instead of before. I rewrote most of the comments. For example, I think the selected_frame_id/selected_frame_level/selected_frame comments are now much clearer. And I made scoped_restore_selected_frame save/restore the language too. I was only doing that in scoped_restore_current_thread before. Let me know what you think of this version. From 1353dbd062debba4056c2413678d3438f7713ca3 Mon Sep 17 00:00:00 2001 From: Pedro Alves <palves@redhat.com> Date: Thu, 9 Jul 2020 11:31:29 +0100 Subject: [PATCH] Make scoped_restore_current_thread's cdtors exception free (RFC) If the remote target closes while we're reading registers/memory for restoring the selected frame in scoped_restore_current_thread's dtor, the corresponding TARGET_CLOSE_ERROR error is swallowed by the scoped_restore_current_thread's dtor, because letting exceptions escape from a dtor is bad. It isn't great to lose that errors like that, though. I've been thinking about how to avoid it, and I came up with this patch. The idea here is to make scoped_restore_current_thread's dtor do as little as possible, to avoid any work that might throw in the first place. And to do that, instead of having the dtor call restore_selected_frame, which re-finds the previously selected frame, just record the frame_id/level of the desired selected frame, and have get_selected_frame find the frame the next time it is called. In effect, this implements most of Cagney's suggestion, here: /* On demand, create the selected frame and then return it. If the selected frame can not be created, this function prints then throws an error. When MESSAGE is non-NULL, use it for the error message, otherwize use a generic error message. */ /* FIXME: cagney/2002-11-28: At present, when there is no selected frame, this function always returns the current (inner most) frame. It should instead, when a thread has previously had its frame selected (but not resumed) and the frame cache invalidated, find and then return that thread's previously selected frame. */ extern struct frame_info *get_selected_frame (const char *message); The only thing missing to fully implement that would be to make reinit_frame_cache just clear selected_frame instead of calling select_frame(NULL), and the call select_frame(NULL) explicitly in the places where we really wanted reinit_frame_cache to go back to the current frame too. That can done separately, though, I'm not proposing to do that in this patch. Note that this patch renames restore_selected_frame to lookup_selected_frame, and adds a new restore_selected_frame function that doesn't throw, to be paired with the also-new save_selected_frame function. lookup_selected_frame should really move from thread.c to frame.c, but I didn't do that here, just to avoid churn in the patch while it collects comments. I did make it extern and declared it in frame.h already, preparing for the move. I will do the move as a follow up patch if people agree with this approach. Incidentally, this patch alone would fix the crashes fixed by the previous patches in the series, because with this, scoped_restore_current_thread's constructor doesn't throw either. gdb/ChangeLog: * blockframe.c (block_innermost_frame): Use get_selected_frame. * frame.c (scoped_restore_selected_frame::scoped_restore_selected_frame): Use save_selected_frame. Save language as well. (scoped_restore_selected_frame::~scoped_restore_selected_frame): Use restore_selected_frame, and restore language as well. (selected_frame_id, selected_frame_level): New. (selected_frame): Update comments. (save_selected_frame, restore_selected_frame): New. (get_selected_frame): Use lookup_selected_frame. (get_selected_frame_if_set): Delete. (select_frame): Record selected_frame_level and selected_frame_id. * frame.h (scoped_restore_selected_frame) <m_level, m_lang>: New fields. (get_selected_frame_if_set): Delete declaration. (select_frame): Update comments. (save_selected_frame, restore_selected_frame) (lookup_selected_frame): Declare. * gdbthread.h (scoped_restore_current_thread) <m_lang>: New field. * stack.c (select_frame_command_core, frame_command_core): Use get_selected_frame. * thread.c (restore_selected_frame): Rename to ... (lookup_selected_frame): ... this and make extern. Select the current frame if the frame level is -1. (scoped_restore_current_thread::restore): Also restore the language. (scoped_restore_current_thread::~scoped_restore_current_thread): Don't try/catch. (scoped_restore_current_thread::scoped_restore_current_thread): Save the language as well. Use save_selected_frame. --- gdb/blockframe.c | 6 +--- gdb/frame.c | 101 +++++++++++++++++++++++++++++++++++++++++++------------ gdb/frame.h | 38 +++++++++++++++++---- gdb/gdbthread.h | 4 +++ gdb/stack.c | 9 +++-- gdb/thread.c | 67 +++++++++--------------------------- 6 files changed, 137 insertions(+), 88 deletions(-) diff --git a/gdb/blockframe.c b/gdb/blockframe.c index 05c26bc2c2..706b6db92c 100644 --- a/gdb/blockframe.c +++ b/gdb/blockframe.c @@ -448,14 +448,10 @@ find_gnu_ifunc_target_type (CORE_ADDR resolver_funaddr) struct frame_info * block_innermost_frame (const struct block *block) { - struct frame_info *frame; - if (block == NULL) return NULL; - frame = get_selected_frame_if_set (); - if (frame == NULL) - frame = get_current_frame (); + frame_info *frame = get_selected_frame (NULL); while (frame != NULL) { const struct block *frame_block = get_frame_block (frame, NULL); diff --git a/gdb/frame.c b/gdb/frame.c index ff27b9f00e..aab501ad67 100644 --- a/gdb/frame.c +++ b/gdb/frame.c @@ -297,17 +297,15 @@ frame_stash_invalidate (void) /* See frame.h */ scoped_restore_selected_frame::scoped_restore_selected_frame () { - m_fid = get_frame_id (get_selected_frame (NULL)); + m_lang = current_language->la_language; + save_selected_frame (&m_fid, &m_level); } /* See frame.h */ scoped_restore_selected_frame::~scoped_restore_selected_frame () { - frame_info *frame = frame_find_by_id (m_fid); - if (frame == NULL) - warning (_("Unable to restore previously selected frame.")); - else - select_frame (frame); + restore_selected_frame (m_fid, m_level); + set_language (m_lang); } /* Flag to control debugging. */ @@ -1641,10 +1639,51 @@ get_current_frame (void) } /* The "selected" stack frame is used by default for local and arg - access. May be zero, for no selected frame. */ - + access. */ + +/* The "single source of truth" for the selected frame is the + SELECTED_FRAME_ID / SELECTED_FRAME_LEVEL pair. Frame IDs can be + saved/restored across reinitializing the frame cache, while + frame_info pointers can't (frame_info objects are invalidated). If + we know the corresponding frame_info object, it is cached in + SELECTED_FRAME. If SELECTED_FRAME_ID / SELECTED_FRAME_LEVEL are + null_ptid / -1, and the target has stack and is stopped, the + selected frame is the current (innermost) frame, otherwise there's + no selected frame. */ +static frame_id selected_frame_id = null_frame_id; +static int selected_frame_level = -1; + +/* The cached frame_info object pointing to the selected frame. + Looked up on demand by get_selected_frame. */ static struct frame_info *selected_frame; +/* See frame.h. */ + +void +save_selected_frame (frame_id *frame_id, int *frame_level) + noexcept +{ + *frame_id = selected_frame_id; + *frame_level = selected_frame_level; +} + +/* See frame.h. */ + +void +restore_selected_frame (frame_id a_frame_id, int frame_level) + noexcept +{ + /* get_selected_frame_info never returns level == 0, so we shouldn't + see it here either. */ + gdb_assert (frame_level != 0); + + selected_frame_id = a_frame_id; + selected_frame_level = frame_level; + + /* Will be looked up latter by get_seleted_frame. */ + selected_frame = nullptr; +} + int has_stack_frames (void) { @@ -1682,24 +1721,14 @@ get_selected_frame (const char *message) { if (message != NULL && !has_stack_frames ()) error (("%s"), message); - /* Hey! Don't trust this. It should really be re-finding the - last selected frame of the currently selected thread. This, - though, is better than nothing. */ - select_frame (get_current_frame ()); + + lookup_selected_frame (selected_frame_id, selected_frame_level); } /* There is always a frame. */ gdb_assert (selected_frame != NULL); return selected_frame; } -/* If there is a selected frame, return it. Otherwise, return NULL. */ - -struct frame_info * -get_selected_frame_if_set (void) -{ - return selected_frame; -} - /* This is a variant of get_selected_frame() which can be called when the inferior does not have a frame; in that case it will return NULL instead of calling error(). */ @@ -1712,12 +1741,42 @@ deprecated_safe_get_selected_frame (void) return get_selected_frame (NULL); } -/* Select frame FI (or NULL - to invalidate the current frame). */ +/* Select frame FI (or NULL - to invalidate the selected frame). */ void select_frame (struct frame_info *fi) { selected_frame = fi; + selected_frame_level = frame_relative_level (fi); + if (selected_frame_level == 0) + { + /* Treat the current frame especially -- we want to always + save/restore it without warning, even if the frame ID changes + (see lookup_selected_frame). E.g.: + + // The current frame is selected, the target had just stopped. + { + scoped_restore_selected_frame restore_frame; + some_operation_that_changes_the_stack (); + } + // scoped_restore_selected_frame's dtor runs, but the + // original frame_id can't be found. No matter whether it + // is found or not, we still end up with the now-current + // frame selected. Warning in lookup_selected_frame in this + // case seems pointless. + + Also get_frame_id may access the target's registers/memory, + and thus skipping get_frame_id optimizes the common case. + + Saving the selected frame this way makes get_selected_frame + and restore_current_frame return/re-select whatever frame is + the innermost (current) then. */ + selected_frame_level = -1; + selected_frame_id = null_frame_id; + } + else + selected_frame_id = get_frame_id (fi); + /* NOTE: cagney/2002-05-04: FI can be NULL. This occurs when the frame is being invalidated. */ diff --git a/gdb/frame.h b/gdb/frame.h index e835d49f9c..97d50b645c 100644 --- a/gdb/frame.h +++ b/gdb/frame.h @@ -181,8 +181,14 @@ class scoped_restore_selected_frame private: - /* The ID of the previously selected frame. */ + /* The ID and level of the previously selected frame. */ struct frame_id m_fid; + int m_level; + + /* Save/restore the language as well, because selecting a frame + changes the current language to the frame's language if "set + language auto". */ + enum language m_lang; }; /* Methods for constructing and comparing Frame IDs. */ @@ -329,13 +335,33 @@ extern void reinit_frame_cache (void); and then return that thread's previously selected frame. */ extern struct frame_info *get_selected_frame (const char *message); -/* If there is a selected frame, return it. Otherwise, return NULL. */ -extern struct frame_info *get_selected_frame_if_set (void); - -/* Select a specific frame. NULL, apparently implies re-select the - inner most frame. */ +/* Select a specific frame. NULL implies re-select the inner most + frame. */ extern void select_frame (struct frame_info *); +/* Save the frame ID and frame level of the selected frame in FRAME_ID + and FRAME_LEVEL, to be restored later with restore_selected_frame. + This is preferred over getting the same info out of + get_selected_frame directly because this function does not create + the selected-frame's frame_info object if it hasn't been created + yet, and thus doesn't throw. */ +extern void save_selected_frame (frame_id *frame_id, int *frame_level) + noexcept; + +/* Restore selected frame as saved with save_selected_frame. Does not + try to find the corresponding frame_info object. Instead the next + call to get_selected_frame will look it up and cache the result. + This function does not throw, it is designed to be safe to called + from the destructors of RAII types. */ +extern void restore_selected_frame (frame_id frame_id, int frame_level) + noexcept; + +/* Lookup the frame_info object for the selected frame FRAME_ID / + FRAME_LEVEL and cache the result. If FRAME_LEVEL > 0 and the + originally selected frame isn't found, warn and select the + innermost (current) frame. */ +extern void lookup_selected_frame (frame_id frame_id, int frame_level); + /* Given a FRAME, return the next (more inner, younger) or previous (more outer, older) frame. */ extern struct frame_info *get_prev_frame (struct frame_info *); diff --git a/gdb/gdbthread.h b/gdb/gdbthread.h index 0166b2000f..edfdf98b3d 100644 --- a/gdb/gdbthread.h +++ b/gdb/gdbthread.h @@ -667,6 +667,10 @@ class scoped_restore_current_thread frame_id m_selected_frame_id; int m_selected_frame_level; bool m_was_stopped; + /* Save/restore the language as well, because selecting a frame + changes the current language to the frame's language if "set + language auto". */ + enum language m_lang; }; /* Returns a pointer into the thread_info corresponding to diff --git a/gdb/stack.c b/gdb/stack.c index 265e764dc2..93de451a12 100644 --- a/gdb/stack.c +++ b/gdb/stack.c @@ -1836,9 +1836,9 @@ trailing_outermost_frame (int count) static void select_frame_command_core (struct frame_info *fi, bool ignored) { - struct frame_info *prev_frame = get_selected_frame_if_set (); + frame_info *prev_frame = get_selected_frame (NULL); select_frame (fi); - if (get_selected_frame_if_set () != prev_frame) + if (get_selected_frame (NULL) != prev_frame) gdb::observers::user_selected_context_changed.notify (USER_SELECTED_FRAME); } @@ -1857,10 +1857,9 @@ select_frame_for_mi (struct frame_info *fi) static void frame_command_core (struct frame_info *fi, bool ignored) { - struct frame_info *prev_frame = get_selected_frame_if_set (); - + frame_info *prev_frame = get_selected_frame (nullptr); select_frame (fi); - if (get_selected_frame_if_set () != prev_frame) + if (get_selected_frame (nullptr) != prev_frame) gdb::observers::user_selected_context_changed.notify (USER_SELECTED_FRAME); else print_selected_thread_frame (current_uiout, USER_SELECTED_FRAME); diff --git a/gdb/thread.c b/gdb/thread.c index a3c2be7dd0..e0b49abf0c 100644 --- a/gdb/thread.c +++ b/gdb/thread.c @@ -1325,20 +1325,25 @@ switch_to_thread (process_stratum_target *proc_target, ptid_t ptid) switch_to_thread (thr); } -static void -restore_selected_frame (struct frame_id a_frame_id, int frame_level) +/* See frame.h. */ + +void +lookup_selected_frame (struct frame_id a_frame_id, int frame_level) { struct frame_info *frame = NULL; int count; - /* This means there was no selected frame. */ + /* This either means there was no selected frame, or the selected + frame was the innermost (the current frame). */ if (frame_level == -1) { - select_frame (NULL); + select_frame (get_current_frame ()); return; } - gdb_assert (frame_level >= 0); + /* select_frame never saves 0 in selected_frame_level, so we + shouldn't see it here. */ + gdb_assert (frame_level > 0); /* Restore by level first, check if the frame id is the same as expected. If that fails, try restoring by frame id. If that @@ -1409,22 +1414,14 @@ scoped_restore_current_thread::restore () && target_has_stack && target_has_memory) restore_selected_frame (m_selected_frame_id, m_selected_frame_level); + + set_language (m_lang); } scoped_restore_current_thread::~scoped_restore_current_thread () { if (!m_dont_restore) - { - try - { - restore (); - } - catch (const gdb_exception &ex) - { - /* We're in a dtor, there's really nothing else we can do - but swallow the exception. */ - } - } + restore (); if (m_thread != NULL) m_thread->decref (); @@ -1436,47 +1433,15 @@ scoped_restore_current_thread::scoped_restore_current_thread () m_inf = current_inferior (); m_inf->incref (); + m_lang = current_language->la_language; + if (inferior_ptid != null_ptid) { m_thread = inferior_thread (); m_thread->incref (); - struct frame_info *frame; - m_was_stopped = m_thread->state == THREAD_STOPPED; - if (m_was_stopped - && target_has_registers - && target_has_stack - && target_has_memory) - { - /* When processing internal events, there might not be a - selected frame. If we naively call get_selected_frame - here, then we can end up reading debuginfo for the - current frame, but we don't generally need the debuginfo - at this point. */ - frame = get_selected_frame_if_set (); - } - else - frame = NULL; - - try - { - m_selected_frame_id = get_frame_id (frame); - m_selected_frame_level = frame_relative_level (frame); - } - catch (const gdb_exception_error &ex) - { - m_selected_frame_id = null_frame_id; - m_selected_frame_level = -1; - - /* Better let this propagate. */ - if (ex.error == TARGET_CLOSE_ERROR) - { - m_thread->decref (); - m_inf->decref (); - throw; - } - } + save_selected_frame (&m_selected_frame_id, &m_selected_frame_level); } else m_thread = NULL; base-commit: ad8464f799a4c96c7ab8bdfec3f95846cf54f9b0 prerequisite-patch-id: 32ffdda7d7d774bc4df88bf848bcb796559b53ce prerequisite-patch-id: 02021b74355b70debd344a6e445285c67dfef7d6 prerequisite-patch-id: c87fcf5a54f6805967cbf8ab107606c57d9ecf52 prerequisite-patch-id: ac7dee583d0ffa519c9d1cd89d27664bca68d8c1 prerequisite-patch-id: eac59ae2ea85d2d51e5be1b03e88a5641cc12c22 prerequisite-patch-id: 13da42ad04dc8e2e3bd6a556a0be0e17cf23669b prerequisite-patch-id: fd3f09fdb58ddc1c595ea014716851f4c8fca48c prerequisite-patch-id: 8230262cb24020a5b37e915843180e940807ba1f prerequisite-patch-id: 59bcf41af4057da60e15e3f35fae1fd4e0bf36b9 -- 2.14.5 ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 3/3] Make scoped_restore_current_thread's cdtors exception free (RFC) 2020-07-09 11:56 ` Pedro Alves @ 2020-07-09 12:09 ` Pedro Alves 2020-07-09 15:40 ` Simon Marchi 1 sibling, 0 replies; 31+ messages in thread From: Pedro Alves @ 2020-07-09 12:09 UTC (permalink / raw) To: Simon Marchi, gdb-patches On 7/9/20 12:56 PM, Pedro Alves wrote: > On 7/9/20 4:49 AM, Simon Marchi wrote: >>> void >>> select_frame (struct frame_info *fi) >>> { >>> selected_frame = fi; >>> + selected_frame_level = frame_relative_level (fi); >>> + if (selected_frame_level == 0) >>> + { >>> + /* Treat the current frame especially -- we want to always >>> + save/restore it without warning, even if the frame ID changes >>> + (see restore_selected_frame). Also get_frame_id may access >>> + the target's registers/memory, and thus skipping get_frame_id >>> + optimizes the common case. */ >>> + selected_frame_level = -1; >>> + selected_frame_id = null_frame_id; >>> + } >>> + else >>> + selected_frame_id = get_frame_id (fi); >>> + >> >> I don't really understand this part, why don't we want to set selected_frame_level >> and selected_frame_id when the level is 0. I'm more interested by why it wouldn't >> be correct or how it would break things, rather than the optimization aspect. >> > > At first, I was recording frame 0 normally, without that special case. > But running the testsuite revealed regressions in a couple testcases: > > gdb.python/py-unwind-maint.exp > gdb.server/bkpt-other-inferior.exp > > Both are related to the get_frame_id call. Before the patch, get_frame_id > isn't called on the current frame until you try to backtrace from it. > Adding the get_frame_id call makes the gdb.python/py-unwind-maint.exp testcase > print the Python unwinder callbacks in a different order, unexpected > by the testcase. I didn't look too deeply into this one, but I suspect > it would just be a matter of adjusting the testcase's expectations. > > The gdb.server/bkpt-other-inferior.exp one though is what got me > thinking. The testcase makes sure that setting a breakpoint in a > function that doesn't exist in the remote inferior does not cause > remote protocol traffic. After the patch, without the special casing, > the testcase would fail because the get_frame_id call, coming from > > check_frame_language_change # called after every command > -> get_selected_frame > -> restore_selected_frame > -> select_frame(get_current_frame()) > -> get_frame_id > > would cause registers and memory to be read from the remote target (when > restoring the selected frame). Those accesses aren't wrong, but they > aren't the kind that the bug the testcase is looking for. Those were > about spurious/incorrect remote protocol accesses when parsing the > function's prologue. > > Neither of these cases were strictly incorrect, though they got me > thinking, and I came to the conclusion that warning when we fail to > re-find the current frame is pointless, and that avoids having > unbreak the testcases mentioned, or even redo them differently in > the gdb.server/bkpt-other-inferior.exp case. > > I've updated the comment to make it clearer with an example. > > I've also polished the patch some more. I now renamed > the current restore_selected_frame to lookup_selected_frame, > to give space to the new save_selected_frame/restore_selected_frame > pair. select_frame_lazy is now restore_selected_frame. > save_selected_frame/restore_selected_frame are now noexcept, and > their intro comments explain why. > > I declared lookup_selected_frame in frame.h already, thinking that > it's easier if I move lookup_selected_frame from thread.c to frame.c > after this is in, instead of before. > > I rewrote most of the comments. For example, I think the > selected_frame_id/selected_frame_level/selected_frame comments are now > much clearer. > > And I made scoped_restore_selected_frame save/restore the language > too. I was only doing that in scoped_restore_current_thread before. > > Let me know what you think of this version. I've pushed this, along with all the PR26199 patches to: users/palves/pr26199-busy-loop-target-events (The version pushed has a couple comment typos fixed compared to the one posted.) ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 3/3] Make scoped_restore_current_thread's cdtors exception free (RFC) 2020-07-09 11:56 ` Pedro Alves 2020-07-09 12:09 ` Pedro Alves @ 2020-07-09 15:40 ` Simon Marchi 2020-07-09 22:22 ` Pedro Alves 1 sibling, 1 reply; 31+ messages in thread From: Simon Marchi @ 2020-07-09 15:40 UTC (permalink / raw) To: Pedro Alves, gdb-patches > I've also polished the patch some more. I now renamed > the current restore_selected_frame to lookup_selected_frame, > to give space to the new save_selected_frame/restore_selected_frame > pair. select_frame_lazy is now restore_selected_frame. > save_selected_frame/restore_selected_frame are now noexcept, and > their intro comments explain why. I noticed there is also a `restore_selected_frame` in infrun.c... I don't know if it's replicating features also implemented in frame.c? > @@ -1641,10 +1639,51 @@ get_current_frame (void) > } > > /* The "selected" stack frame is used by default for local and arg > - access. May be zero, for no selected frame. */ > - > + access. */ > + > +/* The "single source of truth" for the selected frame is the > + SELECTED_FRAME_ID / SELECTED_FRAME_LEVEL pair. Frame IDs can be > + saved/restored across reinitializing the frame cache, while > + frame_info pointers can't (frame_info objects are invalidated). If > + we know the corresponding frame_info object, it is cached in > + SELECTED_FRAME. If SELECTED_FRAME_ID / SELECTED_FRAME_LEVEL are > + null_ptid / -1, and the target has stack and is stopped, the > + selected frame is the current (innermost) frame, otherwise there's > + no selected frame. */ Pedantically, the `otherwise` could let the reader think that if SELECTED_FRAME_ID / SELECTED_FRAME_LEVEL are not null_ptid / -1, then there is no selected frame. I'd suggest moving this out to its own sentence to be clear. I'd also make it more explicit here that when the innermost frame is selected, SELECTED_FRAME_ID / SELECTED_FRAME_LEVEL. Currently, it says that if SELECTED_FRAME_ID / SELECTED_FRAME_LEVEL are null_ptid / -1, it means the innermost frame is selected, but it doesn't imply that if the innermost frame is selected, then SELECTED_FRAME_ID / SELECTED_FRAME_LEVEL are null_ptid / -1 (difference between if and "if and only if"). I think it would help to separate that in paragraphes to ease reading. Also, I just noticed that you comment uses `null_ptid` instead of `null_frame_id`, which just shows how much `null_ptid` is engraved in your brain :). I also re-worked the comment for 15 minutes before noticing :). So: /* The "single source of truth" for the selected frame is the SELECTED_FRAME_ID / SELECTED_FRAME_LEVEL pair. Frame IDs can be saved/restored across reinitializing the frame cache, while frame_info pointers can't (frame_info objects are invalidated). If we know the corresponding frame_info object, it is cached in SELECTED_FRAME. If SELECTED_FRAME_ID / SELECTED_FRAME_LEVEL are null_frame_id / -1, and the target has stack and is stopped, the selected frame is the current (innermost) frame. This means that SELECTED_FRAME_LEVEL is never 0 and SELECTED_FRAME_ID is never the ID of the innermost frame. If SELECTED_FRAME_ID / SELECTED_FRAME_LEVEL are null_frame_id / -1, and the target has no stack or is executing, the selected, then there's no selected frame. */ > +static frame_id selected_frame_id = null_frame_id; > +static int selected_frame_level = -1; > + > +/* The cached frame_info object pointing to the selected frame. > + Looked up on demand by get_selected_frame. */ > static struct frame_info *selected_frame; Ah ok, in my previous reply I almost said: "so this essentially becomes a cache for selected_frame_id/selected_frame_level", but changed it because I *thought* I had understood that it wasn't the case when the frame_level was > 0. But if we can word it this way, that makes it simpler to understand. > > +/* See frame.h. */ > + > +void > +save_selected_frame (frame_id *frame_id, int *frame_level) > + noexcept > +{ > + *frame_id = selected_frame_id; > + *frame_level = selected_frame_level; > +} > + > +/* See frame.h. */ > + > +void > +restore_selected_frame (frame_id a_frame_id, int frame_level) > + noexcept > +{ > + /* get_selected_frame_info never returns level == 0, so we shouldn't > + see it here either. */ > + gdb_assert (frame_level != 0); We could also assert that a_frame_id can be null_frame_id IFF frame_level is -1. > + > + selected_frame_id = a_frame_id; > + selected_frame_level = frame_level; > + > + /* Will be looked up latter by get_seleted_frame. */ > + selected_frame = nullptr; > +} > + > int > has_stack_frames (void) > { > @@ -1682,24 +1721,14 @@ get_selected_frame (const char *message) > { > if (message != NULL && !has_stack_frames ()) > error (("%s"), message); > - /* Hey! Don't trust this. It should really be re-finding the > - last selected frame of the currently selected thread. This, > - though, is better than nothing. */ > - select_frame (get_current_frame ()); > + > + lookup_selected_frame (selected_frame_id, selected_frame_level); Could you fix (in this patch or another one) the comment of `get_selected_frame` to be `/* See frame.h. */` and make sure that the version in frame.h is the most up to date one? > @@ -1712,12 +1741,42 @@ deprecated_safe_get_selected_frame (void) > return get_selected_frame (NULL); > } > > -/* Select frame FI (or NULL - to invalidate the current frame). */ > +/* Select frame FI (or NULL - to invalidate the selected frame). */ > > void > select_frame (struct frame_info *fi) > { > selected_frame = fi; > + selected_frame_level = frame_relative_level (fi); > + if (selected_frame_level == 0) > + { > + /* Treat the current frame especially -- we want to always > + save/restore it without warning, even if the frame ID changes > + (see lookup_selected_frame). E.g.: > + > + // The current frame is selected, the target had just stopped. > + { > + scoped_restore_selected_frame restore_frame; > + some_operation_that_changes_the_stack (); > + } > + // scoped_restore_selected_frame's dtor runs, but the > + // original frame_id can't be found. No matter whether it > + // is found or not, we still end up with the now-current > + // frame selected. Warning in lookup_selected_frame in this > + // case seems pointless. > + > + Also get_frame_id may access the target's registers/memory, > + and thus skipping get_frame_id optimizes the common case. > + > + Saving the selected frame this way makes get_selected_frame > + and restore_current_frame return/re-select whatever frame is restore_selected_frame > @@ -329,13 +335,33 @@ extern void reinit_frame_cache (void); > and then return that thread's previously selected frame. */ > extern struct frame_info *get_selected_frame (const char *message); > > -/* If there is a selected frame, return it. Otherwise, return NULL. */ > -extern struct frame_info *get_selected_frame_if_set (void); > - > -/* Select a specific frame. NULL, apparently implies re-select the > - inner most frame. */ > +/* Select a specific frame. NULL implies re-select the inner most > + frame. */ > extern void select_frame (struct frame_info *); > > +/* Save the frame ID and frame level of the selected frame in FRAME_ID > + and FRAME_LEVEL, to be restored later with restore_selected_frame. > + This is preferred over getting the same info out of > + get_selected_frame directly because this function does not create > + the selected-frame's frame_info object if it hasn't been created > + yet, and thus doesn't throw. */ > +extern void save_selected_frame (frame_id *frame_id, int *frame_level) > + noexcept; > + > +/* Restore selected frame as saved with save_selected_frame. Does not > + try to find the corresponding frame_info object. Instead the next > + call to get_selected_frame will look it up and cache the result. > + This function does not throw, it is designed to be safe to called > + from the destructors of RAII types. */ > +extern void restore_selected_frame (frame_id frame_id, int frame_level) > + noexcept; > + > +/* Lookup the frame_info object for the selected frame FRAME_ID / > + FRAME_LEVEL and cache the result. If FRAME_LEVEL > 0 and the > + originally selected frame isn't found, warn and select the > + innermost (current) frame. */ > +extern void lookup_selected_frame (frame_id frame_id, int frame_level); As I mentioned above, I think the comments would be easier to read with a bit more newlines. In general, I like to have one short sentence by itself first, that says what the function does at a very high level. A bit like in man pages you have a short description next to the name: rm - remove files or directories sudo, sudoedit — execute a command as another user ssh — OpenSSH remote login client Then, you can go in details about the behavior of the function in following paragraphs, making sure to split different ideas in different paragraphs. At first, I thought it would just be nitpicking to ask people to space out their comments and code a bit more, but now I think it really does make a difference in readability (at least, it helps me). > diff --git a/gdb/stack.c b/gdb/stack.c > index 265e764dc2..93de451a12 100644 > --- a/gdb/stack.c > +++ b/gdb/stack.c > @@ -1836,9 +1836,9 @@ trailing_outermost_frame (int count) > static void > select_frame_command_core (struct frame_info *fi, bool ignored) > { > - struct frame_info *prev_frame = get_selected_frame_if_set (); > + frame_info *prev_frame = get_selected_frame (NULL); > select_frame (fi); > - if (get_selected_frame_if_set () != prev_frame) > + if (get_selected_frame (NULL) != prev_frame) I'm telling people that we try to use `nullptr` for new code. I don't know what's your opinion on this. I would just like to have a simple and clear so we don't have to wonder which of `NULL` and `nullptr` to use. > gdb::observers::user_selected_context_changed.notify (USER_SELECTED_FRAME); > } > > @@ -1857,10 +1857,9 @@ select_frame_for_mi (struct frame_info *fi) > static void > frame_command_core (struct frame_info *fi, bool ignored) > { > - struct frame_info *prev_frame = get_selected_frame_if_set (); > - > + frame_info *prev_frame = get_selected_frame (nullptr); > select_frame (fi); > - if (get_selected_frame_if_set () != prev_frame) > + if (get_selected_frame (nullptr) != prev_frame) ... especially that you've used `nullptr` here :). Although maybe that in this case, get_selected_frame could have a default parameter value of `nullptr`. > diff --git a/gdb/thread.c b/gdb/thread.c > index a3c2be7dd0..e0b49abf0c 100644 > --- a/gdb/thread.c > +++ b/gdb/thread.c > @@ -1325,20 +1325,25 @@ switch_to_thread (process_stratum_target *proc_target, ptid_t ptid) > switch_to_thread (thr); > } > > -static void > -restore_selected_frame (struct frame_id a_frame_id, int frame_level) > +/* See frame.h. */ > + > +void > +lookup_selected_frame (struct frame_id a_frame_id, int frame_level) > { > struct frame_info *frame = NULL; > int count; > > - /* This means there was no selected frame. */ > + /* This either means there was no selected frame, or the selected > + frame was the innermost (the current frame). */ It's a bit heavy to always precise that `innermost` means the `current` frame. Let's choose one name and stick to it, it will be clear enough on its own. I'd use `current`, since that's how the functions are named (e.g. get_current_frame). I understand there might be a bit confusion between `selected` and `current` for newcomers, but once you know the distinction, it's pretty clear. I'd also add to that comment, what is the action we take as a result. So, maybe: /* This either means there was no selected frame, or the selected frame was the current one. In either case, try to select the current frame. */ > @@ -1409,22 +1414,14 @@ scoped_restore_current_thread::restore () > && target_has_stack > && target_has_memory) > restore_selected_frame (m_selected_frame_id, m_selected_frame_level); > + > + set_language (m_lang); > } > > scoped_restore_current_thread::~scoped_restore_current_thread () > { > if (!m_dont_restore) > - { > - try > - { > - restore (); > - } > - catch (const gdb_exception &ex) > - { > - /* We're in a dtor, there's really nothing else we can do > - but swallow the exception. */ > - } > - } > + restore (); I don't know if it would be worth it, but I'd like if we could assert (abort GDB) if an exception does try to exit the destructor. The `restore` method is non-trivial and calls into other non-trivial functions, so it would be possible for a change far far away to cause that to happen. What do you think of keeping the try/catch, but using `gdb_assert_not_reached` in it? Simon ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 3/3] Make scoped_restore_current_thread's cdtors exception free (RFC) 2020-07-09 15:40 ` Simon Marchi @ 2020-07-09 22:22 ` Pedro Alves 2020-07-10 2:55 ` Simon Marchi 0 siblings, 1 reply; 31+ messages in thread From: Pedro Alves @ 2020-07-09 22:22 UTC (permalink / raw) To: Simon Marchi, gdb-patches On 7/9/20 4:40 PM, Simon Marchi wrote: >> I've also polished the patch some more. I now renamed >> the current restore_selected_frame to lookup_selected_frame, >> to give space to the new save_selected_frame/restore_selected_frame >> pair. select_frame_lazy is now restore_selected_frame. >> save_selected_frame/restore_selected_frame are now noexcept, and >> their intro comments explain why. > > I noticed there is also a `restore_selected_frame` in infrun.c... I don't > know if it's replicating features also implemented in frame.c? Yeah, I saw that too, but didn't look too deeply, given I was more looking at validating the whole idea. I think that can be replaced, yes. Done now. BTW, I don't know why I missed it before, but we _already_ don't warn the user if we fail to restore frame #0: /* Warn the user. */ if (frame_level > 0 && !current_uiout->is_mi_like_p ()) ^^^^^^^^^^^^^^^ { And that came in with: commit 6c95b8df7fef5273da71c34775918c554aae0ea8 Author: Pedro Alves <palves@redhat.com> AuthorDate: Mon Oct 19 09:51:43 2009 +0000 2009-10-19 Pedro Alves <pedro@codesourcery.com> Stan Shebs <stan@codesourcery.com> Add base multi-executable/process support to GDB. I'm pretty sure I wrote that bit of the patch... > >> @@ -1641,10 +1639,51 @@ get_current_frame (void) >> } >> >> /* The "selected" stack frame is used by default for local and arg >> - access. May be zero, for no selected frame. */ >> - >> + access. */ >> + >> +/* The "single source of truth" for the selected frame is the >> + SELECTED_FRAME_ID / SELECTED_FRAME_LEVEL pair. Frame IDs can be >> + saved/restored across reinitializing the frame cache, while >> + frame_info pointers can't (frame_info objects are invalidated). If >> + we know the corresponding frame_info object, it is cached in >> + SELECTED_FRAME. If SELECTED_FRAME_ID / SELECTED_FRAME_LEVEL are >> + null_ptid / -1, and the target has stack and is stopped, the >> + selected frame is the current (innermost) frame, otherwise there's >> + no selected frame. */ > > Pedantically, the `otherwise` could let the reader think that if > SELECTED_FRAME_ID / SELECTED_FRAME_LEVEL are not null_ptid / -1, then there > is no selected frame. I'd suggest moving this out to its own sentence to be > clear. > > I'd also make it more explicit here that when the innermost frame is selected, > SELECTED_FRAME_ID / SELECTED_FRAME_LEVEL. Currently, it says that if > SELECTED_FRAME_ID / SELECTED_FRAME_LEVEL are null_ptid / -1, it means the > innermost frame is selected, but it doesn't imply that if the innermost frame > is selected, then SELECTED_FRAME_ID / SELECTED_FRAME_LEVEL are null_ptid / -1 > (difference between if and "if and only if"). > > I think it would help to separate that in paragraphes to ease reading. > > Also, I just noticed that you comment uses `null_ptid` instead of `null_frame_id`, > which just shows how much `null_ptid` is engraved in your brain :). I also > re-worked the comment for 15 minutes before noticing :). > > So: > > /* The "single source of truth" for the selected frame is the > SELECTED_FRAME_ID / SELECTED_FRAME_LEVEL pair. > > Frame IDs can be saved/restored across reinitializing the frame cache, while > frame_info pointers can't (frame_info objects are invalidated). If we know > the corresponding frame_info object, it is cached in SELECTED_FRAME. > > If SELECTED_FRAME_ID / SELECTED_FRAME_LEVEL are null_frame_id / -1, and the > target has stack and is stopped, the selected frame is the current > (innermost) frame. This means that SELECTED_FRAME_LEVEL is never 0 and > SELECTED_FRAME_ID is never the ID of the innermost frame. > > If SELECTED_FRAME_ID / SELECTED_FRAME_LEVEL are null_frame_id / -1, and the > target has no stack or is executing, the selected, then there's no selected > frame. */ I like it. > >> +static frame_id selected_frame_id = null_frame_id; >> +static int selected_frame_level = -1; >> + >> +/* The cached frame_info object pointing to the selected frame. >> + Looked up on demand by get_selected_frame. */ >> static struct frame_info *selected_frame; > > Ah ok, in my previous reply I almost said: "so this essentially becomes > a cache for selected_frame_id/selected_frame_level", but changed it because > I *thought* I had understood that it wasn't the case when the frame_level > was > 0. But if we can word it this way, that makes it simpler to understand. > >> >> +/* See frame.h. */ >> + >> +void >> +save_selected_frame (frame_id *frame_id, int *frame_level) >> + noexcept >> +{ >> + *frame_id = selected_frame_id; >> + *frame_level = selected_frame_level; >> +} >> + >> +/* See frame.h. */ >> + >> +void >> +restore_selected_frame (frame_id a_frame_id, int frame_level) >> + noexcept >> +{ >> + /* get_selected_frame_info never returns level == 0, so we shouldn't >> + see it here either. */ >> + gdb_assert (frame_level != 0); > > We could also assert that a_frame_id can be null_frame_id IFF frame_level is -1. Done. > >> + >> + selected_frame_id = a_frame_id; >> + selected_frame_level = frame_level; >> + >> + /* Will be looked up latter by get_seleted_frame. */ >> + selected_frame = nullptr; >> +} >> + >> int >> has_stack_frames (void) >> { >> @@ -1682,24 +1721,14 @@ get_selected_frame (const char *message) >> { >> if (message != NULL && !has_stack_frames ()) >> error (("%s"), message); >> - /* Hey! Don't trust this. It should really be re-finding the >> - last selected frame of the currently selected thread. This, >> - though, is better than nothing. */ >> - select_frame (get_current_frame ()); >> + >> + lookup_selected_frame (selected_frame_id, selected_frame_level); > > Could you fix (in this patch or another one) the comment of `get_selected_frame` > to be `/* See frame.h. */` and make sure that the version in frame.h is the > most up to date one? Done. > >> @@ -1712,12 +1741,42 @@ deprecated_safe_get_selected_frame (void) >> return get_selected_frame (NULL); >> } >> >> -/* Select frame FI (or NULL - to invalidate the current frame). */ >> +/* Select frame FI (or NULL - to invalidate the selected frame). */ >> >> void >> select_frame (struct frame_info *fi) >> { >> selected_frame = fi; >> + selected_frame_level = frame_relative_level (fi); >> + if (selected_frame_level == 0) >> + { >> + /* Treat the current frame especially -- we want to always >> + save/restore it without warning, even if the frame ID changes >> + (see lookup_selected_frame). E.g.: >> + >> + // The current frame is selected, the target had just stopped. >> + { >> + scoped_restore_selected_frame restore_frame; >> + some_operation_that_changes_the_stack (); >> + } >> + // scoped_restore_selected_frame's dtor runs, but the >> + // original frame_id can't be found. No matter whether it >> + // is found or not, we still end up with the now-current >> + // frame selected. Warning in lookup_selected_frame in this >> + // case seems pointless. >> + >> + Also get_frame_id may access the target's registers/memory, >> + and thus skipping get_frame_id optimizes the common case. >> + >> + Saving the selected frame this way makes get_selected_frame >> + and restore_current_frame return/re-select whatever frame is > > restore_selected_frame Fixed. > >> @@ -329,13 +335,33 @@ extern void reinit_frame_cache (void); >> and then return that thread's previously selected frame. */ >> extern struct frame_info *get_selected_frame (const char *message); >> >> -/* If there is a selected frame, return it. Otherwise, return NULL. */ >> -extern struct frame_info *get_selected_frame_if_set (void); >> - >> -/* Select a specific frame. NULL, apparently implies re-select the >> - inner most frame. */ >> +/* Select a specific frame. NULL implies re-select the inner most >> + frame. */ >> extern void select_frame (struct frame_info *); >> >> +/* Save the frame ID and frame level of the selected frame in FRAME_ID >> + and FRAME_LEVEL, to be restored later with restore_selected_frame. >> + This is preferred over getting the same info out of >> + get_selected_frame directly because this function does not create >> + the selected-frame's frame_info object if it hasn't been created >> + yet, and thus doesn't throw. */ >> +extern void save_selected_frame (frame_id *frame_id, int *frame_level) >> + noexcept; >> + >> +/* Restore selected frame as saved with save_selected_frame. Does not >> + try to find the corresponding frame_info object. Instead the next >> + call to get_selected_frame will look it up and cache the result. >> + This function does not throw, it is designed to be safe to called >> + from the destructors of RAII types. */ >> +extern void restore_selected_frame (frame_id frame_id, int frame_level) >> + noexcept; >> + >> +/* Lookup the frame_info object for the selected frame FRAME_ID / >> + FRAME_LEVEL and cache the result. If FRAME_LEVEL > 0 and the >> + originally selected frame isn't found, warn and select the >> + innermost (current) frame. */ >> +extern void lookup_selected_frame (frame_id frame_id, int frame_level); > > As I mentioned above, I think the comments would be easier to read with > a bit more newlines. In general, I like to have one short sentence by > itself first, that says what the function does at a very high level. A > bit like in man pages you have a short description next to the name: > > rm - remove files or directories > sudo, sudoedit — execute a command as another user > ssh — OpenSSH remote login client > > Then, you can go in details about the behavior of the function in following > paragraphs, making sure to split different ideas in different paragraphs. > > At first, I thought it would just be nitpicking to ask people to space > out their comments and code a bit more, but now I think it really does > make a difference in readability (at least, it helps me). > Done. >> diff --git a/gdb/stack.c b/gdb/stack.c >> index 265e764dc2..93de451a12 100644 >> --- a/gdb/stack.c >> +++ b/gdb/stack.c >> @@ -1836,9 +1836,9 @@ trailing_outermost_frame (int count) >> static void >> select_frame_command_core (struct frame_info *fi, bool ignored) >> { >> - struct frame_info *prev_frame = get_selected_frame_if_set (); >> + frame_info *prev_frame = get_selected_frame (NULL); >> select_frame (fi); >> - if (get_selected_frame_if_set () != prev_frame) >> + if (get_selected_frame (NULL) != prev_frame) > > I'm telling people that we try to use `nullptr` for new code. I don't > know what's your opinion on this. I would just like to have a simple > and clear so we don't have to wonder which of `NULL` and `nullptr` to > use. I just tend to use nullptr for new code, unless the surrounding code is already using NULL. In this case I just copy/pasted from elsewhere and didn't even realize it. NULL and nullptr are both cyan and thus look the same on my emacs. :-) I do tend to stick with writing "NULL" in comments. > >> gdb::observers::user_selected_context_changed.notify (USER_SELECTED_FRAME); >> } >> >> @@ -1857,10 +1857,9 @@ select_frame_for_mi (struct frame_info *fi) >> static void >> frame_command_core (struct frame_info *fi, bool ignored) >> { >> - struct frame_info *prev_frame = get_selected_frame_if_set (); >> - >> + frame_info *prev_frame = get_selected_frame (nullptr); >> select_frame (fi); >> - if (get_selected_frame_if_set () != prev_frame) >> + if (get_selected_frame (nullptr) != prev_frame) > > ... especially that you've used `nullptr` here :). > > Although maybe that in this case, get_selected_frame could have a default > parameter value of `nullptr`. > Done. >> diff --git a/gdb/thread.c b/gdb/thread.c >> index a3c2be7dd0..e0b49abf0c 100644 >> --- a/gdb/thread.c >> +++ b/gdb/thread.c >> @@ -1325,20 +1325,25 @@ switch_to_thread (process_stratum_target *proc_target, ptid_t ptid) >> switch_to_thread (thr); >> } >> >> -static void >> -restore_selected_frame (struct frame_id a_frame_id, int frame_level) >> +/* See frame.h. */ >> + >> +void >> +lookup_selected_frame (struct frame_id a_frame_id, int frame_level) >> { >> struct frame_info *frame = NULL; >> int count; >> >> - /* This means there was no selected frame. */ >> + /* This either means there was no selected frame, or the selected >> + frame was the innermost (the current frame). */ > > It's a bit heavy to always precise that `innermost` means the `current` > frame. Let's choose one name and stick to it, it will be clear enough > on its own. I'd use `current`, since that's how the functions are named > (e.g. get_current_frame). I understand there might be a bit confusion > between `selected` and `current` for newcomers, but once you know the > distinction, it's pretty clear. Sure. > > I'd also add to that comment, what is the action we take as a result. > > So, maybe: > > /* This either means there was no selected frame, or the selected frame was > the current one. In either case, try to select the current frame. */ OK. I'll remove the "try" though. > > >> @@ -1409,22 +1414,14 @@ scoped_restore_current_thread::restore () >> && target_has_stack >> && target_has_memory) >> restore_selected_frame (m_selected_frame_id, m_selected_frame_level); >> + >> + set_language (m_lang); >> } >> >> scoped_restore_current_thread::~scoped_restore_current_thread () >> { >> if (!m_dont_restore) >> - { >> - try >> - { >> - restore (); >> - } >> - catch (const gdb_exception &ex) >> - { >> - /* We're in a dtor, there's really nothing else we can do >> - but swallow the exception. */ >> - } >> - } >> + restore (); > > I don't know if it would be worth it, but I'd like if we could assert (abort > GDB) if an exception does try to exit the destructor. The `restore` method > is non-trivial and calls into other non-trivial functions, so it would be > possible for a change far far away to cause that to happen. It will already abort. Destructors are noexcept by default, so if an exception escapes a destructor, std::terminate() is called, and that calls abort by default. > > What do you think of keeping the try/catch, but using `gdb_assert_not_reached` > in it? Not sure. If we do that, we do get a nicer error message. However if the user says "n" to "Quit this debugging session" we still abort. /home/pedro/brno/pedro/gdb/binutils-gdb-2/build/../src/gdb/thread.c:1441: internal-error: scoped_restore_current_thread::~scoped_restore_current_thread(): unexpected exception thrown from destructor: hello A problem internal to GDB has been detected, further debugging may prove unreliable. Quit this debugging session? (y or n) n This is a bug, please report it. For instructions, see: <https://www.gnu.org/software/gdb/bugs/>. /home/pedro/brno/pedro/gdb/binutils-gdb-2/build/../src/gdb/thread.c:1441: internal-error: scoped_restore_current_thread::~scoped_restore_current_thread(): unexpected exception thrown from destructor: hello A problem internal to GDB has been detected, further debugging may prove unreliable. Create a core file of GDB? (y or n) n terminate called after throwing an instance of 'gdb_exception_quit' Aborted (core dumped) Maybe it would be interesting to add a variant of internal_error that did not throw a quit, so the user could swallow the exception... Maybe consider wrapping that as a generic facility to add to all non-trivial RAII destructors we have? Like a function that takes a function_view as parameter, so we would write: foo::~foo () { safe_dtor (__FILE__, __LINE__, [&] () { restore (); }); } Even better, add a SAFE_DTOR macro using similar magic SCOPE_EXIT macro uses to be able to write: foo::~foo () { SAFE_DTOR { restore (); }; } Here's the current version of the patch. From 63310d189bb2516e0fec2b0922041388a5a96374 Mon Sep 17 00:00:00 2001 From: Pedro Alves <pedro@palves.net> Date: Thu, 9 Jul 2020 18:26:15 +0100 Subject: [PATCH] Make scoped_restore_current_thread's cdtors exception free (RFC) If the remote target closes while we're reading registers/memory for restoring the selected frame in scoped_restore_current_thread's dtor, the corresponding TARGET_CLOSE_ERROR error is swallowed by the scoped_restore_current_thread's dtor, because letting exceptions escape from a dtor is bad. It isn't great to lose that errors like that, though. I've been thinking about how to avoid it, and I came up with this patch. The idea here is to make scoped_restore_current_thread's dtor do as little as possible, to avoid any work that might throw in the first place. And to do that, instead of having the dtor call restore_selected_frame, which re-finds the previously selected frame, just record the frame_id/level of the desired selected frame, and have get_selected_frame find the frame the next time it is called. In effect, this implements most of Cagney's suggestion, here: /* On demand, create the selected frame and then return it. If the selected frame can not be created, this function prints then throws an error. When MESSAGE is non-NULL, use it for the error message, otherwize use a generic error message. */ /* FIXME: cagney/2002-11-28: At present, when there is no selected frame, this function always returns the current (inner most) frame. It should instead, when a thread has previously had its frame selected (but not resumed) and the frame cache invalidated, find and then return that thread's previously selected frame. */ extern struct frame_info *get_selected_frame (const char *message); The only thing missing to fully implement that would be to make reinit_frame_cache just clear selected_frame instead of calling select_frame(NULL), and the call select_frame(NULL) explicitly in the places where we really wanted reinit_frame_cache to go back to the current frame too. That can done separately, though, I'm not proposing to do that in this patch. Note that this patch renames restore_selected_frame to lookup_selected_frame, and adds a new restore_selected_frame function that doesn't throw, to be paired with the also-new save_selected_frame function. There's a restore_selected_frame function in infrun.c that I think can be replaced by the new one in frame.c. Also done in this patch is make the get_selected_frame's parameter be optional, so that we don't have to pass down nullptr explicitly all over the place. lookup_selected_frame should really move from thread.c to frame.c, but I didn't do that here, just to avoid churn in the patch while it collects comments. I did make it extern and declared it in frame.h already, preparing for the move. I will do the move as a follow up patch if people agree with this approach. Incidentally, this patch alone would fix the crashes fixed by the previous patches in the series, because with this, scoped_restore_current_thread's constructor doesn't throw either. gdb/ChangeLog: * blockframe.c (block_innermost_frame): Use get_selected_frame. * frame.c (scoped_restore_selected_frame::scoped_restore_selected_frame): Use save_selected_frame. Save language as well. (scoped_restore_selected_frame::~scoped_restore_selected_frame): Use restore_selected_frame, and restore language as well. (selected_frame_id, selected_frame_level): New. (selected_frame): Update comments. (save_selected_frame, restore_selected_frame): New. (get_selected_frame): Use lookup_selected_frame. (get_selected_frame_if_set): Delete. (select_frame): Record selected_frame_level and selected_frame_id. * frame.h (scoped_restore_selected_frame) <m_level, m_lang>: New fields. (get_selected_frame): Make 'message' parameter optional. (get_selected_frame_if_set): Delete declaration. (select_frame): Update comments. (save_selected_frame, restore_selected_frame) (lookup_selected_frame): Declare. * gdbthread.h (scoped_restore_current_thread) <m_lang>: New field. * infrun.c (struct infcall_control_state) <selected_frame_level>: New field. (save_infcall_control_state): Use save_selected_frame. (restore_selected_frame): Delete. (restore_infcall_control_state): Use restore_selected_frame. * stack.c (select_frame_command_core, frame_command_core): Use get_selected_frame. * thread.c (restore_selected_frame): Rename to ... (lookup_selected_frame): ... this and make extern. Select the current frame if the frame level is -1. (scoped_restore_current_thread::restore): Also restore the language. (scoped_restore_current_thread::~scoped_restore_current_thread): Don't try/catch. (scoped_restore_current_thread::scoped_restore_current_thread): Save the language as well. Use save_selected_frame. --- gdb/blockframe.c | 6 +-- gdb/frame.c | 117 +++++++++++++++++++++++++++++++++++++++++++------------ gdb/frame.h | 53 +++++++++++++++++++------ gdb/gdbthread.h | 4 ++ gdb/infrun.c | 40 ++++--------------- gdb/stack.c | 9 ++--- gdb/thread.c | 64 ++++++++---------------------- 7 files changed, 168 insertions(+), 125 deletions(-) diff --git a/gdb/blockframe.c b/gdb/blockframe.c index 05c26bc2c2..0b868d8341 100644 --- a/gdb/blockframe.c +++ b/gdb/blockframe.c @@ -448,14 +448,10 @@ find_gnu_ifunc_target_type (CORE_ADDR resolver_funaddr) struct frame_info * block_innermost_frame (const struct block *block) { - struct frame_info *frame; - if (block == NULL) return NULL; - frame = get_selected_frame_if_set (); - if (frame == NULL) - frame = get_current_frame (); + frame_info *frame = get_selected_frame (); while (frame != NULL) { const struct block *frame_block = get_frame_block (frame, NULL); diff --git a/gdb/frame.c b/gdb/frame.c index ff27b9f00e..86cf9e4b69 100644 --- a/gdb/frame.c +++ b/gdb/frame.c @@ -297,17 +297,15 @@ frame_stash_invalidate (void) /* See frame.h */ scoped_restore_selected_frame::scoped_restore_selected_frame () { - m_fid = get_frame_id (get_selected_frame (NULL)); + m_lang = current_language->la_language; + save_selected_frame (&m_fid, &m_level); } /* See frame.h */ scoped_restore_selected_frame::~scoped_restore_selected_frame () { - frame_info *frame = frame_find_by_id (m_fid); - if (frame == NULL) - warning (_("Unable to restore previously selected frame.")); - else - select_frame (frame); + restore_selected_frame (m_fid, m_level); + set_language (m_lang); } /* Flag to control debugging. */ @@ -1641,10 +1639,63 @@ get_current_frame (void) } /* The "selected" stack frame is used by default for local and arg - access. May be zero, for no selected frame. */ - + access. + + The "single source of truth" for the selected frame is the + SELECTED_FRAME_ID / SELECTED_FRAME_LEVEL pair. + + Frame IDs can be saved/restored across reinitializing the frame + cache, while frame_info pointers can't (frame_info objects are + invalidated). If we know the corresponding frame_info object, it + is cached in SELECTED_FRAME. + + If SELECTED_FRAME_ID / SELECTED_FRAME_LEVEL are null_frame_id / -1, + and the target has stack and is stopped, the selected frame is the + current (innermost) frame. This means that SELECTED_FRAME_LEVEL is + never 0 and SELECTED_FRAME_ID is never the ID of the innermost + frame. + + If SELECTED_FRAME_ID / SELECTED_FRAME_LEVEL are null_frame_id / -1, + and the target has no stack or is executing, then there's no + selected frame. */ +static frame_id selected_frame_id = null_frame_id; +static int selected_frame_level = -1; + +/* The cached frame_info object pointing to the selected frame. + Looked up on demand by get_selected_frame. */ static struct frame_info *selected_frame; +/* See frame.h. */ + +void +save_selected_frame (frame_id *frame_id, int *frame_level) + noexcept +{ + *frame_id = selected_frame_id; + *frame_level = selected_frame_level; +} + +/* See frame.h. */ + +void +restore_selected_frame (frame_id frame_id, int frame_level) + noexcept +{ + /* save_selected_frame never returns level == 0, so we shouldn't see + it here either. */ + gdb_assert (frame_level != 0); + + /* FRAME_ID can be null_frame_id only IFF frame_level is -1. */ + gdb_assert ((frame_level == -1 && !frame_id_p (frame_id)) + || (frame_level != -1 && frame_id_p (frame_id))); + + selected_frame_id = frame_id; + selected_frame_level = frame_level; + + /* Will be looked up later by get_seleted_frame. */ + selected_frame = nullptr; +} + int has_stack_frames (void) { @@ -1671,9 +1722,7 @@ has_stack_frames (void) return 1; } -/* Return the selected frame. Always non-NULL (unless there isn't an - inferior sufficient for creating a frame) in which case an error is - thrown. */ +/* See frame.h. */ struct frame_info * get_selected_frame (const char *message) @@ -1682,24 +1731,14 @@ get_selected_frame (const char *message) { if (message != NULL && !has_stack_frames ()) error (("%s"), message); - /* Hey! Don't trust this. It should really be re-finding the - last selected frame of the currently selected thread. This, - though, is better than nothing. */ - select_frame (get_current_frame ()); + + lookup_selected_frame (selected_frame_id, selected_frame_level); } /* There is always a frame. */ gdb_assert (selected_frame != NULL); return selected_frame; } -/* If there is a selected frame, return it. Otherwise, return NULL. */ - -struct frame_info * -get_selected_frame_if_set (void) -{ - return selected_frame; -} - /* This is a variant of get_selected_frame() which can be called when the inferior does not have a frame; in that case it will return NULL instead of calling error(). */ @@ -1712,12 +1751,42 @@ deprecated_safe_get_selected_frame (void) return get_selected_frame (NULL); } -/* Select frame FI (or NULL - to invalidate the current frame). */ +/* Select frame FI (or NULL - to invalidate the selected frame). */ void select_frame (struct frame_info *fi) { selected_frame = fi; + selected_frame_level = frame_relative_level (fi); + if (selected_frame_level == 0) + { + /* Treat the current frame especially -- we want to always + save/restore it without warning, even if the frame ID changes + (see lookup_selected_frame). E.g.: + + // The current frame is selected, the target had just stopped. + { + scoped_restore_selected_frame restore_frame; + some_operation_that_changes_the_stack (); + } + // scoped_restore_selected_frame's dtor runs, but the + // original frame_id can't be found. No matter whether it + // is found or not, we still end up with the now-current + // frame selected. Warning in lookup_selected_frame in this + // case seems pointless. + + Also get_frame_id may access the target's registers/memory, + and thus skipping get_frame_id optimizes the common case. + + Saving the selected frame this way makes get_selected_frame + and restore_current_frame return/re-select whatever frame is + the innermost (current) then. */ + selected_frame_level = -1; + selected_frame_id = null_frame_id; + } + else + selected_frame_id = get_frame_id (fi); + /* NOTE: cagney/2002-05-04: FI can be NULL. This occurs when the frame is being invalidated. */ diff --git a/gdb/frame.h b/gdb/frame.h index e835d49f9c..4901de1bf5 100644 --- a/gdb/frame.h +++ b/gdb/frame.h @@ -181,8 +181,14 @@ class scoped_restore_selected_frame private: - /* The ID of the previously selected frame. */ + /* The ID and level of the previously selected frame. */ struct frame_id m_fid; + int m_level; + + /* Save/restore the language as well, because selecting a frame + changes the current language to the frame's language if "set + language auto". */ + enum language m_lang; }; /* Methods for constructing and comparing Frame IDs. */ @@ -318,24 +324,49 @@ extern int has_stack_frames (void); modifies the target invalidating the frame cache). */ extern void reinit_frame_cache (void); -/* On demand, create the selected frame and then return it. If the - selected frame can not be created, this function prints then throws - an error. When MESSAGE is non-NULL, use it for the error message, - otherwize use a generic error message. */ +/* Return the selected frame. Always returns non-NULL. If there + isn't an inferior sufficient for creating a frame, an error is + thrown. When MESSAGE is non-NULL, use it for the error message, + otherwise use a generic error message. */ /* FIXME: cagney/2002-11-28: At present, when there is no selected frame, this function always returns the current (inner most) frame. It should instead, when a thread has previously had its frame selected (but not resumed) and the frame cache invalidated, find and then return that thread's previously selected frame. */ -extern struct frame_info *get_selected_frame (const char *message); - -/* If there is a selected frame, return it. Otherwise, return NULL. */ -extern struct frame_info *get_selected_frame_if_set (void); +extern struct frame_info *get_selected_frame (const char *message = nullptr); -/* Select a specific frame. NULL, apparently implies re-select the - inner most frame. */ +/* Select a specific frame. NULL implies re-select the inner most + frame. */ extern void select_frame (struct frame_info *); +/* Save the frame ID and frame level of the selected frame in FRAME_ID + and FRAME_LEVEL, to be restored later with restore_selected_frame. + + This is preferred over getting the same info out of + get_selected_frame directly because this function does not create + the selected-frame's frame_info object if it hasn't been created + yet, and thus is more efficient and doesn't throw. */ +extern void save_selected_frame (frame_id *frame_id, int *frame_level) + noexcept; + +/* Restore selected frame as saved with save_selected_frame. + + Does not try to find the corresponding frame_info object. Instead + the next call to get_selected_frame will look it up and cache the + result. + + This function does not throw. It is designed to be safe to called + from the destructors of RAII types. */ +extern void restore_selected_frame (frame_id frame_id, int frame_level) + noexcept; + +/* Lookup the frame_info object for the selected frame FRAME_ID / + FRAME_LEVEL and cache the result. + + If FRAME_LEVEL > 0 and the originally selected frame isn't found, + warn and select the innermost (current) frame. */ +extern void lookup_selected_frame (frame_id frame_id, int frame_level); + /* Given a FRAME, return the next (more inner, younger) or previous (more outer, older) frame. */ extern struct frame_info *get_prev_frame (struct frame_info *); diff --git a/gdb/gdbthread.h b/gdb/gdbthread.h index ab5771fdb4..da20dd4b4e 100644 --- a/gdb/gdbthread.h +++ b/gdb/gdbthread.h @@ -673,6 +673,10 @@ class scoped_restore_current_thread frame_id m_selected_frame_id; int m_selected_frame_level; bool m_was_stopped; + /* Save/restore the language as well, because selecting a frame + changes the current language to the frame's language if "set + language auto". */ + enum language m_lang; }; /* Returns a pointer into the thread_info corresponding to diff --git a/gdb/infrun.c b/gdb/infrun.c index 31266109a6..ad16385029 100644 --- a/gdb/infrun.c +++ b/gdb/infrun.c @@ -9266,8 +9266,10 @@ struct infcall_control_state enum stop_stack_kind stop_stack_dummy = STOP_NONE; int stopped_by_random_signal = 0; - /* ID if the selected frame when the inferior function call was made. */ + /* ID and level of the selected frame when the inferior function + call was made. */ struct frame_id selected_frame_id {}; + int selected_frame_level = -1; }; /* Save all of the information associated with the inferior<==>gdb @@ -9296,27 +9298,12 @@ save_infcall_control_state () inf_status->stop_stack_dummy = stop_stack_dummy; inf_status->stopped_by_random_signal = stopped_by_random_signal; - inf_status->selected_frame_id = get_frame_id (get_selected_frame (NULL)); + save_selected_frame (&inf_status->selected_frame_id, + &inf_status->selected_frame_level); return inf_status; } -static void -restore_selected_frame (const frame_id &fid) -{ - frame_info *frame = frame_find_by_id (fid); - - /* If inf_status->selected_frame_id is NULL, there was no previously - selected frame. */ - if (frame == NULL) - { - warning (_("Unable to restore previously selected frame.")); - return; - } - - select_frame (frame); -} - /* Restore inferior session state to INF_STATUS. */ void @@ -9344,21 +9331,8 @@ restore_infcall_control_state (struct infcall_control_state *inf_status) if (target_has_stack) { - /* The point of the try/catch is that if the stack is clobbered, - walking the stack might encounter a garbage pointer and - error() trying to dereference it. */ - try - { - restore_selected_frame (inf_status->selected_frame_id); - } - catch (const gdb_exception_error &ex) - { - exception_fprintf (gdb_stderr, ex, - "Unable to restore previously selected frame:\n"); - /* Error in restoring the selected frame. Select the - innermost frame. */ - select_frame (get_current_frame ()); - } + restore_selected_frame (inf_status->selected_frame_id, + inf_status->selected_frame_level); } delete inf_status; diff --git a/gdb/stack.c b/gdb/stack.c index 265e764dc2..dbc02f5ff0 100644 --- a/gdb/stack.c +++ b/gdb/stack.c @@ -1836,9 +1836,9 @@ trailing_outermost_frame (int count) static void select_frame_command_core (struct frame_info *fi, bool ignored) { - struct frame_info *prev_frame = get_selected_frame_if_set (); + frame_info *prev_frame = get_selected_frame (); select_frame (fi); - if (get_selected_frame_if_set () != prev_frame) + if (get_selected_frame () != prev_frame) gdb::observers::user_selected_context_changed.notify (USER_SELECTED_FRAME); } @@ -1857,10 +1857,9 @@ select_frame_for_mi (struct frame_info *fi) static void frame_command_core (struct frame_info *fi, bool ignored) { - struct frame_info *prev_frame = get_selected_frame_if_set (); - + frame_info *prev_frame = get_selected_frame (); select_frame (fi); - if (get_selected_frame_if_set () != prev_frame) + if (get_selected_frame () != prev_frame) gdb::observers::user_selected_context_changed.notify (USER_SELECTED_FRAME); else print_selected_thread_frame (current_uiout, USER_SELECTED_FRAME); diff --git a/gdb/thread.c b/gdb/thread.c index 4dce1ef82a..32f2ccdbd6 100644 --- a/gdb/thread.c +++ b/gdb/thread.c @@ -1325,20 +1325,26 @@ switch_to_thread (process_stratum_target *proc_target, ptid_t ptid) switch_to_thread (thr); } -static void -restore_selected_frame (struct frame_id a_frame_id, int frame_level) +/* See frame.h. */ + +void +lookup_selected_frame (struct frame_id a_frame_id, int frame_level) { struct frame_info *frame = NULL; int count; - /* This means there was no selected frame. */ + /* This either means there was no selected frame, or the selected + frame was the current frame. In either case, select the current + frame. */ if (frame_level == -1) { - select_frame (NULL); + select_frame (get_current_frame ()); return; } - gdb_assert (frame_level >= 0); + /* select_frame never saves 0 in SELECTED_FRAME_LEVEL, so we + shouldn't see it here. */ + gdb_assert (frame_level > 0); /* Restore by level first, check if the frame id is the same as expected. If that fails, try restoring by frame id. If that @@ -1409,64 +1415,28 @@ scoped_restore_current_thread::restore () && target_has_stack && target_has_memory) restore_selected_frame (m_selected_frame_id, m_selected_frame_level); + + set_language (m_lang); } scoped_restore_current_thread::~scoped_restore_current_thread () { if (!m_dont_restore) - { - try - { - restore (); - } - catch (const gdb_exception &ex) - { - /* We're in a dtor, there's really nothing else we can do - but swallow the exception. */ - } - } + restore (); } scoped_restore_current_thread::scoped_restore_current_thread () { m_inf = inferior_ref::new_reference (current_inferior ()); + m_lang = current_language->la_language; + if (inferior_ptid != null_ptid) { m_thread = thread_info_ref::new_reference (inferior_thread ()); - struct frame_info *frame; - m_was_stopped = m_thread->state == THREAD_STOPPED; - if (m_was_stopped - && target_has_registers - && target_has_stack - && target_has_memory) - { - /* When processing internal events, there might not be a - selected frame. If we naively call get_selected_frame - here, then we can end up reading debuginfo for the - current frame, but we don't generally need the debuginfo - at this point. */ - frame = get_selected_frame_if_set (); - } - else - frame = NULL; - - try - { - m_selected_frame_id = get_frame_id (frame); - m_selected_frame_level = frame_relative_level (frame); - } - catch (const gdb_exception_error &ex) - { - m_selected_frame_id = null_frame_id; - m_selected_frame_level = -1; - - /* Better let this propagate. */ - if (ex.error == TARGET_CLOSE_ERROR) - throw; - } + save_selected_frame (&m_selected_frame_id, &m_selected_frame_level); } } base-commit: ad8464f799a4c96c7ab8bdfec3f95846cf54f9b0 prerequisite-patch-id: 32ffdda7d7d774bc4df88bf848bcb796559b53ce prerequisite-patch-id: 02021b74355b70debd344a6e445285c67dfef7d6 prerequisite-patch-id: c87fcf5a54f6805967cbf8ab107606c57d9ecf52 prerequisite-patch-id: ac7dee583d0ffa519c9d1cd89d27664bca68d8c1 prerequisite-patch-id: eac59ae2ea85d2d51e5be1b03e88a5641cc12c22 prerequisite-patch-id: 13da42ad04dc8e2e3bd6a556a0be0e17cf23669b prerequisite-patch-id: fd3f09fdb58ddc1c595ea014716851f4c8fca48c prerequisite-patch-id: 8230262cb24020a5b37e915843180e940807ba1f prerequisite-patch-id: b06932368faf983af6404ae711dfed17e5e32c6f -- 2.14.5 ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 3/3] Make scoped_restore_current_thread's cdtors exception free (RFC) 2020-07-09 22:22 ` Pedro Alves @ 2020-07-10 2:55 ` Simon Marchi 2020-10-30 1:13 ` Pedro Alves 0 siblings, 1 reply; 31+ messages in thread From: Simon Marchi @ 2020-07-10 2:55 UTC (permalink / raw) To: Pedro Alves, gdb-patches >> I don't know if it would be worth it, but I'd like if we could assert (abort >> GDB) if an exception does try to exit the destructor. The `restore` method >> is non-trivial and calls into other non-trivial functions, so it would be >> possible for a change far far away to cause that to happen. > > It will already abort. Destructors are noexcept by default, so if an exception > escapes a destructor, std::terminate() is called, and that calls abort by default. Oh, didn't know that! I thought it was just "undefined behavior". >> What do you think of keeping the try/catch, but using `gdb_assert_not_reached` >> in it? > > Not sure. If we do that, we do get a nicer error message. However if the user > says "n" to "Quit this debugging session" we still abort. > > /home/pedro/brno/pedro/gdb/binutils-gdb-2/build/../src/gdb/thread.c:1441: internal-error: scoped_restore_current_thread::~scoped_restore_current_thread(): unexpected exception thrown from destructor: hello > A problem internal to GDB has been detected, > further debugging may prove unreliable. > Quit this debugging session? (y or n) n > > This is a bug, please report it. For instructions, see: > <https://www.gnu.org/software/gdb/bugs/>. > > /home/pedro/brno/pedro/gdb/binutils-gdb-2/build/../src/gdb/thread.c:1441: internal-error: scoped_restore_current_thread::~scoped_restore_current_thread(): unexpected exception thrown from destructor: hello > A problem internal to GDB has been detected, > further debugging may prove unreliable. > Create a core file of GDB? (y or n) n > terminate called after throwing an instance of 'gdb_exception_quit' > Aborted (core dumped) > > Maybe it would be interesting to add a variant of internal_error that did > not throw a quit, so the user could swallow the exception... Maybe consider > wrapping that as a generic facility to add to all non-trivial RAII destructors > we have? Like a function that takes a function_view as parameter, so > we would write: > > foo::~foo () > { > safe_dtor (__FILE__, __LINE__, [&] () > { > restore (); > }); > } > > Even better, add a SAFE_DTOR macro using similar magic SCOPE_EXIT > macro uses to be able to write: > > foo::~foo () > { > SAFE_DTOR { restore (); }; > } That's fancier than what I hoped for :). My goal was just to make sure we catch it if we ever make a change that causes an exception to escape. Although I wouldn't be against what you proposed. > Here's the current version of the patch. That looks fine to me. Simon ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 3/3] Make scoped_restore_current_thread's cdtors exception free (RFC) 2020-07-10 2:55 ` Simon Marchi @ 2020-10-30 1:13 ` Pedro Alves 2020-10-30 1:37 ` [pushed] Move lookup_selected_frame to frame.c Pedro Alves 2020-10-30 7:44 ` [PATCH 3/3] Make scoped_restore_current_thread's cdtors exception free (RFC) Aktemur, Tankut Baris 0 siblings, 2 replies; 31+ messages in thread From: Pedro Alves @ 2020-10-30 1:13 UTC (permalink / raw) To: Simon Marchi, gdb-patches On 7/10/20 3:55 AM, Simon Marchi wrote: >>> I don't know if it would be worth it, but I'd like if we could assert (abort >>> GDB) if an exception does try to exit the destructor. The `restore` method >>> is non-trivial and calls into other non-trivial functions, so it would be >>> possible for a change far far away to cause that to happen. >> >> It will already abort. Destructors are noexcept by default, so if an exception >> escapes a destructor, std::terminate() is called, and that calls abort by default. > > Oh, didn't know that! I thought it was just "undefined behavior". > >>> What do you think of keeping the try/catch, but using `gdb_assert_not_reached` >>> in it? >> >> Not sure. If we do that, we do get a nicer error message. However if the user >> says "n" to "Quit this debugging session" we still abort. >> >> /home/pedro/brno/pedro/gdb/binutils-gdb-2/build/../src/gdb/thread.c:1441: internal-error: scoped_restore_current_thread::~scoped_restore_current_thread(): unexpected exception thrown from destructor: hello >> A problem internal to GDB has been detected, >> further debugging may prove unreliable. >> Quit this debugging session? (y or n) n >> >> This is a bug, please report it. For instructions, see: >> <https://www.gnu.org/software/gdb/bugs/>. >> >> /home/pedro/brno/pedro/gdb/binutils-gdb-2/build/../src/gdb/thread.c:1441: internal-error: scoped_restore_current_thread::~scoped_restore_current_thread(): unexpected exception thrown from destructor: hello >> A problem internal to GDB has been detected, >> further debugging may prove unreliable. >> Create a core file of GDB? (y or n) n >> terminate called after throwing an instance of 'gdb_exception_quit' >> Aborted (core dumped) >> >> Maybe it would be interesting to add a variant of internal_error that did >> not throw a quit, so the user could swallow the exception... Maybe consider >> wrapping that as a generic facility to add to all non-trivial RAII destructors >> we have? Like a function that takes a function_view as parameter, so >> we would write: >> >> foo::~foo () >> { >> safe_dtor (__FILE__, __LINE__, [&] () >> { >> restore (); >> }); >> } >> >> Even better, add a SAFE_DTOR macro using similar magic SCOPE_EXIT >> macro uses to be able to write: >> >> foo::~foo () >> { >> SAFE_DTOR { restore (); }; >> } > > That's fancier than what I hoped for :). My goal was just to make sure > we catch it if we ever make a change that causes an exception to escape. > Although I wouldn't be against what you proposed. > >> Here's the current version of the patch. > > That looks fine to me. FYI, I've finally merged this to master. Thanks, Pedro Alves ^ permalink raw reply [flat|nested] 31+ messages in thread
* [pushed] Move lookup_selected_frame to frame.c 2020-10-30 1:13 ` Pedro Alves @ 2020-10-30 1:37 ` Pedro Alves 2020-10-30 7:44 ` [PATCH 3/3] Make scoped_restore_current_thread's cdtors exception free (RFC) Aktemur, Tankut Baris 1 sibling, 0 replies; 31+ messages in thread From: Pedro Alves @ 2020-10-30 1:37 UTC (permalink / raw) To: Simon Marchi, gdb-patches On 10/30/20 1:13 AM, Pedro Alves wrote: > On 7/10/20 3:55 AM, Simon Marchi wrote: >>>> I don't know if it would be worth it, but I'd like if we could assert (abort >>>> GDB) if an exception does try to exit the destructor. The `restore` method >>>> is non-trivial and calls into other non-trivial functions, so it would be >>>> possible for a change far far away to cause that to happen. >>> >>> It will already abort. Destructors are noexcept by default, so if an exception >>> escapes a destructor, std::terminate() is called, and that calls abort by default. >> >> Oh, didn't know that! I thought it was just "undefined behavior". >> >>>> What do you think of keeping the try/catch, but using `gdb_assert_not_reached` >>>> in it? >>> >>> Not sure. If we do that, we do get a nicer error message. However if the user >>> says "n" to "Quit this debugging session" we still abort. >>> >>> /home/pedro/brno/pedro/gdb/binutils-gdb-2/build/../src/gdb/thread.c:1441: internal-error: scoped_restore_current_thread::~scoped_restore_current_thread(): unexpected exception thrown from destructor: hello >>> A problem internal to GDB has been detected, >>> further debugging may prove unreliable. >>> Quit this debugging session? (y or n) n >>> >>> This is a bug, please report it. For instructions, see: >>> <https://www.gnu.org/software/gdb/bugs/>. >>> >>> /home/pedro/brno/pedro/gdb/binutils-gdb-2/build/../src/gdb/thread.c:1441: internal-error: scoped_restore_current_thread::~scoped_restore_current_thread(): unexpected exception thrown from destructor: hello >>> A problem internal to GDB has been detected, >>> further debugging may prove unreliable. >>> Create a core file of GDB? (y or n) n >>> terminate called after throwing an instance of 'gdb_exception_quit' >>> Aborted (core dumped) >>> >>> Maybe it would be interesting to add a variant of internal_error that did >>> not throw a quit, so the user could swallow the exception... Maybe consider >>> wrapping that as a generic facility to add to all non-trivial RAII destructors >>> we have? Like a function that takes a function_view as parameter, so >>> we would write: >>> >>> foo::~foo () >>> { >>> safe_dtor (__FILE__, __LINE__, [&] () >>> { >>> restore (); >>> }); >>> } >>> >>> Even better, add a SAFE_DTOR macro using similar magic SCOPE_EXIT >>> macro uses to be able to write: >>> >>> foo::~foo () >>> { >>> SAFE_DTOR { restore (); }; >>> } >> >> That's fancier than what I hoped for :). My goal was just to make sure >> we catch it if we ever make a change that causes an exception to escape. >> Although I wouldn't be against what you proposed. >> >>> Here's the current version of the patch. >> >> That looks fine to me. > > FYI, I've finally merged this to master. > I've merged this obvious follow up patch as well. From d70bdd3cc4cfdfd30bed44a271ff80f98b96eebf Mon Sep 17 00:00:00 2001 From: Pedro Alves <palves@redhat.com> Date: Thu, 9 Jul 2020 20:12:15 +0100 Subject: [PATCH] Move lookup_selected_frame to frame.c This function is now external, and isn't really threads related. Move it to frame.c. gdb/ChangeLog: * thread.c (lookup_selected_frame): Move ... * frame.c (lookup_selected_frame): ... here. Change-Id: Ia96b79c15767337c68efd3358bcc715ce8e26c15 --- gdb/ChangeLog | 5 +++++ gdb/frame.c | 66 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ gdb/thread.c | 63 -------------------------------------------------------- 3 files changed, 71 insertions(+), 63 deletions(-) diff --git a/gdb/ChangeLog b/gdb/ChangeLog index 978576ac7c..d2df843b24 100644 --- a/gdb/ChangeLog +++ b/gdb/ChangeLog @@ -1,3 +1,8 @@ +2020-10-30 Pedro Alves <pedro@palves.net> + + * thread.c (lookup_selected_frame): Move ... + * frame.c (lookup_selected_frame): ... here. + 2020-10-30 Pedro Alves <pedro@palves.net> * blockframe.c (block_innermost_frame): Use get_selected_frame. diff --git a/gdb/frame.c b/gdb/frame.c index bb835e2dba..d0a4ce4d63 100644 --- a/gdb/frame.c +++ b/gdb/frame.c @@ -1740,6 +1740,72 @@ restore_selected_frame (frame_id frame_id, int frame_level) selected_frame = nullptr; } +/* See frame.h. */ + +void +lookup_selected_frame (struct frame_id a_frame_id, int frame_level) +{ + struct frame_info *frame = NULL; + int count; + + /* This either means there was no selected frame, or the selected + frame was the current frame. In either case, select the current + frame. */ + if (frame_level == -1) + { + select_frame (get_current_frame ()); + return; + } + + /* select_frame never saves 0 in SELECTED_FRAME_LEVEL, so we + shouldn't see it here. */ + gdb_assert (frame_level > 0); + + /* Restore by level first, check if the frame id is the same as + expected. If that fails, try restoring by frame id. If that + fails, nothing to do, just warn the user. */ + + count = frame_level; + frame = find_relative_frame (get_current_frame (), &count); + if (count == 0 + && frame != NULL + /* The frame ids must match - either both valid or both + outer_frame_id. The latter case is not failsafe, but since + it's highly unlikely the search by level finds the wrong + frame, it's 99.9(9)% of the time (for all practical purposes) + safe. */ + && frame_id_eq (get_frame_id (frame), a_frame_id)) + { + /* Cool, all is fine. */ + select_frame (frame); + return; + } + + frame = frame_find_by_id (a_frame_id); + if (frame != NULL) + { + /* Cool, refound it. */ + select_frame (frame); + return; + } + + /* Nothing else to do, the frame layout really changed. Select the + innermost stack frame. */ + select_frame (get_current_frame ()); + + /* Warn the user. */ + if (frame_level > 0 && !current_uiout->is_mi_like_p ()) + { + warning (_("Couldn't restore frame #%d in " + "current thread. Bottom (innermost) frame selected:"), + frame_level); + /* For MI, we should probably have a notification about current + frame change. But this error is not very likely, so don't + bother for now. */ + print_stack_frame (get_selected_frame (NULL), 1, SRC_AND_LOC, 1); + } +} + bool has_stack_frames () { diff --git a/gdb/thread.c b/gdb/thread.c index 193f9d4c44..32d14e8662 100644 --- a/gdb/thread.c +++ b/gdb/thread.c @@ -1327,69 +1327,6 @@ switch_to_thread (process_stratum_target *proc_target, ptid_t ptid) /* See frame.h. */ -void -lookup_selected_frame (struct frame_id a_frame_id, int frame_level) -{ - struct frame_info *frame = NULL; - int count; - - /* This either means there was no selected frame, or the selected - frame was the current frame. In either case, select the current - frame. */ - if (frame_level == -1) - { - select_frame (get_current_frame ()); - return; - } - - /* select_frame never saves 0 in SELECTED_FRAME_LEVEL, so we - shouldn't see it here. */ - gdb_assert (frame_level > 0); - - /* Restore by level first, check if the frame id is the same as - expected. If that fails, try restoring by frame id. If that - fails, nothing to do, just warn the user. */ - - count = frame_level; - frame = find_relative_frame (get_current_frame (), &count); - if (count == 0 - && frame != NULL - /* The frame ids must match - either both valid or both outer_frame_id. - The latter case is not failsafe, but since it's highly unlikely - the search by level finds the wrong frame, it's 99.9(9)% of - the time (for all practical purposes) safe. */ - && frame_id_eq (get_frame_id (frame), a_frame_id)) - { - /* Cool, all is fine. */ - select_frame (frame); - return; - } - - frame = frame_find_by_id (a_frame_id); - if (frame != NULL) - { - /* Cool, refound it. */ - select_frame (frame); - return; - } - - /* Nothing else to do, the frame layout really changed. Select the - innermost stack frame. */ - select_frame (get_current_frame ()); - - /* Warn the user. */ - if (frame_level > 0 && !current_uiout->is_mi_like_p ()) - { - warning (_("Couldn't restore frame #%d in " - "current thread. Bottom (innermost) frame selected:"), - frame_level); - /* For MI, we should probably have a notification about - current frame change. But this error is not very - likely, so don't bother for now. */ - print_stack_frame (get_selected_frame (NULL), 1, SRC_AND_LOC, 1); - } -} - void scoped_restore_current_thread::restore () { base-commit: 79952e69634bd02029e79676a38915de60052e89 -- 2.14.5 ^ permalink raw reply [flat|nested] 31+ messages in thread
* RE: [PATCH 3/3] Make scoped_restore_current_thread's cdtors exception free (RFC) 2020-10-30 1:13 ` Pedro Alves 2020-10-30 1:37 ` [pushed] Move lookup_selected_frame to frame.c Pedro Alves @ 2020-10-30 7:44 ` Aktemur, Tankut Baris 2020-10-30 11:32 ` Pedro Alves 1 sibling, 1 reply; 31+ messages in thread From: Aktemur, Tankut Baris @ 2020-10-30 7:44 UTC (permalink / raw) To: Pedro Alves, Simon Marchi, gdb-patches On Friday, October 30, 2020 2:14 AM, Pedro Alves wrote: > On 7/10/20 3:55 AM, Simon Marchi wrote: > >>> I don't know if it would be worth it, but I'd like if we could assert (abort > >>> GDB) if an exception does try to exit the destructor. The `restore` method > >>> is non-trivial and calls into other non-trivial functions, so it would be > >>> possible for a change far far away to cause that to happen. > >> > >> It will already abort. Destructors are noexcept by default, so if an exception > >> escapes a destructor, std::terminate() is called, and that calls abort by default. > > > > Oh, didn't know that! I thought it was just "undefined behavior". > > > >>> What do you think of keeping the try/catch, but using `gdb_assert_not_reached` > >>> in it? > >> > >> Not sure. If we do that, we do get a nicer error message. However if the user > >> says "n" to "Quit this debugging session" we still abort. > >> > >> /home/pedro/brno/pedro/gdb/binutils-gdb-2/build/../src/gdb/thread.c:1441: internal-error: > scoped_restore_current_thread::~scoped_restore_current_thread(): unexpected exception thrown > from destructor: hello > >> A problem internal to GDB has been detected, > >> further debugging may prove unreliable. > >> Quit this debugging session? (y or n) n > >> > >> This is a bug, please report it. For instructions, see: > >> <https://www.gnu.org/software/gdb/bugs/>. > >> > >> /home/pedro/brno/pedro/gdb/binutils-gdb-2/build/../src/gdb/thread.c:1441: internal-error: > scoped_restore_current_thread::~scoped_restore_current_thread(): unexpected exception thrown > from destructor: hello > >> A problem internal to GDB has been detected, > >> further debugging may prove unreliable. > >> Create a core file of GDB? (y or n) n > >> terminate called after throwing an instance of 'gdb_exception_quit' > >> Aborted (core dumped) > >> > >> Maybe it would be interesting to add a variant of internal_error that did > >> not throw a quit, so the user could swallow the exception... Maybe consider > >> wrapping that as a generic facility to add to all non-trivial RAII destructors > >> we have? Like a function that takes a function_view as parameter, so > >> we would write: > >> > >> foo::~foo () > >> { > >> safe_dtor (__FILE__, __LINE__, [&] () > >> { > >> restore (); > >> }); > >> } > >> > >> Even better, add a SAFE_DTOR macro using similar magic SCOPE_EXIT > >> macro uses to be able to write: > >> > >> foo::~foo () > >> { > >> SAFE_DTOR { restore (); }; > >> } > > > > That's fancier than what I hoped for :). My goal was just to make sure > > we catch it if we ever make a change that causes an exception to escape. > > Although I wouldn't be against what you proposed. > > > >> Here's the current version of the patch. > > > > That looks fine to me. > > FYI, I've finally merged this to master. > > Thanks, > Pedro Alves Hi Pedro, This patch created the regression below: Breakpoint 1, 0x00000000004012bb in eh2 (/gdb-up/gdb/frame.c:641: internal-error: frame_id get_frame_id(frame_info*): Assertion `stashed' failed. A problem internal to GDB has been detected, further debugging may prove unreliable. Quit this debugging session? (y or n) FAIL: gdb.base/eh_return.exp: hit breakpoint (GDB internal error) -Baris Intel Deutschland GmbH Registered Address: Am Campeon 10-12, 85579 Neubiberg, Germany Tel: +49 89 99 8853-0, www.intel.de Managing Directors: Christin Eisenschmid, Gary Kershaw Chairperson of the Supervisory Board: Nicole Lau Registered Office: Munich Commercial Register: Amtsgericht Muenchen HRB 186928 ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 3/3] Make scoped_restore_current_thread's cdtors exception free (RFC) 2020-10-30 7:44 ` [PATCH 3/3] Make scoped_restore_current_thread's cdtors exception free (RFC) Aktemur, Tankut Baris @ 2020-10-30 11:32 ` Pedro Alves 2020-10-31 14:35 ` [PATCH] Fix frame cycle detection (Re: [PATCH 3/3] Make scoped_restore_current_thread's cdtors exception free (RFC)) Pedro Alves 0 siblings, 1 reply; 31+ messages in thread From: Pedro Alves @ 2020-10-30 11:32 UTC (permalink / raw) To: Aktemur, Tankut Baris, Simon Marchi, gdb-patches Hi! On 10/30/20 7:44 AM, Aktemur, Tankut Baris wrote: > Hi Pedro, > > This patch created the regression below: > > Breakpoint 1, 0x00000000004012bb in eh2 (/gdb-up/gdb/frame.c:641: internal-error: frame_id get_frame_id(frame_info*): Assertion `stashed' failed. > A problem internal to GDB has been detected, > further debugging may prove unreliable. > Quit this debugging session? (y or n) FAIL: gdb.base/eh_return.exp: hit breakpoint (GDB internal error) Hmm, sorry about that. It passes for me on the machine I was running tests on, so I missed it. I can reproduce it on a F27 machine. I'll take a better look. ^ permalink raw reply [flat|nested] 31+ messages in thread
* [PATCH] Fix frame cycle detection (Re: [PATCH 3/3] Make scoped_restore_current_thread's cdtors exception free (RFC)) 2020-10-30 11:32 ` Pedro Alves @ 2020-10-31 14:35 ` Pedro Alves 2020-11-09 14:05 ` Aktemur, Tankut Baris 0 siblings, 1 reply; 31+ messages in thread From: Pedro Alves @ 2020-10-31 14:35 UTC (permalink / raw) To: Aktemur, Tankut Baris, Simon Marchi, gdb-patches On 10/30/20 11:32 AM, Pedro Alves wrote: > Hi! > > On 10/30/20 7:44 AM, Aktemur, Tankut Baris wrote: > >> Hi Pedro, >> >> This patch created the regression below: >> >> Breakpoint 1, 0x00000000004012bb in eh2 (/gdb-up/gdb/frame.c:641: internal-error: frame_id get_frame_id(frame_info*): Assertion `stashed' failed. >> A problem internal to GDB has been detected, >> further debugging may prove unreliable. >> Quit this debugging session? (y or n) FAIL: gdb.base/eh_return.exp: hit breakpoint (GDB internal error) > > Hmm, sorry about that. It passes for me on the machine I was > running tests on, so I missed it. I can reproduce it on a F27 machine. > I'll take a better look. > Here's a fix. See explanation within. From abbc44629325ad4300cdc7a09c235f4817678693 Mon Sep 17 00:00:00 2001 From: Pedro Alves <pedro@palves.net> Date: Sat, 31 Oct 2020 00:27:18 +0000 Subject: [PATCH] Fix frame cycle detection The recent commit to make scoped_restore_current_thread's cdtors exception free regressed gdb.base/eh_return.exp: Breakpoint 1, 0x00000000004012bb in eh2 (gdb/frame.c:641: internal-error: frame_id get_frame_id(frame_info*): Assertion `stashed' failed. A problem internal to GDB has been detected, further debugging may prove unreliable. Quit this debugging session? (y or n) FAIL: gdb.base/eh_return.exp: hit breakpoint (GDB internal error) That testcase uses __builtin_eh_return and, before the regression, the backtrace at eh2 looked like this: (gdb) bt #0 0x00000000004006eb in eh2 (p=0x4006ec <continuation>) at src/gdb/testsuite/gdb.base/eh_return.c:54 Backtrace stopped: previous frame identical to this frame (corrupt stack?) That "previous frame identical to this frame" is caught by the cycle detection based on frame id. The assertion failing is this one: 638 /* Since this is the first frame in the chain, this should 639 always succeed. */ 640 bool stashed = frame_stash_add (fi); 641 gdb_assert (stashed); originally added by commit f245535cf583ae4ca13b10d47b3c7d3334593ece Author: Pedro Alves <palves@redhat.com> AuthorDate: Mon Sep 5 18:41:38 2016 +0100 Fix PR19927: Avoid unwinder recursion if sniffer uses calls parse_and_eval The assertion is failing because frame #1's frame id was stashed before the id of frame #0 is stashed. The frame id of frame #1 was stashed here: (top-gdb) bt #0 frame_stash_add (frame=0x1e24c90) at src/gdb/frame.c:276 #1 0x0000000000669c1b in get_prev_frame_if_no_cycle (this_frame=0x19f8370) at src/gdb/frame.c:2120 #2 0x000000000066a339 in get_prev_frame_always_1 (this_frame=0x19f8370) at src/gdb/frame.c:2303 #3 0x000000000066a360 in get_prev_frame_always (this_frame=0x19f8370) at src/gdb/frame.c:2319 #4 0x000000000066b56c in get_frame_unwind_stop_reason (frame=0x19f8370) at src/gdb/frame.c:3028 #5 0x000000000059f929 in dwarf2_frame_cfa (this_frame=0x19f8370) at src/gdb/dwarf2/frame.c:1462 #6 0x00000000005ce434 in dwarf_evaluate_loc_desc::get_frame_cfa (this=0x7fffffffc070) at src/gdb/dwarf2/loc.c:666 #7 0x00000000005989a9 in dwarf_expr_context::execute_stack_op (this=0x7fffffffc070, op_ptr=0x1b2a053 "\364\003", op_end=0x1b2a053 "\364\003") at src/gdb/dwarf2/expr.c:1161 #8 0x0000000000596af6 in dwarf_expr_context::eval (this=0x7fffffffc070, addr=0x1b2a052 "\234\364\003", len=1) at src/gdb/dwarf2/expr.c:303 #9 0x0000000000597b4e in dwarf_expr_context::execute_stack_op (this=0x7fffffffc070, op_ptr=0x1b2a063 "", op_end=0x1b2a063 "") at src/gdb/dwarf2/expr.c:865 #10 0x0000000000596af6 in dwarf_expr_context::eval (this=0x7fffffffc070, addr=0x1b2a061 "\221X", len=2) at src/gdb/dwarf2/expr.c:303 #11 0x00000000005c8b5a in dwarf2_evaluate_loc_desc_full (type=0x1b564d0, frame=0x19f8370, data=0x1b2a061 "\221X", size=2, per_cu=0x1b28760, per_objfile=0x1a84930, subobj_type=0x1b564d0, subobj_byte_offset=0) at src/gdb/dwarf2/loc.c:2260 #12 0x00000000005c9243 in dwarf2_evaluate_loc_desc (type=0x1b564d0, frame=0x19f8370, data=0x1b2a061 "\221X", size=2, per_cu=0x1b28760, per_objfile=0x1a84930) at src/gdb/dwarf2/loc.c:2444 #13 0x00000000005cb769 in locexpr_read_variable (symbol=0x1b59840, frame=0x19f8370) at src/gdb/dwarf2/loc.c:3687 #14 0x0000000000663137 in language_defn::read_var_value (this=0x122ea60 <c_language_defn>, var=0x1b59840, var_block=0x0, frame=0x19f8370) at src/gdb/findvar.c:618 #15 0x0000000000663c3b in read_var_value (var=0x1b59840, var_block=0x0, frame=0x19f8370) at src/gdb/findvar.c:822 #16 0x00000000008c7d9f in read_frame_arg (fp_opts=..., sym=0x1b59840, frame=0x19f8370, argp=0x7fffffffc470, entryargp=0x7fffffffc490) at src/gdb/stack.c:542 #17 0x00000000008c89cd in print_frame_args (fp_opts=..., func=0x1b597c0, frame=0x19f8370, num=-1, stream=0x1aba860) at src/gdb/stack.c:890 #18 0x00000000008c9bf8 in print_frame (fp_opts=..., frame=0x19f8370, print_level=0, print_what=SRC_AND_LOC, print_args=1, sal=...) at src/gdb/stack.c:1394 #19 0x00000000008c92b9 in print_frame_info (fp_opts=..., frame=0x19f8370, print_level=0, print_what=SRC_AND_LOC, print_args=1, set_current_sal=1) at src/gdb/stack.c:1119 #20 0x00000000008c75f0 in print_stack_frame (frame=0x19f8370, print_level=0, print_what=SRC_AND_LOC, set_current_sal=1) at src/gdb/stack.c:366 #21 0x000000000070250b in print_stop_location (ws=0x7fffffffc9e0) at src/gdb/infrun.c:8110 #22 0x0000000000702569 in print_stop_event (uiout=0x1a8b9e0, displays=true) at src/gdb/infrun.c:8126 #23 0x000000000096d04b in tui_on_normal_stop (bs=0x1bcd1c0, print_frame=1) at src/gdb/tui/tui-interp.c:98 ... Before the commit to make scoped_restore_current_thread's cdtors exception free, scoped_restore_current_thread's dtor would call get_frame_id on the selected frame, and we use scoped_restore_current_thread pervasively. That had the side effect of stashing the frame id of frame #0 before reaching the path shown in the backtrace. I.e., the frame id of frame #0 happened to be stashed before the frame id of frame #1. But that was by chance, not by design. This commit: commit 256ae5dbc73d1348850f86ee77a0dc3b04bc7cc0 Author: Kevin Buettner <kevinb@redhat.com> AuthorDate: Mon Oct 31 12:47:42 2016 -0700 Stash frame id of current frame before stashing frame id for previous frame Fixed a similar problem, by making sure get_prev_frame computes the frame id of the current frame before unwinding the previous frame, so that the cycle detection works properly. That fix misses the scenario we're now running against, because if you notice, the backtrace above shows that frame #4 calls get_prev_frame_always, not get_prev_frame. I.e., nothing is calling get_frame_id on the current frame. The fix here is to move Kevin's fix down from get_prev_frame to get_prev_frame_always. Or actually, a bit further down to get_prev_frame_always_1 -- note that inline_frame_this_id calls get_prev_frame_always, so we need to be careful to avoid recursion in that scenario. gdb/ChangeLog: * frame.c (get_prev_frame): Move get_frame_id call from here ... (get_prev_frame_always_1): ... to here. * inline-frame.c (inline_frame_this_id): Mention get_prev_frame_always_1 in comment. Change-Id: Id960c98ab2d072c48a436c3eb160cc4b2a5cfd1d --- gdb/frame.c | 27 +++++++++++++++++---------- gdb/inline-frame.c | 3 ++- 2 files changed, 19 insertions(+), 11 deletions(-) diff --git a/gdb/frame.c b/gdb/frame.c index d0a4ce4d63..1436934261 100644 --- a/gdb/frame.c +++ b/gdb/frame.c @@ -2200,6 +2200,23 @@ get_prev_frame_always_1 (struct frame_info *this_frame) if (get_frame_type (this_frame) == INLINE_FRAME) return get_prev_frame_if_no_cycle (this_frame); + /* If this_frame is the current frame, then compute and stash its + frame id prior to fetching and computing the frame id of the + previous frame. Otherwise, the cycle detection code in + get_prev_frame_if_no_cycle() will not work correctly. When + get_frame_id() is called later on, an assertion error will be + triggered in the event of a cycle between the current frame and + its previous frame. + + Note we do this after the INLINE_FRAME check above. That is + because the inline frame's frame id computation needs to fetch + the frame id of its previous real stack frame. I.e., we need to + avoid recursion in that case. This is OK since we're sure the + inline frame won't create a cycle with the real stack frame. See + inline_frame_this_id. */ + if (this_frame->level == 0) + get_frame_id (this_frame); + /* Check that this frame is unwindable. If it isn't, don't try to unwind to the prev frame. */ this_frame->stop_reason @@ -2492,16 +2509,6 @@ get_prev_frame (struct frame_info *this_frame) something should be calling get_selected_frame() or get_current_frame(). */ gdb_assert (this_frame != NULL); - - /* If this_frame is the current frame, then compute and stash - its frame id prior to fetching and computing the frame id of the - previous frame. Otherwise, the cycle detection code in - get_prev_frame_if_no_cycle() will not work correctly. When - get_frame_id() is called later on, an assertion error will - be triggered in the event of a cycle between the current - frame and its previous frame. */ - if (this_frame->level == 0) - get_frame_id (this_frame); frame_pc_p = get_frame_pc_if_available (this_frame, &frame_pc); diff --git a/gdb/inline-frame.c b/gdb/inline-frame.c index 300b1224db..92a7d562ea 100644 --- a/gdb/inline-frame.c +++ b/gdb/inline-frame.c @@ -161,7 +161,8 @@ inline_frame_this_id (struct frame_info *this_frame, real frame's this_id method. So we must call get_prev_frame_always. Because we are inlined into some function, there must be previous frames, so this is safe - as - long as we're careful not to create any cycles. */ + long as we're careful not to create any cycles. See related + comments in get_prev_frame_always_1. */ *this_id = get_frame_id (get_prev_frame_always (this_frame)); /* We need a valid frame ID, so we need to be based on a valid base-commit: 17417fb0ec9842de1774e1e76f1f11c00cdafc47 -- 2.14.5 ^ permalink raw reply [flat|nested] 31+ messages in thread
* RE: [PATCH] Fix frame cycle detection (Re: [PATCH 3/3] Make scoped_restore_current_thread's cdtors exception free (RFC)) 2020-10-31 14:35 ` [PATCH] Fix frame cycle detection (Re: [PATCH 3/3] Make scoped_restore_current_thread's cdtors exception free (RFC)) Pedro Alves @ 2020-11-09 14:05 ` Aktemur, Tankut Baris 2020-11-16 13:48 ` Tom de Vries 0 siblings, 1 reply; 31+ messages in thread From: Aktemur, Tankut Baris @ 2020-11-09 14:05 UTC (permalink / raw) To: Pedro Alves, Simon Marchi, gdb-patches On Saturday, October 31, 2020 3:35 PM, Pedro Alves wrote: > On 10/30/20 11:32 AM, Pedro Alves wrote: > > Hi! > > > > On 10/30/20 7:44 AM, Aktemur, Tankut Baris wrote: > > > >> Hi Pedro, > >> > >> This patch created the regression below: > >> > >> Breakpoint 1, 0x00000000004012bb in eh2 (/gdb-up/gdb/frame.c:641: internal-error: > frame_id get_frame_id(frame_info*): Assertion `stashed' failed. > >> A problem internal to GDB has been detected, > >> further debugging may prove unreliable. > >> Quit this debugging session? (y or n) FAIL: gdb.base/eh_return.exp: hit breakpoint (GDB > internal error) > > > > Hmm, sorry about that. It passes for me on the machine I was > > running tests on, so I missed it. I can reproduce it on a F27 machine. > > I'll take a better look. > > > > Here's a fix. See explanation within. Thanks. This fixed the regression. -Baris Intel Deutschland GmbH Registered Address: Am Campeon 10-12, 85579 Neubiberg, Germany Tel: +49 89 99 8853-0, www.intel.de Managing Directors: Christin Eisenschmid, Gary Kershaw Chairperson of the Supervisory Board: Nicole Lau Registered Office: Munich Commercial Register: Amtsgericht Muenchen HRB 186928 ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH] Fix frame cycle detection (Re: [PATCH 3/3] Make scoped_restore_current_thread's cdtors exception free (RFC)) 2020-11-09 14:05 ` Aktemur, Tankut Baris @ 2020-11-16 13:48 ` Tom de Vries 2020-11-16 14:57 ` Pedro Alves 0 siblings, 1 reply; 31+ messages in thread From: Tom de Vries @ 2020-11-16 13:48 UTC (permalink / raw) To: Aktemur, Tankut Baris, Pedro Alves, Simon Marchi, gdb-patches On 11/9/20 3:05 PM, Aktemur, Tankut Baris via Gdb-patches wrote: > On Saturday, October 31, 2020 3:35 PM, Pedro Alves wrote: >> On 10/30/20 11:32 AM, Pedro Alves wrote: >>> Hi! >>> >>> On 10/30/20 7:44 AM, Aktemur, Tankut Baris wrote: >>> >>>> Hi Pedro, >>>> >>>> This patch created the regression below: >>>> >>>> Breakpoint 1, 0x00000000004012bb in eh2 (/gdb-up/gdb/frame.c:641: internal-error: >> frame_id get_frame_id(frame_info*): Assertion `stashed' failed. >>>> A problem internal to GDB has been detected, >>>> further debugging may prove unreliable. >>>> Quit this debugging session? (y or n) FAIL: gdb.base/eh_return.exp: hit breakpoint (GDB >> internal error) >>> >>> Hmm, sorry about that. It passes for me on the machine I was >>> running tests on, so I missed it. I can reproduce it on a F27 machine. >>> I'll take a better look. >>> >> >> Here's a fix. See explanation within. > > Thanks. This fixed the regression. > I can also confirm that this fixes the FAIL for me. Glad I committed that test-case :) Thanks, - Tom ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH] Fix frame cycle detection (Re: [PATCH 3/3] Make scoped_restore_current_thread's cdtors exception free (RFC)) 2020-11-16 13:48 ` Tom de Vries @ 2020-11-16 14:57 ` Pedro Alves 0 siblings, 0 replies; 31+ messages in thread From: Pedro Alves @ 2020-11-16 14:57 UTC (permalink / raw) To: Tom de Vries, Aktemur, Tankut Baris, Simon Marchi, gdb-patches On 11/16/20 1:48 PM, Tom de Vries wrote: > On 11/9/20 3:05 PM, Aktemur, Tankut Baris via Gdb-patches wrote: >> On Saturday, October 31, 2020 3:35 PM, Pedro Alves wrote: >>> On 10/30/20 11:32 AM, Pedro Alves wrote: >>>> Hi! >>>> >>>> On 10/30/20 7:44 AM, Aktemur, Tankut Baris wrote: >>>> >>>>> Hi Pedro, >>>>> >>>>> This patch created the regression below: >>>>> >>>>> Breakpoint 1, 0x00000000004012bb in eh2 (/gdb-up/gdb/frame.c:641: internal-error: >>> frame_id get_frame_id(frame_info*): Assertion `stashed' failed. >>>>> A problem internal to GDB has been detected, >>>>> further debugging may prove unreliable. >>>>> Quit this debugging session? (y or n) FAIL: gdb.base/eh_return.exp: hit breakpoint (GDB >>> internal error) >>>> >>>> Hmm, sorry about that. It passes for me on the machine I was >>>> running tests on, so I missed it. I can reproduce it on a F27 machine. >>>> I'll take a better look. >>>> >>> >>> Here's a fix. See explanation within. >> >> Thanks. This fixed the regression. >> > > I can also confirm that this fixes the FAIL for me. Glad I committed > that test-case :) Indeed. :-) Thanks for the confirmations. I've merged this now. Pedro Alves ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 0/3] Fix crash if connection drops in scoped_restore_current_thread's ctor 2020-07-08 23:31 [PATCH 0/3] Fix crash if connection drops in scoped_restore_current_thread's ctor Pedro Alves ` (2 preceding siblings ...) 2020-07-08 23:31 ` [PATCH 3/3] Make scoped_restore_current_thread's cdtors exception free (RFC) Pedro Alves @ 2020-07-10 23:02 ` Pedro Alves 2020-07-22 19:37 ` Simon Marchi 3 siblings, 1 reply; 31+ messages in thread From: Pedro Alves @ 2020-07-10 23:02 UTC (permalink / raw) To: gdb-patches On 7/9/20 12:31 AM, Pedro Alves wrote: > (I have internet again: found a sim card of a different operator that > works. This will do until the communications tower near me is > repaired and get I fiber back...) > > This series fixes the crashes exposed by the > gdb.multi/multi-target.exp testcase when run against an Asan-enabled > GDB build, initially reported by Simon here: > > https://sourceware.org/pipermail/gdb-patches/2020-July/170222.html > > The first two patches fix the crashes, and we should probably put them > in GDB 10. > > The last patch is a follow up that avoids swallowing exceptions in > scoped_restore_current_thread's dtor that I'm thinking would be a bit > too invasive to put in GDB 10, I think it could do with a longer > baking period in master. > > Pedro Alves (3): > Fix crash if connection drops in scoped_restore_current_thread's ctor, > part 1 > Fix crash if connection drops in scoped_restore_current_thread's ctor, > part 2 > Make scoped_restore_current_thread's cdtors exception free (RFC) I've now merged patches 1 and 2. Patch 3 will wait until after the branch is cut. ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 0/3] Fix crash if connection drops in scoped_restore_current_thread's ctor 2020-07-10 23:02 ` [PATCH 0/3] Fix crash if connection drops in scoped_restore_current_thread's ctor Pedro Alves @ 2020-07-22 19:37 ` Simon Marchi 2020-07-22 20:37 ` Pedro Alves 0 siblings, 1 reply; 31+ messages in thread From: Simon Marchi @ 2020-07-22 19:37 UTC (permalink / raw) To: Pedro Alves, gdb-patches [-- Attachment #1: Type: text/plain, Size: 9433 bytes --] On 2020-07-10 7:02 p.m., Pedro Alves wrote: > On 7/9/20 12:31 AM, Pedro Alves wrote: >> (I have internet again: found a sim card of a different operator that >> works. This will do until the communications tower near me is >> repaired and get I fiber back...) >> >> This series fixes the crashes exposed by the >> gdb.multi/multi-target.exp testcase when run against an Asan-enabled >> GDB build, initially reported by Simon here: >> >> https://sourceware.org/pipermail/gdb-patches/2020-July/170222.html >> >> The first two patches fix the crashes, and we should probably put them >> in GDB 10. >> >> The last patch is a follow up that avoids swallowing exceptions in >> scoped_restore_current_thread's dtor that I'm thinking would be a bit >> too invasive to put in GDB 10, I think it could do with a longer >> baking period in master. >> >> Pedro Alves (3): >> Fix crash if connection drops in scoped_restore_current_thread's ctor, >> part 1 >> Fix crash if connection drops in scoped_restore_current_thread's ctor, >> part 2 >> Make scoped_restore_current_thread's cdtors exception free (RFC) > > I've now merged patches 1 and 2. Patch 3 will wait until after the branch > is cut. > I now see this other ASan failure when running gdb.multi/multi-target.exp, it's in the attached asan.log. There are colors, so it's easier to read if you "cat" it in your terminal. It looks familiar, because it happens in scoped_restore_current_thread's dtor (not ctor), but maybe it just happens to be there but could happen at any other point. It happens when starting test_continue with non-stop on, just after having completed test_continue with non-stop off. It's when GDB does "monitor exit". Unfortunately, the "freed by thread T0 here" stack trace is again truncated, probably because the stack is too deep for the portion of the stack ASan captures. But I managed to attach to GDB with GDB using gdb_interact and capture it (I broke on unpush_and_perror), here's the equivalent GDB backtrace: #0 xfree<void> (ptr=0x621004a5d900) at /home/smarchi/src/binutils-gdb/gdb/../gdbsupport/common-utils.h:63 #1 0x0000000001626260 in call_freefun (h=0x20f8da0 <frame_cache_obstack>, old_chunk=0x621004a5d900) at /home/smarchi/src/binutils-gdb/libiberty/obstack.c:103 #2 0x0000000001626c87 in _obstack_free (h=0x20f8da0 <frame_cache_obstack>, obj=0x0) at /home/smarchi/src/binutils-gdb/libiberty/obstack.c:280 #3 0x000000000098ae26 in reinit_frame_cache () at /home/smarchi/src/binutils-gdb/gdb/frame.c:1856 #4 0x0000000001098adf in switch_to_no_thread () at /home/smarchi/src/binutils-gdb/gdb/thread.c:1301 #5 0x0000000000acf544 in switch_to_inferior_no_thread (inf=0x615000244d00) at /home/smarchi/src/binutils-gdb/gdb/inferior.c:626 #6 0x0000000000e7c38c in remote_unpush_target (target=0x6170000c0c00) at /home/smarchi/src/binutils-gdb/gdb/remote.c:5521 #7 0x0000000000e92db6 in unpush_and_perror (target=0x6170000c0c00, string=0x191d400 "Remote communication error. Target disconnected.") at /home/smarchi/src/binutils-gdb/gdb/remote.c:9101 #8 0x0000000000e930c7 in remote_target::readchar (this=0x6170000c0c00, timeout=2) at /home/smarchi/src/binutils-gdb/gdb/remote.c:9141 #9 0x0000000000e9576f in remote_target::getpkt_or_notif_sane_1 (this=0x6170000c0c00, buf=0x6170000c0c18, forever=0, expecting_notif=0, is_notif=0x0) at /home/smarchi/src/binutils-gdb/gdb/remote.c:9683 #10 0x0000000000e961c9 in remote_target::getpkt_sane (this=0x6170000c0c00, buf=0x6170000c0c18, forever=0) at /home/smarchi/src/binutils-gdb/gdb/remote.c:9790 #11 0x0000000000e95545 in remote_target::getpkt (this=0x6170000c0c00, buf=0x6170000c0c18, forever=0) at /home/smarchi/src/binutils-gdb/gdb/remote.c:9623 #12 0x0000000000e91ba3 in remote_target::remote_read_bytes_1 (this=0x6170000c0c00, memaddr=0x7ffff78bc38d, myaddr=0x7fff7dca59a0 "", len_units=1, unit_size=1, xfered_len_units=0x7fff7dca58b0) at /home/smarchi/src/binutils-gdb/gdb/remote.c:8860 #13 0x0000000000e9240c in remote_target::remote_read_bytes (this=0x6170000c0c00, memaddr=0x7ffff78bc38d, myaddr=0x7fff7dca59a0 "", len=1, unit_size=1, xfered_len=0x7fff7dca58b0) at /home/smarchi/src/binutils-gdb/gdb/remote.c:8987 #14 0x0000000000e9b821 in remote_target::xfer_partial (this=0x6170000c0c00, object=TARGET_OBJECT_MEMORY, annex=0x0, readbuf=0x7fff7dca59a0 "", writebuf=0x0, offset=140737346519949, len=1, xfered_len=0x7fff7dca58b0) at /home/smarchi/src/binutils-gdb/gdb/remote.c:10987 #15 0x000000000104fd3a in raw_memory_xfer_partial (ops=0x6170000c0c00, readbuf=0x7fff7dca59a0 "", writebuf=0x0, memaddr=140737346519949, len=1, xfered_len=0x7fff7dca58b0) at /home/smarchi/src/binutils-gdb/gdb/target.c:918 #16 0x0000000001050425 in memory_xfer_partial_1 (ops=0x6170000c0c00, object=TARGET_OBJECT_MEMORY, readbuf=0x7fff7dca59a0 "", writebuf=0x0, memaddr=140737346519949, len=1, xfered_len=0x7fff7dca58b0) at /home/smarchi/src/binutils-gdb/gdb/target.c:1047 #17 0x0000000001050608 in memory_xfer_partial (ops=0x6170000c0c00, object=TARGET_OBJECT_MEMORY, readbuf=0x7fff7dca59a0 "", writebuf=0x0, memaddr=140737346519949, len=1, xfered_len=0x7fff7dca58b0) at /home/smarchi/src/binutils-gdb/gdb/target.c:1076 #18 0x0000000001050b92 in target_xfer_partial (ops=0x6170000c0c00, object=TARGET_OBJECT_MEMORY, annex=0x0, readbuf=0x7fff7dca59a0 "", writebuf=0x0, offset=140737346519949, len=1, xfered_len=0x7fff7dca58b0) at /home/smarchi/src/binutils-gdb/gdb/target.c:1133 #19 0x0000000001051a7b in target_read_partial (ops=0x6170000c0c00, object=TARGET_OBJECT_MEMORY, annex=0x0, buf=0x7fff7dca59a0 "", offset=140737346519949, len=1, xfered_len=0x7fff7dca58b0) at /home/smarchi/src/binutils-gdb/gdb/target.c:1379 #20 0x0000000001051c59 in target_read (ops=0x6170000c0c00, object=TARGET_OBJECT_MEMORY, annex=0x0, buf=0x7fff7dca59a0 "", offset=140737346519949, len=1) at /home/smarchi/src/binutils-gdb/gdb/target.c:1419 #21 0x0000000001051178 in target_read_memory (memaddr=0x7ffff78bc38d, myaddr=0x7fff7dca59a0 "", len=1) at /home/smarchi/src/binutils-gdb/gdb/target.c:1222 #22 0x00000000004b4731 in amd64_stack_frame_destroyed_p (gdbarch=0x6210027e8510, pc=0x7ffff78bc38d) at /home/smarchi/src/binutils-gdb/gdb/amd64-tdep.c:2909 #23 0x00000000004b4822 in amd64_epilogue_frame_sniffer (self=0x169df00 <amd64_epilogue_frame_unwind>, this_frame=0x621004a5d9e0, this_prologue_cache=0x621004a5d9f8) at /home/smarchi/src/binutils-gdb/gdb/amd64-tdep.c:2924 #24 0x0000000000981048 in frame_unwind_try_unwinder (this_frame=0x621004a5d9e0, this_cache=0x621004a5d9f8, unwinder=0x169df00 <amd64_epilogue_frame_unwind>) at /home/smarchi/src/binutils-gdb/gdb/frame-unwind.c:128 #25 0x000000000098126d in frame_unwind_find_by_frame (this_frame=0x621004a5d9e0, this_cache=0x621004a5d9f8) at /home/smarchi/src/binutils-gdb/gdb/frame-unwind.c:186 #26 0x0000000000983c9d in compute_frame_id (fi=0x621004a5d9e0) at /home/smarchi/src/binutils-gdb/gdb/frame.c:546 #27 0x0000000000984167 in get_frame_id (fi=0x621004a5d9e0) at /home/smarchi/src/binutils-gdb/gdb/frame.c:582 #28 0x0000000001098eef in restore_selected_frame (a_frame_id=..., frame_level=0) at /home/smarchi/src/binutils-gdb/gdb/thread.c:1355 #29 0x00000000010992f8 in scoped_restore_current_thread::restore (this=0x7fff7dca5f30) at /home/smarchi/src/binutils-gdb/gdb/thread.c:1411 #30 0x0000000001099355 in scoped_restore_current_thread::~scoped_restore_current_thread (this=0x7fff7dca5f30, __in_chrg=<optimized out>) at /home/smarchi/src/binutils-gdb/gdb/thread.c:1420 #31 0x0000000000aeab84 in do_target_wait (wait_ptid=..., ecs=0x7fff7dca6290, options=1) at /home/smarchi/src/binutils-gdb/gdb/infrun.c:3670 #32 0x0000000000aecbe3 in fetch_inferior_event () at /home/smarchi/src/binutils-gdb/gdb/infrun.c:3965 #33 0x0000000000aa8097 in inferior_event_handler (event_type=INF_REG_EVENT) at /home/smarchi/src/binutils-gdb/gdb/inf-loop.c:42 #34 0x0000000000eab8b7 in remote_async_inferior_event_handler (data=0x6170000d6a00) at /home/smarchi/src/binutils-gdb/gdb/remote.c:14166 #35 0x00000000004ca110 in check_async_event_handlers () at /home/smarchi/src/binutils-gdb/gdb/async-event.c:295 #36 0x00000000015bef41 in gdb_do_one_event () at /home/smarchi/src/binutils-gdb/gdbsupport/event-loop.cc:194 #37 0x0000000000bfd50e in start_event_loop () at /home/smarchi/src/binutils-gdb/gdb/main.c:356 #38 0x0000000000bfd816 in captured_command_loop () at /home/smarchi/src/binutils-gdb/gdb/main.c:416 #39 0x0000000000c00c25 in captured_main (data=0x7fff7dca65d0) at /home/smarchi/src/binutils-gdb/gdb/main.c:1253 #40 0x0000000000c00cb5 in gdb_main (args=0x7fff7dca65d0) at /home/smarchi/src/binutils-gdb/gdb/main.c:1268 #41 0x0000000000414d9e in main (argc=5, argv=0x7fff7dca6738) at /home/smarchi/src/binutils-gdb/gdb/gdb.c:32 The problem seems to be: - We create a new frame_info object in restore_selected_frame (by calling find_relative_frame) - The frame is allocated on the frame_cache_obstack - In frame_unwind_try_unwinder, we try to find an unwinder for that frame - While trying unwinders, memory read fails because the remote target closes, because of "monitor exit" - That calls reinit_frame_cache (as shown above), which resets frame_cache_obstack - When handling the exception in frame_unwind_try_unwinder, we try to set some things on the frame_info object (like *this_cache, which in fact tries to write into frame_info::prologue_cache), but the frame_info object is no more, it went away with the obstack. Simon [-- Attachment #2: asan.log --] [-- Type: text/x-log, Size: 12922 bytes --] ^[[1m^[[31m==4074==ERROR: AddressSanitizer: heap-use-after-free on address 0x621004a4fdf8 at pc 0x0000009810b3 bp 0x7fff38bbca90 sp 0x7fff38bbca80 ^[[1m^[[0m^[[1m^[[34mWRITE of size 8 at 0x621004a4fdf8 thread T0^[[1m^[[0m #0 0x9810b2 in frame_unwind_try_unwinder /home/smarchi/src/binutils-gdb/gdb/frame-unwind.c:134 #1 0x98126c in frame_unwind_find_by_frame(frame_info*, void**) /home/smarchi/src/binutils-gdb/gdb/frame-unwind.c:186 #2 0x983c9c in compute_frame_id /home/smarchi/src/binutils-gdb/gdb/frame.c:546 #3 0x984166 in get_frame_id(frame_info*) /home/smarchi/src/binutils-gdb/gdb/frame.c:582 #4 0x1098eee in restore_selected_frame /home/smarchi/src/binutils-gdb/gdb/thread.c:1355 #5 0x10992f7 in scoped_restore_current_thread::restore() /home/smarchi/src/binutils-gdb/gdb/thread.c:1411 #6 0x1099354 in scoped_restore_current_thread::~scoped_restore_current_thread() /home/smarchi/src/binutils-gdb/gdb/thread.c:1420 #7 0xaeab83 in do_target_wait /home/smarchi/src/binutils-gdb/gdb/infrun.c:3670 #8 0xaecbe2 in fetch_inferior_event() /home/smarchi/src/binutils-gdb/gdb/infrun.c:3965 #9 0xaa8096 in inferior_event_handler(inferior_event_type) /home/smarchi/src/binutils-gdb/gdb/inf-loop.c:42 #10 0xeab8b6 in remote_async_inferior_event_handler /home/smarchi/src/binutils-gdb/gdb/remote.c:14166 #11 0x4ca10f in check_async_event_handlers() /home/smarchi/src/binutils-gdb/gdb/async-event.c:295 #12 0x15bef40 in gdb_do_one_event() /home/smarchi/src/binutils-gdb/gdbsupport/event-loop.cc:194 #13 0xbfd50d in start_event_loop /home/smarchi/src/binutils-gdb/gdb/main.c:356 #14 0xbfd815 in captured_command_loop /home/smarchi/src/binutils-gdb/gdb/main.c:416 #15 0xc00c24 in captured_main /home/smarchi/src/binutils-gdb/gdb/main.c:1253 #16 0xc00cb4 in gdb_main(captured_main_args*) /home/smarchi/src/binutils-gdb/gdb/main.c:1268 #17 0x414d9d in main /home/smarchi/src/binutils-gdb/gdb/gdb.c:32 #18 0x7fc78984e83f in __libc_start_main ../csu/libc-start.c:291 #19 0x414b98 in _start (/home/smarchi/build/binutils-gdb/gdb/gdb+0x414b98) ^[[1m^[[32m0x621004a4fdf8 is located 248 bytes inside of 4064-byte region [0x621004a4fd00,0x621004a50ce0) ^[[1m^[[0m^[[1m^[[35mfreed by thread T0 here:^[[1m^[[0m #0 0x7fc78c385c7f in __interceptor_free (/usr/lib/x86_64-linux-gnu/libasan.so.5+0x10bc7f) #1 0x98f9f6 in xfree<void> /home/smarchi/src/binutils-gdb/gdb/../gdbsupport/common-utils.h:62 #2 0x162625f in call_freefun /home/smarchi/src/binutils-gdb/libiberty/obstack.c:103 #3 0x1626c86 in _obstack_free /home/smarchi/src/binutils-gdb/libiberty/obstack.c:280 #4 0x98ae25 in reinit_frame_cache() /home/smarchi/src/binutils-gdb/gdb/frame.c:1856 #5 0x1098ade in switch_to_no_thread() /home/smarchi/src/binutils-gdb/gdb/thread.c:1301 #6 0xacf543 in switch_to_inferior_no_thread(inferior*) /home/smarchi/src/binutils-gdb/gdb/inferior.c:626 #7 0xe7c38b in remote_unpush_target /home/smarchi/src/binutils-gdb/gdb/remote.c:5521 #8 0xe92db5 in unpush_and_perror /home/smarchi/src/binutils-gdb/gdb/remote.c:9101 #9 0xe930c6 in remote_target::readchar(int) /home/smarchi/src/binutils-gdb/gdb/remote.c:9141 #10 0xe9576e in remote_target::getpkt_or_notif_sane_1(std::vector<char, gdb::default_init_allocator<char, std::allocator<char> > >*, int, int, int*) /home/smarchi/src/binutils-gdb/gdb/remote.c:9683 #11 0xe961c8 in remote_target::getpkt_sane(std::vector<char, gdb::default_init_allocator<char, std::allocator<char> > >*, int) /home/smarchi/src/binutils-gdb/gdb/remote.c:9790 #12 0xe95544 in remote_target::getpkt(std::vector<char, gdb::default_init_allocator<char, std::allocator<char> > >*, int) /home/smarchi/src/binutils-gdb/gdb/remote.c:9623 #13 0xe91ba2 in remote_target::remote_read_bytes_1(unsigned long, unsigned char*, unsigned long, int, unsigned long*) /home/smarchi/src/binutils-gdb/gdb/remote.c:8860 #14 0xe9240b in remote_target::remote_read_bytes(unsigned long, unsigned char*, unsigned long, int, unsigned long*) /home/smarchi/src/binutils-gdb/gdb/remote.c:8987 #15 0xe9b820 in remote_target::xfer_partial(target_object, char const*, unsigned char*, unsigned char const*, unsigned long, unsigned long, unsigned long*) /home/smarchi/src/binutils-gdb/gdb/remote.c:10987 #16 0x104fd39 in raw_memory_xfer_partial(target_ops*, unsigned char*, unsigned char const*, unsigned long, long, unsigned long*) /home/smarchi/src/binutils-gdb/gdb/target.c:918 #17 0x1050424 in memory_xfer_partial_1 /home/smarchi/src/binutils-gdb/gdb/target.c:1047 #18 0x1050607 in memory_xfer_partial /home/smarchi/src/binutils-gdb/gdb/target.c:1076 #19 0x1050b91 in target_xfer_partial(target_ops*, target_object, char const*, unsigned char*, unsigned char const*, unsigned long, unsigned long, unsigned long*) /home/smarchi/src/binutils-gdb/gdb/target.c:1133 #20 0x1051a7a in target_read_partial /home/smarchi/src/binutils-gdb/gdb/target.c:1379 #21 0x1051c58 in target_read(target_ops*, target_object, char const*, unsigned char*, unsigned long, long) /home/smarchi/src/binutils-gdb/gdb/target.c:1419 #22 0x1051177 in target_read_memory(unsigned long, unsigned char*, long) /home/smarchi/src/binutils-gdb/gdb/target.c:1222 #23 0x4b4730 in amd64_stack_frame_destroyed_p /home/smarchi/src/binutils-gdb/gdb/amd64-tdep.c:2909 #24 0x4b4821 in amd64_epilogue_frame_sniffer /home/smarchi/src/binutils-gdb/gdb/amd64-tdep.c:2924 #25 0x981047 in frame_unwind_try_unwinder /home/smarchi/src/binutils-gdb/gdb/frame-unwind.c:128 #26 0x98126c in frame_unwind_find_by_frame(frame_info*, void**) /home/smarchi/src/binutils-gdb/gdb/frame-unwind.c:186 #27 0x983c9c in compute_frame_id /home/smarchi/src/binutils-gdb/gdb/frame.c:546 #28 0x984166 in get_frame_id(frame_info*) /home/smarchi/src/binutils-gdb/gdb/frame.c:582 #29 0x1098eee in restore_selected_frame /home/smarchi/src/binutils-gdb/gdb/thread.c:1355 ^[[1m^[[35mpreviously allocated by thread T0 here:^[[1m^[[0m #0 0x7fc78c386078 in malloc (/usr/lib/x86_64-linux-gnu/libasan.so.5+0x10c078) #1 0x4a1ad3 in xmalloc /home/smarchi/src/binutils-gdb/gdb/alloc.c:60 #2 0x162616d in call_chunkfun /home/smarchi/src/binutils-gdb/libiberty/obstack.c:94 #3 0x1626318 in _obstack_begin_worker /home/smarchi/src/binutils-gdb/libiberty/obstack.c:141 #4 0x16265cb in _obstack_begin /home/smarchi/src/binutils-gdb/libiberty/obstack.c:164 #5 0x98ae44 in reinit_frame_cache() /home/smarchi/src/binutils-gdb/gdb/frame.c:1857 #6 0x1098b4c in switch_to_thread(thread_info*) /home/smarchi/src/binutils-gdb/gdb/thread.c:1316 #7 0x1099146 in scoped_restore_current_thread::restore() /home/smarchi/src/binutils-gdb/gdb/thread.c:1399 #8 0x1099354 in scoped_restore_current_thread::~scoped_restore_current_thread() /home/smarchi/src/binutils-gdb/gdb/thread.c:1420 #9 0xaeab83 in do_target_wait /home/smarchi/src/binutils-gdb/gdb/infrun.c:3670 #10 0xaecbe2 in fetch_inferior_event() /home/smarchi/src/binutils-gdb/gdb/infrun.c:3965 #11 0xaa8096 in inferior_event_handler(inferior_event_type) /home/smarchi/src/binutils-gdb/gdb/inf-loop.c:42 #12 0xeab8b6 in remote_async_inferior_event_handler /home/smarchi/src/binutils-gdb/gdb/remote.c:14166 #13 0x4ca10f in check_async_event_handlers() /home/smarchi/src/binutils-gdb/gdb/async-event.c:295 #14 0x15bef40 in gdb_do_one_event() /home/smarchi/src/binutils-gdb/gdbsupport/event-loop.cc:194 #15 0xbfd50d in start_event_loop /home/smarchi/src/binutils-gdb/gdb/main.c:356 #16 0xbfd815 in captured_command_loop /home/smarchi/src/binutils-gdb/gdb/main.c:416 #17 0xc00c24 in captured_main /home/smarchi/src/binutils-gdb/gdb/main.c:1253 #18 0xc00cb4 in gdb_main(captured_main_args*) /home/smarchi/src/binutils-gdb/gdb/main.c:1268 #19 0x414d9d in main /home/smarchi/src/binutils-gdb/gdb/gdb.c:32 #20 0x7fc78984e83f in __libc_start_main ../csu/libc-start.c:291 SUMMARY: AddressSanitizer: heap-use-after-free /home/smarchi/src/binutils-gdb/gdb/frame-unwind.c:134 in frame_unwind_try_unwinder Shadow bytes around the buggy address: 0x0c4280941f60: ^[[1m^[[31mfa^[[1m^[[0m ^[[1m^[[31mfa^[[1m^[[0m ^[[1m^[[31mfa^[[1m^[[0m ^[[1m^[[31mfa^[[1m^[[0m ^[[1m^[[31mfa^[[1m^[[0m ^[[1m^[[31mfa^[[1m^[[0m ^[[1m^[[31mfa^[[1m^[[0m ^[[1m^[[31mfa^[[1m^[[0m ^[[1m^[[31mfa^[[1m^[[0m ^[[1m^[[31mfa^[[1m^[[0m ^[[1m^[[31mfa^[[1m^[[0m ^[[1m^[[31mfa^[[1m^[[0m ^[[1m^[[31mfa^[[1m^[[0m ^[[1m^[[31mfa^[[1m^[[0m ^[[1m^[[31mfa^[[1m^[[0m ^[[1m^[[31mfa^[[1m^[[0m 0x0c4280941f70: ^[[1m^[[31mfa^[[1m^[[0m ^[[1m^[[31mfa^[[1m^[[0m ^[[1m^[[31mfa^[[1m^[[0m ^[[1m^[[31mfa^[[1m^[[0m ^[[1m^[[31mfa^[[1m^[[0m ^[[1m^[[31mfa^[[1m^[[0m ^[[1m^[[31mfa^[[1m^[[0m ^[[1m^[[31mfa^[[1m^[[0m ^[[1m^[[31mfa^[[1m^[[0m ^[[1m^[[31mfa^[[1m^[[0m ^[[1m^[[31mfa^[[1m^[[0m ^[[1m^[[31mfa^[[1m^[[0m ^[[1m^[[31mfa^[[1m^[[0m ^[[1m^[[31mfa^[[1m^[[0m ^[[1m^[[31mfa^[[1m^[[0m ^[[1m^[[31mfa^[[1m^[[0m 0x0c4280941f80: ^[[1m^[[31mfa^[[1m^[[0m ^[[1m^[[31mfa^[[1m^[[0m ^[[1m^[[31mfa^[[1m^[[0m ^[[1m^[[31mfa^[[1m^[[0m ^[[1m^[[31mfa^[[1m^[[0m ^[[1m^[[31mfa^[[1m^[[0m ^[[1m^[[31mfa^[[1m^[[0m ^[[1m^[[31mfa^[[1m^[[0m ^[[1m^[[31mfa^[[1m^[[0m ^[[1m^[[31mfa^[[1m^[[0m ^[[1m^[[31mfa^[[1m^[[0m ^[[1m^[[31mfa^[[1m^[[0m ^[[1m^[[31mfa^[[1m^[[0m ^[[1m^[[31mfa^[[1m^[[0m ^[[1m^[[31mfa^[[1m^[[0m ^[[1m^[[31mfa^[[1m^[[0m 0x0c4280941f90: ^[[1m^[[31mfa^[[1m^[[0m ^[[1m^[[31mfa^[[1m^[[0m ^[[1m^[[31mfa^[[1m^[[0m ^[[1m^[[31mfa^[[1m^[[0m ^[[1m^[[31mfa^[[1m^[[0m ^[[1m^[[31mfa^[[1m^[[0m ^[[1m^[[31mfa^[[1m^[[0m ^[[1m^[[31mfa^[[1m^[[0m ^[[1m^[[31mfa^[[1m^[[0m ^[[1m^[[31mfa^[[1m^[[0m ^[[1m^[[31mfa^[[1m^[[0m ^[[1m^[[31mfa^[[1m^[[0m ^[[1m^[[31mfa^[[1m^[[0m ^[[1m^[[31mfa^[[1m^[[0m ^[[1m^[[31mfa^[[1m^[[0m ^[[1m^[[31mfa^[[1m^[[0m 0x0c4280941fa0: ^[[1m^[[35mfd^[[1m^[[0m ^[[1m^[[35mfd^[[1m^[[0m ^[[1m^[[35mfd^[[1m^[[0m ^[[1m^[[35mfd^[[1m^[[0m ^[[1m^[[35mfd^[[1m^[[0m ^[[1m^[[35mfd^[[1m^[[0m ^[[1m^[[35mfd^[[1m^[[0m ^[[1m^[[35mfd^[[1m^[[0m ^[[1m^[[35mfd^[[1m^[[0m ^[[1m^[[35mfd^[[1m^[[0m ^[[1m^[[35mfd^[[1m^[[0m ^[[1m^[[35mfd^[[1m^[[0m ^[[1m^[[35mfd^[[1m^[[0m ^[[1m^[[35mfd^[[1m^[[0m ^[[1m^[[35mfd^[[1m^[[0m ^[[1m^[[35mfd^[[1m^[[0m =>0x0c4280941fb0: ^[[1m^[[35mfd^[[1m^[[0m ^[[1m^[[35mfd^[[1m^[[0m ^[[1m^[[35mfd^[[1m^[[0m ^[[1m^[[35mfd^[[1m^[[0m ^[[1m^[[35mfd^[[1m^[[0m ^[[1m^[[35mfd^[[1m^[[0m ^[[1m^[[35mfd^[[1m^[[0m ^[[1m^[[35mfd^[[1m^[[0m ^[[1m^[[35mfd^[[1m^[[0m ^[[1m^[[35mfd^[[1m^[[0m ^[[1m^[[35mfd^[[1m^[[0m ^[[1m^[[35mfd^[[1m^[[0m ^[[1m^[[35mfd^[[1m^[[0m ^[[1m^[[35mfd^[[1m^[[0m ^[[1m^[[35mfd^[[1m^[[0m[^[[1m^[[35mfd^[[1m^[[0m] 0x0c4280941fc0: ^[[1m^[[35mfd^[[1m^[[0m ^[[1m^[[35mfd^[[1m^[[0m ^[[1m^[[35mfd^[[1m^[[0m ^[[1m^[[35mfd^[[1m^[[0m ^[[1m^[[35mfd^[[1m^[[0m ^[[1m^[[35mfd^[[1m^[[0m ^[[1m^[[35mfd^[[1m^[[0m ^[[1m^[[35mfd^[[1m^[[0m ^[[1m^[[35mfd^[[1m^[[0m ^[[1m^[[35mfd^[[1m^[[0m ^[[1m^[[35mfd^[[1m^[[0m ^[[1m^[[35mfd^[[1m^[[0m ^[[1m^[[35mfd^[[1m^[[0m ^[[1m^[[35mfd^[[1m^[[0m ^[[1m^[[35mfd^[[1m^[[0m ^[[1m^[[35mfd^[[1m^[[0m 0x0c4280941fd0: ^[[1m^[[35mfd^[[1m^[[0m ^[[1m^[[35mfd^[[1m^[[0m ^[[1m^[[35mfd^[[1m^[[0m ^[[1m^[[35mfd^[[1m^[[0m ^[[1m^[[35mfd^[[1m^[[0m ^[[1m^[[35mfd^[[1m^[[0m ^[[1m^[[35mfd^[[1m^[[0m ^[[1m^[[35mfd^[[1m^[[0m ^[[1m^[[35mfd^[[1m^[[0m ^[[1m^[[35mfd^[[1m^[[0m ^[[1m^[[35mfd^[[1m^[[0m ^[[1m^[[35mfd^[[1m^[[0m ^[[1m^[[35mfd^[[1m^[[0m ^[[1m^[[35mfd^[[1m^[[0m ^[[1m^[[35mfd^[[1m^[[0m ^[[1m^[[35mfd^[[1m^[[0m 0x0c4280941fe0: ^[[1m^[[35mfd^[[1m^[[0m ^[[1m^[[35mfd^[[1m^[[0m ^[[1m^[[35mfd^[[1m^[[0m ^[[1m^[[35mfd^[[1m^[[0m ^[[1m^[[35mfd^[[1m^[[0m ^[[1m^[[35mfd^[[1m^[[0m ^[[1m^[[35mfd^[[1m^[[0m ^[[1m^[[35mfd^[[1m^[[0m ^[[1m^[[35mfd^[[1m^[[0m ^[[1m^[[35mfd^[[1m^[[0m ^[[1m^[[35mfd^[[1m^[[0m ^[[1m^[[35mfd^[[1m^[[0m ^[[1m^[[35mfd^[[1m^[[0m ^[[1m^[[35mfd^[[1m^[[0m ^[[1m^[[35mfd^[[1m^[[0m ^[[1m^[[35mfd^[[1m^[[0m 0x0c4280941ff0: ^[[1m^[[35mfd^[[1m^[[0m ^[[1m^[[35mfd^[[1m^[[0m ^[[1m^[[35mfd^[[1m^[[0m ^[[1m^[[35mfd^[[1m^[[0m ^[[1m^[[35mfd^[[1m^[[0m ^[[1m^[[35mfd^[[1m^[[0m ^[[1m^[[35mfd^[[1m^[[0m ^[[1m^[[35mfd^[[1m^[[0m ^[[1m^[[35mfd^[[1m^[[0m ^[[1m^[[35mfd^[[1m^[[0m ^[[1m^[[35mfd^[[1m^[[0m ^[[1m^[[35mfd^[[1m^[[0m ^[[1m^[[35mfd^[[1m^[[0m ^[[1m^[[35mfd^[[1m^[[0m ^[[1m^[[35mfd^[[1m^[[0m ^[[1m^[[35mfd^[[1m^[[0m 0x0c4280942000: ^[[1m^[[35mfd^[[1m^[[0m ^[[1m^[[35mfd^[[1m^[[0m ^[[1m^[[35mfd^[[1m^[[0m ^[[1m^[[35mfd^[[1m^[[0m ^[[1m^[[35mfd^[[1m^[[0m ^[[1m^[[35mfd^[[1m^[[0m ^[[1m^[[35mfd^[[1m^[[0m ^[[1m^[[35mfd^[[1m^[[0m ^[[1m^[[35mfd^[[1m^[[0m ^[[1m^[[35mfd^[[1m^[[0m ^[[1m^[[35mfd^[[1m^[[0m ^[[1m^[[35mfd^[[1m^[[0m ^[[1m^[[35mfd^[[1m^[[0m ^[[1m^[[35mfd^[[1m^[[0m ^[[1m^[[35mfd^[[1m^[[0m ^[[1m^[[35mfd^[[1m^[[0m Shadow byte legend (one shadow byte represents 8 application bytes): Addressable: ^[[1m^[[0m00^[[1m^[[0m Partially addressable: ^[[1m^[[0m01^[[1m^[[0m ^[[1m^[[0m02^[[1m^[[0m ^[[1m^[[0m03^[[1m^[[0m ^[[1m^[[0m04^[[1m^[[0m ^[[1m^[[0m05^[[1m^[[0m ^[[1m^[[0m06^[[1m^[[0m ^[[1m^[[0m07^[[1m^[[0m Heap left redzone: ^[[1m^[[31mfa^[[1m^[[0m Freed heap region: ^[[1m^[[35mfd^[[1m^[[0m Stack left redzone: ^[[1m^[[31mf1^[[1m^[[0m Stack mid redzone: ^[[1m^[[31mf2^[[1m^[[0m Stack right redzone: ^[[1m^[[31mf3^[[1m^[[0m Stack after return: ^[[1m^[[35mf5^[[1m^[[0m Stack use after scope: ^[[1m^[[35mf8^[[1m^[[0m Global redzone: ^[[1m^[[31mf9^[[1m^[[0m Global init order: ^[[1m^[[36mf6^[[1m^[[0m Poisoned by user: ^[[1m^[[34mf7^[[1m^[[0m Container overflow: ^[[1m^[[34mfc^[[1m^[[0m Array cookie: ^[[1m^[[31mac^[[1m^[[0m Intra object redzone: ^[[1m^[[33mbb^[[1m^[[0m ASan internal: ^[[1m^[[33mfe^[[1m^[[0m Left alloca redzone: ^[[1m^[[34mca^[[1m^[[0m Right alloca redzone: ^[[1m^[[34mcb^[[1m^[[0m Shadow gap: ^[[1m^[[0mcc^[[1m^[[0m ==4074==ABORTING ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 0/3] Fix crash if connection drops in scoped_restore_current_thread's ctor 2020-07-22 19:37 ` Simon Marchi @ 2020-07-22 20:37 ` Pedro Alves 2020-07-22 20:47 ` Simon Marchi 0 siblings, 1 reply; 31+ messages in thread From: Pedro Alves @ 2020-07-22 20:37 UTC (permalink / raw) To: Simon Marchi, gdb-patches On 7/22/20 8:37 PM, Simon Marchi wrote: > On 2020-07-10 7:02 p.m., Pedro Alves wrote: >> On 7/9/20 12:31 AM, Pedro Alves wrote: >>> (I have internet again: found a sim card of a different operator that >>> works. This will do until the communications tower near me is >>> repaired and get I fiber back...) >>> >>> This series fixes the crashes exposed by the >>> gdb.multi/multi-target.exp testcase when run against an Asan-enabled >>> GDB build, initially reported by Simon here: >>> >>> https://sourceware.org/pipermail/gdb-patches/2020-July/170222.html >>> >>> The first two patches fix the crashes, and we should probably put them >>> in GDB 10. >>> >>> The last patch is a follow up that avoids swallowing exceptions in >>> scoped_restore_current_thread's dtor that I'm thinking would be a bit >>> too invasive to put in GDB 10, I think it could do with a longer >>> baking period in master. >>> >>> Pedro Alves (3): >>> Fix crash if connection drops in scoped_restore_current_thread's ctor, >>> part 1 >>> Fix crash if connection drops in scoped_restore_current_thread's ctor, >>> part 2 >>> Make scoped_restore_current_thread's cdtors exception free (RFC) >> >> I've now merged patches 1 and 2. Patch 3 will wait until after the branch >> is cut. >> > > I now see this other ASan failure when running gdb.multi/multi-target.exp, it's in the > attached asan.log. There are colors, so it's easier to read if you "cat" it in your > terminal. It looks familiar, because it happens in scoped_restore_current_thread's dtor > (not ctor), but maybe it just happens to be there but could happen at any other point. > > It happens when starting test_continue with non-stop on, just after having completed > test_continue with non-stop off. It's when GDB does "monitor exit". > > Unfortunately, the "freed by thread T0 here" stack trace is again truncated, probably > because the stack is too deep for the portion of the stack ASan captures. But I managed > to attach to GDB with GDB using gdb_interact and capture it (I broke on unpush_and_perror), > here's the equivalent GDB backtrace: > > #0 xfree<void> (ptr=0x621004a5d900) at /home/smarchi/src/binutils-gdb/gdb/../gdbsupport/common-utils.h:63 > #1 0x0000000001626260 in call_freefun (h=0x20f8da0 <frame_cache_obstack>, old_chunk=0x621004a5d900) at /home/smarchi/src/binutils-gdb/libiberty/obstack.c:103 > #2 0x0000000001626c87 in _obstack_free (h=0x20f8da0 <frame_cache_obstack>, obj=0x0) at /home/smarchi/src/binutils-gdb/libiberty/obstack.c:280 > #3 0x000000000098ae26 in reinit_frame_cache () at /home/smarchi/src/binutils-gdb/gdb/frame.c:1856 > #4 0x0000000001098adf in switch_to_no_thread () at /home/smarchi/src/binutils-gdb/gdb/thread.c:1301 > #5 0x0000000000acf544 in switch_to_inferior_no_thread (inf=0x615000244d00) at /home/smarchi/src/binutils-gdb/gdb/inferior.c:626 > #6 0x0000000000e7c38c in remote_unpush_target (target=0x6170000c0c00) at /home/smarchi/src/binutils-gdb/gdb/remote.c:5521 > #7 0x0000000000e92db6 in unpush_and_perror (target=0x6170000c0c00, string=0x191d400 "Remote communication error. Target disconnected.") at /home/smarchi/src/binutils-gdb/gdb/remote.c:9101 > #8 0x0000000000e930c7 in remote_target::readchar (this=0x6170000c0c00, timeout=2) at /home/smarchi/src/binutils-gdb/gdb/remote.c:9141 > #9 0x0000000000e9576f in remote_target::getpkt_or_notif_sane_1 (this=0x6170000c0c00, buf=0x6170000c0c18, forever=0, expecting_notif=0, is_notif=0x0) at /home/smarchi/src/binutils-gdb/gdb/remote.c:9683 > #10 0x0000000000e961c9 in remote_target::getpkt_sane (this=0x6170000c0c00, buf=0x6170000c0c18, forever=0) at /home/smarchi/src/binutils-gdb/gdb/remote.c:9790 > #11 0x0000000000e95545 in remote_target::getpkt (this=0x6170000c0c00, buf=0x6170000c0c18, forever=0) at /home/smarchi/src/binutils-gdb/gdb/remote.c:9623 > #12 0x0000000000e91ba3 in remote_target::remote_read_bytes_1 (this=0x6170000c0c00, memaddr=0x7ffff78bc38d, myaddr=0x7fff7dca59a0 "", len_units=1, unit_size=1, xfered_len_units=0x7fff7dca58b0) at /home/smarchi/src/binutils-gdb/gdb/remote.c:8860 > #13 0x0000000000e9240c in remote_target::remote_read_bytes (this=0x6170000c0c00, memaddr=0x7ffff78bc38d, myaddr=0x7fff7dca59a0 "", len=1, unit_size=1, xfered_len=0x7fff7dca58b0) at /home/smarchi/src/binutils-gdb/gdb/remote.c:8987 > #14 0x0000000000e9b821 in remote_target::xfer_partial (this=0x6170000c0c00, object=TARGET_OBJECT_MEMORY, annex=0x0, readbuf=0x7fff7dca59a0 "", writebuf=0x0, offset=140737346519949, len=1, xfered_len=0x7fff7dca58b0) at /home/smarchi/src/binutils-gdb/gdb/remote.c:10987 > #15 0x000000000104fd3a in raw_memory_xfer_partial (ops=0x6170000c0c00, readbuf=0x7fff7dca59a0 "", writebuf=0x0, memaddr=140737346519949, len=1, xfered_len=0x7fff7dca58b0) at /home/smarchi/src/binutils-gdb/gdb/target.c:918 > #16 0x0000000001050425 in memory_xfer_partial_1 (ops=0x6170000c0c00, object=TARGET_OBJECT_MEMORY, readbuf=0x7fff7dca59a0 "", writebuf=0x0, memaddr=140737346519949, len=1, xfered_len=0x7fff7dca58b0) at /home/smarchi/src/binutils-gdb/gdb/target.c:1047 > #17 0x0000000001050608 in memory_xfer_partial (ops=0x6170000c0c00, object=TARGET_OBJECT_MEMORY, readbuf=0x7fff7dca59a0 "", writebuf=0x0, memaddr=140737346519949, len=1, xfered_len=0x7fff7dca58b0) at /home/smarchi/src/binutils-gdb/gdb/target.c:1076 > #18 0x0000000001050b92 in target_xfer_partial (ops=0x6170000c0c00, object=TARGET_OBJECT_MEMORY, annex=0x0, readbuf=0x7fff7dca59a0 "", writebuf=0x0, offset=140737346519949, len=1, xfered_len=0x7fff7dca58b0) at /home/smarchi/src/binutils-gdb/gdb/target.c:1133 > #19 0x0000000001051a7b in target_read_partial (ops=0x6170000c0c00, object=TARGET_OBJECT_MEMORY, annex=0x0, buf=0x7fff7dca59a0 "", offset=140737346519949, len=1, xfered_len=0x7fff7dca58b0) at /home/smarchi/src/binutils-gdb/gdb/target.c:1379 > #20 0x0000000001051c59 in target_read (ops=0x6170000c0c00, object=TARGET_OBJECT_MEMORY, annex=0x0, buf=0x7fff7dca59a0 "", offset=140737346519949, len=1) at /home/smarchi/src/binutils-gdb/gdb/target.c:1419 > #21 0x0000000001051178 in target_read_memory (memaddr=0x7ffff78bc38d, myaddr=0x7fff7dca59a0 "", len=1) at /home/smarchi/src/binutils-gdb/gdb/target.c:1222 > #22 0x00000000004b4731 in amd64_stack_frame_destroyed_p (gdbarch=0x6210027e8510, pc=0x7ffff78bc38d) at /home/smarchi/src/binutils-gdb/gdb/amd64-tdep.c:2909 > #23 0x00000000004b4822 in amd64_epilogue_frame_sniffer (self=0x169df00 <amd64_epilogue_frame_unwind>, this_frame=0x621004a5d9e0, this_prologue_cache=0x621004a5d9f8) at /home/smarchi/src/binutils-gdb/gdb/amd64-tdep.c:2924 > #24 0x0000000000981048 in frame_unwind_try_unwinder (this_frame=0x621004a5d9e0, this_cache=0x621004a5d9f8, unwinder=0x169df00 <amd64_epilogue_frame_unwind>) at /home/smarchi/src/binutils-gdb/gdb/frame-unwind.c:128 > #25 0x000000000098126d in frame_unwind_find_by_frame (this_frame=0x621004a5d9e0, this_cache=0x621004a5d9f8) at /home/smarchi/src/binutils-gdb/gdb/frame-unwind.c:186 > #26 0x0000000000983c9d in compute_frame_id (fi=0x621004a5d9e0) at /home/smarchi/src/binutils-gdb/gdb/frame.c:546 > #27 0x0000000000984167 in get_frame_id (fi=0x621004a5d9e0) at /home/smarchi/src/binutils-gdb/gdb/frame.c:582 > #28 0x0000000001098eef in restore_selected_frame (a_frame_id=..., frame_level=0) at /home/smarchi/src/binutils-gdb/gdb/thread.c:1355 > #29 0x00000000010992f8 in scoped_restore_current_thread::restore (this=0x7fff7dca5f30) at /home/smarchi/src/binutils-gdb/gdb/thread.c:1411 > #30 0x0000000001099355 in scoped_restore_current_thread::~scoped_restore_current_thread (this=0x7fff7dca5f30, __in_chrg=<optimized out>) at /home/smarchi/src/binutils-gdb/gdb/thread.c:1420 > #31 0x0000000000aeab84 in do_target_wait (wait_ptid=..., ecs=0x7fff7dca6290, options=1) at /home/smarchi/src/binutils-gdb/gdb/infrun.c:3670 > #32 0x0000000000aecbe3 in fetch_inferior_event () at /home/smarchi/src/binutils-gdb/gdb/infrun.c:3965 > #33 0x0000000000aa8097 in inferior_event_handler (event_type=INF_REG_EVENT) at /home/smarchi/src/binutils-gdb/gdb/inf-loop.c:42 > #34 0x0000000000eab8b7 in remote_async_inferior_event_handler (data=0x6170000d6a00) at /home/smarchi/src/binutils-gdb/gdb/remote.c:14166 > #35 0x00000000004ca110 in check_async_event_handlers () at /home/smarchi/src/binutils-gdb/gdb/async-event.c:295 > #36 0x00000000015bef41 in gdb_do_one_event () at /home/smarchi/src/binutils-gdb/gdbsupport/event-loop.cc:194 > #37 0x0000000000bfd50e in start_event_loop () at /home/smarchi/src/binutils-gdb/gdb/main.c:356 > #38 0x0000000000bfd816 in captured_command_loop () at /home/smarchi/src/binutils-gdb/gdb/main.c:416 > #39 0x0000000000c00c25 in captured_main (data=0x7fff7dca65d0) at /home/smarchi/src/binutils-gdb/gdb/main.c:1253 > #40 0x0000000000c00cb5 in gdb_main (args=0x7fff7dca65d0) at /home/smarchi/src/binutils-gdb/gdb/main.c:1268 > #41 0x0000000000414d9e in main (argc=5, argv=0x7fff7dca6738) at /home/smarchi/src/binutils-gdb/gdb/gdb.c:32 > Bah. > > The problem seems to be: > > - We create a new frame_info object in restore_selected_frame (by calling find_relative_frame) > - The frame is allocated on the frame_cache_obstack > - In frame_unwind_try_unwinder, we try to find an unwinder for that frame > - While trying unwinders, memory read fails because the remote target closes, because of "monitor exit" > - That calls reinit_frame_cache (as shown above), which resets frame_cache_obstack > - When handling the exception in frame_unwind_try_unwinder, we try to set some things on the frame_info > object (like *this_cache, which in fact tries to write into frame_info::prologue_cache), but the > frame_info object is no more, it went away with the obstack. I'm thinking that to fix this we will need a generation counter in reinit_frame_cache. Then in frame_unwind_try_unwinder, don't call frame_cleanup_after_sniffer if the generation is not the same as it was on entry. Something like this. Does it fix it for you? I can't seem to reproduce the crash here. From 202b20db082969cfa156468c3443888761629dee Mon Sep 17 00:00:00 2001 From: Pedro Alves <pedro@palves.net> Date: Wed, 22 Jul 2020 20:53:59 +0100 Subject: [PATCH] Fix yet another bug exposed by ASAN + multi-target.exp --- gdb/frame-unwind.c | 13 ++++++++++--- gdb/frame.c | 13 +++++++++++++ gdb/frame.h | 4 ++++ 3 files changed, 27 insertions(+), 3 deletions(-) diff --git a/gdb/frame-unwind.c b/gdb/frame-unwind.c index 3334c472d02..ba25e19172e 100644 --- a/gdb/frame-unwind.c +++ b/gdb/frame-unwind.c @@ -121,6 +121,8 @@ frame_unwind_try_unwinder (struct frame_info *this_frame, void **this_cache, { int res = 0; + unsigned int entry_generation = get_frame_cache_generation (); + frame_prepare_for_sniffer (this_frame, unwinder); try @@ -130,9 +132,14 @@ frame_unwind_try_unwinder (struct frame_info *this_frame, void **this_cache, catch (const gdb_exception &ex) { /* Catch all exceptions, caused by either interrupt or error. - Reset *THIS_CACHE. */ - *this_cache = NULL; - frame_cleanup_after_sniffer (this_frame); + Reset *THIS_CACHE, unless something reinitialized the frame + cache meanwhile, in which case THIS_FRAME is now + dangling. */ + if (get_frame_cache_generation () == entry_generation) + { + *this_cache = NULL; + frame_cleanup_after_sniffer (this_frame); + } if (ex.error == NOT_AVAILABLE_ERROR) { diff --git a/gdb/frame.c b/gdb/frame.c index ac1016b083f..4ac958a1e95 100644 --- a/gdb/frame.c +++ b/gdb/frame.c @@ -53,6 +53,17 @@ static struct frame_info *sentinel_frame; +/* Number of calls to reinit_frame_cache. */ +static unsigned int frame_cache_generation = 0; + +/* See frame.h. */ + +unsigned int +get_frame_cache_generation () +{ + return frame_cache_generation; +} + /* The values behind the global "set backtrace ..." settings. */ set_backtrace_options user_set_backtrace_options; @@ -1843,6 +1854,8 @@ reinit_frame_cache (void) { struct frame_info *fi; + ++frame_cache_generation; + /* Tear down all frame caches. */ for (fi = sentinel_frame; fi != NULL; fi = fi->prev) { diff --git a/gdb/frame.h b/gdb/frame.h index cfc15022ed5..8d029cc065d 100644 --- a/gdb/frame.h +++ b/gdb/frame.h @@ -949,6 +949,10 @@ extern const gdb::option::option_def set_backtrace_option_defs[2]; /* The values behind the global "set backtrace ..." settings. */ extern set_backtrace_options user_set_backtrace_options; +/* Get the number of calls to reinit_frame_cache. */ + +unsigned int get_frame_cache_generation (); + /* Mark that the PC value is masked for the previous frame. */ extern void set_frame_previous_pc_masked (struct frame_info *frame); base-commit: 32fa152e3bfcf021ce49767be547fae5129d922b -- 2.14.5 ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 0/3] Fix crash if connection drops in scoped_restore_current_thread's ctor 2020-07-22 20:37 ` Pedro Alves @ 2020-07-22 20:47 ` Simon Marchi 2020-07-23 15:28 ` [pushed] Don't touch frame_info objects if frame cache was reinitialized (was: Re: [PATCH 0/3] Fix crash if connection drops in scoped_restore_current_thread's ctor) Pedro Alves 0 siblings, 1 reply; 31+ messages in thread From: Simon Marchi @ 2020-07-22 20:47 UTC (permalink / raw) To: Pedro Alves, gdb-patches On 2020-07-22 4:37 p.m., Pedro Alves wrote: > I'm thinking that to fix this we will need a generation counter in > reinit_frame_cache. Then in frame_unwind_try_unwinder, don't call > frame_cleanup_after_sniffer if the generation is not the same as it was > on entry. > > Something like this. Does it fix it for you? I can't seem to reproduce > the crash here. I don't have time to try, but the approach makes sense. It also crossed my mind, but I thought it would be more complicated to implement than that. Simon ^ permalink raw reply [flat|nested] 31+ messages in thread
* [pushed] Don't touch frame_info objects if frame cache was reinitialized (was: Re: [PATCH 0/3] Fix crash if connection drops in scoped_restore_current_thread's ctor) 2020-07-22 20:47 ` Simon Marchi @ 2020-07-23 15:28 ` Pedro Alves 0 siblings, 0 replies; 31+ messages in thread From: Pedro Alves @ 2020-07-23 15:28 UTC (permalink / raw) To: Simon Marchi, gdb-patches On 7/22/20 9:47 PM, Simon Marchi wrote: > On 2020-07-22 4:37 p.m., Pedro Alves wrote: >> I'm thinking that to fix this we will need a generation counter in >> reinit_frame_cache. Then in frame_unwind_try_unwinder, don't call >> frame_cleanup_after_sniffer if the generation is not the same as it was >> on entry. >> >> Something like this. Does it fix it for you? I can't seem to reproduce >> the crash here. > > I don't have time to try, but the approach makes sense. It also crossed my mind, > but I thought it would be more complicated to implement than that. > Alright, I managed to reproduce it reliably with just: - not having debug info loaded to avoid the dwarf unwinder - killing gdbserver - backtrace The tentative fix was incomplete, because the exception is rethrown, and then caught by get_prev_frame_if_no_cycle which also touches frame_info objects. We just need to apply the same fix there. Simon is away, so I'm going ahead assuming that this also fixes it for him. Here's what I'm merging. From 9552defd7ee0424165599b578dcd8ed71f0aef49 Mon Sep 17 00:00:00 2001 From: Pedro Alves <pedro@palves.net> Date: Thu, 23 Jul 2020 16:19:38 +0100 Subject: [PATCH] Don't touch frame_info objects if frame cache was reinitialized This fixes yet another bug exposed by ASAN + multi-target.exp Running an Asan-enabled GDB against gdb.multi/multi-target.exp exposed yet another latent GDB bug. See here for the full log: https://sourceware.org/pipermail/gdb-patches/2020-July/170761.html As Simon described, the problem is: - We create a new frame_info object in restore_selected_frame (by calling find_relative_frame) - The frame is allocated on the frame_cache_obstack - In frame_unwind_try_unwinder, we try to find an unwinder for that frame - While trying unwinders, memory read fails because the remote target closes, because of "monitor exit" - That calls reinit_frame_cache (as shown above), which resets frame_cache_obstack - When handling the exception in frame_unwind_try_unwinder, we try to set some things on the frame_info object (like *this_cache, which in fact tries to write into frame_info::prologue_cache), but the frame_info object is no more, it went away with the obstack. Fix this by maintaining a frame cache generation counter. Then in exception handling code paths, don't touch frame objects if the generation is not the same as it was on entry. This commit generalizes the gdb.server/server-kill.exp testcase and reuses it to test the scenario in question. The new tests fail without the GDB fix. gdb/ChangeLog: * frame-unwind.c (frame_unwind_try_unwinder): On exception, don't touch THIS_CACHE/THIS_FRAME if the frame cache was cleared meanwhile. * frame.c (frame_cache_generation, get_frame_cache_generation): New. (reinit_frame_cache): Increment FRAME_CACHE_GENERATION. (get_prev_frame_if_no_cycle): On exception, don't touch PREV_FRAME/THIS_FRAME if the frame cache was cleared meanwhile. * frame.h (get_frame_cache_generation): Declare. gdb/testsuite/ChangeLog: * gdb.server/server-kill.exp (prepare): New, factored out from the top level. (kill_server): New. (test_tstatus, test_unwind_nosyms, test_unwind_syms): New. (top level) : Call test_tstatus, test_unwind_nosyms, test_unwind_syms. --- gdb/frame-unwind.c | 13 +++- gdb/frame.c | 22 +++++- gdb/frame.h | 4 ++ gdb/testsuite/gdb.server/server-kill.exp | 114 ++++++++++++++++++++++++------- 4 files changed, 124 insertions(+), 29 deletions(-) diff --git a/gdb/frame-unwind.c b/gdb/frame-unwind.c index 3334c472d02..064f6ebebda 100644 --- a/gdb/frame-unwind.c +++ b/gdb/frame-unwind.c @@ -121,6 +121,8 @@ frame_unwind_try_unwinder (struct frame_info *this_frame, void **this_cache, { int res = 0; + unsigned int entry_generation = get_frame_cache_generation (); + frame_prepare_for_sniffer (this_frame, unwinder); try @@ -130,9 +132,14 @@ frame_unwind_try_unwinder (struct frame_info *this_frame, void **this_cache, catch (const gdb_exception &ex) { /* Catch all exceptions, caused by either interrupt or error. - Reset *THIS_CACHE. */ - *this_cache = NULL; - frame_cleanup_after_sniffer (this_frame); + Reset *THIS_CACHE, unless something reinitialized the frame + cache meanwhile, in which case THIS_FRAME/THIS_CACHE are now + dangling. */ + if (get_frame_cache_generation () == entry_generation) + { + *this_cache = NULL; + frame_cleanup_after_sniffer (this_frame); + } if (ex.error == NOT_AVAILABLE_ERROR) { diff --git a/gdb/frame.c b/gdb/frame.c index ac1016b083f..a3599e8af69 100644 --- a/gdb/frame.c +++ b/gdb/frame.c @@ -53,6 +53,17 @@ static struct frame_info *sentinel_frame; +/* Number of calls to reinit_frame_cache. */ +static unsigned int frame_cache_generation = 0; + +/* See frame.h. */ + +unsigned int +get_frame_cache_generation () +{ + return frame_cache_generation; +} + /* The values behind the global "set backtrace ..." settings. */ set_backtrace_options user_set_backtrace_options; @@ -1843,6 +1854,8 @@ reinit_frame_cache (void) { struct frame_info *fi; + ++frame_cache_generation; + /* Tear down all frame caches. */ for (fi = sentinel_frame; fi != NULL; fi = fi->prev) { @@ -1922,6 +1935,8 @@ get_prev_frame_if_no_cycle (struct frame_info *this_frame) if (prev_frame->level == 0) return prev_frame; + unsigned int entry_generation = get_frame_cache_generation (); + try { compute_frame_id (prev_frame); @@ -1944,8 +1959,11 @@ get_prev_frame_if_no_cycle (struct frame_info *this_frame) } catch (const gdb_exception &ex) { - prev_frame->next = NULL; - this_frame->prev = NULL; + if (get_frame_cache_generation () == entry_generation) + { + prev_frame->next = NULL; + this_frame->prev = NULL; + } throw; } diff --git a/gdb/frame.h b/gdb/frame.h index cfc15022ed5..8d029cc065d 100644 --- a/gdb/frame.h +++ b/gdb/frame.h @@ -949,6 +949,10 @@ extern const gdb::option::option_def set_backtrace_option_defs[2]; /* The values behind the global "set backtrace ..." settings. */ extern set_backtrace_options user_set_backtrace_options; +/* Get the number of calls to reinit_frame_cache. */ + +unsigned int get_frame_cache_generation (); + /* Mark that the PC value is masked for the previous frame. */ extern void set_frame_previous_pc_masked (struct frame_info *frame); diff --git a/gdb/testsuite/gdb.server/server-kill.exp b/gdb/testsuite/gdb.server/server-kill.exp index 0072b28eb97..37b42460730 100644 --- a/gdb/testsuite/gdb.server/server-kill.exp +++ b/gdb/testsuite/gdb.server/server-kill.exp @@ -15,6 +15,9 @@ # You should have received a copy of the GNU General Public License # along with this program. If not, see <http://www.gnu.org/licenses/>. +# Check that GDB handles GDBserver disconnecting abruptly, in several +# scenarios. + load_lib gdbserver-support.exp standard_testfile @@ -23,40 +26,103 @@ if {[skip_gdbserver_tests]} { return 0 } -if { [prepare_for_testing "failed to prepare" ${testfile}] } { +if { [build_executable "failed to prepare" ${testfile}] } { return -1 } -# Make sure we're disconnected, in case we're testing with an -# extended-remote board, therefore already connected. -gdb_test "disconnect" ".*" +# Spawn GDBserver, run to main, extract GDBserver's PID and save it in +# the SERVER_PID global. + +proc prepare {} { + global binfile gdb_prompt srcfile decimal + global server_pid + + clean_restart $binfile + + # Make sure we're disconnected, in case we're testing with an + # extended-remote board, therefore already connected. + gdb_test "disconnect" ".*" -gdbserver_run "" + gdbserver_run "" -# Continue past server_pid assignment. -gdb_breakpoint ${srcfile}:[gdb_get_line_number "i = 0;"] -gdb_continue_to_breakpoint "after server_pid assignment" + # Continue past server_pid assignment. + gdb_breakpoint ${srcfile}:[gdb_get_line_number "i = 0;"] + gdb_continue_to_breakpoint "after server_pid assignment" -# Get the pid of GDBServer. -set test "p server_pid" -gdb_test_multiple $test $test { - -re " = ($decimal)\r\n$gdb_prompt $" { - set server_pid $expect_out(1,string) - pass $test + # Get the pid of GDBServer. + set test "p server_pid" + set server_pid 0 + gdb_test_multiple $test $test { + -re " = ($decimal)\r\n$gdb_prompt $" { + set server_pid $expect_out(1,string) + pass $test + } } + + if {$server_pid == 0} { + return 0 + } + + return 1 } -if ![info exists server_pid] { - return -1 +# Kill GDBserver using the PID saved by prepare. + +proc kill_server {} { + global server_pid + + remote_exec target "kill -9 $server_pid" +} + +# Test issuing "tstatus" right after the connection is dropped. + +proc_with_prefix test_tstatus {} { + if ![prepare] { + return + } + + kill_server + + # Enable trace status packet which is disabled after the + # connection if the remote target doesn't support tracepoint at + # all. Otherwise, no RSP packet is sent out. + gdb_test_no_output "set remote trace-status-packet on" + + # Force GDB to talk with GDBserver, so that we can get the + # "connection closed" error. + gdb_test "tstatus" {Remote connection closed|Remote communication error\. Target disconnected\.: Connection reset by peer\.} +} + +# Test unwinding with no debug/unwind info, right after the connection +# is dropped. + +proc_with_prefix test_unwind_nosyms {} { + if ![prepare] { + return + } + + # Remove symbols, so that we try to unwind with one of the + # heuristic unwinders, and read memory from within its sniffer. + gdb_unload + + kill_server + + gdb_test "bt" "(Target disconnected|Remote connection closed|Remote communication error).*" } -remote_exec target "kill -9 $server_pid" +# Test unwinding with debug/unwind info, right after the connection is +# dropped. -# Enable trace status packet which is disabled after the connection -# if the remote target doesn't support tracepoint at all. Otherwise, -# no RSP packet is sent out. -gdb_test_no_output "set remote trace-status-packet on" +proc_with_prefix test_unwind_syms {} { + if ![prepare] { + return + } + + kill_server + + gdb_test "bt" "(Target disconnected|Remote connection closed|Remote communication error).*" +} -# Force GDB to talk with GDBserver, so that we can get the -# "connection closed" error. -gdb_test "tstatus" {Remote connection closed|Remote communication error\. Target disconnected\.: Connection reset by peer\.} +test_tstatus +test_unwind_nosyms +test_unwind_syms base-commit: 32fa152e3bfcf021ce49767be547fae5129d922b -- 2.14.5 ^ permalink raw reply [flat|nested] 31+ messages in thread
end of thread, other threads:[~2020-11-16 14:57 UTC | newest] Thread overview: 31+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-07-08 23:31 [PATCH 0/3] Fix crash if connection drops in scoped_restore_current_thread's ctor Pedro Alves 2020-07-08 23:31 ` [PATCH 1/3] Fix crash if connection drops in scoped_restore_current_thread's ctor, part 1 Pedro Alves 2020-07-09 3:17 ` Simon Marchi 2020-07-09 10:51 ` Pedro Alves 2020-07-09 14:13 ` Simon Marchi 2020-07-08 23:31 ` [PATCH 2/3] Fix crash if connection drops in scoped_restore_current_thread's ctor, part 2 Pedro Alves 2020-07-09 3:31 ` Simon Marchi 2020-07-09 11:12 ` Pedro Alves 2020-07-09 14:16 ` Simon Marchi 2020-07-09 17:23 ` Pedro Alves 2020-07-09 17:28 ` Simon Marchi 2020-07-08 23:31 ` [PATCH 3/3] Make scoped_restore_current_thread's cdtors exception free (RFC) Pedro Alves 2020-07-09 3:49 ` Simon Marchi 2020-07-09 11:56 ` Pedro Alves 2020-07-09 12:09 ` Pedro Alves 2020-07-09 15:40 ` Simon Marchi 2020-07-09 22:22 ` Pedro Alves 2020-07-10 2:55 ` Simon Marchi 2020-10-30 1:13 ` Pedro Alves 2020-10-30 1:37 ` [pushed] Move lookup_selected_frame to frame.c Pedro Alves 2020-10-30 7:44 ` [PATCH 3/3] Make scoped_restore_current_thread's cdtors exception free (RFC) Aktemur, Tankut Baris 2020-10-30 11:32 ` Pedro Alves 2020-10-31 14:35 ` [PATCH] Fix frame cycle detection (Re: [PATCH 3/3] Make scoped_restore_current_thread's cdtors exception free (RFC)) Pedro Alves 2020-11-09 14:05 ` Aktemur, Tankut Baris 2020-11-16 13:48 ` Tom de Vries 2020-11-16 14:57 ` Pedro Alves 2020-07-10 23:02 ` [PATCH 0/3] Fix crash if connection drops in scoped_restore_current_thread's ctor Pedro Alves 2020-07-22 19:37 ` Simon Marchi 2020-07-22 20:37 ` Pedro Alves 2020-07-22 20:47 ` Simon Marchi 2020-07-23 15:28 ` [pushed] Don't touch frame_info objects if frame cache was reinitialized (was: Re: [PATCH 0/3] Fix crash if connection drops in scoped_restore_current_thread's ctor) Pedro Alves
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).