RE: [PATCH] btrace, gdbserver: check btrace target pointers

["Metzger, Markus T" <markus dot t dot metzger at intel dot com>]

Hello Yao,

Thanks for your review.


> > -#define target_enable_btrace(ptid, conf) \
> > -  (*the_target->enable_btrace) (ptid, conf)
> > +static inline struct btrace_target_info *
> > +target_enable_btrace (ptid_t ptid, const struct btrace_config *conf)
> > +{
> > +  if (the_target->enable_btrace == nullptr)
> > +    error ("Target does not support branch tracing.");
> > +
> > +  return (*the_target->enable_btrace) (ptid, conf);
> > +}
> 
> It is reasonable to me that (*the_target->enable_btrace) may throw
> various exceptions due to different reasons, but I am not convinced that
> we should error on (the_target->enable_btrace == nullptr).  I don't like
> replacing control flow logic with exception.  This is my personal
> flavor.  Instead, can we check
> (the_target->enable_btrace == nullptr) before using
> target_enable_btrace, and error in handle_btrace_general_set if
> thread->btrace is NULL.  What do you think?

I moved two of those checks into the target_* method since I thought
it would be a good idea to be able to handle exceptions and since I had
to add the TRY/CATCH/CATCH_END, anyway, I thought it made sense
to handle the nullptr case also via exceptions.

If we're not doing it that way, the caller would need to check the pointer
(either directly or via a *_p () function like we do with gdbarch) and still
be prepared to handle exceptions from the actual call.

My motivation was to simplify the caller but if you think the other way
is clearer I can move the check back into the callers.  Is that your preference?

Regards,
Markus.

Intel Deutschland GmbH
Registered Address: Am Campeon 10-12, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de
Managing Directors: Christin Eisenschmid, Christian Lamprechter
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928