rpms/gdb
7
0
Fork 0
Code Issues Pull Requests Releases Activity

Fix 'Make the probes-based dynamic linker interface more robust to errors' (Sergio Durigan Junior, RH BZ 1259132).

Browse Source
This commit is contained in:
Sergio Durigan Junior 2015-09-02 16:02:54 -04:00
parent 2f1c4cf200
commit 1c8228902d
3 changed files with 399 additions and 1 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

193
gdb-probes-based-interface-robust-1of2.patch Normal file
View File

@ -0,0 +1,193 @@
From f469e8ce11672e26feb5ba6f9a134275fcfd5b4f Mon Sep 17 00:00:00 2001
From: Sergio Durigan Junior <sergiodj@redhat.com>
Date: Fri, 21 Aug 2015 18:13:46 -0400
Subject: [PATCH 1/4] Improve error reporting when handling SystemTap SDT
probes
This patch improves the error reporting when handling SystemTap SDT
probes. "Handling", in this case, mostly means "parsing".
On gdb/probe.h, only trivial changes on functions' comments in order
to explicitly mention that some of them can throw exceptions. This is
just to make the API a bit more clear.
On gdb/stap-probe.c, I have s/internal_error/error/ on two functions
that are responsible for parsing specific bits of the probes'
arguments: stap_get_opcode and stap_get_expected_argument_type. It is
not correct to call internal_error on such situations because it is
not really GDB's fault if the probes have malformed arguments. I also
improved the error reported on stap_get_expected_argument_type by also
including the probe name on it.
Aside from that, and perhaps most importantly, I added a check on
stap_get_arg to make sure that we don't try to extract an argument
from a probe that has no arguments. This check issues an
internal_error, because it really means that GDB is doing something it
shouldn't.
Although it can be considered almost trivial, and despite the fact
that I am the maintainer for this part of the code, I am posting this
patch for review. I will wait a few days, and if nobody has anything
to say, I will go ahead and push it.
gdb/ChangeLog:
2015-09-01 Sergio Durigan Junior <sergiodj@redhat.com>
* probe.h (struct probe_ops) <get_probe_argument_count,
evaluate_probe_argument, enable_probe, disable_probe>: Mention in
the comment that the function can throw an exception.
(get_probe_argument_count): Likewise.
(evaluate_probe_argument): Likewise.
* stap-probe.c (stap_get_opcode): Call error instead of
internal_error.
(stap_get_expected_argument_type): Likewise. Add argument
'probe'. Improve error message by mentioning the probe's name.
(stap_parse_probe_arguments): Adjust call to
stap_get_expected_argument_type.
(stap_get_arg): Add comment. Assert that 'probe->args_parsed' is
not zero. Call internal_error if GDB requests an argument but the
probe has no arguments.
---
gdb/ChangeLog | 17 +++++++++++++++++
gdb/probe.h | 20 ++++++++++++++------
gdb/stap-probe.c | 29 ++++++++++++++++++++++-------
3 files changed, 53 insertions(+), 13 deletions(-)
Index: gdb-7.10/gdb/probe.h
===================================================================
--- gdb-7.10.orig/gdb/probe.h
+++ gdb-7.10/gdb/probe.h
@@ -70,7 +70,8 @@ struct probe_ops
CORE_ADDR (*get_probe_address) (struct probe *probe,
struct objfile *objfile);
- /* Return the number of arguments of PROBE. */
+ /* Return the number of arguments of PROBE. This function can
+ throw an exception. */
unsigned (*get_probe_argument_count) (struct probe *probe,
struct frame_info *frame);
@@ -82,7 +83,8 @@ struct probe_ops
int (*can_evaluate_probe_arguments) (struct probe *probe);
/* Evaluate the Nth argument from the PROBE, returning a value
- corresponding to it. The argument number is represented N. */
+ corresponding to it. The argument number is represented N.
+ This function can throw an exception. */
struct value *(*evaluate_probe_argument) (struct probe *probe,
unsigned n,
@@ -141,13 +143,15 @@ struct probe_ops
/* Enable a probe. The semantics of "enabling" a probe depend on
the specific backend and the field can be NULL in case enabling
- probes is not supported. */
+ probes is not supported. This function can throw an
+ exception. */
void (*enable_probe) (struct probe *probe);
/* Disable a probe. The semantics of "disabling" a probe depend
on the specific backend and the field can be NULL in case
- disabling probes is not supported. */
+ disabling probes is not supported. This function can throw an
+ exception. */
void (*disable_probe) (struct probe *probe);
};
@@ -264,7 +268,9 @@ extern struct cmd_list_element **info_pr
extern CORE_ADDR get_probe_address (struct probe *probe,
struct objfile *objfile);
-/* Return the argument count of the specified probe. */
+/* Return the argument count of the specified probe.
+
+ This function can throw an exception. */
extern unsigned get_probe_argument_count (struct probe *probe,
struct frame_info *frame);
@@ -276,7 +282,9 @@ extern unsigned get_probe_argument_count
extern int can_evaluate_probe_arguments (struct probe *probe);
/* Evaluate argument N of the specified probe. N must be between 0
- inclusive and get_probe_argument_count exclusive. */
+ inclusive and get_probe_argument_count exclusive.
+
+ This function can throw an exception. */
extern struct value *evaluate_probe_argument (struct probe *probe,
unsigned n,
Index: gdb-7.10/gdb/stap-probe.c
===================================================================
--- gdb-7.10.orig/gdb/stap-probe.c
+++ gdb-7.10/gdb/stap-probe.c
@@ -313,9 +313,8 @@ stap_get_opcode (const char **s)
break;
default:
- internal_error (__FILE__, __LINE__,
- _("Invalid opcode in expression `%s' for SystemTap"
- "probe"), *s);
+ error (_("Invalid opcode in expression `%s' for SystemTap"
+ "probe"), *s);
}
return op;
@@ -326,7 +325,8 @@ stap_get_opcode (const char **s)
static struct type *
stap_get_expected_argument_type (struct gdbarch *gdbarch,
- enum stap_arg_bitness b)
+ enum stap_arg_bitness b,
+ const struct stap_probe *probe)
{
switch (b)
{
@@ -361,8 +361,8 @@ stap_get_expected_argument_type (struct
return builtin_type (gdbarch)->builtin_uint64;
default:
- internal_error (__FILE__, __LINE__,
- _("Undefined bitness for probe."));
+ error (_("Undefined bitness for probe '%s'."),
+ probe->p.name);
break;
}
}
@@ -1172,7 +1172,8 @@ stap_parse_probe_arguments (struct stap_
else
arg.bitness = STAP_ARG_BITNESS_UNDEFINED;
- arg.atype = stap_get_expected_argument_type (gdbarch, arg.bitness);
+ arg.atype = stap_get_expected_argument_type (gdbarch, arg.bitness,
+ probe);
expr = stap_parse_argument (&cur, arg.atype, gdbarch);
@@ -1278,12 +1279,26 @@ stap_is_operator (const char *op)
return ret;
}
+/* Return argument N of probe PROBE.
+
+ If the probe's arguments have not been parsed yet, parse them. If
+ there are no arguments, throw an exception (error). Otherwise,
+ return the requested argument. */
+
static struct stap_probe_arg *
stap_get_arg (struct stap_probe *probe, unsigned n, struct gdbarch *gdbarch)
{
if (!probe->args_parsed)
stap_parse_probe_arguments (probe, gdbarch);
+ gdb_assert (probe->args_parsed);
+ if (probe->args_u.vec == NULL)
+ internal_error (__FILE__, __LINE__,
+ _("Probe '%s' apparently does not have arguments, but \n"
+ "GDB is requesting its argument number %u anyway. "
+ "This should not happen. Please report this bug."),
+ probe->p.name, n);
+
return VEC_index (stap_probe_arg_s, probe->args_u.vec, n);
}

194
gdb-probes-based-interface-robust-2of2.patch Normal file
View File

@ -0,0 +1,194 @@
From 3bd7e5b7ee5ea0b3bbb4030ca841f66faad74f0f Mon Sep 17 00:00:00 2001
From: Sergio Durigan Junior <sergiodj@redhat.com>
Date: Fri, 21 Aug 2015 18:28:07 -0400
Subject: [PATCH 2/4] Catching errors on probes-based dynamic linker interface
This patch is intended to make the interaction between the
probes-based dynamic linker interface and the SystemTap SDT probe code
on GDB more robust. It does that by wrapping the calls to the probe
API with TRY...CATCH'es, so that any exception thrown will be caught
and handled properly.
The idea for this patch came from
<https://bugzilla.redhat.com/show_bug.cgi?id=1196181>, which is a bug
initially filed against Fedora GDB (but now under Fedora GLIBC). This
bug happens on armhfp (although it could happen on other targets as
well), and is triggered because GCC generates a strange argument for
one of the probes used by GDB in the dynamic linker interface. As can
be seen in the bug, this argument is "-4@.L1052".
I don't want to discuss the reasons for this argument to be there
(this discussion belongs to the bug, or to another thread), but GDB
could definitely do a better error handling here. Currently, one sees
the following message when there is an error in the probes-based
dynamic linker interface:
(gdb) run
Starting program: /bin/inferior
warning: Probes-based dynamic linker interface failed.
Reverting to original interface.
Cannot parse expression `.L976 4@r4'.
(gdb)
Which means that one needs to explicitly issue a "continue" command to
make GDB continue running the inferior, even though this error is not
fatal and GDB will fallback to the old interface automatically.
This is where this patch helps: it makes GDB still print the necessary
warnings or error messages, but it *also* does not stop the inferior
unnecessarily.
I have tested this patch on the systems where this error happens, but
I could not come up with a way to create a testcase for it.
Nevertheless, it should be straightforward to see that this patch does
improve the current situation.
gdb/ChangeLog:
2015-09-01 Sergio Durigan Junior <sergiodj@redhat.com>
* solib-svr4.c (solib_event_probe_action): Call
get_probe_argument_count using TRY...CATCH.
(svr4_handle_solib_event): Likewise, for evaluate_probe_argument.
---
gdb/ChangeLog | 6 ++++++
gdb/solib-svr4.c | 43 ++++++++++++++++++++++++++++++++++++++++---
2 files changed, 46 insertions(+), 3 deletions(-)
Index: gdb-7.10/gdb/solib-svr4.c
===================================================================
--- gdb-7.10.orig/gdb/solib-svr4.c
+++ gdb-7.10/gdb/solib-svr4.c
@@ -1796,7 +1796,23 @@ solib_event_probe_action (struct probe_a
arg0: Lmid_t lmid (mandatory)
arg1: struct r_debug *debug_base (mandatory)
arg2: struct link_map *new (optional, for incremental updates) */
- probe_argc = get_probe_argument_count (pa->probe, frame);
+ TRY
+ {
+ probe_argc = get_probe_argument_count (pa->probe, frame);
+ }
+ CATCH (ex, RETURN_MASK_ERROR)
+ {
+ exception_print (gdb_stderr, ex);
+ probe_argc = 0;
+ }
+ END_CATCH
+
+ /* If get_probe_argument_count throws an exception, probe_argc will
+ be set to zero. However, if pa->probe does not have arguments,
+ then get_probe_argument_count will succeed but probe_argc will
+ also be zero. Both cases happen because of different things, but
+ they are treated equally here: action will be set to
+ PROBES_INTERFACE_FAILED. */
if (probe_argc == 2)
action = FULL_RELOAD;
else if (probe_argc < 2)
@@ -1950,7 +1966,17 @@ svr4_handle_solib_event (void)
usm_chain = make_cleanup (resume_section_map_updates_cleanup,
current_program_space);
- val = evaluate_probe_argument (pa->probe, 1, frame);
+ TRY
+ {
+ val = evaluate_probe_argument (pa->probe, 1, frame);
+ }
+ CATCH (ex, RETURN_MASK_ERROR)
+ {
+ exception_print (gdb_stderr, ex);
+ val = NULL;
+ }
+ END_CATCH
+
if (val == NULL)
{
do_cleanups (old_chain);
@@ -1981,7 +2007,18 @@ svr4_handle_solib_event (void)
if (action == UPDATE_OR_RELOAD)
{
- val = evaluate_probe_argument (pa->probe, 2, frame);
+ TRY
+ {
+ val = evaluate_probe_argument (pa->probe, 2, frame);
+ }
+ CATCH (ex, RETURN_MASK_ERROR)
+ {
+ exception_print (gdb_stderr, ex);
+ do_cleanups (old_chain);
+ return;
+ }
+ END_CATCH
+
if (val != NULL)
lm = value_as_address (val);
From ad1c917a79e8c5aa67657f148415c1bee01b240f Mon Sep 17 00:00:00 2001
From: Sergio Durigan Junior <sergiodj@redhat.com>
Date: Wed, 2 Sep 2015 00:34:22 -0400
Subject: [PATCH 3/4] Initialize variable and silence GCC warning from last
commit
BuildBot e-mailed me to let me know that my last commit broke GDB on
RHEL-7.1 s390x. On solib-svr4.c:svr4_handle_solib_event, 'val' now
needs to be initialized as NULL because it is inside a TRY..CATCH
block. This patch does that. Pushed as obvious.
gdb/ChangeLog:
2015-09-01 Sergio Durigan Junior <sergiodj@redhat.com>
* solib-svr4.c (svr4_handle_solib_event): Initialize 'val' as NULL
---
gdb/ChangeLog | 4 ++++
gdb/solib-svr4.c | 2 +-
2 files changed, 5 insertions(+), 1 deletion(-)
Index: gdb-7.10/gdb/solib-svr4.c
===================================================================
--- gdb-7.10.orig/gdb/solib-svr4.c
+++ gdb-7.10/gdb/solib-svr4.c
@@ -1918,7 +1918,7 @@ svr4_handle_solib_event (void)
struct probe_and_action *pa;
enum probe_action action;
struct cleanup *old_chain, *usm_chain;
- struct value *val;
+ struct value *val = NULL;
CORE_ADDR pc, debug_base, lm = 0;
int is_initial_ns;
struct frame_info *frame = get_current_frame ();
From 73c6b4756a7cee53c274ed05fddcd079b8b7e57c Mon Sep 17 00:00:00 2001
From: Sergio Durigan Junior <sergiodj@redhat.com>
Date: Wed, 2 Sep 2015 00:46:43 -0400
Subject: [PATCH 4/4] Initialize yet another variable to silence GCC warning
from last-but-one commit
Yet another BuildBot e-mail, yet another breakage on RHEL-7.1 s390x
(which uses an older GCC). This time,
solib-svr4.c:solib_event_probe_action has the probe_argc variable,
which is now inside a TRY..CATCH and therefore needs to be
initialized. Pushed as obvious.
gdb/ChangeLog:
2015-09-01 Sergio Durigan Junior <sergiodj@redhat.com>
* solib-svr4.c (solib_event_probe_action): Initialize 'probe_argc'
as zero.
---
gdb/ChangeLog | 5 +++++
gdb/solib-svr4.c | 2 +-
2 files changed, 6 insertions(+), 1 deletion(-)
2015-09-01 Sergio Durigan Junior <sergiodj@redhat.com>
Index: gdb-7.10/gdb/solib-svr4.c
===================================================================
--- gdb-7.10.orig/gdb/solib-svr4.c
+++ gdb-7.10/gdb/solib-svr4.c
@@ -1782,7 +1782,7 @@ static enum probe_action
solib_event_probe_action (struct probe_and_action *pa)
{
enum probe_action action;
- unsigned probe_argc;
+ unsigned probe_argc = 0;
struct frame_info *frame = get_current_frame ();
action = pa->action;

13
gdb.spec
View File

@ -26,7 +26,7 @@ Version: 7.10
# The release always contains a leading reserved number, start it at 1.
# `upstream' is not a part of `name' to stay fully rpm dependencies compatible for the testing.
Release: 16%{?dist}
Release: 17%{?dist}
License: GPLv3+ and GPLv3+ with exceptions and GPLv2+ and GPLv2+ with exceptions and GPL+ and LGPLv2+ and BSD and Public Domain and GFDL
Group: Development/Debuggers
@ -535,6 +535,11 @@ Patch927: gdb-python-gil.patch
# Fix jit-reader.h for multi-lib.
Patch978: gdb-jit-reader-multilib.patch
# Fix 'Make the probes-based dynamic linker interface more robust to
# errors' (Sergio Durigan Junior, RH BZ 1259132).
Patch1029: gdb-probes-based-interface-robust-1of2.patch
Patch1030: gdb-probes-based-interface-robust-2of2.patch
%if 0%{!?rhel:1} || 0%{?rhel} > 6
# RL_STATE_FEDORA_GDB would not be found for:
# Patch642: gdb-readline62-ask-more-rh.patch
@ -821,6 +826,8 @@ find -name "*.info*"|xargs rm -f
%patch925 -p1
%patch927 -p1
%patch978 -p1
%patch1029 -p1
%patch1030 -p1
%patch848 -p1
%if 0%{!?el6:1}
@ -1325,6 +1332,10 @@ then
fi
%changelog
* Wed Sep 2 2015 Sergio Durigan Junior <sergiodj@redhat.com> - 7.10-17.fc23
- Fix 'Make the probes-based dynamic linker interface more robust to
errors' (Sergio Durigan Junior, RH BZ 1259132).
* Tue Sep 1 2015 Jan Kratochvil <jan.kratochvil@redhat.com> - 7.10-16.fc23
- [RHEL] Fix librpm Recommends compatibility.