Re: [PATCH 1/2] Re-check fastpoint after reloading symbols.

["Ulrich Weigand" <uweigand@de.ibm.com>]

Wei-cheng Wang wrote:

> Check fast tracepoints after symbols have been re-loaded.
> For example, a pending tracepoint just becomes available after
> a new shared object being loaded.  We didn't check it before,
> because we have no idea where it is.
> 
> If the target rejects the tracepoint, an error is throw in
> check_fast_tracepoint_sals, and we will disable the breakpoint.
> 
> The checking is deliberately put after the loop for adding location
> to breakpoint, so users can check the address for the tracepoint
> with `info trace'.  Otherwise, it will simply show <PENDING> instead
> of the address.
> 
> (gdb) info trace
> Num     Type            Disp Enb Address            What
> 1       fast tracepoint keep n   0x00003fffb7f507dc <pendfunc+20>
>                                  ^^^^^^^^^^^^^^^^^^

This makes sense to me.

However, we now have allowed a disabled breakpoint with an invalid
address in the list.  If the user simply enables the breakpoint now
and continues, we will run into the same internal error as before ...

So we probably ought to add the same check to enable_breakpoint.

> -  /* If there's no new locations, and all existing locations are
> -     pending, don't do anything.  This optimizes the common case where
> -     all locations are in the same shared library, that was unloaded.
> -     We'd like to retain the location, so that when the library is
> -     loaded again, we don't loose the enabled/disabled status of the
> -     individual locations.  */
> -  if (all_locations_are_pending (existing_locations) && sals.nelts == 0)
> -    return;

I don't understand why you're moving this piece of code down.  This just
seems to disable the optimization mentioned in the comment ...

> +  if (b->type == bp_fast_tracepoint)
> +    {
> +      TRY
> +	{
> +	  check_fast_tracepoint_sals (get_current_arch(), &sals);
This should use b->gdbarch instead of get_current_arch().

> +	}
> +      CATCH (e, RETURN_MASK_ERROR)
> +	{
> +	  b->enable_state = bp_disabled;
> +	  throw_exception (e);
> +	}
> +      END_CATCH
> +    }
> +
>    /* If possible, carry over 'disable' status from existing
>       breakpoints.  */

Hmm.  I think the check still needs to be moved further down.

Note that if there are other breakpoints, the have been re-set by the
loop above.  For those other breakpoints, we still should do the
actions done in the rest of this routine:
- carry over disable status
- notify observers
- update global location list

But if you run into an error during check_fast_tracepoint_sals,
you throw an exception, which skips all this code.  That's not
a good idea ...

I think we should move the check to the very end of the routine.
(In fact, we then might move it up into the caller, i.e. to the
end of tracepoint_re_set.)

Also, some test that all this works as expected would be good.

Bye,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU/Linux compilers and toolchain
  Ulrich.Weigand@de.ibm.com