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

[Andrew Burgess <aburgess@redhat.com>]

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.

Thanks,
Andrew




>
> Best,
> Lancelot.
>
>>  
>>    /* Core targets are heap-allocated (see core_target_open), so here
>>       we delete ourselves.  */
>>    delete this;
>> +
>> +  /* Notify that the core file has changed.  This is intentionally done
>> +     after the core_target is deleted as nothing in here depends on the
>> +     core_target itself, the core_target has already been removed from the
>> +     inferior's target stack by this point.  */
>> +  gdb::observers::core_file_changed.notify (current_inferior ());
>>  }