Re: [PATCH 2/3] gdb: refactor core_target ::close and ::detach functions

[Andrew Burgess <aburgess@redhat.com>]

Andrew Burgess <aburgess@redhat.com> writes:

> Lancelot SIX <lancelot.six@amd.com> writes:
>
>> On Mon, Mar 30, 2026 at 04:30:52PM +0100, Andrew Burgess wrote:
>>>  void
>>>  core_target::close ()
>>>  {
>>> -  clear_core ();
>>> +  /* The core BFD is set when the core_target is created and attached to
>>> +     the inferior.  It is never explicitly cleared, instead m_core_bfd will
>>> +     have its reference count reduced when the core_target is deleted.  */
>>> +  gdb_assert (this->core_bfd () != nullptr);
>>> +
>>> +  /* If we called ::detach before calling ::close then the inferior will
>>> +     have already been exited.  This will happen if the user clears the
>>> +     core file with the 'core-file' or 'detach' commands.
>>> +
>>> +     However, if the user just causes the core_target to be unpushed, by
>>> +     pushing an alternative target, e.g. 'target remote ....', then we will
>>> +     not call ::detach before calling ::close.
>>> +
>>> +     In the former case we don't want to exit the inferior twice; this is
>>> +     mostly harmless except it causes two 'exited' events to be emitted in
>>> +     the Python API, which isn't ideal.
>>> +
>>> +     As opening a core_target always ensures that some thread is selected,
>>> +     then we can tell if exit_core_file_inferior has already been called by
>>> +     checking if no thread is now selected.  */
>>> +  if (inferior_ptid != null_ptid)
>>> +    exit_core_file_inferior ();
>>
>> Hi Andrew,
>>
>> We are observing a behaviour change after your change in the downstream
>> ROCgdb port.  Long story short is: when doing "exit" with a core file
>> opened, the "inferior_exit" observer is not called at all.
>>
>> When calling "exit", we execute:
>>
>>   quit_command
>>     quit_force
>>       inferior::pop_all_targets
>>         pop_all_targets_above (dummy_stratum)
>>
>> From there, pop_all_targets does:
>>
>>     switch_to_inferior_no_thread (this);
>>
>>     while (top_target ()->stratum () > stratum)
>>       unpush_target_and_assert (top_target ());
>>
>> The switch_to_inferior_no_thread sets inferior_ptid to null_ptid, then
>> we unpush the core_target, decrement its refcount and end up here in
>> core_target::close.  Because inferior_ptid is null_ptid, we skip calling
>> exit_core_file_inferior.
>>
>> One would expect that we detach before reaching this point, and
>> quit_force tries to do so:
>>
>>      for (inferior *inf : all_inferiors ())
>>        kill_or_detach (inf, from_tty);
>>
>> However, kill_or_detach explicitly does not call target_detach for core
>> files:
>>
>>     /* Leave core files alone.  */
>>     if (target_has_execution ())
>>       {
>>         if (inf->attach_flag)
>> 	  target_detach (inf, from_tty);
>> 	else
>> 	  target_kill ();
>>       }
>>
>> Because exit_core_file_inferior is not called, we never call
>> "exit_inferior (current_inferior ())", and therefore fail to notify the
>> "inferior_exit" observer.
>>
>> In our case, we notice this because we use the inferior_exit observer to
>> detach the GPU side of the process.  Because we fail to do this detach,
>> we leave some unclean state in our GPU debugging library, which
>> eventually causes complaints (i.e. segfault)  when calling global
>> destructors.
>>
>> Given this scenario, I expect the last part of the comment regarding the
>> guarantee of having a thread selected is invalid.  I have not looked too
>> deeply into a solution yet, but I can.  Given that the core_target is
>> not shareable, could each instance have a "detached" flag which could be
>> used in placed of checking inferior_ptid against null_ptid?
>
> Lancelot,
>
> Sorry for the breakage, and thanks for the great analysis.  I'll take a
> look at getting this fixed asap.  Hopefully will get something posted
> next week.

Fix posted here:

  https://inbox.sourceware.org/gdb-patches/fce47c4e13bd626b3b3bc074fd51ab1ed23ad0e3.1780591573.git.aburgess@redhat.com

Thanks,
Andrew