[PATCH 0/3] Use C++ type traits check to catch bugs in Python API

[PATCH 0/3] Use C++ type traits check to catch bugs in Python API

[Andrew Burgess]

While looking at a bug with the Python API I realised that we could
make use of static_assert and C++ type traits to catch this class of
bug.

This series has 3 patches, #1 adds the static_assert check to every
place in GDB where we don't currently have a bug.  Patch #2 fixes an
existing bug in GDB, and then adds the final static_assert which
covers this case.  Patch #3 adds a new test that scans the GDB source
code to make sure that there is a suitable static_assert for every
PyObject sub-class, this ensures the static_assert will be added to
any new code going forward.

Thanks,
Andrew

---

Andrew Burgess (3):
  gdb/python: add type traits check for all PyObject sub-classes
  gdb/python: fix use of frame_info_ptr within pending_frame_object
  gdb/testsuite: add a test to check for Python traits static_assert

 gdb/python/py-arch.c                          |   2 +
 gdb/python/py-block.c                         |   4 +
 gdb/python/py-breakpoint.c                    |   2 +
 gdb/python/py-cmd.c                           |   2 +
 gdb/python/py-color.c                         |   2 +
 gdb/python/py-connection.c                    |   2 +
 gdb/python/py-corefile.c                      |   4 +
 gdb/python/py-disasm.c                        |   8 +
 gdb/python/py-events.h                        |   2 +
 gdb/python/py-frame.c                         |   2 +
 gdb/python/py-instruction.c                   |   2 +
 gdb/python/py-lazy-string.c                   |   2 +
 gdb/python/py-linetable.c                     |   6 +
 gdb/python/py-membuf.c                        |   2 +
 gdb/python/py-micmd.c                         |   2 +
 gdb/python/py-param.c                         |   2 +
 gdb/python/py-prettyprint.c                   |   2 +
 gdb/python/py-record-btrace.c                 |   2 +
 gdb/python/py-record.c                        |   2 +
 gdb/python/py-record.h                        |   4 +
 gdb/python/py-ref.h                           |   3 +
 gdb/python/py-registers.c                     |   8 +
 gdb/python/py-style.c                         |   2 +
 gdb/python/py-symbol.c                        |   2 +
 gdb/python/py-symtab.c                        |   4 +
 gdb/python/py-tui.c                           |   2 +
 gdb/python/py-type.c                          |   4 +
 gdb/python/py-unwind.c                        |  55 +++---
 gdb/python/py-value.c                         |   2 +
 gdb/python/python-internal.h                  |   2 +
 gdb/python/python-traits.h                    |  92 +++++++++++
 gdb/testsuite/gdb.gdb/python-traits-check.exp | 156 ++++++++++++++++++
 32 files changed, 368 insertions(+), 20 deletions(-)
 create mode 100644 gdb/python/python-traits.h
 create mode 100644 gdb/testsuite/gdb.gdb/python-traits-check.exp


base-commit: 80491726b42ff67e06e07068e0e29b66b7595a73
-- 
2.25.4

[PATCH 1/3] gdb/python: add type traits check for all PyObject sub-classes

[Andrew Burgess]

All of our custom Python types are created as structs, like this:

  struct some_new_type : public PyObject
  {
    ... various fields ...
  };

Then instances of this struct are created by calling PyObject_New,
either directly within GDB's C++ code, or within Python when a user's
Python script creates an instance of that class.

The problem is that Python is written in C, and PyObject_New doesn't
call any constructors for `some_new_type`, nor for any of the fields
within `some_new_type`.

If `some_new_type` is Plain Old Data (POD), then this is fine.  Or, to
be more C++ specific, if `some_new_type` is trivially default
constructable, then we're fine.

But if a field within `some_new_type` has a non-trivial constructor,
then we're in trouble as that constructor will never be run.

An example of a problematic field type is frame_info_ptr.  The
constructor for this type registers the new object with a central
management object, recording the `this` pointer, using this type within
`some_new_type` will not work as expected; frame invalidation will not
show up within the frame_info_ptr as you might expect.

And so, this type trait exists.  Whenever a struct is created to define
a new Python type we should add a line like:

  static_assert (gdb::is_python_allocatable_v<some_new_type>);

This will fail if any field of `some_new_type` are unsuitable for this
use.

We don't actually check is_trivially_default_constructible here.  Some
types, e.g. ui_file_style::color, have non-trivial (or no default)
constructors, but are still safe to use within `some_new_type` because
their constructors just initialise data fields; there's nothing
"special" that the constructor does that cannot be achieved by
assigning the fields after creation with PyObject_New.

What actually matters is that the type is trivially destructible
(Python won't call C++ destructors, so destructors with side effects,
like deregistering from a list, would be skipped) and trivially
copyable (Python may copy objects with memcpy).  Types like
frame_info_ptr, whose constructors and destructors have side effects
such as registering with a central management object, will be caught
because they are neither trivially destructible nor trivially copyable.
Simple POD types like ui_file_style are trivially destructible and
copyable, so pass this trait.

This commit adds the new type trait, and makes use of it in all cases
but one, pending_frame_object in python/py-unwind.c, has a field of
type frame_info_ptr, which is currently broken.  This will be fixed,
and the static_assert added, in the next commit.
---
 gdb/python/py-arch.c          |  2 +
 gdb/python/py-block.c         |  4 ++
 gdb/python/py-breakpoint.c    |  2 +
 gdb/python/py-cmd.c           |  2 +
 gdb/python/py-color.c         |  2 +
 gdb/python/py-connection.c    |  2 +
 gdb/python/py-corefile.c      |  4 ++
 gdb/python/py-disasm.c        |  8 +++
 gdb/python/py-events.h        |  2 +
 gdb/python/py-frame.c         |  2 +
 gdb/python/py-instruction.c   |  2 +
 gdb/python/py-lazy-string.c   |  2 +
 gdb/python/py-linetable.c     |  6 +++
 gdb/python/py-membuf.c        |  2 +
 gdb/python/py-micmd.c         |  2 +
 gdb/python/py-param.c         |  2 +
 gdb/python/py-prettyprint.c   |  2 +
 gdb/python/py-record-btrace.c |  2 +
 gdb/python/py-record.c        |  2 +
 gdb/python/py-record.h        |  4 ++
 gdb/python/py-ref.h           |  3 ++
 gdb/python/py-registers.c     |  8 +++
 gdb/python/py-style.c         |  2 +
 gdb/python/py-symbol.c        |  2 +
 gdb/python/py-symtab.c        |  4 ++
 gdb/python/py-tui.c           |  2 +
 gdb/python/py-type.c          |  4 ++
 gdb/python/py-unwind.c        |  2 +
 gdb/python/py-value.c         |  2 +
 gdb/python/python-internal.h  |  2 +
 gdb/python/python-traits.h    | 92 +++++++++++++++++++++++++++++++++++
 31 files changed, 179 insertions(+)
 create mode 100644 gdb/python/python-traits.h

diff --git a/gdb/python/py-arch.c b/gdb/python/py-arch.c
index ac0c40dcf45..7a0cb0a2a59 100644
--- a/gdb/python/py-arch.c
+++ b/gdb/python/py-arch.c
@@ -27,6 +27,8 @@ struct arch_object : public PyObject
   struct gdbarch *gdbarch;
 };
 
+static_assert (gdb::is_python_allocatable_v<arch_object>);
+
 static const registry<gdbarch>::key<PyObject, gdb::noop_deleter<PyObject>>
      arch_object_data;
 
diff --git a/gdb/python/py-block.c b/gdb/python/py-block.c
index d9751d26378..39ec52ae373 100644
--- a/gdb/python/py-block.c
+++ b/gdb/python/py-block.c
@@ -33,6 +33,8 @@ struct block_object : public PyObject
   struct objfile *objfile;
 };
 
+static_assert (gdb::is_python_allocatable_v<block_object>);
+
 struct block_syms_iterator_object : public PyObject
 {
   /* The block.  */
@@ -47,6 +49,8 @@ struct block_syms_iterator_object : public PyObject
   block_object *source;
 };
 
+static_assert (gdb::is_python_allocatable_v<block_syms_iterator_object>);
+
 /* Require a valid block.  All access to block_object->block should be
    gated by this call.  */
 #define BLPY_REQUIRE_VALID(block_obj, block)		\
diff --git a/gdb/python/py-breakpoint.c b/gdb/python/py-breakpoint.c
index d96c67694b3..fb6fab6b60b 100644
--- a/gdb/python/py-breakpoint.c
+++ b/gdb/python/py-breakpoint.c
@@ -46,6 +46,8 @@ struct gdbpy_breakpoint_location_object : public PyObject
   gdbpy_breakpoint_object *owner;
 };
 
+static_assert (gdb::is_python_allocatable_v<gdbpy_breakpoint_location_object>);
+
 /* Require that BREAKPOINT and LOCATION->OWNER are the same; throw a Python
    exception if they are not.  */
 #define BPLOCPY_REQUIRE_VALID(Breakpoint, Location)                         \
diff --git a/gdb/python/py-cmd.c b/gdb/python/py-cmd.c
index 3fbc26b0cde..a1a02bd7eb5 100644
--- a/gdb/python/py-cmd.c
+++ b/gdb/python/py-cmd.c
@@ -63,6 +63,8 @@ struct cmdpy_object : public PyObject
   struct cmd_list_element *sub_list;
 };
 
+static_assert (gdb::is_python_allocatable_v<cmdpy_object>);
+
 extern PyTypeObject cmdpy_object_type;
 
 /* Constants used by this module.  */
diff --git a/gdb/python/py-color.c b/gdb/python/py-color.c
index 971209958cf..29a42977adc 100644
--- a/gdb/python/py-color.c
+++ b/gdb/python/py-color.c
@@ -43,6 +43,8 @@ struct colorpy_object : public PyObject
   ui_file_style::color color;
 };
 
+static_assert (gdb::is_python_allocatable_v<colorpy_object>);
+
 extern PyTypeObject colorpy_object_type;
 
 /* See py-color.h.  */
diff --git a/gdb/python/py-connection.c b/gdb/python/py-connection.c
index a8bea4d832f..8f9fc4a7eee 100644
--- a/gdb/python/py-connection.c
+++ b/gdb/python/py-connection.c
@@ -42,6 +42,8 @@ struct connection_object : public PyObject
   struct process_stratum_target *target;
 };
 
+static_assert (gdb::is_python_allocatable_v<connection_object>);
+
 extern PyTypeObject connection_object_type;
 
 extern PyTypeObject remote_connection_object_type;
diff --git a/gdb/python/py-corefile.c b/gdb/python/py-corefile.c
index 0fa4e90488c..1cdbae98149 100644
--- a/gdb/python/py-corefile.c
+++ b/gdb/python/py-corefile.c
@@ -65,6 +65,8 @@ struct corefile_mapped_file_object : public PyObject
   bool is_main_exec_p;
 };
 
+static_assert (gdb::is_python_allocatable_v<corefile_mapped_file_object>);
+
 extern PyTypeObject corefile_mapped_file_object_type;
 
 /* A gdb.CorefileMappedFileRegion object.  */
@@ -80,6 +82,8 @@ struct corefile_mapped_file_region_object : public PyObject
   ULONGEST file_offset;
 };
 
+static_assert (gdb::is_python_allocatable_v<corefile_mapped_file_region_object>);
+
 extern PyTypeObject corefile_mapped_file_region_object_type;
 
 /* Clear the inferior pointer in a Corefile object OBJ when an inferior is
diff --git a/gdb/python/py-disasm.c b/gdb/python/py-disasm.c
index 1c5661dd307..9591cb82133 100644
--- a/gdb/python/py-disasm.c
+++ b/gdb/python/py-disasm.c
@@ -50,6 +50,8 @@ struct disasm_info_object : public PyObject
   struct disasm_info_object *next;
 };
 
+static_assert (gdb::is_python_allocatable_v<disasm_info_object>);
+
 extern PyTypeObject disasm_info_object_type;
 
 /* Implement gdb.disassembler.DisassembleAddressPart type.  An object of
@@ -69,6 +71,8 @@ struct disasm_addr_part_object : public PyObject
   struct gdbarch *gdbarch;
 };
 
+static_assert (gdb::is_python_allocatable_v<disasm_addr_part_object>);
+
 extern PyTypeObject disasm_addr_part_object_type;
 
 /* Implement gdb.disassembler.DisassembleTextPart type.  An object of
@@ -84,6 +88,8 @@ struct disasm_text_part_object : public PyObject
   enum disassembler_style style;
 };
 
+static_assert (gdb::is_python_allocatable_v<disasm_text_part_object>);
+
 extern PyTypeObject disasm_text_part_object_type;
 
 extern PyTypeObject disasm_part_object_type;
@@ -103,6 +109,8 @@ struct disasm_result_object : public PyObject
   std::vector<gdbpy_ref<>> *parts;
 };
 
+static_assert (gdb::is_python_allocatable_v<disasm_result_object>);
+
 extern PyTypeObject disasm_result_object_type;
 
 /* When this is false we fast path out of gdbpy_print_insn, which should
diff --git a/gdb/python/py-events.h b/gdb/python/py-events.h
index e44b4b4a761..169a5f4a4db 100644
--- a/gdb/python/py-events.h
+++ b/gdb/python/py-events.h
@@ -32,6 +32,8 @@ struct eventregistry_object : public PyObject
   PyObject *callbacks;
 };
 
+static_assert (gdb::is_python_allocatable_v<eventregistry_object>);
+
 /* Struct holding references to event registries both in python and c.
    This is meant to be a singleton.  */
 
diff --git a/gdb/python/py-frame.c b/gdb/python/py-frame.c
index 1420d2ac5b9..ad590c7f55a 100644
--- a/gdb/python/py-frame.c
+++ b/gdb/python/py-frame.c
@@ -44,6 +44,8 @@ struct frame_object : public PyObject
   int frame_id_is_next;
 };
 
+static_assert (gdb::is_python_allocatable_v<frame_object>);
+
 /* Require a valid frame.  This must be called inside a TRY_CATCH, or
    another context in which a gdb exception is allowed.  */
 #define FRAPY_REQUIRE_VALID(frame_obj, frame)		\
diff --git a/gdb/python/py-instruction.c b/gdb/python/py-instruction.c
index df64d85988f..d8dad094ee9 100644
--- a/gdb/python/py-instruction.c
+++ b/gdb/python/py-instruction.c
@@ -32,6 +32,8 @@ PyTypeObject py_insn_type = {
 struct py_insn_obj: public PyObject
 {};
 
+static_assert (gdb::is_python_allocatable_v<py_insn_obj>);
+
 /* Getter function for gdb.Instruction attributes.  */
 
 static PyObject *
diff --git a/gdb/python/py-lazy-string.c b/gdb/python/py-lazy-string.c
index fe191451e54..79592b10aa6 100644
--- a/gdb/python/py-lazy-string.c
+++ b/gdb/python/py-lazy-string.c
@@ -51,6 +51,8 @@ struct lazy_string_object : public PyObject
   PyObject *type;
 };
 
+static_assert (gdb::is_python_allocatable_v<lazy_string_object>);
+
 extern PyTypeObject lazy_string_object_type;
 
 static PyObject *
diff --git a/gdb/python/py-linetable.c b/gdb/python/py-linetable.c
index 2afa9b033e7..c0368355148 100644
--- a/gdb/python/py-linetable.c
+++ b/gdb/python/py-linetable.c
@@ -27,6 +27,8 @@ struct linetable_entry_object : public PyObject
   CORE_ADDR pc;
 };
 
+static_assert (gdb::is_python_allocatable_v<linetable_entry_object>);
+
 extern PyTypeObject linetable_entry_object_type;
 
 struct linetable_object : public PyObject
@@ -37,6 +39,8 @@ struct linetable_object : public PyObject
   PyObject *symtab;
 };
 
+static_assert (gdb::is_python_allocatable_v<linetable_object>);
+
 extern PyTypeObject linetable_object_type;
 
 struct ltpy_iterator_object : public PyObject
@@ -49,6 +53,8 @@ struct ltpy_iterator_object : public PyObject
   PyObject *source;
 };
 
+static_assert (gdb::is_python_allocatable_v<ltpy_iterator_object>);
+
 extern PyTypeObject ltpy_iterator_object_type;
 
 /* Internal helper function to extract gdb.Symtab from a gdb.LineTable
diff --git a/gdb/python/py-membuf.c b/gdb/python/py-membuf.c
index e3bf5e2ceab..fa5156b885e 100644
--- a/gdb/python/py-membuf.c
+++ b/gdb/python/py-membuf.c
@@ -31,6 +31,8 @@ struct membuf_object : public PyObject
   CORE_ADDR length;
 };
 
+static_assert (gdb::is_python_allocatable_v<membuf_object>);
+
 extern PyTypeObject membuf_object_type;
 
 /* Wrap BUFFER, ADDRESS, and LENGTH into a gdb.Membuf object.  ADDRESS is
diff --git a/gdb/python/py-micmd.c b/gdb/python/py-micmd.c
index 6edea04ccb5..241e80bea5b 100644
--- a/gdb/python/py-micmd.c
+++ b/gdb/python/py-micmd.c
@@ -74,6 +74,8 @@ struct micmdpy_object : public PyObject
   char *mi_command_name;
 };
 
+static_assert (gdb::is_python_allocatable_v<micmdpy_object>);
+
 /* The MI command implemented in Python.  */
 
 struct mi_command_py : public mi_command
diff --git a/gdb/python/py-param.c b/gdb/python/py-param.c
index bc028f1746c..35f55c3dbd0 100644
--- a/gdb/python/py-param.c
+++ b/gdb/python/py-param.c
@@ -141,6 +141,8 @@ struct parmpy_object : public PyObject
   const char **enumeration;
 };
 
+static_assert (gdb::is_python_allocatable_v<parmpy_object>);
+
 /* Wraps a setting around an existing parmpy_object.  This abstraction
    is used to manipulate the value in S->VALUE in a type safe manner using
    the setting interface.  */
diff --git a/gdb/python/py-prettyprint.c b/gdb/python/py-prettyprint.c
index 0cf0cde881c..b0a296d8010 100644
--- a/gdb/python/py-prettyprint.c
+++ b/gdb/python/py-prettyprint.c
@@ -784,6 +784,8 @@ gdbpy_get_print_options (value_print_options *opts)
 struct printer_object : public PyObject
 {};
 
+static_assert (gdb::is_python_allocatable_v<printer_object>);
+
 /* The ValuePrinter type object.  */
 PyTypeObject printer_object_type =
 {
diff --git a/gdb/python/py-record-btrace.c b/gdb/python/py-record-btrace.c
index 6026de91e67..267429832d9 100644
--- a/gdb/python/py-record-btrace.c
+++ b/gdb/python/py-record-btrace.c
@@ -48,6 +48,8 @@ struct btpy_list_object : public PyObject
   PyTypeObject* element_type;
 };
 
+static_assert (gdb::is_python_allocatable_v<btpy_list_object>);
+
 /* Python type for btrace lists.  */
 
 static PyTypeObject btpy_list_type = {
diff --git a/gdb/python/py-record.c b/gdb/python/py-record.c
index 294da0725e1..5394d63c803 100644
--- a/gdb/python/py-record.c
+++ b/gdb/python/py-record.c
@@ -67,6 +67,8 @@ struct recpy_gap_object : public PyObject
   Py_ssize_t number;
 };
 
+static_assert (gdb::is_python_allocatable_v<recpy_gap_object>);
+
 /* Implementation of record.method.  */
 
 static PyObject *
diff --git a/gdb/python/py-record.h b/gdb/python/py-record.h
index 35ab54cf6ab..f62fcc2a7e9 100644
--- a/gdb/python/py-record.h
+++ b/gdb/python/py-record.h
@@ -34,6 +34,8 @@ struct recpy_record_object : public PyObject
   enum record_method method;
 };
 
+static_assert (gdb::is_python_allocatable_v<recpy_record_object>);
+
 /* Python recorded element object.  This is generic enough to represent
    recorded instructions as well as recorded function call segments, hence the
    generic name.  */
@@ -49,6 +51,8 @@ struct recpy_element_object : public PyObject
   Py_ssize_t number;
 };
 
+static_assert (gdb::is_python_allocatable_v<recpy_element_object>);
+
 /* Python RecordInstruction type.  */
 extern PyTypeObject recpy_insn_type;
 
diff --git a/gdb/python/py-ref.h b/gdb/python/py-ref.h
index dc0b14814af..3d2906fd7f5 100644
--- a/gdb/python/py-ref.h
+++ b/gdb/python/py-ref.h
@@ -21,6 +21,7 @@
 #define GDB_PYTHON_PY_REF_H
 
 #include "gdbsupport/gdb_ref_ptr.h"
+#include "python-traits.h"
 
 /* A policy class for gdb::ref_ptr for Python reference counting.  */
 struct gdbpy_ref_policy
@@ -102,4 +103,6 @@ struct gdbpy_dict_wrapper : public PyObject
   }
 };
 
+static_assert (gdb::is_python_allocatable_v<gdbpy_dict_wrapper>);
+
 #endif /* GDB_PYTHON_PY_REF_H */
diff --git a/gdb/python/py-registers.c b/gdb/python/py-registers.c
index c6e0748e935..af75cb81bbe 100644
--- a/gdb/python/py-registers.c
+++ b/gdb/python/py-registers.c
@@ -45,6 +45,8 @@ struct register_descriptor_iterator_object : public PyObject
   struct gdbarch *gdbarch;
 };
 
+static_assert (gdb::is_python_allocatable_v<register_descriptor_iterator_object>);
+
 extern PyTypeObject register_descriptor_iterator_object_type;
 
 /* A register descriptor.  */
@@ -57,6 +59,8 @@ struct register_descriptor_object : public PyObject
   struct gdbarch *gdbarch;
 };
 
+static_assert (gdb::is_python_allocatable_v<register_descriptor_object>);
+
 extern PyTypeObject register_descriptor_object_type;
 
 /* Structure for iterator over register groups.  */
@@ -69,6 +73,8 @@ struct reggroup_iterator_object : public PyObject
   struct gdbarch *gdbarch;
 };
 
+static_assert (gdb::is_python_allocatable_v<reggroup_iterator_object>);
+
 extern PyTypeObject reggroup_iterator_object_type;
 
 /* A register group object.  */
@@ -78,6 +84,8 @@ struct reggroup_object : public PyObject
   const struct reggroup *reggroup;
 };
 
+static_assert (gdb::is_python_allocatable_v<reggroup_object>);
+
 extern PyTypeObject reggroup_object_type;
 
 /* Map from GDB's internal reggroup objects to the Python
diff --git a/gdb/python/py-style.c b/gdb/python/py-style.c
index 60a9ea4792f..c45eba60867 100644
--- a/gdb/python/py-style.c
+++ b/gdb/python/py-style.c
@@ -46,6 +46,8 @@ struct style_object : public PyObject
   char *style_name;
 };
 
+static_assert (gdb::is_python_allocatable_v<style_object>);
+
 extern PyTypeObject style_object_type;
 
 /* Initialize the 'style' module.  */
diff --git a/gdb/python/py-symbol.c b/gdb/python/py-symbol.c
index 6293b658ea1..a3b10d3df24 100644
--- a/gdb/python/py-symbol.c
+++ b/gdb/python/py-symbol.c
@@ -31,6 +31,8 @@ struct symbol_object : public PyObject
   struct symbol *symbol;
 };
 
+static_assert (gdb::is_python_allocatable_v<symbol_object>);
+
 /* Require a valid symbol.  All access to symbol_object->symbol should be
    gated by this call.  */
 #define SYMPY_REQUIRE_VALID(symbol_obj, symbol)		\
diff --git a/gdb/python/py-symtab.c b/gdb/python/py-symtab.c
index c5e18543740..dffddecb412 100644
--- a/gdb/python/py-symtab.c
+++ b/gdb/python/py-symtab.c
@@ -32,6 +32,8 @@ struct symtab_object : public PyObject
   struct symtab *symtab;
 };
 
+static_assert (gdb::is_python_allocatable_v<symtab_object>);
+
 extern PyTypeObject symtab_object_type;
 static const gdbpy_registry<gdbpy_memoizing_registry_storage<symtab_object,
   symtab, &symtab_object::symtab>> stpy_registry;
@@ -61,6 +63,8 @@ struct sal_object : public PyObject
   sal_object *next;
 };
 
+static_assert (gdb::is_python_allocatable_v<sal_object>);
+
 /* This is called when an objfile is about to be freed.  Invalidate
    the sal object as further actions on the sal would result in bad
    data.  All access to obj->sal should be gated by
diff --git a/gdb/python/py-tui.c b/gdb/python/py-tui.c
index be19193770f..4db04941d2d 100644
--- a/gdb/python/py-tui.c
+++ b/gdb/python/py-tui.c
@@ -53,6 +53,8 @@ struct gdbpy_tui_window: public PyObject
   bool is_valid () const;
 };
 
+static_assert (gdb::is_python_allocatable_v<gdbpy_tui_window>);
+
 extern PyTypeObject gdbpy_tui_window_object_type;
 
 /* A TUI window written in Python.  */
diff --git a/gdb/python/py-type.c b/gdb/python/py-type.c
index 263d48a9365..4b6ac0e9eef 100644
--- a/gdb/python/py-type.c
+++ b/gdb/python/py-type.c
@@ -33,6 +33,8 @@ struct type_object : public PyObject
   struct type *type;
 };
 
+static_assert (gdb::is_python_allocatable_v<type_object>);
+
 extern PyTypeObject type_object_type;
 
 /* A Field object.  */
@@ -52,6 +54,8 @@ struct typy_iterator_object : public PyObject
   type_object *source;
 };
 
+static_assert (gdb::is_python_allocatable_v<typy_iterator_object>);
+
 extern PyTypeObject type_iterator_object_type;
 
 /* This is used to initialize various gdb.TYPE_ constants.  */
diff --git a/gdb/python/py-unwind.c b/gdb/python/py-unwind.c
index dcf86f7db3d..ed0ba89d267 100644
--- a/gdb/python/py-unwind.c
+++ b/gdb/python/py-unwind.c
@@ -104,6 +104,8 @@ struct unwind_info_object : public PyObject
   std::vector<saved_reg> *saved_regs;
 };
 
+static_assert (gdb::is_python_allocatable_v<unwind_info_object>);
+
 /* The data we keep for a frame we can unwind: frame ID and an array of
    (register_number, register_value) pairs.  */
 
diff --git a/gdb/python/py-value.c b/gdb/python/py-value.c
index a5b2303728c..ca88a35e357 100644
--- a/gdb/python/py-value.c
+++ b/gdb/python/py-value.c
@@ -64,6 +64,8 @@ struct value_object : public PyObject
   PyObject *content_bytes;
 };
 
+static_assert (gdb::is_python_allocatable_v<value_object>);
+
 /* List of all values which are currently exposed to Python. It is
    maintained so that when an objfile is discarded, preserve_values
    can copy the values' types if needed.  */
diff --git a/gdb/python/python-internal.h b/gdb/python/python-internal.h
index 37bc37691fe..ac01fa5a385 100644
--- a/gdb/python/python-internal.h
+++ b/gdb/python/python-internal.h
@@ -364,6 +364,8 @@ struct gdbpy_breakpoint_object : public PyObject
   int is_finish_bp;
 };
 
+static_assert (gdb::is_python_allocatable_v<gdbpy_breakpoint_object>);
+
 /* Require that BREAKPOINT be a valid breakpoint ID; throw a Python
    exception if it is invalid.  */
 #define BPPY_REQUIRE_VALID(Breakpoint)                                  \
diff --git a/gdb/python/python-traits.h b/gdb/python/python-traits.h
new file mode 100644
index 00000000000..7babb0fc131
--- /dev/null
+++ b/gdb/python/python-traits.h
@@ -0,0 +1,92 @@
+/* Type traits relating to GDB's Python integration.
+
+   Copyright (C) 2008-2026 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_PYTHON_PYTHON_TRAITS_H
+#define GDB_PYTHON_PYTHON_TRAITS_H
+
+namespace gdb
+{
+/* All of our custom Python types are created as structs, like this:
+
+   struct some_new_type : public PyObject
+   {
+     ... various fields ...
+   };
+
+   Then instances of this struct are created by calling PyObject_New,
+   either directly within GDB's C++ code, or within Python when a user's
+   Python script creates an instance of that class.
+
+   The problem is that Python is written in C, and PyObject_New doesn't
+   call any constructors for `some_new_type`, nor for any of the fields
+   within `some_new_type`.
+
+   If `some_new_type` is Plain Old Data (POD), then this is fine.  Or, to
+   be more C++ specific, if `some_new_type` is trivially default
+   constructable, then we're fine.
+
+   But if a field within `some_new_type` has a non-trivial constructor,
+   then we're in trouble as that constructor will never be run.
+
+   An example of a problematic field type is frame_info_ptr.  The
+   constructor for this type registers the new object with a central
+   management object, recording the `this` pointer, using this type within
+   `some_new_type` will not work as expected; frame invalidation will not
+   show up within the frame_info_ptr as you might expect.
+
+   And so, this type trait exists.  Whenever a struct is created to define
+   a new Python type we should add a line like:
+
+     static_assert (gdb::is_python_allocatable_v<some_new_type>);
+
+   This will fail if any field of `some_new_type` is unsuitable for this
+   use.
+
+   We don't actually check is_trivially_default_constructible here.  Some
+   types, e.g. ui_file_style::color, have non-trivial (or no default)
+   constructors, but are still safe to use within `some_new_type` because
+   their constructors just initialise data fields; there's nothing
+   "special" that the constructor does that cannot be achieved by
+   assigning the fields after creation with PyObject_New.
+
+   What actually matters is that the type is trivially destructible
+   (Python won't call C++ destructors, so destructors with side effects,
+   like deregistering from a list, would be skipped) and trivially
+   copyable (Python may copy objects with memcpy).  Types like
+   frame_info_ptr, whose constructors and destructors have side effects
+   such as registering with a central management object, will be caught
+   because they are neither trivially destructible nor trivially copyable.
+   Types like ui_file_style are trivially destructible and copyable, so
+   pass this trait.  */
+
+template <typename T>
+struct is_python_allocatable {
+  static constexpr bool value =
+    std::is_trivially_destructible_v<T> &&
+    std::is_trivially_copyable_v<T>;
+};
+
+/* Helper for the above trait to make it more usable.  */
+
+template <typename T>
+inline constexpr bool is_python_allocatable_v
+  = is_python_allocatable<T>::value;
+}
+
+#endif /* GDB_PYTHON_PYTHON_TRAITS_H */
-- 
2.25.4

[PATCH 2/3] gdb/python: fix use of frame_info_ptr within pending_frame_object

[Andrew Burgess]

The previous commit added a type trait which identifies types that
should not be used within Python objects, that is, types that are not
trivially default constructible.  As a result of this, it was
discovered that pending_frame_object includes a frame_info_ptr field.

The problem with frame_info_ptr is that its constructor registers the
new frame_info_ptr with the global frame_list.  It is by this
registration that invalidation of frame_info_ptr objects is performed.

As Python is written in C, C++ constructors are not called, so when a
pending_frame_object is created the constructor for the nested
frame_info_ptr field is never run, and the frame_info_ptr is never
registered with the global frame_list.  As a result the frame_info_ptr
will never be invalidated if the frame cache is flushed, this can then
lead to problems where we make use of the 'frame_info *' within the
frame_info_ptr, even though it is no longer valid.

In this commit I change the frame_info_ptr within pending_frame_object
to a 'frame_info_ptr *' and allocate the frame_info_ptr object on the
heap, releasing the object, and resetting the point to NULL, when we
are done with it.  As the pending_frame_object only needs to remain
valid for the duration of frame_unwind_python::sniff, the 'new' and
'delete' both performed within the function.

We can now check that a pending_frame_object is valid by checking if
the 'frame_info_ptr *' is NULL or not.  As the frame_info_ptr is
created in a valid state, and the point is set back to NULL when we
are done with it, we no longer need to compare the frame_info_ptr
object itself against NULL.

The remaining changes in this patch are to dereference the
'frame_info_ptr *' in places where we need the actual object.  In some
cases I need to move the dereference later within a function, after a
validity check, in order to avoid dereferencing a NULL pointer.

Finally, I can add the static_assert that guarantees that
pending_frame_object is now safe for allocation by Python.

I discovered this bug while looking at PR gdb/32120.  That bug is
about a user's custom frame unwinder that triggers a flush of the
frame cache during the sniffer phase (the
RemoteTargetConnection.send_packet call switches thread, which
triggers the frame cache flush).  While looking at that bug I noticed
that the frame_info_ptr within the pending_frame_object wasn't being
reset when the frame cache was flushed.  Fixing this does not resolve
the user's issue, but I thought it was still worth tagging this commit
with the bug link.

Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=32120
---
 gdb/python/py-unwind.c | 53 ++++++++++++++++++++++++++----------------
 1 file changed, 33 insertions(+), 20 deletions(-)

diff --git a/gdb/python/py-unwind.c b/gdb/python/py-unwind.c
index ed0ba89d267..fd305cb3316 100644
--- a/gdb/python/py-unwind.c
+++ b/gdb/python/py-unwind.c
@@ -68,13 +68,17 @@ show_pyuw_debug (struct ui_file *file, int from_tty,
 
 struct pending_frame_object : public PyObject
 {
-  /* Frame we are unwinding.  */
-  frame_info_ptr frame_info;
+  /* Frame we are unwinding.  We cannot place a frame_info_ptr
+     directly within this struct as it is not trivially default
+     constructable.  */
+  frame_info_ptr *frame_info;
 
   /* Its architecture, passed by the sniffer caller.  */
   struct gdbarch *gdbarch;
 };
 
+static_assert (gdb::is_python_allocatable_v<pending_frame_object>);
+
 /* Saved registers array item.  */
 
 struct saved_reg
@@ -245,9 +249,8 @@ unwind_infopy_repr (PyObject *self)
   unwind_info_object *unwind_info = (unwind_info_object *) self;
   pending_frame_object *pending_frame
     = (pending_frame_object *) (unwind_info->pending_frame);
-  frame_info_ptr frame = pending_frame->frame_info;
 
-  if (frame == nullptr)
+  if (pending_frame->frame_info == nullptr)
     return PyUnicode_FromFormat ("<%s for an invalid frame>",
 				 gdbpy_py_obj_tp_name (self));
 
@@ -263,6 +266,7 @@ unwind_infopy_repr (PyObject *self)
 	saved_reg_names = (saved_reg_names + ", ") + name;
     }
 
+  frame_info_ptr frame (*pending_frame->frame_info);
   return PyUnicode_FromFormat ("<%s frame #%d, saved_regs=(%s)>",
 			       gdbpy_py_obj_tp_name (self),
 			       frame_relative_level (frame),
@@ -331,7 +335,7 @@ unwind_infopy_add_saved_register (PyObject *self, PyObject *args, PyObject *kw)
   if (regnum >= gdbarch_num_cooked_regs (pending_frame->gdbarch))
     {
       struct value *user_reg_value
-	= value_of_user_reg (regnum, pending_frame->frame_info);
+	= value_of_user_reg (regnum, *pending_frame->frame_info);
       if (user_reg_value->lval () == lval_register)
 	regnum = user_reg_value->regnum ();
       if (regnum >= gdbarch_num_cooked_regs (pending_frame->gdbarch))
@@ -414,14 +418,15 @@ unwind_infopy_dealloc (PyObject *self)
 static PyObject *
 pending_framepy_str (PyObject *self)
 {
-  frame_info_ptr frame = ((pending_frame_object *) self)->frame_info;
+  pending_frame_object *pending_frame = (pending_frame_object *) self;
   const char *sp_str = NULL;
   const char *pc_str = NULL;
 
-  if (frame == NULL)
+  if (pending_frame->frame_info == nullptr)
     return PyUnicode_FromString ("Stale PendingFrame instance");
   try
     {
+      frame_info_ptr frame (*pending_frame->frame_info);
       sp_str = core_addr_to_string_nz (get_frame_sp (frame));
       pc_str = core_addr_to_string_nz (get_frame_pc (frame));
     }
@@ -439,14 +444,15 @@ static PyObject *
 pending_framepy_repr (PyObject *self)
 {
   pending_frame_object *pending_frame = (pending_frame_object *) self;
-  frame_info_ptr frame = pending_frame->frame_info;
 
-  if (frame == nullptr)
+  if (pending_frame->frame_info == nullptr)
     return gdb_py_invalid_object_repr (self);
 
   const char *sp_str = nullptr;
   const char *pc_str = nullptr;
 
+  frame_info_ptr frame (*pending_frame->frame_info);
+
   try
     {
       sp_str = core_addr_to_string_nz (get_frame_sp (frame));
@@ -493,7 +499,7 @@ pending_framepy_read_register (PyObject *self, PyObject *args, PyObject *kw)
 	 get_frame_register_value() was used here, which did not
 	 handle the user register case.  */
       value *val = value_of_register
-	(regnum, get_next_frame_sentinel_okay (pending_frame->frame_info));
+	(regnum, get_next_frame_sentinel_okay (*pending_frame->frame_info));
       if (val == NULL)
 	PyErr_Format (PyExc_ValueError,
 		      "Cannot read register %d from frame.",
@@ -520,6 +526,10 @@ pending_framepy_is_valid (PyObject *self, PyObject *args)
   if (pending_frame->frame_info == nullptr)
     Py_RETURN_FALSE;
 
+  /* The frame_info field should never point at an uninitialized
+     object.  */
+  gdb_assert (*pending_frame->frame_info != nullptr);
+
   Py_RETURN_TRUE;
 }
 
@@ -538,7 +548,7 @@ pending_framepy_name (PyObject *self, PyObject *args)
   try
     {
       enum language lang;
-      frame_info_ptr frame = pending_frame->frame_info;
+      frame_info_ptr frame = *pending_frame->frame_info;
 
       name = find_frame_funname (frame, &lang, nullptr);
     }
@@ -568,7 +578,7 @@ pending_framepy_pc (PyObject *self, PyObject *args)
 
   try
     {
-      pc = get_frame_pc (pending_frame->frame_info);
+      pc = get_frame_pc (*pending_frame->frame_info);
     }
   catch (const gdb_exception &except)
     {
@@ -590,7 +600,7 @@ pending_framepy_language (PyObject *self, PyObject *args)
 
   try
     {
-      frame_info_ptr fi = pending_frame->frame_info;
+      frame_info_ptr fi = *pending_frame->frame_info;
 
       enum language lang = get_frame_language (fi);
       const language_defn *lang_def = language_def (lang);
@@ -615,7 +625,7 @@ pending_framepy_find_sal (PyObject *self, PyObject *args)
 
   try
     {
-      frame_info_ptr frame = pending_frame->frame_info;
+      frame_info_ptr frame = *pending_frame->frame_info;
 
       symtab_and_line sal = find_frame_sal (frame);
       return symtab_and_line_to_sal_object (sal).release ();
@@ -636,7 +646,7 @@ pending_framepy_block (PyObject *self, PyObject *args)
 
   PENDING_FRAMEPY_REQUIRE_VALID (pending_frame);
 
-  frame_info_ptr frame = pending_frame->frame_info;
+  frame_info_ptr frame = *pending_frame->frame_info;
   const struct block *block = nullptr, *fn_block;
 
   try
@@ -682,7 +692,7 @@ pending_framepy_function (PyObject *self, PyObject *args)
   try
     {
       enum language funlang;
-      frame_info_ptr frame = pending_frame->frame_info;
+      frame_info_ptr frame = *pending_frame->frame_info;
 
       gdb::unique_xmalloc_ptr<char> funname
 	= find_frame_funname (frame, &funlang, &sym);
@@ -774,7 +784,7 @@ pending_framepy_level (PyObject *self, PyObject *args)
 
   PENDING_FRAMEPY_REQUIRE_VALID (pending_frame);
 
-  int level = frame_relative_level (pending_frame->frame_info);
+  int level = frame_relative_level (*pending_frame->frame_info);
   return gdb_py_object_from_longest (level).release ();
 }
 
@@ -863,9 +873,12 @@ frame_unwind_python::sniff (const frame_info_ptr &this_frame,
       return 0;
     }
   pfo->gdbarch = gdbarch;
-  pfo->frame_info = nullptr;
-  scoped_restore invalidate_frame = make_scoped_restore (&pfo->frame_info,
-							 this_frame);
+  pfo->frame_info = new frame_info_ptr (this_frame);
+  SCOPE_EXIT
+    {
+      delete pfo->frame_info;
+      pfo->frame_info = nullptr;
+    };
 
   /* Run unwinders.  */
   if (gdb_python_module == NULL
-- 
2.25.4

[PATCH 3/3] gdb/testsuite: add a test to check for Python traits static_assert

[Andrew Burgess]

The previous two commits added a new type trait which can be used
within a static_assert to check the properties of a struct used by GDB
to implement Python objects.

The previous commit fixed a bug in GDB which this trait check exposed.

This commit adds a new test gdb.gdb/python-traits-check.exp which
checks that every struct in the Python/ directory that inherits from
PyObject, has a suitable static_assert in place.

Adding this test should mean that if someone adds a new Python object
type to GDB, and they forget to add the static_assert, then this test
should give a failure, which should remind them to add the required
static_assert.  The static_assert will then check that their new
struct is compliant.
---
 gdb/testsuite/gdb.gdb/python-traits-check.exp | 156 ++++++++++++++++++
 1 file changed, 156 insertions(+)
 create mode 100644 gdb/testsuite/gdb.gdb/python-traits-check.exp

diff --git a/gdb/testsuite/gdb.gdb/python-traits-check.exp b/gdb/testsuite/gdb.gdb/python-traits-check.exp
new file mode 100644
index 00000000000..46cb1cabbb0
--- /dev/null
+++ b/gdb/testsuite/gdb.gdb/python-traits-check.exp
@@ -0,0 +1,156 @@
+# Copyright 2026 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/>.
+
+# Check that every struct inheriting from PyObject in the gdb/python/
+# directory has a corresponding static_assert for
+# gdb::is_python_allocatable_v immediately after the struct definition.
+# The expected format is:
+#
+#   struct some_new_type : public PyObject
+#   {
+#     ... fields go here ...
+#   };
+#
+#   static_assert (gdb::is_python_allocatable_v<some_new_type>);
+#
+# It is OK to add comments between the struct and the static_assert if
+# needed, but nothing else, the static_assert must be the next non-empty,
+# non-comment line.
+#
+# If the new type has no fields then this can be written like:
+#
+#   struct some_empty_type : public PyObject
+#   {};
+#
+#   static_assert (gdb::is_python_allocatable_v<some_empty_type>);
+#
+# We do have a few of these in GDB currently.  We require that the
+# static_assert still be present because (a) it has zero run-time cost,
+# and (b) it catches issues if fields are added in the future.
+
+set python_dir "$srcdir/$subdir/../../python"
+
+# Gather all .c and .h files in the python directory.
+set files [lsort [concat \
+		      [glob -nocomplain -directory $python_dir *.c] \
+		      [glob -nocomplain -directory $python_dir *.h]]]
+
+gdb_assert { [llength $files] > 0 } "found python source files"
+
+# Check a single file for PyObject-derived structs and matching
+# static_asserts.
+#
+# Opens FILENAME, reads it line by line looking for struct definitions of
+# the form "struct NAME : public PyObject".  For each one found, scans
+# forward past the struct body, then checks that a matching static_assert
+# line follows, allowing only blank lines and GDB-style comments to
+# intervene.
+
+proc check_file { filename } {
+    set fd [open $filename r]
+    set lines [split [read $fd] "\n"]
+    close $fd
+
+    set num_lines [llength $lines]
+    set short_name [file tail $filename]
+
+    for { set i 0 } { $i < $num_lines } { incr i } {
+	set line [lindex $lines $i]
+
+	# Look for struct definitions inheriting from PyObject.  These
+	# start in column 0.
+	if { ![regexp {^struct (\w+)\s*:\s*public PyObject} \
+		   $line whole struct_name] } {
+	    continue
+	}
+
+	set testname "$short_name: $struct_name: static assert check"
+
+	# Found a struct.  Now scan forward for the closing brace and
+	# semicolon.  Within the struct body, lines are either blank or
+	# start with whitespace.  The closing line starts in column 0.  For
+	# empty structs the open and close brace may appear together on a
+	# single line.
+	set found_close false
+	for { incr i } { $i < $num_lines } { incr i } {
+	    set line [lindex $lines $i]
+	    if { [regexp "^(?:\\{\\s*)?\\};" $line] } {
+		set found_close true
+		break
+	    }
+	}
+
+	if { !$found_close } {
+	    fail "$testname (no closing brace found)"
+	    continue
+	}
+
+	# Now scan forward from the line after the struct close, skipping
+	# empty lines and GDB-style /* ... */ comments.  The next non-blank,
+	# non-comment line should be the static_assert.
+	set in_comment false
+	set found_assert false
+	set found_other false
+	for { incr i } { $i < $num_lines } { incr i } {
+	    set line [lindex $lines $i]
+
+	    if { $in_comment } {
+		# Inside a multi-line comment, look for the closing "*/".
+		if { [regexp {\*/} $line] } {
+		    set in_comment false
+		}
+		continue
+	    }
+
+	    # Skip blank lines.
+	    if { [regexp {^\s*$} $line] } {
+		continue
+	    }
+
+	    # Check for the start of a comment.
+	    if { [regexp {^\s*/\*} $line] } {
+		# If the comment also ends on this line then we don't need
+		# to enter IN_COMMENT mode, we can just ignore this line.
+		if { ![regexp {\*/} $line] } {
+		    set in_comment true
+		}
+		continue
+	    }
+
+	    # This is a non-blank, non-comment line.  Check if it is the
+	    # expected static_assert.
+	    set expected \
+		"static_assert (gdb::is_python_allocatable_v<$struct_name>);"
+	    if { $line eq $expected } {
+		set found_assert true
+	    } else {
+		set found_other true
+	    }
+	    break
+	}
+
+	if { $found_assert } {
+	    pass $testname
+	} elseif { $found_other } {
+	    fail "$testname (missing static_assert)"
+	} else {
+	    fail "$testname (reached end of file)"
+	}
+    }
+}
+
+foreach file $files {
+    check_file $file
+}
-- 
2.25.4

Re: [PATCH 1/3] gdb/python: add type traits check for all PyObject sub-classes

[Tom Tromey]

>>>>> "Andrew" == Andrew Burgess <aburgess@redhat.com> writes:

Andrew> The problem is that Python is written in C, and PyObject_New doesn't
Andrew> call any constructors for `some_new_type`, nor for any of the fields
Andrew> within `some_new_type`.

Since working on the Python safety series, I have been wondering if we
could remedy this.  That is, give gdb's classes constructors and
destructors and arrange for these to be called in-place in tp_init /
whatever the destroy one is.

This way we could use idiomatic gdb code, which would be safer.

I was planning to investigate this more deeply once Matthew Longo's work
was done, since I didn't want to touch all the type objects and cause
conflicts.

Andrew> +
Andrew> +template <typename T>
Andrew> +struct is_python_allocatable {
Andrew> +  static constexpr bool value =
Andrew> +    std::is_trivially_destructible_v<T> &&
Andrew> +    std::is_trivially_copyable_v<T>;

Brace and operator placement.

Otherwise this looks good to me.

If the tp_init experiment works out I suppose we may end up reverting
this.  But IMO it's best not to wait on something that might not happen.

Approved-By: Tom Tromey <tom@tromey.com>

Tom

Re: [PATCH 2/3] gdb/python: fix use of frame_info_ptr within pending_frame_object

[Tom Tromey]

>>>>> "Andrew" == Andrew Burgess <aburgess@redhat.com> writes:

Andrew> The problem with frame_info_ptr is that its constructor registers the
Andrew> new frame_info_ptr with the global frame_list.  It is by this
Andrew> registration that invalidation of frame_info_ptr objects is performed.

Thanks for finding & fixing this.

Andrew> +  frame_info_ptr frame (*pending_frame->frame_info);
Andrew>    return PyUnicode_FromFormat ("<%s frame #%d, saved_regs=(%s)>",
Andrew>  			       gdbpy_py_obj_tp_name (self),
Andrew>  			       frame_relative_level (frame),

This could be 'const frame_info_ptr &frame (...)' and avoid some updates
to globals, since no copy will be done.

I didn't look closely but perhaps other spots could do the same.

It may not matter so if you want to ignore this that's fine by me.

Approved-By: Tom Tromey <tom@tromey.com>

Tom

Re: [PATCH 3/3] gdb/testsuite: add a test to check for Python traits static_assert

[Tom Tromey]

>>>>> "Andrew" == Andrew Burgess <aburgess@redhat.com> writes:

Andrew> The previous two commits added a new type trait which can be used
Andrew> within a static_assert to check the properties of a struct used by GDB
Andrew> to implement Python objects.

Andrew> The previous commit fixed a bug in GDB which this trait check exposed.

Andrew> This commit adds a new test gdb.gdb/python-traits-check.exp which
Andrew> checks that every struct in the Python/ directory that inherits from
Andrew> PyObject, has a suitable static_assert in place.

Andrew> Adding this test should mean that if someone adds a new Python object
Andrew> type to GDB, and they forget to add the static_assert, then this test
Andrew> should give a failure, which should remind them to add the required
Andrew> static_assert.  The static_assert will then check that their new
Andrew> struct is compliant.

With the "safety" work we can have the compiler check this; or if the
tp_init plan works out, I guess just drop the requirement entirely.

Approved-By: Tom Tromey <tom@tromey.com>

Tom

Re: [PATCH 1/3] gdb/python: add type traits check for all PyObject sub-classes

[Andrew Burgess]

Tom Tromey <tom@tromey.com> writes:

>>>>>> "Andrew" == Andrew Burgess <aburgess@redhat.com> writes:
>
> Andrew> The problem is that Python is written in C, and PyObject_New doesn't
> Andrew> call any constructors for `some_new_type`, nor for any of the fields
> Andrew> within `some_new_type`.
>
> Since working on the Python safety series, I have been wondering if we
> could remedy this.  That is, give gdb's classes constructors and
> destructors and arrange for these to be called in-place in tp_init /
> whatever the destroy one is.
>
> This way we could use idiomatic gdb code, which would be safer.
>
> I was planning to investigate this more deeply once Matthew Longo's work
> was done, since I didn't want to touch all the type objects and cause
> conflicts.
>
> Andrew> +
> Andrew> +template <typename T>
> Andrew> +struct is_python_allocatable {
> Andrew> +  static constexpr bool value =
> Andrew> +    std::is_trivially_destructible_v<T> &&
> Andrew> +    std::is_trivially_copyable_v<T>;
>
> Brace and operator placement.

Fixed.  Thanks.

>
> Otherwise this looks good to me.
>
> If the tp_init experiment works out I suppose we may end up reverting
> this.  But IMO it's best not to wait on something that might not
> happen.

ACK.  If/when you have a better solution I'm happy for this series, at
least the static_assert bits and the associated test, to be removed.
This was really just me trying to find a way to "test" the fix that is
in patch #2.  If we can come up with a solution that means we can use
C++ objects and the constructos/destructors "just work" then I'm 100% in
favour.

For now though, I've gone ahead and pushed this series.

Thanks,
Andrew

>
> Approved-By: Tom Tromey <tom@tromey.com>
>
> Tom

Re: [PATCH 2/3] gdb/python: fix use of frame_info_ptr within pending_frame_object

[Andrew Burgess]

Tom Tromey <tom@tromey.com> writes:

>>>>>> "Andrew" == Andrew Burgess <aburgess@redhat.com> writes:
>
> Andrew> The problem with frame_info_ptr is that its constructor registers the
> Andrew> new frame_info_ptr with the global frame_list.  It is by this
> Andrew> registration that invalidation of frame_info_ptr objects is performed.
>
> Thanks for finding & fixing this.
>
> Andrew> +  frame_info_ptr frame (*pending_frame->frame_info);
> Andrew>    return PyUnicode_FromFormat ("<%s frame #%d, saved_regs=(%s)>",
> Andrew>  			       gdbpy_py_obj_tp_name (self),
> Andrew>  			       frame_relative_level (frame),
>
> This could be 'const frame_info_ptr &frame (...)' and avoid some updates
> to globals, since no copy will be done.
>
> I didn't look closely but perhaps other spots could do the same.

In fact they all can change!  I made this update (and retested) before
pushing this patch.

Thanks,
Andrew

>
> It may not matter so if you want to ignore this that's fine by me.
>
> Approved-By: Tom Tromey <tom@tromey.com>
>
> Tom

Re: [PATCH 3/3] gdb/testsuite: add a test to check for Python traits static_assert

[Andrew Burgess]

Tom Tromey <tom@tromey.com> writes:

>>>>>> "Andrew" == Andrew Burgess <aburgess@redhat.com> writes:
>
> Andrew> The previous two commits added a new type trait which can be used
> Andrew> within a static_assert to check the properties of a struct used by GDB
> Andrew> to implement Python objects.
>
> Andrew> The previous commit fixed a bug in GDB which this trait check exposed.
>
> Andrew> This commit adds a new test gdb.gdb/python-traits-check.exp which
> Andrew> checks that every struct in the Python/ directory that inherits from
> Andrew> PyObject, has a suitable static_assert in place.
>
> Andrew> Adding this test should mean that if someone adds a new Python object
> Andrew> type to GDB, and they forget to add the static_assert, then this test
> Andrew> should give a failure, which should remind them to add the required
> Andrew> static_assert.  The static_assert will then check that their new
> Andrew> struct is compliant.
>
> With the "safety" work we can have the compiler check this; or if the
> tp_init plan works out, I guess just drop the requirement entirely.

ACK. As with patch #1, once there's a better way, and the static_asserts
are no longer of value, I'm happy for this patch to be reverted.  But
for now, while it adds some value, I've gone ahead and checked it in.

Thanks for reviewing this series,
Andrew


>
> Approved-By: Tom Tromey <tom@tromey.com>
>
> Tom