Re: Always run GDB command post-hook after pre-hook has been run

[Stephen Cross <scross at undo-software dot com>]

Sorry for the delay in my response.

> It may happen to work in practice on currently supported hosts,
> but it's undefined C/C++.  Wrapper is the way to go, just like the
> comment says.

Thanks for clarifying.

> Do you support debugging with MI?  Or users scripting gdb with Python?

We do support MI and some GDB Python scripting (i.e. that doesn't
interfere with our own scripting; we've tried to minimise our own
scripting for this reason).

> Seems to me you'll still end up with problems with e.g., varobjs with
> function calls, -data-evaluate-expression, etc., which you'd end up
> solving probably with events around expression evaluation, similar to
> the infcall ones.

Yes, this is right and that's another part of this that we're working on.

> If you determined that up until recently, gdb used to call the
> hookpost even on error, and this was a recent regression, then it'd
> shine a different light on the idea.  OTOH, if this has always been
> this way, then I'm more inclined to have some way to distinguish
> normal hookpost vs error.

My testing suggests this behaviour (not calling hookpost on error) has
been consistent since at least GDB 7.0. So it's not a recent
regression.

So yes, hookerror-<command> seems like a sensible addition. I'll start
putting together a patch for this.

> Doesn't have to be literal "hookerror", could be something less
> confusing if you find it.

I think this is a clear name.

> Hmm, perhaps we should at least print the original error before
> losing it?

I think this is a problem more generally with GDB's internal
exceptions, since a new exception overrides the current exception
being handled (these semantics are similar to throwing from a finally
block in Java).

I imagine we could force it to print the original error by adding
another 'print_error_cleanup' that is run when the call to
'execute_cmd_post_hook' ends up throwing inside
'call_post_hook_cleanup'.

Alternatively, we could have a catch around the
'execute_cmd_post_hook' that would print the (new) error that came
from running the 'hookerror' and then we'd return normally from the
cleanup so the original error would be processed as normal. If there
was no original error it would still prevent the 'hookerror' exception
from leaking out of the cleanup. I think these semantics are possibly
more intuitive.

Thanks,
Stephen

On Wed, Dec 9, 2015 at 5:03 PM, Pedro Alves <palves@redhat.com> wrote:
> On 12/03/2015 04:11 PM, Stephen Cross wrote:
>> 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?
>
> It may happen to work in practice on currently supported hosts,
> but it's undefined C/C++.  Wrapper is the way to go, just like the
> comment says.
>
>>
>>> 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.
>
> Thanks for the explanation.
>
> Do you support debugging with MI?  Or users scripting gdb with Python?
> Seems to me you'll still end up with problems with e.g., varobjs with
> function calls, -data-evaluate-expression, etc., which you'd end up
> solving probably with events around expression evaluation, similar to
> the infcall ones.
>
>>
>>> 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 âfooâ, if the
>> user-defined command âhookpost-fooâ 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.
>
> If you determined that up until recently, gdb used to call the
> hookpost even on error, and this was a recent regression, then it'd
> shine a different light on the idea.  OTOH, if this has always been
> this way, then I'm more inclined to have some way to distinguish
> normal hookpost vs error.
>
>>
>>> 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.
>
> Doesn't have to be literal "hookerror", could be something less
> confusing if you find it.
>
>>
>>> 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.
>
> Hmm, perhaps we should at least print the original error before
> losing it?
>
> Thanks,
> Pedro Alves
>



-- 
Stephen Cross

Software Engineer at Undo Software