Re: RFC: next/finish/etc -vs- exceptions

[Doug Evans <dje@google.com>]

Hi.  My limited knowledge didn't find anything seriously broken 1/2
:-), but I do have a few comments.

On Fri, May 29, 2009 at 3:32 PM, Tom Tromey <tromey@redhat.com> wrote:
> With the current gdb, a "next" over a call that throws an exception
> can cause gdb to act as though the user typed "continue".  This can be
> very frustrating when debugging C++ (or Java) code.  This also happens
> with other commands, particularly "finish", "until", and "advance".
>
> This patch fixes this problem.  It relies on a new debugging hook in
> GCC's unwinder, so it will only work for GCC 4.5 and later.

I wouldn't block on this, but one aspect of good u/i is setting expectations.
If something is known to not work, telling the user up front is
usually better than staying silent and leaving it to the user to
figure out.

To that end, should gdb print a warning if _Unwind_DebugHook isn't found?
Your patch isn't alone in this, and I'm not sure when and how to print
such warnings.
It just seems to me to be a good idea to have such warnings.
[To minimize the verbosity, gdb could print a general warning when it
detects such conditions, with instructions on how to get further
details.  E.g. "GDB has detected conditions that will hinder some
debugging efforts.  Use `mumble' for further information." or some
such.  That way the user might just get one warning at startup, and
that's it.  And we could expand on the conditions to detect without
affecting the basic session flow.]
The user might get used to the message, start ignoring it, etc. etc.
Maybe it could be off by default.
The high order bit though is that there would be standard things the
user can do to figure out why (e.g. in this case) "next" isn't working
the way s/he is expecting it to (for typically frequent cases that are
reasonable to handle - stripped libpthread anyone?).
Just a thought.

> @@ -6356,6 +6403,7 @@ struct until_break_command_continuation_args
>  {
>   struct breakpoint *breakpoint;
>   struct breakpoint *breakpoint2;
> +  int thread_num;
>  };
>
>  /* This function is called by fetch_inferior_event via the

Having to record the thread here seems orthogonal to handling next-ing
over exceptions (e.g. why isn't it also required for longjmp?).
I wouldn't necessarily put it in a separate patch though (regardless
of whether it is or isn't orthogonal).
Maybe add further comments explaining what's going on here?

> @@ -3851,6 +3876,112 @@ insert_longjmp_resume_breakpoint (CORE_ADDR pc)
>     set_momentary_breakpoint_at_pc (pc, bp_longjmp_resume);
>  }
>
> +/* Insert an exception resume breakpoint.  TP is the thread throwing
> +   the exception.  The block B is the block of the unwinder debug hook
> +   function.  FRAME is the frame corresponding to the call to this
> +   function.  SYM is the symbol of the function argument holding the
> +   target PC of the exception.  */
> +
> +static void
> +insert_exception_resume_breakpoint (struct thread_info *tp,
> +                                   struct block *b,
> +                                   struct frame_info *frame,
> +                                   struct symbol *sym)
> +{
> +  struct gdb_exception e;
> +
> +  /* We want to ignore errors here.  */
> +  TRY_CATCH (e, RETURN_MASK_ALL)
> +    {
> +      struct symbol *vsym;
> +      struct value *value;
> +      CORE_ADDR handler;
> +      struct breakpoint *bp;
> +
> +      vsym = lookup_symbol (SYMBOL_LINKAGE_NAME (sym), b, VAR_DOMAIN, NULL);
> +      value = read_var_value (vsym, frame);
> +      handler = value_as_address (value);
> +
> +      /* We're going to replace the current step-resume breakpoint
> +        with a longjmp-resume breakpoint.  */

s/longjmp/exception/  ?

> +      delete_step_resume_breakpoint (tp);
> +
> +      if (debug_infrun)
> +       fprintf_unfiltered (gdb_stdlog,
> +                           "infrun: exception resume at %lx\n",
> +                           (unsigned long) handler);
> +
> +      bp = set_momentary_breakpoint_at_pc (handler, bp_exception_resume);
> +      inferior_thread ()->step_resume_breakpoint = bp;
> +    }
> +}
> +
> +/* This is called when an exception has been intercepted.  Check to
> +   see whether the exception's destination is of interest, and if so,
> +   set an exception resume breakpoint there.  */
> +
> +static void
> +check_exception_resume (struct execution_control_state *ecs,
> +                       struct frame_info *frame, struct symbol *func)
> +{
> +  struct gdb_exception e;
> +
> +  TRY_CATCH (e, RETURN_MASK_ALL)
> +    {
> +      struct block *b;
> +      struct dict_iterator iter;
> +      struct symbol *sym;
> +      int argno = 0;
> +
> +      b = SYMBOL_BLOCK_VALUE (func);
> +      ALL_BLOCK_SYMBOLS (b, iter, sym)
> +       {
> +         if (!SYMBOL_IS_ARGUMENT (sym))
> +           continue;

It's a bit odd to check function arguments for something like this.
But maybe I just don't understand the implementation enough.

There's an implementation aspect here that's not clear to me.
Can you add comments elaborating on the argno == 0 vs argno > 0 cases?

Can we assume that function arguments will be seen in program order here?
[or am I misinterpreting argno == 0 vs != 0 ?]

> +
> +         if (argno == 0)
> +           {
> +             struct symbol *vsym;
> +             struct value *value;
> +             CORE_ADDR cfa, nexting_cfa;
> +             struct gdbarch *arch;
> +
> +             vsym = lookup_symbol (SYMBOL_LINKAGE_NAME (sym),
> +                                   b, VAR_DOMAIN, NULL);
> +             value = read_var_value (vsym, frame);
> +             cfa = value_as_address (value);
> +
> +             arch = get_frame_arch (frame);
> +             nexting_cfa = ecs->event_thread->exception_frame;
> +             if (debug_infrun)
> +               fprintf_unfiltered (gdb_stdlog,
> +                                   "infrun: comparing exception target %lx with next-frame %lx: %d\n",
> +                                   (unsigned long) cfa,
> +                                   (unsigned long) nexting_cfa,
> +                                   !nexting_cfa || gdbarch_inner_than (arch, cfa, nexting_cfa));
> +
> +             if (!nexting_cfa
> +                 || gdbarch_inner_than (arch, cfa, nexting_cfa))
> +               {
> +                 /* Not an interesting exception.  */
> +                 break;
> +               }
> +             ++argno;
> +           }
> +         else
> +           {
> +             /* If we got here, then we want to set the exception
> +                resume breakpoint at the address held in the second
> +                argument to the function.  */
> +
> +             insert_exception_resume_breakpoint (ecs->event_thread,
> +                                                 b, frame, sym);
> +             break;
> +           }
> +       }
> +    }
> +}
> +
>  static void
>  stop_stepping (struct execution_control_state *ecs)
>  {
> [...]
> diff --git a/gdb/testsuite/gdb.cp/Makefile.in b/gdb/testsuite/gdb.cp/Makefile.in
> index f4a989c..1a2b908 100644
> --- a/gdb/testsuite/gdb.cp/Makefile.in
> +++ b/gdb/testsuite/gdb.cp/Makefile.in
> @@ -4,7 +4,7 @@ srcdir = @srcdir@
>  EXECUTABLES = ambiguous annota2 anon-union cplusfuncs cttiadd \
>        derivation inherit local member-ptr method misc \
>         overload ovldbreak ref-typ ref-typ2 templates userdef virtfunc namespace \
> -       ref-types ref-params method2
> +       ref-types ref-params method2 gdb9593
>
>  all info install-info dvi install uninstall installcheck check:
>        @echo "Nothing to be done for $@..."

Blech.  I haven't been paying attention to this myself.
It's really nice to be able to add new testcases *without* having to
edit any files like Makefile.in.