From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 29987 invoked by alias); 16 Mar 2011 00:13:06 -0000 Received: (qmail 29971 invoked by uid 22791); 16 Mar 2011 00:13:04 -0000 X-SWARE-Spam-Status: No, hits=-1.9 required=5.0 tests=AWL,BAYES_00,TW_EG,T_RP_MATCHES_RCVD X-Spam-Check-By: sourceware.org Received: from mail.codesourcery.com (HELO mail.codesourcery.com) (38.113.113.100) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Wed, 16 Mar 2011 00:12:55 +0000 Received: (qmail 8334 invoked from network); 16 Mar 2011 00:12:54 -0000 Received: from unknown (HELO scottsdale.localnet) (pedro@127.0.0.2) by mail.codesourcery.com with ESMTPA; 16 Mar 2011 00:12:54 -0000 From: Pedro Alves To: gdb-patches@sourceware.org Subject: Re: [unavailable regs/locals, 01/11] registers status upwards Date: Wed, 16 Mar 2011 01:40:00 -0000 User-Agent: KMail/1.13.5 (Linux/2.6.35-27-generic; KDE/4.6.1; x86_64; ; ) Cc: Jan Kratochvil References: <201102221327.51130.pedro@codesourcery.com> <20110228155325.GB7881@host1.dyn.jankratochvil.net> In-Reply-To: <20110228155325.GB7881@host1.dyn.jankratochvil.net> MIME-Version: 1.0 Content-Type: Text/Plain; charset="iso-8859-1" Content-Transfer-Encoding: 7bit Message-Id: <201103160012.50971.pedro@codesourcery.com> X-IsSubscribed: yes Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org X-SW-Source: 2011-03/txt/msg00827.txt.bz2 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 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=, argi=, argf=, argd=, argstruct=..., argarray=) 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