Re: Crash of GDB with gdbserver btrace enabled [Re: [patch v9 00/23] branch tracing support for Atom]

[Pedro Alves <palves@redhat.com>]

On 03/07/2013 12:32 PM, Metzger, Markus T wrote:
>> -----Original Message-----
>> From: Jan Kratochvil [mailto:jan.kratochvil@redhat.com]
>> Sent: Thursday, March 07, 2013 1:07 PM
> 
> 
>>>> OK, I agree target_close seems excessive here, it also does not correspond to
>>>> the current description of target_close:
>>>> 	This routine is automatically always called after popping the target
>>>> 	off the target stack
>>>>
>>>> While it is nice cleanup I find it a separate patch, not required for btrace.
>>>
>>> With this patch, the crash is gone with only minimal changes to btrace.
>>
>> It is only a coincidence and workaround of it.
> 
> Hmmm, if we must no longer call methods in a certain target, why is
> removing that target a workaround?
> 
> Pedro already moved the target_close call after removing the target in
> http://sourceware.org/ml/gdb-patches/2012-01/msg00701.html.
> 
> He just did not consider the extra target_close call in pop_target, which
> is not only closing the target twice but also breaking his attempt to
> reorder target removing and closing.

Indeed, that seems so.  The close-twice isn't an issue for current
targets, as their close methods are idempotent:

static void
remote_close (int quitting)
{
  if (remote_desc == NULL)
    return; /* already closed */

But the reorder issue here does look like should be fixed.


I think the double closes are actually a workaround for the
"quitting" argument not being propagated to unpush_target (or
maybe unpush_target didn't close the target itself at some point):

void
pop_all_targets_above (enum strata above_stratum, int quitting)
{
  while ((int) (current_target.to_stratum) > (int) above_stratum)
    {
      target_close (target_stack, quitting);
      if (!unpush_target (target_stack))

...

void
pop_all_targets (int quitting)
{
  pop_all_targets_above (dummy_stratum, quitting);
}

...

/* Called by the event loop to process a SIGHUP.  */
static void
async_disconnect (gdb_client_data arg)
{
...

  TRY_CATCH (exception, RETURN_MASK_ALL)
    {
      pop_all_targets (1);
    }
...
}

QUITTING is documented here:

/* Does whatever cleanup is required for a target that we are no
   longer going to be calling.  QUITTING indicates that GDB is exiting
   and should not get hung on an error (otherwise it is important to
   perform clean termination, even if it takes a while).  This routine
   is automatically always called after popping the target off the
   target stack - the target's own methods are no longer available
   through the target vector.  Closing file descriptors and freeing all
   memory allocated memory are typical things it should do.  */

void target_close (struct target_ops *targ, int quitting);

I'm not sure, but ISTR that no target nowadays actually does
anything with the 'quitting' flag.

-- 
Pedro Alves