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

[Tom de Vries <tdevries@suse.de>]

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