[PATCH 0/2] Fix regressions in varobj recreation

[PATCH 0/2] Fix regressions in varobj recreation

[Lancelot SIX via Gdb-patches]

Hi,

Tom de Vries noticed that a series I merged recently
(https://sourceware.org/pipermail/gdb-patches/2022-July/191085.html)
introduced regressions on some systems (reported in PR/29426).

This series aims at fixing those issues.

Patch #2 of the series is mostly about relaxing the testcase as I do not
see a way to "fix" the actual divergence of behavior of the tescase when
debugging PIE/non-PIE executable (see patch's description).  Alternative
approaches are more than welcome.

Best,
Lancelot.

Lancelot SIX (2):
  gdb: Fix regression in varobj recreation
  gdb/testsuite: Accept PIE/noPIE programs in
    gdb.mi/mi-var-invalidate-shlib.exp

 gdb/testsuite/gdb.mi/mi-var-invalidate-shlib.exp | 2 +-
 gdb/varobj.c                                     | 9 +++++++--
 2 files changed, 8 insertions(+), 3 deletions(-)


base-commit: 976f16630b1f7421d6693011333cf0f51417c498
-- 
2.34.1

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

[Lancelot SIX via Gdb-patches]

"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.
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.

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.  */
-- 
2.34.1

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

[Tom de Vries via Gdb-patches]

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.  */

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

[Six, Lancelot via Gdb-patches]

[Public]

> > "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".
> [...]
> 
> stall -> stale
> 
> >
> 
> LGTM.
> 
> Thanks,
> - Tom
> 

Hi,

I have pushed this patch with the fixes as you suggested.

Thanks,
Lancelot.

[PATCH 2/2] gdb/testsuite: Accept PIE/noPIE programs in gdb.mi/mi-var-invalidate-shlib.exp

[Lancelot SIX via Gdb-patches]

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 changes the testcase to accept both behaviors.

Tested on x86_64-linux.

Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=29426
---
 gdb/testsuite/gdb.mi/mi-var-invalidate-shlib.exp | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/gdb/testsuite/gdb.mi/mi-var-invalidate-shlib.exp b/gdb/testsuite/gdb.mi/mi-var-invalidate-shlib.exp
index 87d1d4a6a9d..d44b492ae6b 100644
--- a/gdb/testsuite/gdb.mi/mi-var-invalidate-shlib.exp
+++ b/gdb/testsuite/gdb.mi/mi-var-invalidate-shlib.exp
@@ -103,7 +103,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=\\\[({name=\"global_var\",in_scope=\"true\",type_changed=\"false\",has_more=\"0\"})?\\\]" \
 	    "global_var recreated"
 	mi_gdb_test "-var-update global_shlib_var" \
 	    "\\^done,changelist=\\\[{name=\"global_shlib_var\",in_scope=\"invalid\",has_more=\"0\"}\\\]" \
-- 
2.34.1

Re: [PATCH 2/2] gdb/testsuite: Accept PIE/noPIE programs in gdb.mi/mi-var-invalidate-shlib.exp

[Tom de Vries via Gdb-patches]

On 8/2/22 14:47, Lancelot SIX wrote:
> 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 changes the testcase to accept both behaviors.

Hi,

thanks for the analysis.

After looking into this a bit, I wonder if the root cause is really 
looking at the values in the old process.

Maybe we should either:
- not try to sample the values when recreating, or
- move the recreation to a later point when we have access to the memory
   of the new process.

At least we would have consistency between PIE and no-PIE with either 
solution.

Thanks,
- Tom

RE: [PATCH 2/2] gdb/testsuite: Accept PIE/noPIE programs in gdb.mi/mi-var-invalidate-shlib.exp

[Six, Lancelot via Gdb-patches]

[AMD Official Use Only - General]

> Hi,
> 
> thanks for the analysis.
> 
> After looking into this a bit, I wonder if the root cause is really
> looking at the values in the old process.
> 
> Maybe we should either:
> - not try to sample the values when recreating, or
> - move the recreation to a later point when we have access to the memory
>    of the new process.
> 
> At least we would have consistency between PIE and no-PIE with either
> solution.
> 
> Thanks,
> - Tom 

Hi,

I do agree that it is strange to look for a value for the new objfile in the old process.  I wonder if there are legitimate usecases where one would want to still use "file ..." to load new symbols but keep the same process running while keeping a global varobj around.  If we delay the recreation of the varobj, we would break such usecase.

One way could be to be just invalidate the varobj when the objfile it relies on goes away and give the user a "-var-reinflate" command (or "-var-update -reinflate") so he can try to revive an invalidated varobj at his convenience.  This would break existing behavior but could avoid.

Lancelot.

Re: [PATCH 2/2] gdb/testsuite: Accept PIE/noPIE programs in gdb.mi/mi-var-invalidate-shlib.exp

[Tom de Vries via Gdb-patches]

[-- Attachment #1: Type: text/plain, Size: 1752 bytes --]

On 8/3/22 11:35, Six, Lancelot wrote:
> [AMD Official Use Only - General]
> 
>> Hi,
>>
>> thanks for the analysis.
>>
>> After looking into this a bit, I wonder if the root cause is really
>> looking at the values in the old process.
>>
>> Maybe we should either:
>> - not try to sample the values when recreating, or
>> - move the recreation to a later point when we have access to the memory
>>     of the new process.
>>
>> At least we would have consistency between PIE and no-PIE with either
>> solution.
>>
>> Thanks,
>> - Tom
> 
> Hi,
> 
> I do agree that it is strange to look for a value for the new objfile in the old process.  I wonder if there are legitimate usecases where one would want to still use "file ..." to load new symbols but keep the same process running while keeping a global varobj around.  If we delay the recreation of the varobj, we would break such usecase.
> 

You're right,  apparently that's a valid use case.

> One way could be to be just invalidate the varobj when the objfile it relies on goes away and give the user a "-var-reinflate" command (or "-var-update -reinflate") so he can try to revive an invalidated varobj at his convenience.  This would break existing behavior but could avoid.

I've now reached the conclusion that the bug is merely that for 
recreating the varobjs we don't wait until relocation has happened, as 
we do for breakpoints.

This WIP patch delays recreation of the varobj to the point that the 
file command has relocated the PIE exec.

This makes behaviour equal between PIE and no-PIE (meaning we see the 
same two FAILs now for gdb.mi/mi-var-invalidate-shlib.exp).

I'm not sure about the split between invalidate and re_set, I'm not 
familiar with this code.

WDYT?

Thanks,
- Tom

[-- Attachment #2: tmp.patch --]
[-- Type: text/x-patch, Size: 2226 bytes --]

diff --git a/gdb/symfile.c b/gdb/symfile.c
index aea8c762ee6..d0a32d13231 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 ();
     }
 }
 
diff --git a/gdb/varobj.c b/gdb/varobj.c
index 0683af1991e..ecc53f2643d 100644
--- a/gdb/varobj.c
+++ b/gdb/varobj.c
@@ -2362,6 +2362,26 @@ all_root_varobjs (gdb::function_view<void (struct varobj *var)> func)
 
 static void
 varobj_invalidate_iter (struct varobj *var)
+{
+  /* global and floating var must be re-evaluated.  */
+  if (var->root->floating || var->root->global)
+    ;
+  else /* locals must be invalidated.  */
+    var->root->is_valid = false;
+}
+
+/* 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".  */
+
+void
+varobj_invalidate (void)
+{
+  all_root_varobjs (varobj_invalidate_iter);
+}
+
+static void
+varobj_re_set_iter (struct varobj *var)
 {
   /* global and floating var must be re-evaluated.  */
   if (var->root->floating || var->root->global)
@@ -2387,18 +2407,12 @@ 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
-   are defined on globals.
-   Invalidated varobjs will be always printed in_scope="invalid".  */
-
 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..f3cce8ebfbd 100644
--- a/gdb/varobj.h
+++ b/gdb/varobj.h
@@ -316,6 +316,8 @@ extern std::vector<varobj_update_result>
 
 extern void varobj_invalidate (void);
 
+extern void varobj_re_set (void);
+
 extern bool varobj_editable_p (const struct varobj *var);
 
 extern bool varobj_floating_p (const struct varobj *var);

RE: [PATCH 2/2] gdb/testsuite: Accept PIE/noPIE programs in gdb.mi/mi-var-invalidate-shlib.exp

[Six, Lancelot via Gdb-patches]

[Public]



> -----Original Message-----
> From: Tom de Vries <tdevries@suse.de>
> Sent: 03 August 2022 13:51
> To: Six, Lancelot <Lancelot.Six@amd.com>; gdb-patches@sourceware.org
> Cc: lsix@lancelotsix.com
> Subject: Re: [PATCH 2/2] gdb/testsuite: Accept PIE/noPIE programs in
> gdb.mi/mi-var-invalidate-shlib.exp
> 
> [CAUTION: External Email]
> 
> On 8/3/22 11:35, Six, Lancelot wrote:
> > [AMD Official Use Only - General]
> >
> >> Hi,
> >>
> >> thanks for the analysis.
> >>
> >> After looking into this a bit, I wonder if the root cause is really
> >> looking at the values in the old process.
> >>
> >> Maybe we should either:
> >> - not try to sample the values when recreating, or
> >> - move the recreation to a later point when we have access to the memory
> >>     of the new process.
> >>
> >> At least we would have consistency between PIE and no-PIE with either
> >> solution.
> >>
> >> Thanks,
> >> - Tom
> >
> > Hi,
> >
> > I do agree that it is strange to look for a value for the new objfile in the old
> process.  I wonder if there are legitimate usecases where one would want to
> still use "file ..." to load new symbols but keep the same process running
> while keeping a global varobj around.  If we delay the recreation of the
> varobj, we would break such usecase.
> >
> 
> You're right,  apparently that's a valid use case.
> 
> > One way could be to be just invalidate the varobj when the objfile it relies
> on goes away and give the user a "-var-reinflate" command (or "-var-update
> -reinflate") so he can try to revive an invalidated varobj at his convenience.
> This would break existing behavior but could avoid.
> 
> I've now reached the conclusion that the bug is merely that for
> recreating the varobjs we don't wait until relocation has happened, as
> we do for breakpoints.
> 
> This WIP patch delays recreation of the varobj to the point that the
> file command has relocated the PIE exec.
> 
> This makes behaviour equal between PIE and no-PIE (meaning we see the
> same two FAILs now for gdb.mi/mi-var-invalidate-shlib.exp).
> 
> I'm not sure about the split between invalidate and re_set, I'm not
> familiar with this code.
> 
> WDYT?

Hi

This looks the way forward!

It looks like to me that varobj_invalidate_iter (and varobj_invalidate) can go away entirely.
The only thing they do is invalidate locals which are still valid at this point, and I think this
is not what we want:

- If the varobj was tracking a local which was associated with the objfile being replaced, then
  `varobj_invalidate_if_uses_objfile` would have set the is_valid flag to false earlier when unloading
  the previous objfile.
- If the varobj is tracking a local in another objfile which is still loaded, I do not see why we would invalidate it.
  It is possible to have such scenario with the mi-var-invalidate-shlib example: if stopped in the shlib and create
  a varobj to track a local variable, there is no reason for a "file mi-var-invalidate-shlib" to invalidate it.

Also seeing this makes me wonder if varobj_re_set_iter should only try to re-inflate invalidated globals.  I do not
see why a "file" has to force a "-var-update" on floating varobj, but I need to think a bit more about this one.

Lancelot.

> 
> Thanks,
> - Tom