Error message revelation

Error message revelation

[Keith Seitz]

Hi,

I don't know about you, but I've always wondered why I seem to get a lot 
of messages like "Error:" in the console window... Here's an example:

(gdb) tk gdb_cmd "list" flubber
Error:
(gdb) tk gdb_get_breakpoint_info 12345678
Error:

The reason? When TCL_ERROR is returned by any wrapped call, IT IS ASSUMED 
THAT THE ERROR MESSAGE IS IN THE INTERPRETER.

Here's the interesting bit of code in gdbtk-cmds.c (call_wrapper):

  /*
   * Now copy the result over to the true Tcl result.  If
   * GDBTK_TO_RESULT flag bit is set, this just copies a null object
   * over to the Tcl result, which is fine because we should reset the
   * result in this case anyway.  If the wrapped command returned an
   * error, then we assume that the result is already set correctly.
   */
  if ((result_ptr->flags & GDBTK_IN_TCL_RESULT) || wrapped_returned_error)
    {
      Tcl_DecrRefCount (result_ptr->obj_ptr);
    }
  else
    {
      Tcl_SetObjResult (interp, result_ptr->obj_ptr);
    }

  result_ptr = old_result_ptr;

Anyone know why we only DecrRefCount when wrapped_returned_error is true? 
(In other words, why are we NOT copying result_ptr->obj_ptr into the 
interpreter?)

I can only guess that this was done because we want to do things like:

   Tcl_WrongNumArgs (...);
   return TCL_ERROR;

instead of

   Tcl_WrongNumArgs (...);
   result_ptr->flags &= GDBTK_IN_TCL_RESULT;
   return TCL_ERROR;

However, I think that the latter, more verbose code is much easier to 
remember... The result is ALWAYS taken from result_ptr->obj_ptr EXCEPT if 
GDBTK_IN_TCL_RESULT is specified. That's much simpler than: result is 
taken from result_ptr->obj_ptr EXCEPT if there was an error OR 
GDBTK_IN_TCL_RESULT is set.

If I don't hear any objections, I am going to clean this up to make it 
work. I don't want to remember a bunch of rules. Gdbtk is complex enough 
as it is.

Tom?? You're name is on the change:

2000-11-29  Tom Tromey  <tromey@cygnus.com>

        * gdbtk-cmds.c (call_wrapper): Don't reset result if wrapped
        command returned error.

Any idea what it was supposed to fix?
Keith

Re: Error message revelation

[Tom Tromey]

>>>>> "Keith" == Keith Seitz <keiths@cygnus.com> writes:

Keith>    Tcl_WrongNumArgs (...);
Keith>    return TCL_ERROR;

Yeah, this is what I wanted to be able to do.

Keith> 2000-11-29  Tom Tromey  <tromey@cygnus.com>
Keith>         * gdbtk-cmds.c (call_wrapper): Don't reset result if wrapped
Keith>         command returned error.

I did this because I didn't like the old rules, and nobody objected to
the change -- I'm sure the discussion (perhaps brief) is still in the
archives.

As I recall, what I found confusing was this:

When I'm writing a new Tcl command in gdbtk-cmds.c, it seems logical
to me that it should be like writing any other random Tcl command.
The point of call_wrapper should be, in my opinion, that the inner
oddities of the gdb exception handling mechanism are kept from the Tcl
code.

Also, requiring a new flag to be set on every error return seemed
redundant.  I couldn't think of a situation where you would want to
return a TCL_ERROR but have the result text taken from some other
source.

Keith> However, I think that the latter, more verbose code is much
Keith> easier to remember... The result is ALWAYS taken from
Keith> result_ptr->obj_ptr EXCEPT if GDBTK_IN_TCL_RESULT is
Keith> specified. That's much simpler than: result is taken from
Keith> result_ptr->obj_ptr EXCEPT if there was an error OR
Keith> GDBTK_IN_TCL_RESULT is set.

As I recall, when looking around the code in gdbtk, there were
numerous returns where the writer forgot to set GDBTK_IN_TCL_RESULT.

Personally I think this approach is a bug factory.  They aren't
critical bugs, perhaps, but I'm guessing people will often forget to
set the new flag.

I agree that the change I made isn't ideal.  As I recall it was just a
side track off some other problem I was working on.  I think the
call_wrapper error handling code is ripe for a deeper rethink than
just toggling this patch back out.  IMHO.

OTOH, a cheap-and-dirty method would be use have a new
`RETURN_TCL_ERROR' macro that sets the flag.  Then you could find all
the problem spots by a simple `grep "return TCL_ERROR"', both now and
in the future.

Tom

Re: Error message revelation

[Keith Seitz]

On 23 Apr 2001, Tom Tromey wrote:

> When I'm writing a new Tcl command in gdbtk-cmds.c, it seems logical
> to me that it should be like writing any other random Tcl command.
> The point of call_wrapper should be, in my opinion, that the inner
> oddities of the gdb exception handling mechanism are kept from the Tcl
> code.

I could see why this would be said: it is logical. Unfortunately...

> Also, requiring a new flag to be set on every error return seemed
> redundant.  I couldn't think of a situation where you would want to
> return a TCL_ERROR but have the result text taken from some other
> source.

The problem is that we have TWO ways of returning information to the tcl 
code: the interpreter's result and result_ptr. I really think that both 
of these methods are needed in our case. The confusion comes (as you've 
noted) that there doesn't appear to be any consistent style by the rest 
of the code, so no one (including me) really knows what is supposed to be 
"the right way". (Something more for my styleguide/internals doc).

I like the explicit method: if YOU (a command writer in gdbtk) say 
GDBTK_IN_TCL_RESULT, then the result from tcl is used, otherwise, it's 
the one you've built in result_ptr->obj_ptr.

> As I recall, when looking around the code in gdbtk, there were
> numerous returns where the writer forgot to set GDBTK_IN_TCL_RESULT.

To say the least. That's what's so damn confusing. My bad, really.

> Personally I think this approach is a bug factory.  They aren't
> critical bugs, perhaps, but I'm guessing people will often forget to
> set the new flag.

I really don't think it is a bug factory, although since nothing is 
clearly documented, it actually is turning out to be one. Again, my 
fault as a maintainer (and one of the principal contributors).
 
> OTOH, a cheap-and-dirty method would be use have a new
> `RETURN_TCL_ERROR' macro that sets the flag.  Then you could find all
> the problem spots by a simple `grep "return TCL_ERROR"', both now and
> in the future.

I was thinking along these lines, but now I am beginning to wonder. Maybe 
we should just keep this assumption, change all offending code in the 
sources, and document somewhere that the rules are:

1. You're a tcl command. Behave like one.
2. You're also a gdbtk command, so you _may_ use your own output
   object (result_ptr->obj_ptr, created for you automatically) to
   optimize list creation and assembling piecemeal output from gdb.

So, then, there is a proposal: MAINTAINERS and CONTRIBUTORS: PICK ONE:

1) ALL errors come from tcl interp
   ALL output comes from tcl interp UNLESS GDBTK_IN_TCL_RESULT set
2) ALL output/errors come from result_ptr->obj_ptr UNLESS
   GDBTK_IN_TCL_RESULT is set.

Which do people prefer? I can see pros and cons for each, but I think I 
am beginning to come around to #1...

Keith

Re: Error message revelation

[Tom Tromey]

>>>>> "Keith" == Keith Seitz <keiths@cygnus.com> writes:

Keith> The problem is that we have TWO ways of returning information
Keith> to the tcl code: the interpreter's result and result_ptr. I
Keith> really think that both of these methods are needed in our
Keith> case. The confusion comes (as you've noted) that there doesn't
Keith> appear to be any consistent style by the rest of the code, so
Keith> no one (including me) really knows what is supposed to be "the
Keith> right way". (Something more for my styleguide/internals doc).

Keith> I like the explicit method: if YOU (a command writer in gdbtk)
Keith> say GDBTK_IN_TCL_RESULT, then the result from tcl is used,
Keith> otherwise, it's the one you've built in result_ptr->obj_ptr.

I have a question: is it more common that you'd want to use the Tcl
result, or is it more common that you'd want to use the gdb result?
That is, maybe it makes sense to invert the flag to
GDBTK_IN_GDB_RESULT.

You would know this better than me.  In fact, I think really you
should do whatever you think is best, since you'll be working on this
code much, much more than I will.  I think as long as the convention
is documented and clearly used in all the gdbtk code, then it will be
fine in the long run.  That's particularly true if you use a helper
macro and have some automatic way to test for new bugs being added
(this is a technique we use in Automake, and it really does help -- we
have a "maintainer-check" target which checks for the checkable parts
of our coding standard.  We just run this occasionally, eg before
releases, and fix whatever it points out).

Checking in a jumbo patch to fix all the gdbtk*.c problems would be
really cool.  Unfortunately it is a daunting amount of code.  Finding
a day or two to do cleanup is hard :-(

Tom