From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 21776 invoked by alias); 27 Oct 2007 12:15:47 -0000 Received: (qmail 21764 invoked by uid 22791); 27 Oct 2007 12:15:46 -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; Sat, 27 Oct 2007 12:15:43 +0000 Received: from Debian-exim by zigzag.lvk.cs.msu.su with spam-scanned (Exim 4.50) id 1IlkZj-0000zy-Fl for gdb-patches@sources.redhat.com; Sat, 27 Oct 2007 16:15:40 +0400 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 1IlkZZ-0000zj-I9; Sat, 27 Oct 2007 16:15:25 +0400 From: Vladimir Prus To: Nick Roberts Subject: Re: [PATCH] MI: lvalues and variable_editable Date: Sat, 27 Oct 2007 14:22:00 -0000 User-Agent: KMail/1.9.6 Cc: gdb-patches@sources.redhat.com References: <18210.27153.559569.601092@kahikatea.snap.net.nz> <18211.10636.539778.664602@kahikatea.snap.net.nz> In-Reply-To: <18211.10636.539778.664602@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: <200710271615.23579.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/msg00742.txt.bz2 On Saturday 27 October 2007 16:05:32 Nick Roberts wrote: > > I think the checking of lvalue-ness is a very good change. I have some > > comments, however: > > > > 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. > > 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. > > 2. In varobj_value_is_changeable_p, you have changed from returning 'r' at > > the end of function, to returning in several places. I don't think this > > change has any effect on logic and therefore, if committed, should be > > committed separately. And, I actually prefer the original code -- return in > > one place makes logic simpler. > > It used to be a bigger change. It's a consistent style with other functions > in varobj.c but maybe it's gratuitous. I don't mind leaving it out. Ok, good ;-) > > 3. I think your change to c_name_of_variable should be a separate patch. I > > also not sure it's right. Consider java_name_of_variable -- it calls > > cplus_name_of_variable and then does some quoting. With your change > > cplus_name_of_variable will return varobj->name, the the following code will > > directly modify it. Is it intended? > > Perhaps the right place for savestring, or better xsprintf, is in > java_name_of_variable. Alternatively when varobj_get_expression is called > in mi-cmd-var.c, it's value should be freed. I'll remove this change for now. Yeah, it's somewhat nasty, as for c varobj we don't need any processing and can just return varobj->name, but for java we need this dash-to-dot replacement. I was thinking that maybe we should have 'java_name' field in varobj -- but it would just waste space. Really, this all is just crying for C++. > > 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? 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) > I could add another test for -var-show-attributes which will now give: > > ^done,attr="noneditable" > > for a cast but I never use this command. I think the biggest value of this patch is that we get this attribute, *before* trying to assign the value, so yes, I think a test for this specific behaviour would be great. - Volodya