From: Sergio Durigan Junior <sergiodj@redhat.com>
To: Gary Benson <gbenson@redhat.com>
Cc: GDB Patches <gdb-patches@sourceware.org>
Subject: Re: [PATCH 2/2] Catching errors on probes-based dynamic linker interface
Date: Mon, 24 Aug 2015 16:09:00 -0000 [thread overview]
Message-ID: <87r3msd5xr.fsf@redhat.com> (raw)
In-Reply-To: <20150824084255.GA16508@blade.nx> (Gary Benson's message of "Mon, 24 Aug 2015 09:42:55 +0100")
Thanks for the review, Gary.
On Monday, August 24 2015, Gary Benson wrote:
>> diff --git a/gdb/solib-svr4.c b/gdb/solib-svr4.c
>> index 1fb07d5..028c3d0 100644
>> --- a/gdb/solib-svr4.c
>> +++ b/gdb/solib-svr4.c
>> @@ -1786,7 +1786,17 @@ solib_event_probe_action (struct probe_and_action *pa)
>> 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 (probe_argc == 2)
>> action = FULL_RELOAD;
>> else if (probe_argc < 2)
>
> Maybe this would be clearer and more robust:
>
> TRY
> {
> unsigned probe_argc;
>
> probe_argc = get_probe_argument_count (pa->probe, frame);
>
> if (probe_argc == 2)
> action = FULL_RELOAD;
> else if (probe_argc < 2)
> action = PROBES_INTERFACE_FAILED;
> }
> CATCH (ex, RETURN_MASK_ERROR)
> {
> exception_print (gdb_stderr, ex);
> action = PROBES_INTERFACE_FAILED;
> }
> END_CATCH
Maybe it's a matter of preference, but I don't like this (and I don't
see why it is more robust). I prefer to have as little code as possible
running on the TRY block, and handle everything else outside of it. I
think it also makes things a bit more confuse because you have two
places where action can be PROBES_INTERFACE_FAILED.
I am using this idiom in the other places as well, so I also think it is
good to keep things similar across the code.
>> @@ -1971,7 +1991,17 @@ 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);
>> + val = NULL;
>> + }
>> + END_CATCH
>> +
>> if (val != NULL)
>> lm = value_as_address (val);
>>
>
> This failure will not cause the probes interface to be disabled.
> FULL_RELOAD is an ok thing to do here, but it could be difficult
> to debug in future (if this one probe argument is broken GDB will
> be very much slower than it could be.) So maybe this should be:
>
> CATCH (ex, RETURN_MASK_ERROR)
> {
> exception_print (gdb_stderr, ex);
> do_cleanups (old_chain);
> return;
> }
> END_CATCH
You're right, thanks for... catching... this.
> As an aside it would clarify this code greatly if "old_chain"
> were renamed "disable_probes_interface" or similar. It took
> me a while to figure out what the code was doing, and I wrote
> it!
Yeah. I'll leave this to another patch.
Here's the updated version of the current patch.
Thanks,
--
Sergio
GPG key ID: 237A 54B1 0287 28BF 00EF 31F4 D0EB 7628 65FC 5E36
Please send encrypted e-mail if possible
http://sergiodj.net/
gdb/ChangeLog:
2015-08-21 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.
diff --git a/gdb/solib-svr4.c b/gdb/solib-svr4.c
index 1fb07d5..fd90fc8 100644
--- a/gdb/solib-svr4.c
+++ b/gdb/solib-svr4.c
@@ -1786,7 +1786,17 @@ solib_event_probe_action (struct probe_and_action *pa)
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 (probe_argc == 2)
action = FULL_RELOAD;
else if (probe_argc < 2)
@@ -1940,7 +1950,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);
@@ -1971,7 +1991,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);
next prev parent reply other threads:[~2015-08-24 16:09 UTC|newest]
Thread overview: 17+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-08-21 23:37 [PATCH 0/2] Improve error management on probes-based dynamic linker interface (and workaround RH BZ 1196181) Sergio Durigan Junior
2015-08-21 23:37 ` [PATCH 1/2] Improve error reporting when handling SystemTap SDT probes Sergio Durigan Junior
2015-09-02 4:20 ` Sergio Durigan Junior
2015-08-21 23:37 ` [PATCH 2/2] Catching errors on probes-based dynamic linker interface Sergio Durigan Junior
2015-08-24 8:43 ` Gary Benson
2015-08-24 16:09 ` Sergio Durigan Junior [this message]
2015-08-25 12:47 ` Gary Benson
2015-08-25 18:17 ` Sergio Durigan Junior
2015-09-01 3:27 ` Sergio Durigan Junior
2015-09-01 9:24 ` Gary Benson
2015-09-01 16:26 ` Sergio Durigan Junior
2015-09-02 4:18 ` Sergio Durigan Junior
2015-09-02 4:22 ` Sergio Durigan Junior
2015-09-02 4:38 ` [PATCH] Initialize variable and silence GCC warning from last commit Sergio Durigan Junior
2015-09-02 4:50 ` [PATCH] Initialize yet another variable to silence GCC warning from last-but-one commit Sergio Durigan Junior
2015-09-02 4:20 ` [PATCH 0/2] Improve error management on probes-based dynamic linker interface (and workaround RH BZ 1196181) Sergio Durigan Junior
2015-09-02 16:38 ` Gary Benson
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=87r3msd5xr.fsf@redhat.com \
--to=sergiodj@redhat.com \
--cc=gbenson@redhat.com \
--cc=gdb-patches@sourceware.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).