Re: [PATCH] Remove unnecessary get_current_frame calls from infrun.c

[Bernd Edlinger <bernd.edlinger@hotmail.de>]

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