From 2ef6d5f976d4aca1f803ac9a7fc5d0c377685da3 Mon Sep 17 00:00:00 2001 From: Sergio Durigan Junior Date: Wed, 2 Sep 2015 17:35:52 -0400 Subject: [PATCH] Fix 'Make the probes-based dynamic linker interface more robust to errors' (Sergio Durigan Junior, RH BZ 1259132). --- gdb-probes-based-interface-robust-1of2.patch | 175 +++++++++++++++++++ gdb-probes-based-interface-robust-2of2.patch | 143 +++++++++++++++ gdb.spec | 13 +- 3 files changed, 330 insertions(+), 1 deletion(-) create mode 100644 gdb-probes-based-interface-robust-1of2.patch create mode 100644 gdb-probes-based-interface-robust-2of2.patch diff --git a/gdb-probes-based-interface-robust-1of2.patch b/gdb-probes-based-interface-robust-1of2.patch new file mode 100644 index 0000000..72091dd --- /dev/null +++ b/gdb-probes-based-interface-robust-1of2.patch @@ -0,0 +1,175 @@ +From f469e8ce11672e26feb5ba6f9a134275fcfd5b4f Mon Sep 17 00:00:00 2001 +From: Sergio Durigan Junior +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 + + * probe.h (struct probe_ops) : 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.9.1/gdb/probe.h +=================================================================== +--- gdb-7.9.1.orig/gdb/probe.h ++++ gdb-7.9.1/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, +@@ -246,7 +248,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); +@@ -258,7 +262,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.9.1/gdb/stap-probe.c +=================================================================== +--- gdb-7.9.1.orig/gdb/stap-probe.c ++++ gdb-7.9.1/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); + } + diff --git a/gdb-probes-based-interface-robust-2of2.patch b/gdb-probes-based-interface-robust-2of2.patch new file mode 100644 index 0000000..4b2f6f9 --- /dev/null +++ b/gdb-probes-based-interface-robust-2of2.patch @@ -0,0 +1,143 @@ +From 3bd7e5b7ee5ea0b3bbb4030ca841f66faad74f0f Mon Sep 17 00:00:00 2001 +From: Sergio Durigan Junior +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 +, 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 + + * 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.9.1/gdb/solib-svr4.c +=================================================================== +--- gdb-7.9.1.orig/gdb/solib-svr4.c ++++ gdb-7.9.1/gdb/solib-svr4.c +@@ -1769,7 +1769,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; +@@ -1783,7 +1783,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) +@@ -1889,7 +1905,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 (); +@@ -1937,7 +1953,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); +@@ -1968,7 +1994,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); + diff --git a/gdb.spec b/gdb.spec index 1f7a45e..fd6e514 100644 --- a/gdb.spec +++ b/gdb.spec @@ -26,7 +26,7 @@ Version: 7.9.1 # 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: 17%{?dist} +Release: 18%{?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 @@ -544,6 +544,11 @@ Patch991: gdb-cxx-enum-tag.patch # Fix 'info type-printers' Python error (Clem Dickey, RH BZ 1085576). Patch992: gdb-type-printers-error.patch +# Fix 'Make the probes-based dynamic linker interface more robust to +# errors' (Sergio Durigan Junior, RH BZ 1259132). +Patch1031: gdb-probes-based-interface-robust-1of2.patch +Patch1032: 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 @@ -834,6 +839,8 @@ find -name "*.info*"|xargs rm -f %patch985 -p1 %patch991 -p1 %patch992 -p1 +%patch1031 -p1 +%patch1032 -p1 %patch848 -p1 %if 0%{!?el6:1} @@ -1334,6 +1341,10 @@ then fi %changelog +* Wed Sep 2 2015 Sergio Durigan Junior - 7.9.1-18.fc22 +- Fix 'Make the probes-based dynamic linker interface more robust to + errors' (Sergio Durigan Junior, RH BZ 1259132). + * Fri Jun 26 2015 Jan Kratochvil - 7.9.1-17.fc22 - Fix 'info type-printers' Python error (Clem Dickey, RH BZ 1085576).