Re: [PATCH] gdb: fix dealloc function not being called for frame 0

[Tom de Vries <tdevries@suse.de>]

[-- Attachment #1: Type: text/plain, Size: 4984 bytes --]

On 2/10/23 04:10, Simon Marchi wrote:
> 
>> @@ -2105,7 +2106,23 @@ reinit_frame_cache (void)
>>     invalidate_selected_frame ();
>>   
>>     /* Invalidate cache.  */
>> -  sentinel_frame = NULL;
>> +  if (sentinel_frame != nullptr)
>> +    {
>> +      /* If frame 0's id is not computed, it is not in the frame stash, so its
>> +	 dealloc functions will not be called when emptying the frash stash.
>> +	 Call frame_info_del manually in that case.  */
>> +      frame_info *current_frame = sentinel_frame->prev;
>> +      if (current_frame != nullptr)
>> +	{
>> +	  gdb_assert (current_frame->this_id.p != frame_id_status::COMPUTING);
> 
> Well, this gdb_assert happens to cause a failure in
> gdb.server/server-kill.exp:
> 
>      (gdb) PASS: gdb.server/server-kill.exp: kill_pid_of=inferior: test_unwind_syms: p server_pid
>      Executing on target: kill -9 1567851    (timeout = 300)
>      builtin_spawn -ignore SIGHUP kill -9 1567851
>      bt
>      /home/simark/src/binutils-gdb/gdb/frame.c:2117: internal-error: reinit_frame_cache: Assertion `current_frame->this_id.p != frame_id_status::COMPUTING' failed.
>      A problem internal to GDB has been detected, further debugging may prove unreliable.
>      FAIL: gdb.server/server-kill.exp: kill_pid_of=inferior: test_unwind_syms: bt (GDB internal error)
> 
> This happens because the remote connection breaks while computing a
> frame id.  The stack looks roughly like this (many frames removed for
> readability):
> 
> 0x55cd4623396c internal_error_loc(char const*, int, char const*, ...)
>          /home/simark/src/binutils-gdb/gdbsupport/errors.cc:58
> 0x55cd4434f5a1 reinit_frame_cache()
>          /home/simark/src/binutils-gdb/gdb/frame.c:2117
> 0x55cd457e1bce switch_to_no_thread()
>          /home/simark/src/binutils-gdb/gdb/thread.c:1330
> 0x55cd44618270 switch_to_inferior_no_thread(inferior*)
>          /home/simark/src/binutils-gdb/gdb/inferior.c:671
> 0x55cd451467c9 remote_unpush_target
>          /home/simark/src/binutils-gdb/gdb/remote.c:5903
> 0x55cd45183b88 unpush_and_perror
>          /home/simark/src/binutils-gdb/gdb/remote.c:9612
> 0x55cd451840ce remote_target::readchar(int)
>          /home/simark/src/binutils-gdb/gdb/remote.c:9652
> 0x55cd45188e91 remote_target::getpkt_or_notif_sane_1(std::__debug::vector<char, gdb::default_init_allocator<char, std::allocator<char> > >*, int, int, int*)
>          /home/simark/src/binutils-gdb/gdb/remote.c:10118
> 0x55cd4518a2fe remote_target::getpkt_sane(std::__debug::vector<char, gdb::default_init_allocator<char, std::allocator<char> > >*, int)
>          /home/simark/src/binutils-gdb/gdb/remote.c:10220
> 0x55cd451889d6 remote_target::getpkt(std::__debug::vector<char, gdb::default_init_allocator<char, std::allocator<char> > >*, int)
>          /home/simark/src/binutils-gdb/gdb/remote.c:10062
> 0x55cd45181137 remote_target::remote_read_bytes_1(unsigned long, unsigned char*, unsigned long, int, unsigned long*)
>          /home/simark/src/binutils-gdb/gdb/remote.c:9379
> 0x55cd45182772 remote_target::remote_read_bytes(unsigned long, unsigned char*, unsigned long, int, unsigned long*)
>          /home/simark/src/binutils-gdb/gdb/remote.c:9503
> 0x55cd4519cd2d remote_target::xfer_partial(target_object, char const*, unsigned char*, unsigned char const*, unsigned long, unsigned long, unsigned long*)
>          /home/simark/src/binutils-gdb/gdb/remote.c:11421
> ...
> 0x55cd4433d812 compute_frame_id
>          /home/simark/src/binutils-gdb/gdb/frame.c:606
> 
> So, I will remove that gdb_assert and simplify the code to:
> 
>        /* If frame 0's id is not computed, it is not in the frame stash, so its
> 	 dealloc functions will not be called when emptying the frash stash.
> 	 Call frame_info_del manually in that case.  */
>        frame_info *current_frame = sentinel_frame->prev;
>        if (current_frame != nullptr
> 	  && current_frame->this_id.p == frame_id_status::NOT_COMPUTED)
> 	frame_info_del (current_frame);
> 
> With that change, the test passes again.
> 
> If the state of the id of the current frame is "COMPUTING", I think it's
> better not to call frame_info_del and the dealloc functions, because the
> state of the per-unwinder cache object is maybe not in a state expected
> that the dealloc functions expect.  If something goes wrong during the
> computation of the frame id, I think it's best to let the unwinder make
> sure to clean up anything it has started.

Hi Simon,

I've:
- applied the initial patch
- verified that it fixes the gdb.btrace/record_goto.exp test-case
- verified that it breaks the gdb.server/server-kill.exp test-case
- applied your proposed change (attached in patch form for clarity)
- verified that it fixes the gdb.server/server-kill.exp test-case
- verified that it still fixes the gdb.btrace/record_goto.exp test-case
- done a complete build and test run, no issues found (in other words,
   I'm back to just one FAIL, in gdb.server/monitor-exit-quit.exp,
   PR29833)

Thanks,
- Tom

[-- Attachment #2: 0001-fixup.patch --]
[-- Type: text/x-patch, Size: 1073 bytes --]

From a63fe09438deba286464ac79685587890ea8ae39 Mon Sep 17 00:00:00 2001
From: Simon Marchi <simon.marchi@efficios.com>
Date: Wed, 15 Feb 2023 09:43:35 +0100
Subject: [PATCH] fixup

---
 gdb/frame.c | 10 +++-------
 1 file changed, 3 insertions(+), 7 deletions(-)

diff --git a/gdb/frame.c b/gdb/frame.c
index 69730979703..6629f444afe 100644
--- a/gdb/frame.c
+++ b/gdb/frame.c
@@ -2112,13 +2112,9 @@ reinit_frame_cache (void)
 	 dealloc functions will not be called when emptying the frash stash.
 	 Call frame_info_del manually in that case.  */
       frame_info *current_frame = sentinel_frame->prev;
-      if (current_frame != nullptr)
-	{
-	  gdb_assert (current_frame->this_id.p != frame_id_status::COMPUTING);
-
-	  if (current_frame->this_id.p == frame_id_status::NOT_COMPUTED)
-	    frame_info_del (current_frame);
-	}
+      if (current_frame != nullptr
+	  && current_frame->this_id.p == frame_id_status::NOT_COMPUTED)
+	frame_info_del (current_frame);
 
       sentinel_frame = nullptr;
     }

base-commit: b6daa77d0d91216266bccbe182cc7dc689591692
-- 
2.35.3