[RFC] varobj deletion after the binary has changed

[RFC] varobj deletion after the binary has changed

[Denis PILAT]

[-- Attachment #1: Type: text/plain, Size: 456 bytes --]

Hi,

We have a bug in one of our gdb target, when the binary changed while 
beeing debugged it appears that some of our varobj refers to invalid 
symbols or type.

I propose a patch that delete all varobj when symbols are reloaded. May 
be there is a better place to do that but I think we must do that 
somewhere, don't you ?

Daniel: There is no regression in the testsuite on linux native target 
with this patch :)

-- 
Denis Pilat
STMicroelectronics


[-- Attachment #2: varobj.patch --]
[-- Type: text/plain, Size: 1881 bytes --]

Index: symfile.c
===================================================================
RCS file: /cvs/src/src/gdb/symfile.c,v
retrieving revision 1.180
diff -u -p -r1.180 symfile.c
--- symfile.c	21 Jan 2007 01:02:03 -0000	1.180
+++ symfile.c	23 Jan 2007 10:00:36 -0000
@@ -52,6 +52,7 @@
 #include "observer.h"
 #include "exec.h"
 #include "parser-defs.h"
+#include "varobj.h"
 
 #include <sys/types.h>
 #include <fcntl.h>
@@ -2602,6 +2603,10 @@ clear_symtab_users (void)
      between expressions and which ought to be reset each time.  */
   expression_context_block = NULL;
   innermost_block = NULL;
+
+  /* Varobj may refer to symbols, delete all of them.  */
+  varobj_delete_all ();
+
 }
 
 static void
Index: varobj.c
===================================================================
RCS file: /cvs/src/src/gdb/varobj.c,v
retrieving revision 1.79
diff -u -p -r1.79 varobj.c
--- varobj.c	16 Jan 2007 02:12:49 -0000	1.79
+++ varobj.c	23 Jan 2007 10:00:46 -0000
@@ -2545,3 +2545,23 @@ When non-zero, varobj debugging is enabl
 			    show_varobjdebug,
 			    &setlist, &showlist);
 }
+
+int 
+varobj_delete_all (void)
+{
+  struct varobj **allvarobj;
+  struct varobj **var;
+  int nv;
+  nv = varobj_list (&allvarobj);
+  if (nv > 0)
+  {
+    var = allvarobj;
+    while (*var != NULL)
+      {
+        varobj_delete (*var, NULL, 0);        
+        var++;
+      }
+    xfree (allvarobj);
+  }
+  return nv;
+}
Index: varobj.h
===================================================================
RCS file: /cvs/src/src/gdb/varobj.h,v
retrieving revision 1.7
diff -u -p -r1.7 varobj.h
--- varobj.h	9 Jan 2007 17:58:59 -0000	1.7
+++ varobj.h	23 Jan 2007 10:00:46 -0000
@@ -99,4 +99,6 @@ extern int varobj_list (struct varobj **
 
 extern int varobj_update (struct varobj **varp, struct varobj ***changelist);
 
+extern int varobj_delete_all (void);
+
 #endif /* VAROBJ_H */

Re: [RFC] varobj deletion after the binary has changed

[Daniel Jacobowitz]

On Tue, Jan 23, 2007 at 01:32:22PM +0100, Denis PILAT wrote:
> Hi,
> 
> We have a bug in one of our gdb target, when the binary changed while 
> beeing debugged it appears that some of our varobj refers to invalid 
> symbols or type.
> 
> I propose a patch that delete all varobj when symbols are reloaded. May 
> be there is a better place to do that but I think we must do that 
> somewhere, don't you ?

The right thing to do is probably to figure out where the
invalid references come from and fix them - probably by re-evaluating
expressions at the next -var-update.  Deleting things behind the front
end's back is a bad policy.


-- 
Daniel Jacobowitz
CodeSourcery

Re: [RFC] varobj deletion after the binary has changed

[Vladimir Prus]

Daniel Jacobowitz wrote:

> On Tue, Jan 23, 2007 at 01:32:22PM +0100, Denis PILAT wrote:
>> Hi,
>> 
>> We have a bug in one of our gdb target, when the binary changed while
>> beeing debugged it appears that some of our varobj refers to invalid
>> symbols or type.

And what kind of problems does it cause, for the record? I'd expect that
attempt of evaluating such expressions will result in value being "",
and in_scope="false". Do you get anything worse than that?

>> 
>> I propose a patch that delete all varobj when symbols are reloaded. May
>> be there is a better place to do that but I think we must do that
>> somewhere, don't you ?
> 
> The right thing to do is probably to figure out where the
> invalid references come from and fix them - probably by re-evaluating
> expressions at the next -var-update.  Deleting things behind the front
> end's back is a bad policy.

Or maybe, implement "binary changed" MI notification that can be used
by the frontend as it sees fit.

- Volodya

Re: [RFC] varobj deletion after the binary has changed

[Denis PILAT]

Vladimir Prus wrote:
> And what kind of problems does it cause, for the record? I'd expect that
> attempt of evaluating such expressions will result in value being "",
> and in_scope="false". Do you get anything worse than that?
>   
Yes we are, gdb crashes.
In our case, when the -exec-run reloads the binary file, it stops at a 
breakpoint previously set and the frame_id is exactly the same as the 
one stored in the varobj object during the previous run (our SP does not 
change, under linux native targets, it is "randomized" I guess.). So 
when front-end executes -var-update, GDB thinks the varobj refers to a 
valid frame (but it's wrong since the program has changed) and try to 
access stuff he should not.

There is a way to reproduce it under native linux target:
Compile the following source file to get "a.out"
    #include <stdio.h>
    int a_global = 0;
    int main()
    {
      printf("hello");
      a_global++;
      return a_global;
    }

Run "gdb -i mi a.out" and the following command:
    -var-create varname * a_global
    shell touch a.out
    70-break-insert -t main
    71-exec-run
    75-var-update varname

==> you got it ! (Segmentation fault)

> Or maybe, implement "binary changed" MI notification that can be used
> by the frontend as it sees fit.
>   
That's a good option for the front end to make cleanup in its "watched" 
variable but it won't fix the current problem.

-- 
Denis

Re: [RFC] varobj deletion after the binary has changed

[Nick Roberts]

[-- Attachment #1: message body and .signature --]
[-- Type: text/plain, Size: 2320 bytes --]

 > > And what kind of problems does it cause, for the record? I'd expect that
 > > attempt of evaluating such expressions will result in value being "",
 > > and in_scope="false". Do you get anything worse than that?

It's a global variable so I don't think it will ever be reported as out
of scope.

 > Yes we are, gdb crashes.

This is a bug in GDB and how to deal with variable objects when restarting
should be a separate issue.  I see the same problem in a CVS snapshot of
6.2 vintage but not in GDB 6.3 from Fedora Core 5.  So unless the problem
disappeared in 6.3 and reasppeared later, which is unlikely, I think Fedora
must have solved it with one of their patches.  The problem is I don't know
which one.

I attach the SPEC file which lists the patches.  If anyone can give me an
educated guess as to which the relevant patch is, I'll try it.  Some of the
patches specify an architecture, but they may be more general than they
suggest.

In case it helps here's part of the stack at crash:

#0  0x081454e4 in check_typedef (type=0x95e1e6d) at gdbtypes.c:1402
#1  0x080f5769 in allocate_value (type=0x95e1e6d) at value.c:217
#2  0x080eca5d in read_var_value (var=0x95ee3f0, frame=0x0) at findvar.c:399
#3  0x080ffb3b in value_of_variable (var=0x95ee3f0, b=0x0) at valops.c:808
#4  0x080f88ef in evaluate_subexp_standard (expect_type=0x0, exp=0x95e9958, pos=0xbfdba084, noside=EVAL_NORMAL) at eval.c:480
#5  0x080f79f2 in evaluate_subexp (expect_type=0x0, exp=0x95e9958, pos=0xbfdba084, noside=EVAL_NORMAL) at eval.c:76
#6  0x080f7bc9 in evaluate_expression (exp=0x95e9958) at eval.c:166
#7  0x081a8613 in gdb_evaluate_expression (exp=0x95e9958, value=0xbfdba0e4) at wrapper.c:49
#8  0x081a7734 in c_value_of_root (var_handle=0xbfdba1e0) at varobj.c:2025
#9  0x081a6ede in value_of_root (var_handle=0xbfdba1e0, type_changed=0xbfdba17c) at varobj.c:1672
#10 0x081a6054 in varobj_update (varp=0xbfdba1e0, changelist=0xbfdba1c8) at varobj.c:1068

The crash occurs because computing TYPE_CODE (type) involves a memory
violation.  This comes from "type = SYMBOL_TYPE (var)" in read_var_value, in
turn from var = exp->elts[pc + 2].symbol which is from exp = var->root->exp of
the variable object in question (if that makes sense!).


-- 
Nick                                           http://www.inet.net.nz/~nickrob


[-- Attachment #2: SPEC file for FC5 GDB --]
[-- Type: application/octet-stream, Size: 18053 bytes --]

Re: [RFC] varobj deletion after the binary has changed

[Daniel Jacobowitz]

On Thu, Jan 25, 2007 at 10:49:01AM +1300, Nick Roberts wrote:
> The crash occurs because computing TYPE_CODE (type) involves a memory
> violation.  This comes from "type = SYMBOL_TYPE (var)" in read_var_value, in
> turn from var = exp->elts[pc + 2].symbol which is from exp = var->root->exp of
> the variable object in question (if that makes sense!).

Right, that's about what I expected.  A parsed exp has pointers into
the symbol table, which at this point has been flushed and reloaded.

For expressions not associated with a particular block, we ought to
reparse them from the original string if the symbol file changes. I'm
not sure what to do for things involving local variables; we need to
discard valid_block too, for the same reason.  Breakpoints have similar
issues, which are handled in breakpoint.c, but they don't usually carry
block pointers around.

The other things which could be invalidated when we reload the file
are the type pointers in values.  So, we probably need to discard all
values when reloading the file.

-- 
Daniel Jacobowitz
CodeSourcery

Re: [RFC] varobj deletion after the binary has changed

[Frédéric Riss]

Le jeudi 25 janvier 2007 à 10:49 +1300, Nick Roberts a écrit :
>  > > And what kind of problems does it cause, for the record? I'd expect that
>  > > attempt of evaluating such expressions will result in value being "",
>  > > and in_scope="false". Do you get anything worse than that?
> 
> It's a global variable so I don't think it will ever be reported as out
> of scope.
> 
>  > Yes we are, gdb crashes.
> 
> This is a bug in GDB and how to deal with variable objects when restarting
> should be a separate issue.  I see the same problem in a CVS snapshot of
> 6.2 vintage but not in GDB 6.3 from Fedora Core 5.  So unless the problem
> disappeared in 6.3 and reasppeared later, which is unlikely, I think Fedora
> must have solved it with one of their patches.  The problem is I don't know
> which one.

Just a shot in the dark: did you test with a memory checker like
valgrind? The fact that there's no crash doesn't mean that there's no
buggy memory access.

Fred.

Re: [RFC] varobj deletion after the binary has changed

[Nick Roberts]

 > Just a shot in the dark: did you test with a memory checker like
 > valgrind? The fact that there's no crash doesn't mean that there's no
 > buggy memory access.

The execution proceeds as normal and correctly reports when a_global changes.
If there was a buggy memory access I presume it would manifest itself in the
execution of GDB in some way.

-- 
Nick                                           http://www.inet.net.nz/~nickrob

Re: [RFC] varobj deletion after the binary has changed

[Frédéric Riss]

Le jeudi 25 janvier 2007 à 11:18 +1300, Nick Roberts a écrit :
>  > Just a shot in the dark: did you test with a memory checker like
>  > valgrind? The fact that there's no crash doesn't mean that there's no
>  > buggy memory access.
> 
> The execution proceeds as normal and correctly reports when a_global changes.
> If there was a buggy memory access I presume it would manifest itself in the
> execution of GDB in some way.

You might be true. I mentioned that because it's easy to check and there
seems to be no matching patch description in your spec file. Anyway
fixing that seems simple enough that we don't need to dig through 80
RedHat patches to come up with a patch. 

AFAIK Denis's working on a version re-evealuating (actually re-parsing)
varobjs with no attached blocks and putting the others in some error
state that'll return in_scope="false" at the next update. Does that seem
reasonable?

Fred.

Re: [RFC] varobj deletion after the binary has changed

[Nick Roberts]

 > You might be true. I mentioned that because it's easy to check and there
 > seems to be no matching patch description in your spec file. Anyway
 > fixing that seems simple enough that we don't need to dig through 80
 > RedHat patches to come up with a patch. 

It's not simple for me as I'm not familiar with that part (symbol tables)
of the code yet.

 > AFAIK Denis's working on a version re-evealuating (actually re-parsing)
 > varobjs with no attached blocks and putting the others in some error
 > state that'll return in_scope="false" at the next update. Does that seem
 > reasonable?

In FC5 GDB there doesn't seem to be a problem with varobjs with no attached
blocks i.e global variables, and those which do have attached blocks i.e locals
do currently return in_scope="false", on GNU/Linux at least (maybe it has
something to do with randomisation though).  Like Dennis, I think there should
be some provision for deleting the latter on restarting.  With watchpoints this
happens automatically.

-- 
Nick                                           http://www.inet.net.nz/~nickrob

Re: [RFC] varobj deletion after the binary has changed

[Daniel Jacobowitz]

On Thu, Jan 25, 2007 at 11:18:26AM +1300, Nick Roberts wrote:
> The execution proceeds as normal and correctly reports when a_global changes.
> If there was a buggy memory access I presume it would manifest itself in the
> execution of GDB in some way.

No, that's a bad assumption to make about invalid memory access.  It's
often impossible to detect it without some luck or a tool like
valgrind.

-- 
Daniel Jacobowitz
CodeSourcery

Re: [RFC] varobj deletion after the binary has changed

[Nick Roberts]

 > > If there was a buggy memory access I presume it would manifest itself in
 > > the execution of GDB in some way.
 > 
 > No, that's a bad assumption to make about invalid memory access.  It's
 > often impossible to detect it without some luck or a tool like
 > valgrind.

It even identifies the variable object as int:

-var-info-type var1
^done,type="int"
(gdb)

which would seem a remarkable coincidence if it was looking at the wrong
address.  I'm not sure what happens on rereading, maybe space gets malloced
for the symbol table, freed and then malloced again.  Perhaps with FC5 it's
just fortunate that the symbol table is allocated exactly as before.  In any
case, it can segfault if you edit the source file and recompile before
(re)starting execution, so some change is necessary.

-- 
Nick                                           http://www.inet.net.nz/~nickrob

Re: [RFC] varobj deletion after the binary has changed

[Denis PILAT]

Nick Roberts wrote:
>   In any
> case, it can segfault if you edit the source file and recompile before
> (re)starting execution, so some change is necessary.
>
>   
It can segfault just by using the command "file" on an other binary file 
before starting the execution, and that is a more common feature if we 
consider that gdb must not be killed and restarted to debug a new program.

-- 
Denis

Re: [RFC] varobj deletion after the binary has changed

[Denis PILAT]

Daniel Jacobowitz wrote:
> On Tue, Jan 23, 2007 at 01:32:22PM +0100, Denis PILAT wrote:
>   
>> Hi,
>>
>> We have a bug in one of our gdb target, when the binary changed while 
>> beeing debugged it appears that some of our varobj refers to invalid 
>> symbols or type.
>>
>> I propose a patch that delete all varobj when symbols are reloaded. May 
>> be there is a better place to do that but I think we must do that 
>> somewhere, don't you ?
>>     
>
> The right thing to do is probably to figure out where the
> invalid references come from and fix them - probably by re-evaluating
> expressions at the next -var-update.  Deleting things behind the front
> end's back is a bad policy.
>
>
>   
Deleting all is a little bit radical, I can understand that.
Re-evaluating expression would give the expected thing for globale 
variables, but for varobj that has been set on local variable (so within 
a "valid_block"), it should have side effects: the evaluation of the 
expression might tied the varobj to a variable different than the one 
the user originally decided.

I propose to distinguish 2 cases
- varobj on global variable (valid_block == null) must have their 
expression re-evaluated.
- other one must be put in a kind of invalid state that will gives 
"in_scope="false" to the user.
This last point will involve a new field in varobj structure, I haven't 
seen something to mark varobj as invalid.

Denis

Re: [RFC] varobj deletion after the binary has changed

[Denis PILAT]

[-- Attachment #1: Type: text/plain, Size: 2291 bytes --]

Denis PILAT wrote:
> Daniel Jacobowitz wrote:
>> On Tue, Jan 23, 2007 at 01:32:22PM +0100, Denis PILAT wrote:
>>  
>>> Hi,
>>>
>>> We have a bug in one of our gdb target, when the binary changed 
>>> while beeing debugged it appears that some of our varobj refers to 
>>> invalid symbols or type.
>>>
>>> I propose a patch that delete all varobj when symbols are reloaded. 
>>> May be there is a better place to do that but I think we must do 
>>> that somewhere, don't you ?
>>>     
>>
>> The right thing to do is probably to figure out where the
>> invalid references come from and fix them - probably by re-evaluating
>> expressions at the next -var-update.  Deleting things behind the front
>> end's back is a bad policy.
>>
>>
>>   
> Deleting all is a little bit radical, I can understand that.
> Re-evaluating expression would give the expected thing for globale 
> variables, but for varobj that has been set on local variable (so 
> within a "valid_block"), it should have side effects: the evaluation 
> of the expression might tied the varobj to a variable different than 
> the one the user originally decided.
>
> I propose to distinguish 2 cases
> - varobj on global variable (valid_block == null) must have their 
> expression re-evaluated.
> - other one must be put in a kind of invalid state that will gives 
> "in_scope="false" to the user.
> This last point will involve a new field in varobj structure, I 
> haven't seen something to mark varobj as invalid.
>
> Denis
>
Hi

Attached is a new patch proposal when I implemented what I explained above.

During the symbol re-loading I invalidate varobjs linked to locals 
variable and try to re-evaluate the expression for globals.
No more varobjs are deleted, and during varobj_update invalidated 
symbols are skipped so displayed as not in scope.

If you're fine with this implementation I'll write 2 more test cases in 
the testsuite that lead to segmentation fault in the current GDB:
- one where the binary file completely changed so any varobjs defined on 
global variable are invalidated.
- one where the binary file is just updated so varobjs on globals can 
still be evaluated.

I check the testsuite with i386-linux target with no regression and I 
passed a valgrind as well.

-- 
Denis PILAT
STMicroelectronics


[-- Attachment #2: varobj.patch --]
[-- Type: text/plain, Size: 3292 bytes --]

Index: symfile.c
===================================================================
--- symfile.c	(revision 552)
+++ symfile.c	(working copy)
@@ -52,6 +52,7 @@
 #include "observer.h"
 #include "exec.h"
 #include "parser-defs.h"
+#include "varobj.h"
 
 #include <sys/types.h>
 #include <fcntl.h>
@@ -2585,6 +2586,10 @@ clear_symtab_users (void)
      between expressions and which ought to be reset each time.  */
   expression_context_block = NULL;
   innermost_block = NULL;
+
+  /* Varobj may refer to old symbols, perform a cleanup.  */
+  varobj_invalidate ();
+
 }
 
 static void
Index: varobj.c
===================================================================
--- varobj.c	(revision 552)
+++ varobj.c	(working copy)
@@ -77,6 +77,10 @@ struct varobj_root
 
   /* Next root variable */
   struct varobj_root *next;
+
+  /* Flag that indicates validity: set to 0 when this varobj_root refers 
+     to symbols that do not exist anymore.  */
+  int is_valid;
 };
 
 /* Every variable in the system has a structure of this type defined
@@ -912,6 +916,9 @@ varobj_update (struct varobj **varp, str
     /* Not a root var */
     return -1;
 
+  if (!(*varp)->root->is_valid)
+    return -1;
+
   /* Save the selected stack frame, since we will need to change it
      in order to evaluate expressions. */
   old_fid = get_frame_id (deprecated_selected_frame);
@@ -1361,6 +1368,7 @@ new_root_variable (void)
   var->root->frame = null_frame_id;
   var->root->use_selected_frame = 0;
   var->root->rootvar = NULL;
+  var->root->is_valid = 1;
 
   return var;
 }
@@ -2569,3 +2577,45 @@ When non-zero, varobj debugging is enabl
 			    show_varobjdebug,
 			    &setlist, &showlist);
 }
+
+/* Invalidate the varobjs that are tied to locals and re-create the ones that
+   are defined on globals.
+   Invalidated varobjs will be always printed in_scope="false".  */
+void 
+varobj_invalidate (void)
+{
+  struct varobj **all_rootvarobj;
+  struct varobj **varp;
+
+  if (varobj_list (&all_rootvarobj) > 0)
+  {
+    varp = all_rootvarobj;
+    while (*varp != NULL)
+      {
+        /* global var must be re-evaluated.  */     
+        if ((*varp)->root->valid_block == NULL)
+        {
+          struct varobj *tmp_var;
+
+          /* try to create a varobj with same expression. if success we replace
+             the old varobj, othewize we invalidate it.  */      
+          tmp_var = varobj_create (NULL, (*varp)->name, (CORE_ADDR) 0, USE_CURRENT_FRAME);
+          if (tmp_var != NULL) 
+            { 
+	      tmp_var->obj_name =
+		savestring ((*varp)->obj_name, strlen ((*varp)->obj_name));
+              varobj_delete (*varp, NULL, 0);
+              install_variable (tmp_var);
+            }
+          else
+              (*varp)->root->is_valid = 0;
+        }
+        else /* locals must be invalidated.  */
+          (*varp)->root->is_valid = 0;
+
+        varp++;
+      }
+    xfree (all_rootvarobj);
+  }
+  return;
+}
Index: varobj.h
===================================================================
--- varobj.h	(revision 552)
+++ varobj.h	(working copy)
@@ -99,4 +99,6 @@ extern int varobj_list (struct varobj **
 
 extern int varobj_update (struct varobj **varp, struct varobj ***changelist);
 
+extern void varobj_invalidate (void);
+
 #endif /* VAROBJ_H */

Re: [RFC] varobj deletion after the binary has changed

[Nick Roberts]

To avoid confusion these are my opinions; I don't have authority to approve
the patch.

 > --- varobj.c	(revision 552)
 > +++ varobj.c	(working copy)
 > @@ -77,6 +77,10 @@ struct varobj_root
 >  
 >    /* Next root variable */
 >    struct varobj_root *next;
 > +
 > +  /* Flag that indicates validity: set to 0 when this varobj_root refers 
 > +     to symbols that do not exist anymore.  */
 > +  int is_valid;
 >  };

I would move the is_valid member before *next.

 >  /* Every variable in the system has a structure of this type defined
 > @@ -912,6 +916,9 @@ varobj_update (struct varobj **varp, str
 >      /* Not a root var */
 >      return -1;
 >  
 > +  if (!(*varp)->root->is_valid)
 > +    return -1;

This seems to make the variable object some kind of zombie.  Wouldn't it
be better to make GDB report something like:

^done,changelist=[{name="var1",in_scope="invalid",type_changed="false"}]

so then the frontend can choose to delete these variable objects?

 >...
 > +void 
 > +varobj_invalidate (void)
 > +{
 > +  struct varobj **all_rootvarobj;
 > +  struct varobj **varp;
 > +
 > +  if (varobj_list (&all_rootvarobj) > 0)
 > +  {
 > +    varp = all_rootvarobj;
 > +    while (*varp != NULL)
 > +      {
 > +        /* global var must be re-evaluated.  */     
 > +        if ((*varp)->root->valid_block == NULL)
 > +        {
 > +          struct varobj *tmp_var;
 > +
 > +          /* try to create a varobj with same expression. if success we replace
 > +             the old varobj, othewize we invalidate it.  */      

              /* Try to create a varobj with same expression.  If we succeed replace
                 the old varobj, otherwise invalidate it.  */      



 > +          tmp_var = varobj_create (NULL, (*varp)->name, (CORE_ADDR) 0, USE_CURRENT_FRAME);
 > +          if (tmp_var != NULL) 
 > +            { 
 > +	      tmp_var->obj_name =
 > +		savestring ((*varp)->obj_name, strlen ((*varp)->obj_name));

Is xstrdup just a simpler version of savestring? i.e.
   
              tmp_var->obj_name = xstrdup (*varp)->obj_name);


 > +              varobj_delete (*varp, NULL, 0);
 > +              install_variable (tmp_var);
 > +            }
 > +          else
 > +              (*varp)->root->is_valid = 0;
 > +        }
 > +        else /* locals must be invalidated.  */
 > +          (*varp)->root->is_valid = 0;
 > +
 > +        varp++;
 > +      }
 > +    xfree (all_rootvarobj);
 > +  }
 > +  return;
 > +}
 >...

-- 
Nick                                           http://www.inet.net.nz/~nickrob

Re: [RFC] varobj deletion after the binary has changed

[Daniel Jacobowitz]

I haven't looked at the patch yet, just Nick's comments, which I
generally agree with :-)

On Fri, Jan 26, 2007 at 11:31:35AM +1300, Nick Roberts wrote:
> Is xstrdup just a simpler version of savestring? i.e.
>    
>               tmp_var->obj_name = xstrdup (*varp)->obj_name);

Yes - savestring is handy for saving only part of a string but xstrdup
is clearer.

-- 
Daniel Jacobowitz
CodeSourcery

Re: [RFC] varobj deletion after the binary has changed

[Denis PILAT]

[-- Attachment #1: Type: text/plain, Size: 1057 bytes --]

Daniel Jacobowitz wrote:
> I haven't looked at the patch yet, just Nick's comments, which I
> generally agree with :-)
>
> On Fri, Jan 26, 2007 at 11:31:35AM +1300, Nick Roberts wrote:
>   
>> Is xstrdup just a simpler version of savestring? i.e.
>>    
>>               tmp_var->obj_name = xstrdup (*varp)->obj_name);
>>     
>
> Yes - savestring is handy for saving only part of a string but xstrdup
> is clearer.
>
>   
Attached is a new implementation, it takes care of more cases where seg 
fault occured.

I take Nick's suggestion into account, -var-update outputs the following 
for invalid variables:
    changelist=[{name="linteger",in_scope="invalid"}]

About this last change, I'm wondering if frond-ends that expect "true" 
or "false" will handle this new outputs correctly, it's just a question 
of backward compatibility. Of course it will be better that the old 
behavior that crashes gdb !
If you're fine I'll write appropriate ChangeLogs.

I also wrote a new .exp file in gdb.mi testsuite, I'll propose it on 
this one approval.

-- 
Denis

[-- Attachment #2: varobj.patch2 --]
[-- Type: text/plain, Size: 5752 bytes --]

Index: symfile.c
===================================================================
RCS file: /cvs/src/src/gdb/symfile.c,v
retrieving revision 1.180
diff -u -p -r1.180 symfile.c
--- symfile.c	21 Jan 2007 01:02:03 -0000	1.180
+++ symfile.c	29 Jan 2007 12:09:46 -0000
@@ -52,6 +52,7 @@
 #include "observer.h"
 #include "exec.h"
 #include "parser-defs.h"
+#include "varobj.h"
 
 #include <sys/types.h>
 #include <fcntl.h>
@@ -2602,6 +2603,10 @@ clear_symtab_users (void)
      between expressions and which ought to be reset each time.  */
   expression_context_block = NULL;
   innermost_block = NULL;
+
+  /* Varobj may refer to old symbols, perform a cleanup.  */
+  varobj_invalidate ();
+
 }
 
 static void
Index: varobj.c
===================================================================
RCS file: /cvs/src/src/gdb/varobj.c,v
retrieving revision 1.82
diff -u -p -r1.82 varobj.c
--- varobj.c	24 Jan 2007 19:54:13 -0000	1.82
+++ varobj.c	29 Jan 2007 12:09:55 -0000
@@ -71,6 +71,10 @@ struct varobj_root
      using the currently selected frame. */
   int use_selected_frame;
 
+  /* Flag that indicates validity: set to 0 when this varobj_root refers 
+     to symbols that do not exist anymore.  */
+  int is_valid;
+
   /* Language info for this variable and its children */
   struct language_specific *lang;
 
@@ -742,8 +746,9 @@ varobj_get_type (struct varobj *var)
   long length;
 
   /* For the "fake" variables, do not return a type. (It's type is
-     NULL, too.) */
-  if (CPLUS_FAKE_CHILD (var))
+     NULL, too.)
+     Do not return a type for invalid variables as well.  */
+  if (CPLUS_FAKE_CHILD (var) || !var->root->is_valid)
     return NULL;
 
   stb = mem_fileopen ();
@@ -778,7 +783,7 @@ varobj_get_attributes (struct varobj *va
 {
   int attributes = 0;
 
-  if (variable_editable (var))
+  if (var->root->is_valid && variable_editable (var))
     /* FIXME: define masks for attributes */
     attributes |= 0x00000001;	/* Editable */
 
@@ -1021,6 +1026,7 @@ install_new_value (struct varobj *var, s
    Return value:
     -1 if there was an error updating the varobj
     -2 if the type changed
+    -3 if the varobj root is invalid
     Otherwise it is the number of children + parent changed
 
    Only root variables can be updated... 
@@ -1055,6 +1061,9 @@ varobj_update (struct varobj **varp, str
     /* Not a root var */
     return -1;
 
+  if (!(*varp)->root->is_valid)
+    return -3;
+
   /* Save the selected stack frame, since we will need to change it
      in order to evaluate expressions. */
   old_fid = get_frame_id (deprecated_selected_frame);
@@ -1409,6 +1418,7 @@ new_root_variable (void)
   var->root->frame = null_frame_id;
   var->root->use_selected_frame = 0;
   var->root->rootvar = NULL;
+  var->root->is_valid = 1;
 
   return var;
 }
@@ -1692,7 +1702,10 @@ variable_editable (struct varobj *var)
 static char *
 my_value_of_variable (struct varobj *var)
 {
-  return (*var->root->lang->value_of_variable) (var);
+  if (var->root->is_valid)
+    return (*var->root->lang->value_of_variable) (var);
+  else
+    return NULL;
 }
 
 static char *
@@ -2504,3 +2517,44 @@ When non-zero, varobj debugging is enabl
 			    show_varobjdebug,
 			    &setlist, &showlist);
 }
+
+/* Invalidate the varobjs that are tied to locals and re-create the ones that
+   are defined on globals.
+   Invalidated varobjs will be always printed in_scope="false".  */
+void 
+varobj_invalidate (void)
+{
+  struct varobj **all_rootvarobj;
+  struct varobj **varp;
+
+  if (varobj_list (&all_rootvarobj) > 0)
+  {
+    varp = all_rootvarobj;
+    while (*varp != NULL)
+      {
+        /* global var must be re-evaluated.  */     
+        if ((*varp)->root->valid_block == NULL)
+        {
+          struct varobj *tmp_var;
+
+          /* Try to create a varobj with same expression.  If we succeed replace
+             the old varobj, otherwise invalidate it.  */
+          tmp_var = varobj_create (NULL, (*varp)->name, (CORE_ADDR) 0, USE_CURRENT_FRAME);
+          if (tmp_var != NULL) 
+            { 
+	      tmp_var->obj_name = xstrdup ((*varp)->obj_name);
+              varobj_delete (*varp, NULL, 0);
+              install_variable (tmp_var);
+            }
+          else
+              (*varp)->root->is_valid = 0;
+        }
+        else /* locals must be invalidated.  */
+          (*varp)->root->is_valid = 0;
+
+        varp++;
+      }
+    xfree (all_rootvarobj);
+  }
+  return;
+}
Index: varobj.h
===================================================================
RCS file: /cvs/src/src/gdb/varobj.h,v
retrieving revision 1.7
diff -u -p -r1.7 varobj.h
--- varobj.h	9 Jan 2007 17:58:59 -0000	1.7
+++ varobj.h	29 Jan 2007 12:09:56 -0000
@@ -99,4 +99,6 @@ extern int varobj_list (struct varobj **
 
 extern int varobj_update (struct varobj **varp, struct varobj ***changelist);
 
+extern void varobj_invalidate (void);
+
 #endif /* VAROBJ_H */
Index: mi/mi-cmd-var.c
===================================================================
RCS file: /cvs/src/src/gdb/mi/mi-cmd-var.c,v
retrieving revision 1.28
diff -u -p -r1.28 mi-cmd-var.c
--- mi/mi-cmd-var.c	9 Jan 2007 17:59:08 -0000	1.28
+++ mi/mi-cmd-var.c	29 Jan 2007 12:09:58 -0000
@@ -555,12 +555,12 @@ varobj_update_one (struct varobj *var, e
   
   if (nc == 0)
     return 1;
-  else if (nc == -1)
+  else if (nc == -1 || nc == -3)
     {
       if (mi_version (uiout) > 1)
         cleanup = make_cleanup_ui_out_tuple_begin_end (uiout, NULL);
       ui_out_field_string (uiout, "name", varobj_get_objname(var));
-      ui_out_field_string (uiout, "in_scope", "false");
+      ui_out_field_string (uiout, "in_scope", (nc == -3 ? "invalid" : "false"));
       if (mi_version (uiout) > 1)
         do_cleanups (cleanup);
       return -1;

Re: [RFC] varobj deletion after the binary has changed

[Nick Roberts]

 > Attached is a new implementation, it takes care of more cases where seg 
 > fault occured.
 > 
 > I take Nick's suggestion into account, -var-update outputs the following 
 > for invalid variables:
 >     changelist=[{name="linteger",in_scope="invalid"}]
 > 
 > About this last change, I'm wondering if frond-ends that expect "true" 
 > or "false" will handle this new outputs correctly, it's just a question 
 > of backward compatibility. Of course it will be better that the old 
 > behavior that crashes gdb !

The documentation doesn't say that "true" and "false" are exhaustive
possibilities but it may be reasonable to expect that they are.  It's only
needed when the inferior is rebuilt with extant variable objects.  I've never
found a problem in my use, although it clearly depends on pattern of use.
In any case, as you say, if we make no change gdb will just crash.

If Daniel approves the patch, I will submit some documentation as we are
considering other values for in_scope such as "exited".  That way frontend
developers can factor in the possobility of further values.

 > If you're fine I'll write appropriate ChangeLogs.
 > 
 > I also wrote a new .exp file in gdb.mi testsuite, I'll propose it on 
 > this one approval.
 
Looks good.  Maybe varobj_update could use an enum:

enum varobj_update_values
  {
    SCOPE_FALSE = -1,
    TYPE_CHANGED,
    SCOPE_INVALID
  }

 >...
 > Index: mi/mi-cmd-var.c
 > ===================================================================
 > RCS file: /cvs/src/src/gdb/mi/mi-cmd-var.c,v
 > retrieving revision 1.28
 > diff -u -p -r1.28 mi-cmd-var.c
 > --- mi/mi-cmd-var.c	9 Jan 2007 17:59:08 -0000	1.28
 > +++ mi/mi-cmd-var.c	29 Jan 2007 12:09:58 -0000
 > @@ -555,12 +555,12 @@ varobj_update_one (struct varobj *var, e
 >    
 >    if (nc == 0)
 >      return 1;
 > -  else if (nc == -1)
 > +  else if (nc == -1 || nc == -3)
 >      {
 >        if (mi_version (uiout) > 1)
 >          cleanup = make_cleanup_ui_out_tuple_begin_end (uiout, NULL);
 >        ui_out_field_string (uiout, "name", varobj_get_objname(var));
 > -      ui_out_field_string (uiout, "in_scope", "false");
 > +      ui_out_field_string (uiout, "in_scope", (nc == -3 ? "invalid" : "false"));
 >        if (mi_version (uiout) > 1)
 >          do_cleanups (cleanup);
 >        return -1;


Including the case nc == -2, maybe this reads more easily (just thinking out
aloud):

  else if (nc < 0)
    {
      if (mi_version (uiout) > 1)
        cleanup = make_cleanup_ui_out_tuple_begin_end (uiout, NULL);
      ui_out_field_string (uiout, "name", varobj_get_objname(var));
      switch (nc)
      {
        case (SCOPE_FALSE)
          ui_out_field_string (uiout, "in_scope", "false");
	  break;
        case (TYPE_CHANGED)
	  ui_out_field_string (uiout, "in_scope", "true");
          ui_out_field_string (uiout, "new_type", varobj_get_type(var));
          ui_out_field_int (uiout, "new_num_children", 
			    varobj_get_num_children(var));
	  break;
        case (SCOPE_invalid)
          ui_out_field_string (uiout, "in_scope", "invalid");
 	  break;
      }
      if (mi_version (uiout) > 1)
        do_cleanups (cleanup);
    }

(It looks like we could remove the return value of varobj_update_one as it
doesn't seem to be used.)


-- 
Nick                                           http://www.inet.net.nz/~nickrob

Re: [RFC] varobj deletion after the binary has changed

[Denis PILAT]

> The documentation doesn't say that "true" and "false" are exhaustive
> possibilities but it may be reasonable to expect that they are.  It's only
> needed when the inferior is rebuilt with extant variable objects.  I've never
> found a problem in my use, although it clearly depends on pattern of use.
> In any case, as you say, if we make no change gdb will just crash.
>
>   
For information our Eclipse based front-end is not lost anymore when the 
user decide to re-compile the sources while debugging. And we haven't 
dealt yet with variables marked as invalid, it's just standard Eclipse.
-- 
Denis

Re: [RFC] varobj deletion after the binary has changed

[Denis PILAT]

[-- Attachment #1: Type: text/plain, Size: 599 bytes --]

Nick Roberts wrote:
>  
> Looks good.  Maybe varobj_update could use an enum:
>
> enum varobj_update_values
>   {
>     SCOPE_FALSE = -1,
>     TYPE_CHANGED,
>     SCOPE_INVALID
>   }
>   
Yes, it's better and more readable.
As you'll see in my patch, the varobj_update function could return more 
than these case of error, it's the caller that decides how to deal with 
these errors.
>
> (It looks like we could remove the return value of varobj_update_one as it
> doesn't seem to be used.)
>   
You're right, I removed it as well.

Attached is the new implementation plus the new exp file.

Denis

[-- Attachment #2: varobj.patch --]
[-- Type: text/plain, Size: 10307 bytes --]

2007-01-31  Denis Pilat  <denis.pilat@st.com>

	* varobj.c (struct varobj_root): Add is_valid member.
	(varobj_get_type): Check for invalid varobj.
	(varobj_get_attributes): Likewise.
	(variable_editable):Likewise.
	(varobj_update): Likewise plus use an enum for returned error values.
	(new_root_variable): Set root varobj as valid by default.
	(varobj_invalidate): New function.
	* varobj.h (enum varobj_update_error): New enum.
	(varobj_invalidate): New function.
	* symfile.c (clear_symtab_users): Use varobj_invalidate.
	* mi/mi-cmd-var.c (varobj_update_one): Change returned type to void and
	use of new enum varobj_update_error to deal with errors.


Index: varobj.c
===================================================================
--- varobj.c	(revision 553)
+++ varobj.c	(working copy)
@@ -71,6 +71,10 @@ struct varobj_root
      using the currently selected frame. */
   int use_selected_frame;
 
+  /* Flag that indicates validity: set to 0 when this varobj_root refers 
+     to symbols that do not exist anymore.  */
+  int is_valid;
+
   /* Language info for this variable and its children */
   struct language_specific *lang;
 
@@ -742,8 +746,9 @@ varobj_get_type (struct varobj *var)
   long length;
 
   /* For the "fake" variables, do not return a type. (It's type is
-     NULL, too.) */
-  if (CPLUS_FAKE_CHILD (var))
+     NULL, too.)
+     Do not return a type for invalid variables as well.  */
+  if (CPLUS_FAKE_CHILD (var) || !var->root->is_valid)
     return NULL;
 
   stb = mem_fileopen ();
@@ -778,7 +783,7 @@ varobj_get_attributes (struct varobj *va
 {
   int attributes = 0;
 
-  if (variable_editable (var))
+  if (var->root->is_valid && variable_editable (var))
     /* FIXME: define masks for attributes */
     attributes |= 0x00000001;	/* Editable */
 
@@ -1018,16 +1023,15 @@ install_new_value (struct varobj *var, s
    expression to see if it's changed.  Then go all the way
    through its children, reconstructing them and noting if they've
    changed.
-   Return value:
-    -1 if there was an error updating the varobj
-    -2 if the type changed
-    Otherwise it is the number of children + parent changed
+   Return value: 
+    < 0 for error values, see varobj.h.
+    Otherwise it is the number of children + parent changed.
 
    Only root variables can be updated... 
 
    NOTE: This function may delete the caller's varobj. If it
-   returns -2, then it has done this and VARP will be modified
-   to point to the new varobj. */
+   returns TYPE_CHANGED, then it has done this and VARP will be modified
+   to point to the new varobj.  */
 
 int
 varobj_update (struct varobj **varp, struct varobj ***changelist)
@@ -1048,12 +1052,15 @@ varobj_update (struct varobj **varp, str
 
   /* sanity check: have we been passed a pointer? */
   if (changelist == NULL)
-    return -1;
+    return WRONG_PARAM;
 
   /*  Only root variables can be updated... */
   if (!is_root_p (*varp))
     /* Not a root var */
-    return -1;
+    return WRONG_PARAM;
+
+  if (!(*varp)->root->is_valid)
+    return INVALID;
 
   /* Save the selected stack frame, since we will need to change it
      in order to evaluate expressions. */
@@ -1090,7 +1097,7 @@ varobj_update (struct varobj **varp, str
       /* This means the varobj itself is out of scope.
 	 Report it.  */
       VEC_free (varobj_p, result);
-      return -1;
+      return NOT_IN_SCOPE;
     }
 
   VEC_safe_push (varobj_p, stack, *varp);
@@ -1140,7 +1147,7 @@ varobj_update (struct varobj **varp, str
   *cv = 0;
 
   if (type_changed)
-    return -2;
+    return TYPE_CHANGED;
   else
     return changed;
 }
@@ -1409,6 +1416,7 @@ new_root_variable (void)
   var->root->frame = null_frame_id;
   var->root->use_selected_frame = 0;
   var->root->rootvar = NULL;
+  var->root->is_valid = 1;
 
   return var;
 }
@@ -1692,7 +1700,10 @@ variable_editable (struct varobj *var)
 static char *
 my_value_of_variable (struct varobj *var)
 {
-  return (*var->root->lang->value_of_variable) (var);
+  if (var->root->is_valid)
+    return (*var->root->lang->value_of_variable) (var);
+  else
+    return NULL;
 }
 
 static char *
@@ -2504,3 +2515,44 @@ When non-zero, varobj debugging is enabl
 			    show_varobjdebug,
 			    &setlist, &showlist);
 }
+
+/* Invalidate the varobjs that are tied to locals and re-create the ones that
+   are defined on globals.
+   Invalidated varobjs will be always printed in_scope="false".  */
+void 
+varobj_invalidate (void)
+{
+  struct varobj **all_rootvarobj;
+  struct varobj **varp;
+
+  if (varobj_list (&all_rootvarobj) > 0)
+  {
+    varp = all_rootvarobj;
+    while (*varp != NULL)
+      {
+        /* global var must be re-evaluated.  */     
+        if ((*varp)->root->valid_block == NULL)
+        {
+          struct varobj *tmp_var;
+
+          /* Try to create a varobj with same expression.  If we succeed replace
+             the old varobj, otherwise invalidate it.  */
+          tmp_var = varobj_create (NULL, (*varp)->name, (CORE_ADDR) 0, USE_CURRENT_FRAME);
+          if (tmp_var != NULL) 
+            { 
+	      tmp_var->obj_name = xstrdup ((*varp)->obj_name);
+              varobj_delete (*varp, NULL, 0);
+              install_variable (tmp_var);
+            }
+          else
+              (*varp)->root->is_valid = 0;
+        }
+        else /* locals must be invalidated.  */
+          (*varp)->root->is_valid = 0;
+
+        varp++;
+      }
+    xfree (all_rootvarobj);
+  }
+  return;
+}
Index: varobj.h
===================================================================
--- varobj.h	(revision 553)
+++ varobj.h	(working copy)
@@ -38,7 +38,16 @@ enum varobj_type
     USE_CURRENT_FRAME,          /* Use the current frame */
     USE_SELECTED_FRAME          /* Always reevaluate in selected frame */
   };
-    
+
+/* Error return values for varobj_update function.  */
+enum varobj_update_error
+  {
+    NOT_IN_SCOPE = -1,          /* varobj not in scope, can not be updated.  */
+    TYPE_CHANGED = -2,          /* varobj type has changed.  */
+    INVALID = -3,               /* varobj is not valid anymore.  */
+    WRONG_PARAM = -4            /* function is called with wrong arguments.  */
+  };
+
 /* String representations of gdb's format codes (defined in varobj.c) */
 extern char *varobj_format_string[];
 
@@ -99,4 +108,6 @@ extern int varobj_list (struct varobj **
 
 extern int varobj_update (struct varobj **varp, struct varobj ***changelist);
 
+extern void varobj_invalidate (void);
+
 #endif /* VAROBJ_H */
Index: symfile.c
===================================================================
--- symfile.c	(revision 553)
+++ symfile.c	(working copy)
@@ -52,6 +52,7 @@
 #include "observer.h"
 #include "exec.h"
 #include "parser-defs.h"
+#include "varobj.h"
 
 #include <sys/types.h>
 #include <fcntl.h>
@@ -2602,6 +2603,10 @@ clear_symtab_users (void)
      between expressions and which ought to be reset each time.  */
   expression_context_block = NULL;
   innermost_block = NULL;
+
+  /* Varobj may refer to old symbols, perform a cleanup.  */
+  varobj_invalidate ();
+
 }
 
 static void
Index: mi/mi-cmd-var.c
===================================================================
--- mi/mi-cmd-var.c	(revision 553)
+++ mi/mi-cmd-var.c	(working copy)
@@ -34,9 +34,9 @@ const char mi_no_values[] = "--no-values
 const char mi_simple_values[] = "--simple-values";
 const char mi_all_values[] = "--all-values";
 
-extern int varobjdebug;		/* defined in varobj.c */
+extern int varobjdebug;		/* defined in varobj.c.  */
 
-static int varobj_update_one (struct varobj *var,
+static void varobj_update_one (struct varobj *var,
 			      enum print_values print_values);
 
 static int mi_print_value_p (struct type *type, enum print_values print_values);
@@ -535,11 +535,9 @@ mi_cmd_var_update (char *command, char *
     return MI_CMD_DONE;
 }
 
-/* Helper for mi_cmd_var_update() Returns 0 if the update for
-   the variable fails (usually because the variable is out of
-   scope), and 1 if it succeeds. */
+/* Helper for mi_cmd_var_update().  */
 
-static int
+static void
 varobj_update_one (struct varobj *var, enum print_values print_values)
 {
   struct varobj **changelist;
@@ -549,37 +547,39 @@ varobj_update_one (struct varobj *var, e
 
   nc = varobj_update (&var, &changelist);
 
-  /* nc == 0 means that nothing has changed.
-     nc == -1 means that an error occured in updating the variable.
-     nc == -2 means the variable has changed type. */
+  /* nc >= 0  represents the number of changes reported into changelist.
+     nc < 0   means that an error occured or the the variable has 
+              changed type (TYPE_CHANGED).  */
   
   if (nc == 0)
-    return 1;
-  else if (nc == -1)
+    return;
+  else if (nc < 0)
     {
       if (mi_version (uiout) > 1)
         cleanup = make_cleanup_ui_out_tuple_begin_end (uiout, NULL);
       ui_out_field_string (uiout, "name", varobj_get_objname(var));
-      ui_out_field_string (uiout, "in_scope", "false");
-      if (mi_version (uiout) > 1)
-        do_cleanups (cleanup);
-      return -1;
-    }
-  else if (nc == -2)
-    {
-      if (mi_version (uiout) > 1)
-        cleanup = make_cleanup_ui_out_tuple_begin_end (uiout, NULL);
-      ui_out_field_string (uiout, "name", varobj_get_objname (var));
-      ui_out_field_string (uiout, "in_scope", "true");
-      ui_out_field_string (uiout, "new_type", varobj_get_type(var));
-      ui_out_field_int (uiout, "new_num_children", 
-			   varobj_get_num_children(var));
+
+      switch (nc)
+      {
+        case NOT_IN_SCOPE:
+        case WRONG_PARAM:
+          ui_out_field_string (uiout, "in_scope", "false");
+	  break;
+        case INVALID:
+          ui_out_field_string (uiout, "in_scope", "invalid");
+ 	  break;
+        case TYPE_CHANGED:
+	  ui_out_field_string (uiout, "in_scope", "true");
+          ui_out_field_string (uiout, "new_type", varobj_get_type(var));
+          ui_out_field_int (uiout, "new_num_children", 
+			    varobj_get_num_children(var));
+	  break;
+      }
       if (mi_version (uiout) > 1)
         do_cleanups (cleanup);
     }
   else
     {
-      
       cc = changelist;
       while (*cc != NULL)
 	{
@@ -595,7 +595,5 @@ varobj_update_one (struct varobj *var, e
 	  cc++;
 	}
       xfree (changelist);
-      return 1;
     }
-  return 1;
 }

[-- Attachment #3: mi-var-invalidate.exp --]
[-- Type: text/plain, Size: 3821 bytes --]

# Copyright 1999, 2000, 2001, 2002, 2004, 2005 Free Software
# Foundation, Inc.
#
# This Program Is Free software; you can redistribute it and/or modify
# it under the terms of the GNU General Public License as published by
# the Free Software Foundation; either version 2 of the License, or
# (at your option) any later version.
#
# This program is distributed in the hope that it will be useful,
# but WITHOUT ANY WARRANTY; without even the implied warranty of
# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
# GNU General Public License for more details.
#
# You should have received a copy of the GNU General Public License
# along with this program; if not, write to the Free Software
# Foundation, Inc., 59 Temple Place - Suite 330, Boston, MA 02111-1307, USA.

# Test essential Machine interface (MI) operations
#
# Verify that once binary file has changed, GDB correctly handles
# previously defined MI variables.
#


load_lib mi-support.exp
set MIFLAGS "-i=mi"

gdb_exit
if [mi_gdb_start] {
    continue
}

set testfile "var-cmd"
set srcfile ${testfile}.c
set binfile ${objdir}/${subdir}/${testfile}
if  { [gdb_compile "${srcdir}/${subdir}/${srcfile}" "${binfile}" executable {debug additional_flags=-DFAKEARGV}] != "" } {
    untested mi-var-invalidate.exp
    return -1
}
# Just change the output binary.
set binfile_bis ${objdir}/${subdir}/${testfile}_bis
if  { [gdb_compile "${srcdir}/${subdir}/${srcfile}" "${binfile_bis}" executable {debug additional_flags=-DFAKEARGV}] != "" } {
    untested mi-var-invalidate.exp
    return -1
}

set testfile2 "basics"
set srcfile2 ${testfile2}.c
set binfile2 ${objdir}/${subdir}/${testfile2}
if  { [gdb_compile "${srcdir}/${subdir}/${srcfile2}" "${binfile2}" executable {debug additional_flags=-DFAKEARGV}] != "" } {
    untested mi-var-invalidate.exp
    return -1
}

mi_delete_breakpoints
mi_gdb_reinitialize_dir $srcdir/$subdir
mi_gdb_load ${binfile}

# Desc:  Create global variable
mi_gdb_test "-var-create global_simple * global_simple" \
	"\\^done,name=\"global_simple\",numchild=\"6\",type=\"simpleton\"" \
	"create global variable"

mi_runto do_locals_tests

# Desc: create local variables
mi_gdb_test "-var-create linteger * linteger" \
	"\\^done,name=\"linteger\",numchild=\"0\",type=\"int\"" \
	"create local variable linteger"

#
# reload the same binary
# global variable must be keeped, other invalidated
#
mi_delete_breakpoints
mi_gdb_load ${binfile_bis}
mi_runto main

# Check local variable are "invalid"
mi_gdb_test "-var-update linteger" \
	"\\^done,changelist=\\\[\{name=\"linteger\",in_scope=\"invalid\"\}\\\]" \
	"linteger not anymore in scope due to binary changes"

mi_gdb_test "-var-info-type linteger" \
	"\\^done,type=\"\"" \
	"not type for invalid variable linteger"

# Check global variable are still correct.
mi_gdb_test "-var-update global_simple" \
	"\\^done,changelist=\\\[\]" \
	"global_simple still alive"

mi_gdb_test "-var-info-type global_simple" \
	"\\^done,type=\"simpleton\"" \
	"type simpleton for valid variable global_simple"


#
# load an other binary
# all variables must be invalidated
#
mi_delete_breakpoints
mi_gdb_load ${binfile2}
# Check local variable are "invalid"
mi_gdb_test "-var-update linteger" \
	"\\^done,changelist=\\\[\{name=\"linteger\",in_scope=\"invalid\"\}\\\]" \
	"linteger not valid anymore due to binary changes"

mi_gdb_test "-var-info-type linteger" \
	"\\^done,type=\"\"" \
	"not type for invalid variable linteger"

# Check global variable are still correct.
mi_gdb_test "-var-update global_simple" \
	"\\^done,changelist=\\\[\{name=\"global_simple\",in_scope=\"invalid\"\}\\\]" \
	"global_simple not anymore in scope due to binary changes"

mi_gdb_test "-var-info-type global_simple" \
	"\\^done,type=\"\"" \
	"not type for invalid variable global_simple"

mi_gdb_exit
return 0

Re: [RFC] varobj deletion after the binary has changed

[Nick Roberts]

 >    /* sanity check: have we been passed a pointer? */
 >    if (changelist == NULL)
 > -    return -1;
 > +    return WRONG_PARAM;
 >  
 >    /*  Only root variables can be updated... */
 >    if (!is_root_p (*varp))
 >      /* Not a root var */
 > -    return -1;
 > +    return WRONG_PARAM;
 > +
 > +  if (!(*varp)->root->is_valid)
 > +    return INVALID;

OK I hadn't noticed the distinction between WRONG_PARAM and INVALID.  I think
it would be better to throw an error in the case of WRONG_PARAM, otherwise
changes to leaf values may go unnoticed.  But this is a separate change.

I've not checked the test but I think the English could be improved and
different names used:

>  ...
> # Verify that once binary file has changed, GDB correctly handles
> # previously defined MI variables.

# Verify that once binary file has changed, GDB correctly handles
# previously defined MI variable objects.

 > ...
 > #
 > # reload the same binary
 > # global variable must be keeped, other invalidated

 # Reload the same binary.
 # Global variable should remain, local should be invalidated.

 > #
 > mi_delete_breakpoints
 > mi_gdb_load ${binfile_bis}
 > mi_runto main

 > # Check local variable are "invalid"

 # Check local variable is "invalid"

 > mi_gdb_test "-var-update linteger" \
 > 	"\\^done,changelist=\\\[\{name=\"linteger\",in_scope=\"invalid\"\}\\\]" \
 > 	"linteger not anymore in scope due to binary changes"
 > 
 > mi_gdb_test "-var-info-type linteger" \
 > 	"\\^done,type=\"\"" \
 > 	"not type for invalid variable linteger"

 "no type for invalid variable linteger (1)"

 > # Check global variable are still correct.
 > mi_gdb_test "-var-update global_simple" \
 > 	"\\^done,changelist=\\\[\]" \
 > 	"global_simple still alive"
 > 
 > mi_gdb_test "-var-info-type global_simple" \
 > 	"\\^done,type=\"simpleton\"" \
 > 	"type simpleton for valid variable global_simple"
 > 
 > 
 > #
 > # load an other binary
 > # all variables must be invalidated

I guess capital letters at the start and full stops.

 > mi_delete_breakpoints
 > mi_gdb_load ${binfile2}
 > # Check local variable are "invalid"
 > mi_gdb_test "-var-update linteger" \
 > 	"\\^done,changelist=\\\[\{name=\"linteger\",in_scope=\"invalid\"\}\\\]" \
 > 	"linteger not valid anymore due to binary changes"
 > 
 > mi_gdb_test "-var-info-type linteger" \
 > 	"\\^done,type=\"\"" \
 > 	"not type for invalid variable linteger"

 "no type for invalid variable linteger (2)"
 > 
 > # Check global variable are still correct.
 > mi_gdb_test "-var-update global_simple" \
 > 	"\\^done,changelist=\\\[\{name=\"global_simple\",in_scope=\"invalid\"\}\\\]" \
 > 	"global_simple not anymore in scope due to binary changes"
 > 
 > mi_gdb_test "-var-info-type global_simple" \
 > 	"\\^done,type=\"\"" \
 > 	"not type for invalid variable global_simple"
 > 
 
 "no type for invalid variable global_simple"

 > mi_gdb_exit
 > return 0

-- 
Nick                                           http://www.inet.net.nz/~nickrob

Re: [RFC] varobj deletion after the binary has changed

[Denis PILAT]

Nick Roberts wrote:
>  >    /* sanity check: have we been passed a pointer? */
>  >    if (changelist == NULL)
>  > -    return -1;
>  > +    return WRONG_PARAM;
>  >  
>  >    /*  Only root variables can be updated... */
>  >    if (!is_root_p (*varp))
>  >      /* Not a root var */
>  > -    return -1;
>  > +    return WRONG_PARAM;
>  > +
>  > +  if (!(*varp)->root->is_valid)
>  > +    return INVALID;
>
> OK I hadn't noticed the distinction between WRONG_PARAM and INVALID.  I think
> it would be better to throw an error in the case of WRONG_PARAM, otherwise
> changes to leaf values may go unnoticed.  But this is a separate change.
>   
Yes it is, and the bigger is this patch the less it has chance to be 
approved.
> I've not checked the test but I think the English could be improved and
> different names used:
>   
Oh really?? I though my froggy English was the best.
Anyway thanks for your comments, I'll take them into account once you've 
tested the test and once Daniel gives me a feedback about the C part of 
the patch.

-- 
Denis

Re: [RFC] varobj deletion after the binary has changed

[Daniel Jacobowitz]

On Thu, Feb 01, 2007 at 10:49:00AM +0100, Denis PILAT wrote:
> Anyway thanks for your comments, I'll take them into account once you've 
> tested the test and once Daniel gives me a feedback about the C part of 
> the patch.

The C seems reasonable to me; though let's not check it in until we
have a documentation update to go with it, if possible.  How about
under the -var-update section, by the example which uses
in_scope="true"?

-- 
Daniel Jacobowitz
CodeSourcery

Re: [RFC] varobj deletion after the binary has changed

[Nick Roberts]

 > The C seems reasonable to me; though let's not check it in until we
 > have a documentation update to go with it, if possible.  How about
 > under the -var-update section, by the example which uses
 > in_scope="true"?

How about this?

-- 
Nick                                           http://www.inet.net.nz/~nickrob


2007-02-09  Nick Roberts  <nickrob@snap.net.nz>

	* gdb.texinfo (GDB/MI Variable Objects): Describe meanings of
	values for in_scope.  Mention that only root variables can be
	updated.
	(GDB/MI Development and Front Ends): Explain new values may be
	added to existing fields.


*** gdb.texinfo	08 Feb 2007 13:24:03 +1300	1.384
--- gdb.texinfo	09 Feb 2007 08:27:39 +1300	
*************** New MI commands may be added.
*** 17732,17737 ****
--- 17732,17741 ----
  @item
  New fields may be added to the output of any MI command.
  
+ @item
+ The range of values for fields with specified values e.g in_scope,
+ type_changed may be extended.
+ 
  @c The format of field's content e.g type prefix, may change so parse it
  @c   at your own risk.  Yes, in general?
  
*************** subsequent @code{-var-update} list.
*** 19972,19987 ****
  
  Reevaluate the expressions corresponding to the variable object
  @var{name} and all its direct and indirect children, and return the
! list of variable objects whose values have changed. Here,
! ``changed'' means that the result of @code{-var-evaluate-expression} before
! and after the @code{-var-update} is different.  If @samp{*} is used
! as the variable object names, all existing variable objects are
! updated.  The option @var{print-values} determines whether both names 
! and values, or just names are printed.  The possible values of
! this options are the same as for @code{-var-list-children}
! (@pxref{-var-list-children}).  It is recommended to use the
! @samp{--all-values} option, to reduce the number of MI commands needed
! on each program stop.
  
  
  @subsubheading Example
--- 19976,19991 ----
  
  Reevaluate the expressions corresponding to the variable object
  @var{name} and all its direct and indirect children, and return the
! list of variable objects whose values have changed.  @var{name} must
! be a root variable object.  Here, ``changed'' means that the result of
! @code{-var-evaluate-expression} before and after the
! @code{-var-update} is different.  If @samp{*} is used as the variable
! object names, all existing variable objects are updated.  The option
! @var{print-values} determines whether both names and values, or just
! names are printed.  The possible values of this options are the same
! as for @code{-var-list-children} (@pxref{-var-list-children}).  It is
! recommended to use the @samp{--all-values} option, to reduce the
! number of MI commands needed on each program stop.
  
  
  @subsubheading Example
*************** type_changed="false"@}]
*** 19997,20002 ****
--- 20001,20027 ----
  (gdb)
  @end smallexample
  
+ The field in_scope may take three values:
+ 
+ @table @code
+ @item "true"
+ The variable object's current value is valid.
+ 
+ @item "false"
+ The variable object does not currently hold a valid value but it
+ may hold one in the future it's associated expression comes back into scope.
+ 
+ @item "invalid"
+ The variable object no longer holds a valid value.
+ This can occur when the executable file being debugged has changed,
+ either through recompilation or by using the @value{GDBN} @code{file}
+ command.  The front end should normally choose to delete these variable
+ objects.
+ @end table
+ 
+ In the future new values may be added to this list so the front should
+ be prepared for this possibility.  @xref{GDB/MI Development and Front Ends, ,@sc{GDB/MI} Development and Front Ends}.
+ 
  @c %%%%%%%%%%%%%%%%%%%%%%%%%%%% SECTION %%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%
  @node GDB/MI Data Manipulation
  @section @sc{gdb/mi} Data Manipulation

Re: [RFC] varobj deletion after the binary has changed

[Daniel Jacobowitz]

Thanks!

On Fri, Feb 09, 2007 at 08:33:08AM +1300, Nick Roberts wrote:
> + @item "false"
> + The variable object does not currently hold a valid value but it
> + may hold one in the future it's associated expression comes back into scope.

"it's" -> "if its"

Other than that the content is fine with me, if the texinfo is OK with
Eli.

-- 
Daniel Jacobowitz
CodeSourcery

Re: [RFC] varobj deletion after the binary has changed

[Eli Zaretskii]

> From: Nick Roberts <nickrob@snap.net.nz>
> Date: Fri, 9 Feb 2007 08:33:08 +1300
> Cc: Denis PILAT <denis.pilat@st.com>, 	gdb-patches <gdb-patches@sourceware.org>
> 
> How about this?

Approved, provided that you fix the following:

> + @item
> + The range of values for fields with specified values e.g in_scope,
> + type_changed may be extended.

First, "e.g" lacks the second period and a comma after it ("e.g.,").

Second, in_scope and type_changed should be in @code or @samp.

Third, these two are not mentioned anywhere in the node you are
patching, so please either add a cross-reference to where they are
described or add some minimal description to the text.

>   Reevaluate the expressions corresponding to the variable object
>   @var{name} and all its direct and indirect children, and return the
> ! list of variable objects whose values have changed.  @var{name} must
> ! be a root variable object.

The last sentence will begin with a non-capital letter, which is not
valid English.  How about using `;' instead of a period before
@var{name}?

Thanks.

Re: [RFC] varobj deletion after the binary has changed

[Denis PILAT]

[-- Attachment #1: Type: text/plain, Size: 652 bytes --]

Nick,
Thanks you for the documentation update.
Hereby is 2 patches I would like to commit, the C part and the new . exp 
file.
I change the .exp file with your comment, with a newer copyright notice, 
and with including value field in output of -var-create.
I also changed a comment plus typos in the C code.

Daniel already approved the sources, Eli is near to be fine with the doc.
Could have a look once more into the  following, at least for the 
ChangeLog entry ?
Then how could we synchronize our commit ?

-- 
Denis

ChangeLog for the testsuite:

2007-02-12  Denis Pilat  <denis.pilat@st.com>

	* gdb.mi/mi-var-invalidate.exp: New files.

    


[-- Attachment #2: varobj.patch --]
[-- Type: text/plain, Size: 12315 bytes --]

2007-02-12  Denis Pilat  <denis.pilat@st.com>

	* varobj.c (struct varobj_root): Add is_valid member.
	(varobj_get_type): Check for invalid varobj.
	(varobj_get_attributes): Likewise.
	(variable_editable):Likewise.
	(varobj_update): Likewise plus use an enum for returned error values.
	(new_root_variable): Set root varobj as valid by default.
	(varobj_invalidate): New function.
	* varobj.h (enum varobj_update_error): New enum.
	(varobj_invalidate): New function.
	* symfile.c (clear_symtab_users): Use varobj_invalidate.
	* mi/mi-cmd-var.c (varobj_update_one): Change returned type to void and
	use of new enum varobj_update_error to deal with errors.

Index: symfile.c
===================================================================
RCS file: /cvs/src/src/gdb/symfile.c,v
retrieving revision 1.180
diff -u -p -r1.180 symfile.c
--- symfile.c	21 Jan 2007 01:02:03 -0000	1.180
+++ symfile.c	12 Feb 2007 09:54:22 -0000
@@ -52,6 +52,7 @@
 #include "observer.h"
 #include "exec.h"
 #include "parser-defs.h"
+#include "varobj.h"
 
 #include <sys/types.h>
 #include <fcntl.h>
@@ -2602,6 +2603,10 @@ clear_symtab_users (void)
      between expressions and which ought to be reset each time.  */
   expression_context_block = NULL;
   innermost_block = NULL;
+
+  /* Varobj may refer to old symbols, perform a cleanup.  */
+  varobj_invalidate ();
+
 }
 
 static void
Index: varobj.c
===================================================================
RCS file: /cvs/src/src/gdb/varobj.c,v
retrieving revision 1.83
diff -u -p -r1.83 varobj.c
--- varobj.c	8 Feb 2007 17:39:48 -0000	1.83
+++ varobj.c	12 Feb 2007 09:54:30 -0000
@@ -71,6 +71,10 @@ struct varobj_root
      using the currently selected frame. */
   int use_selected_frame;
 
+  /* Flag that indicates validity: set to 0 when this varobj_root refers 
+     to symbols that do not exist anymore.  */
+  int is_valid;
+
   /* Language info for this variable and its children */
   struct language_specific *lang;
 
@@ -742,8 +746,9 @@ varobj_get_type (struct varobj *var)
   long length;
 
   /* For the "fake" variables, do not return a type. (It's type is
-     NULL, too.) */
-  if (CPLUS_FAKE_CHILD (var))
+     NULL, too.)
+     Do not return a type for invalid variables as well.  */
+  if (CPLUS_FAKE_CHILD (var) || !var->root->is_valid)
     return NULL;
 
   stb = mem_fileopen ();
@@ -778,7 +783,7 @@ varobj_get_attributes (struct varobj *va
 {
   int attributes = 0;
 
-  if (variable_editable (var))
+  if (var->root->is_valid && variable_editable (var))
     /* FIXME: define masks for attributes */
     attributes |= 0x00000001;	/* Editable */
 
@@ -1018,16 +1023,15 @@ install_new_value (struct varobj *var, s
    expression to see if it's changed.  Then go all the way
    through its children, reconstructing them and noting if they've
    changed.
-   Return value:
-    -1 if there was an error updating the varobj
-    -2 if the type changed
-    Otherwise it is the number of children + parent changed
+   Return value: 
+    < 0 for error values, see varobj.h.
+    Otherwise it is the number of children + parent changed.
 
    Only root variables can be updated... 
 
    NOTE: This function may delete the caller's varobj. If it
-   returns -2, then it has done this and VARP will be modified
-   to point to the new varobj. */
+   returns TYPE_CHANGED, then it has done this and VARP will be modified
+   to point to the new varobj.  */
 
 int
 varobj_update (struct varobj **varp, struct varobj ***changelist)
@@ -1046,34 +1050,37 @@ varobj_update (struct varobj **varp, str
   struct frame_id old_fid;
   struct frame_info *fi;
 
-  /* sanity check: have we been passed a pointer? */
+  /* sanity check: have we been passed a pointer?  */
   if (changelist == NULL)
-    return -1;
+    return WRONG_PARAM;
 
-  /*  Only root variables can be updated... */
+  /*  Only root variables can be updated...  */
   if (!is_root_p (*varp))
-    /* Not a root var */
-    return -1;
+    /* Not a root var.  */
+    return WRONG_PARAM;
+
+  if (!(*varp)->root->is_valid)
+    return INVALID;
 
   /* Save the selected stack frame, since we will need to change it
-     in order to evaluate expressions. */
+     in order to evaluate expressions.  */
   old_fid = get_frame_id (deprecated_selected_frame);
 
   /* Update the root variable. value_of_root can return NULL
      if the variable is no longer around, i.e. we stepped out of
      the frame in which a local existed. We are letting the 
      value_of_root variable dispose of the varobj if the type
-     has changed. */
+     has changed.  */
   type_changed = 1;
   new = value_of_root (varp, &type_changed);
 
-  /* Restore selected frame */
+  /* Restore selected frame.  */
   fi = frame_find_by_id (old_fid);
   if (fi)
     select_frame (fi);
 
   /* If this is a "use_selected_frame" varobj, and its type has changed,
-     them note that it's changed. */
+     them note that it's changed.  */
   if (type_changed)
     VEC_safe_push (varobj_p, result, *varp);
 
@@ -1090,12 +1097,12 @@ varobj_update (struct varobj **varp, str
       /* This means the varobj itself is out of scope.
 	 Report it.  */
       VEC_free (varobj_p, result);
-      return -1;
+      return NOT_IN_SCOPE;
     }
 
   VEC_safe_push (varobj_p, stack, *varp);
 
-  /* Walk through the children, reconstructing them all. */
+  /* Walk through the children, reconstructing them all.  */
   while (!VEC_empty (varobj_p, stack))
     {
       v = VEC_pop (varobj_p, stack);
@@ -1126,7 +1133,7 @@ varobj_update (struct varobj **varp, str
 	}
     }
 
-  /* Alloc (changed + 1) list entries */
+  /* Alloc (changed + 1) list entries.  */
   changed = VEC_length (varobj_p, result);
   *changelist = xmalloc ((changed + 1) * sizeof (struct varobj *));
   cv = *changelist;
@@ -1140,7 +1147,7 @@ varobj_update (struct varobj **varp, str
   *cv = 0;
 
   if (type_changed)
-    return -2;
+    return TYPE_CHANGED;
   else
     return changed;
 }
@@ -1409,6 +1416,7 @@ new_root_variable (void)
   var->root->frame = null_frame_id;
   var->root->use_selected_frame = 0;
   var->root->rootvar = NULL;
+  var->root->is_valid = 1;
 
   return var;
 }
@@ -1692,7 +1700,10 @@ variable_editable (struct varobj *var)
 static char *
 my_value_of_variable (struct varobj *var)
 {
-  return (*var->root->lang->value_of_variable) (var);
+  if (var->root->is_valid)
+    return (*var->root->lang->value_of_variable) (var);
+  else
+    return NULL;
 }
 
 static char *
@@ -2504,3 +2515,44 @@ When non-zero, varobj debugging is enabl
 			    show_varobjdebug,
 			    &setlist, &showlist);
 }
+
+/* Invalidate the varobjs that are tied to locals and re-create the ones that
+   are defined on globals.
+   Invalidated varobjs will be always printed in_scope="invalid".  */
+void 
+varobj_invalidate (void)
+{
+  struct varobj **all_rootvarobj;
+  struct varobj **varp;
+
+  if (varobj_list (&all_rootvarobj) > 0)
+  {
+    varp = all_rootvarobj;
+    while (*varp != NULL)
+      {
+        /* global var must be re-evaluated.  */     
+        if ((*varp)->root->valid_block == NULL)
+        {
+          struct varobj *tmp_var;
+
+          /* Try to create a varobj with same expression.  If we succeed replace
+             the old varobj, otherwise invalidate it.  */
+          tmp_var = varobj_create (NULL, (*varp)->name, (CORE_ADDR) 0, USE_CURRENT_FRAME);
+          if (tmp_var != NULL) 
+            { 
+	      tmp_var->obj_name = xstrdup ((*varp)->obj_name);
+              varobj_delete (*varp, NULL, 0);
+              install_variable (tmp_var);
+            }
+          else
+              (*varp)->root->is_valid = 0;
+        }
+        else /* locals must be invalidated.  */
+          (*varp)->root->is_valid = 0;
+
+        varp++;
+      }
+    xfree (all_rootvarobj);
+  }
+  return;
+}
Index: varobj.h
===================================================================
RCS file: /cvs/src/src/gdb/varobj.h,v
retrieving revision 1.7
diff -u -p -r1.7 varobj.h
--- varobj.h	9 Jan 2007 17:58:59 -0000	1.7
+++ varobj.h	12 Feb 2007 09:54:33 -0000
@@ -38,7 +38,16 @@ enum varobj_type
     USE_CURRENT_FRAME,          /* Use the current frame */
     USE_SELECTED_FRAME          /* Always reevaluate in selected frame */
   };
-    
+
+/* Error return values for varobj_update function.  */
+enum varobj_update_error
+  {
+    NOT_IN_SCOPE = -1,          /* varobj not in scope, can not be updated.  */
+    TYPE_CHANGED = -2,          /* varobj type has changed.  */
+    INVALID = -3,               /* varobj is not valid anymore.  */
+    WRONG_PARAM = -4            /* function is called with wrong arguments.  */
+  };
+
 /* String representations of gdb's format codes (defined in varobj.c) */
 extern char *varobj_format_string[];
 
@@ -99,4 +108,6 @@ extern int varobj_list (struct varobj **
 
 extern int varobj_update (struct varobj **varp, struct varobj ***changelist);
 
+extern void varobj_invalidate (void);
+
 #endif /* VAROBJ_H */
Index: mi/mi-cmd-var.c
===================================================================
RCS file: /cvs/src/src/gdb/mi/mi-cmd-var.c,v
retrieving revision 1.29
diff -u -p -r1.29 mi-cmd-var.c
--- mi/mi-cmd-var.c	8 Feb 2007 19:46:30 -0000	1.29
+++ mi/mi-cmd-var.c	12 Feb 2007 09:54:33 -0000
@@ -34,9 +34,9 @@ const char mi_no_values[] = "--no-values
 const char mi_simple_values[] = "--simple-values";
 const char mi_all_values[] = "--all-values";
 
-extern int varobjdebug;		/* defined in varobj.c */
+extern int varobjdebug;		/* defined in varobj.c.  */
 
-static int varobj_update_one (struct varobj *var,
+static void varobj_update_one (struct varobj *var,
 			      enum print_values print_values);
 
 static int mi_print_value_p (struct type *type, enum print_values print_values);
@@ -535,11 +535,9 @@ mi_cmd_var_update (char *command, char *
     return MI_CMD_DONE;
 }
 
-/* Helper for mi_cmd_var_update() Returns 0 if the update for
-   the variable fails (usually because the variable is out of
-   scope), and 1 if it succeeds. */
+/* Helper for mi_cmd_var_update().  */
 
-static int
+static void
 varobj_update_one (struct varobj *var, enum print_values print_values)
 {
   struct varobj **changelist;
@@ -549,37 +547,39 @@ varobj_update_one (struct varobj *var, e
 
   nc = varobj_update (&var, &changelist);
 
-  /* nc == 0 means that nothing has changed.
-     nc == -1 means that an error occured in updating the variable.
-     nc == -2 means the variable has changed type. */
+  /* nc >= 0  represents the number of changes reported into changelist.
+     nc < 0   means that an error occured or the the variable has 
+              changed type (TYPE_CHANGED).  */
   
   if (nc == 0)
-    return 1;
-  else if (nc == -1)
+    return;
+  else if (nc < 0)
     {
       if (mi_version (uiout) > 1)
         cleanup = make_cleanup_ui_out_tuple_begin_end (uiout, NULL);
       ui_out_field_string (uiout, "name", varobj_get_objname(var));
-      ui_out_field_string (uiout, "in_scope", "false");
-      if (mi_version (uiout) > 1)
-        do_cleanups (cleanup);
-      return -1;
-    }
-  else if (nc == -2)
-    {
-      if (mi_version (uiout) > 1)
-        cleanup = make_cleanup_ui_out_tuple_begin_end (uiout, NULL);
-      ui_out_field_string (uiout, "name", varobj_get_objname (var));
-      ui_out_field_string (uiout, "in_scope", "true");
-      ui_out_field_string (uiout, "new_type", varobj_get_type(var));
-      ui_out_field_int (uiout, "new_num_children", 
-			   varobj_get_num_children(var));
+
+      switch (nc)
+      {
+        case NOT_IN_SCOPE:
+        case WRONG_PARAM:
+          ui_out_field_string (uiout, "in_scope", "false");
+	  break;
+        case INVALID:
+          ui_out_field_string (uiout, "in_scope", "invalid");
+ 	  break;
+        case TYPE_CHANGED:
+	  ui_out_field_string (uiout, "in_scope", "true");
+          ui_out_field_string (uiout, "new_type", varobj_get_type(var));
+          ui_out_field_int (uiout, "new_num_children", 
+			    varobj_get_num_children(var));
+	  break;
+      }
       if (mi_version (uiout) > 1)
         do_cleanups (cleanup);
     }
   else
     {
-      
       cc = changelist;
       while (*cc != NULL)
 	{
@@ -595,7 +595,5 @@ varobj_update_one (struct varobj *var, e
 	  cc++;
 	}
       xfree (changelist);
-      return 1;
     }
-  return 1;
 }

[-- Attachment #3: mi-var-invalidate.exp --]
[-- Type: text/plain, Size: 3840 bytes --]

# Copyright 2007 Free Software Foundation, Inc.

# This program is free software; you can redistribute it and/or modify
# it under the terms of the GNU General Public License as published by
# the Free Software Foundation; either version 2 of the License, or
# (at your option) any later version.
#
# This program is distributed in the hope that it will be useful,
# but WITHOUT ANY WARRANTY; without even the implied warranty of
# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
# GNU General Public License for more details.
#
# You should have received a copy of the GNU General Public License
# along with this program; if not, write to the Free Software
# Foundation, Inc., 59 Temple Place - Suite 330, Boston, MA 02111-1307, USA.
#
# Test essential Machine interface (MI) operations
#
# Verify that once binary file has changed, GDB correctly handles
# previously defined MI variables.
#


load_lib mi-support.exp
set MIFLAGS "-i=mi"

gdb_exit
if [mi_gdb_start] {
    continue
}

set testfile "var-cmd"
set srcfile ${testfile}.c
set binfile ${objdir}/${subdir}/${testfile}
if  { [gdb_compile "${srcdir}/${subdir}/${srcfile}" "${binfile}" executable {debug additional_flags=-DFAKEARGV}] != "" } {
    untested mi-var-invalidate.exp
    return -1
}
# Just change the output binary.
set binfile_bis ${objdir}/${subdir}/${testfile}_bis
if  { [gdb_compile "${srcdir}/${subdir}/${srcfile}" "${binfile_bis}" executable {debug additional_flags=-DFAKEARGV}] != "" } {
    untested mi-var-invalidate.exp
    return -1
}

set testfile2 "basics"
set srcfile2 ${testfile2}.c
set binfile2 ${objdir}/${subdir}/${testfile2}
if  { [gdb_compile "${srcdir}/${subdir}/${srcfile2}" "${binfile2}" executable {debug additional_flags=-DFAKEARGV}] != "" } {
    untested mi-var-invalidate.exp
    return -1
}

mi_delete_breakpoints
mi_gdb_reinitialize_dir $srcdir/$subdir
mi_gdb_load ${binfile}

# Desc:  Create global variable.
mi_gdb_test "-var-create global_simple * global_simple" \
        "\\^done,name=\"global_simple\",numchild=\"6\",value=\".*\",type=\"simpleton\"" \
	"create global variable"

mi_runto do_locals_tests

# Desc: create local variables
mi_gdb_test "-var-create linteger * linteger" \
	"\\^done,name=\"linteger\",numchild=\"0\",value=\".*\",type=\"int\"" \
	"create local variable linteger"

#
# Reload the same binary.
# Global variable should remain, local should be invalidated.
#
mi_delete_breakpoints
mi_gdb_load ${binfile_bis}
mi_runto main

# Check local variable is "invalid".
mi_gdb_test "-var-update linteger" \
	"\\^done,changelist=\\\[\{name=\"linteger\",in_scope=\"invalid\"\}\\\]" \
	"linteger not anymore in scope due to binary changes"

mi_gdb_test "-var-info-type linteger" \
	"\\^done,type=\"\"" \
	"no type for invalid variable linteger (1)"

# Check global variable is still correct.
mi_gdb_test "-var-update global_simple" \
	"\\^done,changelist=\\\[\]" \
	"global_simple still alive"

mi_gdb_test "-var-info-type global_simple" \
	"\\^done,type=\"simpleton\"" \
	"type simpleton for valid variable global_simple"


#
# Load an other binary.
# All variables must be invalidated.
#
mi_delete_breakpoints
mi_gdb_load ${binfile2}
# Check local variable are "invalid"
mi_gdb_test "-var-update linteger" \
	"\\^done,changelist=\\\[\{name=\"linteger\",in_scope=\"invalid\"\}\\\]" \
	"linteger not valid anymore due to binary changes"

mi_gdb_test "-var-info-type linteger" \
	"\\^done,type=\"\"" \
	"no type for invalid variable linteger (2)"

# Check global variable are still correct.
mi_gdb_test "-var-update global_simple" \
	"\\^done,changelist=\\\[\{name=\"global_simple\",in_scope=\"invalid\"\}\\\]" \
	"global_simple not anymore in scope due to binary changes"

mi_gdb_test "-var-info-type global_simple" \
	"\\^done,type=\"\"" \
	"no type for invalid variable global_simple"

mi_gdb_exit
return 0

Re: [RFC] varobj deletion after the binary has changed

[Nick Roberts]

 > Thanks you for the documentation update.
 > Hereby is 2 patches I would like to commit, the C part and the new . exp 
 > file.
 > I change the .exp file with your comment, with a newer copyright notice, 
 > and with including value field in output of -var-create.
 > I also changed a comment plus typos in the C code.
 > 
 > Daniel already approved the sources, Eli is near to be fine with the doc.
 > Could have a look once more into the  following, at least for the 
 > ChangeLog entry ?
 > Then how could we synchronize our commit ?

I've committed my documentation with requested changes (see below).  All you
need to do is commit yours.

 > 2007-02-12  Denis Pilat  <denis.pilat@st.com>
 > 
 > 	* gdb.mi/mi-var-invalidate.exp: New files.

                                        New file.

 > 2007-02-12  Denis Pilat  <denis.pilat@st.com>
 > 
 > 	* varobj.c (struct varobj_root): Add is_valid member.
 > 	(varobj_get_type): Check for invalid varobj.
 > 	(varobj_get_attributes): Likewise.
 > 	(variable_editable):Likewise.
 > 	(varobj_update): Likewise plus use an enum for returned error values.
 > 	(new_root_variable): Set root varobj as valid by default.
 > 	(varobj_invalidate): New function.
 > 	* varobj.h (enum varobj_update_error): New enum.
 > 	(varobj_invalidate): New function.
duplicate entry

 > 	* symfile.c (clear_symtab_users): Use varobj_invalidate.
 > 	* mi/mi-cmd-var.c (varobj_update_one): Change returned type to void and
 > 	use of new enum varobj_update_error to deal with errors.

 	* varobj.h (enum varobj_update_error): New enum.
 	* varobj.c (struct varobj_root): Add is_valid member.
 	(varobj_get_type): Check for invalid varobj.
 	(varobj_get_attributes): Likewise.
 	(variable_editable):Likewise.
 	(varobj_update): Likewise.  Use varobj_update_error.
 	(new_root_variable): Set root varobj as valid by default.
 	(varobj_invalidate): New function.
 	* symfile.c (clear_symtab_users): Use varobj_invalidate.
 	* mi/mi-cmd-var.c (varobj_update_one): Change return type to void.
  	Use varobj_update_error.

(Suggestion)

-- 
Nick                                           http://www.inet.net.nz/~nickrob


2007-02-13  Nick Roberts  <nickrob@snap.net.nz>

	* gdb.texinfo (GDB/MI Variable Objects): Describe meanings of
	values for in_scope.  Mention that only root variables can be
	updated.
	(GDB/MI Development and Front Ends): Explain new values may be
	added to existing fields.


*** gdb.texinfo	13 Feb 2007 12:48:47 +1300	1.385
--- gdb.texinfo	10 Feb 2007 10:15:05 +1300	
*************** New MI commands may be added.
*** 17732,17737 ****
--- 17732,17741 ----
  @item
  New fields may be added to the output of any MI command.
  
+ @item
+ The range of values for fields with specified values, e.g.,
+ @code{in_scope} (@pxref{-var-update}) may be extended.
+ 
  @c The format of field's content e.g type prefix, may change so parse it
  @c   at your own risk.  Yes, in general?
  
*************** subsequent @code{-var-update} list.
*** 19972,19987 ****
  
  Reevaluate the expressions corresponding to the variable object
  @var{name} and all its direct and indirect children, and return the
! list of variable objects whose values have changed. Here,
! ``changed'' means that the result of @code{-var-evaluate-expression} before
! and after the @code{-var-update} is different.  If @samp{*} is used
! as the variable object names, all existing variable objects are
! updated.  The option @var{print-values} determines whether both names 
! and values, or just names are printed.  The possible values of
! this options are the same as for @code{-var-list-children}
! (@pxref{-var-list-children}).  It is recommended to use the
! @samp{--all-values} option, to reduce the number of MI commands needed
! on each program stop.
  
  
  @subsubheading Example
--- 19976,19991 ----
  
  Reevaluate the expressions corresponding to the variable object
  @var{name} and all its direct and indirect children, and return the
! list of variable objects whose values have changed; @var{name} must
! be a root variable object.  Here, ``changed'' means that the result of
! @code{-var-evaluate-expression} before and after the
! @code{-var-update} is different.  If @samp{*} is used as the variable
! object names, all existing variable objects are updated.  The option
! @var{print-values} determines whether both names and values, or just
! names are printed.  The possible values of this options are the same
! as for @code{-var-list-children} (@pxref{-var-list-children}).  It is
! recommended to use the @samp{--all-values} option, to reduce the
! number of MI commands needed on each program stop.
  
  
  @subsubheading Example
*************** type_changed="false"@}]
*** 19997,20002 ****
--- 20001,20029 ----
  (gdb)
  @end smallexample
  
+ @anchor{-var-update} 
+ The field in_scope may take three values:
+ 
+ @table @code
+ @item "true"
+ The variable object's current value is valid.
+ 
+ @item "false"
+ The variable object does not currently hold a valid value but it may
+ hold one in the future if its associated expression comes back into
+ scope.
+ 
+ @item "invalid"
+ The variable object no longer holds a valid value.
+ This can occur when the executable file being debugged has changed,
+ either through recompilation or by using the @value{GDBN} @code{file}
+ command.  The front end should normally choose to delete these variable
+ objects.
+ @end table
+ 
+ In the future new values may be added to this list so the front should
+ be prepared for this possibility.  @xref{GDB/MI Development and Front Ends, ,@sc{GDB/MI} Development and Front Ends}.
+ 
  @c %%%%%%%%%%%%%%%%%%%%%%%%%%%% SECTION %%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%
  @node GDB/MI Data Manipulation
  @section @sc{gdb/mi} Data Manipulation

Re: [RFC] varobj deletion after the binary has changed

[Denis PILAT]

Nick Roberts wrote:
>  > Thanks you for the documentation update.
>  > Hereby is 2 patches I would like to commit, the C part and the new . exp 
>  > file.
>  > I change the .exp file with your comment, with a newer copyright notice, 
>  > and with including value field in output of -var-create.
>  > I also changed a comment plus typos in the C code.
>  > 
>  > Daniel already approved the sources, Eli is near to be fine with the doc.
>  > Could have a look once more into the  following, at least for the 
>  > ChangeLog entry ?
>  > Then how could we synchronize our commit ?
>
> I've committed my documentation with requested changes (see below).  All you
> need to do is commit yours.
>
>  > 2007-02-12  Denis Pilat  <denis.pilat@st.com>
>  > 
>  > 	* gdb.mi/mi-var-invalidate.exp: New files.
>
>                                         New file.
>
>  > 2007-02-12  Denis Pilat  <denis.pilat@st.com>
>  > 
>  > 	* varobj.c (struct varobj_root): Add is_valid member.
>  > 	(varobj_get_type): Check for invalid varobj.
>  > 	(varobj_get_attributes): Likewise.
>  > 	(variable_editable):Likewise.
>  > 	(varobj_update): Likewise plus use an enum for returned error values.
>  > 	(new_root_variable): Set root varobj as valid by default.
>  > 	(varobj_invalidate): New function.
>  > 	* varobj.h (enum varobj_update_error): New enum.
>  > 	(varobj_invalidate): New function.
> duplicate entry
>
>  > 	* symfile.c (clear_symtab_users): Use varobj_invalidate.
>  > 	* mi/mi-cmd-var.c (varobj_update_one): Change returned type to void and
>  > 	use of new enum varobj_update_error to deal with errors.
>
>  	* varobj.h (enum varobj_update_error): New enum.
>  	* varobj.c (struct varobj_root): Add is_valid member.
>  	(varobj_get_type): Check for invalid varobj.
>  	(varobj_get_attributes): Likewise.
>  	(variable_editable):Likewise.
>  	(varobj_update): Likewise.  Use varobj_update_error.
>  	(new_root_variable): Set root varobj as valid by default.
>  	(varobj_invalidate): New function.
>  	* symfile.c (clear_symtab_users): Use varobj_invalidate.
>  	* mi/mi-cmd-var.c (varobj_update_one): Change return type to void.
>   	Use varobj_update_error.
>
> (Suggestion)
>
>   
The following has just been committed:
- symfile.c, varobj.c, varobj.h, mi/mi-cmd-var.c
- testsuite/gdb.mi/mi-var-invalidate.exp

-- 
Denis

Re: [RFC] varobj deletion after the binary has changed

[Andreas Schwab]

Denis PILAT <denis.pilat@st.com> writes:

> Index: symfile.c
> ===================================================================
> RCS file: /cvs/src/src/gdb/symfile.c,v
> retrieving revision 1.180
> diff -u -p -r1.180 symfile.c
> --- symfile.c	21 Jan 2007 01:02:03 -0000	1.180
> +++ symfile.c	12 Feb 2007 09:54:22 -0000
> @@ -52,6 +52,7 @@
>  #include "observer.h"
>  #include "exec.h"
>  #include "parser-defs.h"
> +#include "varobj.h"
>  
>  #include <sys/types.h>
>  #include <fcntl.h>

You forgot to update the dependencies in the makefile.  Installed as
obvious.

Andreas.

2007-02-20  Andreas Schwab  <schwab@suse.de>

	* Makefile.in (symfile.o): Update dependencies.

--- gdb/Makefile.in.~1.877.~	2007-02-20 16:57:34.000000000 +0100
+++ gdb/Makefile.in	2007-02-20 16:58:03.000000000 +0100
@@ -2772,7 +2772,7 @@ symfile.o: symfile.c $(defs_h) $(bfdlink
 	$(gdb_stabs_h) $(gdb_obstack_h) $(completer_h) $(bcache_h) \
 	$(hashtab_h) $(readline_h) $(gdb_assert_h) $(block_h) \
 	$(gdb_string_h) $(gdb_stat_h) $(observer_h) $(exec_h) \
-	$(parser_defs_h)
+	$(parser_defs_h) $(varobj_h)
 symfile-mem.o: symfile-mem.c $(defs_h) $(symtab_h) $(gdbcore_h) \
 	$(objfiles_h) $(exceptions_h) $(gdbcmd_h) $(target_h) $(value_h) \
 	$(symfile_h) $(observer_h) $(auxv_h) $(elf_common_h)

-- 
Andreas Schwab, SuSE Labs, schwab@suse.de
SuSE Linux Products GmbH, Maxfeldstraße 5, 90409 Nürnberg, Germany
PGP key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: [RFC] varobj deletion after the binary has changed

[Denis PILAT]

Andreas Schwab wrote:
> You forgot to update the dependencies in the makefile.  Installed as
> obvious.
>
>   
Many thanks,

Denis