Patchwork [1/2] Fix varobj updation after symbol removal

login
register
mail settings
Submitter Raza, Saqlain
Date Oct. 17, 2019, 10:03 a.m.
Message ID <1571306592-24472-2-git-send-email-Saqlain_Raza@mentor.com>
Download mbox | patch
Permalink /patch/35086/
State New
Headers show

Comments

Raza, Saqlain - Oct. 17, 2019, 10:03 a.m.
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:
Raza, Saqlain - Oct. 22, 2019, 6:46 a.m.
Ping !

Thanks,
Saqlain

On 10/17/19 3:03 PM, Raza, Saqlain 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
> for x86_64 and arm-none-linux-gnueabi targets.
>
> 2019-10-17  Saqlain Raza  <saqlain_raza@mentor.com>
>              Taimoor Mirza  <taimoor_mirza@mentor.com>
>              Maciej W. Rozycki  <macro@codesourcery.com>
>
>          gdb/
>          * objfiles.c: Include varobj.h.
>          (invalidate_objfile_varobj_type_iter): New function.
>          (free_objfile): Call it.
>          * 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.
>          * varobj.h (varobj_is_valid_p, varobj_set_invalid): New
>          prototypes.
>
> Signed-off-by: Raza, Saqlain <Saqlain_Raza@mentor.com>
> ---
>   gdb/ChangeLog  | 13 +++++++++++++
>   gdb/objfiles.c | 19 +++++++++++++++++++
>   gdb/varobj.c   | 35 +++++++++++++++++++++++++++++++++++
>   gdb/varobj.h   |  4 ++++
>   4 files changed, 71 insertions(+)
>
> diff --git a/gdb/ChangeLog b/gdb/ChangeLog
> index ced508f..b92da1b 100644
> --- a/gdb/ChangeLog
> +++ b/gdb/ChangeLog
> @@ -1,3 +1,16 @@
> +2019-10-17  Saqlain Raza  <saqlain_raza@mentor.com>
> +	    Taimoor Mirza  <taimoor_mirza@mentor.com>
> +	    Maciej W. Rozycki  <macro@codesourcery.com>
> +
> +	* objfiles.c: Include varobj.h.
> +	(invalidate_objfile_varobj_type_iter): New function.
> +	(free_objfile): Call it.
> +	* 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.
> +	* varobj.h (varobj_is_valid_p, varobj_set_invalid): New
> +	prototypes.
> +
>   2019-10-16  Tom Tromey  <tom@tromey.com>
>   
>   	* objfiles.h (struct objfile) <original_name>: Now const.
> diff --git a/gdb/objfiles.c b/gdb/objfiles.c
> index f9e7d20..8704814 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 <sys/types.h>
>   #include <sys/stat.h>
> @@ -568,6 +569,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 = (struct 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.  */
>   
>   objfile::~objfile ()
> @@ -613,6 +629,9 @@ objfile::~objfile ()
>        lists.  */
>     preserve_values (this);
>   
> +  /* Varobj may refer to types stored in objfile's obstack.  */
> +  all_root_varobjs (invalidate_objfile_varobj_type_iter, this);
> +
>     /* It still may reference data modules have associated with the objfile and
>        the symbol file data.  */
>     forget_cached_source_info_for_objfile (this);
> diff --git a/gdb/varobj.c b/gdb/varobj.c
> index 37a522b..f7696e7 100644
> --- a/gdb/varobj.c
> +++ b/gdb/varobj.c
> @@ -2431,6 +2431,22 @@ varobj_floating_p (const 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.  */
>   
> @@ -2492,6 +2508,7 @@ varobj_invalidate_iter (struct varobj *var, void *unused)
>     if (var->root->floating || var->root->valid_block == NULL)
>       {
>         struct varobj *tmp_var;
> +      std::string tmp_var_value, var_value;
>   
>         /* Try to create a varobj with same expression.  If we succeed
>   	 replace the old varobj, otherwise invalidate it.  */
> @@ -2500,6 +2517,24 @@ varobj_invalidate_iter (struct varobj *var, void *unused)
>         if (tmp_var != NULL)
>   	{
>   	  tmp_var->obj_name = var->obj_name;
> +         tmp_var_value = varobj_get_value (tmp_var);
> +         var_value = varobj_get_value (var);
> +
> +         /* Since varobjs are re-evaluated during creation, there is a
> +            chance the new value is different from old one.  Compare
> +            old varobj and the newly created varobj and mark varobj
> +            updated ff new value is different.  */
> +         if (var_value.empty() && tmp_var_value.empty())
> +           ; /* Equal.  */
> +         else if (var_value.empty() || tmp_var_value.empty())
> +           tmp_var->updated = 1;
> +         else
> +           {
> +             /* Mark tmp_var updated if the new value is different.  */
> +             if (tmp_var_value != var_value)
> +               tmp_var->updated = 1;
> +           }
> +
>   	  varobj_delete (var, 0);
>   	  install_variable (tmp_var);
>   	}
> diff --git a/gdb/varobj.h b/gdb/varobj.h
> index 66db780..0885f70 100644
> --- a/gdb/varobj.h
> +++ b/gdb/varobj.h
> @@ -323,6 +323,10 @@ extern bool varobj_editable_p (const struct varobj *var);
>   
>   extern bool varobj_floating_p (const 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);
>

Patch

=============
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
for x86_64 and arm-none-linux-gnueabi targets.

2019-10-17  Saqlain Raza  <saqlain_raza@mentor.com>
            Taimoor Mirza  <taimoor_mirza@mentor.com>
            Maciej W. Rozycki  <macro@codesourcery.com>

        gdb/
        * objfiles.c: Include varobj.h.
        (invalidate_objfile_varobj_type_iter): New function.
        (free_objfile): Call it.
        * 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.
        * varobj.h (varobj_is_valid_p, varobj_set_invalid): New
        prototypes.

Signed-off-by: Raza, Saqlain <Saqlain_Raza@mentor.com>
---
 gdb/ChangeLog  | 13 +++++++++++++
 gdb/objfiles.c | 19 +++++++++++++++++++
 gdb/varobj.c   | 35 +++++++++++++++++++++++++++++++++++
 gdb/varobj.h   |  4 ++++
 4 files changed, 71 insertions(+)

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index ced508f..b92da1b 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,16 @@ 
+2019-10-17  Saqlain Raza  <saqlain_raza@mentor.com>
+	    Taimoor Mirza  <taimoor_mirza@mentor.com>
+	    Maciej W. Rozycki  <macro@codesourcery.com>
+
+	* objfiles.c: Include varobj.h.
+	(invalidate_objfile_varobj_type_iter): New function.
+	(free_objfile): Call it.
+	* 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.
+	* varobj.h (varobj_is_valid_p, varobj_set_invalid): New
+	prototypes.
+
 2019-10-16  Tom Tromey  <tom@tromey.com>
 
 	* objfiles.h (struct objfile) <original_name>: Now const.
diff --git a/gdb/objfiles.c b/gdb/objfiles.c
index f9e7d20..8704814 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 <sys/types.h>
 #include <sys/stat.h>
@@ -568,6 +569,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 = (struct 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.  */
 
 objfile::~objfile ()
@@ -613,6 +629,9 @@  objfile::~objfile ()
      lists.  */
   preserve_values (this);
 
+  /* Varobj may refer to types stored in objfile's obstack.  */
+  all_root_varobjs (invalidate_objfile_varobj_type_iter, this);
+
   /* It still may reference data modules have associated with the objfile and
      the symbol file data.  */
   forget_cached_source_info_for_objfile (this);
diff --git a/gdb/varobj.c b/gdb/varobj.c
index 37a522b..f7696e7 100644
--- a/gdb/varobj.c
+++ b/gdb/varobj.c
@@ -2431,6 +2431,22 @@  varobj_floating_p (const 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.  */
 
@@ -2492,6 +2508,7 @@  varobj_invalidate_iter (struct varobj *var, void *unused)
   if (var->root->floating || var->root->valid_block == NULL)
     {
       struct varobj *tmp_var;
+      std::string tmp_var_value, var_value;
 
       /* Try to create a varobj with same expression.  If we succeed
 	 replace the old varobj, otherwise invalidate it.  */
@@ -2500,6 +2517,24 @@  varobj_invalidate_iter (struct varobj *var, void *unused)
       if (tmp_var != NULL) 
 	{ 
 	  tmp_var->obj_name = var->obj_name;
+         tmp_var_value = varobj_get_value (tmp_var);
+         var_value = varobj_get_value (var);
+
+         /* Since varobjs are re-evaluated during creation, there is a
+            chance the new value is different from old one.  Compare
+            old varobj and the newly created varobj and mark varobj
+            updated ff new value is different.  */
+         if (var_value.empty() && tmp_var_value.empty())
+           ; /* Equal.  */
+         else if (var_value.empty() || tmp_var_value.empty())
+           tmp_var->updated = 1;
+         else
+           {
+             /* Mark tmp_var updated if the new value is different.  */
+             if (tmp_var_value != var_value)
+               tmp_var->updated = 1;
+           }
+
 	  varobj_delete (var, 0);
 	  install_variable (tmp_var);
 	}
diff --git a/gdb/varobj.h b/gdb/varobj.h
index 66db780..0885f70 100644
--- a/gdb/varobj.h
+++ b/gdb/varobj.h
@@ -323,6 +323,10 @@  extern bool varobj_editable_p (const struct varobj *var);
 
 extern bool varobj_floating_p (const 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);