From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 23059 invoked by alias); 21 Oct 2014 20:30:29 -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 23048 invoked by uid 89); 21 Oct 2014 20:30:28 -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-f181.google.com Received: from mail-ob0-f181.google.com (HELO mail-ob0-f181.google.com) (209.85.214.181) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES128-SHA encrypted) ESMTPS; Tue, 21 Oct 2014 20:30:26 +0000 Received: by mail-ob0-f181.google.com with SMTP id wm4so1688066obc.26 for ; Tue, 21 Oct 2014 13:30:24 -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=paajIiN+DtN/wfgpWEIrIXiHu3jYS7xWP+hMa8vnS4E=; b=P7JwkKjIkkGsvht7Ym1/yC7z1H6xGuu8buoQxt+Rll1cnvTeoRHh9yGfps/n1AvmWl QuB7JlAgzJpIQskAhLkSHEiasp/nWm1boPYKOXp6RoMcJlJXLV5pPP+LDvT3iogjxo1c aOq0Z65IZvg7d36YqVs1msc/Xg5i82CXl7yeXUddEkqjkUFDzjaiDcgbEv9oKXCT/y3e EnMlo8s/MvGtfL4WW/tEviyG7F3uq4bySy4G+j1zqXqbKOBuHqqPcIW2HLt845LtJcv6 57IHeUbVcbowB/IUcgN9aDBKm86KA0cmFyffPkwrqcGwmXEaImtYL9wE+36afOgb5KTA a7Sw== X-Gm-Message-State: ALoCoQkrT7ftoHgBWDDCC2AdTQQ8fjIaDNpUgEBuj5lxH4uBRmsG267HJ0cWR5J2Xe3gaI7aPEd6 MIME-Version: 1.0 X-Received: by 10.182.55.74 with SMTP id q10mr32376161obp.26.1413923424668; Tue, 21 Oct 2014 13:30:24 -0700 (PDT) Received: by 10.202.197.13 with HTTP; Tue, 21 Oct 2014 13:30:24 -0700 (PDT) In-Reply-To: <201410211115.s9LBFkro030607@d06av02.portsmouth.uk.ibm.com> References: <201410211115.s9LBFkro030607@d06av02.portsmouth.uk.ibm.com> Date: Tue, 21 Oct 2014 20:30: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/msg00553.txt.bz2 On Tue, Oct 21, 2014 at 4:15 AM, Ulrich Weigand wrote: > I was not refering to the ABI, but the C++ standard semantics that > define what happen when you pass an *object* as argument to a function > that expects a *reference* parameter. See e.g.: > http://en.cppreference.com/w/cpp/language/reference_initialization > > References are initialized in the following situations: > [...] > 3) In a function call expression, when the function parameter > has reference type > [...] > The effects of reference initialization are: > [...] > if the reference is [...] lvalue reference to const: > [...] > a temporary of type T is constructed and copy-initialized from > object. The reference is then bound to this temporary I do not think applies in general for const references. IIUC, it applies to prvalues/literals [section 12.2 in the C++ std]. For all other kind of values, the two cases above this point should be applied. (Moreover, if it applies to all const reference arguments, what benefit are references bringing in over value arguments? One could just pass const value args.) > [ B.t.w. note that for full compatibility with C++, we might also need > to invoke the *constructor* when creating the temporary, not just the > destructor afterwards. ] This is another reason why targeting return values is simpler: we do not need to construct anything (via a constructor) as the inferior function would have constructed it appropriately. We only need to invoke the destructor on these objects. In the current state of the union however: 1. GDB does not distinguish between const and non-const reference arguments. IMO, this is semantically OK as the compiler would have ensured the inferior function does not modify values pointed to by const-references. 2. For value arguments which need to be passed as a reference (when the ABI determines so), GDB still does what it does in #1 above. This IMO is wrong in the case on non-const value arguments as non-const value arguments can be written to in the called inferior function. The correct fix for this would be to invoke the constructor and destructor. (At the very least, make a bit copy on the stack which will not need construction or destruction, but see point 3 below.) 3. For values passed by value, GDB does not invoke a copy constructor, but only does a bit copy. Consequently, invocation of a destructor is also not required. I think this is OK for 99% of the use cases, but could be fixed for the remaining cases. 4. GDB does not invoke destructors on function return values returned by value. This is not OK as it can potentially leak inferior memory. >> > 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. > > My point was that if you'd implemented construction of the temporary *there*, > then the patch would handle that case as well, without needing two separate > implementations. > >> > 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. > > The C++ standard says that if the function accepts a const reference, > then *any* object of that type can be passed, and will always lead to > creation of a temporary (unless the object is already an lvalue). Function arguments could be xvalues. IIUC, temporaries are created only for prvalues. You can point me to the appropriate section in the C++ standard if I am wrong. > This should make it safe to do the same in GDB as well. > >> > 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? > > As mentioned above, it should always be safe if we have a const reference. > (And if we don't, the call *should* fail even if we pass the return value > of another function call, since it wouldn't be allowed in C++ either.) Functions can have value arguments and can also return by value. This piece of code is valid C++ without involving const references in a chained function call: class A { public: int a; A () { } A (const A &obj) { a = obj.a; } int geta (void); }; int A::geta () { return a; } A func (A a) { A n; n.a = a.a + 1000; return n; } int main (void) { A a; a.a = 0; a.a = func (func (func (a))).geta(); /* function chain */ return 0; } >> 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? > > I really feel this attempts to address the problem at the wrong point. > Not only do you not handle arguments that aren't function return values, > it would be difficult to get constructor calls correct (see above), and As I mentioned above, return values will not need a constructor invocation. > you create mirrored values even for return values that are *not* used I think > as function arguments subsequently ... 1. The last return value is mirrored on the stack even if not used as an argument to a subsequent function call. But, this is necessary anyway. We clean it up after the expression is evaluated. 2. Intermediate non-class(struct/union) return values also get mirrored on stack even though they might not be used as a function args. But, since expressions are actually "interpreted" rather than "compiled" from within GDB, there is no way to determine which need a copy, which do not. >> > 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. > > Yes, I think it would be better to just use lval_memory. There isn't really > anything special about those values, *except* that might have to clean them > up later; and for that, we have to keep them on a separate list anyway. OK. I will remove the new lval type. > [ As an aside, I'm wondering whether it wouldn't be cleaner to keep that list > of on-stack temporaries associated with the current *thread* instead of an > expression; it might be more natural since they reside on the thread stack, > and call_function_by_hand already uses the thread struct, so no need to > pass an expression all the way down into it. Also, infrun then might > verify that temporaries were indeed cleaned up before restarting execution > of the thread (for anything except inferior calls). ] > >> > (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. > > Note that 2/2 of the v3 seems to be missing the actual patch ... Sent it now.