Re: [PATCH v2 0/7] improve btrace enable error reporting

[Pedro Alves <palves at redhat dot com>]

Hi Markus,

On 01/26/2018 02:14 PM, Markus Metzger wrote:
> Recording may fail for a variety of reasons.  Improve the error
> messages by stating more clearly what operation failed and try to give
> a reason why it failed.
> 
> Further align the error messages for native and remote debugging.
> 
> Changes to v1:
>   - move helper classes into gdb/common/
>   - add unit tests for helpers
>   - simplify helpers
> 
> Markus Metzger (7):
>   common: add scoped_fd
>   common: add scoped_mmap
>   btrace: prepare for throwing exceptions when enabling btrace
>   btrace, gdbserver: use exceptions to convey btrace enable/disable
>     errors
>   btrace, gdbserver: remove the to_supports_btrace target method
>   btrace: improve enable error messages
>   btrace: check perf_event_paranoid

This LGTM, though I have a couple questions, and a nit.

#1 - Where does this leave up wrt to:

  'old gdb' x 'new gdbserver'
and
  'new gdb' x 'old gdbserver'
 
?


#2 - Where we now say

  +  error (_("GDB does not support Intel PT."));

(and similarly for BTS)

shouldn't that say something like "_This_ GDB does not",
so that the user can tell that it's a matter of that
particular build of gdb, not that GDB-the-project is lacking
support for PT?


#3 - in patch 7:

Instead of:

  const char *filename = "/proc/sys/kernel/perf_event_paranoid";

write:

  static const char filename[] = ...;

Thanks,
Pedro Alves