From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 22661 invoked by alias); 30 Oct 2007 09:22:32 -0000 Received: (qmail 22651 invoked by uid 22791); 30 Oct 2007 09:22:31 -0000 X-Spam-Check-By: sourceware.org Received: from zigzag.lvk.cs.msu.su (HELO zigzag.lvk.cs.msu.su) (158.250.17.23) by sourceware.org (qpsmtpd/0.31) with ESMTP; Tue, 30 Oct 2007 09:22:25 +0000 Received: from Debian-exim by zigzag.lvk.cs.msu.su with spam-scanned (Exim 4.50) id 1ImoEg-0005c7-Q0 for gdb-patches@sources.redhat.com; Tue, 30 Oct 2007 13:22:20 +0300 Received: from localhost ([127.0.0.1] helo=ip6-localhost) by zigzag.lvk.cs.msu.su with esmtps (TLS-1.0:DHE_RSA_AES_256_CBC_SHA:32) (Exim 4.50) id 1ImoEV-0005bs-FL; Tue, 30 Oct 2007 13:22:03 +0300 From: Vladimir Prus To: Nick Roberts Subject: Re: [PATCH] MI: lvalues and variable_editable Date: Tue, 30 Oct 2007 09:55:00 -0000 User-Agent: KMail/1.9.6 Cc: gdb-patches@sources.redhat.com References: <18210.27153.559569.601092@kahikatea.snap.net.nz> <200710271615.23579.ghost@cs.msu.su> <18214.46470.57856.84852@kahikatea.snap.net.nz> In-Reply-To: <18214.46470.57856.84852@kahikatea.snap.net.nz> MIME-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: 7bit Content-Disposition: inline Message-Id: <200710301222.03459.ghost@cs.msu.su> 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: 2007-10/txt/msg00793.txt.bz2 On Tuesday 30 October 2007 07:39:34 Nick Roberts wrote: > > > > 1. In varobj_editable_p you call gdb_evaluate_expression, and I believe > > > > this to be wrong. We call gdb_evaluate_expression when we create > > > > varobj, and it either succeeds, eventually setting varobj->value to > > > > something, or it does not. There's no point to call > > > > gdb_evaluate_expression again. > > > > > > If gdb_evaluate_expression fails in varobj_create, a variable object is > > > still created, but just with an undefined value. It needs to be called to > > > get value for VALUE_LVAL. > > > > Why would a second call to gdb_evaluate_expression succeed? We already tried > > to evaluate expression, we failed, varobj has no value whatsoever. So, > > I'd say we should refuse to assign anything in this case. > > The second call succeeds if the first one did (when the varobj does have > a value) - but we seem to be going backwards. Probably. Let me try again: 1. If the first call succeeds, we already have a value, and we can directly check if that's lvalue. Why call gdb_evaluate_expression in that case? 2. If the first call fails, we get a varobj with no associated value at all. I believe that var->value will be NULL. Why should varobj_editable_p try to evaluate expression again? Specifically: - Why would this second call to gdb_evaluate succeed, if it failed previously? - If we reevaluate the expression in a frame different from frame where -var-create was invoked, we can get value that bears no relation to what was meant in -var-create, do we want this? - If var->value is NULL, then varobj_set_value will return immediately, doing nothing. So, actually, varobj with value of NULL are not editable anyway. > > > > Further, > > > > in varobj_create, gdb_evaluate_expression is called in specific frame, > > > > and varobj_editable_p calls it in current frame. > > > > > > The current frame should be the frame in which the variable object is defined. > > > Can you explain why that's a problem? > > > > Here's the code in question: > > > > /* When the frame is different from the current frame, > > we must select the appropriate frame before parsing > > the expression, otherwise the value will not be current. > > Since select_frame is so benign, just call it for all cases. */ > > if (fi != NULL) > > { > > var->root->frame = get_frame_id (fi); > > old_fi = get_selected_frame (NULL); > > select_frame (fi); > > } > > > > /* We definitively need to catch errors here. > > If evaluate_expression succeeds we got the value we wanted. > > But if it fails, we still go on with a call to evaluate_type() */ > > if (!gdb_evaluate_expression (var->root->exp, &value)) > > > > So it seems to be varobj is not necessary defined in the current frame. > > When it's created it can be in a different frame according to the syntax. > However, in Emacs I only create watch expressions in the selected frame (the > manual suggests that "*" sets the value in the current frame but there > are many anomalies like this). Are you suggesting that GDB code should only work when used from Emacs? GDB, as it stands, allows you to create a variable object in any frame, and your patch does not work with that. So, either creating variable objects in any frame should be disallowed, or you patch improved. In fact, note that creating variable objects in specific frame is not necessary to trigger problem. If you create variable in current frame, then change frame, and then invoke varobj_editable_p, you'll evaluate expression in a different frame. I don't see anyting wrong about this use case. > Likewise I think variable assignment should only be done in the selected frame. Err, why? I might have varobj for global variable called like 'g'. When I assign value to that varobj, I don't really want any local variable called 'g' to get in. Of course, real program would have the variable named differently, but still. > Perhaps GDB should only allow this scenario, I don't know. I have used this > patch for several months now without a problem and I'd rather focus on real > use cases than hypothetical pathological ones. It's true that all new features should handle real use cases. However, it's also necessary that MI interface is consistent. In current MI, an ordinary (not "use selected frame") varobj uses the original frame for all operations -- for example -var-update. If any other operation starts to use selected frame, it's inconsistent. > > > > 4. I don't think your test actually tests that the 'editable' attribute > > > > comes out as 'false'. > > > > > > I'm not sure what to say. It shows that if you try to assign a value to a > > > cast GDB says "Variable object is not editable". > > > > > > The error message for a variable object of a cast used to be: > > > > > > &"mi_cmd_var_assign: Could not assign expression to variable object\n" > > > ^error,msg="mi_cmd_var_assign: Could not assign expression to variable object" > > > (gdb) > > > > Is this test failing with current GDB, and passing after your change? > > The "not editable" message is new for this case. > > > I think > > GDB will refuse to assign value to rvalue anyway -- at least here's what I > > get without your patch: > > > > (gdb) > > -var-create V * (float)argc > > ^done,name="V",numchild="0",value="0",type="float" > > (gdb) > > -var-assign V 10 > > &"mi_cmd_var_assign: Could not assign expression to variable object\n" > > ^error,msg="mi_cmd_var_assign: Could not assign expression to variable object" > > (gdb) > > That's what I've just said. So, in essence, the primary think you test is that this message now say "Variable object is not editable"? I personally think that the fact that -var-show-attributes reports rvalues as non-editable is equally important, but if you don't feel like writing a test for that, up to you -- in which case I'll probably submit a patch myself (and maybe even make the 'editable' field into regular varobj printing, as opposed to this 'attribute' thing). - Volodya