[v2,0/7] improve btrace enable error reporting

Message ID 1516976072-19282-1-git-send-email-markus.t.metzger@intel.com
Headers show
Series
  • improve btrace enable error reporting
Related show

Message

Metzger, Markus T Jan. 26, 2018, 2:14 p.m.
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

 gdb/Makefile.in                       |   2 +
 gdb/btrace.c                          |   3 -
 gdb/common/scoped_fd.h                |  60 +++++
 gdb/common/scoped_mmap.h              |  76 ++++++
 gdb/gdbserver/linux-low.c             |   2 -
 gdb/gdbserver/nto-low.c               |   1 -
 gdb/gdbserver/server.c                |  80 +++----
 gdb/gdbserver/spu-low.c               |   1 -
 gdb/gdbserver/target.h                |   7 -
 gdb/gdbserver/win32-low.c             |   1 -
 gdb/nat/linux-btrace.c                | 427 ++++++++--------------------------
 gdb/nat/linux-btrace.h                |   3 -
 gdb/remote.c                          |  32 ---
 gdb/target-delegates.c                |  33 ---
 gdb/target.c                          |   8 -
 gdb/target.h                          |   7 -
 gdb/unittests/scoped_fd-selftests.c   |  90 +++++++
 gdb/unittests/scoped_mmap-selftests.c |  92 ++++++++
 gdb/x86-linux-nat.c                   |  20 +-
 19 files changed, 458 insertions(+), 487 deletions(-)
 create mode 100644 gdb/common/scoped_fd.h
 create mode 100644 gdb/common/scoped_mmap.h
 create mode 100644 gdb/unittests/scoped_fd-selftests.c
 create mode 100644 gdb/unittests/scoped_mmap-selftests.c

-- 
1.8.3.1

Comments

Pedro Alves Feb. 6, 2018, 4:28 p.m. | #1
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
Metzger, Markus T Feb. 7, 2018, 10:41 a.m. | #2
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
Pedro Alves Feb. 7, 2018, 12:11 p.m. | #3
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
Metzger, Markus T Feb. 8, 2018, 10:40 a.m. | #4
Hello Pedro,

Thanks for your comments.

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

> git log.



commit 41df50232209c74f3465556c9cfa690e16ee63ca
Author: Markus Metzger <markus.t.metzger@intel.com>
Date:   Fri Jan 19 09:41:42 2018 +0100

    btrace, gdbserver: use exceptions to convey btrace enable/disable errors
    
    Change error reporting to use exceptions and be prepared to catch them in
    gdbserver.  We use the exception message in our error reply to GDB.
    
    This may remove some detail from the error message in the native case since
    errno is no longer printed.  Later patches will improve that.
    
    We're still using error strings on the RSP level.  This patch does not affect
    the interoperability of older/newer GDB/gdbserver.


commit 54ab396195eee25b4045a2fc5226e73c508dc5a6
Author: Markus Metzger <markus.t.metzger@intel.com>
Date:   Fri Jan 19 14:32:09 2018 +0100

    btrace, gdbserver: remove the to_supports_btrace target method
    
    Remove the to_supports_btrace target method and instead rely on detecting errors
    when trying to enable recording.  This will also provide a suitable error
    message explaining why recording is not possible.
    
    For remote debugging, gdbserver will now always advertise branch tracing related
    packets.  When talking to an older GDB, this will cause GDB to try to enable
    branch tracing and gdbserver to report a suitable error message every time.
    
    An older gdbserver will not advertise branch tracing related packets if the
    one-time check failed, so a newer GDB with this patch will fail to enable branch
    tracing at remote_enable_btrace() rather than at btrace_enable().  The error
    message is the same in both cases so there should be no user-visible change.

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


X is always 'enable branch tracing' in our case.  But the reason for the error may
be different:
- libipt missing at build time
- kernel headers missing or too old at build time

Those reasons are not relevant for the user, though.  There are already config
warnings for those so if GDB is configured with --with-intel-pt=yes, the build
should fail with an appropriate diagnostic.  I'd omit it in the error message.

So you're suggesting to change

    "GDB does not support Intel Processor Trace."

into

    "Intel Processor Trace support was disabled at compile time."

Correct?

We're already using the above in a warning in remote.c.  I'd change that, as well,
to be consistent with the new wording.

OK?


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


The error message might be misleading on other systems but it is not very likely
that perf_event_paranoid is set to another value than those supported by that
system.

If upstream introduces a level 3, we may need to remove this diagnostic again.

So I'll leave the patch in and correct the declaration of FILENAME.  I found one
more occurrence in this series.

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
Pedro Alves Feb. 8, 2018, 1:01 p.m. | #5
On 02/08/2018 10:40 AM, Metzger, Markus T wrote:

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

>> git log.

> 

> [snip new logs]


Thanks.

> So you're suggesting to change

> 

>     "GDB does not support Intel Processor Trace."

> 

> into

> 

>     "Intel Processor Trace support was disabled at compile time."

> 

> Correct?


Correct.

> 

> We're already using the above in a warning in remote.c.  I'd change that, as well,

> to be consistent with the new wording.

> 

> OK?


OK.

Thanks,
Pedro Alves
Metzger, Markus T Feb. 8, 2018, 1:49 p.m. | #6
Hello Pedro, Eli,

Here's the follow-up patch to change a few existing error messages.

> > So you're suggesting to change

> >

> >     "GDB does not support Intel Processor Trace."

> >

> > into

> >

> >     "Intel Processor Trace support was disabled at compile time."

> >

> > Correct?

> 

> Correct.

> 

> >

> > We're already using the above in a warning in remote.c.  I'd change

> > that, as well, to be consistent with the new wording.

> >


commit d635c0d842f95f9f76562a132dcabd9330ff6455
Author: Markus Metzger <markus.t.metzger@intel.com>
Date:   Thu Feb 8 14:35:44 2018 +0100

    btrace: reword error messages
    
    Reword some btrace error messages to align with the format discussed in
    https://sourceware.org/ml/gdb-patches/2018-02/msg00135.html.
    
    2018-02-08  Markus Metzger  <markus.t.metzger@intel.com>
    
    gdb/
        * remote.c (remote_btrace_maybe_reopen): Change error message.
        * btrace.c (btrace_enable): Likewise.
        (parse_xml_btrace): Likewise.
        (parse_xml_btrace_conf): Likewise.
    
diff --git a/gdb/btrace.c b/gdb/btrace.c
index 2b031a4..158d03c 100644
--- a/gdb/btrace.c
+++ b/gdb/btrace.c
@@ -1579,7 +1579,7 @@ btrace_enable (struct thread_info *tp, const struct btrace_config *conf)
 
 #if !defined (HAVE_LIBIPT)
   if (conf->format == BTRACE_FORMAT_PT)
-    error (_("GDB does not support Intel Processor Trace."));
+    error (_("Intel Processor Trace support was disabled at compile time."));
 #endif /* !defined (HAVE_LIBIPT) */
 
   DEBUG ("enable thread %s (%s)", print_thread_id (tp),
@@ -2218,7 +2218,8 @@ parse_xml_btrace (struct btrace_data *btrace, const char *buffer)
 
 #else  /* !defined (HAVE_LIBEXPAT) */
 
-  error (_("Cannot process branch trace.  XML parsing is not supported."));
+  error (_("Cannot process branch trace.  XML support was disabled at "
+          "compile time."));
 
 #endif  /* !defined (HAVE_LIBEXPAT) */
 }
@@ -2312,7 +2313,8 @@ parse_xml_btrace_conf (struct btrace_config *conf, const char *xml)
 
 #else  /* !defined (HAVE_LIBEXPAT) */
 
-  error (_("XML parsing is not supported."));
+  error (_("Cannot process the branch trace configuration.  XML support "
+          "was disabled at compile time."));
 
 #endif  /* !defined (HAVE_LIBEXPAT) */
 }
diff --git a/gdb/remote.c b/gdb/remote.c
index e5680f0..15d6c5b 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -13193,8 +13193,8 @@ remote_btrace_maybe_reopen (void)
          if (!warned)
            {
              warned = 1;
-             warning (_("GDB does not support Intel Processor Trace. "
-                        "\"record\" will not work in this session."));
+             warning (_("Target is recording using Intel Processor Trace "
+                        "but support was disabled at compile time."));
            }
 
          continue;

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
Pedro Alves Feb. 8, 2018, 2:23 p.m. | #7
On 02/08/2018 01:49 PM, Metzger, Markus T wrote:
> Hello Pedro, Eli,

> 

> Here's the follow-up patch to change a few existing error messages.

> 

>>> So you're suggesting to change

>>>

>>>     "GDB does not support Intel Processor Trace."

>>>

>>> into

>>>

>>>     "Intel Processor Trace support was disabled at compile time."

>>>

>>> Correct?

>>

>> Correct.

>>

>>>

>>> We're already using the above in a warning in remote.c.  I'd change

>>> that, as well, to be consistent with the new wording.

>>>

> 


Very nice.  Thanks!

Pedro Alves