diff mbox series

[4/7] gdb: remove manual frame_info reinflation code in backtrace_command_1

Message ID 20221107155310.2590069-4-simon.marchi@polymtl.ca
State Committed
Commit 73cafdbd1ddd6ebfa21d737e3b69ae5aafd70c23
Headers
Series [1/7] gdb: clear other.m_cached_id in frame_info_ptr's move ctor |

Commit Message

Simon Marchi Nov. 7, 2022, 3:53 p.m. UTC 
  From: Simon Marchi <simon.marchi@efficios.com>

With the following patch applied (gdb: use frame_id_p instead of
comparing to null_frame_id in frame_info_ptr::reinflate), I would get:

    $ ./gdb -q -nx --data-directory=data-directory testsuite/outputs/gdb.base/bt-selected-frame/bt-selected-frame -ex "b breakpt" -ex r -ex "bt full"
    Reading symbols from testsuite/outputs/gdb.base/bt-selected-frame/bt-selected-frame...
    Breakpoint 1 at 0x1131: file /home/smarchi/src/binutils-gdb/gdb/testsuite/gdb.base/bt-selected-frame.c, line 22.
    Starting program: /home/smarchi/build/binutils-gdb/gdb/testsuite/outputs/gdb.base/bt-selected-frame/bt-selected-frame
    [Thread debugging using libthread_db enabled]
    Using host libthread_db library "/lib/x86_64-linux-gnu/libthread_db.so.1".

    Breakpoint 1, breakpt () at /home/smarchi/src/binutils-gdb/gdb/testsuite/gdb.base/bt-selected-frame.c:22
    22      }
    #0  breakpt () at /home/smarchi/src/binutils-gdb/gdb/testsuite/gdb.base/bt-selected-frame.c:22
    No locals.
    /home/smarchi/src/binutils-gdb/gdb/frame-info.c:42: internal-error: reinflate: Assertion `frame_id_p (m_cached_id)' failed.

This is because the code in backtrace_command_1 to manually reinflate
`fi` steps overs frame_info_ptr's toes.

When calling

    fi.prepare_reinflate ();

`fi` gets properly filled with the cached frame id.  But when this
happens:

    fi = frame_find_by_id (frame_id);

`fi` gets replaced by a brand new frame_info_ptr that doesn't have a
cached frame id.  Then this is called without a cached frame id:

    fi.reinflate ();

That doesn't cause any problem currently, since

 - the gdb_assert in the reinflate method doesn't actually do anything
   (the following patch fixes that)
 - `fi.m_ptr` will always be non-nullptr, since we just got it from
   frame_find_by_id, so reinflate will not do anything, it won't try to
   use m_cached_id

Fix that by removing the code to manually re-fetch the frame.  That
should be taken care of by frame_info_ptr::reinflate.

Note that the old code checked if we successfully re-inflated the frame
or not, and if not it did emit a warning.  The equivalent in
frame_info_ptr::reinflate asserts that the frame has been successfully
re-inflated.  It's not clear if / when this can happen, but if it can
happen, we'll need to find a solution to this problem globally
(everywhere a frame_info_ptr can be re-inflated), not just here.  So I
propose to leave it like this, until it does become a problem.

Change-Id: I07b783d94e2853e0a2d058fe7deaf04eddf24835
---
 gdb/stack.c | 15 +--------------
 1 file changed, 1 insertion(+), 14 deletions(-)

Comments

Guinevere Larsen Nov. 8, 2022, 10:14 a.m. UTC | #1 
On 07/11/2022 16:53, Simon Marchi via Gdb-patches wrote:
> From: Simon Marchi <simon.marchi@efficios.com>
>
> With the following patch applied (gdb: use frame_id_p instead of
> comparing to null_frame_id in frame_info_ptr::reinflate), I would get:
>
>      $ ./gdb -q -nx --data-directory=data-directory testsuite/outputs/gdb.base/bt-selected-frame/bt-selected-frame -ex "b breakpt" -ex r -ex "bt full"
>      Reading symbols from testsuite/outputs/gdb.base/bt-selected-frame/bt-selected-frame...
>      Breakpoint 1 at 0x1131: file /home/smarchi/src/binutils-gdb/gdb/testsuite/gdb.base/bt-selected-frame.c, line 22.
>      Starting program: /home/smarchi/build/binutils-gdb/gdb/testsuite/outputs/gdb.base/bt-selected-frame/bt-selected-frame
>      [Thread debugging using libthread_db enabled]
>      Using host libthread_db library "/lib/x86_64-linux-gnu/libthread_db.so.1".
>
>      Breakpoint 1, breakpt () at /home/smarchi/src/binutils-gdb/gdb/testsuite/gdb.base/bt-selected-frame.c:22
>      22      }
>      #0  breakpt () at /home/smarchi/src/binutils-gdb/gdb/testsuite/gdb.base/bt-selected-frame.c:22
>      No locals.
>      /home/smarchi/src/binutils-gdb/gdb/frame-info.c:42: internal-error: reinflate: Assertion `frame_id_p (m_cached_id)' failed.
>
> This is because the code in backtrace_command_1 to manually reinflate
> `fi` steps overs frame_info_ptr's toes.
>
> When calling
>
>      fi.prepare_reinflate ();
>
> `fi` gets properly filled with the cached frame id.  But when this
> happens:
>
>      fi = frame_find_by_id (frame_id);
>
> `fi` gets replaced by a brand new frame_info_ptr that doesn't have a
> cached frame id.  Then this is called without a cached frame id:
>
>      fi.reinflate ();
>
> That doesn't cause any problem currently, since
>
>   - the gdb_assert in the reinflate method doesn't actually do anything
>     (the following patch fixes that)
>   - `fi.m_ptr` will always be non-nullptr, since we just got it from
>     frame_find_by_id, so reinflate will not do anything, it won't try to
>     use m_cached_id
>
> Fix that by removing the code to manually re-fetch the frame.  That
> should be taken care of by frame_info_ptr::reinflate.
>
> Note that the old code checked if we successfully re-inflated the frame
> or not, and if not it did emit a warning.  The equivalent in
> frame_info_ptr::reinflate asserts that the frame has been successfully
> re-inflated.  It's not clear if / when this can happen, but if it can
> happen, we'll need to find a solution to this problem globally
> (everywhere a frame_info_ptr can be re-inflated), not just here.  So I
> propose to leave it like this, until it does become a problem.
>
I think this patch could be squashed with the one you mention at the top 
of the commit message. I think they are both related to the same thing 
(fixing the assert and dealing with fallout) and I don't see much point 
in separating how you did here.
Simon Marchi Nov. 8, 2022, 4:05 p.m. UTC | #2 
On 11/8/22 05:14, Bruno Larsen wrote:
> On 07/11/2022 16:53, Simon Marchi via Gdb-patches wrote:
>> From: Simon Marchi <simon.marchi@efficios.com>
>>
>> With the following patch applied (gdb: use frame_id_p instead of
>> comparing to null_frame_id in frame_info_ptr::reinflate), I would get:
>>
>>      $ ./gdb -q -nx --data-directory=data-directory testsuite/outputs/gdb.base/bt-selected-frame/bt-selected-frame -ex "b breakpt" -ex r -ex "bt full"
>>      Reading symbols from testsuite/outputs/gdb.base/bt-selected-frame/bt-selected-frame...
>>      Breakpoint 1 at 0x1131: file /home/smarchi/src/binutils-gdb/gdb/testsuite/gdb.base/bt-selected-frame.c, line 22.
>>      Starting program: /home/smarchi/build/binutils-gdb/gdb/testsuite/outputs/gdb.base/bt-selected-frame/bt-selected-frame
>>      [Thread debugging using libthread_db enabled]
>>      Using host libthread_db library "/lib/x86_64-linux-gnu/libthread_db.so.1".
>>
>>      Breakpoint 1, breakpt () at /home/smarchi/src/binutils-gdb/gdb/testsuite/gdb.base/bt-selected-frame.c:22
>>      22      }
>>      #0  breakpt () at /home/smarchi/src/binutils-gdb/gdb/testsuite/gdb.base/bt-selected-frame.c:22
>>      No locals.
>>      /home/smarchi/src/binutils-gdb/gdb/frame-info.c:42: internal-error: reinflate: Assertion `frame_id_p (m_cached_id)' failed.
>>
>> This is because the code in backtrace_command_1 to manually reinflate
>> `fi` steps overs frame_info_ptr's toes.
>>
>> When calling
>>
>>      fi.prepare_reinflate ();
>>
>> `fi` gets properly filled with the cached frame id.  But when this
>> happens:
>>
>>      fi = frame_find_by_id (frame_id);
>>
>> `fi` gets replaced by a brand new frame_info_ptr that doesn't have a
>> cached frame id.  Then this is called without a cached frame id:
>>
>>      fi.reinflate ();
>>
>> That doesn't cause any problem currently, since
>>
>>   - the gdb_assert in the reinflate method doesn't actually do anything
>>     (the following patch fixes that)
>>   - `fi.m_ptr` will always be non-nullptr, since we just got it from
>>     frame_find_by_id, so reinflate will not do anything, it won't try to
>>     use m_cached_id
>>
>> Fix that by removing the code to manually re-fetch the frame.  That
>> should be taken care of by frame_info_ptr::reinflate.
>>
>> Note that the old code checked if we successfully re-inflated the frame
>> or not, and if not it did emit a warning.  The equivalent in
>> frame_info_ptr::reinflate asserts that the frame has been successfully
>> re-inflated.  It's not clear if / when this can happen, but if it can
>> happen, we'll need to find a solution to this problem globally
>> (everywhere a frame_info_ptr can be re-inflated), not just here.  So I
>> propose to leave it like this, until it does become a problem.
>>
> I think this patch could be squashed with the one you mention at the top of the commit message. I think they are both related to the same thing (fixing the assert and dealing with fallout) and I don't see much point in separating how you did here.

I think it would be fine to have them merged or have them separate, but
at this point I'd prefer to keep it like this, since it's done.

Simon
diff mbox series

Patch

diff --git a/gdb/stack.c b/gdb/stack.c
index 4e2342c2a8d9..5f29566fcfe9 100644
--- a/gdb/stack.c
+++ b/gdb/stack.c
@@ -2082,20 +2082,7 @@  backtrace_command_1 (const frame_print_options &fp_opts,
 
 	  print_frame_info (fp_opts, fi, 1, LOCATION, 1, 0);
 	  if ((flags & PRINT_LOCALS) != 0)
-	    {
-	      struct frame_id frame_id = get_frame_id (fi);
-
-	      print_frame_local_vars (fi, false, NULL, NULL, 1, gdb_stdout);
-
-	      /* print_frame_local_vars invalidates FI.  */
-	      fi = frame_find_by_id (frame_id);
-	      if (fi == NULL)
-		{
-		  trailing = NULL;
-		  warning (_("Unable to restore previously selected frame."));
-		  break;
-		}
-	    }
+	    print_frame_local_vars (fi, false, NULL, NULL, 1, gdb_stdout);
 
 	  /* Save the last frame to check for error conditions.  */
 	  fi.reinflate ();