[1/2] Fix varobj updation after symbol removal

This problem was observed while loading and unloading symbols using 
add-symbol-file and remove-symbol-file. When remove-symbol-file 
command is invoked, it calls clear_symtab_users that calls varobj_invalidate 
to invalidate variable objects. This function invalidates the varobjs 
that are tied to locals and re-create the ones that are defined on 
globals. During this re-creation of globals, variable objects are 
re-evaluated that can result in new value. But this change is not recorded
and because of this, -var-update for such modified variable objects
gives empty change list.

Proposed Fix:

ping.

-Taimoor

On 06/27/2014 03:13 PM, Taimoor Mirza wrote:
> This problem was observed while loading and unloading symbols using
> add-symbol-file and remove-symbol-file. When remove-symbol-file
> command is invoked, it calls clear_symtab_users that calls varobj_invalidate
> to invalidate variable objects. This function invalidates the varobjs
> that are tied to locals and re-create the ones that are defined on
> globals. During this re-creation of globals, variable objects are
> re-evaluated that can result in new value. But this change is not recorded
> and because of this, -var-update for such modified variable objects
> gives empty change list.
>
> Proposed Fix:
> =============
> GDB has mechanism of marking varobj's updated if they are set via
> varobj_set_value operation. This allows any next -var-update to report
> this change. Same approach should be used during varobj invalidation.
> If value of newly created varobj is different from previous one, mark it
> updated so that -var-update can get this change.
>
> Variable object invalidation code is cleaned up to avoid using pointers
> whose target has been already freed.
>
> Fix Testing:
> ===========
> This fix has been regression tested on both simulator and real boards.
>
> 2014-06-27  Taimoor Mirza  <tmirza@codesourcery.com>
>          Maciej W. Rozycki  <macro@codesourcery.com>
>
>          gdb/
>          * varobj.h (varobj_is_valid_p, varobj_set_invalid): New
>          prototypes.
>          * varobj.c (varobj_is_valid_p, varobj_set_invalid): New functions.
>          (varobj_invalidate_iter): Mark re-created global object updated
>          if its value is different from previous value.
>          * objfiles.c (invalidate_objfile_varobj_type_iter): New function.
>          (free_objfile): Call it.
>
> ---
>   gdb/objfiles.c |   19 +++++++++++++++++++
>   gdb/varobj.c   |   37 +++++++++++++++++++++++++++++++++++++
>   gdb/varobj.h   |    3 +++
>   3 files changed, 59 insertions(+)
>
> diff --git a/gdb/objfiles.c b/gdb/objfiles.c
> index 0a0b1cb..03559a3 100644
> --- a/gdb/objfiles.c
> +++ b/gdb/objfiles.c
> @@ -32,6 +32,7 @@
>   #include "bcache.h"
>   #include "expression.h"
>   #include "parser-defs.h"
> +#include "varobj.h"
>
>   #include "gdb_assert.h"
>   #include <sys/types.h>
> @@ -538,6 +539,21 @@ free_objfile_separate_debug (struct objfile *objfile)
>       }
>   }
>
> +/* Mark the variable object VAR invalid if built upon a type coming from
> +   the objfile requested, passed as DATA.  Also clear the type reference.  */
> +
> +static void
> +invalidate_objfile_varobj_type_iter (struct varobj *var, void *data)
> +{
> +  struct objfile *objfile = data;
> +
> +  if (varobj_is_valid_p (var) && TYPE_OBJFILE (var->type) == objfile)
> +    {
> +      varobj_set_invalid (var);
> +      var->type = NULL;
> +    }
> +}
> +
>   /* Destroy an objfile and all the symtabs and psymtabs under it.  */
>
>   void
> @@ -584,6 +600,9 @@ free_objfile (struct objfile *objfile)
>        lists.  */
>     preserve_values (objfile);
>
> +  /* Varobj may refer to types stored in objfile's obstack.  */
> +  all_root_varobjs (invalidate_objfile_varobj_type_iter, objfile);
> +
>     /* It still may reference data modules have associated with the objfile and
>        the symbol file data.  */
>     forget_cached_source_info_for_objfile (objfile);
> diff --git a/gdb/varobj.c b/gdb/varobj.c
> index 7446f10..2a563af 100644
> --- a/gdb/varobj.c
> +++ b/gdb/varobj.c
> @@ -2683,6 +2683,22 @@ varobj_floating_p (struct varobj *var)
>     return var->root->floating;
>   }
>
> +/* Get the valid flag of varobj VAR.  */
> +
> +int
> +varobj_is_valid_p (struct varobj *var)
> +{
> +  return var->root->is_valid;
> +}
> +
> +/* Clear the valid flag on varobj VAR.  */
> +
> +void
> +varobj_set_invalid (struct varobj *var)
> +{
> +  var->root->is_valid = 0;
> +}
> +
>   /* Implement the "value_is_changeable_p" varobj callback for most
>      languages.  */
>
> @@ -2762,6 +2778,7 @@ varobj_invalidate_iter (struct varobj *var, void *unused)
>     if (var->root->floating || var->root->valid_block == NULL)
>       {
>         struct varobj *tmp_var;
> +      char *tmp_var_value, *var_value;
>
>         /* Try to create a varobj with same expression.  If we succeed
>   	 replace the old varobj, otherwise invalidate it.  */
> @@ -2770,6 +2787,26 @@ varobj_invalidate_iter (struct varobj *var, void *unused)
>         if (tmp_var != NULL)
>   	{
>   	  tmp_var->obj_name = xstrdup (var->obj_name);
> +	  tmp_var_value = varobj_get_value (tmp_var);
> +	  var_value = varobj_get_value (var);
> +
> +	  /* As varobjs are re-evaluated during creation so there is a
> +	     chance that new value is different from old one.  Compare
> +	     value of old varobj and newly created varobj and mark
> +	     varobj updated If new value is different.  */
> +	  if (var_value == NULL && tmp_var_value == NULL)
> +	    ; /* Equal.  */
> +	  else if (var_value == NULL || tmp_var_value == NULL)
> +	    tmp_var->updated = 1;
> +	  else
> +	    {
> +	      /* Mark tmp_var updated if new value is different.  */
> +	      if (strcmp (tmp_var_value, var_value) != 0)
> +		tmp_var->updated = 1;
> +	    }
> +
> +	  xfree (tmp_var_value);
> +	  xfree (var_value);
>   	  varobj_delete (var, NULL, 0);
>   	  install_variable (tmp_var);
>   	}
> diff --git a/gdb/varobj.h b/gdb/varobj.h
> index 74d41cf..7439a94 100644
> --- a/gdb/varobj.h
> +++ b/gdb/varobj.h
> @@ -299,6 +299,9 @@ extern int varobj_editable_p (struct varobj *var);
>
>   extern int varobj_floating_p (struct varobj *var);
>
> +extern int varobj_is_valid_p (struct varobj *var);
> +extern void varobj_set_invalid (struct varobj *var);
> +
>   extern void varobj_set_visualizer (struct varobj *var,
>   				   const char *visualizer);
>
>

Ping.

On 06/27/2014 03:13 PM, Taimoor Mirza wrote:
> This problem was observed while loading and unloading symbols using
> add-symbol-file and remove-symbol-file. When remove-symbol-file
> command is invoked, it calls clear_symtab_users that calls varobj_invalidate
> to invalidate variable objects. This function invalidates the varobjs
> that are tied to locals and re-create the ones that are defined on
> globals. During this re-creation of globals, variable objects are
> re-evaluated that can result in new value. But this change is not recorded
> and because of this, -var-update for such modified variable objects
> gives empty change list.
>
> Proposed Fix:
> =============
> GDB has mechanism of marking varobj's updated if they are set via
> varobj_set_value operation. This allows any next -var-update to report
> this change. Same approach should be used during varobj invalidation.
> If value of newly created varobj is different from previous one, mark it
> updated so that -var-update can get this change.
>
> Variable object invalidation code is cleaned up to avoid using pointers
> whose target has been already freed.
>
> Fix Testing:
> ===========
> This fix has been regression tested on both simulator and real boards.
>
> 2014-06-27  Taimoor Mirza  <tmirza@codesourcery.com>
>          Maciej W. Rozycki  <macro@codesourcery.com>
>
>          gdb/
>          * varobj.h (varobj_is_valid_p, varobj_set_invalid): New
>          prototypes.
>          * varobj.c (varobj_is_valid_p, varobj_set_invalid): New functions.
>          (varobj_invalidate_iter): Mark re-created global object updated
>          if its value is different from previous value.
>          * objfiles.c (invalidate_objfile_varobj_type_iter): New function.
>          (free_objfile): Call it.
>
> ---
>   gdb/objfiles.c |   19 +++++++++++++++++++
>   gdb/varobj.c   |   37 +++++++++++++++++++++++++++++++++++++
>   gdb/varobj.h   |    3 +++
>   3 files changed, 59 insertions(+)
>
> diff --git a/gdb/objfiles.c b/gdb/objfiles.c
> index 0a0b1cb..03559a3 100644
> --- a/gdb/objfiles.c
> +++ b/gdb/objfiles.c
> @@ -32,6 +32,7 @@
>   #include "bcache.h"
>   #include "expression.h"
>   #include "parser-defs.h"
> +#include "varobj.h"
>
>   #include "gdb_assert.h"
>   #include <sys/types.h>
> @@ -538,6 +539,21 @@ free_objfile_separate_debug (struct objfile *objfile)
>       }
>   }
>
> +/* Mark the variable object VAR invalid if built upon a type coming from
> +   the objfile requested, passed as DATA.  Also clear the type reference.  */
> +
> +static void
> +invalidate_objfile_varobj_type_iter (struct varobj *var, void *data)
> +{
> +  struct objfile *objfile = data;
> +
> +  if (varobj_is_valid_p (var) && TYPE_OBJFILE (var->type) == objfile)
> +    {
> +      varobj_set_invalid (var);
> +      var->type = NULL;
> +    }
> +}
> +
>   /* Destroy an objfile and all the symtabs and psymtabs under it.  */
>
>   void
> @@ -584,6 +600,9 @@ free_objfile (struct objfile *objfile)
>        lists.  */
>     preserve_values (objfile);
>
> +  /* Varobj may refer to types stored in objfile's obstack.  */
> +  all_root_varobjs (invalidate_objfile_varobj_type_iter, objfile);
> +
>     /* It still may reference data modules have associated with the objfile and
>        the symbol file data.  */
>     forget_cached_source_info_for_objfile (objfile);
> diff --git a/gdb/varobj.c b/gdb/varobj.c
> index 7446f10..2a563af 100644
> --- a/gdb/varobj.c
> +++ b/gdb/varobj.c
> @@ -2683,6 +2683,22 @@ varobj_floating_p (struct varobj *var)
>     return var->root->floating;
>   }
>
> +/* Get the valid flag of varobj VAR.  */
> +
> +int
> +varobj_is_valid_p (struct varobj *var)
> +{
> +  return var->root->is_valid;
> +}
> +
> +/* Clear the valid flag on varobj VAR.  */
> +
> +void
> +varobj_set_invalid (struct varobj *var)
> +{
> +  var->root->is_valid = 0;
> +}
> +
>   /* Implement the "value_is_changeable_p" varobj callback for most
>      languages.  */
>
> @@ -2762,6 +2778,7 @@ varobj_invalidate_iter (struct varobj *var, void *unused)
>     if (var->root->floating || var->root->valid_block == NULL)
>       {
>         struct varobj *tmp_var;
> +      char *tmp_var_value, *var_value;
>
>         /* Try to create a varobj with same expression.  If we succeed
>   	 replace the old varobj, otherwise invalidate it.  */
> @@ -2770,6 +2787,26 @@ varobj_invalidate_iter (struct varobj *var, void *unused)
>         if (tmp_var != NULL)
>   	{
>   	  tmp_var->obj_name = xstrdup (var->obj_name);
> +	  tmp_var_value = varobj_get_value (tmp_var);
> +	  var_value = varobj_get_value (var);
> +
> +	  /* As varobjs are re-evaluated during creation so there is a
> +	     chance that new value is different from old one.  Compare
> +	     value of old varobj and newly created varobj and mark
> +	     varobj updated If new value is different.  */
> +	  if (var_value == NULL && tmp_var_value == NULL)
> +	    ; /* Equal.  */
> +	  else if (var_value == NULL || tmp_var_value == NULL)
> +	    tmp_var->updated = 1;
> +	  else
> +	    {
> +	      /* Mark tmp_var updated if new value is different.  */
> +	      if (strcmp (tmp_var_value, var_value) != 0)
> +		tmp_var->updated = 1;
> +	    }
> +
> +	  xfree (tmp_var_value);
> +	  xfree (var_value);
>   	  varobj_delete (var, NULL, 0);
>   	  install_variable (tmp_var);
>   	}
> diff --git a/gdb/varobj.h b/gdb/varobj.h
> index 74d41cf..7439a94 100644
> --- a/gdb/varobj.h
> +++ b/gdb/varobj.h
> @@ -299,6 +299,9 @@ extern int varobj_editable_p (struct varobj *var);
>
>   extern int varobj_floating_p (struct varobj *var);
>
> +extern int varobj_is_valid_p (struct varobj *var);
> +extern void varobj_set_invalid (struct varobj *var);
> +
>   extern void varobj_set_visualizer (struct varobj *var,
>   				   const char *visualizer);
>
>

Ping

On 08/05/2014 04:00 PM, Taimoor wrote:
>
> Ping.
>
> On 06/27/2014 03:13 PM, Taimoor Mirza wrote:
>> This problem was observed while loading and unloading symbols using
>> add-symbol-file and remove-symbol-file. When remove-symbol-file
>> command is invoked, it calls clear_symtab_users that calls
>> varobj_invalidate
>> to invalidate variable objects. This function invalidates the varobjs
>> that are tied to locals and re-create the ones that are defined on
>> globals. During this re-creation of globals, variable objects are
>> re-evaluated that can result in new value. But this change is not
>> recorded
>> and because of this, -var-update for such modified variable objects
>> gives empty change list.
>>
>> Proposed Fix:
>> =============
>> GDB has mechanism of marking varobj's updated if they are set via
>> varobj_set_value operation. This allows any next -var-update to report
>> this change. Same approach should be used during varobj invalidation.
>> If value of newly created varobj is different from previous one, mark it
>> updated so that -var-update can get this change.
>>
>> Variable object invalidation code is cleaned up to avoid using pointers
>> whose target has been already freed.
>>
>> Fix Testing:
>> ===========
>> This fix has been regression tested on both simulator and real boards.
>>
>> 2014-06-27  Taimoor Mirza  <tmirza@codesourcery.com>
>>          Maciej W. Rozycki  <macro@codesourcery.com>
>>
>>          gdb/
>>          * varobj.h (varobj_is_valid_p, varobj_set_invalid): New
>>          prototypes.
>>          * varobj.c (varobj_is_valid_p, varobj_set_invalid): New
>> functions.
>>          (varobj_invalidate_iter): Mark re-created global object updated
>>          if its value is different from previous value.
>>          * objfiles.c (invalidate_objfile_varobj_type_iter): New
>> function.
>>          (free_objfile): Call it.
>>
>> ---
>>   gdb/objfiles.c |   19 +++++++++++++++++++
>>   gdb/varobj.c   |   37 +++++++++++++++++++++++++++++++++++++
>>   gdb/varobj.h   |    3 +++
>>   3 files changed, 59 insertions(+)
>>
>> diff --git a/gdb/objfiles.c b/gdb/objfiles.c
>> index 0a0b1cb..03559a3 100644
>> --- a/gdb/objfiles.c
>> +++ b/gdb/objfiles.c
>> @@ -32,6 +32,7 @@
>>   #include "bcache.h"
>>   #include "expression.h"
>>   #include "parser-defs.h"
>> +#include "varobj.h"
>>
>>   #include "gdb_assert.h"
>>   #include <sys/types.h>
>> @@ -538,6 +539,21 @@ free_objfile_separate_debug (struct objfile
>> *objfile)
>>       }
>>   }
>>
>> +/* Mark the variable object VAR invalid if built upon a type coming from
>> +   the objfile requested, passed as DATA.  Also clear the type
>> reference.  */
>> +
>> +static void
>> +invalidate_objfile_varobj_type_iter (struct varobj *var, void *data)
>> +{
>> +  struct objfile *objfile = data;
>> +
>> +  if (varobj_is_valid_p (var) && TYPE_OBJFILE (var->type) == objfile)
>> +    {
>> +      varobj_set_invalid (var);
>> +      var->type = NULL;
>> +    }
>> +}
>> +
>>   /* Destroy an objfile and all the symtabs and psymtabs under it.  */
>>
>>   void
>> @@ -584,6 +600,9 @@ free_objfile (struct objfile *objfile)
>>        lists.  */
>>     preserve_values (objfile);
>>
>> +  /* Varobj may refer to types stored in objfile's obstack.  */
>> +  all_root_varobjs (invalidate_objfile_varobj_type_iter, objfile);
>> +
>>     /* It still may reference data modules have associated with the
>> objfile and
>>        the symbol file data.  */
>>     forget_cached_source_info_for_objfile (objfile);
>> diff --git a/gdb/varobj.c b/gdb/varobj.c
>> index 7446f10..2a563af 100644
>> --- a/gdb/varobj.c
>> +++ b/gdb/varobj.c
>> @@ -2683,6 +2683,22 @@ varobj_floating_p (struct varobj *var)
>>     return var->root->floating;
>>   }
>>
>> +/* Get the valid flag of varobj VAR.  */
>> +
>> +int
>> +varobj_is_valid_p (struct varobj *var)
>> +{
>> +  return var->root->is_valid;
>> +}
>> +
>> +/* Clear the valid flag on varobj VAR.  */
>> +
>> +void
>> +varobj_set_invalid (struct varobj *var)
>> +{
>> +  var->root->is_valid = 0;
>> +}
>> +
>>   /* Implement the "value_is_changeable_p" varobj callback for most
>>      languages.  */
>>
>> @@ -2762,6 +2778,7 @@ varobj_invalidate_iter (struct varobj *var, void
>> *unused)
>>     if (var->root->floating || var->root->valid_block == NULL)
>>       {
>>         struct varobj *tmp_var;
>> +      char *tmp_var_value, *var_value;
>>
>>         /* Try to create a varobj with same expression.  If we succeed
>>        replace the old varobj, otherwise invalidate it.  */
>> @@ -2770,6 +2787,26 @@ varobj_invalidate_iter (struct varobj *var,
>> void *unused)
>>         if (tmp_var != NULL)
>>       {
>>         tmp_var->obj_name = xstrdup (var->obj_name);
>> +      tmp_var_value = varobj_get_value (tmp_var);
>> +      var_value = varobj_get_value (var);
>> +
>> +      /* As varobjs are re-evaluated during creation so there is a
>> +         chance that new value is different from old one.  Compare
>> +         value of old varobj and newly created varobj and mark
>> +         varobj updated If new value is different.  */
>> +      if (var_value == NULL && tmp_var_value == NULL)
>> +        ; /* Equal.  */
>> +      else if (var_value == NULL || tmp_var_value == NULL)
>> +        tmp_var->updated = 1;
>> +      else
>> +        {
>> +          /* Mark tmp_var updated if new value is different.  */
>> +          if (strcmp (tmp_var_value, var_value) != 0)
>> +        tmp_var->updated = 1;
>> +        }
>> +
>> +      xfree (tmp_var_value);
>> +      xfree (var_value);
>>         varobj_delete (var, NULL, 0);
>>         install_variable (tmp_var);
>>       }
>> diff --git a/gdb/varobj.h b/gdb/varobj.h
>> index 74d41cf..7439a94 100644
>> --- a/gdb/varobj.h
>> +++ b/gdb/varobj.h
>> @@ -299,6 +299,9 @@ extern int varobj_editable_p (struct varobj *var);
>>
>>   extern int varobj_floating_p (struct varobj *var);
>>
>> +extern int varobj_is_valid_p (struct varobj *var);
>> +extern void varobj_set_invalid (struct varobj *var);
>> +
>>   extern void varobj_set_visualizer (struct varobj *var,
>>                      const char *visualizer);
>>
>>