From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 17708 invoked by alias); 20 Oct 2014 16:01:32 -0000 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 Received: (qmail 17696 invoked by uid 89); 20 Oct 2014 16:01:30 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.5 required=5.0 tests=AWL,BAYES_00,MSGID_FROM_MTA_HEADER,RP_MATCHES_RCVD,SPF_PASS autolearn=ham version=3.3.2 X-HELO: e06smtp11.uk.ibm.com Received: from e06smtp11.uk.ibm.com (HELO e06smtp11.uk.ibm.com) (195.75.94.107) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES256-SHA encrypted) ESMTPS; Mon, 20 Oct 2014 16:01:29 +0000 Received: from /spool/local by e06smtp11.uk.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Mon, 20 Oct 2014 17:01:25 +0100 Received: from d06dlp03.portsmouth.uk.ibm.com (9.149.20.15) by e06smtp11.uk.ibm.com (192.168.101.141) with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted; Mon, 20 Oct 2014 17:01:25 +0100 Received: from b06cxnps3074.portsmouth.uk.ibm.com (d06relay09.portsmouth.uk.ibm.com [9.149.109.194]) by d06dlp03.portsmouth.uk.ibm.com (Postfix) with ESMTP id 15FAE1B08049 for ; Mon, 20 Oct 2014 17:02:45 +0100 (BST) Received: from d06av02.portsmouth.uk.ibm.com (d06av02.portsmouth.uk.ibm.com [9.149.37.228]) by b06cxnps3074.portsmouth.uk.ibm.com (8.14.9/8.14.9/NCO v10.0) with ESMTP id s9KG1Omr17498486 for ; Mon, 20 Oct 2014 16:01:24 GMT Received: from d06av02.portsmouth.uk.ibm.com (localhost [127.0.0.1]) by d06av02.portsmouth.uk.ibm.com (8.14.4/8.14.4/NCO v10.0 AVout) with ESMTP id s9KG1NZ5031905 for ; Mon, 20 Oct 2014 10:01:23 -0600 Received: from tuxmaker.boeblingen.de.ibm.com (tuxmaker.boeblingen.de.ibm.com [9.152.85.9]) by d06av02.portsmouth.uk.ibm.com (8.14.4/8.14.4/NCO v10.0 AVin) with SMTP id s9KG1LDM031801; Mon, 20 Oct 2014 10:01:21 -0600 Message-Id: <201410201601.s9KG1LDM031801@d06av02.portsmouth.uk.ibm.com> Received: by tuxmaker.boeblingen.de.ibm.com (sSMTP sendmail emulation); Mon, 20 Oct 2014 18:01:20 +0200 Subject: Re: [PATCH 0/2] Make chained function calls in expressions work To: sivachandra@google.com (Siva Chandra) Date: Mon, 20 Oct 2014 16:01:00 -0000 From: "Ulrich Weigand" Cc: gdb-patches@sourceware.org (gdb-patches) In-Reply-To: from "Siva Chandra" at Oct 01, 2014 11:25:58 AM MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Transfer-Encoding: 7bit X-TM-AS-MML: disable X-Content-Scanned: Fidelis XPS MAILER x-cbid: 14102016-0005-0000-0000-000001BA7C3B X-SW-Source: 2014-10/txt/msg00510.txt.bz2 Siva Chandra wrote: > On Wed, Oct 1, 2014 at 11:05 AM, Siva Chandra wrote: > > On Wed, Oct 1, 2014 at 6:15 AM, Ulrich Weigand wrote: > >> I'm wondering if there isn't a simpler way to solve this issue: couldn't > >> you instead during preparation of the second call_function_by_hand simply > >> allocate extra space on the stack and copy not_lval values whose address > >> needs to be taken there? This would avoid adding the new lval type, all > >> the extra state to track mirrored values during an expression, and would > >> actually allow you to pass *other* not_lval values to inferior calls too > >> (not just those originating from another inferior call). > > > > I did think about this route. However, look at the comment at > > eval.c:1405. It has an argument for why we should not in general copy > > function args on to the stack. Good point, we cannot do that in the general case. > > My patches here target return values of functions. Though return > > values end up being function arguments in a chained function call > > expression, IMO return values do not suffer from the same problem > > pointed to in the comment from above. > > > > 1. If a function returns a reference, creating a copy of the reference > > on the stack and passing it around for the duration the expression is > > being evaluated should not be a problem. > > > > 2. If a function returns a value, then it is either returned on the > > stack or in a register. My patches do not really disturb the case of a > > value being returned on stack. Even when values are returned in > > registers, intermediate return values are only temporaries and holding > > onto their addresses in some stateful entity will be an error. However, I'm still not convinced that the decision ought to made at the point a function *returns*. As I understand the C++ standard, creation of the temporary is rather triggered by the fact that a value is bound to a function *argument* of reference type. For example, if we had a function taking a "const int &" argument, then passing an integer constant to that function should result in the creation of a temporary, even though there is no function return value involved. It seems to me the proper place to handle this would be the TYPE_CODE_REF case in value_arg_coerce, where we currently have this comment: /* Cast the value to the reference's target type, and then convert it back to a reference. This will issue an error if the value was not previously in memory - in some cases we should clearly be allowing this, but how? */ > A tangential point, GDB does not call destructors on these temporaries > which IMO is an error. That is why, in my 2/2, you will notice that > the expression's state is holding onto all the return value > temporaries in a vector instead of just the last one created; In a > future pass, I would like to implement invoking the destructors on > these temporaries after the expression result is evaluated. I see. This is indeed a good argument for temporaries to persist longer than a single inferior call. We could still *allocate* (and initialize) the temporary when processing function arguments for the "outer" call, though, see above. (As an aside, the precise rules on temporary creation and lifetime are of course language specific. So in an ideal world, we might want the language parser to handle this and insert temporary creation/destruction opcodes into the expression, and have expression evaluation simply process those as usual without having to hard-code C++ semantics. However, as there's unfortunately already lots of other C++ semantics hardcoded, this is certainly not a hard requirement to get the patch accepted at this point ...) Another point: Even if we allow for temporaries to persist, I'm still not sure I understand the need for the new lval_mirrored_on_inferior_stack type. Why can't they just be regular (non-lazy) lval_memory values? Those already both refer to an inferior address and cache the contents in GDB memory. At the point the temporaries in the inferior disappear, those values could then be flipped to non_lval. (Actually, I'm not seeing anywhere in your current patches where lval_mirrored_on_inferior_stack values are marked invalid once the inferior stack contents disappear?) A final concern is that leaving temporaries on the stack after an inferior call is finishes just seems a bit fragile, in particular since they're *below* the thread's SP value at times, making them potentially vulnerable to being overwritten by signal handlers etc. Now I *think* this is not a problem in current GDB (with your patch) since we will not be running the inferior thread before expression evaluation ends, *except* for executing further inferior calls, and those will again adjust the SP. But future modifications to epression evaluation logic will have to keep this in mind, so there should at least be a comment somewhere ... Bye, Ulrich PS: Sorry for the delay in replying, I was out of office for a bit and then got sidetracked with other stuff. I'll try to be more prompt in further discussion on this topic. I certainly appreciate you working on this; improving GDB's C++ debugging capabilities is important! -- Dr. Ulrich Weigand GNU/Linux compilers and toolchain Ulrich.Weigand@de.ibm.com