Re: [PATCH 1/3] gdb: remove unpush_target free function

[Simon Marchi <simon.marchi@polymtl.ca>]

On 2021-03-22 12:22 p.m., Aktemur, Tankut Baris wrote:
> On Monday, March 22, 2021 4:20 AM, Simon Marchi Wrote:
>> unpush_target unpushes the passed-in target from the current inferior's
>> target stack.  Calling it is therefore an implicit dependency on the
>> current global inferior.  Remove that function and make the callers use
>> the inferior::unpush_target method directly.  This sometimes allows
>> using the inferior from the context rather than the global current
>> inferior.
>>
>> target_unpusher::operator() now needs to be implemented in target.c,
>> otherwise target.h and inferior.h both need to include each other, and
>> that wouldn't work.
>>
>> gdb/ChangeLog:
>>
>> 	* target.h (unpush_target): Remove, update all callers
>> 	to use `inferior::unpush_target` instead.
>> 	(struct target_unpusher) <operator()>: Just declare.
>> 	* target.c (unpush_target): Remove.
>> 	(target_unpusher::operator()): New.
>>
>> Change-Id: Ia5172dfb3f373e0a75b991885b50322ca2142a8c
>> ---
>>  gdb/aix-thread.c       |  2 +-
>>  gdb/bsd-kvm.c          |  2 +-
>>  gdb/bsd-uthread.c      |  2 +-
>>  gdb/corelow.c          |  2 +-
>>  gdb/exec.c             |  2 +-
>>  gdb/inf-child.c        |  2 +-
>>  gdb/linux-thread-db.c  |  6 +++---
>>  gdb/ravenscar-thread.c |  2 +-
>>  gdb/record-btrace.c    |  2 +-
>>  gdb/record-full.c      |  2 +-
>>  gdb/record.c           |  2 +-
>>  gdb/remote-sim.c       |  4 ++--
>>  gdb/sol-thread.c       |  4 ++--
>>  gdb/target.c           | 18 ++++++++----------
>>  gdb/target.h           |  7 +------
>>  gdb/tracefile-tfile.c  |  4 ++--
>>  16 files changed, 28 insertions(+), 35 deletions(-)
>>
>> diff --git a/gdb/aix-thread.c b/gdb/aix-thread.c
>> index f4b6c1b06ab6..a479d0150bc2 100644
>> --- a/gdb/aix-thread.c
>> +++ b/gdb/aix-thread.c
>> @@ -993,7 +993,7 @@ pd_disable (void)
>>    if (pd_active)
>>      pd_deactivate ();
>>    pd_able = 0;
>> -  unpush_target (&aix_thread_ops);
>> +  current_inferior ()->unpush_target (&aix_thread_ops);
>>  }
>>
>>  /* new_objfile observer callback.
>> diff --git a/gdb/bsd-kvm.c b/gdb/bsd-kvm.c
>> index cd25e19a544e..17db2fe1cd60 100644
>> --- a/gdb/bsd-kvm.c
>> +++ b/gdb/bsd-kvm.c
>> @@ -132,7 +132,7 @@ bsd_kvm_target_open (const char *arg, int from_tty)
>>      error (("%s"), errbuf);
>>
>>    bsd_kvm_corefile = filename;
>> -  unpush_target (&bsd_kvm_ops);
>> +  current_inferior ()->unpush_target (&bsd_kvm_ops);
>>    core_kd = temp_kd;
>>    push_target (&bsd_kvm_ops);
>>
>> diff --git a/gdb/bsd-uthread.c b/gdb/bsd-uthread.c
>> index d7dd0a180142..2ee47bfb5c47 100644
>> --- a/gdb/bsd-uthread.c
>> +++ b/gdb/bsd-uthread.c
>> @@ -259,7 +259,7 @@ bsd_uthread_deactivate (void)
>>    if (!bsd_uthread_active)
>>      return;
>>
>> -  unpush_target (&bsd_uthread_ops);
>> +  current_inferior ()->unpush_target (&bsd_uthread_ops);
>>  }
>>
>>  static void
>> diff --git a/gdb/corelow.c b/gdb/corelow.c
>> index a2d2d20afc62..a4c1f6354c6e 100644
>> --- a/gdb/corelow.c
>> +++ b/gdb/corelow.c
>> @@ -580,7 +580,7 @@ core_target::detach (inferior *inf, int from_tty)
>>    /* Note that 'this' is dangling after this call.  unpush_target
>>       closes the target, and our close implementation deletes
>>       'this'.  */
>> -  unpush_target (this);
>> +  inf->unpush_target (this);
>>
>>    /* Clear the register cache and the frame cache.  */
>>    registers_changed ();
>> diff --git a/gdb/exec.c b/gdb/exec.c
>> index 544a05873f11..bcc54bd966fe 100644
>> --- a/gdb/exec.c
>> +++ b/gdb/exec.c
>> @@ -671,7 +671,7 @@ program_space::remove_target_sections (void *owner)
>>  	    continue;
>>
>>  	  switch_to_inferior_no_thread (inf);
>> -	  unpush_target (&exec_ops);
>> +	  inf->unpush_target (&exec_ops);
>>  	}
>>      }
>>  }
> 
> I think the purpose of the 'switch_to_inferior_no_thread' above
> was to unpush_target from the current inferior.  It should be OK to
> remove the switch.

In fact, unpush_target decrefs the target, which can cause its to be
closed.  There are some target_ops::close methods which reference the
current_inferior (though I'm not sure that it's really appropriate for
them to do so).  I've left the switch_to_inferior calls to be safe for
this reason.  I'll add a note in the commit message to that effect.

But I agree this is the right direction.

Simon