Re: [unavailable regs/locals, 01/11] registers status upwards

[Jan Kratochvil <jan dot kratochvil at redhat dot com>]

On Wed, 16 Mar 2011 01:12:50 +0100, Pedro Alves wrote:
> I agree that throwing versions may be something good to have.
> But if we do have them, I think they should be alternative
> variants, not replacements,

The goal is to make any current code safe by default (that it throws an
exception), if one forgets to fix/modify/review it.  Another possibility is to
make it a compilation error by default (warn_unused_result) unless it is
either manually reviewed as safe (and marked by some (void) expr;) or changed
to be safe etc.  But there should be no invalid user data (=value 0) when the
developer forgets to review some part of code.


> > and while I did not try I believe one could still find some case(s) where GDB
> > will print 0.
> 
> I dare you find some (on x86-linux)  :-)

That is not the point whether you have or have not forgotten for such case
this time.  You can be sure at least any 3rd party patches (and all the
distros out there incl. CodeSourcery have some patches) can/will become
silently violating the <unavailable> rules.


> Not even touching the contents is a bit better during development, since
> then valgrind tells us when we touch such invalid buffer contents.

OK, true.


> > Just __attribute__((warn_unused_result)) errors on too many cases which
> > suggests more for the NOT_AVAILABLE_ERROR throw.
> 
> IMO it makes sense for the low level register cache read functions to have
> non-throwing variants, to let the caller decide to throw or not.  Making the
> low level functions themselves throw makes the code that _needs_ to care about
> the register status be quite awkward by needing to wrap in TRY_CATCH.

Yes, for callers who want to deal with it there should be some way to report
it without the GDB exceptions magic.

My point is by default it should behave safely (throw exception or
a compilation error).  How to code properly some alternative variants when one
is already modifying the caller code should be obvious.


> This patch (and the series), is only concerned with the _reading_ 
> side of the story, and making that work gracefuly.  And on that
> side I think it makes sense to use non-throwing variants, when
> directly manipulating a regcache.

In such case these functions should be `warn_unused_result' and they should be
renamed from the original functions.  The original function names should throw
an exception (for the case of infcall etc.) - if any <unavailable> code should
be handled directly by the callers the original function names can even
internal_error on <unavailable> values.


> --- src.orig/gdb/frame.c        2011-03-15 23:52:19.000000000 +0000
> +++ src/gdb/frame.c     2011-03-15 23:53:09.418353559 +0000
> @@ -912,6 +912,12 @@ frame_unwind_register (struct frame_info
>  
>    frame_register_unwind (frame, regnum, &optimized, &unavailable,
>                          &lval, &addr, &realnum, buf);
> +
> +  if (optimized)
> +    error (_("Register %d was optimized out"), regnum);
> +  if (unavailable)
> +    throw_error (NOT_AVAILABLE_ERROR,
> +                _("Register %d is not available"), regnum);
>  }

This shows how fragile the code it is.  If you forget a new error-checking
code invalid values silently leak into the upper layers.


> Does that answer your questions?

I wanted to express general disagreement with this style expecting preciseness
and no mistakes by the developers, which is not considered to be a "safe
enterprise development style".


Thanks,
Jan