Remove unnecessary get_current_frame calls from infrun.c

  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(-)
  

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
  

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
  

>>>>> "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
  

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
  

>>>>> "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
  

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.