Re: [PATCH 1/2] gdb: Fix regression in varobj recreation

[Tom de Vries via Gdb-patches <gdb-patches@sourceware.org>]

On 8/2/22 14:47, Lancelot SIX wrote:
> "bc20e562ec0 gdb/varobj: Fix use after free in varobj" introduced a

I'd do:
...
Commit bc20e562ec0 "gdb/varobj: Fix use after free in varobj" introduced a
...
in other words, start the line with a word rather than a git hash, and 
do the quoting around "$subject".

[ Alternatively, you can use the 'fixes' style:
...
Commit bc20e562ec0 ("gdb/varobj: Fix use after free in varobj") introduced a
... ]

> regression.  This commit makes sure that the varobj object does not
> keeps stall references to object being freed when we unload an objfile.

stall -> stale	

> This includes the "valid_block" field which is reset to nullptr if the
> pointed to block is tied to an objfile being freed.
> 
> However, at some point varobj_invalidate_iter might try to recreate
> varobjs tracking either floating or globals.  Varobj tracking globals
> are identified as having the "valid_block" field set nullptr, but as
> bc20e562ec0 might clear this field, we have lost the ability to
> distinguish between varobj referring to globals and non globals.
> 
> Fix this by introducing a "global" flag which tracks if a given varobj
> was initially created as tracking a global.
> 

LGTM.

Thanks,
- Tom

> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=29426
> ---
>   gdb/varobj.c | 9 +++++++--
>   1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/gdb/varobj.c b/gdb/varobj.c
> index e558794617a..0683af1991e 100644
> --- a/gdb/varobj.c
> +++ b/gdb/varobj.c
> @@ -102,6 +102,9 @@ struct varobj_root
>        to symbols that do not exist anymore.  */
>     bool is_valid = true;
>   
> +  /* Set to true if the varobj was created as tracking a global.  */
> +  bool global = false;
> +
>     /* Language-related operations for this variable and its
>        children.  */
>     const struct lang_varobj_ops *lang_ops = NULL;
> @@ -336,6 +339,8 @@ varobj_create (const char *objname,
>         var->format = variable_default_display (var.get ());
>         var->root->valid_block =
>   	var->root->floating ? NULL : tracker.block ();
> +      var->root->global
> +	= var->root->floating ? false : var->root->valid_block == nullptr;
>         var->name = expression;
>         /* For a root var, the name and the expr are the same.  */
>         var->path_expr = expression;
> @@ -2359,7 +2364,7 @@ static void
>   varobj_invalidate_iter (struct varobj *var)
>   {
>     /* global and floating var must be re-evaluated.  */
> -  if (var->root->floating || var->root->valid_block == nullptr)
> +  if (var->root->floating || var->root->global)
>       {
>         struct varobj *tmp_var;
>   
> @@ -2375,7 +2380,7 @@ varobj_invalidate_iter (struct varobj *var)
>   	  varobj_delete (var, 0);
>   	  install_variable (tmp_var);
>   	}
> -      else if (!var->root->floating)
> +      else if (var->root->global)
>   	{
>   	  /* Only invalidate globals as floating vars might still be valid in
>   	     some other frame.  */