[PATCH 0/3] Varobj re_set clanup

[PATCH 0/3] Varobj re_set clanup

[Lancelot SIX]

This series is a follow-up to this series:
https://sourceware.org/pipermail/gdb-patches/2022-August/191172.html

Patch 1 of the original series has already been merged.

Patch 2 of the original series is replaced by patch 2 of this series which is
much cleaner.  Patch 1 and 3 are meant to simplify varobj lifecycle.

All feedbacks are welcome.
Best,
Lancelot.

Lancelot SIX (2):
  gdb/varobj: Do not invalidate locals in varobj_invalidate_iter
  gdb/varobj: Only re-evaluate invalid globals durin re_set

Tom de Vries (1):
  gdb/varobj: Reset varobj after relocations have been computed

 gdb/symfile.c                                 |  6 +--
 .../gdb.mi/mi-var-invalidate-shlib.exp        | 42 ++++++++++++++++---
 gdb/testsuite/gdb.mi/mi-var-invalidate.exp    |  4 +-
 gdb/varobj.c                                  | 38 +++++------------
 gdb/varobj.h                                  |  5 ++-
 5 files changed, 57 insertions(+), 38 deletions(-)


base-commit: f74a5e6f2ed5180ce28d6478a6d7a7a1982c5d43
-- 
2.34.1

[PATCH 1/3] gdb/varobj: Do not invalidate locals in varobj_invalidate_iter

[Lancelot SIX]

The varobj_invalidate_iter function has logic to invalidate any local
varobj it can find.  However since bc20e562ec0 "gdb/varobj: Fix use
after free in varobj" all varobj containing references to an objfile are
cleared when the objfile goes out of scope.  This means that at this
point any local varobj seen by varobj_invalidate_iter either has
already been invalidated by varobj_invalidate_if_uses_objfile or only
contains valid references and there is no reason to invalidate it.

This patch proposes to remove this un-necessary invalidation and adds a
testcase which exercises a scenario where a local varobj can legitimately
survive a call to varobj_invalidate_iter.

At this point the varobj_invalidate and varobj_invalidate seem misnamed
since they deal with re-creating invalid objects and do not do
invalidation, but this will be fixed in a following patch.

Tested on x86_64-linux.
---
 .../gdb.mi/mi-var-invalidate-shlib.exp        | 40 +++++++++++++++++--
 gdb/varobj.c                                  |  2 -
 2 files changed, 36 insertions(+), 6 deletions(-)

diff --git a/gdb/testsuite/gdb.mi/mi-var-invalidate-shlib.exp b/gdb/testsuite/gdb.mi/mi-var-invalidate-shlib.exp
index 87d1d4a6a9d..9ba5af0c028 100644
--- a/gdb/testsuite/gdb.mi/mi-var-invalidate-shlib.exp
+++ b/gdb/testsuite/gdb.mi/mi-var-invalidate-shlib.exp
@@ -40,10 +40,6 @@ if  { [gdb_compile "${srcdir}/${subdir}/${srcfile}" "${binfile}" executable $opt
 }
 
 proc do_test { separate_debuginfo } {
-    if { $separate_debuginfo } {
-	gdb_gnu_strip_debug $::shlib_path
-    }
-
     if { [mi_clean_restart] } {
 	unsupported "failed to start GDB"
 	return
@@ -132,6 +128,42 @@ proc do_test { separate_debuginfo } {
     }
 }
 
+proc_with_prefix local_not_invalidated { separate_debuginfo } {
+    if { [mi_clean_restart] } {
+	unsupported "failed to start GDB"
+	return
+    }
+
+    # Start the process once and create varobjs referencing the loaded objfiles.
+    with_test_prefix "setup" {
+	mi_load_shlibs $::shlib_path
+	if { $separate_debuginfo } {
+	    mi_load_shlibs ${::shlib_path}.debug
+	}
+
+	mi_gdb_reinitialize_dir $::srcdir/$::subdir
+	mi_gdb_load $::binfile
+
+	mi_runto foo -pending
+	mi_next "next"
+	mi_create_varobj local_var local_var "create local varobj"
+    }
+
+    # A this point we are stoped in the shared library.  If we reload symbols
+    # for the main binary, symbols for the shared library remain valid.
+    # A varobj tracking variables in the scope of the shared library only
+    # should not be invalidated.
+    mi_gdb_load ${::binfile}
+    mi_gdb_test "-var-update local_var" \
+	"\\^done,changelist=\\\[\\\]" \
+	"local_var preserved"
+}
+
 foreach_with_prefix separate_debuginfo {0 1} {
+    if { $separate_debuginfo } {
+	gdb_gnu_strip_debug $::shlib_path
+    }
+
     do_test $separate_debuginfo
+    local_not_invalidated $separate_debuginfo
 }
diff --git a/gdb/varobj.c b/gdb/varobj.c
index 0683af1991e..a142bb02e03 100644
--- a/gdb/varobj.c
+++ b/gdb/varobj.c
@@ -2387,8 +2387,6 @@ varobj_invalidate_iter (struct varobj *var)
 	  var->root->is_valid = false;
 	}
     }
-  else /* locals must be invalidated.  */
-    var->root->is_valid = false;
 }
 
 /* Invalidate the varobjs that are tied to locals and re-create the ones that
-- 
2.34.1

Re: [PATCH 1/3] gdb/varobj: Do not invalidate locals in varobj_invalidate_iter

[Tom de Vries]

On 8/4/22 15:02, Lancelot SIX wrote:
> The varobj_invalidate_iter function has logic to invalidate any local
> varobj it can find.  However since bc20e562ec0 "gdb/varobj: Fix use
> after free in varobj" all varobj containing references to an objfile are
> cleared when the objfile goes out of scope.  This means that at this
> point any local varobj seen by varobj_invalidate_iter either has
> already been invalidated by varobj_invalidate_if_uses_objfile or only
> contains valid references and there is no reason to invalidate it.
> 
> This patch proposes to remove this un-necessary invalidation and adds a

un-necessary - > unnecessary

> testcase which exercises a scenario where a local varobj can legitimately
> survive a call to varobj_invalidate_iter.
> 
> At this point the varobj_invalidate and varobj_invalidate seem misnamed

Missing _iter on the latter ?

> since they deal with re-creating invalid objects and do not do
> invalidation, but this will be fixed in a following patch.
> 
> Tested on x86_64-linux.
> ---
>   .../gdb.mi/mi-var-invalidate-shlib.exp        | 40 +++++++++++++++++--
>   gdb/varobj.c                                  |  2 -
>   2 files changed, 36 insertions(+), 6 deletions(-)
> 
> diff --git a/gdb/testsuite/gdb.mi/mi-var-invalidate-shlib.exp b/gdb/testsuite/gdb.mi/mi-var-invalidate-shlib.exp
> index 87d1d4a6a9d..9ba5af0c028 100644
> --- a/gdb/testsuite/gdb.mi/mi-var-invalidate-shlib.exp
> +++ b/gdb/testsuite/gdb.mi/mi-var-invalidate-shlib.exp
> @@ -40,10 +40,6 @@ if  { [gdb_compile "${srcdir}/${subdir}/${srcfile}" "${binfile}" executable $opt
>   }
>   
>   proc do_test { separate_debuginfo } {
> -    if { $separate_debuginfo } {
> -	gdb_gnu_strip_debug $::shlib_path
> -    }
> -
>       if { [mi_clean_restart] } {
>   	unsupported "failed to start GDB"
>   	return
> @@ -132,6 +128,42 @@ proc do_test { separate_debuginfo } {
>       }
>   }
>   
> +proc_with_prefix local_not_invalidated { separate_debuginfo } {
> +    if { [mi_clean_restart] } {
> +	unsupported "failed to start GDB"
> +	return
> +    }
> +
> +    # Start the process once and create varobjs referencing the loaded objfiles.
> +    with_test_prefix "setup" {
> +	mi_load_shlibs $::shlib_path
> +	if { $separate_debuginfo } {
> +	    mi_load_shlibs ${::shlib_path}.debug
> +	}
> +
> +	mi_gdb_reinitialize_dir $::srcdir/$::subdir
> +	mi_gdb_load $::binfile
> +
> +	mi_runto foo -pending
> +	mi_next "next"
> +	mi_create_varobj local_var local_var "create local varobj"
> +    }
> +
> +    # A this point we are stoped in the shared library.  If we reload symbols

stoped -> stopped.

Otherwise, LGTM.

Thanks,
- Tom

> +    # for the main binary, symbols for the shared library remain valid.
> +    # A varobj tracking variables in the scope of the shared library only
> +    # should not be invalidated.
> +    mi_gdb_load ${::binfile}
> +    mi_gdb_test "-var-update local_var" \
> +	"\\^done,changelist=\\\[\\\]" \
> +	"local_var preserved"
> +}
> +
>   foreach_with_prefix separate_debuginfo {0 1} {
> +    if { $separate_debuginfo } {
> +	gdb_gnu_strip_debug $::shlib_path
> +    }
> +
>       do_test $separate_debuginfo
> +    local_not_invalidated $separate_debuginfo
>   }
> diff --git a/gdb/varobj.c b/gdb/varobj.c
> index 0683af1991e..a142bb02e03 100644
> --- a/gdb/varobj.c
> +++ b/gdb/varobj.c
> @@ -2387,8 +2387,6 @@ varobj_invalidate_iter (struct varobj *var)
>   	  var->root->is_valid = false;
>   	}
>       }
> -  else /* locals must be invalidated.  */
> -    var->root->is_valid = false;
>   }
>   
>   /* Invalidate the varobjs that are tied to locals and re-create the ones that

[PATCH 2/3] gdb/varobj: Reset varobj after relocations have been computed

[Lancelot SIX]

From: Tom de Vries <tdevries@suse.de>

[This patch is a followup to the discussion in
https://sourceware.org/pipermail/gdb-patches/2022-August/191188.html]

PR/29426 shows failures when running the gdb.mi/mi-var-invalidate-shlib
test when using a compiler which does not produce a PIE executable by
default.

In the testcase, a varobj is created to track a global variable, and
then the main binary is reloaded in GDB (using the file command).

During the load of the new binary, GDB tries to recreate the varobj to
track the global in the new binary (varobj_invalidate_iter).  At this
point, the old process is still in flight.  So when we try to access to
the value of the global, in a PIE executable we only have access to the
unrelocated address (the objfile's text_section_offset () is 0).  As a
consequence down the line read_value_memory fails to read the unrelated
address, so cannot evaluate the value of the global.  Note that the
expression used to access to the global’s value is valid, so the varobj
can be created.  When using a non PIE executable, the address of the
global GDB knows about at this point does not need relocation, so
read_value_memory can access the (old binary’s) value.

So at this point, in the case of a non-PIE executable the value field is
set, while it is cleared in the case of PIE executable.  Later when the
test issues a "-var-update global_var", the command sees no change in
the case of the non-PIE executable, while in the case of the PIE
executable install_new_value sees that value changes, leading to a
different output.

This patch makes sure that, as we do for breakpoints, we wait until
relocation has happened before we try to recreate varobjs.  This way we
have a consistent behavior between PIE and non-PIE binaries.

Tested on x86_64-linux.

Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=29426
Co-authored-by: Lancelot SIX <lancelot.six@amd.com>
---
 gdb/symfile.c                                 |  6 ++---
 .../gdb.mi/mi-var-invalidate-shlib.exp        |  2 +-
 gdb/varobj.c                                  | 22 +++++++------------
 gdb/varobj.h                                  |  5 ++++-
 4 files changed, 16 insertions(+), 19 deletions(-)

diff --git a/gdb/symfile.c b/gdb/symfile.c
index aea8c762ee6..f61235dd271 100644
--- a/gdb/symfile.c
+++ b/gdb/symfile.c
@@ -1656,6 +1656,9 @@ symbol_file_command (const char *args, int from_tty)
 
       /* Now it's safe to re-add the breakpoints.  */
       breakpoint_re_set ();
+
+      /* Also, it's safe to re-add varobjs.  */
+      varobj_re_set ();
     }
 }
 
@@ -2870,9 +2873,6 @@ clear_symtab_users (symfile_add_flags add_flags)
   clear_pc_function_cache ();
   gdb::observers::new_objfile.notify (NULL);
 
-  /* Varobj may refer to old symbols, perform a cleanup.  */
-  varobj_invalidate ();
-
   /* Now that the various caches have been cleared, we can re_set
      our breakpoints without risking it using stale data.  */
   if ((add_flags & SYMFILE_DEFER_BP_RESET) == 0)
diff --git a/gdb/testsuite/gdb.mi/mi-var-invalidate-shlib.exp b/gdb/testsuite/gdb.mi/mi-var-invalidate-shlib.exp
index 9ba5af0c028..fdf80b79741 100644
--- a/gdb/testsuite/gdb.mi/mi-var-invalidate-shlib.exp
+++ b/gdb/testsuite/gdb.mi/mi-var-invalidate-shlib.exp
@@ -99,7 +99,7 @@ proc do_test { separate_debuginfo } {
 	# When reloading the symbol file, only the var for the global in the main
 	# executable is re-created.
 	mi_gdb_test "-var-update global_var" \
-	    "\\^done,changelist=\\\[{name=\"global_var\",in_scope=\"true\",type_changed=\"false\",has_more=\"0\"}\\\]" \
+	    "\\^done,changelist=\\\[\\\]" \
 	    "global_var recreated"
 	mi_gdb_test "-var-update global_shlib_var" \
 	    "\\^done,changelist=\\\[{name=\"global_shlib_var\",in_scope=\"invalid\",has_more=\"0\"}\\\]" \
diff --git a/gdb/varobj.c b/gdb/varobj.c
index a142bb02e03..55a7bd97f43 100644
--- a/gdb/varobj.c
+++ b/gdb/varobj.c
@@ -2353,18 +2353,14 @@ all_root_varobjs (gdb::function_view<void (struct varobj *var)> func)
     }
 }
 
-/* Invalidate varobj VAR if it is tied to locals and re-create it if it is
-   defined on globals.  It is a helper for varobj_invalidate.
-
-   This function is called after changing the symbol file, in this case the
-   pointers to "struct type" stored by the varobj are no longer valid.  All
-   varobj must be either re-evaluated, or marked as invalid here.  */
+/* Try to recreate the varobj VAR if it is a global or floating.  This is a
+   helper function for varobj_re_set.  */
 
 static void
-varobj_invalidate_iter (struct varobj *var)
+varobj_re_set_iter (struct varobj *var)
 {
-  /* global and floating var must be re-evaluated.  */
-  if (var->root->floating || var->root->global)
+  /* Invalidated globals and floating var must be re-evaluated.  */
+  if (var->root->global || var->root->floating)
     {
       struct varobj *tmp_var;
 
@@ -2389,14 +2385,12 @@ varobj_invalidate_iter (struct varobj *var)
     }
 }
 
-/* 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".  */
+/* See varobj.h.  */
 
 void 
-varobj_invalidate (void)
+varobj_re_set (void)
 {
-  all_root_varobjs (varobj_invalidate_iter);
+  all_root_varobjs (varobj_re_set_iter);
 }
 
 /* Ensure that no varobj keep references to OBJFILE.  */
diff --git a/gdb/varobj.h b/gdb/varobj.h
index 073da60d772..0c92d67e4f4 100644
--- a/gdb/varobj.h
+++ b/gdb/varobj.h
@@ -314,7 +314,10 @@ extern void all_root_varobjs (gdb::function_view<void (struct varobj *var)>);
 extern std::vector<varobj_update_result>
   varobj_update (struct varobj **varp, bool is_explicit);
 
-extern void varobj_invalidate (void);
+/* Try to recreate any global or floating varobj.  This is called after
+   changing symbol files.  */
+
+extern void varobj_re_set (void);
 
 extern bool varobj_editable_p (const struct varobj *var);
 
-- 
2.34.1

Re: [PATCH 2/3] gdb/varobj: Reset varobj after relocations have been computed

[Tom de Vries]

On 8/4/22 15:02, Lancelot SIX wrote:
> From: Tom de Vries <tdevries@suse.de>
> 
> [This patch is a followup to the discussion in
> https://sourceware.org/pipermail/gdb-patches/2022-August/191188.html]
> 
> PR/29426 shows failures when running the gdb.mi/mi-var-invalidate-shlib
> test when using a compiler which does not produce a PIE executable by
> default.
> 
> In the testcase, a varobj is created to track a global variable, and
> then the main binary is reloaded in GDB (using the file command).
> 
> During the load of the new binary, GDB tries to recreate the varobj to
> track the global in the new binary (varobj_invalidate_iter).  At this
> point, the old process is still in flight.  So when we try to access to
> the value of the global, in a PIE executable we only have access to the
> unrelocated address (the objfile's text_section_offset () is 0).  As a
> consequence down the line read_value_memory fails to read the unrelated
> address, so cannot evaluate the value of the global.  Note that the
> expression used to access to the global’s value is valid, so the varobj
> can be created.  When using a non PIE executable, the address of the
> global GDB knows about at this point does not need relocation, so
> read_value_memory can access the (old binary’s) value.
> 
> So at this point, in the case of a non-PIE executable the value field is
> set, while it is cleared in the case of PIE executable.  Later when the
> test issues a "-var-update global_var", the command sees no change in
> the case of the non-PIE executable, while in the case of the PIE
> executable install_new_value sees that value changes, leading to a
> different output.
> 
> This patch makes sure that, as we do for breakpoints, we wait until
> relocation has happened before we try to recreate varobjs.  This way we
> have a consistent behavior between PIE and non-PIE binaries.
> 
> Tested on x86_64-linux.
> 

LGTM.

Thanks,
- Tom

> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=29426
> Co-authored-by: Lancelot SIX <lancelot.six@amd.com>
> ---
>   gdb/symfile.c                                 |  6 ++---
>   .../gdb.mi/mi-var-invalidate-shlib.exp        |  2 +-
>   gdb/varobj.c                                  | 22 +++++++------------
>   gdb/varobj.h                                  |  5 ++++-
>   4 files changed, 16 insertions(+), 19 deletions(-)
> 
> diff --git a/gdb/symfile.c b/gdb/symfile.c
> index aea8c762ee6..f61235dd271 100644
> --- a/gdb/symfile.c
> +++ b/gdb/symfile.c
> @@ -1656,6 +1656,9 @@ symbol_file_command (const char *args, int from_tty)
>   
>         /* Now it's safe to re-add the breakpoints.  */
>         breakpoint_re_set ();
> +
> +      /* Also, it's safe to re-add varobjs.  */
> +      varobj_re_set ();
>       }
>   }
>   
> @@ -2870,9 +2873,6 @@ clear_symtab_users (symfile_add_flags add_flags)
>     clear_pc_function_cache ();
>     gdb::observers::new_objfile.notify (NULL);
>   
> -  /* Varobj may refer to old symbols, perform a cleanup.  */
> -  varobj_invalidate ();
> -
>     /* Now that the various caches have been cleared, we can re_set
>        our breakpoints without risking it using stale data.  */
>     if ((add_flags & SYMFILE_DEFER_BP_RESET) == 0)
> diff --git a/gdb/testsuite/gdb.mi/mi-var-invalidate-shlib.exp b/gdb/testsuite/gdb.mi/mi-var-invalidate-shlib.exp
> index 9ba5af0c028..fdf80b79741 100644
> --- a/gdb/testsuite/gdb.mi/mi-var-invalidate-shlib.exp
> +++ b/gdb/testsuite/gdb.mi/mi-var-invalidate-shlib.exp
> @@ -99,7 +99,7 @@ proc do_test { separate_debuginfo } {
>   	# When reloading the symbol file, only the var for the global in the main
>   	# executable is re-created.
>   	mi_gdb_test "-var-update global_var" \
> -	    "\\^done,changelist=\\\[{name=\"global_var\",in_scope=\"true\",type_changed=\"false\",has_more=\"0\"}\\\]" \
> +	    "\\^done,changelist=\\\[\\\]" \
>   	    "global_var recreated"
>   	mi_gdb_test "-var-update global_shlib_var" \
>   	    "\\^done,changelist=\\\[{name=\"global_shlib_var\",in_scope=\"invalid\",has_more=\"0\"}\\\]" \
> diff --git a/gdb/varobj.c b/gdb/varobj.c
> index a142bb02e03..55a7bd97f43 100644
> --- a/gdb/varobj.c
> +++ b/gdb/varobj.c
> @@ -2353,18 +2353,14 @@ all_root_varobjs (gdb::function_view<void (struct varobj *var)> func)
>       }
>   }
>   
> -/* Invalidate varobj VAR if it is tied to locals and re-create it if it is
> -   defined on globals.  It is a helper for varobj_invalidate.
> -
> -   This function is called after changing the symbol file, in this case the
> -   pointers to "struct type" stored by the varobj are no longer valid.  All
> -   varobj must be either re-evaluated, or marked as invalid here.  */
> +/* Try to recreate the varobj VAR if it is a global or floating.  This is a
> +   helper function for varobj_re_set.  */
>   
>   static void
> -varobj_invalidate_iter (struct varobj *var)
> +varobj_re_set_iter (struct varobj *var)
>   {
> -  /* global and floating var must be re-evaluated.  */
> -  if (var->root->floating || var->root->global)
> +  /* Invalidated globals and floating var must be re-evaluated.  */
> +  if (var->root->global || var->root->floating)
>       {
>         struct varobj *tmp_var;
>   
> @@ -2389,14 +2385,12 @@ varobj_invalidate_iter (struct varobj *var)
>       }
>   }
>   
> -/* 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".  */
> +/* See varobj.h.  */
>   
>   void
> -varobj_invalidate (void)
> +varobj_re_set (void)
>   {
> -  all_root_varobjs (varobj_invalidate_iter);
> +  all_root_varobjs (varobj_re_set_iter);
>   }
>   
>   /* Ensure that no varobj keep references to OBJFILE.  */
> diff --git a/gdb/varobj.h b/gdb/varobj.h
> index 073da60d772..0c92d67e4f4 100644
> --- a/gdb/varobj.h
> +++ b/gdb/varobj.h
> @@ -314,7 +314,10 @@ extern void all_root_varobjs (gdb::function_view<void (struct varobj *var)>);
>   extern std::vector<varobj_update_result>
>     varobj_update (struct varobj **varp, bool is_explicit);
>   
> -extern void varobj_invalidate (void);
> +/* Try to recreate any global or floating varobj.  This is called after
> +   changing symbol files.  */
> +
> +extern void varobj_re_set (void);
>   
>   extern bool varobj_editable_p (const struct varobj *var);
>   

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

[Lancelot SIX]

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.
---
 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;
-	}
     }
 }
 
-- 
2.34.1

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

[Tom de Vries]

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;
> -	}
>       }
>   }
>   

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

[Lancelot SIX]

On 09/08/2022 11:55, Tom de Vries wrote:
> [CAUTION: External Email]
> 
> 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
Hi,

Thanks.

I have done the changes you asked in patch #1 locally.  I am about to 
push the series.

Best,
Lanceolt.