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

["Metzger, Markus T" <markus.t.metzger@intel.com>]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 2997 bytes --]

Hello Pedro,

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

An old gdbserver would still produce the old error responses.  They would be turned
into errors on the GDB side in remote.c both for old and new GDB.  No change.

Removing remote_supports_btrace in a new GDB will defer the packet availability check
to the individual functions.  The "Target does not support branch tracing" error will now
come from record_enable_btrace() instead of btrace_enble().  The old gdbserver still does
the initial availability check and will not announce the respective enabling packets.  No
user-visible change.

A new gdbserver will produce the new error messages and will always announce btrace
packets in qSupported.  An old GDB will hence always ask to enable branch tracing and
the new gdbserver will always try and report the reason using the new error messages.
This will look like a new GDB.

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

I thought we meant GDB in errors to always refer to this instance of GDB.  There would
hardly be an error about missing PT support if the GDB project didn't know about it.

I don't mind changing the error string.  Would "This GDB" be the correct wording?  Or
should we refer to the configuration and say something like "GDB has not been configured
to support ..." or "GDB has been built without support for ..."?

Both are substantially longer and not more helpful IMHO, even though they describe
more accurately what's wrong.  The term "This GDB" would refer to this particular GDB
executable more clearly but I'm not sure whether this would be more helpful, either.

 
> #3 - in patch 7:
> 
> Instead of:
> 
>   const char *filename = "/proc/sys/kernel/perf_event_paranoid";
> 
> write:
> 
>   static const char filename[] = ...;

Regarding that last patch, it is checking a Debian-specific kernel feature.  The upstream
kernel only knows levels -1, 0, 1, and 2.  Even setting the perf_event_paranoid level to 3
wouldn't cause any issues.

What is our policy regarding this?  Do we accept such distribution-specific checks into
upstream GDB or do we expect the Debian maintainers to maintain this patch on top
of upstream GDB?


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
\x16º&Öéj×!zÊÞ¶êç׎w×yb²Ö«r\x18\x1dn–­r\x17¬