From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 30133 invoked by alias); 3 Dec 2015 16:11:32 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 30066 invoked by uid 89); 3 Dec 2015 16:11:32 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.6 required=5.0 tests=BAYES_00,RCVD_IN_DNSWL_LOW,SPF_PASS autolearn=unavailable version=3.3.2 X-HELO: mail-wm0-f45.google.com Received: from mail-wm0-f45.google.com (HELO mail-wm0-f45.google.com) (74.125.82.45) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES128-GCM-SHA256 encrypted) ESMTPS; Thu, 03 Dec 2015 16:11:29 +0000 Received: by wmec201 with SMTP id c201so28938739wme.1 for ; Thu, 03 Dec 2015 08:11:26 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:mime-version:in-reply-to:references:date :message-id:subject:from:to:cc:content-type :content-transfer-encoding; bh=actJbYRqPrtyMdotsensl3lsHvTc2zsT7kRxJfr2ldg=; b=Mt8As3FOfQI+uoyJIePN9m4EyPlG/tMQzSDZAxH5JQNPgbaRV6TmJTzMMcSsU31Nph U7LGwbBpR/AozC4yBGOeTnf4QSXRXlS1GQSYTi3RarssaPxRC0qRM+ILTX3jVDaa48db GvMV8HQMB3gaGVCOeWU9PiYCJU7hkM7ZNWa3uywBW+ND6WpGrYQDgWnFmV0Ed598ARCw bt9Muu2xXWUMuc/TMuXcLYhBqoLTo0/HEpAxmoCUS1bn57gRTTIbXnOgWrc+AZTj3hSX o0p7yG6JasOlAA20d1NgCTj4PqURzB/FLpXx6K/sZ0NW7B9HXXXQw5ECm/Li2mWfpaXC I6ew== X-Gm-Message-State: ALoCoQlt6/6euVVBjVZNEbpkU+KIFNLjx2/a+eUeOngEfRQnGSOSC8JjsdG8H0qB0cfAcRnw8NUb MIME-Version: 1.0 X-Received: by 10.28.35.203 with SMTP id j194mr50796480wmj.13.1449159086113; Thu, 03 Dec 2015 08:11:26 -0800 (PST) Received: by 10.28.25.4 with HTTP; Thu, 3 Dec 2015 08:11:26 -0800 (PST) In-Reply-To: <56602D7B.2010404@redhat.com> References: <56602D7B.2010404@redhat.com> Date: Thu, 03 Dec 2015 16:11:00 -0000 Message-ID: Subject: Re: Always run GDB command post-hook after pre-hook has been run From: Stephen Cross To: Pedro Alves Cc: gdb-patches@sourceware.org, gdb@sourceware.org Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable X-IsSubscribed: yes X-SW-Source: 2015-12/txt/msg00054.txt.bz2 Hi Pedro, > Implementation approach, or on the idea in the first place? I'm mostly looking for input on the idea, because I know that the proposed change affects the behaviour of post-hooks. The only relevant point about the implementation was that I added: +static void +call_post_hook_cleanup(void* p) +{ + execute_cmd_post_hook (p); +} I added this in response to a comment in 'cleanups.h' that says (emphasis on the last line): /* NOTE: cagney/2000-03-04: This typedef is strictly for the make_cleanup function declarations below. Do not use this typedef as a cast when passing functions into the make_cleanup() code. Instead either use a bounce function or add a wrapper function. Calling a f(char*) function with f(void*) is non-portable. */ I had thought that calling f(char*) via f(void*) would be portable, but I've added the wrapper function just in case. What do you think? > I think it'd help if you told us the motivation. What's the intent > of running the hookpost even on error? What are you trying to use the > hooks for? Our focus here is on commands that can perform inferior calls. We have a replacement for gdbserver that by default doesn't support inferior calls, since we can be part way through a debuggee's history (our core product is a reversible debugger). So for inferior calls we have to issue a command to tell our server to fork the current debuggee process and then GDB can make arbitrary modifications to the fork child process; once the inferior call is complete we then issue another command to tell the server to drop the fork child process and switch back to the parent process. We're currently using the inferior call events added by my colleague (Nick Bull) and these work for most cases. However we've found that if GDB performs an inferior call which returns a pointer and then prints that, GDB will access the memory *after* issuing the inferior call end event. If the inferior call returns a buffer it allocated/modified then this can cause us to print the old value of the buffer. Hooks solve this problem because we can keep the fork child process around long enough for GDB to read the correct value. Unfortunately this means that if the 'print' command fails for any reason then we won't have been notified to drop the child process, affecting the rest of the debug session. > Playing devil's advocate, isn't it reasonable to say that existing > hookpost scripts out there may be assuming that they only run if > the hooked command finished successfully? The online docs say "Whenever you run the command =E2=80=98foo=E2=80=99, if= the user-defined command =E2=80=98hookpost-foo=E2=80=99 exists, it is executed = (with no arguments) after that command.". They don't seem to mention that sometimes the post-hook might not be run. Having said that, users may have observed this happening in practice. > Wonder whether we should have that. Alternatively, guess we could have > a new hookerror hook, that would run on error instead of hookpost. Yes, I think having a new 'hookerror' hook would be reasonable. This would ensure existing users wouldn't be affected, but the naming might cause confusion, so hookpost would need to be clearly documented as only being run in the success case. I'm happy to augment the patch to do this. > What happens / should happen if the hookpost itself throws an error? Do > we lose the original hooked-command's error? Is that OK? I previously tested this case with the patch applied and it appears that the error in the post-hook is what appears. So yes, we do lose the original hooked command's error. It looks like this is because the throw_exception() function inside GDB first calls 'do_cleanups (all_cleanups ());' and do_cleanups() allows cleanup functions to throw (and it updates the list of cleanups before running each cleanup). This behaviour seems OK to me, particularly if we added a new 'hookerror' and warned in the documentation that this occurs. Presumably this should also have a testcase. Thanks, Stephen On Thu, Dec 3, 2015 at 11:54 AM, Pedro Alves wrote: > On 12/02/2015 06:24 PM, Stephen Cross wrote: >> I was wondering if anyone has had a chance to look at this patch? >> (I've also CC'ed the gdb list since I'm looking for input on the >> approach.) > > Implementation approach, or on the idea in the first place? > > I think it'd help if you told us the motivation. What's the intent > of running the hookpost even on error? What are you trying to use the > hooks for? > >>> As you can see the post-hook is now always being called, even if the >>> command fails. > > At first blush, it looks reasonable. But as always, the devil's in the > details. I think only defining what happens around the corner cases > can we be sure. > > Playing devil's advocate, isn't it reasonable to say that existing > hookpost scripts out there may be assuming that they only run if > the hooked command finished successfully? > > Curiously, the existing documentation actually has a related comment: > >> @cindex hooks, post-command >> @kindex hookpost >> A hook may also be defined which is run after the command you executed. >> Whenever you run the command @samp{foo}, if the user-defined command >> @samp{hookpost-foo} exists, it is executed (with no arguments) after >> that command. Post-execution hooks may exist simultaneously with >> pre-execution hooks, for the same command. >> >> It is valid for a hook to call the command which it hooks. If this >> occurs, the hook is not re-executed, thereby avoiding infinite recursion. >> >> @c It would be nice if hookpost could be passed a parameter indicating >> @c if the command it hooks executed properly or not. FIXME! > > Wonder whether we should have that. Alternatively, guess we could have > a new hookerror hook, that would run on error instead of hookpost. > > What happens / should happen if the hookpost itself throws an error? Do > we lose the original hooked-command's error? Is that OK? > > Thanks, > Pedro Alves > --=20 Stephen Cross Software Engineer at Undo Software