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

[Pedro Alves <pedro at codesourcery dot com>]

Finally getting back to this.

On Monday 28 February 2011 15:53:25, Jan Kratochvil wrote:
> On Tue, 22 Feb 2011 14:27:50 +0100, Pedro Alves wrote:
> > Make the regcache_XXX_read_XXX functions return an indication
> > of whether the register's value is valid, so that the
> > frame module can mark frame registers as unavailable.
> 
> The basic question on my mind is why instead of REG_UNAVAILABLE it does not
> throw NOT_AVAILABLE_ERROR?

There are other non-valid statuses.  Then, from the patch:

-/* NOTE: cagney/2002-08-13: At present GDB has no reliable mechanism
-   for indicating when a ``cooked'' register was constructed from
-   invalid or unavailable ``raw'' registers.  One fairly easy way of
-   adding such a mechanism would be for the cooked functions to return
-   a register valid indication.  Given the possibility of such a
-   change, the extract functions below use a reference parameter,
-   rather than a function result.  */

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, and they should look something like:

extern LONGEST regcache_cooked_get_signed (struct regcache *regcache,
                                          int regnum);

rather than:

extern enum register_status
  regcache_cooked_read_signed (struct regcache *regcache,
                              int regnum, LONGEST *val);

or the current:

extern void
  regcache_cooked_read_signed (struct regcache *regcache,
                              int regnum, LONGEST *val);

That is, make the return be the register value.  The frame
code has similar throw and non-throw variants.

> And if not NOT_AVAILABLE_ERROR then there should be
> __attribute__((warn_unused_result)) as if the caller is operating with
> not-available value - it is the case this patchset exactly tries to fix.

The thing is there are many many callers that only read register
values in the context of an inferior with execution.  E.g., stuff around infcalls, and extracting return values, and _writting_ registers (stuff that
needs to read-combine-write).  For those (and it's really a big a bunch
of those), I do agree that trying to work with such an unavailable
value might as well throw, because there's nothing else the
callers can do other than letting the error propagate up until the CLI.
But OTOH, there's no pressing need to change those.

> In fact all the memsets (, 0, ) could be rather changed to debug-stub 0x55.

Agreed, but I think it'd be better if we _don't_ do that _yet_.  Users are
quite used to seeing 0's and recognizing that as "bogus".  I think that if
we do that, we do it after branching.  Not even touching the contents is
a bit better during development, since then valgrind tells us when we touch
such invalid buffer contents.  That helped me a bit while developing all
this <unavailable> stuff.

> 
> 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.  The
REG_UNAVAILABLE error is more for the case when you want the
error to cross several layers  up (e.g., register -> frame | value -> expression -> CLI).  In those cases, it would be from awkward to 
impossible-to-do-sanely to propagate an "unavailable" error all the way
through error returns.

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.

So while I agree that warn_unused_result is an interesting idea,
I think to get there we'd need the throwing register read
routine variants mentioned above first, and to go through the
hundreds of regcache_XX_read_XXX instances, replacing those that
should throw, with a call to the throwing variant.

> The mail:
> 	graceful unwind termination when we'd need unavailable/uncollect memory or registers to unwind further
> is sure better but without this last mail it it printed:
> (gdb) bt
> #0  f () at 1.c:11
> #1  0x0000000000000000 in ?? ()

That was is because "frame_unwind_register" currently swallows
errors.  As you've discovered, only the "graceful unwind termination"
fixes that.  It has this:

Index: src/gdb/frame.c
===================================================================
--- 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);
 }

The problem is that I can't bring in that hunk into this
series without the rest of the "graceful unwind termination" patch,
otherwise, the user will get this:

(gdb) bt
#0  args_test_func (argc=<unavailable>, argi=<unavailable>, argf=<unavailable>, argd=<unavailable>, argstruct=..., argarray=<unavailable>)
    at ../../../src/gdb/testsuite/gdb.trace/unavailable.cc:199
#1  0x0000000000400d0b in main (argc=1, argv=0x7fff16571178, envp=0x7fff16571188) at ../../../src/gdb/testsuite/gdb.trace/unavailable.cc:364
PC not available

The "PC not available" note is an error thrown.  With MI, one
would get this:

(gdb) interpreter-exec mi "-stack-list-frames"
^error,msg="PC not available"

Instead of:

(gdb) interpreter-exec mi "-stack-list-frames"
^done,stack=[frame={level="0",addr="0x00000000004007ba",func="args_test_func",file="../../../src/gdb/testsuite/gdb.trace/unavailable.cc",fullname="/home/pedro/gdb/tdd_upstream/src/gdb/testsuite/gdb.trace/unavailable.cc",line="199"},frame={level="1",addr="0x0000000000400d0b",func="main",file="../../../src/gdb/testsuite/gdb.trace/unavailable.cc",fullname="/home/pedro/gdb/tdd_upstream/src/gdb/testsuite/gdb.trace/unavailable.cc",line="364"},frame={level="2",addr="0x0000000000000000",func="??"}]

... which would be a serious regression.

> 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)  :-)

Does that answer your questions?

-- 
Pedro Alves