btrace, gdbserver: check btrace target pointers

  By removing the supports_btrace gdbserver target method we relied on GDB trying
to enable branch tracing and failing on the attempt.

For targets that do not provide the btrace methods, however, an initial request
from GDB for the branch trace configuration to detect whether gdbserver is
already recording resulted in a protocol error.

Have the btrace target methods throw a "Target does not suppor branch tracing"
error and be prepared to handle exceptions in all functions that call btrace
target methods.  We therefore turn the target_* macros into static inline
functions.

Also remove the additional btrace target method checks that resulted in the
above protocol error.

Thanks to Maciej W. Rozycki <macro@mips.com> for reporting this.

2018-02-27  Markus Metzger  <markus.t.metzger@intel.com>

gdbserver/
	* target.h (target_enable_btrace, target_disable_btrace,
	target_read_btrace, target_read_btrace_conf): Turn macro into inline
	function.  Throw error if target method not defined.
	* server.c (handle_qxfer_btrace, handle_qxfer_btrace_conf): Remove
	check for btrace target method.  Be prepared to handle exceptions
	from btrace target methods.
---
 gdb/gdbserver/server.c | 38 ++++++++++++++++++++++++++++----------
 gdb/gdbserver/target.h | 42 ++++++++++++++++++++++++++++++++++--------
 2 files changed, 62 insertions(+), 18 deletions(-)
  

Markus Metzger <markus.t.metzger@intel.com> writes:

> -#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?
  

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
  

On Wed, Feb 28, 2018 at 5:10 PM, Metzger, Markus T
<markus.t.metzger@intel.com> wrote:
>
> 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?
>

I don't have a strong opinion on this.  It is more about personal flavour of
writing code.  You are the major btrace contributor, so it is better to keep
the code in a way you prefer.  Patch is good to me, please push.
  

On 03/01/2018 09:14 AM, Yao Qi wrote:
> On Wed, Feb 28, 2018 at 5:10 PM, Metzger, Markus T
> <markus.t.metzger@intel.com> wrote:
>>
>> 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?
>>
> 
> I don't have a strong opinion on this.  It is more about personal flavour of
> writing code.  You are the major btrace contributor, so it is better to keep
> the code in a way you prefer.  Patch is good to me, please push.

To me too.

A comment based on my experience hacking on the multi-target branch:

I like imagining how the code would look like if it were C++-ified already.

Here, if gdbserver's target_ops function pointers are converted to C++ methods,
then it'll no longer be possible to check for null pointer.  [1]

So, whatever we do, I think checking for null pointers in function
pointer tables is the option that just kicks the can down the road.

The only options here then will be error out or check a *_p() function upfront.
The latter is another way to say, go back to having a supports_btrace method,
I guess.

[*] - I've run into some cases like these in my gdb target_ops C++-ification
in my multi-target branch.  Like e.g., I had to add
target_ops::can_create_inferior to replace target_ops->to_create_inferior
null pointer checks.

Thanks,
Pedro Alves