[PATCH 0/3] Fix some use-after-free errors in varobj code

[PATCH 0/3] Fix some use-after-free errors in varobj code

[Lancelot SIX]

Hi,

This series aims at fixing some use-after free errors we have observed around
the varobj code.  When a objfile is freed, the varobj can keep references to
the objfile and to objects that used to live on the objfile's objstack (types
among other things).

This can mainly be observed when debugging code which loads and unloads shared
libraries during its lifetime.  Without such scenario the problems exist but
are rarely exposed as the references to freed memory are not used.

The first patch of the series was originally written by Pedro.  It improves
mi-support.exp so `mi_runto` now accepts a `-pending` flag, which will be used
in the following patch.

Patch #2 fixes the actual use-after free errors by ensuring that we clear all
references to the objfile before it is freed.

Patch #3 fix some inaccuracies in the current varobj_invalidate mechanism which
is used to invalidate/recreate varobj when loading a new objfile.

All feedback are welcome.
Regression tested on x86_64.

Lancelot SIX (2):
  gdb/varobj: Fix use after free in varobj
  gdb/varobj: Fix varobj_invalidate_iter

Pedro Alves (1):
  MI: mi_runto -pending

 .../gdb.mi/mi-var-invalidate-shlib-lib.c      | 30 ++++++
 .../gdb.mi/mi-var-invalidate-shlib.c          | 27 ++++++
 .../gdb.mi/mi-var-invalidate-shlib.exp        | 91 +++++++++++++++++++
 gdb/testsuite/lib/mi-support.exp              | 68 ++++++++++++--
 gdb/value.c                                   | 21 +++++
 gdb/varobj.c                                  | 86 ++++++++++++++++--
 6 files changed, 308 insertions(+), 15 deletions(-)
 create mode 100644 gdb/testsuite/gdb.mi/mi-var-invalidate-shlib-lib.c
 create mode 100644 gdb/testsuite/gdb.mi/mi-var-invalidate-shlib.c
 create mode 100644 gdb/testsuite/gdb.mi/mi-var-invalidate-shlib.exp


base-commit: 5fb28d2607a8325559b44a5dc0c8760236c81218
-- 
2.25.1

[PATCH 1/3] MI: mi_runto -pending

[Lancelot SIX]

From: Pedro Alves <pedro@palves.net>

With the CLI testsuite's runto proc, we can pass "allow-pending" as an
option, like:

  runto func allow-pending

That is currently not possible with MI's mi_runto, however.  This
patch makes it possible, by adding a new "-pending" option to
mi_runto.

A pending breakpoint shows different MI attributes compared to a
breakpoint with a location, so the regexp returned by
mi_make_breakpoint isn't suitable.  Thus, add a new
mi_make_breakpoint_pending proc for pending breakpoints.

Tweak mi_runto to let it take and pass down arguments.

Change-Id: I185fef00ab545a1df2ce12b4dbc3da908783a37c
---
 gdb/testsuite/lib/mi-support.exp | 66 +++++++++++++++++++++++++++++---
 1 file changed, 60 insertions(+), 6 deletions(-)

diff --git a/gdb/testsuite/lib/mi-support.exp b/gdb/testsuite/lib/mi-support.exp
index e578a7e6f9b..e724b2eeb51 100644
--- a/gdb/testsuite/lib/mi-support.exp
+++ b/gdb/testsuite/lib/mi-support.exp
@@ -1114,6 +1114,8 @@ proc mi_clean_restart { args } {
 # Supported options:
 #
 #  -qualified -- pass --qualified to -break-insert
+#  -pending   -- pass -f to -break-insert to create a pending
+#                breakpoint.
 
 proc mi_runto_helper {func run_or_continue args} {
   global mi_gdb_prompt expect_out
@@ -1122,13 +1124,24 @@ proc mi_runto_helper {func run_or_continue args} {
   parse_args {{qualified}}
 
   set test "mi runto $func"
-  set bp [mi_make_breakpoint -type breakpoint -disp del \
-	      -func $func\(\\\(.*\\\)\)?]
+  if {$pending} {
+      set bp [mi_make_breakpoint_pending -type breakpoint -disp del]
+  } else {
+      set bp [mi_make_breakpoint -type breakpoint -disp del \
+		  -func $func\(\\\(.*\\\)\)?]
+  }
   set extra_opts ""
+  set extra_output ""
   if {$qualified} {
-      append extra_opts "--qualified"
+      lappend extra_opts "--qualified"
   }
-  mi_gdb_test "200-break-insert $extra_opts -t $func" "200\\^done,$bp" \
+  if {$pending} {
+      lappend extra_opts "-f"
+      # MI prints "Function FUNC not defined", "No line NNN in current
+      # file.", etc. to the CLI stream.
+      set extra_output "&\"\[^\r\n\]+\"\r\n"
+  }
+  mi_gdb_test "200-break-insert [join $extra_opts " "] -t $func" "${extra_output}200\\^done,$bp" \
       "breakpoint at $func"
 
   if {$run_or_continue == "run"} {
@@ -1142,8 +1155,8 @@ proc mi_runto_helper {func run_or_continue args} {
   mi_expect_stop "breakpoint-hit" $func ".*" ".*" "\[0-9\]+" { "" "disp=\"del\"" } $test
 }
 
-proc mi_runto {func} {
-    return [mi_runto_helper $func "run"]
+proc mi_runto {func args} {
+    return [mi_runto_helper $func "run" {*}$args]
 }
 
 # Just like runto_main but works with the MI interface.
@@ -2683,6 +2696,47 @@ proc mi_make_breakpoint_multi {args} {
     return $result
 }
 
+# Construct a breakpoint regexp, for a pending breakpoint.  This may
+# be used to test the output of -break-insert, -dprintf-insert, or
+# -break-info for pending breakpoints.
+#
+# Arguments for the breakpoint may be specified using the options
+# number, type, disp, enabled, pending.
+#
+# Example: mi_make_breakpoint_pending -number 2 -pending func
+# will return the breakpoint:
+# bkpt={number="2",type=".*",disp=".*",enabled=".*",addr="<PENDING>",
+#       pending="func", times="0".*original-location=".*"}
+
+proc mi_make_breakpoint_pending {args} {
+    parse_args {{number .*} {type .*} {disp .*} {enabled .*}
+	{pending .*} {original-location .*}}
+
+    set attr_list {}
+    foreach attr [list number type disp enabled] {
+	lappend attr_list $attr [set $attr]
+    }
+
+    lappend attr_list "addr" "<PENDING>"
+
+    foreach attr [list pending] {
+	lappend attr_list $attr [set $attr]
+    }
+
+    set ignore 0
+    set times 0
+    set script ""
+    set cond ""
+    set evaluated-by ""
+
+    set result [mi_make_breakpoint_1 \
+		    $attr_list $cond ${evaluated-by} $times \
+		    $ignore $script ${original-location}]
+
+    append result "\\\}"
+    return $result
+}
+
 # Construct a breakpoint regexp.  This may be used to test the output of
 # -break-insert, -dprintf-insert, or -break-info.
 #
-- 
2.25.1

[PATCH 2/3] gdb/varobj: Fix use after free in varobj

[Lancelot SIX]

Varobj object contains references to types, variables (i.e. struct
variable) and expression.  All of those can reference data on an
objfile's obstack.  It is possible for this objfile to be deleted (and
the obstack to be feed), while the varobj remains valid.  Later, if the
user uses the varobj, this will result in a use-after-free error.  With
address sanitizer build, this leads to a plain error.  For non address
sanitizer build we might see undefined behaviour, which manifest
themself as assertion failures when accessing data backed by feed
memory.

This can be observed if we create a varobj that refers to ta symbol in a
shared library, after either the objfile gets reloaded (using the `file`
command) or after the shared library is unloaded (with a call to dlclose
for example).

This patch fixes those issues by:

- Adding cleanup procedure to the free_objfile observable.  When
  activated this observer clears expressions referencing the objfile
  being freed, and removes references to blocks belonging to this
  objfile.
- Adding varobj support in the `preserve_values` (gdb.value.c).  This
  ensures that before the objfile is unloaded, any type owned by the
  objfile referenced by the varobj is replaced by an equivalent type
  not owned by the objfile.  This process is done here instead of in the
  free_objfile observer in order to reuse the type hash table already
  used for similar purpose when replacing types of values kept in the
  value history.

A consequence of those changes is that the varobj->root->exp field of a
varobj can now be NULL.  The rest of the changes ensure that the varobj
machinery is able to support such situation when re-evaluating the var
(under varobj_update).

When the varobj->root->exp is initialized, this patch also makes sure to
keep a reference to its gdbarch and language_defn members.  Those
structures outlive the objfile, so this is safe.  This is done because
those references might be used initialize a python context even after
exp is invalidated.  Another approach could have been to initialize the
python context with default gdbarch and language_defn (i.e. nullptr) if
expr is NULL, but since we might still try to display the value which
was obtained by evaluating exp when it was still valid, keeping track of
the context which was used at this time seems reasonable.

Tested on x86_64-Linux.
---
 .../gdb.mi/mi-var-invalidate-shlib-lib.c      | 30 +++++++
 .../gdb.mi/mi-var-invalidate-shlib.c          | 27 +++++++
 .../gdb.mi/mi-var-invalidate-shlib.exp        | 80 +++++++++++++++++++
 gdb/testsuite/lib/mi-support.exp              |  2 +-
 gdb/value.c                                   | 21 +++++
 gdb/varobj.c                                  | 66 ++++++++++++++-
 6 files changed, 224 insertions(+), 2 deletions(-)
 create mode 100644 gdb/testsuite/gdb.mi/mi-var-invalidate-shlib-lib.c
 create mode 100644 gdb/testsuite/gdb.mi/mi-var-invalidate-shlib.c
 create mode 100644 gdb/testsuite/gdb.mi/mi-var-invalidate-shlib.exp

diff --git a/gdb/testsuite/gdb.mi/mi-var-invalidate-shlib-lib.c b/gdb/testsuite/gdb.mi/mi-var-invalidate-shlib-lib.c
new file mode 100644
index 00000000000..0abbdbdf9c6
--- /dev/null
+++ b/gdb/testsuite/gdb.mi/mi-var-invalidate-shlib-lib.c
@@ -0,0 +1,30 @@
+/* Copyright 2022 Free Software Foundation, Inc.
+
+   This file is part of GDB.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+struct bar
+{
+  int a;
+  int b;
+};
+
+static struct bar global_var = { 2, 2 };
+
+void
+foo ()
+{
+  struct bar local_var = { 1, 1 };
+}
diff --git a/gdb/testsuite/gdb.mi/mi-var-invalidate-shlib.c b/gdb/testsuite/gdb.mi/mi-var-invalidate-shlib.c
new file mode 100644
index 00000000000..10101bb8d22
--- /dev/null
+++ b/gdb/testsuite/gdb.mi/mi-var-invalidate-shlib.c
@@ -0,0 +1,27 @@
+/* Copyright 2022 Free Software Foundation, Inc.
+
+   This file is part of GDB.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+#include <dlfcn.h>
+
+int
+main (int argc, char *argv [])
+{
+  void *shlib = dlopen (SHLIB_PATH, RTLD_LAZY);
+  void (*foo)() = dlsym (shlib, "foo");
+  foo ();
+  dlclose (shlib);
+}
diff --git a/gdb/testsuite/gdb.mi/mi-var-invalidate-shlib.exp b/gdb/testsuite/gdb.mi/mi-var-invalidate-shlib.exp
new file mode 100644
index 00000000000..5219946d845
--- /dev/null
+++ b/gdb/testsuite/gdb.mi/mi-var-invalidate-shlib.exp
@@ -0,0 +1,80 @@
+# Copyright 2007-2022 Free Software Foundation, Inc.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+#
+# Test that varobj are invalidated after the shlib they point to goes
+# away.
+
+
+load_lib mi-support.exp
+set MIFLAGS "-i=mi"
+
+if { [skip_shlib_tests] } {
+    return 0
+}
+
+standard_testfile .c -lib.c
+set shlib_path [standard_output_file ${testfile}-lib.so]
+
+if { [gdb_compile_shlib $srcdir/$subdir/$srcfile2 $shlib_path {debug}] != "" } {
+    untested "failed to compile"
+    return -1
+}
+
+set opts [list shlib_load debug additional_flags=-DSHLIB_PATH="${shlib_path}"]
+if  { [gdb_compile "${srcdir}/${subdir}/${srcfile}" "${binfile}" executable $opts] != "" } {
+    untested "failed to compile"
+    return -1
+}
+
+gdb_exit
+if { [mi_gdb_start] } {
+    return 0
+}
+
+# Start the process once and create varobjs referencing the loaded objfiles.
+with_test_prefix "setup" {
+    mi_load_shlibs $shlib_path
+    mi_delete_breakpoints
+    mi_gdb_reinitialize_dir $srcdir/$subdir
+    mi_gdb_load $binfile
+
+    mi_runto foo -pending
+
+    mi_create_varobj global_var global_var "create global gloal_var"
+    mi_create_floating_varobj floating_local local_var "create floating local_var"
+
+    # Get ourself in a frame where the floating var is invalid.
+    mi_gdb_test "-exec-finish" ".*"
+    mi_expect_stop "function-finished" main ".*" ".*" "\[0-9\]+" { ".*" } "out of foo"
+}
+
+# Reload the entire process
+with_test_prefix "restart process" {
+    mi_delete_breakpoints
+    mi_gdb_load ${binfile}
+    mi_runto_main
+}
+
+with_test_prefix "in new process" {
+    # The global var was invalidated when the objfile got unloaded.
+    mi_gdb_test "-var-update global_var" \
+	"\\^done,changelist=\\\[\{name=\"global_var\",in_scope=\"invalid\",has_more=\"0\"\}\]" \
+	"global invalidated"
+
+    # Floating varobj should still be valid, but out of scope at the moment.
+    mi_gdb_test "-var-update floating_local" \
+	"\\^done,changelist=\\\[{name=\"floating_local\",in_scope=\"false\",type_changed=\"false\",has_more=\"0\"}\\\]" \
+	"floating_local still valid but not in scope"
+}
diff --git a/gdb/testsuite/lib/mi-support.exp b/gdb/testsuite/lib/mi-support.exp
index e724b2eeb51..ca56e12b06b 100644
--- a/gdb/testsuite/lib/mi-support.exp
+++ b/gdb/testsuite/lib/mi-support.exp
@@ -1121,7 +1121,7 @@ proc mi_runto_helper {func run_or_continue args} {
   global mi_gdb_prompt expect_out
   global hex decimal fullname_syntax
 
-  parse_args {{qualified}}
+  parse_args {{qualified} {pending}}
 
   set test "mi runto $func"
   if {$pending} {
diff --git a/gdb/value.c b/gdb/value.c
index 022fca91a42..b9d2937c608 100644
--- a/gdb/value.c
+++ b/gdb/value.c
@@ -46,6 +46,7 @@
 #include "cli/cli-style.h"
 #include "expop.h"
 #include "inferior.h"
+#include "varobj.h"
 
 /* Definition of a user function.  */
 struct internal_function
@@ -2596,6 +2597,21 @@ preserve_one_internalvar (struct internalvar *var, struct objfile *objfile,
     }
 }
 
+static void
+preserve_one_varobj (struct varobj *varobj, struct objfile *objfile,
+		     htab_t copied_types)
+{
+  if (varobj->type->is_objfile_owned ()
+      && varobj->type->objfile_owner () == objfile)
+    {
+      varobj->type
+	= copy_type_recursive (objfile, varobj->type, copied_types);
+    }
+
+  if (varobj->value != nullptr)
+    preserve_one_value (varobj->value.get (), objfile, copied_types);
+}
+
 /* Update the internal variables and value history when OBJFILE is
    discarded; we must copy the types out of the objfile.  New global types
    will be created for every convenience variable which currently points to
@@ -2617,6 +2633,11 @@ preserve_values (struct objfile *objfile)
   for (var = internalvars; var; var = var->next)
     preserve_one_internalvar (var, objfile, copied_types.get ());
 
+  /* For the remaining varobj, check that none has type owned by OBJFILE.  */
+  all_root_varobjs ([&copied_types, objfile](struct varobj *varobj)
+		    { preserve_one_varobj (varobj, objfile,
+					   copied_types.get ()); });
+
   preserve_ext_lang_values (objfile, copied_types.get ());
 }
 
diff --git a/gdb/varobj.c b/gdb/varobj.c
index 1aca015a21a..caaffe4ea70 100644
--- a/gdb/varobj.c
+++ b/gdb/varobj.c
@@ -32,6 +32,7 @@
 #include "parser-defs.h"
 #include "gdbarch.h"
 #include <algorithm>
+#include "observable.h"
 
 #if HAVE_PYTHON
 #include "python/python.h"
@@ -72,6 +73,12 @@ struct varobj_root
   /* The expression for this parent.  */
   expression_up exp;
 
+  /* Cached arch from exp, for use in case exp gets invalidated.  */
+  struct gdbarch *gdbarch = nullptr;
+
+  /* Cached language from exp, for use in case exp gets invalidated.  */
+  const struct language_defn *language_defn = nullptr;
+
   /* Block for which this expression is valid.  */
   const struct block *valid_block = NULL;
 
@@ -206,7 +213,7 @@ is_root_p (const struct varobj *var)
 
 /* See python-internal.h.  */
 gdbpy_enter_varobj::gdbpy_enter_varobj (const struct varobj *var)
-: gdbpy_enter (var->root->exp->gdbarch, var->root->exp->language_defn)
+: gdbpy_enter (var->root->gdbarch, var->root->language_defn)
 {
 }
 
@@ -303,6 +310,11 @@ varobj_create (const char *objname,
       try
 	{
 	  var->root->exp = parse_exp_1 (&p, pc, block, 0, &tracker);
+
+	  /* Cache gdbarch and language_defn as they might be used even
+	     after var is invalidated and var->root->exp cleared.  */
+	  var->root->gdbarch = var->root->exp->gdbarch;
+	  var->root->language_defn = var->root->exp->language_defn;
 	}
 
       catch (const gdb_exception_error &except)
@@ -2071,6 +2083,12 @@ value_of_root (struct varobj **var_handle, bool *type_changed)
   {
     struct value *value;
 
+    /* The expression have been invalidated (because it did reference an
+       objfile which is not available anymore) and we did not manage to
+       recerate it for a floating varobj.  */
+    if ((*var_handle)->root->exp == nullptr)
+      return nullptr;
+
     value = value_of_root_1 (var_handle);
     if (var->value == NULL || value == NULL)
       {
@@ -2373,6 +2391,49 @@ varobj_invalidate (void)
   all_root_varobjs (varobj_invalidate_iter);
 }
 
+/* Ensure that no varobj keep references to OBJFILE.  */
+
+static void
+varobj_invalidate_if_uses_objfile (struct objfile *objfile)
+{
+  auto varobj_invalidate_if_uses_objfile_worker
+    = [objfile](struct varobj *var)
+      {
+	if (var->root->valid_block != nullptr
+	    && block_objfile (var->root->valid_block) == objfile)
+	  {
+	    /* The varobj is tied to a block which is going away.  There is
+	       no way to reconstruct something later, so invalidate the
+	       varobj completly and drop the reference to the block which is
+	       being freed.  */
+	    var->root->is_valid = false;
+	    var->root->valid_block = nullptr;
+	  }
+
+	if (var->root->exp != nullptr
+	    && exp_uses_objfile (var->root->exp.get (), objfile))
+	  {
+	    /* The varobj's current expression references the objfile.  For
+	       globals and floating, it is possible that when we try to
+	       re-evaluate the expression later it is still valid with
+	       whatever is in scope at this moment.  Just invalidate the
+	       expression for now.  */
+	    var->root->exp.reset ();
+
+	    /* It only makes sense to keep a floating varobj around.  */
+	    if (!var->root->floating)
+	      var->root->is_valid = false;
+	  }
+
+	/* var->value->type and var->type might also reference the objfile.
+	   This is taken care of in value.c:preserve_values which deals with
+	   making sure that objfile-owend types are changed with gdbarch-owned
+	   equivalents.  */
+      };
+
+  all_root_varobjs (varobj_invalidate_if_uses_objfile_worker);
+}
+
 /* A hash function for a varobj.  */
 
 static hashval_t
@@ -2407,4 +2468,7 @@ _initialize_varobj ()
 			     _("When non-zero, varobj debugging is enabled."),
 			     NULL, show_varobjdebug,
 			     &setdebuglist, &showdebuglist);
+
+  gdb::observers::free_objfile.attach (varobj_invalidate_if_uses_objfile,
+				       "varobj");
 }
-- 
2.25.1

[PATCH 3/3] gdb/varobj: Fix varobj_invalidate_iter

[Lancelot SIX]

The varobj_invalidate function is used meant to be called when
restarting a process, and check at this point if some of the previously
existing varobj can be recreated in the context of the new process.

Two kind of varobj are subject to re-creation:  global varobj (i.e.
varobj which reference a global variable), and floating varobj (i.e.
varobj which are always re-evaluated in the context of whatever is
the currently selected frame at the time of evaluation).

However, in the re-creation process, the varobj_invalidate_iter
recreates floating varobj as non-floating, due to an invalid parameter.
This patches fixes this and adds an assertion to check that if a varobj
is indeed recreated, it matches the original varobj "floating" property.

Another issue is that if at this recreation time the expression watched
by the floating varobj is not in scope, then the varobj is marked as
invalid.  If later the user selects a frame where the expression becomes
valid, the varobj remains invalid and this is wrong.  This patch also
make sure that floating varobj are not invalidated if they cannot be
evaluated.

The last important thing to note is that due to the previous patch, when
varobj_invalidate is executed (in the context of a new process), any
global var have already been invalidated (this has been done when the
objfile it referred to got invalidated).  As a consequence,
varobj_invalidate tries to recreate vars which are already marked as
invalid.  This does not entirely feels right, but I keep this behavior
for backward compatibility.

Tested on x86_64-linux
---
 .../gdb.mi/mi-var-invalidate-shlib.exp        | 11 ++++++++++
 gdb/varobj.c                                  | 20 ++++++++++++-------
 2 files changed, 24 insertions(+), 7 deletions(-)

diff --git a/gdb/testsuite/gdb.mi/mi-var-invalidate-shlib.exp b/gdb/testsuite/gdb.mi/mi-var-invalidate-shlib.exp
index 5219946d845..3ae76755309 100644
--- a/gdb/testsuite/gdb.mi/mi-var-invalidate-shlib.exp
+++ b/gdb/testsuite/gdb.mi/mi-var-invalidate-shlib.exp
@@ -77,4 +77,15 @@ with_test_prefix "in new process" {
     mi_gdb_test "-var-update floating_local" \
 	"\\^done,changelist=\\\[{name=\"floating_local\",in_scope=\"false\",type_changed=\"false\",has_more=\"0\"}\\\]" \
 	"floating_local still valid but not in scope"
+
+    # Continue inside foo
+    mi_gdb_test "299-break-insert -f -t foo" \
+	"&\"Function \\\\\"foo\\\\\" not defined.\\\\n\"\r\n299\\^done,[mi_make_breakpoint_pending -type breakpoint -disp del -pending foo]"
+    mi_send_resuming_command "exec-continue" "continue to foo"
+    mi_expect_stop "breakpoint-hit" foo ".*" ".*" "\[0-9\]+" { "" "disp=\"del\"" } "arrived at foo"
+
+    # Floating varobj is still valid, and now in scope.
+    mi_gdb_test "-var-update floating_local" \
+	"\\^done,changelist=\\\[{name=\"floating_local\",in_scope=\"true\",type_changed=\"false\",has_more=\"0\"}\\\]" \
+	"floating_local still valid and in scope"
 }
diff --git a/gdb/varobj.c b/gdb/varobj.c
index caaffe4ea70..a316f90c9e2 100644
--- a/gdb/varobj.c
+++ b/gdb/varobj.c
@@ -2360,22 +2360,28 @@ static void
 varobj_invalidate_iter (struct varobj *var)
 {
   /* global and floating var must be re-evaluated.  */
-  if (var->root->floating || var->root->valid_block == NULL)
+  if (var->root->floating || var->root->valid_block == nullptr)
     {
       struct varobj *tmp_var;
 
       /* Try to create a varobj with same expression.  If we succeed
 	 replace the old varobj, otherwise invalidate it.  */
-      tmp_var = varobj_create (NULL, var->name.c_str (), (CORE_ADDR) 0,
-			       USE_CURRENT_FRAME);
-      if (tmp_var != NULL) 
-	{ 
+      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)
+	{
+	  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
-	var->root->is_valid = false;
+      else if (!var->root->floating)
+	{
+	  /* Only invalidate globals as floating vars might still be valid in
+	     some other frame.  */
+	  var->root->is_valid = false;
+	}
     }
   else /* locals must be invalidated.  */
     var->root->is_valid = false;
-- 
2.25.1

Re: [PATCH 2/3] gdb/varobj: Fix use after free in varobj

[Andrew Burgess]

Lancelot SIX via Gdb-patches <gdb-patches@sourceware.org> writes:

> Varobj object contains references to types, variables (i.e. struct
> variable) and expression.  All of those can reference data on an
> objfile's obstack.  It is possible for this objfile to be deleted (and
> the obstack to be feed), while the varobj remains valid.  Later, if the
> user uses the varobj, this will result in a use-after-free error.  With
> address sanitizer build, this leads to a plain error.  For non address
> sanitizer build we might see undefined behaviour, which manifest
> themself as assertion failures when accessing data backed by feed
> memory.
>
> This can be observed if we create a varobj that refers to ta symbol in a
> shared library, after either the objfile gets reloaded (using the `file`
> command) or after the shared library is unloaded (with a call to dlclose
> for example).
>
> This patch fixes those issues by:
>
> - Adding cleanup procedure to the free_objfile observable.  When
>   activated this observer clears expressions referencing the objfile
>   being freed, and removes references to blocks belonging to this
>   objfile.
> - Adding varobj support in the `preserve_values` (gdb.value.c).  This
>   ensures that before the objfile is unloaded, any type owned by the
>   objfile referenced by the varobj is replaced by an equivalent type
>   not owned by the objfile.  This process is done here instead of in the
>   free_objfile observer in order to reuse the type hash table already
>   used for similar purpose when replacing types of values kept in the
>   value history.
>
> A consequence of those changes is that the varobj->root->exp field of a
> varobj can now be NULL.  The rest of the changes ensure that the varobj
> machinery is able to support such situation when re-evaluating the var
> (under varobj_update).
>
> When the varobj->root->exp is initialized, this patch also makes sure to
> keep a reference to its gdbarch and language_defn members.  Those
> structures outlive the objfile, so this is safe.  This is done because
> those references might be used initialize a python context even after
> exp is invalidated.  Another approach could have been to initialize the
> python context with default gdbarch and language_defn (i.e. nullptr) if
> expr is NULL, but since we might still try to display the value which
> was obtained by evaluating exp when it was still valid, keeping track of
> the context which was used at this time seems reasonable.
>
> Tested on x86_64-Linux.
> ---
>  .../gdb.mi/mi-var-invalidate-shlib-lib.c      | 30 +++++++
>  .../gdb.mi/mi-var-invalidate-shlib.c          | 27 +++++++
>  .../gdb.mi/mi-var-invalidate-shlib.exp        | 80 +++++++++++++++++++
>  gdb/testsuite/lib/mi-support.exp              |  2 +-
>  gdb/value.c                                   | 21 +++++
>  gdb/varobj.c                                  | 66 ++++++++++++++-
>  6 files changed, 224 insertions(+), 2 deletions(-)
>  create mode 100644 gdb/testsuite/gdb.mi/mi-var-invalidate-shlib-lib.c
>  create mode 100644 gdb/testsuite/gdb.mi/mi-var-invalidate-shlib.c
>  create mode 100644 gdb/testsuite/gdb.mi/mi-var-invalidate-shlib.exp
>
> diff --git a/gdb/testsuite/gdb.mi/mi-var-invalidate-shlib-lib.c b/gdb/testsuite/gdb.mi/mi-var-invalidate-shlib-lib.c
> new file mode 100644
> index 00000000000..0abbdbdf9c6
> --- /dev/null
> +++ b/gdb/testsuite/gdb.mi/mi-var-invalidate-shlib-lib.c
> @@ -0,0 +1,30 @@
> +/* Copyright 2022 Free Software Foundation, Inc.
> +
> +   This file is part of GDB.
> +
> +   This program is free software; you can redistribute it and/or modify
> +   it under the terms of the GNU General Public License as published by
> +   the Free Software Foundation; either version 3 of the License, or
> +   (at your option) any later version.
> +
> +   This program is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +   GNU General Public License for more details.
> +
> +   You should have received a copy of the GNU General Public License
> +   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
> +
> +struct bar
> +{
> +  int a;
> +  int b;
> +};
> +
> +static struct bar global_var = { 2, 2 };
> +
> +void
> +foo ()
> +{
> +  struct bar local_var = { 1, 1 };
> +}
> diff --git a/gdb/testsuite/gdb.mi/mi-var-invalidate-shlib.c b/gdb/testsuite/gdb.mi/mi-var-invalidate-shlib.c
> new file mode 100644
> index 00000000000..10101bb8d22
> --- /dev/null
> +++ b/gdb/testsuite/gdb.mi/mi-var-invalidate-shlib.c
> @@ -0,0 +1,27 @@
> +/* Copyright 2022 Free Software Foundation, Inc.
> +
> +   This file is part of GDB.
> +
> +   This program is free software; you can redistribute it and/or modify
> +   it under the terms of the GNU General Public License as published by
> +   the Free Software Foundation; either version 3 of the License, or
> +   (at your option) any later version.
> +
> +   This program is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +   GNU General Public License for more details.
> +
> +   You should have received a copy of the GNU General Public License
> +   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
> +
> +#include <dlfcn.h>
> +
> +int
> +main (int argc, char *argv [])
> +{
> +  void *shlib = dlopen (SHLIB_PATH, RTLD_LAZY);
> +  void (*foo)() = dlsym (shlib, "foo");
> +  foo ();
> +  dlclose (shlib);
> +}
> diff --git a/gdb/testsuite/gdb.mi/mi-var-invalidate-shlib.exp b/gdb/testsuite/gdb.mi/mi-var-invalidate-shlib.exp
> new file mode 100644
> index 00000000000..5219946d845
> --- /dev/null
> +++ b/gdb/testsuite/gdb.mi/mi-var-invalidate-shlib.exp
> @@ -0,0 +1,80 @@
> +# Copyright 2007-2022 Free Software Foundation, Inc.
> +
> +# This program is free software; you can redistribute it and/or modify
> +# it under the terms of the GNU General Public License as published by
> +# the Free Software Foundation; either version 3 of the License, or
> +# (at your option) any later version.
> +#
> +# This program is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with this program.  If not, see <http://www.gnu.org/licenses/>.
> +#
> +# Test that varobj are invalidated after the shlib they point to goes
> +# away.
> +
> +
> +load_lib mi-support.exp
> +set MIFLAGS "-i=mi"
> +
> +if { [skip_shlib_tests] } {
> +    return 0
> +}
> +
> +standard_testfile .c -lib.c
> +set shlib_path [standard_output_file ${testfile}-lib.so]
> +
> +if { [gdb_compile_shlib $srcdir/$subdir/$srcfile2 $shlib_path {debug}] != "" } {
> +    untested "failed to compile"
> +    return -1
> +}
> +
> +set opts [list shlib_load debug additional_flags=-DSHLIB_PATH="${shlib_path}"]
> +if  { [gdb_compile "${srcdir}/${subdir}/${srcfile}" "${binfile}" executable $opts] != "" } {
> +    untested "failed to compile"
> +    return -1
> +}
> +
> +gdb_exit
> +if { [mi_gdb_start] } {
> +    return 0
> +}

Could/should the gdb_exit and mi_gdb_start calls be replaced with:

  if { [mi_clean_restart] } {
    # Should we have an unsupported call in here maybe?
    return
  }

> +
> +# Start the process once and create varobjs referencing the loaded objfiles.
> +with_test_prefix "setup" {
> +    mi_load_shlibs $shlib_path
> +    mi_delete_breakpoints
> +    mi_gdb_reinitialize_dir $srcdir/$subdir
> +    mi_gdb_load $binfile
> +
> +    mi_runto foo -pending
> +
> +    mi_create_varobj global_var global_var "create global gloal_var"
> +    mi_create_floating_varobj floating_local local_var "create floating local_var"
> +
> +    # Get ourself in a frame where the floating var is invalid.
> +    mi_gdb_test "-exec-finish" ".*"
> +    mi_expect_stop "function-finished" main ".*" ".*" "\[0-9\]+" { ".*" } "out of foo"
> +}
> +
> +# Reload the entire process

Missing '.' at the end.

> +with_test_prefix "restart process" {
> +    mi_delete_breakpoints
> +    mi_gdb_load ${binfile}
> +    mi_runto_main
> +}
> +
> +with_test_prefix "in new process" {
> +    # The global var was invalidated when the objfile got unloaded.
> +    mi_gdb_test "-var-update global_var" \
> +	"\\^done,changelist=\\\[\{name=\"global_var\",in_scope=\"invalid\",has_more=\"0\"\}\]" \
> +	"global invalidated"
> +
> +    # Floating varobj should still be valid, but out of scope at the moment.
> +    mi_gdb_test "-var-update floating_local" \
> +	"\\^done,changelist=\\\[{name=\"floating_local\",in_scope=\"false\",type_changed=\"false\",has_more=\"0\"}\\\]" \
> +	"floating_local still valid but not in scope"

For me, this test is failing, the output looks like:

  -var-update floating_local
  ^done,changelist=[{name="floating_local",in_scope="invalid",has_more="0"}]
  (gdb) 
  FAIL: gdb.mi/mi-var-invalidate-shlib.exp: in new process: floating_local still valid but not in scope (unexpected output)

But, once the next patch is applied, the test starts to pass.  So maybe
this test just needs moving into the next patch?

> +}
> diff --git a/gdb/testsuite/lib/mi-support.exp b/gdb/testsuite/lib/mi-support.exp
> index e724b2eeb51..ca56e12b06b 100644
> --- a/gdb/testsuite/lib/mi-support.exp
> +++ b/gdb/testsuite/lib/mi-support.exp
> @@ -1121,7 +1121,7 @@ proc mi_runto_helper {func run_or_continue args} {
>    global mi_gdb_prompt expect_out
>    global hex decimal fullname_syntax
>  
> -  parse_args {{qualified}}
> +  parse_args {{qualified} {pending}}
>  
>    set test "mi runto $func"
>    if {$pending} {
> diff --git a/gdb/value.c b/gdb/value.c
> index 022fca91a42..b9d2937c608 100644
> --- a/gdb/value.c
> +++ b/gdb/value.c
> @@ -46,6 +46,7 @@
>  #include "cli/cli-style.h"
>  #include "expop.h"
>  #include "inferior.h"
> +#include "varobj.h"
>  
>  /* Definition of a user function.  */
>  struct internal_function
> @@ -2596,6 +2597,21 @@ preserve_one_internalvar (struct internalvar *var, struct objfile *objfile,
>      }
>  }
>  
> +static void
> +preserve_one_varobj (struct varobj *varobj, struct objfile *objfile,
> +		     htab_t copied_types)

This function should have a comment before it.

> +{
> +  if (varobj->type->is_objfile_owned ()
> +      && varobj->type->objfile_owner () == objfile)
> +    {
> +      varobj->type
> +	= copy_type_recursive (objfile, varobj->type, copied_types);
> +    }
> +
> +  if (varobj->value != nullptr)
> +    preserve_one_value (varobj->value.get (), objfile, copied_types);
> +}
> +
>  /* Update the internal variables and value history when OBJFILE is
>     discarded; we must copy the types out of the objfile.  New global types
>     will be created for every convenience variable which currently points to
> @@ -2617,6 +2633,11 @@ preserve_values (struct objfile *objfile)
>    for (var = internalvars; var; var = var->next)
>      preserve_one_internalvar (var, objfile, copied_types.get ());
>  
> +  /* For the remaining varobj, check that none has type owned by OBJFILE.  */
> +  all_root_varobjs ([&copied_types, objfile](struct varobj *varobj)
> +		    { preserve_one_varobj (varobj, objfile,
> +					   copied_types.get ()); });
> +

I think the formatting here is a little off.  Looking through other
examples in GDB I think the most common layout would be:

  all_root_varobjs ([&copied_types, objfile] (struct varobj *varobj)
                    {
		      preserve_one_varobj (varobj, objfile,
					   copied_types.get ());
		    });


>    preserve_ext_lang_values (objfile, copied_types.get ());
>  }
>  
> diff --git a/gdb/varobj.c b/gdb/varobj.c
> index 1aca015a21a..caaffe4ea70 100644
> --- a/gdb/varobj.c
> +++ b/gdb/varobj.c
> @@ -32,6 +32,7 @@
>  #include "parser-defs.h"
>  #include "gdbarch.h"
>  #include <algorithm>
> +#include "observable.h"
>  
>  #if HAVE_PYTHON
>  #include "python/python.h"
> @@ -72,6 +73,12 @@ struct varobj_root
>    /* The expression for this parent.  */
>    expression_up exp;
>  
> +  /* Cached arch from exp, for use in case exp gets invalidated.  */
> +  struct gdbarch *gdbarch = nullptr;
> +
> +  /* Cached language from exp, for use in case exp gets invalidated.  */
> +  const struct language_defn *language_defn = nullptr;
> +
>    /* Block for which this expression is valid.  */
>    const struct block *valid_block = NULL;
>  
> @@ -206,7 +213,7 @@ is_root_p (const struct varobj *var)
>  
>  /* See python-internal.h.  */
>  gdbpy_enter_varobj::gdbpy_enter_varobj (const struct varobj *var)
> -: gdbpy_enter (var->root->exp->gdbarch, var->root->exp->language_defn)
> +: gdbpy_enter (var->root->gdbarch, var->root->language_defn)
>  {
>  }
>  
> @@ -303,6 +310,11 @@ varobj_create (const char *objname,
>        try
>  	{
>  	  var->root->exp = parse_exp_1 (&p, pc, block, 0, &tracker);
> +
> +	  /* Cache gdbarch and language_defn as they might be used even
> +	     after var is invalidated and var->root->exp cleared.  */
> +	  var->root->gdbarch = var->root->exp->gdbarch;
> +	  var->root->language_defn = var->root->exp->language_defn;
>  	}
>  
>        catch (const gdb_exception_error &except)
> @@ -2071,6 +2083,12 @@ value_of_root (struct varobj **var_handle, bool *type_changed)
>    {
>      struct value *value;
>  
> +    /* The expression have been invalidated (because it did reference an

'...expression HAS been invalidated (because IT REFERENCED an ...'

> +       objfile which is not available anymore) and we did not manage to
> +       recerate it for a floating varobj.  */

'... RECREATE ...'

> +    if ((*var_handle)->root->exp == nullptr)
> +      return nullptr;
> +

I notice that non of the tests in either this patch, or the next one,
exercise this condition.

Is it possible to create a test for this case?


>      value = value_of_root_1 (var_handle);
>      if (var->value == NULL || value == NULL)
>        {
> @@ -2373,6 +2391,49 @@ varobj_invalidate (void)
>    all_root_varobjs (varobj_invalidate_iter);
>  }
>  
> +/* Ensure that no varobj keep references to OBJFILE.  */
> +
> +static void
> +varobj_invalidate_if_uses_objfile (struct objfile *objfile)
> +{
> +  auto varobj_invalidate_if_uses_objfile_worker
> +    = [objfile](struct varobj *var)
> +      {
> +	if (var->root->valid_block != nullptr
> +	    && block_objfile (var->root->valid_block) == objfile)
> +	  {
> +	    /* The varobj is tied to a block which is going away.  There is
> +	       no way to reconstruct something later, so invalidate the
> +	       varobj completly and drop the reference to the block which is
> +	       being freed.  */
> +	    var->root->is_valid = false;
> +	    var->root->valid_block = nullptr;
> +	  }
> +
> +	if (var->root->exp != nullptr
> +	    && exp_uses_objfile (var->root->exp.get (), objfile))
> +	  {
> +	    /* The varobj's current expression references the objfile.  For
> +	       globals and floating, it is possible that when we try to
> +	       re-evaluate the expression later it is still valid with
> +	       whatever is in scope at this moment.  Just invalidate the
> +	       expression for now.  */

maybe '....is in scope at THAT moment.' ?

> +	    var->root->exp.reset ();
> +
> +	    /* It only makes sense to keep a floating varobj around.  */
> +	    if (!var->root->floating)
> +	      var->root->is_valid = false;
> +	  }
> +
> +	/* var->value->type and var->type might also reference the objfile.
> +	   This is taken care of in value.c:preserve_values which deals with
> +	   making sure that objfile-owend types are changed with gdbarch-owned
> +	   equivalents.  */

maybe ' ...types are REPLACED with .... ' ?

Thanks,
Andrew

> +      };
> +
> +  all_root_varobjs (varobj_invalidate_if_uses_objfile_worker);
> +}
> +
>  /* A hash function for a varobj.  */
>  
>  static hashval_t
> @@ -2407,4 +2468,7 @@ _initialize_varobj ()
>  			     _("When non-zero, varobj debugging is enabled."),
>  			     NULL, show_varobjdebug,
>  			     &setdebuglist, &showdebuglist);
> +
> +  gdb::observers::free_objfile.attach (varobj_invalidate_if_uses_objfile,
> +				       "varobj");
>  }
> -- 
> 2.25.1

Re: [PATCH 2/3] gdb/varobj: Fix use after free in varobj

[Lancelot SIX]

Hi,

Thanks for the feedbacks and spot-on comments.  I'll integrate them in a 
V2 shortly.

>> +gdb_exit
>> +if { [mi_gdb_start] } {
>> +    return 0
>> +}
> 
> Could/should the gdb_exit and mi_gdb_start calls be replaced with:
> 
>    if { [mi_clean_restart] } {
>      # Should we have an unsupported call in here maybe?
>      return
>    }
> 

Yes, it should probably.  I'll change this in the V2.

This is what happens when we create a new test by copying an existing one…

>> +    # Floating varobj should still be valid, but out of scope at the moment.
>> +    mi_gdb_test "-var-update floating_local" \
>> +     "\\^done,changelist=\\\[{name=\"floating_local\",in_scope=\"false\",type_changed=\"false\",has_more=\"0\"}\\\]" \
>> +     "floating_local still valid but not in scope"
> 
> For me, this test is failing, the output looks like:
> 
>    -var-update floating_local
>    ^done,changelist=[{name="floating_local",in_scope="invalid",has_more="0"}]
>    (gdb)
>    FAIL: gdb.mi/mi-var-invalidate-shlib.exp: in new process: floating_local still valid but not in scope (unexpected output)
> 
> But, once the next patch is applied, the test starts to pass.  So maybe
> this test just needs moving into the next patch?

Indeed, this part of the test should really be in the patch #2.  Thanks 
for spotting this.


> 
> I notice that non of the tests in either this patch, or the next one,
> exercise this condition.
> 
> Is it possible to create a test for this case?

Will do.  Issuing a "-var-update" after the dlclose call in the test 
should exercise this.  I did that manually, but forgot to include it in 
the testcaes.

Best,
Lancelot.

Re: [PATCH 2/3] gdb/varobj: Fix use after free in varobj

[Lancelot SIX]

 >>> +    if ((*var_handle)->root->exp == nullptr)
 >>> +      return nullptr;
 >>> +
>>
>> I notice that non of the tests in either this patch, or the next one,
>> exercise this condition.
>>
>> Is it possible to create a test for this case?
> 
> Will do.  Issuing a "-var-update" after the dlclose call in the test 
> should exercise this.  I did that manually, but forgot to include it in 
> the testcaes.
> 

Actually, after double checking, this case is not possible.  We cannot 
reach this point if the the varobj is invalid.  The only way to have
`exp == nullptr` and a valid varobj is if the varobj floating.

In this particular case, code a couple of lines above ensure that either 
the expression is re-parsed (ensuring exp is not nullptr), or exit the 
all function early:

   if (var->root->floating)
     {
       struct varobj *tmp_var;

       tmp_var = varobj_create (NULL, var->name.c_str (), (CORE_ADDR) 0,
			       USE_SELECTED_FRAME);
       if (tmp_var == NULL)
	{
	  return NULL;
	}
       ...
     }

I'll drop this part of the patch in V2.

I will still update the testcase to still exercise the case where:

     var->root->floating && var->root->exp == nullptr

on entry of this function.

Best,
Lancelot.

Formatting/indentation of lambdas (Re: [PATCH 2/3] gdb/varobj: Fix use after free in varobj)

[Pedro Alves]

On 2022-06-17 17:09, Andrew Burgess via Gdb-patches wrote:

>>  /* Update the internal variables and value history when OBJFILE is
>>     discarded; we must copy the types out of the objfile.  New global types
>>     will be created for every convenience variable which currently points to
>> @@ -2617,6 +2633,11 @@ preserve_values (struct objfile *objfile)
>>    for (var = internalvars; var; var = var->next)
>>      preserve_one_internalvar (var, objfile, copied_types.get ());
>>  
>> +  /* For the remaining varobj, check that none has type owned by OBJFILE.  */
>> +  all_root_varobjs ([&copied_types, objfile](struct varobj *varobj)
>> +		    { preserve_one_varobj (varobj, objfile,
>> +					   copied_types.get ()); });
>> +
> 
> I think the formatting here is a little off.  Looking through other
> examples in GDB I think the most common layout would be:
> 
>   all_root_varobjs ([&copied_types, objfile] (struct varobj *varobj)
>                     {
> 		      preserve_one_varobj (varobj, objfile,
> 					   copied_types.get ());
> 		    });

For for-each-like functions that take a lambda, I like to indent the body
of lambda as-if you really had a for loop.  Like:

  all_root_varobjs ([&copied_types, objfile] (struct varobj *varobj)
    {
      preserve_one_varobj (varobj, objfile, copied_types.get ());
    });

which looks similar to what you'd have if you had a real for, like:

  for (varobj *varobj: root_varobjs ())
    {
      preserve_one_varobj (varobj, objfile, copied_types.get ());
    }


I'll give you at least a couple existing examples.  E.g., in gdb/linux-nat.c:

  /* No use iterating unless we're resuming other threads.  */
  if (scope_ptid != lp->ptid)
    iterate_over_lwps (scope_ptid, [=] (struct lwp_info *info)
      {
        return linux_nat_resume_callback (info, lp);
      });

and in gdbserver, any for_each_thread call, like:

  for_each_thread ([&] (thread_info *thread)
    {
      handle_qxfer_threads_worker (thread, buffer);
    });

I've noticed that LLVM also uses this style.  It is nicely described here:

 https://llvm.org/docs/CodingStandards.html#format-lambdas-like-blocks-of-code

For the case where we have multiple lambdas in a single function call, or when the
lambda isn't the last argument, I agree with LLVM, adjusted for GNU style.  As in, indent
like other parameters. An example for this is expand_symtabs_matching, which takes
several gdb::function_view arguments.  Here's an example call:

  /* Look through the partial symtabs for all symbols which begin by
     matching SYM_TEXT.  Expand all CUs that you find to the list.  */
  expand_symtabs_matching (NULL,
			   lookup_name,
			   NULL,
			   [&] (compunit_symtab *symtab) /* expansion notify */
			     {
			       add_symtab_completions (symtab,
						       tracker, mode, lookup_name,
						       sym_text, word, code);
			       return true;
			     },
			   SEARCH_GLOBAL_BLOCK | SEARCH_STATIC_BLOCK,
			   ALL_DOMAIN);

I suggest we follow these conventions, and document it in the internals manual,
so we have a url we can point to the next time this comes up (it's not the first time).

Re: Formatting/indentation of lambdas (Re: [PATCH 2/3] gdb/varobj: Fix use after free in varobj)

[Lancelot SIX]

Hi Pedro,

Thanks a lot for this.  I find conventions you describe appropriate, and 
the guidelines will be useful as I always wonder how to properly indent 
lambdas in GDB codebase.

I have updated the wiki to mention the rules you describe above: 
https://sourceware.org/gdb/wiki/Internals%20GDB-C-Coding-Standards#Indentation_of_lambdas_as_parameters

If someone objects to the rules, we can always revert / update the wiki 
accordingly.

Best,
Lancelot.

On 30/06/2022 19:43, Pedro Alves wrote:
> [CAUTION: External Email]
> 
> On 2022-06-17 17:09, Andrew Burgess via Gdb-patches wrote:
> 
>>>   /* Update the internal variables and value history when OBJFILE is
>>>      discarded; we must copy the types out of the objfile.  New global types
>>>      will be created for every convenience variable which currently points to
>>> @@ -2617,6 +2633,11 @@ preserve_values (struct objfile *objfile)
>>>     for (var = internalvars; var; var = var->next)
>>>       preserve_one_internalvar (var, objfile, copied_types.get ());
>>>
>>> +  /* For the remaining varobj, check that none has type owned by OBJFILE.  */
>>> +  all_root_varobjs ([&copied_types, objfile](struct varobj *varobj)
>>> +                { preserve_one_varobj (varobj, objfile,
>>> +                                       copied_types.get ()); });
>>> +
>>
>> I think the formatting here is a little off.  Looking through other
>> examples in GDB I think the most common layout would be:
>>
>>    all_root_varobjs ([&copied_types, objfile] (struct varobj *varobj)
>>                      {
>>                      preserve_one_varobj (varobj, objfile,
>>                                           copied_types.get ());
>>                    });
> 
> For for-each-like functions that take a lambda, I like to indent the body
> of lambda as-if you really had a for loop.  Like:
> 
>    all_root_varobjs ([&copied_types, objfile] (struct varobj *varobj)
>      {
>        preserve_one_varobj (varobj, objfile, copied_types.get ());
>      });
> 
> which looks similar to what you'd have if you had a real for, like:
> 
>    for (varobj *varobj: root_varobjs ())
>      {
>        preserve_one_varobj (varobj, objfile, copied_types.get ());
>      }
> 
> 
> I'll give you at least a couple existing examples.  E.g., in gdb/linux-nat.c:
> 
>    /* No use iterating unless we're resuming other threads.  */
>    if (scope_ptid != lp->ptid)
>      iterate_over_lwps (scope_ptid, [=] (struct lwp_info *info)
>        {
>          return linux_nat_resume_callback (info, lp);
>        });
> 
> and in gdbserver, any for_each_thread call, like:
> 
>    for_each_thread ([&] (thread_info *thread)
>      {
>        handle_qxfer_threads_worker (thread, buffer);
>      });
> 
> I've noticed that LLVM also uses this style.  It is nicely described here:
> 
>   https://llvm.org/docs/CodingStandards.html#format-lambdas-like-blocks-of-code
> 
> For the case where we have multiple lambdas in a single function call, or when the
> lambda isn't the last argument, I agree with LLVM, adjusted for GNU style.  As in, indent
> like other parameters. An example for this is expand_symtabs_matching, which takes
> several gdb::function_view arguments.  Here's an example call:
> 
>    /* Look through the partial symtabs for all symbols which begin by
>       matching SYM_TEXT.  Expand all CUs that you find to the list.  */
>    expand_symtabs_matching (NULL,
>                             lookup_name,
>                             NULL,
>                             [&] (compunit_symtab *symtab) /* expansion notify */
>                               {
>                                 add_symtab_completions (symtab,
>                                                         tracker, mode, lookup_name,
>                                                         sym_text, word, code);
>                                 return true;
>                               },
>                             SEARCH_GLOBAL_BLOCK | SEARCH_STATIC_BLOCK,
>                             ALL_DOMAIN);
> 
> I suggest we follow these conventions, and document it in the internals manual,
> so we have a url we can point to the next time this comes up (it's not the first time).