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

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

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