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

[Pedro Alves <palves at redhat dot com>]

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