Re: [PATCH] gdb: fix target_ops reference count for some cases

[Andrew Burgess <aburgess@redhat.com>]

Simon Marchi <simark@simark.ca> writes:

>>> Just wondering, why do we need to restore explicitly the current
>>> pspace, instead of using just scoped_restore_current_thread?
>>>
>>> scoped_restore_current_pspace_and_thread's doc says:
>>>
>>> /* Save/restore the current program space, thread, inferior and frame.
>>>    Use this when you need to call
>>>    switch_to_program_space_and_thread.  */
>>>
>>> ... but you are not using switch_to_program_space_and_thread here.
>>> Maybe it's ok and I just don't understand.  Same in
>>> ~scoped_mock_context.
>> 
>> I suspect the comment you quote is just out of date.
>> 
>> switch_to_program_space_and_thread can end up calling
>> switch_to_inferior_no_thread if there are no running threads in the
>> program space being switched too.  But, even if switch_to_thread does
>> end up being called we:
>> 
>>   - set the program space,
>>   - set the inferior,
>>   - set the current thread,
>>   - reinit the frame cache,
>> 
>> By comparison, switch_to_inferior_no_thread does:
>> 
>>   - sets the program space,
>>   - sets the inferior,
>>   - sets the current thread (to nullptr this time though),
>>   - reinits the frame cache,
>> 
>> As you can see they do the same set of things, all of which I think
>> should be reverted once we leave the scope, hence
>> scoped_restore_current_pspace_and_thread seems like the way to go.
>
> Hmm okay, but won't scoped_restore_current_thread always restore the
> pspace one way or another?  scoped_restore_current_thread::restore will
> either call switch_to_thread or switch_to_inferior_no_thread, which both
> end up setting the pspace.

That sounds completely correct.  So I did an experiment.  I added
~scoped_restore_current_pspace_and_thread, and with a little hacking,
had it check: is the program_space in m_restore_pspace the same
program_space as would be restored by m_restore_thread?

Turns out that it _isn't_ always the case!

Just to be clear on what this means: sometimes GDB is in a state where
the currently selected program_space is NOT the program_space for the
currently selected inferior!

One example of when this occurs is create_longjmp_master_breakpoint in
breakpoint.c, in here we do this:

  scoped_restore_current_program_space restore_pspace;

  for (struct program_space *pspace : program_spaces)
    {
      set_current_program_space (pspace);

      ...
    }

Obviously we enter this code with some inferior selected, but, within
the body of this loop we select each program_space in turn.  All but one
of these will be the wrong program_space for the current inferior.

This feels (to me) like a problem waiting to happen.  I wonder if we
should have some better approach here.  Some ideas would be:

  1. Change to always require switch_to_program_space_and_thread and
  remove set_current_program_space.  This would then require that we use
  scoped_restore_current_thread and remove
  scoped_restore_current_program_space.  This would mean GDB always had
  a sane thread/inferior/program_space combo selected,

  2. Allow GDB to have no inferior selected, then, in the above code,
  where (I guess) we are claiming that everything in the for loop only
  cares about the program_space, and not the inferior, we could
  temporarily switch to no inferior.  This would suggest adding a
  set_current_program_space_no_inferior function, and again, we would
  use scoped_restore_current_thread and remove
  scoped_restore_current_program_space from GDB,

  3. Embrace the mismatch and have scoped_restore_current_thread capture
  the current_program_space as well as the current thread and inferior.
  It would then restore the program space after restoring the
  thread/inferior, thus ensuring that we always restore the
  program_space correctly.  This would allow for
  scoped_restore_current_pspace_and_thread to be removed and just use
  scoped_restore_current_thread in all cases.

I rather like #2, but I worry that too much of GDB would just assume
there's always a current inferior, so I'm tempted to think #1 is
probably the best approach.  My concern is mostly the performance impact
of searching for a suitable inferior for a given program_space (this
currently involves a linear search through all inferiors).  With a low
number of inferiors this shouldn't be an issue, but maybe if we started
to deal with large numbers of inferiors we'd need to be smarter?

Ideally I think we shouldn't just live with the mismatch, though as a
short term stop-gap, maybe we could implement #3?

Thoughts?

Thanks,
Andrew


>                             I just don't understand why
> scoped_restore_current_pspace_and_thread exists, it seems like
> scoped_restore_current_thread would always work where
> scoped_restore_current_pspace_and_thread is used.  I must be missing
> something, scoped_restore_current_pspace_and_thread must have been added
> for a reason.
>
> Simon