From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 23073 invoked by alias); 20 Oct 2014 19:56:41 -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 23061 invoked by uid 89); 20 Oct 2014 19:56:40 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-3.6 required=5.0 tests=AWL,BAYES_00,RCVD_IN_DNSWL_LOW,RP_MATCHES_RCVD,SPF_PASS autolearn=ham version=3.3.2 X-HELO: mail-ob0-f171.google.com Received: from mail-ob0-f171.google.com (HELO mail-ob0-f171.google.com) (209.85.214.171) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES128-SHA encrypted) ESMTPS; Mon, 20 Oct 2014 19:56:38 +0000 Received: by mail-ob0-f171.google.com with SMTP id va2so4518493obc.30 for ; Mon, 20 Oct 2014 12:56:36 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:mime-version:in-reply-to:references:date :message-id:subject:from:to:cc:content-type; bh=ZReqkytZjDbJGjwUu9qJeScw/SKFIBoX6BUB7ogkUFs=; b=JJj7lDaHLAriELhXEwaQyxA7lCBkec3TS3icO/sLTVHpkKGY04rk6lKk57jZEO+Acw 26io6FMIDtFL4wMXr5ozQ6i2XyM/6PMbPJGyE3OqHSoFYYA/S0xYTLzttU+8qnFadXRZ qXuWM00Oo1hBftYkqN9KDhNSVMEIubnTZG46qr4V72YPGfASn/hOlPQc6DzpYt3vC2LP tI7Ykotg480nxC4whW8PsLESpEMraGDxYIaAqBkf93er/oJwPLmxM/PYoQTkOB/mtfxB fsPyG2hbYG0LWqAYquUtIBnzZbaswVbm1ijTq+44u2sno1MiqN8fGVDKUlA65XCWDRBq OBuQ== X-Gm-Message-State: ALoCoQmWeXtTtnxAm0IPwRRO9DY7RkBYTbz10MBYWwuSKb57bJdec2eSJXwmz5zdeyR5Ho9hUqbp MIME-Version: 1.0 X-Received: by 10.202.172.83 with SMTP id v80mr2399279oie.105.1413834996846; Mon, 20 Oct 2014 12:56:36 -0700 (PDT) Received: by 10.202.197.13 with HTTP; Mon, 20 Oct 2014 12:56:36 -0700 (PDT) In-Reply-To: <201410201601.s9KG1LDM031801@d06av02.portsmouth.uk.ibm.com> References: <201410201601.s9KG1LDM031801@d06av02.portsmouth.uk.ibm.com> Date: Mon, 20 Oct 2014 19:56:00 -0000 Message-ID: Subject: Re: [PATCH 0/2] Make chained function calls in expressions work From: Siva Chandra To: Ulrich Weigand Cc: gdb-patches Content-Type: text/plain; charset=UTF-8 X-IsSubscribed: yes X-SW-Source: 2014-10/txt/msg00520.txt.bz2 On Mon, Oct 20, 2014 at 9:01 AM, Ulrich Weigand wrote: > However, I'm still not convinced that the decision ought to made at the > point a function *returns*. I am not sure any decision is being made here. All my patch set does is to make temporary copies of return values on the stack. > 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. I am confused about what you are describing. As far as mechanics of function calls goes, references are just pointers: http://mentorembedded.github.io/cxx-abi/abi.html#reference-parameter And, that is how GDB treats reference arguments any way. But ... > 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. ... for this case, as you point out, a temporary on the stack has to be created and its address has to be passed as argument. GDB is broken for this case currently and my patch set does not fix it. > 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? */ I have not thought enough about "any" function argument in general as it can in general be a bad thing. But I agree that there could be exceptions made for cases like this. I can take this up as a follow up task after this patch set is either accepted or abandoned. >> 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. How do we distinguish between which func arg can be copied on to the stack and which cannot? May be we could, but GDB already creates temporary copies for objects returned by value when the ABI requires that the return value's address be passed as a hidden first arg. My patch set actually derives inspiration from this; why not treat all return values like this instead of only when the ABI requires it? > 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. I agree that the new lval type is not required in principle. However, I brought them in so that it is clear as to how they have actually come to exist in that way at all. I can remove them it if it seems like noise. > (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?) Ah, Catch! It seems like I generated the diff from a wrong commit. Will shortly send the v3 of the patch set which has this correctly handled. > 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 ... I will add a comment for this. However, I am not sure how an expression evaluation (involving inferior function calls) can be performed when the inferior thread is actually running. > 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! Thanks a lot for actually taking time to look at this. The gymnastics of this patch set should not be required iff and when the 'compile' command can get going for C++. Until then, it will be great if we can get GDB to do "obvious" things correctly.