Mirror of the gdb-patches mailing list
 help / color / mirror / Atom feed
* [PATCH] Remove unnecessary get_current_frame calls from infrun.c
@ 2024-03-31 10:50 Bernd Edlinger
  2024-04-01  2:52 ` Simon Marchi
  2024-04-26 16:31 ` Tom Tromey
  0 siblings, 2 replies; 7+ messages in thread
From: Bernd Edlinger @ 2024-03-31 10:50 UTC (permalink / raw)
  To: gdb-patches

Since the frame variable is now a frame_info_ptr, the issue
with the dangling frame pointer is apparently no longer there.

So remove the re-fetch code and the corresponding meanwhile
misleading comments.
---
 gdb/infrun.c | 24 ++++--------------------
 1 file changed, 4 insertions(+), 20 deletions(-)

diff --git a/gdb/infrun.c b/gdb/infrun.c
index a5030b16376..521c3b0299c 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -7056,11 +7056,6 @@ handle_signal_stop (struct execution_control_state *ecs)
 					   ecs->event_thread->stop_pc (),
 					   ecs->ws);
 	  skip_inline_frames (ecs->event_thread, stop_chain);
-
-	  /* Re-fetch current thread's frame in case that invalidated
-	     the frame cache.  */
-	  frame = get_current_frame ();
-	  gdbarch = get_frame_arch (frame);
 	}
     }
 
@@ -7419,12 +7414,6 @@ process_event_stop_test (struct execution_control_state *ecs)
      bp_jit_event).  Run them now.  */
   bpstat_run_callbacks (ecs->event_thread->control.stop_bpstat);
 
-  /* If we hit an internal event that triggers symbol changes, the
-     current frame will be invalidated within bpstat_what (e.g., if we
-     hit an internal solib event).  Re-fetch it.  */
-  frame = get_current_frame ();
-  gdbarch = get_frame_arch (frame);
-
   /* Shorthand to make if statements smaller.  */
   struct frame_id original_frame_id
     = ecs->event_thread->control.step_frame_id;
@@ -7670,11 +7659,6 @@ process_event_stop_test (struct execution_control_state *ecs)
       return;
     }
 
-  /* Re-fetch current thread's frame in case the code above caused
-     the frame cache to be re-initialized, making our FRAME variable
-     a dangling pointer.  */
-  frame = get_current_frame ();
-  gdbarch = get_frame_arch (frame);
   fill_in_stop_func (gdbarch, ecs);
 
   /* If stepping through a line, keep going if still within it.
@@ -7855,7 +7839,7 @@ process_event_stop_test (struct execution_control_state *ecs)
   if ((get_stack_frame_id (frame)
        != ecs->event_thread->control.step_stack_frame_id)
       && get_frame_type (frame) != SIGTRAMP_FRAME
-      && ((frame_unwind_caller_id (get_current_frame ())
+      && ((frame_unwind_caller_id (frame)
 	   == ecs->event_thread->control.step_stack_frame_id)
 	  && ((ecs->event_thread->control.step_stack_frame_id
 	       != outer_frame_id)
@@ -8138,7 +8122,7 @@ process_event_stop_test (struct execution_control_state *ecs)
     {
       infrun_debug_printf ("stepped into inlined function");
 
-      symtab_and_line call_sal = find_frame_sal (get_current_frame ());
+      symtab_and_line call_sal = find_frame_sal (frame);
 
       if (ecs->event_thread->control.step_over_calls != STEP_OVER_ALL)
 	{
@@ -8180,9 +8164,9 @@ process_event_stop_test (struct execution_control_state *ecs)
      to go further up to find the exact frame ID, we are stepping
      through a more inlined call beyond its call site.  */
 
-  if (get_frame_type (get_current_frame ()) == INLINE_FRAME
+  if (get_frame_type (frame) == INLINE_FRAME
       && (*curr_frame_id != original_frame_id)
-      && stepped_in_from (get_current_frame (), original_frame_id))
+      && stepped_in_from (frame, original_frame_id))
     {
       infrun_debug_printf ("stepping through inlined function");
 
-- 
2.39.2


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] Remove unnecessary get_current_frame calls from infrun.c
  2024-03-31 10:50 [PATCH] Remove unnecessary get_current_frame calls from infrun.c Bernd Edlinger
@ 2024-04-01  2:52 ` Simon Marchi
  2024-04-01 11:45   ` Bernd Edlinger
  2024-04-01 16:07   ` Tom Tromey
  2024-04-26 16:31 ` Tom Tromey
  1 sibling, 2 replies; 7+ messages in thread
From: Simon Marchi @ 2024-04-01  2:52 UTC (permalink / raw)
  To: Bernd Edlinger, gdb-patches



On 2024-03-31 06:50, Bernd Edlinger wrote:
> Since the frame variable is now a frame_info_ptr, the issue
> with the dangling frame pointer is apparently no longer there.
> 
> So remove the re-fetch code and the corresponding meanwhile
> misleading comments.
> ---
>  gdb/infrun.c | 24 ++++--------------------
>  1 file changed, 4 insertions(+), 20 deletions(-)
> 
> diff --git a/gdb/infrun.c b/gdb/infrun.c
> index a5030b16376..521c3b0299c 100644
> --- a/gdb/infrun.c
> +++ b/gdb/infrun.c
> @@ -7056,11 +7056,6 @@ handle_signal_stop (struct execution_control_state *ecs)
>  					   ecs->event_thread->stop_pc (),
>  					   ecs->ws);
>  	  skip_inline_frames (ecs->event_thread, stop_chain);
> -
> -	  /* Re-fetch current thread's frame in case that invalidated
> -	     the frame cache.  */
> -	  frame = get_current_frame ();
> -	  gdbarch = get_frame_arch (frame);

For `frame` I agree, I think we can remove it.  But I'm wondering about
`gdbarch`.  Before we had `frame_info_ptr`, even if `frame` got
invalidated, it didn't seem necessary to reset `gdbarch`.  Are there
cases where you would get a different value for `gdbarch` as it
currently holds?  I can't think of any.  I'm leaning towards saying that
this is fine.

This is the patch that introduced it, if you want more context:

https://inbox.sourceware.org/gdb-patches/alpine.DEB.1.10.1206121703180.23962@tp.orcam.me.uk/#t

Simon

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] Remove unnecessary get_current_frame calls from infrun.c
  2024-04-01  2:52 ` Simon Marchi
@ 2024-04-01 11:45   ` Bernd Edlinger
  2024-04-01 16:07   ` Tom Tromey
  1 sibling, 0 replies; 7+ messages in thread
From: Bernd Edlinger @ 2024-04-01 11:45 UTC (permalink / raw)
  To: Simon Marchi, gdb-patches



On 4/1/24 04:52, Simon Marchi wrote:
> 
> 
> On 2024-03-31 06:50, Bernd Edlinger wrote:
>> Since the frame variable is now a frame_info_ptr, the issue
>> with the dangling frame pointer is apparently no longer there.
>>
>> So remove the re-fetch code and the corresponding meanwhile
>> misleading comments.
>> ---
>>  gdb/infrun.c | 24 ++++--------------------
>>  1 file changed, 4 insertions(+), 20 deletions(-)
>>
>> diff --git a/gdb/infrun.c b/gdb/infrun.c
>> index a5030b16376..521c3b0299c 100644
>> --- a/gdb/infrun.c
>> +++ b/gdb/infrun.c
>> @@ -7056,11 +7056,6 @@ handle_signal_stop (struct execution_control_state *ecs)
>>  					   ecs->event_thread->stop_pc (),
>>  					   ecs->ws);
>>  	  skip_inline_frames (ecs->event_thread, stop_chain);
>> -
>> -	  /* Re-fetch current thread's frame in case that invalidated
>> -	     the frame cache.  */
>> -	  frame = get_current_frame ();
>> -	  gdbarch = get_frame_arch (frame);
> 
> For `frame` I agree, I think we can remove it.  But I'm wondering about
> `gdbarch`.  Before we had `frame_info_ptr`, even if `frame` got
> invalidated, it didn't seem necessary to reset `gdbarch`.  Are there
> cases where you would get a different value for `gdbarch` as it
> currently holds?  I can't think of any.  I'm leaning towards saying that
> this is fine.

Yes I was not sure either, and I tried first to add those assertions

gdb_assert(frame == get_current_frame ());
gdb_assert(gdbarch == get_frame_arch (frame));
gdb_assert(*curr_frame_id == get_frame_id (frame));

since I had previously learned the hard way what could happen here.
All those assertions did not trigger anywhere in "make check-gdb"
But gdb_assert(frame.get() == previous_frame_get_value); did trigger
at various places.

> 
> This is the patch that introduced it, if you want more context:
> 
> https://inbox.sourceware.org/gdb-patches/alpine.DEB.1.10.1206121703180.23962@tp.orcam.me.uk/#t
> 

Yeah, reading that it sounds like the author did mean that he was not
sure if the gdbarch is still valid or not, but it would look odd to
refresh gdbarch here while not refreshing frame.
I guess gdbarch objects they are more or less static by nature,
but I was curious as well to find where they do actually come from.
I think there are only very few different values of gdbarch possible here:

My experiment below shows there are only 3 objects ever allocated (for x86_64):

diff --git a/gdb/arch-utils.c b/gdb/arch-utils.c
index 456bfe971ff..30207db9694 100644
--- a/gdb/arch-utils.c
+++ b/gdb/arch-utils.c
@@ -1233,6 +1233,7 @@ gdbarch_obstack_strdup (struct gdbarch *arch, const char *string)
 void
 gdbarch_free (struct gdbarch *arch)
 {
+  printf("free gdbarch=%p\n", arch);
   gdb_assert (arch != NULL);
   gdb_assert (!arch->initialized_p);
   delete arch;
diff --git a/gdb/gdbarch.c b/gdb/gdbarch.c
index 9319571deba..79e8dcec122 100644
--- a/gdb/gdbarch.c
+++ b/gdb/gdbarch.c
@@ -278,6 +278,7 @@ gdbarch_alloc (const struct gdbarch_info *info,
   gdbarch->byte_order_for_code = info->byte_order_for_code;
   gdbarch->osabi = info->osabi;
   gdbarch->target_desc = info->target_desc;
+  printf("gdbarch=%p\n", gdbarch);
 
   return gdbarch;
 }

One of the 3 allocated objects is returned from get_frame_arch every time.
And since the gdbarch_free function is apparently never called,
there are probably memory leaks somewhere, which wouldn't surprise me.


Bernd.

> Simon

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] Remove unnecessary get_current_frame calls from infrun.c
  2024-04-01  2:52 ` Simon Marchi
  2024-04-01 11:45   ` Bernd Edlinger
@ 2024-04-01 16:07   ` Tom Tromey
  2024-04-01 17:00     ` Bernd Edlinger
  1 sibling, 1 reply; 7+ messages in thread
From: Tom Tromey @ 2024-04-01 16:07 UTC (permalink / raw)
  To: Simon Marchi; +Cc: Bernd Edlinger, gdb-patches

>>>>> "Simon" == Simon Marchi <simark@simark.ca> writes:

>> -	  /* Re-fetch current thread's frame in case that invalidated
>> -	     the frame cache.  */
>> -	  frame = get_current_frame ();
>> -	  gdbarch = get_frame_arch (frame);

Simon> For `frame` I agree, I think we can remove it.  But I'm wondering about
Simon> `gdbarch`.  Before we had `frame_info_ptr`, even if `frame` got
Simon> invalidated, it didn't seem necessary to reset `gdbarch`.  Are there
Simon> cases where you would get a different value for `gdbarch` as it
Simon> currently holds?  I can't think of any.  I'm leaning towards saying that
Simon> this is fine.

I think the main danger in this patch is if the current comments are
wrong -- that is, when it is referring to something reinitializing the
frame cache, is that correct?  Or could the inferior be restarted
somewhere in here, resulting in a different current frame?  In the
latter case get_current_frame definitely has to be called again, the old
frame may not even exist.

I don't think it's possible for the gdbarch to change mid-frame, only
between frames.  This capability has been desired a few times (x86
boot-up apparently switches modes or something), but patches have never
been supplied.

So, I tend to think not resetting gdbarch is fine.

Tom

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] Remove unnecessary get_current_frame calls from infrun.c
  2024-04-01 16:07   ` Tom Tromey
@ 2024-04-01 17:00     ` Bernd Edlinger
  0 siblings, 0 replies; 7+ messages in thread
From: Bernd Edlinger @ 2024-04-01 17:00 UTC (permalink / raw)
  To: Tom Tromey, Simon Marchi; +Cc: gdb-patches

On 4/1/24 18:07, Tom Tromey wrote:
>>>>>> "Simon" == Simon Marchi <simark@simark.ca> writes:
> 
>>> -	  /* Re-fetch current thread's frame in case that invalidated
>>> -	     the frame cache.  */
>>> -	  frame = get_current_frame ();
>>> -	  gdbarch = get_frame_arch (frame);
> 
> Simon> For `frame` I agree, I think we can remove it.  But I'm wondering about
> Simon> `gdbarch`.  Before we had `frame_info_ptr`, even if `frame` got
> Simon> invalidated, it didn't seem necessary to reset `gdbarch`.  Are there
> Simon> cases where you would get a different value for `gdbarch` as it
> Simon> currently holds?  I can't think of any.  I'm leaning towards saying that
> Simon> this is fine.
> 
> I think the main danger in this patch is if the current comments are
> wrong -- that is, when it is referring to something reinitializing the
> frame cache, is that correct?  Or could the inferior be restarted
> somewhere in here, resulting in a different current frame?  In the
> latter case get_current_frame definitely has to be called again, the old
> frame may not even exist.
> 

I am definitely sure that the comments refer to the previous side-effects of
`skip_inline_frames` and `step_into_inline_frame` and probably others which
may call `reinit_frame_cache`.  But even before the invention of `frame_info_ptr`
the `gdbarch *` was never really changing because of `reinit_frame_cache`.
My experiments show at least that currently the gdbarch pointer will not become
released while stepping, not even when re-loading the inferior the gdbarch pointer
has ever changed for me.

> I don't think it's possible for the gdbarch to change mid-frame, only
> between frames.  This capability has been desired a few times (x86
> boot-up apparently switches modes or something), but patches have never
> been supplied.
> 
> So, I tend to think not resetting gdbarch is fine.
> 

Yep, is the patch approved then?

Thanks,
Bernd.

> Tom

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] Remove unnecessary get_current_frame calls from infrun.c
  2024-03-31 10:50 [PATCH] Remove unnecessary get_current_frame calls from infrun.c Bernd Edlinger
  2024-04-01  2:52 ` Simon Marchi
@ 2024-04-26 16:31 ` Tom Tromey
  2024-04-26 17:47   ` Bernd Edlinger
  1 sibling, 1 reply; 7+ messages in thread
From: Tom Tromey @ 2024-04-26 16:31 UTC (permalink / raw)
  To: Bernd Edlinger; +Cc: gdb-patches

>>>>> "Bernd" == Bernd Edlinger <bernd.edlinger@hotmail.de> writes:

Bernd> Since the frame variable is now a frame_info_ptr, the issue
Bernd> with the dangling frame pointer is apparently no longer there.

Bernd> So remove the re-fetch code and the corresponding meanwhile
Bernd> misleading comments.

This kind of dropped off my radar for a while, sorry about that.
AFAIK this didn't go in yet?  Anyway I think it is ok.

Approved-By: Tom Tromey <tom@tromey.com>

Tom

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] Remove unnecessary get_current_frame calls from infrun.c
  2024-04-26 16:31 ` Tom Tromey
@ 2024-04-26 17:47   ` Bernd Edlinger
  0 siblings, 0 replies; 7+ messages in thread
From: Bernd Edlinger @ 2024-04-26 17:47 UTC (permalink / raw)
  To: Tom Tromey; +Cc: gdb-patches

On 4/26/24 18:31, Tom Tromey wrote:
>>>>>> "Bernd" == Bernd Edlinger <bernd.edlinger@hotmail.de> writes:
> 
> Bernd> Since the frame variable is now a frame_info_ptr, the issue
> Bernd> with the dangling frame pointer is apparently no longer there.
> 
> Bernd> So remove the re-fetch code and the corresponding meanwhile
> Bernd> misleading comments.
> 
> This kind of dropped off my radar for a while, sorry about that.
> AFAIK this didn't go in yet?  Anyway I think it is ok.
> 
> Approved-By: Tom Tromey <tom@tromey.com>
> 
> Tom

Pushed as 354f8d0a1f9.

Thanks
Bernd.

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2024-04-26 17:45 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-03-31 10:50 [PATCH] Remove unnecessary get_current_frame calls from infrun.c Bernd Edlinger
2024-04-01  2:52 ` Simon Marchi
2024-04-01 11:45   ` Bernd Edlinger
2024-04-01 16:07   ` Tom Tromey
2024-04-01 17:00     ` Bernd Edlinger
2024-04-26 16:31 ` Tom Tromey
2024-04-26 17:47   ` Bernd Edlinger

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox