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

[Simon Marchi <simark@simark.ca>]

> @@ -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.

Simon