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

[Pedro Alves <palves at redhat dot com>]

On 02/07/2018 10:41 AM, Metzger, Markus T wrote:
> 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.

Great, thanks.  I'd think it a good idea to copy/paste that to the relevant
patch's git log.

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

GDB can know about PT but not have support implemented, because no one wrote
the support.  The error message "GDB does not support Intel PT" to me sounds
like what we'd say if we had written some initial support in form of
commands that do nothing.   How can the user tell the difference to
"gdb supports PT, but you need to build it against a newer version of some library,
or something like that."

> 
> 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.

Yeah "This GDB" was a straw man proposal, something to get the discussion started
to maybe find some better warning.

Off hand, I recall that in the no-expat cases, we say things like this:

      warning (_("Can not parse XML trace frame info; XML support "
                 "was disabled at compile time"));

      warning (_("Can not parse XML memory map; XML support was disabled "
                 "at compile time"));

      warning (_("Can not parse XML OS data; XML support was disabled "
                "at compile time"));

So maybe something around

  "Cannot do X; Intel PT support disabled at compile time."


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

I don't think we have a written down policy.  

I don't mind having it in master.  It helps users/developers running
Debian and derivatives, and is not invasive.

Thanks,
Pedro Alves