public inbox for gdb-patches@sourceware.org
 help / color / mirror / Atom feed
* [PATCH v2 00/13] Make frame_info_ptr automatic
@ 2022-12-14  3:34 Simon Marchi
  2022-12-14  3:34 ` [PATCH v2 01/13] gdb: move type_map_instance to compile/compile.c Simon Marchi
                   ` (15 more replies)
  0 siblings, 16 replies; 28+ messages in thread
From: Simon Marchi @ 2022-12-14  3:34 UTC (permalink / raw)
  To: gdb-patches; +Cc: Simon Marchi

This is v2 of:

  https://inbox.sourceware.org/gdb-patches/20221202180052.212745-1-simon.marchi@polymtl.ca/

The first main change is that I dropped the patch removing the user of
frame_info_ptr at many places, following Tom's comment.  If we end up
trying to reinflate a frame at a moment where it's not possible (because
the frame's id is being computed), we'll hit an assert in
frame_info_ptr.  This is fine, because it indicates some logic problem
in GDB.

The second main change, found after doing the first one, is to fix a bug
with the user-created frame reinflation.  In v1, if we had two
frame_info_ptr wrapping the same user-created frame_info object, they
would each create their own frame_info instance on reinflation.  And
that confused GDB down the line.  There are a few new patches to deal
with that, by adding user-created frames to the frame stash, so they can
be found by frame_info_ptr when reinflating.

At the end of the series, frame_info_ptr::frame_info_ptr (at the end of
the series) accesses some frame_info internals (the frame's id, without
calling get_frame_id), so I decided to move frame_info_ptr to
frame.{c,h} (patch 6).  I think that makes sense anyway, as the logic in
the implementation of frame_info_ptr is tightly coupled to the rest of
the frame stuff.  However, to do so, we need to break some inclusion
cycles.  This is done in the few patches before.  Patches 1 and 2 are
not strictly necessary, but fix oddities I found along the way.

Regression-tested on Ubuntu 22.04 x86-64.

Simon Marchi (13):
  gdb: move type_map_instance to compile/compile.c
  gdb: move compile_instance to compile/compile.h
  gdb: remove language.h include from frame.h
  gdb: move sect_offset and cu_offset to dwarf2/types.h
  gdb: move call site types to call-site.h
  gdb: move frame_info_ptr to frame.{c,h}
  gdb: add frame_id::user_created_p
  gdb: add user-created frames to stash
  gdb: add create_new_frame(frame_id) overload
  gdb: make it possible to restore selected user-created frames
  gdb: make user-created frames reinflatable
  gdb: make frame_info_ptr grab frame level and id on construction
  gdb: make frame_info_ptr auto-reinflatable

 gdb/Makefile.in                          |   2 +-
 gdb/aarch64-tdep.c                       |   1 +
 gdb/amd64-tdep.c                         |   1 +
 gdb/arm-tdep.c                           |   1 +
 gdb/c-lang.c                             |   1 -
 gdb/c-lang.h                             |   2 +
 gdb/compile/compile-c.h                  |   1 +
 gdb/compile/compile-cplus.h              |   1 +
 gdb/compile/compile-internal.h           | 129 ------------
 gdb/compile/compile.c                    |  13 ++
 gdb/compile/compile.h                    | 116 +++++++++++
 gdb/cp-abi.c                             |   1 +
 gdb/cp-support.c                         |   1 +
 gdb/dwarf2/abbrev-cache.h                |   1 -
 gdb/dwarf2/attribute.h                   |   2 +-
 gdb/dwarf2/call-site.h                   | 244 +++++++++++++++++++++++
 gdb/dwarf2/comp-unit-head.h              |   5 +-
 gdb/dwarf2/cooked-index.h                |   2 +-
 gdb/dwarf2/expr.h                        |   2 +-
 gdb/dwarf2/line-header.h                 |   2 -
 gdb/dwarf2/types.h                       |  40 ++++
 gdb/expop.h                              |   1 +
 gdb/f-lang.h                             |   1 +
 gdb/frame-id.h                           |   4 +
 gdb/frame-info.c                         |  65 ------
 gdb/frame-info.h                         | 211 --------------------
 gdb/frame.c                              | 127 ++++++++++--
 gdb/frame.h                              | 206 ++++++++++++++++++-
 gdb/gdbtypes.h                           | 229 ---------------------
 gdb/gnu-v3-abi.c                         |   1 +
 gdb/go-lang.h                            |   1 +
 gdb/infcmd.c                             |   2 -
 gdb/language.c                           |   1 -
 gdb/m2-typeprint.c                       |   1 +
 gdb/mi/mi-cmd-stack.c                    |   2 -
 gdb/ppc-sysv-tdep.c                      |   1 +
 gdb/python/py-disasm.c                   |   1 +
 gdb/python/py-frame.c                    |   1 +
 gdb/stack.c                              |  16 --
 gdb/symtab.c                             |   1 +
 gdb/symtab.h                             |   1 +
 gdb/testsuite/gdb.base/frame-view.c      |  80 ++++++++
 gdb/testsuite/gdb.base/frame-view.exp    | 109 ++++++++++
 gdb/testsuite/gdb.base/frame-view.py     |  41 ++++
 gdb/thread.c                             |   1 +
 gdb/unittests/frame_info_ptr-selftests.c |  76 +++++++
 46 files changed, 1065 insertions(+), 683 deletions(-)
 create mode 100644 gdb/dwarf2/call-site.h
 create mode 100644 gdb/dwarf2/types.h
 delete mode 100644 gdb/frame-info.c
 delete mode 100644 gdb/frame-info.h
 create mode 100644 gdb/testsuite/gdb.base/frame-view.c
 create mode 100644 gdb/testsuite/gdb.base/frame-view.exp
 create mode 100644 gdb/testsuite/gdb.base/frame-view.py
 create mode 100644 gdb/unittests/frame_info_ptr-selftests.c


base-commit: a4b83845ded201e7d46220213d3e38950c30dbb5
-- 
2.38.1


^ permalink raw reply	[flat|nested] 28+ messages in thread

* [PATCH v2 01/13] gdb: move type_map_instance to compile/compile.c
  2022-12-14  3:34 [PATCH v2 00/13] Make frame_info_ptr automatic Simon Marchi
@ 2022-12-14  3:34 ` Simon Marchi
  2022-12-14  3:34 ` [PATCH v2 02/13] gdb: move compile_instance to compile/compile.h Simon Marchi
                   ` (14 subsequent siblings)
  15 siblings, 0 replies; 28+ messages in thread
From: Simon Marchi @ 2022-12-14  3:34 UTC (permalink / raw)
  To: gdb-patches; +Cc: Simon Marchi

From: Simon Marchi <simon.marchi@efficios.com>

It's only used in compile/compile.c, it doesn't need to be in a header.

Change-Id: Ic5bec996b7b0cd7130055d1e8ff238b5ac4292a3
---
 gdb/compile/compile-internal.h | 13 -------------
 gdb/compile/compile.c          | 13 +++++++++++++
 2 files changed, 13 insertions(+), 13 deletions(-)

diff --git a/gdb/compile/compile-internal.h b/gdb/compile/compile-internal.h
index 042ddf9c5f92..0d3fcd76f0c0 100644
--- a/gdb/compile/compile-internal.h
+++ b/gdb/compile/compile-internal.h
@@ -26,19 +26,6 @@ extern bool compile_debug;
 
 struct block;
 
-/* An object that maps a gdb type to a gcc type.  */
-
-struct type_map_instance
-{
-  /* The gdb type.  */
-
-  struct type *type;
-
-  /* The corresponding gcc type handle.  */
-
-  gcc_type gcc_type_handle;
-};
-
 /* An object of this type holds state associated with a given
    compilation job.  */
 
diff --git a/gdb/compile/compile.c b/gdb/compile/compile.c
index 2843f05fdd7d..65bbf6711635 100644
--- a/gdb/compile/compile.c
+++ b/gdb/compile/compile.c
@@ -74,6 +74,19 @@ struct symbol_error
   char *message;
 };
 
+/* An object that maps a gdb type to a gcc type.  */
+
+struct type_map_instance
+{
+  /* The gdb type.  */
+
+  struct type *type;
+
+  /* The corresponding gcc type handle.  */
+
+  gcc_type gcc_type_handle;
+};
+
 /* Hash a type_map_instance.  */
 
 static hashval_t
-- 
2.38.1


^ permalink raw reply	[flat|nested] 28+ messages in thread

* [PATCH v2 02/13] gdb: move compile_instance to compile/compile.h
  2022-12-14  3:34 [PATCH v2 00/13] Make frame_info_ptr automatic Simon Marchi
  2022-12-14  3:34 ` [PATCH v2 01/13] gdb: move type_map_instance to compile/compile.c Simon Marchi
@ 2022-12-14  3:34 ` Simon Marchi
  2022-12-14  3:34 ` [PATCH v2 03/13] gdb: remove language.h include from frame.h Simon Marchi
                   ` (13 subsequent siblings)
  15 siblings, 0 replies; 28+ messages in thread
From: Simon Marchi @ 2022-12-14  3:34 UTC (permalink / raw)
  To: gdb-patches; +Cc: Simon Marchi

From: Simon Marchi <simon.marchi@efficios.com>

struct compile_instance needs to be visible to users, since we use
std::unique<compile_instance>.  language.c and c-lang.c currently
includes compile-internal.h for this reason, which kind of defeats the
purpose of having an "internal" header file.

Change-Id: Iedffe5f1173b3de7bdc1be533ee2a68e6f6c549f
---
 gdb/c-lang.c                   |   1 -
 gdb/c-lang.h                   |   1 +
 gdb/compile/compile-c.h        |   1 +
 gdb/compile/compile-cplus.h    |   1 +
 gdb/compile/compile-internal.h | 116 ---------------------------------
 gdb/compile/compile.h          | 116 +++++++++++++++++++++++++++++++++
 gdb/language.c                 |   1 -
 7 files changed, 119 insertions(+), 118 deletions(-)

diff --git a/gdb/c-lang.c b/gdb/c-lang.c
index 46c0da0ff797..0a10619f38fe 100644
--- a/gdb/c-lang.c
+++ b/gdb/c-lang.c
@@ -36,7 +36,6 @@
 #include <ctype.h>
 #include "gdbcore.h"
 #include "gdbarch.h"
-#include "compile/compile-internal.h"
 #include "c-exp.h"
 
 /* Given a C string type, STR_TYPE, return the corresponding target
diff --git a/gdb/c-lang.h b/gdb/c-lang.h
index 652f147f6569..c2db16e0bf07 100644
--- a/gdb/c-lang.h
+++ b/gdb/c-lang.h
@@ -26,6 +26,7 @@ struct language_arch_info;
 struct type_print_options;
 struct parser_state;
 
+#include "compile/compile.h"
 #include "value.h"
 #include "macroexp.h"
 #include "gdbsupport/enum-flags.h"
diff --git a/gdb/compile/compile-c.h b/gdb/compile/compile-c.h
index 4f7e5205eb7e..db17d5eedeb6 100644
--- a/gdb/compile/compile-c.h
+++ b/gdb/compile/compile-c.h
@@ -17,6 +17,7 @@
 #ifndef COMPILE_COMPILE_C_H
 #define COMPILE_COMPILE_C_H
 
+#include "compile/compile.h"
 #include "gdbsupport/enum-flags.h"
 #include "gcc-c-plugin.h"
 
diff --git a/gdb/compile/compile-cplus.h b/gdb/compile/compile-cplus.h
index 4f633e3eb585..b17f1c9c40a6 100644
--- a/gdb/compile/compile-cplus.h
+++ b/gdb/compile/compile-cplus.h
@@ -17,6 +17,7 @@
 #ifndef COMPILE_COMPILE_CPLUS_H
 #define COMPILE_COMPILE_CPLUS_H
 
+#include "compile/compile.h"
 #include "gdbsupport/enum-flags.h"
 #include "gcc-cp-plugin.h"
 #include "symtab.h"
diff --git a/gdb/compile/compile-internal.h b/gdb/compile/compile-internal.h
index 0d3fcd76f0c0..4607f33738cc 100644
--- a/gdb/compile/compile-internal.h
+++ b/gdb/compile/compile-internal.h
@@ -24,122 +24,6 @@
 
 extern bool compile_debug;
 
-struct block;
-
-/* An object of this type holds state associated with a given
-   compilation job.  */
-
-class compile_instance
-{
-public:
-  compile_instance (struct gcc_base_context *gcc_fe, const char *options);
-
-  virtual ~compile_instance ()
-  {
-    m_gcc_fe->ops->destroy (m_gcc_fe);
-  }
-
-  /* Returns the GCC options to be passed during compilation.  */
-  const std::string &gcc_target_options () const
-  {
-    return m_gcc_target_options;
-  }
-
-  /* Query the type cache for TYPE, returning the compiler's
-     type for it in RET.  */
-  bool get_cached_type (struct type *type, gcc_type *ret) const;
-
-  /* Insert GCC_TYPE into the type cache for TYPE.
-
-     It is ok for a given type to be inserted more than once, provided that
-     the exact same association is made each time.  */
-  void insert_type (struct type *type, gcc_type gcc_type);
-
-  /* Associate SYMBOL with some error text.  */
-  void insert_symbol_error (const struct symbol *sym, const char *text);
-
-  /* Emit the error message corresponding to SYM, if one exists, and
-     arrange for it not to be emitted again.  */
-  void error_symbol_once (const struct symbol *sym);
-
-  /* These currently just forward to the underlying ops
-     vtable.  */
-
-  /* Set the plug-in print callback.  */
-  void set_print_callback (void (*print_function) (void *, const char *),
-			   void *datum);
-
-  /* Return the plug-in's front-end version.  */
-  unsigned int version () const;
-
-  /* Set the plug-in's verbosity level.  Nop for GCC_FE_VERSION_0.  */
-  void set_verbose (int level);
-
-  /* Set the plug-in driver program.  Nop for GCC_FE_VERSION_0.  */
-  void set_driver_filename (const char *filename);
-
-  /* Set the regular expression used to match the configury triplet
-     prefix to the compiler.  Nop for GCC_FE_VERSION_0.  */
-  void set_triplet_regexp (const char *regexp);
-
-  /* Set compilation arguments.  REGEXP is only used for protocol
-     version GCC_FE_VERSION_0.  */
-  gdb::unique_xmalloc_ptr<char> set_arguments (int argc, char **argv,
-					       const char *regexp = NULL);
-
-  /* Set the filename of the program to compile.  Nop for GCC_FE_VERSION_0.  */
-  void set_source_file (const char *filename);
-
-  /* Compile the previously specified source file to FILENAME.
-     VERBOSE_LEVEL is only used for protocol version GCC_FE_VERSION_0.  */
-  bool compile (const char *filename, int verbose_level = -1);
-
-  /* Set the scope type for this compile.  */
-  void set_scope (enum compile_i_scope_types scope)
-  {
-    m_scope = scope;
-  }
-
-  /* Return the scope type.  */
-  enum compile_i_scope_types scope () const
-  {
-    return m_scope;
-  }
-
-  /* Set the block to be used for symbol searches.  */
-  void set_block (const struct block *block)
-  {
-    m_block = block;
-  }
-
-  /* Return the search block.  */
-  const struct block *block () const
-  {
-    return m_block;
-  }
-
-protected:
-
-  /* The GCC front end.  */
-  struct gcc_base_context *m_gcc_fe;
-
-  /* The "scope" of this compilation.  */
-  enum compile_i_scope_types m_scope;
-
-  /* The block in which an expression is being parsed.  */
-  const struct block *m_block;
-
-  /* Specify "-std=gnu11", "-std=gnu++11" or similar.  These options are put
-     after CU's DW_AT_producer compilation options to override them.  */
-  std::string m_gcc_target_options;
-
-  /* Map from gdb types to gcc types.  */
-  htab_up m_type_map;
-
-  /* Map from gdb symbols to gcc error messages to emit.  */
-  htab_up m_symbol_err_map;
-};
-
 /* Define header and footers for different scopes.  */
 
 /* A simple scope just declares a function named "_gdb_expr", takes no
diff --git a/gdb/compile/compile.h b/gdb/compile/compile.h
index d517660dd40a..877509544787 100644
--- a/gdb/compile/compile.h
+++ b/gdb/compile/compile.h
@@ -18,6 +18,8 @@
 #ifndef COMPILE_COMPILE_H
 #define COMPILE_COMPILE_H
 
+#include "gcc-c-interface.h"
+
 struct ui_file;
 struct gdbarch;
 struct dwarf2_per_cu_data;
@@ -25,6 +27,120 @@ struct dwarf2_per_objfile;
 struct symbol;
 struct dynamic_prop;
 
+/* An object of this type holds state associated with a given
+   compilation job.  */
+
+class compile_instance
+{
+public:
+  compile_instance (struct gcc_base_context *gcc_fe, const char *options);
+
+  virtual ~compile_instance ()
+  {
+    m_gcc_fe->ops->destroy (m_gcc_fe);
+  }
+
+  /* Returns the GCC options to be passed during compilation.  */
+  const std::string &gcc_target_options () const
+  {
+    return m_gcc_target_options;
+  }
+
+  /* Query the type cache for TYPE, returning the compiler's
+     type for it in RET.  */
+  bool get_cached_type (struct type *type, gcc_type *ret) const;
+
+  /* Insert GCC_TYPE into the type cache for TYPE.
+
+     It is ok for a given type to be inserted more than once, provided that
+     the exact same association is made each time.  */
+  void insert_type (struct type *type, gcc_type gcc_type);
+
+  /* Associate SYMBOL with some error text.  */
+  void insert_symbol_error (const struct symbol *sym, const char *text);
+
+  /* Emit the error message corresponding to SYM, if one exists, and
+     arrange for it not to be emitted again.  */
+  void error_symbol_once (const struct symbol *sym);
+
+  /* These currently just forward to the underlying ops
+     vtable.  */
+
+  /* Set the plug-in print callback.  */
+  void set_print_callback (void (*print_function) (void *, const char *),
+			   void *datum);
+
+  /* Return the plug-in's front-end version.  */
+  unsigned int version () const;
+
+  /* Set the plug-in's verbosity level.  Nop for GCC_FE_VERSION_0.  */
+  void set_verbose (int level);
+
+  /* Set the plug-in driver program.  Nop for GCC_FE_VERSION_0.  */
+  void set_driver_filename (const char *filename);
+
+  /* Set the regular expression used to match the configury triplet
+     prefix to the compiler.  Nop for GCC_FE_VERSION_0.  */
+  void set_triplet_regexp (const char *regexp);
+
+  /* Set compilation arguments.  REGEXP is only used for protocol
+     version GCC_FE_VERSION_0.  */
+  gdb::unique_xmalloc_ptr<char> set_arguments (int argc, char **argv,
+					       const char *regexp = NULL);
+
+  /* Set the filename of the program to compile.  Nop for GCC_FE_VERSION_0.  */
+  void set_source_file (const char *filename);
+
+  /* Compile the previously specified source file to FILENAME.
+     VERBOSE_LEVEL is only used for protocol version GCC_FE_VERSION_0.  */
+  bool compile (const char *filename, int verbose_level = -1);
+
+  /* Set the scope type for this compile.  */
+  void set_scope (enum compile_i_scope_types scope)
+  {
+    m_scope = scope;
+  }
+
+  /* Return the scope type.  */
+  enum compile_i_scope_types scope () const
+  {
+    return m_scope;
+  }
+
+  /* Set the block to be used for symbol searches.  */
+  void set_block (const struct block *block)
+  {
+    m_block = block;
+  }
+
+  /* Return the search block.  */
+  const struct block *block () const
+  {
+    return m_block;
+  }
+
+protected:
+
+  /* The GCC front end.  */
+  struct gcc_base_context *m_gcc_fe;
+
+  /* The "scope" of this compilation.  */
+  enum compile_i_scope_types m_scope;
+
+  /* The block in which an expression is being parsed.  */
+  const struct block *m_block;
+
+  /* Specify "-std=gnu11", "-std=gnu++11" or similar.  These options are put
+     after CU's DW_AT_producer compilation options to override them.  */
+  std::string m_gcc_target_options;
+
+  /* Map from gdb types to gcc types.  */
+  htab_up m_type_map;
+
+  /* Map from gdb symbols to gcc error messages to emit.  */
+  htab_up m_symbol_err_map;
+};
+
 /* Public function that is called from compile_control case in the
    expression command.  GDB returns either a CMD, or a CMD_STRING, but
    never both.  */
diff --git a/gdb/language.c b/gdb/language.c
index 3962ee8fa246..df4a06cbcc2a 100644
--- a/gdb/language.c
+++ b/gdb/language.c
@@ -46,7 +46,6 @@
 #include "c-lang.h"
 #include <algorithm>
 #include "gdbarch.h"
-#include "compile/compile-internal.h"
 
 static void set_range_case (void);
 
-- 
2.38.1


^ permalink raw reply	[flat|nested] 28+ messages in thread

* [PATCH v2 03/13] gdb: remove language.h include from frame.h
  2022-12-14  3:34 [PATCH v2 00/13] Make frame_info_ptr automatic Simon Marchi
  2022-12-14  3:34 ` [PATCH v2 01/13] gdb: move type_map_instance to compile/compile.c Simon Marchi
  2022-12-14  3:34 ` [PATCH v2 02/13] gdb: move compile_instance to compile/compile.h Simon Marchi
@ 2022-12-14  3:34 ` Simon Marchi
  2022-12-14  3:34 ` [PATCH v2 04/13] gdb: move sect_offset and cu_offset to dwarf2/types.h Simon Marchi
                   ` (12 subsequent siblings)
  15 siblings, 0 replies; 28+ messages in thread
From: Simon Marchi @ 2022-12-14  3:34 UTC (permalink / raw)
  To: gdb-patches; +Cc: Simon Marchi

From: Simon Marchi <simon.marchi@efficios.com>

This helps resolve some cyclic include problem later in the series.
The only language-related thing frame.h needs is enum language, and that
is in defs.h.

Doing so reveals that a bunch of files were relying on frame.h to
include language.h, so fix the fallouts here and there.

Change-Id: I178a7efec1953c2d088adb58483bade1f349b705
---
 gdb/aarch64-tdep.c     | 1 +
 gdb/amd64-tdep.c       | 1 +
 gdb/arm-tdep.c         | 1 +
 gdb/cp-abi.c           | 1 +
 gdb/cp-support.c       | 1 +
 gdb/expop.h            | 1 +
 gdb/f-lang.h           | 1 +
 gdb/frame.h            | 1 -
 gdb/gnu-v3-abi.c       | 1 +
 gdb/go-lang.h          | 1 +
 gdb/m2-typeprint.c     | 1 +
 gdb/ppc-sysv-tdep.c    | 1 +
 gdb/python/py-disasm.c | 1 +
 gdb/python/py-frame.c  | 1 +
 gdb/thread.c           | 1 +
 15 files changed, 14 insertions(+), 1 deletion(-)

diff --git a/gdb/aarch64-tdep.c b/gdb/aarch64-tdep.c
index 024385a9fd81..6db50c710834 100644
--- a/gdb/aarch64-tdep.c
+++ b/gdb/aarch64-tdep.c
@@ -21,6 +21,7 @@
 #include "defs.h"
 
 #include "frame.h"
+#include "language.h"
 #include "gdbcmd.h"
 #include "gdbcore.h"
 #include "dis-asm.h"
diff --git a/gdb/amd64-tdep.c b/gdb/amd64-tdep.c
index bbfc509319cb..4ed547e6e520 100644
--- a/gdb/amd64-tdep.c
+++ b/gdb/amd64-tdep.c
@@ -20,6 +20,7 @@
    along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
 
 #include "defs.h"
+#include "language.h"
 #include "opcode/i386.h"
 #include "dis-asm.h"
 #include "arch-utils.h"
diff --git a/gdb/arm-tdep.c b/gdb/arm-tdep.c
index 6f02f04b5cb2..d291fbd8645d 100644
--- a/gdb/arm-tdep.c
+++ b/gdb/arm-tdep.c
@@ -22,6 +22,7 @@
 #include <ctype.h>		/* XXX for isupper ().  */
 
 #include "frame.h"
+#include "language.h"
 #include "inferior.h"
 #include "infrun.h"
 #include "gdbcmd.h"
diff --git a/gdb/cp-abi.c b/gdb/cp-abi.c
index c32b1f7d2f04..8c24c9d4ac61 100644
--- a/gdb/cp-abi.c
+++ b/gdb/cp-abi.c
@@ -18,6 +18,7 @@
    along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
 
 #include "defs.h"
+#include "language.h"
 #include "value.h"
 #include "cp-abi.h"
 #include "command.h"
diff --git a/gdb/cp-support.c b/gdb/cp-support.c
index 77895a8bc98a..50cf00453177 100644
--- a/gdb/cp-support.c
+++ b/gdb/cp-support.c
@@ -20,6 +20,7 @@
 
 #include "defs.h"
 #include "cp-support.h"
+#include "language.h"
 #include "demangle.h"
 #include "gdbcmd.h"
 #include "dictionary.h"
diff --git a/gdb/expop.h b/gdb/expop.h
index 635580da2205..ed10b6dc36cc 100644
--- a/gdb/expop.h
+++ b/gdb/expop.h
@@ -24,6 +24,7 @@
 #include "c-lang.h"
 #include "cp-abi.h"
 #include "expression.h"
+#include "language.h"
 #include "objfiles.h"
 #include "gdbsupport/traits.h"
 #include "gdbsupport/enum-flags.h"
diff --git a/gdb/f-lang.h b/gdb/f-lang.h
index 6a97fabbd48a..afff9c8217d5 100644
--- a/gdb/f-lang.h
+++ b/gdb/f-lang.h
@@ -23,6 +23,7 @@
 #ifndef F_LANG_H
 #define F_LANG_H
 
+#include "language.h"
 #include "valprint.h"
 
 struct type_print_options;
diff --git a/gdb/frame.h b/gdb/frame.h
index cf8bbc6a52bd..5c292cd2b952 100644
--- a/gdb/frame.h
+++ b/gdb/frame.h
@@ -71,7 +71,6 @@
 
    */
 
-#include "language.h"
 #include "cli/cli-option.h"
 #include "gdbsupport/common-debug.h"
 
diff --git a/gdb/gnu-v3-abi.c b/gdb/gnu-v3-abi.c
index 2f000e634a4f..136630cda3b6 100644
--- a/gdb/gnu-v3-abi.c
+++ b/gdb/gnu-v3-abi.c
@@ -19,6 +19,7 @@
    along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
 
 #include "defs.h"
+#include "language.h"
 #include "value.h"
 #include "cp-abi.h"
 #include "cp-support.h"
diff --git a/gdb/go-lang.h b/gdb/go-lang.h
index 523501bba4c3..5a098dc8a3eb 100644
--- a/gdb/go-lang.h
+++ b/gdb/go-lang.h
@@ -22,6 +22,7 @@
 
 struct type_print_options;
 
+#include "language.h"
 #include "gdbtypes.h"
 #include "symtab.h"
 #include "value.h"
diff --git a/gdb/m2-typeprint.c b/gdb/m2-typeprint.c
index 67afddd5c391..6d1be6ff8b36 100644
--- a/gdb/m2-typeprint.c
+++ b/gdb/m2-typeprint.c
@@ -17,6 +17,7 @@
    along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
 
 #include "defs.h"
+#include "language.h"
 #include "gdbsupport/gdb_obstack.h"
 #include "bfd.h"		/* Binary File Description */
 #include "symtab.h"
diff --git a/gdb/ppc-sysv-tdep.c b/gdb/ppc-sysv-tdep.c
index 32d007235660..ac721aa0ca1e 100644
--- a/gdb/ppc-sysv-tdep.c
+++ b/gdb/ppc-sysv-tdep.c
@@ -19,6 +19,7 @@
    along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
 
 #include "defs.h"
+#include "language.h"
 #include "gdbcore.h"
 #include "inferior.h"
 #include "regcache.h"
diff --git a/gdb/python/py-disasm.c b/gdb/python/py-disasm.c
index a25252b43067..242cf6a57582 100644
--- a/gdb/python/py-disasm.c
+++ b/gdb/python/py-disasm.c
@@ -19,6 +19,7 @@
 
 #include "defs.h"
 #include "python-internal.h"
+#include "language.h"
 #include "dis-asm.h"
 #include "arch-utils.h"
 #include "charset.h"
diff --git a/gdb/python/py-frame.c b/gdb/python/py-frame.c
index cbce9457755c..fac51a420320 100644
--- a/gdb/python/py-frame.c
+++ b/gdb/python/py-frame.c
@@ -18,6 +18,7 @@
    along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
 
 #include "defs.h"
+#include "language.h"
 #include "charset.h"
 #include "block.h"
 #include "frame.h"
diff --git a/gdb/thread.c b/gdb/thread.c
index cd7f1a7d5bb4..10ecda63e8b7 100644
--- a/gdb/thread.c
+++ b/gdb/thread.c
@@ -20,6 +20,7 @@
    along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
 
 #include "defs.h"
+#include "language.h"
 #include "symtab.h"
 #include "frame.h"
 #include "inferior.h"
-- 
2.38.1


^ permalink raw reply	[flat|nested] 28+ messages in thread

* [PATCH v2 04/13] gdb: move sect_offset and cu_offset to dwarf2/types.h
  2022-12-14  3:34 [PATCH v2 00/13] Make frame_info_ptr automatic Simon Marchi
                   ` (2 preceding siblings ...)
  2022-12-14  3:34 ` [PATCH v2 03/13] gdb: remove language.h include from frame.h Simon Marchi
@ 2022-12-14  3:34 ` Simon Marchi
  2022-12-14  3:34 ` [PATCH v2 05/13] gdb: move call site types to call-site.h Simon Marchi
                   ` (11 subsequent siblings)
  15 siblings, 0 replies; 28+ messages in thread
From: Simon Marchi @ 2022-12-14  3:34 UTC (permalink / raw)
  To: gdb-patches; +Cc: Simon Marchi

From: Simon Marchi <simon.marchi@efficios.com>

I want to move the call_site stuff out of gdbtypes.h, to a new header
file, to break some cyclic include problem.  The call_site stuff uses
cu_offset, also defined in gdbtypes.h, so cu_offset also needs to move
somewhere else (otherwise, call-site.h will need to include gdbtypes.h,
and we are back to square 1).  I could move cu_offset to the future new
file dwarf2/call-site.h, but it doesn't sound like a good place for it,
at cu_offset is not specific to call sites, it's used throughout
dwarf2/.  So, move it to its own file, dwarf2/types.h.  For now,
gdbtypes.h includes dwarf2/types.h, but that will be removed once the
call site stuff is moved to its own file.

Move sect_offset with it too.  sect_offset is not a DWARF-specific
concept, but for the moment it is only used in dwarf2/.

Change-Id: I1fd2a3b7b67dee789c4874244b044bde7db43d8e
---
 gdb/dwarf2/types.h | 39 +++++++++++++++++++++++++++++++++++++++
 gdb/gdbtypes.h     | 18 +-----------------
 2 files changed, 40 insertions(+), 17 deletions(-)
 create mode 100644 gdb/dwarf2/types.h

diff --git a/gdb/dwarf2/types.h b/gdb/dwarf2/types.h
new file mode 100644
index 000000000000..463dc485925f
--- /dev/null
+++ b/gdb/dwarf2/types.h
@@ -0,0 +1,39 @@
+/* Common internal types for the DWARF reader
+
+   Copyright (C) 2017-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/>.  */
+
+#ifndef DWARF2_TYPES_H
+#define DWARF2_TYPES_H
+
+#include "gdbsupport/offset-type.h"
+
+/* Offset relative to the start of its containing CU (compilation
+   unit).  */
+DEFINE_OFFSET_TYPE (cu_offset, unsigned int);
+
+/* Offset relative to the start of its .debug_info or .debug_types
+   section.  */
+DEFINE_OFFSET_TYPE (sect_offset, uint64_t);
+
+static inline char *
+sect_offset_str (sect_offset offset)
+{
+  return hex_string (to_underlying (offset));
+}
+
+#endif /* DWARF2_TYPES_H */
diff --git a/gdb/gdbtypes.h b/gdb/gdbtypes.h
index 8fc5c97c95aa..fbe46323216f 100644
--- a/gdb/gdbtypes.h
+++ b/gdb/gdbtypes.h
@@ -57,6 +57,7 @@
 #include "gdbsupport/gdb_obstack.h"
 #include "gmp-utils.h"
 #include "frame-info.h"
+#include "dwarf2/types.h"
 
 /* Forward declarations for prototypes.  */
 struct field;
@@ -66,23 +67,6 @@ struct language_defn;
 struct dwarf2_per_cu_data;
 struct dwarf2_per_objfile;
 
-/* These declarations are DWARF-specific as some of the gdbtypes.h data types
-   are already DWARF-specific.  */
-
-/* * Offset relative to the start of its containing CU (compilation
-   unit).  */
-DEFINE_OFFSET_TYPE (cu_offset, unsigned int);
-
-/* * Offset relative to the start of its .debug_info or .debug_types
-   section.  */
-DEFINE_OFFSET_TYPE (sect_offset, uint64_t);
-
-static inline char *
-sect_offset_str (sect_offset offset)
-{
-  return hex_string (to_underlying (offset));
-}
-
 /* Some macros for char-based bitfields.  */
 
 #define B_SET(a,x)	((a)[(x)>>3] |= (1 << ((x)&7)))
-- 
2.38.1


^ permalink raw reply	[flat|nested] 28+ messages in thread

* [PATCH v2 05/13] gdb: move call site types to call-site.h
  2022-12-14  3:34 [PATCH v2 00/13] Make frame_info_ptr automatic Simon Marchi
                   ` (3 preceding siblings ...)
  2022-12-14  3:34 ` [PATCH v2 04/13] gdb: move sect_offset and cu_offset to dwarf2/types.h Simon Marchi
@ 2022-12-14  3:34 ` Simon Marchi
  2022-12-14  3:34 ` [PATCH v2 06/13] gdb: move frame_info_ptr to frame.{c,h} Simon Marchi
                   ` (10 subsequent siblings)
  15 siblings, 0 replies; 28+ messages in thread
From: Simon Marchi @ 2022-12-14  3:34 UTC (permalink / raw)
  To: gdb-patches; +Cc: Simon Marchi

From: Simon Marchi <simon.marchi@efficios.com>

I hesitated between putting  the file in the dwarf2 directory (as
gdb/dwarf2/call-site.h) or in the common directory (as gdb/call-site.h).
The concept of call site is not DWARF-specific, another debug info
reader could provide this information.  But as it is, the implementation
is a bit DWARF-specific, as one form it can take is a DWARF expression
and parameters can be defined using a DWARF register number.  So I ended up
choosing to put it under dwarf2/.  If another debug info reader ever
wants to provide call site information, we can introduce a layer of
abstraction between the "common" call site and the "dwarf2" call site.

The copyright start year comes from the date `struct call_site` was
introduced.

Change-Id: I1cd84aa581fbbf729edc91b20f7d7a6e0377014d
---
 gdb/dwarf2/abbrev-cache.h   |   1 -
 gdb/dwarf2/attribute.h      |   2 +-
 gdb/dwarf2/call-site.h      | 244 ++++++++++++++++++++++++++++++++++++
 gdb/dwarf2/comp-unit-head.h |   5 +-
 gdb/dwarf2/cooked-index.h   |   2 +-
 gdb/dwarf2/expr.h           |   2 +-
 gdb/dwarf2/line-header.h    |   2 -
 gdb/dwarf2/types.h          |   1 +
 gdb/gdbtypes.h              | 211 -------------------------------
 gdb/symtab.c                |   1 +
 10 files changed, 253 insertions(+), 218 deletions(-)
 create mode 100644 gdb/dwarf2/call-site.h

diff --git a/gdb/dwarf2/abbrev-cache.h b/gdb/dwarf2/abbrev-cache.h
index d729eb9dd1bb..c56a7aba217a 100644
--- a/gdb/dwarf2/abbrev-cache.h
+++ b/gdb/dwarf2/abbrev-cache.h
@@ -21,7 +21,6 @@
 #define GDB_DWARF2_ABBREV_CACHE_H
 
 #include "dwarf2/abbrev.h"
-#include "gdbtypes.h"
 
 /* An abbrev cache holds abbrev tables for easier reuse.  */
 class abbrev_cache
diff --git a/gdb/dwarf2/attribute.h b/gdb/dwarf2/attribute.h
index 447d195eb9a4..aa6380cb11fb 100644
--- a/gdb/dwarf2/attribute.h
+++ b/gdb/dwarf2/attribute.h
@@ -28,7 +28,7 @@
 #define GDB_DWARF2_ATTRIBUTE_H
 
 #include "dwarf2.h"
-#include "gdbtypes.h"
+#include "dwarf2/types.h"
 #include "gdbsupport/gdb_optional.h"
 
 /* Blocks are a bunch of untyped bytes.  */
diff --git a/gdb/dwarf2/call-site.h b/gdb/dwarf2/call-site.h
new file mode 100644
index 000000000000..f5bee967594e
--- /dev/null
+++ b/gdb/dwarf2/call-site.h
@@ -0,0 +1,244 @@
+/* Call site information.
+
+   Copyright (C) 2011-2022 Free Software Foundation, Inc.
+
+   Contributed by Cygnus Support, using pieces from other GDB modules.
+
+   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/>.  */
+
+#ifndef CALL_SITE_H
+#define CALL_SITE_H
+
+#include "dwarf2/types.h"
+#include "gdbsupport/function-view.h"
+#include "frame-info.h"
+
+struct dwarf2_locexpr_baton;
+struct dwarf2_per_cu_data;
+struct dwarf2_per_objfile;
+
+/* struct call_site_parameter can be referenced in callees by several ways.  */
+
+enum call_site_parameter_kind
+{
+  /* * Use field call_site_parameter.u.dwarf_reg.  */
+  CALL_SITE_PARAMETER_DWARF_REG,
+
+  /* * Use field call_site_parameter.u.fb_offset.  */
+  CALL_SITE_PARAMETER_FB_OFFSET,
+
+  /* * Use field call_site_parameter.u.param_offset.  */
+  CALL_SITE_PARAMETER_PARAM_OFFSET
+};
+
+struct call_site_target
+{
+  /* The kind of location held by this call site target.  */
+  enum kind
+  {
+    /* An address.  */
+    PHYSADDR,
+    /* A name.  */
+    PHYSNAME,
+    /* A DWARF block.  */
+    DWARF_BLOCK,
+    /* An array of addresses.  */
+    ADDRESSES,
+  };
+
+  void set_loc_physaddr (CORE_ADDR physaddr)
+  {
+    m_loc_kind = PHYSADDR;
+    m_loc.physaddr = physaddr;
+  }
+
+  void set_loc_physname (const char *physname)
+    {
+      m_loc_kind = PHYSNAME;
+      m_loc.physname = physname;
+    }
+
+  void set_loc_dwarf_block (dwarf2_locexpr_baton *dwarf_block)
+    {
+      m_loc_kind = DWARF_BLOCK;
+      m_loc.dwarf_block = dwarf_block;
+    }
+
+  void set_loc_array (unsigned length, const CORE_ADDR *data)
+  {
+    m_loc_kind = ADDRESSES;
+    m_loc.addresses.length = length;
+    m_loc.addresses.values = data;
+  }
+
+  /* Callback type for iterate_over_addresses.  */
+
+  using iterate_ftype = gdb::function_view<void (CORE_ADDR)>;
+
+  /* Call CALLBACK for each DW_TAG_call_site's DW_AT_call_target
+     address.  CALLER_FRAME (for registers) can be NULL if it is not
+     known.  This function always may throw NO_ENTRY_VALUE_ERROR.  */
+
+  void iterate_over_addresses (struct gdbarch *call_site_gdbarch,
+			       const struct call_site *call_site,
+			       frame_info_ptr caller_frame,
+			       iterate_ftype callback) const;
+
+private:
+
+  union
+  {
+    /* Address.  */
+    CORE_ADDR physaddr;
+    /* Mangled name.  */
+    const char *physname;
+    /* DWARF block.  */
+    struct dwarf2_locexpr_baton *dwarf_block;
+    /* Array of addresses.  */
+    struct
+    {
+      unsigned length;
+      const CORE_ADDR *values;
+    } addresses;
+  } m_loc;
+
+  /* * Discriminant for union field_location.  */
+  enum kind m_loc_kind;
+};
+
+union call_site_parameter_u
+{
+  /* * DW_TAG_formal_parameter's DW_AT_location's DW_OP_regX
+     as DWARF register number, for register passed
+     parameters.  */
+
+  int dwarf_reg;
+
+  /* * Offset from the callee's frame base, for stack passed
+     parameters.  This equals offset from the caller's stack
+     pointer.  */
+
+  CORE_ADDR fb_offset;
+
+  /* * Offset relative to the start of this PER_CU to
+     DW_TAG_formal_parameter which is referenced by both
+     caller and the callee.  */
+
+  cu_offset param_cu_off;
+};
+
+struct call_site_parameter
+{
+  ENUM_BITFIELD (call_site_parameter_kind) kind : 2;
+
+  union call_site_parameter_u u;
+
+  /* * DW_TAG_formal_parameter's DW_AT_call_value.  It is never NULL.  */
+
+  const gdb_byte *value;
+  size_t value_size;
+
+  /* * DW_TAG_formal_parameter's DW_AT_call_data_value.
+     It may be NULL if not provided by DWARF.  */
+
+  const gdb_byte *data_value;
+  size_t data_value_size;
+};
+
+/* * A place where a function gets called from, represented by
+   DW_TAG_call_site.  It can be looked up from symtab->call_site_htab.  */
+
+struct call_site
+{
+  call_site (CORE_ADDR pc, dwarf2_per_cu_data *per_cu,
+	     dwarf2_per_objfile *per_objfile)
+    : per_cu (per_cu), per_objfile (per_objfile), m_unrelocated_pc (pc)
+  {}
+
+  static int
+  eq (const call_site *a, const call_site *b)
+  {
+    return a->m_unrelocated_pc == b->m_unrelocated_pc;
+  }
+
+  static hashval_t
+  hash (const call_site *a)
+  {
+    return a->m_unrelocated_pc;
+  }
+
+  static int
+  eq (const void *a, const void *b)
+  {
+    return eq ((const call_site *)a, (const call_site *)b);
+  }
+
+  static hashval_t
+  hash (const void *a)
+  {
+    return hash ((const call_site *)a);
+  }
+
+  /* Return the address of the first instruction after this call.  */
+
+  CORE_ADDR pc () const;
+
+  /* Call CALLBACK for each target address.  CALLER_FRAME (for
+     registers) can be NULL if it is not known.  This function may
+     throw NO_ENTRY_VALUE_ERROR.  */
+
+  void iterate_over_addresses (struct gdbarch *call_site_gdbarch,
+			       frame_info_ptr caller_frame,
+			       call_site_target::iterate_ftype callback)
+    const
+  {
+    return target.iterate_over_addresses (call_site_gdbarch, this,
+					  caller_frame, callback);
+  }
+
+  /* * List successor with head in FUNC_TYPE.TAIL_CALL_LIST.  */
+
+  struct call_site *tail_call_next = nullptr;
+
+  /* * Describe DW_AT_call_target.  Missing attribute uses
+     FIELD_LOC_KIND_DWARF_BLOCK with FIELD_DWARF_BLOCK == NULL.  */
+
+  struct call_site_target target {};
+
+  /* * Size of the PARAMETER array.  */
+
+  unsigned parameter_count = 0;
+
+  /* * CU of the function where the call is located.  It gets used
+     for DWARF blocks execution in the parameter array below.  */
+
+  dwarf2_per_cu_data *const per_cu = nullptr;
+
+  /* objfile of the function where the call is located.  */
+
+  dwarf2_per_objfile *const per_objfile = nullptr;
+
+private:
+  /* Unrelocated address of the first instruction after this call.  */
+  const CORE_ADDR m_unrelocated_pc;
+
+public:
+  /* * Describe DW_TAG_call_site's DW_TAG_formal_parameter.  */
+
+  struct call_site_parameter parameter[];
+};
+
+#endif /* CALL_SITE_H */
diff --git a/gdb/dwarf2/comp-unit-head.h b/gdb/dwarf2/comp-unit-head.h
index a7ee3e64546d..ba63c26d3a76 100644
--- a/gdb/dwarf2/comp-unit-head.h
+++ b/gdb/dwarf2/comp-unit-head.h
@@ -27,8 +27,11 @@
 #ifndef GDB_DWARF2_COMP_UNIT_H
 #define GDB_DWARF2_COMP_UNIT_H
 
+#include "dwarf2.h"
 #include "dwarf2/leb.h"
-#include "gdbtypes.h"
+#include "dwarf2/types.h"
+
+struct dwarf2_per_objfile;
 
 /* The data in a compilation unit header, after target2host
    translation, looks like this.  */
diff --git a/gdb/dwarf2/cooked-index.h b/gdb/dwarf2/cooked-index.h
index 2ea32781be5c..8f9d250f467b 100644
--- a/gdb/dwarf2/cooked-index.h
+++ b/gdb/dwarf2/cooked-index.h
@@ -21,7 +21,7 @@
 #define GDB_DWARF2_COOKED_INDEX_H
 
 #include "dwarf2.h"
-#include "gdbtypes.h"
+#include "dwarf2/types.h"
 #include "symtab.h"
 #include "hashtab.h"
 #include "dwarf2/index-common.h"
diff --git a/gdb/dwarf2/expr.h b/gdb/dwarf2/expr.h
index 6078dce0abc5..aa4b392bc99b 100644
--- a/gdb/dwarf2/expr.h
+++ b/gdb/dwarf2/expr.h
@@ -23,7 +23,7 @@
 #define DWARF2EXPR_H
 
 #include "leb128.h"
-#include "gdbtypes.h"
+#include "dwarf2/call-site.h"
 
 struct dwarf2_per_objfile;
 
diff --git a/gdb/dwarf2/line-header.h b/gdb/dwarf2/line-header.h
index e7a63a37918a..ac73bf2e4ede 100644
--- a/gdb/dwarf2/line-header.h
+++ b/gdb/dwarf2/line-header.h
@@ -20,8 +20,6 @@
 #ifndef DWARF2_LINE_HEADER_H
 #define DWARF2_LINE_HEADER_H
 
-#include "gdbtypes.h"
-
 /* dir_index is 1-based in DWARF 4 and before, and is 0-based in DWARF 5 and
    later.  */
 typedef int dir_index;
diff --git a/gdb/dwarf2/types.h b/gdb/dwarf2/types.h
index 463dc485925f..db026d3b6f3f 100644
--- a/gdb/dwarf2/types.h
+++ b/gdb/dwarf2/types.h
@@ -21,6 +21,7 @@
 #define DWARF2_TYPES_H
 
 #include "gdbsupport/offset-type.h"
+#include "gdbsupport/underlying.h"
 
 /* Offset relative to the start of its containing CU (compilation
    unit).  */
diff --git a/gdb/gdbtypes.h b/gdb/gdbtypes.h
index fbe46323216f..d5b966dc73f1 100644
--- a/gdb/gdbtypes.h
+++ b/gdb/gdbtypes.h
@@ -57,7 +57,6 @@
 #include "gdbsupport/gdb_obstack.h"
 #include "gmp-utils.h"
 #include "frame-info.h"
-#include "dwarf2/types.h"
 
 /* Forward declarations for prototypes.  */
 struct field;
@@ -1735,216 +1734,6 @@ struct func_type
     struct type *self_type;
   };
 
-/* struct call_site_parameter can be referenced in callees by several ways.  */
-
-enum call_site_parameter_kind
-{
-  /* * Use field call_site_parameter.u.dwarf_reg.  */
-  CALL_SITE_PARAMETER_DWARF_REG,
-
-  /* * Use field call_site_parameter.u.fb_offset.  */
-  CALL_SITE_PARAMETER_FB_OFFSET,
-
-  /* * Use field call_site_parameter.u.param_offset.  */
-  CALL_SITE_PARAMETER_PARAM_OFFSET
-};
-
-struct call_site_target
-{
-  /* The kind of location held by this call site target.  */
-  enum kind
-  {
-    /* An address.  */
-    PHYSADDR,
-    /* A name.  */
-    PHYSNAME,
-    /* A DWARF block.  */
-    DWARF_BLOCK,
-    /* An array of addresses.  */
-    ADDRESSES,
-  };
-
-  void set_loc_physaddr (CORE_ADDR physaddr)
-  {
-    m_loc_kind = PHYSADDR;
-    m_loc.physaddr = physaddr;
-  }
-
-  void set_loc_physname (const char *physname)
-    {
-      m_loc_kind = PHYSNAME;
-      m_loc.physname = physname;
-    }
-
-  void set_loc_dwarf_block (dwarf2_locexpr_baton *dwarf_block)
-    {
-      m_loc_kind = DWARF_BLOCK;
-      m_loc.dwarf_block = dwarf_block;
-    }
-
-  void set_loc_array (unsigned length, const CORE_ADDR *data)
-  {
-    m_loc_kind = ADDRESSES;
-    m_loc.addresses.length = length;
-    m_loc.addresses.values = data;
-  }
-
-  /* Callback type for iterate_over_addresses.  */
-
-  using iterate_ftype = gdb::function_view<void (CORE_ADDR)>;
-
-  /* Call CALLBACK for each DW_TAG_call_site's DW_AT_call_target
-     address.  CALLER_FRAME (for registers) can be NULL if it is not
-     known.  This function always may throw NO_ENTRY_VALUE_ERROR.  */
-
-  void iterate_over_addresses (struct gdbarch *call_site_gdbarch,
-			       const struct call_site *call_site,
-			       frame_info_ptr caller_frame,
-			       iterate_ftype callback) const;
-
-private:
-
-  union
-  {
-    /* Address.  */
-    CORE_ADDR physaddr;
-    /* Mangled name.  */
-    const char *physname;
-    /* DWARF block.  */
-    struct dwarf2_locexpr_baton *dwarf_block;
-    /* Array of addresses.  */
-    struct
-    {
-      unsigned length;
-      const CORE_ADDR *values;
-    } addresses;
-  } m_loc;
-
-  /* * Discriminant for union field_location.  */
-  enum kind m_loc_kind;
-};
-
-union call_site_parameter_u
-{
-  /* * DW_TAG_formal_parameter's DW_AT_location's DW_OP_regX
-     as DWARF register number, for register passed
-     parameters.  */
-
-  int dwarf_reg;
-
-  /* * Offset from the callee's frame base, for stack passed
-     parameters.  This equals offset from the caller's stack
-     pointer.  */
-
-  CORE_ADDR fb_offset;
-
-  /* * Offset relative to the start of this PER_CU to
-     DW_TAG_formal_parameter which is referenced by both
-     caller and the callee.  */
-
-  cu_offset param_cu_off;
-};
-
-struct call_site_parameter
-{
-  ENUM_BITFIELD (call_site_parameter_kind) kind : 2;
-
-  union call_site_parameter_u u;
-
-  /* * DW_TAG_formal_parameter's DW_AT_call_value.  It is never NULL.  */
-
-  const gdb_byte *value;
-  size_t value_size;
-
-  /* * DW_TAG_formal_parameter's DW_AT_call_data_value.
-     It may be NULL if not provided by DWARF.  */
-
-  const gdb_byte *data_value;
-  size_t data_value_size;
-};
-
-/* * A place where a function gets called from, represented by
-   DW_TAG_call_site.  It can be looked up from symtab->call_site_htab.  */
-
-struct call_site
-  {
-    call_site (CORE_ADDR pc, dwarf2_per_cu_data *per_cu,
-	       dwarf2_per_objfile *per_objfile)
-      : per_cu (per_cu), per_objfile (per_objfile), m_unrelocated_pc (pc)
-    {}
-
-    static int
-    eq (const call_site *a, const call_site *b)
-    {
-      return a->m_unrelocated_pc == b->m_unrelocated_pc;
-    }
-
-    static hashval_t
-    hash (const call_site *a)
-    {
-      return a->m_unrelocated_pc;
-    }
-
-    static int
-    eq (const void *a, const void *b)
-    {
-      return eq ((const call_site *)a, (const call_site *)b);
-    }
-
-    static hashval_t
-    hash (const void *a)
-    {
-      return hash ((const call_site *)a);
-    }
-
-    /* Return the address of the first instruction after this call.  */
-
-    CORE_ADDR pc () const;
-
-    /* Call CALLBACK for each target address.  CALLER_FRAME (for
-       registers) can be NULL if it is not known.  This function may
-       throw NO_ENTRY_VALUE_ERROR.  */
-
-    void iterate_over_addresses (struct gdbarch *call_site_gdbarch,
-				 frame_info_ptr caller_frame,
-				 call_site_target::iterate_ftype callback)
-      const
-    {
-      return target.iterate_over_addresses (call_site_gdbarch, this,
-					    caller_frame, callback);
-    }
-
-    /* * List successor with head in FUNC_TYPE.TAIL_CALL_LIST.  */
-
-    struct call_site *tail_call_next = nullptr;
-
-    /* * Describe DW_AT_call_target.  Missing attribute uses
-       FIELD_LOC_KIND_DWARF_BLOCK with FIELD_DWARF_BLOCK == NULL.  */
-
-    struct call_site_target target {};
-
-    /* * Size of the PARAMETER array.  */
-
-    unsigned parameter_count = 0;
-
-    /* * CU of the function where the call is located.  It gets used
-       for DWARF blocks execution in the parameter array below.  */
-
-    dwarf2_per_cu_data *const per_cu = nullptr;
-
-    /* objfile of the function where the call is located.  */
-
-    dwarf2_per_objfile *const per_objfile = nullptr;
-
-  private:
-    /* Unrelocated address of the first instruction after this call.  */
-    const CORE_ADDR m_unrelocated_pc;
-
-  public:
-    /* * Describe DW_TAG_call_site's DW_TAG_formal_parameter.  */
-
-    struct call_site_parameter parameter[];
-  };
 
 /* The type-specific info for TYPE_CODE_FIXED_POINT types.  */
 
diff --git a/gdb/symtab.c b/gdb/symtab.c
index 0d342f765f2f..57102e2c085a 100644
--- a/gdb/symtab.c
+++ b/gdb/symtab.c
@@ -18,6 +18,7 @@
    along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
 
 #include "defs.h"
+#include "dwarf2/call-site.h"
 #include "symtab.h"
 #include "gdbtypes.h"
 #include "gdbcore.h"
-- 
2.38.1


^ permalink raw reply	[flat|nested] 28+ messages in thread

* [PATCH v2 06/13] gdb: move frame_info_ptr to frame.{c,h}
  2022-12-14  3:34 [PATCH v2 00/13] Make frame_info_ptr automatic Simon Marchi
                   ` (4 preceding siblings ...)
  2022-12-14  3:34 ` [PATCH v2 05/13] gdb: move call site types to call-site.h Simon Marchi
@ 2022-12-14  3:34 ` Simon Marchi
  2022-12-20 17:01   ` Bruno Larsen
  2022-12-14  3:34 ` [PATCH v2 07/13] gdb: add frame_id::user_created_p Simon Marchi
                   ` (9 subsequent siblings)
  15 siblings, 1 reply; 28+ messages in thread
From: Simon Marchi @ 2022-12-14  3:34 UTC (permalink / raw)
  To: gdb-patches; +Cc: Simon Marchi

Change-Id: Ic5949759e6262ea0da6123858702d48fe5673fea
---
 gdb/Makefile.in        |   1 -
 gdb/c-lang.h           |   1 +
 gdb/dwarf2/call-site.h |   2 +-
 gdb/frame-info.c       |  65 -------------
 gdb/frame-info.h       | 211 -----------------------------------------
 gdb/frame.c            |  43 ++++++++-
 gdb/frame.h            | 188 +++++++++++++++++++++++++++++++++++-
 gdb/gdbtypes.h         |   2 -
 gdb/symtab.h           |   1 +
 9 files changed, 230 insertions(+), 284 deletions(-)
 delete mode 100644 gdb/frame-info.c
 delete mode 100644 gdb/frame-info.h

diff --git a/gdb/Makefile.in b/gdb/Makefile.in
index fb4d42c7baad..0f5df2ccb7b0 100644
--- a/gdb/Makefile.in
+++ b/gdb/Makefile.in
@@ -1088,7 +1088,6 @@ COMMON_SFILES = \
 	findvar.c \
 	frame.c \
 	frame-base.c \
-	frame-info.c \
 	frame-unwind.c \
 	gcore.c \
 	gdb-demangle.c \
diff --git a/gdb/c-lang.h b/gdb/c-lang.h
index c2db16e0bf07..1dbcfeb8f102 100644
--- a/gdb/c-lang.h
+++ b/gdb/c-lang.h
@@ -25,6 +25,7 @@ struct ui_file;
 struct language_arch_info;
 struct type_print_options;
 struct parser_state;
+struct compile_instance;
 
 #include "compile/compile.h"
 #include "value.h"
diff --git a/gdb/dwarf2/call-site.h b/gdb/dwarf2/call-site.h
index f5bee967594e..c8a1e8b2971d 100644
--- a/gdb/dwarf2/call-site.h
+++ b/gdb/dwarf2/call-site.h
@@ -23,8 +23,8 @@
 #define CALL_SITE_H
 
 #include "dwarf2/types.h"
+#include "../frame.h"
 #include "gdbsupport/function-view.h"
-#include "frame-info.h"
 
 struct dwarf2_locexpr_baton;
 struct dwarf2_per_cu_data;
diff --git a/gdb/frame-info.c b/gdb/frame-info.c
deleted file mode 100644
index 40a872ea152d..000000000000
--- a/gdb/frame-info.c
+++ /dev/null
@@ -1,65 +0,0 @@
-/* Frame info pointer
-
-   Copyright (C) 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 "defs.h"
-
-#include "frame-info.h"
-#include "frame.h"
-
-/* See frame-info-ptr.h.  */
-
-intrusive_list<frame_info_ptr> frame_info_ptr::frame_list;
-
-/* See frame-info-ptr.h.  */
-
-void
-frame_info_ptr::prepare_reinflate ()
-{
-  m_cached_level = frame_relative_level (*this);
-
-  if (m_cached_level != 0)
-    m_cached_id = get_frame_id (*this);
-}
-
-/* See frame-info-ptr.h.  */
-
-void
-frame_info_ptr::reinflate ()
-{
-  /* Ensure we have a valid frame level (sentinel frame or above), indicating
-     prepare_reinflate was called.  */
-  gdb_assert (m_cached_level >= -1);
-
-  if (m_ptr != nullptr)
-    {
-      /* The frame_info wasn't invalidated, no need to reinflate.  */
-      return;
-    }
-
-  /* Frame #0 needs special handling, see comment in select_frame.  */
-  if (m_cached_level == 0)
-    m_ptr = get_current_frame ().get ();
-  else
-    {
-      gdb_assert (frame_id_p (m_cached_id));
-      m_ptr = frame_find_by_id (m_cached_id).get ();
-    }
-
-  gdb_assert (m_ptr != nullptr);
-}
diff --git a/gdb/frame-info.h b/gdb/frame-info.h
deleted file mode 100644
index 893b6632363d..000000000000
--- a/gdb/frame-info.h
+++ /dev/null
@@ -1,211 +0,0 @@
-/* Frame info pointer
-
-   Copyright (C) 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/>.  */
-
-#ifndef GDB_FRAME_INFO_H
-#define GDB_FRAME_INFO_H
-
-#include "gdbsupport/intrusive_list.h"
-#include "frame-id.h"
-
-struct frame_info;
-
-/* A wrapper for "frame_info *".  frame_info objects are invalidated
-   whenever reinit_frame_cache is called.  This class arranges to
-   invalidate the pointer when appropriate.  This is done to help
-   detect a GDB bug that was relatively common.
-
-   A small amount of code must still operate on raw pointers, so a
-   "get" method is provided.  However, you should normally not use
-   this in new code.  */
-
-class frame_info_ptr : public intrusive_list_node<frame_info_ptr>
-{
-public:
-  /* Create a frame_info_ptr from a raw pointer.  */
-  explicit frame_info_ptr (struct frame_info *ptr)
-    : m_ptr (ptr)
-  {
-    frame_list.push_back (*this);
-  }
-
-  /* Create a null frame_info_ptr.  */
-  frame_info_ptr ()
-  {
-    frame_list.push_back (*this);
-  }
-
-  frame_info_ptr (std::nullptr_t)
-  {
-    frame_list.push_back (*this);
-  }
-
-  frame_info_ptr (const frame_info_ptr &other)
-    : m_ptr (other.m_ptr),
-      m_cached_id (other.m_cached_id),
-      m_cached_level (other.m_cached_level)
-  {
-    frame_list.push_back (*this);
-  }
-
-  frame_info_ptr (frame_info_ptr &&other)
-    : m_ptr (other.m_ptr),
-      m_cached_id (other.m_cached_id),
-      m_cached_level (other.m_cached_level)
-  {
-    other.m_ptr = nullptr;
-    other.m_cached_id = null_frame_id;
-    other.m_cached_level = invalid_level;
-    frame_list.push_back (*this);
-  }
-
-  ~frame_info_ptr ()
-  {
-    /* If this node has static storage, it may be deleted after
-       frame_list.  Attempting to erase ourselves would then trigger
-       internal errors, so make sure we are still linked first.  */
-    if (is_linked ())
-      frame_list.erase (frame_list.iterator_to (*this));
-  }
-
-  frame_info_ptr &operator= (const frame_info_ptr &other)
-  {
-    m_ptr = other.m_ptr;
-    m_cached_id = other.m_cached_id;
-    m_cached_level = other.m_cached_level;
-    return *this;
-  }
-
-  frame_info_ptr &operator= (std::nullptr_t)
-  {
-    m_ptr = nullptr;
-    m_cached_id = null_frame_id;
-    m_cached_level = invalid_level;
-    return *this;
-  }
-
-  frame_info_ptr &operator= (frame_info_ptr &&other)
-  {
-    m_ptr = other.m_ptr;
-    m_cached_id = other.m_cached_id;
-    m_cached_level = other.m_cached_level;
-    other.m_ptr = nullptr;
-    other.m_cached_id = null_frame_id;
-    other.m_cached_level = invalid_level;
-    return *this;
-  }
-
-  frame_info *operator-> () const
-  {
-    return m_ptr;
-  }
-
-  /* Fetch the underlying pointer.  Note that new code should
-     generally not use this -- avoid it if at all possible.  */
-  frame_info *get () const
-  {
-    return m_ptr;
-  }
-
-  /* This exists for compatibility with pre-existing code that checked
-     a "frame_info *" using "!".  */
-  bool operator! () const
-  {
-    return m_ptr == nullptr;
-  }
-
-  /* This exists for compatibility with pre-existing code that checked
-     a "frame_info *" like "if (ptr)".  */
-  explicit operator bool () const
-  {
-    return m_ptr != nullptr;
-  }
-
-  /* Invalidate this pointer.  */
-  void invalidate ()
-  {
-    m_ptr = nullptr;
-  }
-
-  /* Cache the frame_id that the pointer will use to reinflate.  */
-  void prepare_reinflate ();
-
-  /* Use the cached frame_id to reinflate the pointer.  */
-  void reinflate ();
-
-private:
-  /* We sometimes need to construct frame_info_ptr objects around the
-     sentinel_frame, which has level -1.  Therefore, make the invalid frame
-     level value -2.  */
-  static constexpr int invalid_level = -2;
-
-  /* The underlying pointer.  */
-  frame_info *m_ptr = nullptr;
-
-  /* The frame_id of the underlying pointer.  */
-  frame_id m_cached_id = null_frame_id;
-
-  /* The frame level of the underlying pointer.  */
-  int m_cached_level = invalid_level;
-
-  /* All frame_info_ptr objects are kept on an intrusive list.
-     This keeps their construction and destruction costs
-     reasonably small.  */
-  static intrusive_list<frame_info_ptr> frame_list;
-
-  /* A friend so it can invalidate the pointers.  */
-  friend void reinit_frame_cache ();
-};
-
-static inline bool
-operator== (const frame_info *self, const frame_info_ptr &other)
-{
-  return self == other.get ();
-}
-
-static inline bool
-operator== (const frame_info_ptr &self, const frame_info_ptr &other)
-{
-  return self.get () == other.get ();
-}
-
-static inline bool
-operator== (const frame_info_ptr &self, const frame_info *other)
-{
-  return self.get () == other;
-}
-
-static inline bool
-operator!= (const frame_info *self, const frame_info_ptr &other)
-{
-  return self != other.get ();
-}
-
-static inline bool
-operator!= (const frame_info_ptr &self, const frame_info_ptr &other)
-{
-  return self.get () != other.get ();
-}
-
-static inline bool
-operator!= (const frame_info_ptr &self, const frame_info *other)
-{
-  return self.get () != other;
-}
-
-#endif /* GDB_FRAME_INFO_H */
diff --git a/gdb/frame.c b/gdb/frame.c
index 6a6d968b9a97..ba06f9b0b3e4 100644
--- a/gdb/frame.c
+++ b/gdb/frame.c
@@ -19,7 +19,6 @@
 
 #include "defs.h"
 #include "frame.h"
-#include "frame-info.h"
 #include "target.h"
 #include "value.h"
 #include "inferior.h"	/* for inferior_ptid */
@@ -3161,6 +3160,48 @@ maintenance_print_frame_id (const char *args, int from_tty)
 	      get_frame_id (frame).to_string ().c_str ());
 }
 
+/* See frame-info-ptr.h.  */
+
+intrusive_list<frame_info_ptr> frame_info_ptr::frame_list;
+
+/* See frame-info-ptr.h.  */
+
+void
+frame_info_ptr::prepare_reinflate ()
+{
+  m_cached_level = frame_relative_level (*this);
+
+  if (m_cached_level != 0)
+    m_cached_id = get_frame_id (*this);
+}
+
+/* See frame-info-ptr.h.  */
+
+void
+frame_info_ptr::reinflate ()
+{
+  /* Ensure we have a valid frame level (sentinel frame or above), indicating
+     prepare_reinflate was called.  */
+  gdb_assert (m_cached_level >= -1);
+
+  if (m_ptr != nullptr)
+    {
+      /* The frame_info wasn't invalidated, no need to reinflate.  */
+      return;
+    }
+
+  /* Frame #0 needs special handling, see comment in select_frame.  */
+  if (m_cached_level == 0)
+    m_ptr = get_current_frame ().get ();
+  else
+    {
+      gdb_assert (frame_id_p (m_cached_id));
+      m_ptr = frame_find_by_id (m_cached_id).get ();
+    }
+
+  gdb_assert (m_ptr != nullptr);
+}
+
 void _initialize_frame ();
 void
 _initialize_frame ()
diff --git a/gdb/frame.h b/gdb/frame.h
index 5c292cd2b952..ebbd9e08868e 100644
--- a/gdb/frame.h
+++ b/gdb/frame.h
@@ -20,8 +20,6 @@
 #if !defined (FRAME_H)
 #define FRAME_H 1
 
-#include "frame-info.h"
-
 /* The following is the intended naming schema for frame functions.
    It isn't 100% consistent, but it is approaching that.  Frame naming
    schema:
@@ -72,7 +70,9 @@
    */
 
 #include "cli/cli-option.h"
+#include "frame-id.h"
 #include "gdbsupport/common-debug.h"
+#include "gdbsupport/intrusive_list.h"
 
 struct symtab_and_line;
 struct frame_unwind;
@@ -85,7 +85,6 @@ struct frame_print_options;
 
 /* The frame object.  */
 
-class frame_info_ptr;
 
 /* Save and restore the currently selected frame.  */
 
@@ -194,6 +193,189 @@ enum frame_type
   SENTINEL_FRAME
 };
 
+/* A wrapper for "frame_info *".  frame_info objects are invalidated
+   whenever reinit_frame_cache is called.  This class arranges to
+   invalidate the pointer when appropriate.  This is done to help
+   detect a GDB bug that was relatively common.
+
+   A small amount of code must still operate on raw pointers, so a
+   "get" method is provided.  However, you should normally not use
+   this in new code.  */
+
+class frame_info_ptr : public intrusive_list_node<frame_info_ptr>
+{
+public:
+  /* Create a frame_info_ptr from a raw pointer.  */
+  explicit frame_info_ptr (struct frame_info *ptr)
+    : m_ptr (ptr)
+  {
+    frame_list.push_back (*this);
+  }
+
+  /* Create a null frame_info_ptr.  */
+  frame_info_ptr ()
+  {
+    frame_list.push_back (*this);
+  }
+
+  frame_info_ptr (std::nullptr_t)
+  {
+    frame_list.push_back (*this);
+  }
+
+  frame_info_ptr (const frame_info_ptr &other)
+    : m_ptr (other.m_ptr),
+      m_cached_id (other.m_cached_id),
+      m_cached_level (other.m_cached_level)
+  {
+    frame_list.push_back (*this);
+  }
+
+  frame_info_ptr (frame_info_ptr &&other)
+    : m_ptr (other.m_ptr),
+      m_cached_id (other.m_cached_id),
+      m_cached_level (other.m_cached_level)
+  {
+    other.m_ptr = nullptr;
+    other.m_cached_id = null_frame_id;
+    other.m_cached_level = invalid_level;
+    frame_list.push_back (*this);
+  }
+
+  ~frame_info_ptr ()
+  {
+    /* If this node has static storage, it may be deleted after
+       frame_list.  Attempting to erase ourselves would then trigger
+       internal errors, so make sure we are still linked first.  */
+    if (is_linked ())
+      frame_list.erase (frame_list.iterator_to (*this));
+  }
+
+  frame_info_ptr &operator= (const frame_info_ptr &other)
+  {
+    m_ptr = other.m_ptr;
+    m_cached_id = other.m_cached_id;
+    m_cached_level = other.m_cached_level;
+    return *this;
+  }
+
+  frame_info_ptr &operator= (std::nullptr_t)
+  {
+    m_ptr = nullptr;
+    m_cached_id = null_frame_id;
+    m_cached_level = invalid_level;
+    return *this;
+  }
+
+  frame_info_ptr &operator= (frame_info_ptr &&other)
+  {
+    m_ptr = other.m_ptr;
+    m_cached_id = other.m_cached_id;
+    m_cached_level = other.m_cached_level;
+    other.m_ptr = nullptr;
+    other.m_cached_id = null_frame_id;
+    other.m_cached_level = invalid_level;
+    return *this;
+  }
+
+  frame_info *operator-> () const
+  {
+    return m_ptr;
+  }
+
+  /* Fetch the underlying pointer.  Note that new code should
+     generally not use this -- avoid it if at all possible.  */
+  frame_info *get () const
+  {
+    return m_ptr;
+  }
+
+  /* This exists for compatibility with pre-existing code that checked
+     a "frame_info *" using "!".  */
+  bool operator! () const
+  {
+    return m_ptr == nullptr;
+  }
+
+  /* This exists for compatibility with pre-existing code that checked
+     a "frame_info *" like "if (ptr)".  */
+  explicit operator bool () const
+  {
+    return m_ptr != nullptr;
+  }
+
+  /* Invalidate this pointer.  */
+  void invalidate ()
+  {
+    m_ptr = nullptr;
+  }
+
+  /* Cache the frame_id that the pointer will use to reinflate.  */
+  void prepare_reinflate ();
+
+  /* Use the cached frame_id to reinflate the pointer.  */
+  void reinflate ();
+
+private:
+  /* We sometimes need to construct frame_info_ptr objects around the
+     sentinel_frame, which has level -1.  Therefore, make the invalid frame
+     level value -2.  */
+  static constexpr int invalid_level = -2;
+
+  /* The underlying pointer.  */
+  frame_info *m_ptr = nullptr;
+
+  /* The frame_id of the underlying pointer.  */
+  frame_id m_cached_id = null_frame_id;
+
+  /* The frame level of the underlying pointer.  */
+  int m_cached_level = invalid_level;
+
+  /* All frame_info_ptr objects are kept on an intrusive list.
+     This keeps their construction and destruction costs
+     reasonably small.  */
+  static intrusive_list<frame_info_ptr> frame_list;
+
+  /* A friend so it can invalidate the pointers.  */
+  friend void reinit_frame_cache ();
+};
+
+static inline bool
+operator== (const frame_info *self, const frame_info_ptr &other)
+{
+  return self == other.get ();
+}
+
+static inline bool
+operator== (const frame_info_ptr &self, const frame_info_ptr &other)
+{
+  return self.get () == other.get ();
+}
+
+static inline bool
+operator== (const frame_info_ptr &self, const frame_info *other)
+{
+  return self.get () == other;
+}
+
+static inline bool
+operator!= (const frame_info *self, const frame_info_ptr &other)
+{
+  return self != other.get ();
+}
+
+static inline bool
+operator!= (const frame_info_ptr &self, const frame_info_ptr &other)
+{
+  return self.get () != other.get ();
+}
+
+static inline bool
+operator!= (const frame_info_ptr &self, const frame_info *other)
+{
+  return self.get () != other;
+}
+
 /* For every stopped thread, GDB tracks two frames: current and
    selected.  Current frame is the inner most frame of the selected
    thread.  Selected frame is the one being examined by the GDB
diff --git a/gdb/gdbtypes.h b/gdb/gdbtypes.h
index d5b966dc73f1..27499853d060 100644
--- a/gdb/gdbtypes.h
+++ b/gdb/gdbtypes.h
@@ -56,7 +56,6 @@
 #include "dwarf2.h"
 #include "gdbsupport/gdb_obstack.h"
 #include "gmp-utils.h"
-#include "frame-info.h"
 
 /* Forward declarations for prototypes.  */
 struct field;
@@ -1734,7 +1733,6 @@ struct func_type
     struct type *self_type;
   };
 
-
 /* The type-specific info for TYPE_CODE_FIXED_POINT types.  */
 
 struct fixed_point_type_info
diff --git a/gdb/symtab.h b/gdb/symtab.h
index 4f3e84bbbe93..7b8704f83087 100644
--- a/gdb/symtab.h
+++ b/gdb/symtab.h
@@ -37,6 +37,7 @@
 #include "completer.h"
 #include "gdb-demangle.h"
 #include "split-name.h"
+#include "frame.h"
 
 /* Opaque declarations.  */
 struct ui_file;
-- 
2.38.1


^ permalink raw reply	[flat|nested] 28+ messages in thread

* [PATCH v2 07/13] gdb: add frame_id::user_created_p
  2022-12-14  3:34 [PATCH v2 00/13] Make frame_info_ptr automatic Simon Marchi
                   ` (5 preceding siblings ...)
  2022-12-14  3:34 ` [PATCH v2 06/13] gdb: move frame_info_ptr to frame.{c,h} Simon Marchi
@ 2022-12-14  3:34 ` Simon Marchi
  2022-12-14  3:34 ` [PATCH v2 08/13] gdb: add user-created frames to stash Simon Marchi
                   ` (8 subsequent siblings)
  15 siblings, 0 replies; 28+ messages in thread
From: Simon Marchi @ 2022-12-14  3:34 UTC (permalink / raw)
  To: gdb-patches; +Cc: Simon Marchi

Later in this series, we'll need to differentiate frame ids for regular
frames (obtained from the target state and unwinding from it) vs frame
ids for user-created frames (created with create_new_frame).  Add the
frame_id::user_created_p field to indicate a frame is user-created, and
set it in create_new_frame.

The field is otherwise not used yet, so not changes in behavior are
expected.

Change-Id: I60de3ce581ed01bf0fddb30dff9bd932840120c3
---
 gdb/frame-id.h | 4 ++++
 gdb/frame.c    | 1 +
 2 files changed, 5 insertions(+)

diff --git a/gdb/frame-id.h b/gdb/frame-id.h
index 142488f7319d..929be98eed4a 100644
--- a/gdb/frame-id.h
+++ b/gdb/frame-id.h
@@ -99,6 +99,10 @@ struct frame_id
   unsigned int code_addr_p : 1;
   unsigned int special_addr_p : 1;
 
+  /* True if this frame was created from addresses given by the user (see
+     create_new_frame) rather than through unwinding.  */
+  unsigned int user_created_p : 1;
+
   /* It is non-zero for a frame made up by GDB without stack data
      representation in inferior, such as INLINE_FRAME or TAILCALL_FRAME.
      Caller of inlined function will have it zero, each more inner called frame
diff --git a/gdb/frame.c b/gdb/frame.c
index ba06f9b0b3e4..1010f82d4738 100644
--- a/gdb/frame.c
+++ b/gdb/frame.c
@@ -1953,6 +1953,7 @@ create_new_frame (CORE_ADDR addr, CORE_ADDR pc)
 
   fi->this_id.p = frame_id_status::COMPUTED;
   fi->this_id.value = frame_id_build (addr, pc);
+  fi->this_id.value.user_created_p = 1;
 
   frame_debug_printf ("  -> %s", fi->to_string ().c_str ());
 
-- 
2.38.1


^ permalink raw reply	[flat|nested] 28+ messages in thread

* [PATCH v2 08/13] gdb: add user-created frames to stash
  2022-12-14  3:34 [PATCH v2 00/13] Make frame_info_ptr automatic Simon Marchi
                   ` (6 preceding siblings ...)
  2022-12-14  3:34 ` [PATCH v2 07/13] gdb: add frame_id::user_created_p Simon Marchi
@ 2022-12-14  3:34 ` Simon Marchi
  2022-12-14  3:34 ` [PATCH v2 09/13] gdb: add create_new_frame(frame_id) overload Simon Marchi
                   ` (7 subsequent siblings)
  15 siblings, 0 replies; 28+ messages in thread
From: Simon Marchi @ 2022-12-14  3:34 UTC (permalink / raw)
  To: gdb-patches; +Cc: Simon Marchi

A subsequent patch makes it possible for frame_info_ptr to reinflate
user-created frames.  If two frame_info_ptr objects wrapping the same
user-created frame_info need to do reinflation, we want them to end up
pointing to the same frame_info instance, and not create two separate
frame_infos.  Otherwise, GDB gets confused down the line, as the state
kept in one frame_info object starts differing from the other
frame_info.

Achieve this by making create_new_frame place the user-created frames in
the frame stash.  This way, when the second frame_info_ptr does
reinflation, it will find the existing frame_info object, created by the
other frame_info_ptr, in the frame stash.

To make the frame stash differentiate between regular and user-created
frame infos which would otherwise be equal, change frame_addr_hash and
frame_id::operator== to account for frame_id::user_created_p.

I made create_new_frame look up existing frames in the stash, and only
create one if it doesn't find one.  The goal is to avoid the
"select-frame view"/"info frame view"/"frame view" commands from
overriding existing entries into the stash, should the user specify the
same frame more than once.  This will also help in the subsequent patch
that makes frame_info_ptr capable of reinflating user-created frames.
It will be able to just call create_new_frame and it will do the right
thing.

Change-Id: I14ba5799012056c007b4992ecb5c7adafd0c2404
---
 gdb/frame.c | 19 +++++++++++++++++--
 1 file changed, 17 insertions(+), 2 deletions(-)

diff --git a/gdb/frame.c b/gdb/frame.c
index 1010f82d4738..528ad76b58de 100644
--- a/gdb/frame.c
+++ b/gdb/frame.c
@@ -240,6 +240,9 @@ frame_addr_hash (const void *ap)
     hash = iterative_hash (&f_id.special_addr,
 			   sizeof (f_id.special_addr), hash);
 
+  char user_created_p = f_id.user_created_p;
+  hash = iterative_hash (&user_created_p, sizeof (user_created_p), hash);
+
   return hash;
 }
 
@@ -775,6 +778,8 @@ frame_id::operator== (const frame_id &r) const
   else if (artificial_depth != r.artificial_depth)
     /* If artificial depths are different, the frames must be different.  */
     eq = false;
+  else if (user_created_p != r.user_created_p)
+    eq = false;
   else
     /* Frames are equal.  */
     eq = true;
@@ -1931,6 +1936,14 @@ create_new_frame (CORE_ADDR addr, CORE_ADDR pc)
 
   frame_debug_printf ("addr=%s, pc=%s", hex_string (addr), hex_string (pc));
 
+  /* Avoid creating duplicate frames, search for an existing frame with that id
+     in the stash.  */
+  frame_id id = frame_id_build (addr, pc);
+  id.user_created_p = 1;
+  frame_info_ptr frame = frame_stash_find (id);
+  if (frame != nullptr)
+    return frame;
+
   fi = FRAME_OBSTACK_ZALLOC (struct frame_info);
 
   fi->next = create_sentinel_frame (current_program_space,
@@ -1952,8 +1965,10 @@ create_new_frame (CORE_ADDR addr, CORE_ADDR pc)
   frame_unwind_find_by_frame (frame_info_ptr (fi), &fi->prologue_cache);
 
   fi->this_id.p = frame_id_status::COMPUTED;
-  fi->this_id.value = frame_id_build (addr, pc);
-  fi->this_id.value.user_created_p = 1;
+  fi->this_id.value = id;
+
+  bool added = frame_stash_add (fi);
+  gdb_assert (added);
 
   frame_debug_printf ("  -> %s", fi->to_string ().c_str ());
 
-- 
2.38.1


^ permalink raw reply	[flat|nested] 28+ messages in thread

* [PATCH v2 09/13] gdb: add create_new_frame(frame_id) overload
  2022-12-14  3:34 [PATCH v2 00/13] Make frame_info_ptr automatic Simon Marchi
                   ` (7 preceding siblings ...)
  2022-12-14  3:34 ` [PATCH v2 08/13] gdb: add user-created frames to stash Simon Marchi
@ 2022-12-14  3:34 ` Simon Marchi
  2022-12-14  3:34 ` [PATCH v2 10/13] gdb: make it possible to restore selected user-created frames Simon Marchi
                   ` (6 subsequent siblings)
  15 siblings, 0 replies; 28+ messages in thread
From: Simon Marchi @ 2022-12-14  3:34 UTC (permalink / raw)
  To: gdb-patches; +Cc: Simon Marchi

The subsequent patches will need to call create_new_frame with an
existing frame_id representing a user created frame.  They could call
the existing create_new_frame, passing both addresses, but it seems
nicer to have a version of the function that takes a frame_id directly.

Change-Id: If31025314fec0c3e644703e4391a5ef8079e1a32
---
 gdb/frame.c | 26 ++++++++++++++++++--------
 1 file changed, 18 insertions(+), 8 deletions(-)

diff --git a/gdb/frame.c b/gdb/frame.c
index 528ad76b58de..b9132d0f35a7 100644
--- a/gdb/frame.c
+++ b/gdb/frame.c
@@ -1929,22 +1929,23 @@ select_frame (frame_info_ptr fi)
 /* Create an arbitrary (i.e. address specified by user) or innermost frame.
    Always returns a non-NULL value.  */
 
-frame_info_ptr
-create_new_frame (CORE_ADDR addr, CORE_ADDR pc)
+static frame_info_ptr
+create_new_frame (frame_id id)
 {
-  frame_info *fi;
+  gdb_assert (id.user_created_p);
+  gdb_assert (id.stack_status == frame_id_stack_status::FID_STACK_VALID);
+  gdb_assert (id.code_addr_p);
 
-  frame_debug_printf ("addr=%s, pc=%s", hex_string (addr), hex_string (pc));
+  frame_debug_printf ("stack_addr=%s, core_addr=%s",
+		      hex_string (id.stack_addr), hex_string (id.code_addr));
 
   /* Avoid creating duplicate frames, search for an existing frame with that id
      in the stash.  */
-  frame_id id = frame_id_build (addr, pc);
-  id.user_created_p = 1;
   frame_info_ptr frame = frame_stash_find (id);
   if (frame != nullptr)
     return frame;
 
-  fi = FRAME_OBSTACK_ZALLOC (struct frame_info);
+  frame_info *fi = FRAME_OBSTACK_ZALLOC (struct frame_info);
 
   fi->next = create_sentinel_frame (current_program_space,
 				    get_current_regcache ());
@@ -1953,7 +1954,7 @@ create_new_frame (CORE_ADDR addr, CORE_ADDR pc)
      Do this before looking for this frame's unwinder.  A sniffer is
      very likely to read this, and the corresponding unwinder is
      entitled to rely that the PC doesn't magically change.  */
-  fi->next->prev_pc.value = pc;
+  fi->next->prev_pc.value = id.code_addr;
   fi->next->prev_pc.status = CC_VALUE;
 
   /* We currently assume that frame chain's can't cross spaces.  */
@@ -1975,6 +1976,15 @@ create_new_frame (CORE_ADDR addr, CORE_ADDR pc)
   return frame_info_ptr (fi);
 }
 
+frame_info_ptr
+create_new_frame (CORE_ADDR stack, CORE_ADDR pc)
+{
+  frame_id id = frame_id_build (stack, pc);
+  id.user_created_p = 1;
+
+  return create_new_frame (id);
+}
+
 /* Return the frame that THIS_FRAME calls (NULL if THIS_FRAME is the
    innermost frame).  Be careful to not fall off the bottom of the
    frame chain and onto the sentinel frame.  */
-- 
2.38.1


^ permalink raw reply	[flat|nested] 28+ messages in thread

* [PATCH v2 10/13] gdb: make it possible to restore selected user-created frames
  2022-12-14  3:34 [PATCH v2 00/13] Make frame_info_ptr automatic Simon Marchi
                   ` (8 preceding siblings ...)
  2022-12-14  3:34 ` [PATCH v2 09/13] gdb: add create_new_frame(frame_id) overload Simon Marchi
@ 2022-12-14  3:34 ` Simon Marchi
  2022-12-14  3:34 ` [PATCH v2 11/13] gdb: make user-created frames reinflatable Simon Marchi
                   ` (5 subsequent siblings)
  15 siblings, 0 replies; 28+ messages in thread
From: Simon Marchi @ 2022-12-14  3:34 UTC (permalink / raw)
  To: gdb-patches; +Cc: Simon Marchi

From: Simon Marchi <simon.marchi@efficios.com>

I would like to improve frame_info_ptr to automatically grab the
information needed to reinflate a frame, and automatically reinflate it
as needed.  One thing that is in the way is the fact that some frames
can be created out of thin air by the create_new_frame function.  These
frames are not the fruit of unwinding from the target's current frame.
These frames are created by the "select-frame view" command.

These frames are not correctly handled by the frame save/restore
functions, save_selected_frame, restore_selected_frame and
lookup_selected_frame.  This can be observed here, using the test
included in this patch:

    $ ./gdb --data-directory=data-directory -nx -q testsuite/outputs/gdb.base/frame-view/frame-view
    Reading symbols from testsuite/outputs/gdb.base/frame-view/frame-view...
    (gdb) break thread_func
    Breakpoint 1 at 0x11a2: file /home/simark/src/binutils-gdb/gdb/testsuite/gdb.base/frame-view.c, line 42.
    (gdb) run
    Starting program: /home/simark/build/binutils-gdb/gdb/testsuite/outputs/gdb.base/frame-view/frame-view

    [Thread debugging using libthread_db enabled]
    Using host libthread_db library "/usr/lib/../lib/libthread_db.so.1".
    [New Thread 0x7ffff7cc46c0 (LWP 4171134)]
    [Switching to Thread 0x7ffff7cc46c0 (LWP 4171134)]

    Thread 2 "frame-view" hit Breakpoint 1, thread_func (p=0x0) at /home/simark/src/binutils-gdb/gdb/testsuite/gdb.base/frame-view.c:42
    42        foo (11);
    (gdb) info frame
    Stack level 0, frame at 0x7ffff7cc3ee0:
     rip = 0x5555555551a2 in thread_func (/home/simark/src/binutils-gdb/gdb/testsuite/gdb.base/frame-view.c:42); saved rip = 0x7ffff7d4e8fd
     called by frame at 0x7ffff7cc3f80
     source language c.
     Arglist at 0x7ffff7cc3ed0, args: p=0x0
     Locals at 0x7ffff7cc3ed0, Previous frame's sp is 0x7ffff7cc3ee0
     Saved registers:
      rbp at 0x7ffff7cc3ed0, rip at 0x7ffff7cc3ed8
    (gdb) thread 1
    [Switching to thread 1 (Thread 0x7ffff7cc5740 (LWP 4171122))]
    #0  0x00007ffff7d4b4b6 in ?? () from /usr/lib/libc.so.6

Here, we create a custom frame for thread 1 (using the stack from thread
2, for convenience):

    (gdb) select-frame view 0x7ffff7cc3f80 0x5555555551a2

The first calls to "frame" looks good:

    (gdb) frame
    #0  thread_func (p=0x7ffff7d4e630) at /home/simark/src/binutils-gdb/gdb/testsuite/gdb.base/frame-view.c:42
    42        foo (11);

But not the second one:

    (gdb) frame
    #0  0x00007ffff7d4b4b6 in ?? () from /usr/lib/libc.so.6

This second "frame" command shows the current target frame instead of
the user-created frame.

It's not totally clear how the "select-frame view" feature is expected
to behave, especially since it's not tested.  I heard accounts that it
used to be possible to select a frame like this and do "up" and "down"
to navigate the backtrace starting from that frame.  The fact that
create_new_frame calls frame_unwind_find_by_frame to install the right
unwinder suggest that it used to be possible.  But that doesn't work
today:

    (gdb) select-frame view 0x7ffff7cc3f80 0x5555555551a2
    (gdb) up
    Initial frame selected; you cannot go up.
    (gdb) down
    Bottom (innermost) frame selected; you cannot go down.

and "backtrace" always shows the actual thread's backtrace, it ignores
the user-created frame:

    (gdb) bt
    #0  0x00007ffff7d4b4b6 in ?? () from /usr/lib/libc.so.6
    #1  0x00007ffff7d50403 in ?? () from /usr/lib/libc.so.6
    #2  0x000055555555521a in main () at /home/simark/src/binutils-gdb/gdb/testsuite/gdb.base/frame-view.c:56

I don't want to address all the `select-frame view` issues , but I think
we can agree that the "frame" command changing the selected frame, as
shown above, is a bug.  I would expect that command to show the
currently selected frame and not change it.

This happens because of the scoped_restore_selected_frame object in
print_frame_args.  The frame information is saved in the constructor
(the backtrace below), and restored in the destructor.

    #0  save_selected_frame (frame_id=0x7ffdc0020ad0, frame_level=0x7ffdc0020af0) at /home/simark/src/binutils-gdb/gdb/frame.c:1682
    #1  0x00005631390242f0 in scoped_restore_selected_frame::scoped_restore_selected_frame (this=0x7ffdc0020ad0) at /home/simark/src/binutils-gdb/gdb/frame.c:324
    #2  0x000056313993581e in print_frame_args (fp_opts=..., func=0x62100023bde0, frame=..., num=-1, stream=0x60b000000300) at /home/simark/src/binutils-gdb/gdb/stack.c:755
    #3  0x000056313993ad49 in print_frame (fp_opts=..., frame=..., print_level=1, print_what=SRC_AND_LOC, print_args=1, sal=...) at /home/simark/src/binutils-gdb/gdb/stack.c:1401
    #4  0x000056313993835d in print_frame_info (fp_opts=..., frame=..., print_level=1, print_what=SRC_AND_LOC, print_args=1, set_current_sal=1) at /home/simark/src/binutils-gdb/gdb/stack.c:1126
    #5  0x0000563139932e0b in print_stack_frame (frame=..., print_level=1, print_what=SRC_AND_LOC, set_current_sal=1) at /home/simark/src/binutils-gdb/gdb/stack.c:368
    #6  0x0000563139932bbe in print_stack_frame_to_uiout (uiout=0x611000016840, frame=..., print_level=1, print_what=SRC_AND_LOC, set_current_sal=1) at /home/simark/src/binutils-gdb/gdb/stack.c:346
    #7  0x0000563139b0641e in print_selected_thread_frame (uiout=0x611000016840, selection=...) at /home/simark/src/binutils-gdb/gdb/thread.c:1993
    #8  0x0000563139940b7f in frame_command_core (fi=..., ignored=true) at /home/simark/src/binutils-gdb/gdb/stack.c:1871
    #9  0x000056313994db9e in frame_command_helper<frame_command_core>::base_command (arg=0x0, from_tty=1) at /home/simark/src/binutils-gdb/gdb/stack.c:1976

Since the user-created frame has level 0 (identified by the saved level
-1), lookup_selected_frame just reselects the target's current frame,
and the user-created frame is lost.

My goal here is to fix this particular problem.

Currently, select_frame does not set selected_frame_id and
selected_frame_level for frames with level 0.  It leaves them at
null_frame_id / -1, indicating to restore_selected_frame to use the
target's current frame.  User-created frames also have level 0, so add a
special case them such that select_frame saves their selected id and
level.

save_selected_frame does not need any change.

Change the assertion in restore_selected_frame that checks `frame_level
!= 0` to account for the fact that we can restore user-created frames,
which have level 0.

Finally, change lookup_selected_frame to make it able to re-create
user-created frame_info objects from selected_frame_level and
selected_frame_id.

Add a minimal test case for the case described above, that is the
"select-frame view" command followed by the "frame" command twice.  In
order to have a known stack frame to switch to, the test spawns a second
thread, and tells the first thread to use the other thread's top frame.

Change-Id: Ifc77848dc465fbd21324b9d44670833e09fe98c7
---
 gdb/frame.c                           | 30 ++++++++---
 gdb/testsuite/gdb.base/frame-view.c   | 74 +++++++++++++++++++++++++++
 gdb/testsuite/gdb.base/frame-view.exp | 70 +++++++++++++++++++++++++
 3 files changed, 167 insertions(+), 7 deletions(-)
 create mode 100644 gdb/testsuite/gdb.base/frame-view.c
 create mode 100644 gdb/testsuite/gdb.base/frame-view.exp

diff --git a/gdb/frame.c b/gdb/frame.c
index b9132d0f35a7..9fba29ec13eb 100644
--- a/gdb/frame.c
+++ b/gdb/frame.c
@@ -69,6 +69,7 @@ set_backtrace_options user_set_backtrace_options;
 
 static frame_info_ptr get_prev_frame_raw (frame_info_ptr this_frame);
 static const char *frame_stop_reason_symbol_string (enum unwind_stop_reason reason);
+static frame_info_ptr create_new_frame (frame_id id);
 
 /* Status of some values cached in the frame_info object.  */
 
@@ -1669,9 +1670,12 @@ get_current_frame (void)
 
    If SELECTED_FRAME_ID / SELECTED_FRAME_LEVEL are null_frame_id / -1,
    and the target has stack and is stopped, the selected frame is the
-   current (innermost) frame.  This means that SELECTED_FRAME_LEVEL is
-   never 0 and SELECTED_FRAME_ID is never the ID of the innermost
-   frame.
+   current (innermost) target frame.  SELECTED_FRAME_ID is never the ID
+   of the current (innermost) target frame.  SELECTED_FRAME_LEVEL may
+   only be 0 if the selected frame is a user-created one (created and
+   selected through the "select-frame view" command), in which case
+   SELECTED_FRAME_ID is the frame id derived from the user-provided
+   addresses.
 
    If SELECTED_FRAME_ID / SELECTED_FRAME_LEVEL are null_frame_id / -1,
    and the target has no stack or is executing, then there's no
@@ -1699,9 +1703,9 @@ void
 restore_selected_frame (frame_id frame_id, int frame_level)
   noexcept
 {
-  /* save_selected_frame never returns level == 0, so we shouldn't see
-     it here either.  */
-  gdb_assert (frame_level != 0);
+  /* Unless it is a user-created frame, save_selected_frame never returns
+     level == 0, so we shouldn't see it here either.  */
+  gdb_assert (frame_level != 0 || frame_id.user_created_p);
 
   /* FRAME_ID can be null_frame_id only IFF frame_level is -1.  */
   gdb_assert ((frame_level == -1 && !frame_id_p (frame_id))
@@ -1735,6 +1739,15 @@ lookup_selected_frame (struct frame_id a_frame_id, int frame_level)
       return;
     }
 
+  /* This means the selected frame was a user-created one.  Create a new one
+     using the user-provided addresses, which happen to be in the frame id.  */
+  if (frame_level == 0)
+    {
+      gdb_assert (a_frame_id.user_created_p);
+      select_frame (create_new_frame (a_frame_id));
+      return;
+    }
+
   /* select_frame never saves 0 in SELECTED_FRAME_LEVEL, so we
      shouldn't see it here.  */
   gdb_assert (frame_level > 0);
@@ -1859,7 +1872,10 @@ select_frame (frame_info_ptr fi)
 
   selected_frame = fi;
   selected_frame_level = frame_relative_level (fi);
-  if (selected_frame_level == 0)
+
+  /* If the frame is a user-created one, save its level and frame id just like
+     any other non-level-0 frame.  */
+  if (selected_frame_level == 0 && !fi->this_id.value.user_created_p)
     {
       /* Treat the current frame especially -- we want to always
 	 save/restore it without warning, even if the frame ID changes
diff --git a/gdb/testsuite/gdb.base/frame-view.c b/gdb/testsuite/gdb.base/frame-view.c
new file mode 100644
index 000000000000..7b1cbd6abd42
--- /dev/null
+++ b/gdb/testsuite/gdb.base/frame-view.c
@@ -0,0 +1,74 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 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/>.  */
+
+#include <pthread.h>
+#include <assert.h>
+
+struct type_1
+{
+  int m;
+};
+
+struct type_2
+{
+  int n;
+};
+
+static int
+baz (struct type_1 z1, struct type_2 z2)
+{
+  return z1.m + z2.n;
+}
+
+static int
+bar (struct type_1 y1, struct type_2 y2)
+{
+  return baz (y1, y2);
+}
+
+static int
+foo (struct type_1 x1, struct type_2 x2)
+{
+  return bar (x1, x2);
+}
+
+static void *
+thread_func (void *p)
+{
+  struct type_1 t1;
+  struct type_2 t2;
+  t1.m = 11;
+  t2.n = 11;
+  foo (t1, t2);
+
+  return NULL;
+}
+
+int
+main (void)
+{
+  pthread_t thread;
+  int res;
+
+  res = pthread_create (&thread, NULL, thread_func, NULL);
+  assert (res == 0);
+
+  res = pthread_join (thread, NULL);
+  assert (res == 0);
+
+  return 0;
+}
diff --git a/gdb/testsuite/gdb.base/frame-view.exp b/gdb/testsuite/gdb.base/frame-view.exp
new file mode 100644
index 000000000000..d2dba143448d
--- /dev/null
+++ b/gdb/testsuite/gdb.base/frame-view.exp
@@ -0,0 +1,70 @@
+# Copyright 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 the "frame view" family of commands.
+
+standard_testfile
+
+if { [build_executable "failed to prepare" \
+	${testfile} ${srcfile}] } {
+    return
+}
+
+proc test_select_frame_view {} {
+    clean_restart $::binfile
+
+    if { ![runto_main] } {
+	return
+    }
+
+    # Stop thread 2 at a baz.
+    gdb_test "break baz"
+    gdb_test "continue" "Thread 2.*hit Breakpoint $::decimal, baz .*"
+
+    # Grab the stack pointer and pc of thread 2's frame.
+    set frame_sp ""
+    set frame_pc ""
+
+    gdb_test_multiple "info frame" "" {
+	-re -wrap ".*frame at ($::hex):.*" {
+	    set frame_sp $expect_out(1,string)
+	    pass $gdb_test_name
+	}
+    }
+
+    gdb_test_multiple "print/x \$pc" "" {
+	-re -wrap " = ($::hex)" {
+	    set frame_pc $expect_out(1,string)
+	    pass $gdb_test_name
+	}
+    }
+
+    if { $frame_sp == "" || $frame_pc == "" } {
+	# Something must have failed and logged a failure above.
+	return
+    }
+
+    # Select thread 2's frame in thread 1.
+    gdb_test "thread 1" "Switching to thread 1 .*"
+    gdb_test_no_output "select-frame view $frame_sp $frame_pc"
+
+    # Verify that the "frame" command does not change the selected frame.
+    # There used to be a bug where the "frame" command would lose the
+    # selection of user-created frames.
+    gdb_test "frame" "#0  baz \\(z1=.*, z2=.*\\).*" "frame"
+    gdb_test "frame" "#0  baz \\(z1=.*, z2=.*\\).*" "frame again"
+}
+
+test_select_frame_view
-- 
2.38.1


^ permalink raw reply	[flat|nested] 28+ messages in thread

* [PATCH v2 11/13] gdb: make user-created frames reinflatable
  2022-12-14  3:34 [PATCH v2 00/13] Make frame_info_ptr automatic Simon Marchi
                   ` (9 preceding siblings ...)
  2022-12-14  3:34 ` [PATCH v2 10/13] gdb: make it possible to restore selected user-created frames Simon Marchi
@ 2022-12-14  3:34 ` Simon Marchi
  2023-01-23 12:57   ` Tom de Vries
  2022-12-14  3:34 ` [PATCH v2 12/13] gdb: make frame_info_ptr grab frame level and id on construction Simon Marchi
                   ` (4 subsequent siblings)
  15 siblings, 1 reply; 28+ messages in thread
From: Simon Marchi @ 2022-12-14  3:34 UTC (permalink / raw)
  To: gdb-patches; +Cc: Simon Marchi

This patch teaches frame_info_ptr to reinflate user-created frames
(frames created through create_new_frame, with the "select-frame view"
command).

Before this patch, frame_info_ptr doesn't support reinflating
user-created frames, because it currently reinflates by getting the
current target frame (for frame 0) or frame_find_by_id (for other
frames).  To reinflate a user-created frame, we need to call
create_new_frame, to make it lookup an existing user-created frame, or
otherwise create one.

So, in prepare_reinflate, get the frame id even if the frame has level
0, if it is user-created.  In reinflate, if the saved frame id is user
create it, call create_new_frame.

In order to test this, I initially enhanced the gdb.base/frame-view.exp
test added by the previous patch by setting a pretty-printer for the
type of the function parameters, in which we do an inferior call.  This
causes print_frame_args to not reinflate its frame (which is a
user-created one) properly.  On one machine (my Arch Linux one), it
properly catches the bug, as the frame is not correctly restored after
printing the first parameter, so it messes up the second parameter:

    frame
    #0  baz (z1=hahaha, z2=<error reading variable: frame address is not available.>) at /home/simark/src/binutils-gdb/gdb/testsuite/gdb.base/frame-view.c:40
    40        return z1.m + z2.n;
    (gdb) FAIL: gdb.base/frame-view.exp: with_pretty_printer=true: frame
    frame
    #0  baz (z1=hahaha, z2=<error reading variable: frame address is not available.>) at /home/simark/src/binutils-gdb/gdb/testsuite/gdb.base/frame-view.c:40
    40        return z1.m + z2.n;
    (gdb) FAIL: gdb.base/frame-view.exp: with_pretty_printer=true: frame again

However, on another machine (my Ubuntu 22.04 one), it just passes fine,
without the appropriate fix.  I then thought about writing a selftest
for that, it's more reliable.  I left the gdb.base/frame-view.exp pretty
printer test there, it's already written, and we never know, it might
catch some unrelated issue some day.

Change-Id: I5849baf77991fc67a15bfce4b5e865a97265b386
---
 gdb/Makefile.in                          |  1 +
 gdb/frame.c                              | 22 +++++--
 gdb/frame.h                              |  7 ++-
 gdb/testsuite/gdb.base/frame-view.c      |  6 ++
 gdb/testsuite/gdb.base/frame-view.exp    | 47 ++++++++++++--
 gdb/testsuite/gdb.base/frame-view.py     | 41 ++++++++++++
 gdb/unittests/frame_info_ptr-selftests.c | 80 ++++++++++++++++++++++++
 7 files changed, 193 insertions(+), 11 deletions(-)
 create mode 100644 gdb/testsuite/gdb.base/frame-view.py
 create mode 100644 gdb/unittests/frame_info_ptr-selftests.c

diff --git a/gdb/Makefile.in b/gdb/Makefile.in
index 0f5df2ccb7b0..7859315cf366 100644
--- a/gdb/Makefile.in
+++ b/gdb/Makefile.in
@@ -459,6 +459,7 @@ SELFTESTS_SRCS = \
 	unittests/environ-selftests.c \
 	unittests/filtered_iterator-selftests.c \
 	unittests/format_pieces-selftests.c \
+	unittests/frame_info_ptr-selftests.c \
 	unittests/function-view-selftests.c \
 	unittests/gdb_tilde_expand-selftests.c \
 	unittests/gmp-utils-selftests.c \
diff --git a/gdb/frame.c b/gdb/frame.c
index 9fba29ec13eb..873c3912e076 100644
--- a/gdb/frame.c
+++ b/gdb/frame.c
@@ -3213,7 +3213,8 @@ frame_info_ptr::prepare_reinflate ()
 {
   m_cached_level = frame_relative_level (*this);
 
-  if (m_cached_level != 0)
+  if (m_cached_level != 0
+      || (m_ptr != nullptr && m_ptr->this_id.value.user_created_p))
     m_cached_id = get_frame_id (*this);
 }
 
@@ -3232,13 +3233,22 @@ frame_info_ptr::reinflate ()
       return;
     }
 
-  /* Frame #0 needs special handling, see comment in select_frame.  */
-  if (m_cached_level == 0)
-    m_ptr = get_current_frame ().get ();
+  if (m_cached_id.user_created_p)
+    m_ptr = create_new_frame (m_cached_id).get ();
   else
     {
-      gdb_assert (frame_id_p (m_cached_id));
-      m_ptr = frame_find_by_id (m_cached_id).get ();
+      /* Frame #0 needs special handling, see comment in select_frame.  */
+      if (m_cached_level == 0)
+	m_ptr = get_current_frame ().get ();
+      else
+	{
+	  /* If we reach here without a valid frame id, it means we are trying
+	     to reinflate a frame whose id was not know at construction time.
+	     We're probably trying to reinflate a frame while computing its id
+	     which is not possible, and would indicate a problem with GDB.  */
+	  gdb_assert (frame_id_p (m_cached_id));
+	  m_ptr = frame_find_by_id (m_cached_id).get ();
+	}
     }
 
   gdb_assert (m_ptr != nullptr);
diff --git a/gdb/frame.h b/gdb/frame.h
index ebbd9e08868e..88af96f0c619 100644
--- a/gdb/frame.h
+++ b/gdb/frame.h
@@ -325,7 +325,12 @@ class frame_info_ptr : public intrusive_list_node<frame_info_ptr>
   /* The underlying pointer.  */
   frame_info *m_ptr = nullptr;
 
-  /* The frame_id of the underlying pointer.  */
+  /* The frame_id of the underlying pointer.
+
+     For the current target frames (frames with level 0, obtained through
+     get_current_frame), we don't save the frame id, we leave it at
+     null_frame_id.  For user-created frames (also with level 0, but created
+     with create_new_frame), we do save the id.  */
   frame_id m_cached_id = null_frame_id;
 
   /* The frame level of the underlying pointer.  */
diff --git a/gdb/testsuite/gdb.base/frame-view.c b/gdb/testsuite/gdb.base/frame-view.c
index 7b1cbd6abd42..e330da0b1afe 100644
--- a/gdb/testsuite/gdb.base/frame-view.c
+++ b/gdb/testsuite/gdb.base/frame-view.c
@@ -28,6 +28,12 @@ struct type_2
   int n;
 };
 
+__attribute__((used)) static int
+called_from_pretty_printer (void)
+{
+  return 23;
+}
+
 static int
 baz (struct type_1 z1, struct type_2 z2)
 {
diff --git a/gdb/testsuite/gdb.base/frame-view.exp b/gdb/testsuite/gdb.base/frame-view.exp
index d2dba143448d..6c6e4e3fcc37 100644
--- a/gdb/testsuite/gdb.base/frame-view.exp
+++ b/gdb/testsuite/gdb.base/frame-view.exp
@@ -22,13 +22,29 @@ if { [build_executable "failed to prepare" \
     return
 }
 
-proc test_select_frame_view {} {
+# If WITH_PRETTY_PRINTER is true, load pretty printers for the function
+# parameter types, in which we do an inferior call.  This is meant to test
+# that the frame_info_ptr correctly reinflates frames created using
+# "select-frame view".
+
+proc test_select_frame_view { with_pretty_printer } {
     clean_restart $::binfile
 
+    if { $with_pretty_printer && [skip_python_tests] } {
+	untested "skipping Python test"
+	return
+    }
+
     if { ![runto_main] } {
 	return
     }
 
+    if { $with_pretty_printer } {
+	set remote_python_file \
+	    [gdb_remote_download host "${::srcdir}/${::subdir}/${::testfile}.py"]
+	gdb_test_no_output "source ${remote_python_file}" "load python file"
+    }
+
     # Stop thread 2 at a baz.
     gdb_test "break baz"
     gdb_test "continue" "Thread 2.*hit Breakpoint $::decimal, baz .*"
@@ -60,11 +76,34 @@ proc test_select_frame_view {} {
     gdb_test "thread 1" "Switching to thread 1 .*"
     gdb_test_no_output "select-frame view $frame_sp $frame_pc"
 
+    if { $with_pretty_printer } {
+	# When the pretty printer does its infcall, it is done on the currently
+	# selected thread, thread 1 here.  However, other threads are resumed
+	# at the same time.  This causes thread 2 to exit during that infcall,
+	# leading to this weirdness:
+	#
+	#     frame^M
+	#     #0  baz (z=[Thread 0x7ffff7cc26c0 (LWP 417519) exited]^M
+	#     hohoho) at /home/simark/src/binutils-gdb/gdb/testsuite/gdb.base/frame-view.c:35^M
+	#     35        return z.n + 2;^M
+	#
+	# Avoid that by setting scheduler-locking on.
+	gdb_test_no_output "set scheduler-locking on"
+
+	set z1_pattern "hahaha"
+	set z2_pattern "hohoho"
+    } else {
+	set z1_pattern "\\.\\.\\."
+	set z2_pattern "\\.\\.\\."
+    }
+
     # Verify that the "frame" command does not change the selected frame.
     # There used to be a bug where the "frame" command would lose the
     # selection of user-created frames.
-    gdb_test "frame" "#0  baz \\(z1=.*, z2=.*\\).*" "frame"
-    gdb_test "frame" "#0  baz \\(z1=.*, z2=.*\\).*" "frame again"
+    gdb_test "frame" "#0  baz \\(z1=$z1_pattern, z2=$z2_pattern\\).*" "frame"
+    gdb_test "frame" "#0  baz \\(z1=$z1_pattern, z2=$z2_pattern\\).*" "frame again"
 }
 
-test_select_frame_view
+foreach_with_prefix with_pretty_printer {false true} {
+    test_select_frame_view $with_pretty_printer
+}
diff --git a/gdb/testsuite/gdb.base/frame-view.py b/gdb/testsuite/gdb.base/frame-view.py
new file mode 100644
index 000000000000..6c1b8bcf82be
--- /dev/null
+++ b/gdb/testsuite/gdb.base/frame-view.py
@@ -0,0 +1,41 @@
+# Copyright (C) 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/>.
+
+
+class Printer1:
+    def to_string(self):
+        n = gdb.parse_and_eval("called_from_pretty_printer ()")
+        assert n == 23
+        return "hahaha"
+
+
+class Printer2:
+    def to_string(self):
+        n = gdb.parse_and_eval("called_from_pretty_printer ()")
+        assert n == 23
+        return "hohoho"
+
+
+def lookup_function(val):
+    if str(val.type) == "struct type_1":
+        return Printer1()
+
+    if str(val.type) == "struct type_2":
+        return Printer2()
+
+    return None
+
+
+gdb.pretty_printers.append(lookup_function)
diff --git a/gdb/unittests/frame_info_ptr-selftests.c b/gdb/unittests/frame_info_ptr-selftests.c
new file mode 100644
index 000000000000..edf1b9fa0bb7
--- /dev/null
+++ b/gdb/unittests/frame_info_ptr-selftests.c
@@ -0,0 +1,80 @@
+/* Self tests for frame_info_ptr.
+
+   Copyright (C) 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 "defs.h"
+
+#include "frame.h"
+#include "gdbsupport/selftest.h"
+#include "scoped-mock-context.h"
+#include "test-target.h"
+
+namespace selftests {
+
+static void
+validate_user_created_frame (frame_id id)
+{
+  SELF_CHECK (id.stack_status == FID_STACK_VALID);
+  SELF_CHECK (id.stack_addr == 0x1234);
+  SELF_CHECK (id.code_addr_p);
+  SELF_CHECK (id.code_addr == 0x5678);
+}
+
+static frame_info_ptr
+user_created_frame_callee (frame_info_ptr frame)
+{
+  validate_user_created_frame (get_frame_id (frame));
+
+  frame.prepare_reinflate ();
+  reinit_frame_cache ();
+  frame.reinflate ();
+
+  validate_user_created_frame (get_frame_id (frame));
+
+  return frame;
+}
+
+static void
+test_user_created_frame ()
+{
+  scoped_mock_context<test_target_ops> mock_context
+    (current_inferior ()->gdbarch);
+  frame_info_ptr frame = create_new_frame (0x1234, 0x5678);
+
+  validate_user_created_frame (get_frame_id (frame));
+
+  /* Pass the frame to a callee, which calls reinit_frame_cache.  This lets us
+     validate that the reinflation in both the callee and caller restore the
+     same frame_info object.  */
+  frame.prepare_reinflate ();
+  frame_info_ptr callees_frame_info = user_created_frame_callee (frame);
+  frame.reinflate ();
+
+  validate_user_created_frame (get_frame_id (frame));
+  SELF_CHECK (frame.get () == callees_frame_info.get ());
+}
+
+} /* namespace selftests */
+
+void _initialize_frame_info_ptr_selftests ();
+void
+_initialize_frame_info_ptr_selftests ()
+{
+  selftests::register_test ("frame_info_ptr_user",
+			    selftests::test_user_created_frame);
+}
-- 
2.38.1


^ permalink raw reply	[flat|nested] 28+ messages in thread

* [PATCH v2 12/13] gdb: make frame_info_ptr grab frame level and id on construction
  2022-12-14  3:34 [PATCH v2 00/13] Make frame_info_ptr automatic Simon Marchi
                   ` (10 preceding siblings ...)
  2022-12-14  3:34 ` [PATCH v2 11/13] gdb: make user-created frames reinflatable Simon Marchi
@ 2022-12-14  3:34 ` Simon Marchi
  2022-12-14  3:34 ` [PATCH v2 13/13] gdb: make frame_info_ptr auto-reinflatable Simon Marchi
                   ` (3 subsequent siblings)
  15 siblings, 0 replies; 28+ messages in thread
From: Simon Marchi @ 2022-12-14  3:34 UTC (permalink / raw)
  To: gdb-patches; +Cc: Simon Marchi, Bruno Larsen

From: Simon Marchi <simon.marchi@efficios.com>

This is the first step of making frame_info_ptr automatic.  Remove the
frame_info_ptr::prepare_reinflate method, move that code to the
constructor.

Change-Id: I85cdae3ab1c043c70e2702e7fb38e9a4a8a675d8
Reviewed-By: Bruno Larsen <blarsen@redhat.com>
---
 gdb/frame.c                              | 19 +++++++++++--------
 gdb/frame.h                              |  9 +--------
 gdb/infcmd.c                             |  1 -
 gdb/mi/mi-cmd-stack.c                    |  1 -
 gdb/stack.c                              | 10 ----------
 gdb/unittests/frame_info_ptr-selftests.c |  2 --
 6 files changed, 12 insertions(+), 30 deletions(-)

diff --git a/gdb/frame.c b/gdb/frame.c
index 873c3912e076..4755c8e9aa7f 100644
--- a/gdb/frame.c
+++ b/gdb/frame.c
@@ -3208,14 +3208,18 @@ intrusive_list<frame_info_ptr> frame_info_ptr::frame_list;
 
 /* See frame-info-ptr.h.  */
 
-void
-frame_info_ptr::prepare_reinflate ()
+frame_info_ptr::frame_info_ptr (struct frame_info *ptr)
+  : m_ptr (ptr)
 {
-  m_cached_level = frame_relative_level (*this);
+  frame_list.push_back (*this);
+
+  if (m_ptr == nullptr)
+    return;
+
+  m_cached_level = ptr->level;
 
-  if (m_cached_level != 0
-      || (m_ptr != nullptr && m_ptr->this_id.value.user_created_p))
-    m_cached_id = get_frame_id (*this);
+  if (m_cached_level != 0 || m_ptr->this_id.value.user_created_p)
+    m_cached_id = m_ptr->this_id.value;
 }
 
 /* See frame-info-ptr.h.  */
@@ -3223,8 +3227,7 @@ frame_info_ptr::prepare_reinflate ()
 void
 frame_info_ptr::reinflate ()
 {
-  /* Ensure we have a valid frame level (sentinel frame or above), indicating
-     prepare_reinflate was called.  */
+  /* Ensure we have a valid frame level (sentinel frame or above).  */
   gdb_assert (m_cached_level >= -1);
 
   if (m_ptr != nullptr)
diff --git a/gdb/frame.h b/gdb/frame.h
index 88af96f0c619..f8ca57815bf6 100644
--- a/gdb/frame.h
+++ b/gdb/frame.h
@@ -206,11 +206,7 @@ class frame_info_ptr : public intrusive_list_node<frame_info_ptr>
 {
 public:
   /* Create a frame_info_ptr from a raw pointer.  */
-  explicit frame_info_ptr (struct frame_info *ptr)
-    : m_ptr (ptr)
-  {
-    frame_list.push_back (*this);
-  }
+  explicit frame_info_ptr (struct frame_info *ptr);
 
   /* Create a null frame_info_ptr.  */
   frame_info_ptr ()
@@ -310,9 +306,6 @@ class frame_info_ptr : public intrusive_list_node<frame_info_ptr>
     m_ptr = nullptr;
   }
 
-  /* Cache the frame_id that the pointer will use to reinflate.  */
-  void prepare_reinflate ();
-
   /* Use the cached frame_id to reinflate the pointer.  */
   void reinflate ();
 
diff --git a/gdb/infcmd.c b/gdb/infcmd.c
index a27d3577b3aa..ee3d97b585dd 100644
--- a/gdb/infcmd.c
+++ b/gdb/infcmd.c
@@ -1837,7 +1837,6 @@ finish_command (const char *arg, int from_tty)
   frame = get_prev_frame (get_selected_frame (_("No selected frame.")));
   if (frame == 0)
     error (_("\"finish\" not meaningful in the outermost frame."));
-  frame.prepare_reinflate ();
 
   clear_proceed_status (0);
 
diff --git a/gdb/mi/mi-cmd-stack.c b/gdb/mi/mi-cmd-stack.c
index f0af7c9a0143..186d53156730 100644
--- a/gdb/mi/mi-cmd-stack.c
+++ b/gdb/mi/mi-cmd-stack.c
@@ -176,7 +176,6 @@ mi_cmd_stack_list_frames (const char *command, char **argv, int argc)
 	   i++, fi = get_prev_frame (fi))
 	{
 	  QUIT;
-	  fi.prepare_reinflate ();
 	  /* Print the location and the address always, even for level 0.
 	     If args is 0, don't print the arguments.  */
 	  print_frame_info (user_frame_print_options,
diff --git a/gdb/stack.c b/gdb/stack.c
index 4ad51c2eb50e..126ce0b8cc2a 100644
--- a/gdb/stack.c
+++ b/gdb/stack.c
@@ -362,7 +362,6 @@ print_stack_frame (frame_info_ptr frame, int print_level,
   if (current_uiout->is_mi_like_p ())
     print_what = LOC_AND_ADDRESS;
 
-  frame.prepare_reinflate ();
   try
     {
       print_frame_info (user_frame_print_options,
@@ -744,11 +743,6 @@ print_frame_args (const frame_print_options &fp_opts,
     = (print_names
        && fp_opts.print_frame_arguments != print_frame_arguments_none);
 
-  /* If one of the arguments has a pretty printer that calls a
-     function of the inferior to print it, the pointer must be
-     reinflatable.  */
-  frame.prepare_reinflate ();
-
   /* Temporarily change the selected frame to the given FRAME.
      This allows routines that rely on the selected frame instead
      of being given a frame as parameter to use the correct frame.  */
@@ -1047,8 +1041,6 @@ print_frame_info (const frame_print_options &fp_opts,
   int location_print;
   struct ui_out *uiout = current_uiout;
 
-  frame.prepare_reinflate ();
-
   if (!current_uiout->is_mi_like_p ()
       && fp_opts.print_frame_info != print_frame_info_auto)
     {
@@ -1682,7 +1674,6 @@ info_frame_command_core (frame_info_ptr fi, bool selected_frame_p)
 	      gdb_printf (" %d args: ", numargs);
 	  }
 
-	fi.prepare_reinflate ();
 	print_frame_args (user_frame_print_options,
 			  func, fi, numargs, gdb_stdout);
 	fi.reinflate ();
@@ -2075,7 +2066,6 @@ backtrace_command_1 (const frame_print_options &fp_opts,
       for (fi = trailing; fi && count--; fi = get_prev_frame (fi))
 	{
 	  QUIT;
-	  fi.prepare_reinflate ();
 
 	  /* Don't use print_stack_frame; if an error() occurs it probably
 	     means further attempts to backtrace would fail (on the other
diff --git a/gdb/unittests/frame_info_ptr-selftests.c b/gdb/unittests/frame_info_ptr-selftests.c
index edf1b9fa0bb7..47306113c7aa 100644
--- a/gdb/unittests/frame_info_ptr-selftests.c
+++ b/gdb/unittests/frame_info_ptr-selftests.c
@@ -40,7 +40,6 @@ user_created_frame_callee (frame_info_ptr frame)
 {
   validate_user_created_frame (get_frame_id (frame));
 
-  frame.prepare_reinflate ();
   reinit_frame_cache ();
   frame.reinflate ();
 
@@ -61,7 +60,6 @@ test_user_created_frame ()
   /* Pass the frame to a callee, which calls reinit_frame_cache.  This lets us
      validate that the reinflation in both the callee and caller restore the
      same frame_info object.  */
-  frame.prepare_reinflate ();
   frame_info_ptr callees_frame_info = user_created_frame_callee (frame);
   frame.reinflate ();
 
-- 
2.38.1


^ permalink raw reply	[flat|nested] 28+ messages in thread

* [PATCH v2 13/13] gdb: make frame_info_ptr auto-reinflatable
  2022-12-14  3:34 [PATCH v2 00/13] Make frame_info_ptr automatic Simon Marchi
                   ` (11 preceding siblings ...)
  2022-12-14  3:34 ` [PATCH v2 12/13] gdb: make frame_info_ptr grab frame level and id on construction Simon Marchi
@ 2022-12-14  3:34 ` Simon Marchi
  2022-12-20 16:57 ` [PATCH v2 00/13] Make frame_info_ptr automatic Bruno Larsen
                   ` (2 subsequent siblings)
  15 siblings, 0 replies; 28+ messages in thread
From: Simon Marchi @ 2022-12-14  3:34 UTC (permalink / raw)
  To: gdb-patches; +Cc: Simon Marchi

From: Simon Marchi <simon.marchi@efficios.com>

This is the second step of making frame_info_ptr automatic, reinflate on
demand whenever trying to obtain the wrapper frame_info pointer, either
through the get method or operator->.  Make the reinflate method
private, it is used as a convenience method in those two.

Add an "is_null" method, because it is often needed to know whether the
frame_info_ptr wraps an frame_info or is empty.

Make m_ptr mutable, so that it's possible to reinflate const
frame_info_ptr objects.  Whether m_ptr is nullptr or not does not change
the logical state of the object, because we re-create it on demand.  I
believe this is the right use case for mutable.

Change-Id: Icb0552d0035e227f81eb3c121d8a9bb2f9d25794
---
 gdb/frame.c                              |  7 ++--
 gdb/frame.h                              | 45 +++++++++++++++++-------
 gdb/infcmd.c                             |  1 -
 gdb/mi/mi-cmd-stack.c                    |  1 -
 gdb/stack.c                              |  6 ----
 gdb/unittests/frame_info_ptr-selftests.c |  2 --
 6 files changed, 36 insertions(+), 26 deletions(-)

diff --git a/gdb/frame.c b/gdb/frame.c
index 4755c8e9aa7f..ef58253312e8 100644
--- a/gdb/frame.c
+++ b/gdb/frame.c
@@ -3224,8 +3224,8 @@ frame_info_ptr::frame_info_ptr (struct frame_info *ptr)
 
 /* See frame-info-ptr.h.  */
 
-void
-frame_info_ptr::reinflate ()
+frame_info *
+frame_info_ptr::reinflate () const
 {
   /* Ensure we have a valid frame level (sentinel frame or above).  */
   gdb_assert (m_cached_level >= -1);
@@ -3233,7 +3233,7 @@ frame_info_ptr::reinflate ()
   if (m_ptr != nullptr)
     {
       /* The frame_info wasn't invalidated, no need to reinflate.  */
-      return;
+      return m_ptr;
     }
 
   if (m_cached_id.user_created_p)
@@ -3255,6 +3255,7 @@ frame_info_ptr::reinflate ()
     }
 
   gdb_assert (m_ptr != nullptr);
+  return m_ptr;
 }
 
 void _initialize_frame ();
diff --git a/gdb/frame.h b/gdb/frame.h
index f8ca57815bf6..e02635c21408 100644
--- a/gdb/frame.h
+++ b/gdb/frame.h
@@ -275,29 +275,38 @@ class frame_info_ptr : public intrusive_list_node<frame_info_ptr>
   }
 
   frame_info *operator-> () const
-  {
-    return m_ptr;
-  }
+  { return this->reinflate (); }
 
   /* Fetch the underlying pointer.  Note that new code should
      generally not use this -- avoid it if at all possible.  */
   frame_info *get () const
   {
-    return m_ptr;
+    if (this->is_null ())
+      return nullptr;
+
+    return this->reinflate ();
   }
 
+  /* Return true if this object is empty (does not wrap a frame_info
+     object).  */
+
+  bool is_null () const
+  {
+    return m_cached_level == this->invalid_level;
+  };
+
   /* This exists for compatibility with pre-existing code that checked
      a "frame_info *" using "!".  */
   bool operator! () const
   {
-    return m_ptr == nullptr;
+    return this->is_null ();
   }
 
   /* This exists for compatibility with pre-existing code that checked
      a "frame_info *" like "if (ptr)".  */
   explicit operator bool () const
   {
-    return m_ptr != nullptr;
+    return !this->is_null ();
   }
 
   /* Invalidate this pointer.  */
@@ -306,17 +315,18 @@ class frame_info_ptr : public intrusive_list_node<frame_info_ptr>
     m_ptr = nullptr;
   }
 
-  /* Use the cached frame_id to reinflate the pointer.  */
-  void reinflate ();
-
 private:
   /* We sometimes need to construct frame_info_ptr objects around the
      sentinel_frame, which has level -1.  Therefore, make the invalid frame
      level value -2.  */
   static constexpr int invalid_level = -2;
 
+  /* Use the cached frame level and id to reinflate the pointer, and return
+     it.  */
+  frame_info *reinflate () const;
+
   /* The underlying pointer.  */
-  frame_info *m_ptr = nullptr;
+  mutable frame_info *m_ptr = nullptr;
 
   /* The frame_id of the underlying pointer.
 
@@ -341,37 +351,46 @@ class frame_info_ptr : public intrusive_list_node<frame_info_ptr>
 static inline bool
 operator== (const frame_info *self, const frame_info_ptr &other)
 {
+  if (self == nullptr || other.is_null ())
+    return self == nullptr && other.is_null ();
+
   return self == other.get ();
 }
 
 static inline bool
 operator== (const frame_info_ptr &self, const frame_info_ptr &other)
 {
+  if (self.is_null () || other.is_null ())
+    return self.is_null () && other.is_null ();
+
   return self.get () == other.get ();
 }
 
 static inline bool
 operator== (const frame_info_ptr &self, const frame_info *other)
 {
+  if (self.is_null () || other == nullptr)
+    return self.is_null () && other == nullptr;
+
   return self.get () == other;
 }
 
 static inline bool
 operator!= (const frame_info *self, const frame_info_ptr &other)
 {
-  return self != other.get ();
+  return !(self == other);
 }
 
 static inline bool
 operator!= (const frame_info_ptr &self, const frame_info_ptr &other)
 {
-  return self.get () != other.get ();
+  return !(self == other);
 }
 
 static inline bool
 operator!= (const frame_info_ptr &self, const frame_info *other)
 {
-  return self.get () != other;
+  return !(self == other);
 }
 
 /* For every stopped thread, GDB tracks two frames: current and
diff --git a/gdb/infcmd.c b/gdb/infcmd.c
index ee3d97b585dd..d1f634f86b1d 100644
--- a/gdb/infcmd.c
+++ b/gdb/infcmd.c
@@ -1917,7 +1917,6 @@ finish_command (const char *arg, int from_tty)
 
       print_stack_frame (callee_frame, 1, LOCATION, 0);
     }
-  frame.reinflate ();
 
   if (execution_direction == EXEC_REVERSE)
     finish_backward (sm);
diff --git a/gdb/mi/mi-cmd-stack.c b/gdb/mi/mi-cmd-stack.c
index 186d53156730..16be1938c033 100644
--- a/gdb/mi/mi-cmd-stack.c
+++ b/gdb/mi/mi-cmd-stack.c
@@ -180,7 +180,6 @@ mi_cmd_stack_list_frames (const char *command, char **argv, int argc)
 	     If args is 0, don't print the arguments.  */
 	  print_frame_info (user_frame_print_options,
 			    fi, 1, LOC_AND_ADDRESS, 0 /* args */, 0);
-	  fi.reinflate ();
 	}
     }
 }
diff --git a/gdb/stack.c b/gdb/stack.c
index 126ce0b8cc2a..5bc5b2d26cfe 100644
--- a/gdb/stack.c
+++ b/gdb/stack.c
@@ -367,7 +367,6 @@ print_stack_frame (frame_info_ptr frame, int print_level,
       print_frame_info (user_frame_print_options,
 			frame, print_level, print_what, 1 /* print_args */,
 			set_current_sal);
-      frame.reinflate ();
       if (set_current_sal)
 	set_current_sal_from_frame (frame);
     }
@@ -903,7 +902,6 @@ print_frame_args (const frame_print_options &fp_opts,
 	    }
 
 	  first = 0;
-	  frame.reinflate ();
 	}
     }
 
@@ -1173,7 +1171,6 @@ print_frame_info (const frame_print_options &fp_opts,
 
 	  print_source_lines (sal.symtab, sal.line, sal.line + 1, 0);
 	}
-      frame.reinflate ();
 
       /* If disassemble-next-line is set to on and there is line debug
 	 messages, output assembly codes for next line.  */
@@ -1676,8 +1673,6 @@ info_frame_command_core (frame_info_ptr fi, bool selected_frame_p)
 
 	print_frame_args (user_frame_print_options,
 			  func, fi, numargs, gdb_stdout);
-	fi.reinflate ();
-
 	gdb_puts ("\n");
       }
   }
@@ -2077,7 +2072,6 @@ backtrace_command_1 (const frame_print_options &fp_opts,
 	    print_frame_local_vars (fi, false, NULL, NULL, 1, gdb_stdout);
 
 	  /* Save the last frame to check for error conditions.  */
-	  fi.reinflate ();
 	  trailing = fi;
 	}
 
diff --git a/gdb/unittests/frame_info_ptr-selftests.c b/gdb/unittests/frame_info_ptr-selftests.c
index 47306113c7aa..fe1e7be35daa 100644
--- a/gdb/unittests/frame_info_ptr-selftests.c
+++ b/gdb/unittests/frame_info_ptr-selftests.c
@@ -41,7 +41,6 @@ user_created_frame_callee (frame_info_ptr frame)
   validate_user_created_frame (get_frame_id (frame));
 
   reinit_frame_cache ();
-  frame.reinflate ();
 
   validate_user_created_frame (get_frame_id (frame));
 
@@ -61,7 +60,6 @@ test_user_created_frame ()
      validate that the reinflation in both the callee and caller restore the
      same frame_info object.  */
   frame_info_ptr callees_frame_info = user_created_frame_callee (frame);
-  frame.reinflate ();
 
   validate_user_created_frame (get_frame_id (frame));
   SELF_CHECK (frame.get () == callees_frame_info.get ());
-- 
2.38.1


^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH v2 00/13] Make frame_info_ptr automatic
  2022-12-14  3:34 [PATCH v2 00/13] Make frame_info_ptr automatic Simon Marchi
                   ` (12 preceding siblings ...)
  2022-12-14  3:34 ` [PATCH v2 13/13] gdb: make frame_info_ptr auto-reinflatable Simon Marchi
@ 2022-12-20 16:57 ` Bruno Larsen
  2023-01-03 19:00   ` Simon Marchi
  2023-01-03 19:09 ` Simon Marchi
  2023-01-18 18:10 ` Tom Tromey
  15 siblings, 1 reply; 28+ messages in thread
From: Bruno Larsen @ 2022-12-20 16:57 UTC (permalink / raw)
  To: Simon Marchi, gdb-patches

On 14/12/2022 04:34, Simon Marchi via Gdb-patches wrote:
> This is v2 of:
>
>    https://inbox.sourceware.org/gdb-patches/20221202180052.212745-1-simon.marchi@polymtl.ca/
>
> The first main change is that I dropped the patch removing the user of
> frame_info_ptr at many places, following Tom's comment.  If we end up
> trying to reinflate a frame at a moment where it's not possible (because
> the frame's id is being computed), we'll hit an assert in
> frame_info_ptr.  This is fine, because it indicates some logic problem
> in GDB.
>
> The second main change, found after doing the first one, is to fix a bug
> with the user-created frame reinflation.  In v1, if we had two
> frame_info_ptr wrapping the same user-created frame_info object, they
> would each create their own frame_info instance on reinflation.  And
> that confused GDB down the line.  There are a few new patches to deal
> with that, by adding user-created frames to the frame stash, so they can
> be found by frame_info_ptr when reinflating.
>
> At the end of the series, frame_info_ptr::frame_info_ptr (at the end of
> the series) accesses some frame_info internals (the frame's id, without
> calling get_frame_id), so I decided to move frame_info_ptr to
> frame.{c,h} (patch 6).  I think that makes sense anyway, as the logic in
> the implementation of frame_info_ptr is tightly coupled to the rest of
> the frame stuff.  However, to do so, we need to break some inclusion
> cycles.  This is done in the few patches before.  Patches 1 and 2 are
> not strictly necessary, but fix oddities I found along the way.
>
> Regression-tested on Ubuntu 22.04 x86-64.

This series doesn't add any regressions and is where I wanted to take 
frame_info_ptr all along. I have only one nit with patch 6 (sent as a 
direct reply), but other than that:

Reviewed-By: Bruno Larsen <blarsen@redhat.com>

-- 
Cheers,
Bruno

>
> Simon Marchi (13):
>    gdb: move type_map_instance to compile/compile.c
>    gdb: move compile_instance to compile/compile.h
>    gdb: remove language.h include from frame.h
>    gdb: move sect_offset and cu_offset to dwarf2/types.h
>    gdb: move call site types to call-site.h
>    gdb: move frame_info_ptr to frame.{c,h}
>    gdb: add frame_id::user_created_p
>    gdb: add user-created frames to stash
>    gdb: add create_new_frame(frame_id) overload
>    gdb: make it possible to restore selected user-created frames
>    gdb: make user-created frames reinflatable
>    gdb: make frame_info_ptr grab frame level and id on construction
>    gdb: make frame_info_ptr auto-reinflatable
>
>   gdb/Makefile.in                          |   2 +-
>   gdb/aarch64-tdep.c                       |   1 +
>   gdb/amd64-tdep.c                         |   1 +
>   gdb/arm-tdep.c                           |   1 +
>   gdb/c-lang.c                             |   1 -
>   gdb/c-lang.h                             |   2 +
>   gdb/compile/compile-c.h                  |   1 +
>   gdb/compile/compile-cplus.h              |   1 +
>   gdb/compile/compile-internal.h           | 129 ------------
>   gdb/compile/compile.c                    |  13 ++
>   gdb/compile/compile.h                    | 116 +++++++++++
>   gdb/cp-abi.c                             |   1 +
>   gdb/cp-support.c                         |   1 +
>   gdb/dwarf2/abbrev-cache.h                |   1 -
>   gdb/dwarf2/attribute.h                   |   2 +-
>   gdb/dwarf2/call-site.h                   | 244 +++++++++++++++++++++++
>   gdb/dwarf2/comp-unit-head.h              |   5 +-
>   gdb/dwarf2/cooked-index.h                |   2 +-
>   gdb/dwarf2/expr.h                        |   2 +-
>   gdb/dwarf2/line-header.h                 |   2 -
>   gdb/dwarf2/types.h                       |  40 ++++
>   gdb/expop.h                              |   1 +
>   gdb/f-lang.h                             |   1 +
>   gdb/frame-id.h                           |   4 +
>   gdb/frame-info.c                         |  65 ------
>   gdb/frame-info.h                         | 211 --------------------
>   gdb/frame.c                              | 127 ++++++++++--
>   gdb/frame.h                              | 206 ++++++++++++++++++-
>   gdb/gdbtypes.h                           | 229 ---------------------
>   gdb/gnu-v3-abi.c                         |   1 +
>   gdb/go-lang.h                            |   1 +
>   gdb/infcmd.c                             |   2 -
>   gdb/language.c                           |   1 -
>   gdb/m2-typeprint.c                       |   1 +
>   gdb/mi/mi-cmd-stack.c                    |   2 -
>   gdb/ppc-sysv-tdep.c                      |   1 +
>   gdb/python/py-disasm.c                   |   1 +
>   gdb/python/py-frame.c                    |   1 +
>   gdb/stack.c                              |  16 --
>   gdb/symtab.c                             |   1 +
>   gdb/symtab.h                             |   1 +
>   gdb/testsuite/gdb.base/frame-view.c      |  80 ++++++++
>   gdb/testsuite/gdb.base/frame-view.exp    | 109 ++++++++++
>   gdb/testsuite/gdb.base/frame-view.py     |  41 ++++
>   gdb/thread.c                             |   1 +
>   gdb/unittests/frame_info_ptr-selftests.c |  76 +++++++
>   46 files changed, 1065 insertions(+), 683 deletions(-)
>   create mode 100644 gdb/dwarf2/call-site.h
>   create mode 100644 gdb/dwarf2/types.h
>   delete mode 100644 gdb/frame-info.c
>   delete mode 100644 gdb/frame-info.h
>   create mode 100644 gdb/testsuite/gdb.base/frame-view.c
>   create mode 100644 gdb/testsuite/gdb.base/frame-view.exp
>   create mode 100644 gdb/testsuite/gdb.base/frame-view.py
>   create mode 100644 gdb/unittests/frame_info_ptr-selftests.c
>
>
> base-commit: a4b83845ded201e7d46220213d3e38950c30dbb5


^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH v2 06/13] gdb: move frame_info_ptr to frame.{c,h}
  2022-12-14  3:34 ` [PATCH v2 06/13] gdb: move frame_info_ptr to frame.{c,h} Simon Marchi
@ 2022-12-20 17:01   ` Bruno Larsen
  2023-01-03 18:59     ` Simon Marchi
  0 siblings, 1 reply; 28+ messages in thread
From: Bruno Larsen @ 2022-12-20 17:01 UTC (permalink / raw)
  To: Simon Marchi, gdb-patches

On 14/12/2022 04:34, Simon Marchi via Gdb-patches wrote:
> Change-Id: Ic5949759e6262ea0da6123858702d48fe5673fea
> ---
>   gdb/Makefile.in        |   1 -
>   gdb/c-lang.h           |   1 +
>   gdb/dwarf2/call-site.h |   2 +-
>   gdb/frame-info.c       |  65 -------------
>   gdb/frame-info.h       | 211 -----------------------------------------
>   gdb/frame.c            |  43 ++++++++-
>   gdb/frame.h            | 188 +++++++++++++++++++++++++++++++++++-
>   gdb/gdbtypes.h         |   2 -
>   gdb/symtab.h           |   1 +
>   9 files changed, 230 insertions(+), 284 deletions(-)
>   delete mode 100644 gdb/frame-info.c
>   delete mode 100644 gdb/frame-info.h

Not sure if this is on purpose but this commit doesn't seem to have a 
commit message. If you'd like some extra context to write it, I ran into 
cyclic includes when writing the original version (which you found and 
fixed). Since I am not familiar with the rest of the code, a new file 
seemed like a good enough hack to get things working. Now that you 
solved the cyclic includes, it makes sense to remove the files.

-- 
Cheers,
Bruno


^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH v2 06/13] gdb: move frame_info_ptr to frame.{c,h}
  2022-12-20 17:01   ` Bruno Larsen
@ 2023-01-03 18:59     ` Simon Marchi
  0 siblings, 0 replies; 28+ messages in thread
From: Simon Marchi @ 2023-01-03 18:59 UTC (permalink / raw)
  To: Bruno Larsen, gdb-patches



On 12/20/22 12:01, Bruno Larsen wrote:
> On 14/12/2022 04:34, Simon Marchi via Gdb-patches wrote:
>> Change-Id: Ic5949759e6262ea0da6123858702d48fe5673fea
>> ---
>>   gdb/Makefile.in        |   1 -
>>   gdb/c-lang.h           |   1 +
>>   gdb/dwarf2/call-site.h |   2 +-
>>   gdb/frame-info.c       |  65 -------------
>>   gdb/frame-info.h       | 211 -----------------------------------------
>>   gdb/frame.c            |  43 ++++++++-
>>   gdb/frame.h            | 188 +++++++++++++++++++++++++++++++++++-
>>   gdb/gdbtypes.h         |   2 -
>>   gdb/symtab.h           |   1 +
>>   9 files changed, 230 insertions(+), 284 deletions(-)
>>   delete mode 100644 gdb/frame-info.c
>>   delete mode 100644 gdb/frame-info.h
> 

> Not sure if this is on purpose but this commit doesn't seem to have a
> commit message. If you'd like some extra context to write it, I ran
> into cyclic includes when writing the original version (which you
> found and fixed). Since I am not familiar with the rest of the code, a
> new file seemed like a good enough hack to get things working. Now
> that you solved the cyclic includes, it makes sense to remove the
> files.

No, it wasn't intended, I must have forgotten to write the message for
this commit!  Here it is, I will update my local branch:

    A patch later in this series will make frame_info_ptr access some
    fields internal to frame_info, which we don't want to expose outside of
    frame.c.  Move the frame_info_ptr class to frame.h, and the definitions
    to frame.c.  Remove frame-info.c and frame-info.h.

Simon

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH v2 00/13] Make frame_info_ptr automatic
  2022-12-20 16:57 ` [PATCH v2 00/13] Make frame_info_ptr automatic Bruno Larsen
@ 2023-01-03 19:00   ` Simon Marchi
  0 siblings, 0 replies; 28+ messages in thread
From: Simon Marchi @ 2023-01-03 19:00 UTC (permalink / raw)
  To: Bruno Larsen, gdb-patches



On 12/20/22 11:57, Bruno Larsen wrote:
> On 14/12/2022 04:34, Simon Marchi via Gdb-patches wrote:
>> This is v2 of:
>>
>>    https://inbox.sourceware.org/gdb-patches/20221202180052.212745-1-simon.marchi@polymtl.ca/
>>
>> The first main change is that I dropped the patch removing the user of
>> frame_info_ptr at many places, following Tom's comment.  If we end up
>> trying to reinflate a frame at a moment where it's not possible (because
>> the frame's id is being computed), we'll hit an assert in
>> frame_info_ptr.  This is fine, because it indicates some logic problem
>> in GDB.
>>
>> The second main change, found after doing the first one, is to fix a bug
>> with the user-created frame reinflation.  In v1, if we had two
>> frame_info_ptr wrapping the same user-created frame_info object, they
>> would each create their own frame_info instance on reinflation.  And
>> that confused GDB down the line.  There are a few new patches to deal
>> with that, by adding user-created frames to the frame stash, so they can
>> be found by frame_info_ptr when reinflating.
>>
>> At the end of the series, frame_info_ptr::frame_info_ptr (at the end of
>> the series) accesses some frame_info internals (the frame's id, without
>> calling get_frame_id), so I decided to move frame_info_ptr to
>> frame.{c,h} (patch 6).  I think that makes sense anyway, as the logic in
>> the implementation of frame_info_ptr is tightly coupled to the rest of
>> the frame stuff.  However, to do so, we need to break some inclusion
>> cycles.  This is done in the few patches before.  Patches 1 and 2 are
>> not strictly necessary, but fix oddities I found along the way.
>>
>> Regression-tested on Ubuntu 22.04 x86-64.
> 
> This series doesn't add any regressions and is where I wanted to take frame_info_ptr all along. I have only one nit with patch 6 (sent as a direct reply), but other than that:
> 
> Reviewed-By: Bruno Larsen <blarsen@redhat.com>
> 

Thanks for reviewing, I will add your RB to all patches.

Simon

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH v2 00/13] Make frame_info_ptr automatic
  2022-12-14  3:34 [PATCH v2 00/13] Make frame_info_ptr automatic Simon Marchi
                   ` (13 preceding siblings ...)
  2022-12-20 16:57 ` [PATCH v2 00/13] Make frame_info_ptr automatic Bruno Larsen
@ 2023-01-03 19:09 ` Simon Marchi
  2023-01-18 18:10 ` Tom Tromey
  15 siblings, 0 replies; 28+ messages in thread
From: Simon Marchi @ 2023-01-03 19:09 UTC (permalink / raw)
  To: gdb-patches



On 12/13/22 22:34, Simon Marchi wrote:
> This is v2 of:
> 
>   https://inbox.sourceware.org/gdb-patches/20221202180052.212745-1-simon.marchi@polymtl.ca/
> 
> The first main change is that I dropped the patch removing the user of
> frame_info_ptr at many places, following Tom's comment.  If we end up
> trying to reinflate a frame at a moment where it's not possible (because
> the frame's id is being computed), we'll hit an assert in
> frame_info_ptr.  This is fine, because it indicates some logic problem
> in GDB.
> 
> The second main change, found after doing the first one, is to fix a bug
> with the user-created frame reinflation.  In v1, if we had two
> frame_info_ptr wrapping the same user-created frame_info object, they
> would each create their own frame_info instance on reinflation.  And
> that confused GDB down the line.  There are a few new patches to deal
> with that, by adding user-created frames to the frame stash, so they can
> be found by frame_info_ptr when reinflating.
> 
> At the end of the series, frame_info_ptr::frame_info_ptr (at the end of
> the series) accesses some frame_info internals (the frame's id, without
> calling get_frame_id), so I decided to move frame_info_ptr to
> frame.{c,h} (patch 6).  I think that makes sense anyway, as the logic in
> the implementation of frame_info_ptr is tightly coupled to the rest of
> the frame stuff.  However, to do so, we need to break some inclusion
> cycles.  This is done in the few patches before.  Patches 1 and 2 are
> not strictly necessary, but fix oddities I found along the way.
> 
> Regression-tested on Ubuntu 22.04 x86-64.

FYI, I intend to merge this series by the end of the week.  It was
reviewed by Bruno and acknowledged by Tom Tromey.

Simon

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH v2 00/13] Make frame_info_ptr automatic
  2022-12-14  3:34 [PATCH v2 00/13] Make frame_info_ptr automatic Simon Marchi
                   ` (14 preceding siblings ...)
  2023-01-03 19:09 ` Simon Marchi
@ 2023-01-18 18:10 ` Tom Tromey
  2023-01-19  3:40   ` Simon Marchi
  15 siblings, 1 reply; 28+ messages in thread
From: Tom Tromey @ 2023-01-18 18:10 UTC (permalink / raw)
  To: Simon Marchi via Gdb-patches; +Cc: Simon Marchi

>>>>> "Simon" == Simon Marchi via Gdb-patches <gdb-patches@sourceware.org> writes:

Simon> This is v2 of:
Simon>   https://inbox.sourceware.org/gdb-patches/20221202180052.212745-1-simon.marchi@polymtl.ca/

FWIW this seems good to me.  It's definitely more robust in that it
eliminates this class of bug.

Simon> The first main change is that I dropped the patch removing the user of
Simon> frame_info_ptr at many places, following Tom's comment.

I don't think I intended for you to drop that, at least if we're talking
about this email:

https://inbox.sourceware.org/gdb-patches/87cz8xjzp5.fsf@tromey.com/

If the current series works, though, then that seems fine as well.

Thanks for doing this.

Tom

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH v2 00/13] Make frame_info_ptr automatic
  2023-01-18 18:10 ` Tom Tromey
@ 2023-01-19  3:40   ` Simon Marchi
  0 siblings, 0 replies; 28+ messages in thread
From: Simon Marchi @ 2023-01-19  3:40 UTC (permalink / raw)
  To: Tom Tromey, Simon Marchi via Gdb-patches



On 1/18/23 13:10, Tom Tromey wrote:
>>>>>> "Simon" == Simon Marchi via Gdb-patches <gdb-patches@sourceware.org> writes:
> 
> Simon> This is v2 of:
> Simon>   https://inbox.sourceware.org/gdb-patches/20221202180052.212745-1-simon.marchi@polymtl.ca/
> 
> FWIW this seems good to me.  It's definitely more robust in that it
> eliminates this class of bug.
> 
> Simon> The first main change is that I dropped the patch removing the user of
> Simon> frame_info_ptr at many places, following Tom's comment.
> 
> I don't think I intended for you to drop that, at least if we're talking
> about this email:
> 
> https://inbox.sourceware.org/gdb-patches/87cz8xjzp5.fsf@tromey.com/
> 
> If the current series works, though, then that seems fine as well.

While replying to that email (this reply [1]), I actually became
convinced that it would be fine and simpler to just use frame_info_ptr
everywhere, so I did not do that change against my will.

I will rebase and push, if everything still looks good after the rebase.

Simon

[1] https://inbox.sourceware.org/gdb-patches/b73e19be-165d-bcfb-3baf-9f56fcdc9537@polymtl.ca/

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH v2 11/13] gdb: make user-created frames reinflatable
  2022-12-14  3:34 ` [PATCH v2 11/13] gdb: make user-created frames reinflatable Simon Marchi
@ 2023-01-23 12:57   ` Tom de Vries
  2023-01-23 14:34     ` Luis Machado
  0 siblings, 1 reply; 28+ messages in thread
From: Tom de Vries @ 2023-01-23 12:57 UTC (permalink / raw)
  To: Simon Marchi, gdb-patches; +Cc: Luis Machado

On 12/14/22 04:34, Simon Marchi via Gdb-patches wrote:
>   gdb/testsuite/gdb.base/frame-view.exp    | 47 ++++++++++++--

Hi,

on aarch64-linux I get:
...
FAIL: gdb.base/frame-view.exp: with_pretty_printer=true: frame (GDB 
internal error)
FAIL: gdb.base/frame-view.exp: with_pretty_printer=true: frame again 
(GDB internal error)
...

In more detail:
...
(gdb) PASS: gdb.base/frame-view.exp: with_pretty_printer=true: set 
scheduler-locking on
frame^M
#0  baz (z1=hahaha, /home/tdevries/gdb/src/gdb/value.c:4056: 
internal-error: value_fetch_lazy_register: Assertion `next_frame != 
NULL' failed.^M
...


Back-trace:
...
----- Backtrace -----
Resyncing due to internal error.
0x563033 gdb_internal_backtrace_1
         /home/tdevries/gdb/src/gdb/bt-utils.c:122
0x5630f3 _Z22gdb_internal_backtracev
         /home/tdevries/gdb/src/gdb/bt-utils.c:168
0xb4461f internal_vproblem
         /home/tdevries/gdb/src/gdb/utils.c:396
0xb44a33 _Z15internal_verrorPKciS0_St9__va_list
         /home/tdevries/gdb/src/gdb/utils.c:476
0xdb8e97 _Z18internal_error_locPKciS0_z
         /home/tdevries/gdb/src/gdbsupport/errors.cc:58
0xb7a1d7 value_fetch_lazy_register
         /home/tdevries/gdb/src/gdb/value.c:4056
0xb7a88b _Z16value_fetch_lazyP5value
         /home/tdevries/gdb/src/gdb/value.c:4180
0xb74c93 _Z19value_optimized_outP5value
         /home/tdevries/gdb/src/gdb/value.c:1500
0x770b2f _Z30frame_unwind_register_unsigned14frame_info_ptri
         /home/tdevries/gdb/src/gdb/frame.c:1337
0x42bac3 aarch64_dwarf2_prev_register
         /home/tdevries/gdb/src/gdb/aarch64-tdep.c:1280
0x680ec3 dwarf2_frame_prev_register
         /home/tdevries/gdb/src/gdb/dwarf2/frame.c:1217
0x770587 _Z27frame_unwind_register_value14frame_info_ptri
         /home/tdevries/gdb/src/gdb/frame.c:1244
0x770ad3 _Z30frame_unwind_register_unsigned14frame_info_ptri
         /home/tdevries/gdb/src/gdb/frame.c:1333
0x76c30f _Z17default_unwind_pcP7gdbarch14frame_info_ptr
         /home/tdevries/gdb/src/gdb/frame-unwind.c:244
0x4b652b _Z17gdbarch_unwind_pcP7gdbarch14frame_info_ptr
         /home/tdevries/gdb/src/gdb/gdbarch.c:2959
0x67d363 _Z29dwarf2_tailcall_sniffer_first14frame_info_ptrPPvPKl
         /home/tdevries/gdb/src/gdb/dwarf2/frame-tailcall.c:390
0x680593 dwarf2_frame_cache
         /home/tdevries/gdb/src/gdb/dwarf2/frame.c:1089
0x68070f dwarf2_frame_unwind_stop_reason
         /home/tdevries/gdb/src/gdb/dwarf2/frame.c:1101
0x773503 get_prev_frame_always_1
         /home/tdevries/gdb/src/gdb/frame.c:2281
0x773beb _Z21get_prev_frame_always14frame_info_ptr
         /home/tdevries/gdb/src/gdb/frame.c:2376
0x775d6b _Z28get_frame_unwind_stop_reason14frame_info_ptr
         /home/tdevries/gdb/src/gdb/frame.c:3051
0x68151f _Z16dwarf2_frame_cfa14frame_info_ptr
         /home/tdevries/gdb/src/gdb/dwarf2/frame.c:1356
0x678b13 _ZN18dwarf_expr_context16execute_stack_opEPKhS1_
         /home/tdevries/gdb/src/gdb/dwarf2/expr.c:2110
0x676f4f _ZN18dwarf_expr_context4evalEPKhm
         /home/tdevries/gdb/src/gdb/dwarf2/expr.c:1239
0x677faf _ZN18dwarf_expr_context16execute_stack_opEPKhS1_
         /home/tdevries/gdb/src/gdb/dwarf2/expr.c:1811
0x676f4f _ZN18dwarf_expr_context4evalEPKhm
         /home/tdevries/gdb/src/gdb/dwarf2/expr.c:1239
0x6769e3 
_ZN18dwarf_expr_context8evaluateEPKhmbP18dwarf2_per_cu_data14frame_info_ptrPK18property_addr_infoP4typeS9_l
         /home/tdevries/gdb/src/gdb/dwarf2/expr.c:1078
0x6a924b dwarf2_evaluate_loc_desc_full
         /home/tdevries/gdb/src/gdb/dwarf2/loc.c:1513
0x6a941f 
_Z24dwarf2_evaluate_loc_descP4type14frame_info_ptrPKhmP18dwarf2_per_cu_dataP18dwarf2_per_objfileb
         /home/tdevries/gdb/src/gdb/dwarf2/loc.c:1558
0x6abb83 locexpr_read_variable
         /home/tdevries/gdb/src/gdb/dwarf2/loc.c:3052
0x769083 
_ZNK13language_defn14read_var_valueEP6symbolPK5block14frame_info_ptr
         /home/tdevries/gdb/src/gdb/findvar.c:578
0x769a17 _Z14read_var_valueP6symbolPK5block14frame_info_ptr
         /home/tdevries/gdb/src/gdb/findvar.c:794
0xa188ef 
_Z14read_frame_argRK19frame_print_optionsP6symbol14frame_info_ptrP9frame_argS6_
         /home/tdevries/gdb/src/gdb/stack.c:540
0xa19557 print_frame_args
         /home/tdevries/gdb/src/gdb/stack.c:888
0xa1ae4b print_frame
         /home/tdevries/gdb/src/gdb/stack.c:1390
0xa1a0e3 
_Z16print_frame_infoRK19frame_print_options14frame_info_ptri10print_whatii
         /home/tdevries/gdb/src/gdb/stack.c:1116
0xa180c7 _Z17print_stack_frame14frame_info_ptri10print_whati
         /home/tdevries/gdb/src/gdb/stack.c:367
0xa1801b 
_Z26print_stack_frame_to_uioutP6ui_out14frame_info_ptri10print_whati
         /home/tdevries/gdb/src/gdb/stack.c:346
0xa9d44f 
_Z27print_selected_thread_frameP6ui_out10enum_flagsI23user_selected_what_flagE
         /home/tdevries/gdb/src/gdb/thread.c:1994
0xa1c907 frame_command_core
         /home/tdevries/gdb/src/gdb/stack.c:1857
0xa20ec7 base_command
         /home/tdevries/gdb/src/gdb/stack.c:1962
0x5b8f9f do_simple_func
         /home/tdevries/gdb/src/gdb/cli/cli-decode.c:95
0x5be36b _Z8cmd_funcP16cmd_list_elementPKci
         /home/tdevries/gdb/src/gdb/cli/cli-decode.c:2737
0xaa41ef _Z15execute_commandPKci
         /home/tdevries/gdb/src/gdb/top.c:688
0x7421cf _Z15command_handlerPKc
         /home/tdevries/gdb/src/gdb/event-top.c:616
0x74272f _Z20command_line_handlerOSt10unique_ptrIcN3gdb13xfree_deleterIcEEE
         /home/tdevries/gdb/src/gdb/event-top.c:852
0xacc9c7 tui_command_line_handler
         /home/tdevries/gdb/src/gdb/tui/tui-interp.c:104
0x74184f gdb_rl_callback_handler
         /home/tdevries/gdb/src/gdb/event-top.c:246
0xffff957ba97f ???
0x74169b gdb_rl_callback_read_char_wrapper_noexcept
         /home/tdevries/gdb/src/gdb/event-top.c:188
0x741733 gdb_rl_callback_read_char_wrapper
         /home/tdevries/gdb/src/gdb/event-top.c:221
0x741fa3 stdin_event_handler
         /home/tdevries/gdb/src/gdb/event-top.c:541
0xdba083 handle_file_event
         /home/tdevries/gdb/src/gdbsupport/event-loop.cc:573
0xdba537 gdb_wait_for_event
         /home/tdevries/gdb/src/gdbsupport/event-loop.cc:694
0xdb91cb _Z16gdb_do_one_eventi
         /home/tdevries/gdb/src/gdbsupport/event-loop.cc:264
0x868d9b start_event_loop
         /home/tdevries/gdb/src/gdb/main.c:411
0x868ee7 captured_command_loop
         /home/tdevries/gdb/src/gdb/main.c:471
0x86a807 captured_main
         /home/tdevries/gdb/src/gdb/main.c:1310
0x86a86f _Z8gdb_mainP18captured_main_args
         /home/tdevries/gdb/src/gdb/main.c:1325
0x418597 main
         /home/tdevries/gdb/src/gdb/gdb.c:32
...

Thanks,
- Tom

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH v2 11/13] gdb: make user-created frames reinflatable
  2023-01-23 12:57   ` Tom de Vries
@ 2023-01-23 14:34     ` Luis Machado
  2023-01-24  3:55       ` Simon Marchi
  0 siblings, 1 reply; 28+ messages in thread
From: Luis Machado @ 2023-01-23 14:34 UTC (permalink / raw)
  To: Tom de Vries, Simon Marchi, gdb-patches

On 1/23/23 12:57, Tom de Vries wrote:
> On 12/14/22 04:34, Simon Marchi via Gdb-patches wrote:
>>   gdb/testsuite/gdb.base/frame-view.exp    | 47 ++++++++++++--
> 
> Hi,
> 
> on aarch64-linux I get:
> ...
> FAIL: gdb.base/frame-view.exp: with_pretty_printer=true: frame (GDB internal error)
> FAIL: gdb.base/frame-view.exp: with_pretty_printer=true: frame again (GDB internal error)


I can reproduce this as well. It also happens on arm-linux Ubuntu 22.04/20.04, with a similar kind of backtrace.

> ...
> 
> In more detail:
> ...
> (gdb) PASS: gdb.base/frame-view.exp: with_pretty_printer=true: set scheduler-locking on
> frame^M
> #0  baz (z1=hahaha, /home/tdevries/gdb/src/gdb/value.c:4056: internal-error: value_fetch_lazy_register: Assertion `next_frame != NULL' failed.^M
> ...
> 
> 
> Back-trace:
> ...
> ----- Backtrace -----
> Resyncing due to internal error.
> 0x563033 gdb_internal_backtrace_1
>          /home/tdevries/gdb/src/gdb/bt-utils.c:122
> 0x5630f3 _Z22gdb_internal_backtracev
>          /home/tdevries/gdb/src/gdb/bt-utils.c:168
> 0xb4461f internal_vproblem
>          /home/tdevries/gdb/src/gdb/utils.c:396
> 0xb44a33 _Z15internal_verrorPKciS0_St9__va_list
>          /home/tdevries/gdb/src/gdb/utils.c:476
> 0xdb8e97 _Z18internal_error_locPKciS0_z
>          /home/tdevries/gdb/src/gdbsupport/errors.cc:58
> 0xb7a1d7 value_fetch_lazy_register
>          /home/tdevries/gdb/src/gdb/value.c:4056
> 0xb7a88b _Z16value_fetch_lazyP5value
>          /home/tdevries/gdb/src/gdb/value.c:4180
> 0xb74c93 _Z19value_optimized_outP5value
>          /home/tdevries/gdb/src/gdb/value.c:1500
> 0x770b2f _Z30frame_unwind_register_unsigned14frame_info_ptri
>          /home/tdevries/gdb/src/gdb/frame.c:1337
> 0x42bac3 aarch64_dwarf2_prev_register
>          /home/tdevries/gdb/src/gdb/aarch64-tdep.c:1280
> 0x680ec3 dwarf2_frame_prev_register
>          /home/tdevries/gdb/src/gdb/dwarf2/frame.c:1217
> 0x770587 _Z27frame_unwind_register_value14frame_info_ptri
>          /home/tdevries/gdb/src/gdb/frame.c:1244
> 0x770ad3 _Z30frame_unwind_register_unsigned14frame_info_ptri
>          /home/tdevries/gdb/src/gdb/frame.c:1333
> 0x76c30f _Z17default_unwind_pcP7gdbarch14frame_info_ptr
>          /home/tdevries/gdb/src/gdb/frame-unwind.c:244
> 0x4b652b _Z17gdbarch_unwind_pcP7gdbarch14frame_info_ptr
>          /home/tdevries/gdb/src/gdb/gdbarch.c:2959
> 0x67d363 _Z29dwarf2_tailcall_sniffer_first14frame_info_ptrPPvPKl
>          /home/tdevries/gdb/src/gdb/dwarf2/frame-tailcall.c:390
> 0x680593 dwarf2_frame_cache
>          /home/tdevries/gdb/src/gdb/dwarf2/frame.c:1089
> 0x68070f dwarf2_frame_unwind_stop_reason
>          /home/tdevries/gdb/src/gdb/dwarf2/frame.c:1101
> 0x773503 get_prev_frame_always_1
>          /home/tdevries/gdb/src/gdb/frame.c:2281
> 0x773beb _Z21get_prev_frame_always14frame_info_ptr
>          /home/tdevries/gdb/src/gdb/frame.c:2376
> 0x775d6b _Z28get_frame_unwind_stop_reason14frame_info_ptr
>          /home/tdevries/gdb/src/gdb/frame.c:3051
> 0x68151f _Z16dwarf2_frame_cfa14frame_info_ptr
>          /home/tdevries/gdb/src/gdb/dwarf2/frame.c:1356
> 0x678b13 _ZN18dwarf_expr_context16execute_stack_opEPKhS1_
>          /home/tdevries/gdb/src/gdb/dwarf2/expr.c:2110
> 0x676f4f _ZN18dwarf_expr_context4evalEPKhm
>          /home/tdevries/gdb/src/gdb/dwarf2/expr.c:1239
> 0x677faf _ZN18dwarf_expr_context16execute_stack_opEPKhS1_
>          /home/tdevries/gdb/src/gdb/dwarf2/expr.c:1811
> 0x676f4f _ZN18dwarf_expr_context4evalEPKhm
>          /home/tdevries/gdb/src/gdb/dwarf2/expr.c:1239
> 0x6769e3 _ZN18dwarf_expr_context8evaluateEPKhmbP18dwarf2_per_cu_data14frame_info_ptrPK18property_addr_infoP4typeS9_l
>          /home/tdevries/gdb/src/gdb/dwarf2/expr.c:1078
> 0x6a924b dwarf2_evaluate_loc_desc_full
>          /home/tdevries/gdb/src/gdb/dwarf2/loc.c:1513
> 0x6a941f _Z24dwarf2_evaluate_loc_descP4type14frame_info_ptrPKhmP18dwarf2_per_cu_dataP18dwarf2_per_objfileb
>          /home/tdevries/gdb/src/gdb/dwarf2/loc.c:1558
> 0x6abb83 locexpr_read_variable
>          /home/tdevries/gdb/src/gdb/dwarf2/loc.c:3052
> 0x769083 _ZNK13language_defn14read_var_valueEP6symbolPK5block14frame_info_ptr
>          /home/tdevries/gdb/src/gdb/findvar.c:578
> 0x769a17 _Z14read_var_valueP6symbolPK5block14frame_info_ptr
>          /home/tdevries/gdb/src/gdb/findvar.c:794
> 0xa188ef _Z14read_frame_argRK19frame_print_optionsP6symbol14frame_info_ptrP9frame_argS6_
>          /home/tdevries/gdb/src/gdb/stack.c:540
> 0xa19557 print_frame_args
>          /home/tdevries/gdb/src/gdb/stack.c:888
> 0xa1ae4b print_frame
>          /home/tdevries/gdb/src/gdb/stack.c:1390
> 0xa1a0e3 _Z16print_frame_infoRK19frame_print_options14frame_info_ptri10print_whatii
>          /home/tdevries/gdb/src/gdb/stack.c:1116
> 0xa180c7 _Z17print_stack_frame14frame_info_ptri10print_whati
>          /home/tdevries/gdb/src/gdb/stack.c:367
> 0xa1801b _Z26print_stack_frame_to_uioutP6ui_out14frame_info_ptri10print_whati
>          /home/tdevries/gdb/src/gdb/stack.c:346
> 0xa9d44f _Z27print_selected_thread_frameP6ui_out10enum_flagsI23user_selected_what_flagE
>          /home/tdevries/gdb/src/gdb/thread.c:1994
> 0xa1c907 frame_command_core
>          /home/tdevries/gdb/src/gdb/stack.c:1857
> 0xa20ec7 base_command
>          /home/tdevries/gdb/src/gdb/stack.c:1962
> 0x5b8f9f do_simple_func
>          /home/tdevries/gdb/src/gdb/cli/cli-decode.c:95
> 0x5be36b _Z8cmd_funcP16cmd_list_elementPKci
>          /home/tdevries/gdb/src/gdb/cli/cli-decode.c:2737
> 0xaa41ef _Z15execute_commandPKci
>          /home/tdevries/gdb/src/gdb/top.c:688
> 0x7421cf _Z15command_handlerPKc
>          /home/tdevries/gdb/src/gdb/event-top.c:616
> 0x74272f _Z20command_line_handlerOSt10unique_ptrIcN3gdb13xfree_deleterIcEEE
>          /home/tdevries/gdb/src/gdb/event-top.c:852
> 0xacc9c7 tui_command_line_handler
>          /home/tdevries/gdb/src/gdb/tui/tui-interp.c:104
> 0x74184f gdb_rl_callback_handler
>          /home/tdevries/gdb/src/gdb/event-top.c:246
> 0xffff957ba97f ???
> 0x74169b gdb_rl_callback_read_char_wrapper_noexcept
>          /home/tdevries/gdb/src/gdb/event-top.c:188
> 0x741733 gdb_rl_callback_read_char_wrapper
>          /home/tdevries/gdb/src/gdb/event-top.c:221
> 0x741fa3 stdin_event_handler
>          /home/tdevries/gdb/src/gdb/event-top.c:541
> 0xdba083 handle_file_event
>          /home/tdevries/gdb/src/gdbsupport/event-loop.cc:573
> 0xdba537 gdb_wait_for_event
>          /home/tdevries/gdb/src/gdbsupport/event-loop.cc:694
> 0xdb91cb _Z16gdb_do_one_eventi
>          /home/tdevries/gdb/src/gdbsupport/event-loop.cc:264
> 0x868d9b start_event_loop
>          /home/tdevries/gdb/src/gdb/main.c:411
> 0x868ee7 captured_command_loop
>          /home/tdevries/gdb/src/gdb/main.c:471
> 0x86a807 captured_main
>          /home/tdevries/gdb/src/gdb/main.c:1310
> 0x86a86f _Z8gdb_mainP18captured_main_args
>          /home/tdevries/gdb/src/gdb/main.c:1325
> 0x418597 main
>          /home/tdevries/gdb/src/gdb/gdb.c:32
> ...
> 
> Thanks,
> - Tom


^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH v2 11/13] gdb: make user-created frames reinflatable
  2023-01-23 14:34     ` Luis Machado
@ 2023-01-24  3:55       ` Simon Marchi
  2023-01-24  8:22         ` Luis Machado
  0 siblings, 1 reply; 28+ messages in thread
From: Simon Marchi @ 2023-01-24  3:55 UTC (permalink / raw)
  To: Luis Machado, Tom de Vries, gdb-patches



On 1/23/23 09:34, Luis Machado wrote:
> On 1/23/23 12:57, Tom de Vries wrote:
>> On 12/14/22 04:34, Simon Marchi via Gdb-patches wrote:
>>>   gdb/testsuite/gdb.base/frame-view.exp    | 47 ++++++++++++--
>>
>> Hi,
>>
>> on aarch64-linux I get:
>> ...
>> FAIL: gdb.base/frame-view.exp: with_pretty_printer=true: frame (GDB internal error)
>> FAIL: gdb.base/frame-view.exp: with_pretty_printer=true: frame again (GDB internal error)
> 
> 
> I can reproduce this as well. It also happens on arm-linux Ubuntu 22.04/20.04, with a similar kind of backtrace.

I can reproduce too, I have a potential fix.  I'll try to send it
tomorrow.

Simon

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH v2 11/13] gdb: make user-created frames reinflatable
  2023-01-24  3:55       ` Simon Marchi
@ 2023-01-24  8:22         ` Luis Machado
  2023-01-25  3:45           ` Simon Marchi
  0 siblings, 1 reply; 28+ messages in thread
From: Luis Machado @ 2023-01-24  8:22 UTC (permalink / raw)
  To: Simon Marchi, Tom de Vries, gdb-patches

On 1/24/23 03:55, Simon Marchi wrote:
> 
> 
> On 1/23/23 09:34, Luis Machado wrote:
>> On 1/23/23 12:57, Tom de Vries wrote:
>>> On 12/14/22 04:34, Simon Marchi via Gdb-patches wrote:
>>>>    gdb/testsuite/gdb.base/frame-view.exp    | 47 ++++++++++++--
>>>
>>> Hi,
>>>
>>> on aarch64-linux I get:
>>> ...
>>> FAIL: gdb.base/frame-view.exp: with_pretty_printer=true: frame (GDB internal error)
>>> FAIL: gdb.base/frame-view.exp: with_pretty_printer=true: frame again (GDB internal error)
>>
>>
>> I can reproduce this as well. It also happens on arm-linux Ubuntu 22.04/20.04, with a similar kind of backtrace.
> 
> I can reproduce too, I have a potential fix.  I'll try to send it
> tomorrow.

Let me know if I can help with testing it on one of the targets.

> 
> Simon


^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH v2 11/13] gdb: make user-created frames reinflatable
  2023-01-24  8:22         ` Luis Machado
@ 2023-01-25  3:45           ` Simon Marchi
  2023-01-30  8:49             ` Luis Machado
  0 siblings, 1 reply; 28+ messages in thread
From: Simon Marchi @ 2023-01-25  3:45 UTC (permalink / raw)
  To: Luis Machado, Tom de Vries, gdb-patches



On 1/24/23 03:22, Luis Machado wrote:
> On 1/24/23 03:55, Simon Marchi wrote:
>>
>>
>> On 1/23/23 09:34, Luis Machado wrote:
>>> On 1/23/23 12:57, Tom de Vries wrote:
>>>> On 12/14/22 04:34, Simon Marchi via Gdb-patches wrote:
>>>>>    gdb/testsuite/gdb.base/frame-view.exp    | 47 ++++++++++++--
>>>>
>>>> Hi,
>>>>
>>>> on aarch64-linux I get:
>>>> ...
>>>> FAIL: gdb.base/frame-view.exp: with_pretty_printer=true: frame (GDB internal error)
>>>> FAIL: gdb.base/frame-view.exp: with_pretty_printer=true: frame again (GDB internal error)
>>>
>>>
>>> I can reproduce this as well. It also happens on arm-linux Ubuntu 22.04/20.04, with a similar kind of backtrace.
>>
>> I can reproduce too, I have a potential fix.  I'll try to send it
>> tomorrow.
> 
> Let me know if I can help with testing it on one of the targets.

Ok, so I have a patch that fixes the test, but I'm not sure it's really
good.

First, here's my analysis of what I think is happening.

 - When we create the user frame (the "select-frame view" command), we
   create a sentinel frame just for our user-created frame, in
   create_new_frame.  This sentinel frame has the same id as the regular
   sentinel frame.
 - When printing the frame, after doing the "select-frame view"
   command, the argument's pretty printer is invoked, which does an
   inferior function call (this is the point of the test).  This clears
   the frame cache, including the "real" sentinel frame, which sets the
   sentinel_frame global to nullptr.
 - Later in the frame-printing process (when printing the second
   argument), the auto-reinflation mechanism (which would have been the
   manual reinflate call before, doesn't matter here) re-creates the
   user frame by calling create_new_frame again, creating its own
   special sentinel frame again.  However, note that the "real" sentinel
   frame, the sentinel_frame global, is still nullptr.  If the selected
   frame had been a regular frame, we would have called
   get_current_frame at some point during the reinflation, which would
   have re-created the "real" sentinel frame.  But it's not the case
   here.
 - Deep down the stack, something wants to fill in the unwind stop
   reason for frame 0, which requires trying to unwind frame 1.  This
   leads us to trying to unwind the PC of frame 1:

     #0  gdbarch_unwind_pc (gdbarch=0xffff8d010080, next_frame=...) at /home/simark/src/binutils-gdb/gdb/gdbarch.c:2955
     #1  0x000000000134569c in dwarf2_tailcall_sniffer_first (this_frame=..., tailcall_cachep=0xffff773fcae0, entry_cfa_sp_offsetp=0xfffff7f7d450)
         at /home/simark/src/binutils-gdb/gdb/dwarf2/frame-tailcall.c:390
     #2  0x0000000001355d84 in dwarf2_frame_cache (this_frame=..., this_cache=0xffff773fc928) at /home/simark/src/binutils-gdb/gdb/dwarf2/frame.c:1089
     #3  0x00000000013562b0 in dwarf2_frame_unwind_stop_reason (this_frame=..., this_cache=0xffff773fc928) at /home/simark/src/binutils-gdb/gdb/dwarf2/frame.c:1101
     #4  0x0000000001990f64 in get_prev_frame_always_1 (this_frame=...) at /home/simark/src/binutils-gdb/gdb/frame.c:2281
     #5  0x0000000001993034 in get_prev_frame_always (this_frame=...) at /home/simark/src/binutils-gdb/gdb/frame.c:2376
     #6  0x000000000199b814 in get_frame_unwind_stop_reason (frame=...) at /home/simark/src/binutils-gdb/gdb/frame.c:3051
     #7  0x0000000001359cd8 in dwarf2_frame_cfa (this_frame=...) at /home/simark/src/binutils-gdb/gdb/dwarf2/frame.c:1356
     #8  0x000000000132122c in dwarf_expr_context::execute_stack_op (this=0xfffff7f80170, op_ptr=0xffff8d8883ee "\217\002", op_end=0xffff8d8883ee "\217\002")
         at /home/simark/src/binutils-gdb/gdb/dwarf2/expr.c:2110
     #9  0x0000000001317b30 in dwarf_expr_context::eval (this=0xfffff7f80170, addr=0xffff8d8883ed "\234\217\002", len=1) at /home/simark/src/binutils-gdb/gdb/dwarf2/expr.c:1239
     #10 0x000000000131d68c in dwarf_expr_context::execute_stack_op (this=0xfffff7f80170, op_ptr=0xffff8d88840e "", op_end=0xffff8d88840e "") at /home/simark/src/binutils-gdb/gdb/dwarf2/expr.c:1811
     #11 0x0000000001317b30 in dwarf_expr_context::eval (this=0xfffff7f80170, addr=0xffff8d88840c "\221p", len=2) at /home/simark/src/binutils-gdb/gdb/dwarf2/expr.c:1239
     #12 0x0000000001314c3c in dwarf_expr_context::evaluate (this=0xfffff7f80170, addr=0xffff8d88840c "\221p", len=2, as_lval=true, per_cu=0xffff90b03700, frame=..., addr_info=0x0, 
         type=0xffff8f6c8400, subobj_type=0xffff8f6c8400, subobj_offset=0) at /home/simark/src/binutils-gdb/gdb/dwarf2/expr.c:1078
     #13 0x000000000149f9e0 in dwarf2_evaluate_loc_desc_full (type=0xffff8f6c8400, frame=..., data=0xffff8d88840c "\221p", size=2, per_cu=0xffff90b03700, per_objfile=0xffff9070b980, 
         subobj_type=0xffff8f6c8400, subobj_byte_offset=0, as_lval=true) at /home/simark/src/binutils-gdb/gdb/dwarf2/loc.c:1513
     #14 0x00000000014a0100 in dwarf2_evaluate_loc_desc (type=0xffff8f6c8400, frame=..., data=0xffff8d88840c "\221p", size=2, per_cu=0xffff90b03700, per_objfile=0xffff9070b980, as_lval=true)
         at /home/simark/src/binutils-gdb/gdb/dwarf2/loc.c:1557
     #15 0x00000000014aa584 in locexpr_read_variable (symbol=0xffff8f6cd770, frame=...) at /home/simark/src/binutils-gdb/gdb/dwarf2/loc.c:3052
 - AArch64 defines a special "prev register" function,
   aarch64_dwarf2_prev_register, to handle unwinding the PC.  This
   function does

     frame_unwind_register_unsigned (this_frame, AARCH64_LR_REGNUM);

   (I must admit that I don't really understand this part, it seems to
    me like you would want to unwind LR's value from this_frame->next...
    but anyway.)
 - frame_unwind_register_unsigned ultimately creates a lazy register
   value, saving the frame id of this_frame->next.  this_frame is the
   user-created frame, to this_frame->next is the special sentinel frame
   we created for it.
 - When time comes to un-lazify the value, value_fetch_lazy_register
   calls frame_find_by_id, to find the frame with the id we saved.
 - frame_find_by_id sees it's the sentinel frame id, so returns the
   sentinel_frame global, which is, if you remember, nullptr.
 - We hit the `gdb_assert (next_frame != NULL)` assertion.

I have a hard time pinpointing a single thing that is wrong.  The fact
that the user-created frames have their own sentinel frame, which share
the same id as the real sentinel frame, that sounds like it can't work
with frame_find_by_id.  In the case without the pretty printer, where
things appear to work, it means that frame_find_by_id returns the "real"
sentinel frame, part of the "real" stack frame chain, and not the
sentinel that is attached to the user-created frame.  Things still kinda
work in the end because those two sentinel frames behave the same, as
far as "prev register" is concerned.

Ultimately, I don't really understand how this "frame view" feature is
useful.  Sure, we pretend that the frame's sp and pc is what the user
gave, but what about the other registers?  It seems to me like you are
very likely to get bogus values.

The patch I had in mind was to give user-created frames a special
unwinder, rather than letting other unwinders (the arch-specific ones,
the dwarf2 one, etc) kick in.  That unwinder would always return the
unwind stop reason UNWIND_OUTERMOST, which effectively makes the
user-created frame an outer frame, and we would never try to unwind any
register value from it.  It makes the test pass, because it breaks the
chain of event explained above.  But it's probably just papering over
the actual problem.

Simon

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH v2 11/13] gdb: make user-created frames reinflatable
  2023-01-25  3:45           ` Simon Marchi
@ 2023-01-30  8:49             ` Luis Machado
  2023-01-30 16:20               ` Simon Marchi
  0 siblings, 1 reply; 28+ messages in thread
From: Luis Machado @ 2023-01-30  8:49 UTC (permalink / raw)
  To: Simon Marchi, Tom de Vries, gdb-patches

On 1/25/23 03:45, Simon Marchi wrote:
> 
> 
> On 1/24/23 03:22, Luis Machado wrote:
>> On 1/24/23 03:55, Simon Marchi wrote:
>>>
>>>
>>> On 1/23/23 09:34, Luis Machado wrote:
>>>> On 1/23/23 12:57, Tom de Vries wrote:
>>>>> On 12/14/22 04:34, Simon Marchi via Gdb-patches wrote:
>>>>>>     gdb/testsuite/gdb.base/frame-view.exp    | 47 ++++++++++++--
>>>>>
>>>>> Hi,
>>>>>
>>>>> on aarch64-linux I get:
>>>>> ...
>>>>> FAIL: gdb.base/frame-view.exp: with_pretty_printer=true: frame (GDB internal error)
>>>>> FAIL: gdb.base/frame-view.exp: with_pretty_printer=true: frame again (GDB internal error)
>>>>
>>>>
>>>> I can reproduce this as well. It also happens on arm-linux Ubuntu 22.04/20.04, with a similar kind of backtrace.
>>>
>>> I can reproduce too, I have a potential fix.  I'll try to send it
>>> tomorrow.
>>
>> Let me know if I can help with testing it on one of the targets.
> 
> Ok, so I have a patch that fixes the test, but I'm not sure it's really
> good.
> 
> First, here's my analysis of what I think is happening.
> 
>   - When we create the user frame (the "select-frame view" command), we
>     create a sentinel frame just for our user-created frame, in
>     create_new_frame.  This sentinel frame has the same id as the regular
>     sentinel frame.
>   - When printing the frame, after doing the "select-frame view"
>     command, the argument's pretty printer is invoked, which does an
>     inferior function call (this is the point of the test).  This clears
>     the frame cache, including the "real" sentinel frame, which sets the
>     sentinel_frame global to nullptr.
>   - Later in the frame-printing process (when printing the second
>     argument), the auto-reinflation mechanism (which would have been the
>     manual reinflate call before, doesn't matter here) re-creates the
>     user frame by calling create_new_frame again, creating its own
>     special sentinel frame again.  However, note that the "real" sentinel
>     frame, the sentinel_frame global, is still nullptr.  If the selected
>     frame had been a regular frame, we would have called
>     get_current_frame at some point during the reinflation, which would
>     have re-created the "real" sentinel frame.  But it's not the case
>     here.
>   - Deep down the stack, something wants to fill in the unwind stop
>     reason for frame 0, which requires trying to unwind frame 1.  This
>     leads us to trying to unwind the PC of frame 1:
> 
>       #0  gdbarch_unwind_pc (gdbarch=0xffff8d010080, next_frame=...) at /home/simark/src/binutils-gdb/gdb/gdbarch.c:2955
>       #1  0x000000000134569c in dwarf2_tailcall_sniffer_first (this_frame=..., tailcall_cachep=0xffff773fcae0, entry_cfa_sp_offsetp=0xfffff7f7d450)
>           at /home/simark/src/binutils-gdb/gdb/dwarf2/frame-tailcall.c:390
>       #2  0x0000000001355d84 in dwarf2_frame_cache (this_frame=..., this_cache=0xffff773fc928) at /home/simark/src/binutils-gdb/gdb/dwarf2/frame.c:1089
>       #3  0x00000000013562b0 in dwarf2_frame_unwind_stop_reason (this_frame=..., this_cache=0xffff773fc928) at /home/simark/src/binutils-gdb/gdb/dwarf2/frame.c:1101
>       #4  0x0000000001990f64 in get_prev_frame_always_1 (this_frame=...) at /home/simark/src/binutils-gdb/gdb/frame.c:2281
>       #5  0x0000000001993034 in get_prev_frame_always (this_frame=...) at /home/simark/src/binutils-gdb/gdb/frame.c:2376
>       #6  0x000000000199b814 in get_frame_unwind_stop_reason (frame=...) at /home/simark/src/binutils-gdb/gdb/frame.c:3051
>       #7  0x0000000001359cd8 in dwarf2_frame_cfa (this_frame=...) at /home/simark/src/binutils-gdb/gdb/dwarf2/frame.c:1356
>       #8  0x000000000132122c in dwarf_expr_context::execute_stack_op (this=0xfffff7f80170, op_ptr=0xffff8d8883ee "\217\002", op_end=0xffff8d8883ee "\217\002")
>           at /home/simark/src/binutils-gdb/gdb/dwarf2/expr.c:2110
>       #9  0x0000000001317b30 in dwarf_expr_context::eval (this=0xfffff7f80170, addr=0xffff8d8883ed "\234\217\002", len=1) at /home/simark/src/binutils-gdb/gdb/dwarf2/expr.c:1239
>       #10 0x000000000131d68c in dwarf_expr_context::execute_stack_op (this=0xfffff7f80170, op_ptr=0xffff8d88840e "", op_end=0xffff8d88840e "") at /home/simark/src/binutils-gdb/gdb/dwarf2/expr.c:1811
>       #11 0x0000000001317b30 in dwarf_expr_context::eval (this=0xfffff7f80170, addr=0xffff8d88840c "\221p", len=2) at /home/simark/src/binutils-gdb/gdb/dwarf2/expr.c:1239
>       #12 0x0000000001314c3c in dwarf_expr_context::evaluate (this=0xfffff7f80170, addr=0xffff8d88840c "\221p", len=2, as_lval=true, per_cu=0xffff90b03700, frame=..., addr_info=0x0,
>           type=0xffff8f6c8400, subobj_type=0xffff8f6c8400, subobj_offset=0) at /home/simark/src/binutils-gdb/gdb/dwarf2/expr.c:1078
>       #13 0x000000000149f9e0 in dwarf2_evaluate_loc_desc_full (type=0xffff8f6c8400, frame=..., data=0xffff8d88840c "\221p", size=2, per_cu=0xffff90b03700, per_objfile=0xffff9070b980,
>           subobj_type=0xffff8f6c8400, subobj_byte_offset=0, as_lval=true) at /home/simark/src/binutils-gdb/gdb/dwarf2/loc.c:1513
>       #14 0x00000000014a0100 in dwarf2_evaluate_loc_desc (type=0xffff8f6c8400, frame=..., data=0xffff8d88840c "\221p", size=2, per_cu=0xffff90b03700, per_objfile=0xffff9070b980, as_lval=true)
>           at /home/simark/src/binutils-gdb/gdb/dwarf2/loc.c:1557
>       #15 0x00000000014aa584 in locexpr_read_variable (symbol=0xffff8f6cd770, frame=...) at /home/simark/src/binutils-gdb/gdb/dwarf2/loc.c:3052
>   - AArch64 defines a special "prev register" function,
>     aarch64_dwarf2_prev_register, to handle unwinding the PC.  This
>     function does
> 
>       frame_unwind_register_unsigned (this_frame, AARCH64_LR_REGNUM);
> 
>     (I must admit that I don't really understand this part, it seems to
>      me like you would want to unwind LR's value from this_frame->next...
>      but anyway.)

It is somewhat historical, and it is something I'm planning to address eventually. Right now, though, we hardwire the PC to
obtaining LR.

>   - frame_unwind_register_unsigned ultimately creates a lazy register
>     value, saving the frame id of this_frame->next.  this_frame is the
>     user-created frame, to this_frame->next is the special sentinel frame
>     we created for it.
>   - When time comes to un-lazify the value, value_fetch_lazy_register
>     calls frame_find_by_id, to find the frame with the id we saved.
>   - frame_find_by_id sees it's the sentinel frame id, so returns the
>     sentinel_frame global, which is, if you remember, nullptr.
>   - We hit the `gdb_assert (next_frame != NULL)` assertion.
> 
> I have a hard time pinpointing a single thing that is wrong.  The fact
> that the user-created frames have their own sentinel frame, which share
> the same id as the real sentinel frame, that sounds like it can't work

 From your (very detailed) explanation, it does sound a bit iffy to abuse the frame id mechanism and use
the same id as the existing real frame.

> with frame_find_by_id.  In the case without the pretty printer, where
> things appear to work, it means that frame_find_by_id returns the "real"
> sentinel frame, part of the "real" stack frame chain, and not the
> sentinel that is attached to the user-created frame.  Things still kinda
> work in the end because those two sentinel frames behave the same, as
> far as "prev register" is concerned.
> 
> Ultimately, I don't really understand how this "frame view" feature is
> useful.  Sure, we pretend that the frame's sp and pc is what the user
> gave, but what about the other registers?  It seems to me like you are
> very likely to get bogus values.
> 
> The patch I had in mind was to give user-created frames a special
> unwinder, rather than letting other unwinders (the arch-specific ones,
> the dwarf2 one, etc) kick in.  That unwinder would always return the
> unwind stop reason UNWIND_OUTERMOST, which effectively makes the
> user-created frame an outer frame, and we would never try to unwind any
> register value from it.  It makes the test pass, because it breaks the
> chain of event explained above.  But it's probably just papering over
> the actual problem.
> 
> Simon


^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH v2 11/13] gdb: make user-created frames reinflatable
  2023-01-30  8:49             ` Luis Machado
@ 2023-01-30 16:20               ` Simon Marchi
  0 siblings, 0 replies; 28+ messages in thread
From: Simon Marchi @ 2023-01-30 16:20 UTC (permalink / raw)
  To: Luis Machado, Tom de Vries, gdb-patches

On 1/30/23 03:49, Luis Machado wrote:
> On 1/25/23 03:45, Simon Marchi wrote:
>>
>>
>> On 1/24/23 03:22, Luis Machado wrote:
>>> On 1/24/23 03:55, Simon Marchi wrote:
>>>>
>>>>
>>>> On 1/23/23 09:34, Luis Machado wrote:
>>>>> On 1/23/23 12:57, Tom de Vries wrote:
>>>>>> On 12/14/22 04:34, Simon Marchi via Gdb-patches wrote:
>>>>>>>     gdb/testsuite/gdb.base/frame-view.exp    | 47 ++++++++++++--
>>>>>>
>>>>>> Hi,
>>>>>>
>>>>>> on aarch64-linux I get:
>>>>>> ...
>>>>>> FAIL: gdb.base/frame-view.exp: with_pretty_printer=true: frame (GDB internal error)
>>>>>> FAIL: gdb.base/frame-view.exp: with_pretty_printer=true: frame again (GDB internal error)
>>>>>
>>>>>
>>>>> I can reproduce this as well. It also happens on arm-linux Ubuntu 22.04/20.04, with a similar kind of backtrace.
>>>>
>>>> I can reproduce too, I have a potential fix.  I'll try to send it
>>>> tomorrow.
>>>
>>> Let me know if I can help with testing it on one of the targets.
>>
>> Ok, so I have a patch that fixes the test, but I'm not sure it's really
>> good.
>>
>> First, here's my analysis of what I think is happening.
>>
>>   - When we create the user frame (the "select-frame view" command), we
>>     create a sentinel frame just for our user-created frame, in
>>     create_new_frame.  This sentinel frame has the same id as the regular
>>     sentinel frame.
>>   - When printing the frame, after doing the "select-frame view"
>>     command, the argument's pretty printer is invoked, which does an
>>     inferior function call (this is the point of the test).  This clears
>>     the frame cache, including the "real" sentinel frame, which sets the
>>     sentinel_frame global to nullptr.
>>   - Later in the frame-printing process (when printing the second
>>     argument), the auto-reinflation mechanism (which would have been the
>>     manual reinflate call before, doesn't matter here) re-creates the
>>     user frame by calling create_new_frame again, creating its own
>>     special sentinel frame again.  However, note that the "real" sentinel
>>     frame, the sentinel_frame global, is still nullptr.  If the selected
>>     frame had been a regular frame, we would have called
>>     get_current_frame at some point during the reinflation, which would
>>     have re-created the "real" sentinel frame.  But it's not the case
>>     here.
>>   - Deep down the stack, something wants to fill in the unwind stop
>>     reason for frame 0, which requires trying to unwind frame 1.  This
>>     leads us to trying to unwind the PC of frame 1:
>>
>>       #0  gdbarch_unwind_pc (gdbarch=0xffff8d010080, next_frame=...) at /home/simark/src/binutils-gdb/gdb/gdbarch.c:2955
>>       #1  0x000000000134569c in dwarf2_tailcall_sniffer_first (this_frame=..., tailcall_cachep=0xffff773fcae0, entry_cfa_sp_offsetp=0xfffff7f7d450)
>>           at /home/simark/src/binutils-gdb/gdb/dwarf2/frame-tailcall.c:390
>>       #2  0x0000000001355d84 in dwarf2_frame_cache (this_frame=..., this_cache=0xffff773fc928) at /home/simark/src/binutils-gdb/gdb/dwarf2/frame.c:1089
>>       #3  0x00000000013562b0 in dwarf2_frame_unwind_stop_reason (this_frame=..., this_cache=0xffff773fc928) at /home/simark/src/binutils-gdb/gdb/dwarf2/frame.c:1101
>>       #4  0x0000000001990f64 in get_prev_frame_always_1 (this_frame=...) at /home/simark/src/binutils-gdb/gdb/frame.c:2281
>>       #5  0x0000000001993034 in get_prev_frame_always (this_frame=...) at /home/simark/src/binutils-gdb/gdb/frame.c:2376
>>       #6  0x000000000199b814 in get_frame_unwind_stop_reason (frame=...) at /home/simark/src/binutils-gdb/gdb/frame.c:3051
>>       #7  0x0000000001359cd8 in dwarf2_frame_cfa (this_frame=...) at /home/simark/src/binutils-gdb/gdb/dwarf2/frame.c:1356
>>       #8  0x000000000132122c in dwarf_expr_context::execute_stack_op (this=0xfffff7f80170, op_ptr=0xffff8d8883ee "\217\002", op_end=0xffff8d8883ee "\217\002")
>>           at /home/simark/src/binutils-gdb/gdb/dwarf2/expr.c:2110
>>       #9  0x0000000001317b30 in dwarf_expr_context::eval (this=0xfffff7f80170, addr=0xffff8d8883ed "\234\217\002", len=1) at /home/simark/src/binutils-gdb/gdb/dwarf2/expr.c:1239
>>       #10 0x000000000131d68c in dwarf_expr_context::execute_stack_op (this=0xfffff7f80170, op_ptr=0xffff8d88840e "", op_end=0xffff8d88840e "") at /home/simark/src/binutils-gdb/gdb/dwarf2/expr.c:1811
>>       #11 0x0000000001317b30 in dwarf_expr_context::eval (this=0xfffff7f80170, addr=0xffff8d88840c "\221p", len=2) at /home/simark/src/binutils-gdb/gdb/dwarf2/expr.c:1239
>>       #12 0x0000000001314c3c in dwarf_expr_context::evaluate (this=0xfffff7f80170, addr=0xffff8d88840c "\221p", len=2, as_lval=true, per_cu=0xffff90b03700, frame=..., addr_info=0x0,
>>           type=0xffff8f6c8400, subobj_type=0xffff8f6c8400, subobj_offset=0) at /home/simark/src/binutils-gdb/gdb/dwarf2/expr.c:1078
>>       #13 0x000000000149f9e0 in dwarf2_evaluate_loc_desc_full (type=0xffff8f6c8400, frame=..., data=0xffff8d88840c "\221p", size=2, per_cu=0xffff90b03700, per_objfile=0xffff9070b980,
>>           subobj_type=0xffff8f6c8400, subobj_byte_offset=0, as_lval=true) at /home/simark/src/binutils-gdb/gdb/dwarf2/loc.c:1513
>>       #14 0x00000000014a0100 in dwarf2_evaluate_loc_desc (type=0xffff8f6c8400, frame=..., data=0xffff8d88840c "\221p", size=2, per_cu=0xffff90b03700, per_objfile=0xffff9070b980, as_lval=true)
>>           at /home/simark/src/binutils-gdb/gdb/dwarf2/loc.c:1557
>>       #15 0x00000000014aa584 in locexpr_read_variable (symbol=0xffff8f6cd770, frame=...) at /home/simark/src/binutils-gdb/gdb/dwarf2/loc.c:3052
>>   - AArch64 defines a special "prev register" function,
>>     aarch64_dwarf2_prev_register, to handle unwinding the PC.  This
>>     function does
>>
>>       frame_unwind_register_unsigned (this_frame, AARCH64_LR_REGNUM);
>>
>>     (I must admit that I don't really understand this part, it seems to
>>      me like you would want to unwind LR's value from this_frame->next...
>>      but anyway.)
> 
> It is somewhat historical, and it is something I'm planning to address eventually. Right now, though, we hardwire the PC to
> obtaining LR.

Ok, I'd be interesting in knowing more about the situation, for my own
general knowledge.  I might be understanding the GDB unwinding wrong of
the ARM-specific unwinding wrong.

>>   - frame_unwind_register_unsigned ultimately creates a lazy register
>>     value, saving the frame id of this_frame->next.  this_frame is the
>>     user-created frame, to this_frame->next is the special sentinel frame
>>     we created for it.
>>   - When time comes to un-lazify the value, value_fetch_lazy_register
>>     calls frame_find_by_id, to find the frame with the id we saved.
>>   - frame_find_by_id sees it's the sentinel frame id, so returns the
>>     sentinel_frame global, which is, if you remember, nullptr.
>>   - We hit the `gdb_assert (next_frame != NULL)` assertion.
>>
>> I have a hard time pinpointing a single thing that is wrong.  The fact
>> that the user-created frames have their own sentinel frame, which share
>> the same id as the real sentinel frame, that sounds like it can't work
> 
> From your (very detailed) explanation, it does sound a bit iffy to abuse the frame id mechanism and use
> the same id as the existing real frame.

Yeah, my latest attempt at fixing it revolves around giving the
user-created frame's sentinels their own specific ID.

Simon

^ permalink raw reply	[flat|nested] 28+ messages in thread

end of thread, other threads:[~2023-01-30 16:20 UTC | newest]

Thread overview: 28+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-12-14  3:34 [PATCH v2 00/13] Make frame_info_ptr automatic Simon Marchi
2022-12-14  3:34 ` [PATCH v2 01/13] gdb: move type_map_instance to compile/compile.c Simon Marchi
2022-12-14  3:34 ` [PATCH v2 02/13] gdb: move compile_instance to compile/compile.h Simon Marchi
2022-12-14  3:34 ` [PATCH v2 03/13] gdb: remove language.h include from frame.h Simon Marchi
2022-12-14  3:34 ` [PATCH v2 04/13] gdb: move sect_offset and cu_offset to dwarf2/types.h Simon Marchi
2022-12-14  3:34 ` [PATCH v2 05/13] gdb: move call site types to call-site.h Simon Marchi
2022-12-14  3:34 ` [PATCH v2 06/13] gdb: move frame_info_ptr to frame.{c,h} Simon Marchi
2022-12-20 17:01   ` Bruno Larsen
2023-01-03 18:59     ` Simon Marchi
2022-12-14  3:34 ` [PATCH v2 07/13] gdb: add frame_id::user_created_p Simon Marchi
2022-12-14  3:34 ` [PATCH v2 08/13] gdb: add user-created frames to stash Simon Marchi
2022-12-14  3:34 ` [PATCH v2 09/13] gdb: add create_new_frame(frame_id) overload Simon Marchi
2022-12-14  3:34 ` [PATCH v2 10/13] gdb: make it possible to restore selected user-created frames Simon Marchi
2022-12-14  3:34 ` [PATCH v2 11/13] gdb: make user-created frames reinflatable Simon Marchi
2023-01-23 12:57   ` Tom de Vries
2023-01-23 14:34     ` Luis Machado
2023-01-24  3:55       ` Simon Marchi
2023-01-24  8:22         ` Luis Machado
2023-01-25  3:45           ` Simon Marchi
2023-01-30  8:49             ` Luis Machado
2023-01-30 16:20               ` Simon Marchi
2022-12-14  3:34 ` [PATCH v2 12/13] gdb: make frame_info_ptr grab frame level and id on construction Simon Marchi
2022-12-14  3:34 ` [PATCH v2 13/13] gdb: make frame_info_ptr auto-reinflatable Simon Marchi
2022-12-20 16:57 ` [PATCH v2 00/13] Make frame_info_ptr automatic Bruno Larsen
2023-01-03 19:00   ` Simon Marchi
2023-01-03 19:09 ` Simon Marchi
2023-01-18 18:10 ` Tom Tromey
2023-01-19  3:40   ` Simon Marchi

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).