Re: [PATCH 3/3] gdb/varobj: Only re-evaluate invalid globals during re_set

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

On 8/4/22 15:02, Lancelot SIX wrote:
> When doing varobj_re_set, we currently try to recreate floating varobj.
> This was introduced by 4e969b4f0128 "Re-evaluate floating varobj as part
> of varobj_invalidate" to deal with use a after free issue.  However
> since bc20e562ec0 "Fix use after free in varobj" we now ensure that we
> never have dangling pointers so this all recreation is not strictly
> needed anymore for floating varobjs.
> 
> This commit proposes to remove this recreation process for floating
> varobjs.
> 
> Tested on x86_64-linux.

LGTM.

Thanks,
- Tom

> ---
>   gdb/testsuite/gdb.mi/mi-var-invalidate.exp |  4 ++--
>   gdb/varobj.c                               | 18 +++++-------------
>   2 files changed, 7 insertions(+), 15 deletions(-)
> 
> diff --git a/gdb/testsuite/gdb.mi/mi-var-invalidate.exp b/gdb/testsuite/gdb.mi/mi-var-invalidate.exp
> index 1b2c68df18e..348515671c1 100644
> --- a/gdb/testsuite/gdb.mi/mi-var-invalidate.exp
> +++ b/gdb/testsuite/gdb.mi/mi-var-invalidate.exp
> @@ -75,8 +75,8 @@ mi_runto_main
>   # Change format of floating variable immediately after reload reveals a
>   # bug where gdb still uses a free'd pointer.
>   mi_gdb_test "-var-set-format float_simple hexadecimal" \
> -        "\\^done,format=\"hexadecimal\",value=\"\\\[-1\\\]\"" \
> -       "set format variable float_simple"
> +	"\\^done,format=\"hexadecimal\",value=\"\\\[3\\\]\"" \
> +	"set format variable float_simple"
>   
>   # Check local variable is "invalid".
>   mi_gdb_test "-var-update linteger" \
> diff --git a/gdb/varobj.c b/gdb/varobj.c
> index 55a7bd97f43..d3df608c55f 100644
> --- a/gdb/varobj.c
> +++ b/gdb/varobj.c
> @@ -2359,29 +2359,21 @@ all_root_varobjs (gdb::function_view<void (struct varobj *var)> func)
>   static void
>   varobj_re_set_iter (struct varobj *var)
>   {
> -  /* Invalidated globals and floating var must be re-evaluated.  */
> -  if (var->root->global || var->root->floating)
> +  /* Invalidated global varobjs must be re-evaluated.  */
> +  if (!var->root->is_valid && var->root->global)
>       {
>         struct varobj *tmp_var;
>   
>         /* Try to create a varobj with same expression.  If we succeed
> -	 replace the old varobj, otherwise invalidate it.  */
> +	 and have a global replace the old varobj.  */
>         tmp_var = varobj_create (nullptr, var->name.c_str (), (CORE_ADDR) 0,
> -			       var->root->floating
> -			       ? USE_SELECTED_FRAME : USE_CURRENT_FRAME);
> -      if (tmp_var != nullptr)
> +			       USE_CURRENT_FRAME);
> +      if (tmp_var != nullptr && tmp_var->root->global)
>   	{
> -	  gdb_assert (var->root->floating == tmp_var->root->floating);
>   	  tmp_var->obj_name = var->obj_name;
>   	  varobj_delete (var, 0);
>   	  install_variable (tmp_var);
>   	}
> -      else if (var->root->global)
> -	{
> -	  /* Only invalidate globals as floating vars might still be valid in
> -	     some other frame.  */
> -	  var->root->is_valid = false;
> -	}
>       }
>   }
>