public inbox for gdb-patches@sourceware.org
 help / color / mirror / Atom feed
* [PATCH 0/1] Integrate GNU poke in GDB
@ 2021-05-10 15:10 Jose E. Marchesi
  2021-05-10 15:10 ` [PATCH 1/1] " Jose E. Marchesi
                   ` (2 more replies)
  0 siblings, 3 replies; 18+ messages in thread
From: Jose E. Marchesi @ 2021-05-10 15:10 UTC (permalink / raw)
  To: gdb-patches

Hi GDB people!

The patch below integrates GNU poke (http://jemarch.net/poke) in
GDB by mean of libpoke.

It allows the GDB user to execute Poke code from within the
debugger with access to the target memory, types and values.

A few notes:

- The poke support is optional, and built if specified with
  --enable-poke at configure time.

- The poke support requires libpoke from git master (which will
  become poke 2.x in a few months) since it uses some interfaces
  that are not available in the released poke 1.x series.

- Sorry for what is probably an atrocious mixture of C and C++
  idioms.  I avoid C++ like the pest, but I am all willing to try
  and write proper C++ for this integration.

- Eventually we will probably want to ship some prewritten Poke
  code in a pickle gdb.pk.  Would $pkddatadir/poke/ be a good
  location for Poke code distributed with GDB?

And a few questions:

- Where am I supposed to cleanup and shut down the incremental
  compiler when GDB exits?  I looked but I cannot find a
  counterpart to _initialize_FOO, like _finalize_FOO.

- There are many global parameters that can be configured in the
  poke incremental compiler: the numeration base used when
  printing values, the global endianness, a cut-off value for how
  many array elements are printed, and the like.  Reasonable
  defaults for them are set in `start_poke', but I would like the
  GDB user to be able to change these settings.  I could add a
  `poke-set SETTING [VALUE]' command, but I was wondering whether
  there is a better way, like a global register of settings
  already in GDB?

- I am using target_{read,write}_raw_memory to access the
  target's memory.  What is the difference between the raw and
  non-raw accessors?

- How can I demangle C++ identifiers?  And how can I detect when
  a given type is a C++ one that needs demangling?

How this stuff works:

- GDB links with libpoke.so and uses the interface in libpoke.h.
  This is also how the GNU poke application (the command-line
  editor) is implemented.

- There are three commands:

  poke STR
  poke-add-type EXPR
  poke-add-types REGEXP
  poke-dump-types

  All three commands make sure to start the poke incremental
  compiler if it isn't running already.

- Access to the target's memory is provided by GDB by installing
  a Foreign IO device in the incremental compiler.  This is
  `iod_if' in poke.c.

- Access to the terminal is provided by GDB by providing a
  pk_term_if implementation to the incremental compiler.  This is
  `poke_term_if' in poke.c.

- Access to GDB values is provided by GDB by installing an alien
  token handler in the incremental compiler.  This is
  `poke_alien_token_handler' in poke.c.

Jose E. Marchesi (1):
  Integrate GNU poke in GDB

 gdb/ChangeLog       |   8 +
 gdb/Makefile.in     |   6 +-
 gdb/configure       |  89 +++++-
 gdb/configure.ac    |  15 +
 gdb/doc/ChangeLog   |   6 +
 gdb/doc/Makefile.in |   1 +
 gdb/doc/gdb.texinfo |   4 +
 gdb/doc/poke.texi   | 373 ++++++++++++++++++++++
 gdb/poke.c          | 762 ++++++++++++++++++++++++++++++++++++++++++++
 9 files changed, 1257 insertions(+), 7 deletions(-)
 create mode 100644 gdb/doc/poke.texi
 create mode 100644 gdb/poke.c

-- 
2.25.0.2.g232378479e


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

* [PATCH 1/1] Integrate GNU poke in GDB
  2021-05-10 15:10 [PATCH 0/1] Integrate GNU poke in GDB Jose E. Marchesi
@ 2021-05-10 15:10 ` Jose E. Marchesi
  2021-05-10 16:56   ` Eli Zaretskii
                     ` (2 more replies)
  2021-05-10 18:39 ` [PATCH 0/1] " Simon Marchi
  2021-05-11 18:56 ` Tom Tromey
  2 siblings, 3 replies; 18+ messages in thread
From: Jose E. Marchesi @ 2021-05-10 15:10 UTC (permalink / raw)
  To: gdb-patches

This patch integrates GNU poke (http://jemarch.net/poke) in GDB by
mean of libpoke.  It allows the GDB user to execute Poke code from
within the debugger with access to the target memory, types and
values.

How this stuff works:

- GDB links with libpoke.so and uses the interface in libpoke.h.
  This is also how the GNU poke application (the command-line
  editor) is implemented.

- There are three commands:

  poke STR
  poke-add-type EXPR
  poke-add-types REGEXP
  poke-dump-types

  All three commands make sure to start the poke incremental
  compiler if it isn't running already.

- Access to the target's memory is provided by GDB by installing
  a Foreign IO device in the incremental compiler.  This is
  `iod_if' in poke.c.

- Access to the terminal is provided by GDB by providing a
  pk_term_if implementation to the incremental compiler.  This is
  `poke_term_if' in poke.c.

- Access to GDB values is provided by GDB by installing an alien
  token handler in the incremental compiler.  This is
  `poke_alien_token_handler' in poke.c.

gdb/ChangeLog:

2021-05-10  Jose E. Marchesi  <jose.marchesi@oracle.com>

	* configure.ac: Support --enable-poke.
	* configure: Regenerate.
	* Makefile.in (POKE_OBS): Define based on @POKE_OBS@.
	(DEPFILES): Add POKE_OBS.
	* poke.c: New file.

gdb/doc/ChangeLog:

2021-05-10  Jose E. Marchesi  <jose.marchesi@oracle.com>

	* Makefile.in (GDB_DOC_FILES): Add poke.texi.
	* poke.texi: New file.
	* gdb.texinfo (Data): Add meny entry for Poke and @include poke.texi.
---
 gdb/ChangeLog       |   8 +
 gdb/Makefile.in     |   6 +-
 gdb/configure       |  89 +++++-
 gdb/configure.ac    |  15 +
 gdb/doc/ChangeLog   |   6 +
 gdb/doc/Makefile.in |   1 +
 gdb/doc/gdb.texinfo |   4 +
 gdb/doc/poke.texi   | 373 ++++++++++++++++++++++
 gdb/poke.c          | 762 ++++++++++++++++++++++++++++++++++++++++++++
 9 files changed, 1257 insertions(+), 7 deletions(-)
 create mode 100644 gdb/doc/poke.texi
 create mode 100644 gdb/poke.c

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 70d4972cd52..00f9a39f9f4 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,11 @@
+2021-05-10  Jose E. Marchesi  <jose.marchesi@oracle.com>
+
+	* configure.ac: Support --enable-poke.
+	* configure: Regenerate.
+	* Makefile.in (POKE_OBS): Define based on @POKE_OBS@.
+	(DEPFILES): Add POKE_OBS.
+	* poke.c: New file.
+
 2021-05-07  Tom de Vries  <tdevries@suse.de>
 
 	PR symtab/26327
diff --git a/gdb/Makefile.in b/gdb/Makefile.in
index ba0cabb0216..7085f1196d2 100644
--- a/gdb/Makefile.in
+++ b/gdb/Makefile.in
@@ -670,6 +670,9 @@ SER_HARDWIRE = @SER_HARDWIRE@
 # This is remote-sim.o if a simulator is to be linked in.
 SIM_OBS = @SIM_OBS@
 
+# Object files to integrate with GNU poke.
+POKE_OBS = @POKE_OBS@
+
 # Target-dependent object files.
 TARGET_OBS = @TARGET_OBS@
 
@@ -1570,7 +1573,8 @@ HFILES_WITH_SRCDIR = \
 # variables analogous to SER_HARDWIRE which get defaulted in this
 # Makefile.in
 
-DEPFILES = $(TARGET_OBS) $(SER_HARDWIRE) $(NATDEPFILES) $(SIM_OBS)
+DEPFILES = $(TARGET_OBS) $(SER_HARDWIRE) $(NATDEPFILES) $(SIM_OBS) \
+           $(POKE_OBS)
 
 SOURCES = $(SFILES) $(ALLDEPFILES) $(YYFILES) $(CONFIG_SRCS)
 # Don't include YYFILES (*.c) because we already include *.y in SFILES,
diff --git a/gdb/configure b/gdb/configure
index b47de77bf64..909935ffece 100755
--- a/gdb/configure
+++ b/gdb/configure
@@ -748,6 +748,7 @@ LTLIBICONV
 LIBICONV
 zlibinc
 zlibdir
+POKE_OBS
 MIG
 WINDRES
 DLLTOOL
@@ -846,6 +847,7 @@ infodir
 docdir
 oldincludedir
 includedir
+runstatedir
 localstatedir
 sharedstatedir
 sysconfdir
@@ -888,6 +890,7 @@ enable_profiling
 enable_codesign
 with_pkgversion
 with_bugurl
+enable_poke
 with_system_zlib
 with_gnu_ld
 enable_rpath
@@ -995,6 +998,7 @@ datadir='${datarootdir}'
 sysconfdir='${prefix}/etc'
 sharedstatedir='${prefix}/com'
 localstatedir='${prefix}/var'
+runstatedir='${localstatedir}/run'
 includedir='${prefix}/include'
 oldincludedir='/usr/include'
 docdir='${datarootdir}/doc/${PACKAGE}'
@@ -1247,6 +1251,15 @@ do
   | -silent | --silent | --silen | --sile | --sil)
     silent=yes ;;
 
+  -runstatedir | --runstatedir | --runstatedi | --runstated \
+  | --runstate | --runstat | --runsta | --runst | --runs \
+  | --run | --ru | --r)
+    ac_prev=runstatedir ;;
+  -runstatedir=* | --runstatedir=* | --runstatedi=* | --runstated=* \
+  | --runstate=* | --runstat=* | --runsta=* | --runst=* | --runs=* \
+  | --run=* | --ru=* | --r=*)
+    runstatedir=$ac_optarg ;;
+
   -sbindir | --sbindir | --sbindi | --sbind | --sbin | --sbi | --sb)
     ac_prev=sbindir ;;
   -sbindir=* | --sbindir=* | --sbindi=* | --sbind=* | --sbin=* \
@@ -1384,7 +1397,7 @@ fi
 for ac_var in	exec_prefix prefix bindir sbindir libexecdir datarootdir \
 		datadir sysconfdir sharedstatedir localstatedir includedir \
 		oldincludedir docdir infodir htmldir dvidir pdfdir psdir \
-		libdir localedir mandir
+		libdir localedir mandir runstatedir
 do
   eval ac_val=\$$ac_var
   # Remove trailing slashes.
@@ -1537,6 +1550,7 @@ Fine tuning of the installation directories:
   --sysconfdir=DIR        read-only single-machine data [PREFIX/etc]
   --sharedstatedir=DIR    modifiable architecture-independent data [PREFIX/com]
   --localstatedir=DIR     modifiable single-machine data [PREFIX/var]
+  --runstatedir=DIR       modifiable per-process data [LOCALSTATEDIR/run]
   --libdir=DIR            object code libraries [EPREFIX/lib]
   --includedir=DIR        C header files [PREFIX/include]
   --oldincludedir=DIR     C header files for non-gcc [/usr/include]
@@ -1591,6 +1605,7 @@ Optional Features:
   --enable-gdbtk          enable gdbtk graphical user interface (GUI)
   --enable-profiling      enable profiling of GDB
   --enable-codesign=CERT  sign gdb with 'codesign -s CERT'
+  --enable-poke           Build GDB with poke support (default is NO)
   --disable-rpath         do not hardcode runtime library paths
   --enable-source-highlight
                           enable source-highlight for source listings
@@ -4852,7 +4867,7 @@ else
     We can't simply define LARGE_OFF_T to be 9223372036854775807,
     since some C++ compilers masquerading as C compilers
     incorrectly reject 9223372036854775807.  */
-#define LARGE_OFF_T (((off_t) 1 << 62) - 1 + ((off_t) 1 << 62))
+#define LARGE_OFF_T ((((off_t) 1 << 31) << 31) - 1 + (((off_t) 1 << 31) << 31))
   int off_t_is_large[(LARGE_OFF_T % 2147483629 == 721
 		       && LARGE_OFF_T % 2147483647 == 1)
 		      ? 1 : -1];
@@ -4898,7 +4913,7 @@ else
     We can't simply define LARGE_OFF_T to be 9223372036854775807,
     since some C++ compilers masquerading as C compilers
     incorrectly reject 9223372036854775807.  */
-#define LARGE_OFF_T (((off_t) 1 << 62) - 1 + ((off_t) 1 << 62))
+#define LARGE_OFF_T ((((off_t) 1 << 31) << 31) - 1 + (((off_t) 1 << 31) << 31))
   int off_t_is_large[(LARGE_OFF_T % 2147483629 == 721
 		       && LARGE_OFF_T % 2147483647 == 1)
 		      ? 1 : -1];
@@ -4922,7 +4937,7 @@ rm -f core conftest.err conftest.$ac_objext conftest.$ac_ext
     We can't simply define LARGE_OFF_T to be 9223372036854775807,
     since some C++ compilers masquerading as C compilers
     incorrectly reject 9223372036854775807.  */
-#define LARGE_OFF_T (((off_t) 1 << 62) - 1 + ((off_t) 1 << 62))
+#define LARGE_OFF_T ((((off_t) 1 << 31) << 31) - 1 + (((off_t) 1 << 31) << 31))
   int off_t_is_large[(LARGE_OFF_T % 2147483629 == 721
 		       && LARGE_OFF_T % 2147483647 == 1)
 		      ? 1 : -1];
@@ -4967,7 +4982,7 @@ else
     We can't simply define LARGE_OFF_T to be 9223372036854775807,
     since some C++ compilers masquerading as C compilers
     incorrectly reject 9223372036854775807.  */
-#define LARGE_OFF_T (((off_t) 1 << 62) - 1 + ((off_t) 1 << 62))
+#define LARGE_OFF_T ((((off_t) 1 << 31) << 31) - 1 + (((off_t) 1 << 31) << 31))
   int off_t_is_large[(LARGE_OFF_T % 2147483629 == 721
 		       && LARGE_OFF_T % 2147483647 == 1)
 		      ? 1 : -1];
@@ -4991,7 +5006,7 @@ rm -f core conftest.err conftest.$ac_objext conftest.$ac_ext
     We can't simply define LARGE_OFF_T to be 9223372036854775807,
     since some C++ compilers masquerading as C compilers
     incorrectly reject 9223372036854775807.  */
-#define LARGE_OFF_T (((off_t) 1 << 62) - 1 + ((off_t) 1 << 62))
+#define LARGE_OFF_T ((((off_t) 1 << 31) << 31) - 1 + (((off_t) 1 << 31) << 31))
   int off_t_is_large[(LARGE_OFF_T % 2147483629 == 721
 		       && LARGE_OFF_T % 2147483647 == 1)
 		      ? 1 : -1];
@@ -8188,6 +8203,68 @@ if test "$ac_res" != no; then :
 fi
 
 
+# Integration with GNU poke is done through the libpoke library.
+# Check whether --enable-poke was given.
+if test "${enable_poke+set}" = set; then :
+  enableval=$enable_poke; poke_enabled=$enableval
+else
+  poke_enabled=no
+fi
+
+if test "x$poke_enabled" = "xyes"; then
+  # Note that we need a libpoke with support for registering foreign
+  # IO devices, hence the symbol pk_register_iod.
+  { $as_echo "$as_me:${as_lineno-$LINENO}: checking for pk_register_iod in -lpoke" >&5
+$as_echo_n "checking for pk_register_iod in -lpoke... " >&6; }
+if ${ac_cv_lib_poke_pk_register_iod+:} false; then :
+  $as_echo_n "(cached) " >&6
+else
+  ac_check_lib_save_LIBS=$LIBS
+LIBS="-lpoke  $LIBS"
+cat confdefs.h - <<_ACEOF >conftest.$ac_ext
+/* end confdefs.h.  */
+
+/* Override any GCC internal prototype to avoid an error.
+   Use char because int might match the return type of a GCC
+   builtin and then its argument prototype would still apply.  */
+#ifdef __cplusplus
+extern "C"
+#endif
+char pk_register_iod ();
+int
+main ()
+{
+return pk_register_iod ();
+  ;
+  return 0;
+}
+_ACEOF
+if ac_fn_c_try_link "$LINENO"; then :
+  ac_cv_lib_poke_pk_register_iod=yes
+else
+  ac_cv_lib_poke_pk_register_iod=no
+fi
+rm -f core conftest.err conftest.$ac_objext \
+    conftest$ac_exeext conftest.$ac_ext
+LIBS=$ac_check_lib_save_LIBS
+fi
+{ $as_echo "$as_me:${as_lineno-$LINENO}: result: $ac_cv_lib_poke_pk_register_iod" >&5
+$as_echo "$ac_cv_lib_poke_pk_register_iod" >&6; }
+if test "x$ac_cv_lib_poke_pk_register_iod" = xyes; then :
+  cat >>confdefs.h <<_ACEOF
+#define HAVE_LIBPOKE 1
+_ACEOF
+
+  LIBS="-lpoke $LIBS"
+
+fi
+
+  POKE_OBS="poke.o"
+else
+  POKE_OBS=
+fi
+
+
 # Link in zlib if we can.  This allows us to read compressed debug sections.
 
   # Use the system's zlib library.
diff --git a/gdb/configure.ac b/gdb/configure.ac
index acfbe4a9a6c..ea149651419 100644
--- a/gdb/configure.ac
+++ b/gdb/configure.ac
@@ -500,6 +500,21 @@ AC_SEARCH_LIBS(gethostbyname, nsl)
 # Some systems (e.g. Solaris) have `socketpair' in libsocket.
 AC_SEARCH_LIBS(socketpair, socket)
 
+# Integration with GNU poke is done through the libpoke library.
+AC_ARG_ENABLE([poke],
+              AS_HELP_STRING([--enable-poke],
+                             [Build GDB with poke support (default is NO)]),
+              [poke_enabled=$enableval], [poke_enabled=no])
+if test "x$poke_enabled" = "xyes"; then
+  # Note that we need a libpoke with support for registering foreign
+  # IO devices, hence the symbol pk_register_iod.
+  AC_CHECK_LIB(poke, pk_register_iod)
+  POKE_OBS="poke.o"
+else
+  POKE_OBS=
+fi
+AC_SUBST(POKE_OBS)
+
 # Link in zlib if we can.  This allows us to read compressed debug sections.
 AM_ZLIB
 
diff --git a/gdb/doc/ChangeLog b/gdb/doc/ChangeLog
index a56735896c5..db4e97c8ad1 100644
--- a/gdb/doc/ChangeLog
+++ b/gdb/doc/ChangeLog
@@ -1,3 +1,9 @@
+2021-05-10  Jose E. Marchesi  <jose.marchesi@oracle.com>
+
+	* Makefile.in (GDB_DOC_FILES): Add poke.texi.
+	* poke.texi: New file.
+	* gdb.texinfo (Data): Add meny entry for Poke and @include poke.texi.
+
 2021-05-07  Tom de Vries  <tdevries@suse.de>
 
 	PR symtab/26327
diff --git a/gdb/doc/Makefile.in b/gdb/doc/Makefile.in
index 13b288d3e42..457c332e46d 100644
--- a/gdb/doc/Makefile.in
+++ b/gdb/doc/Makefile.in
@@ -138,6 +138,7 @@ GDB_DOC_FILES = \
 	$(srcdir)/gdb.texinfo \
 	$(srcdir)/guile.texi \
 	$(srcdir)/python.texi \
+        $(srcdir)/poke.texi \
 	$(GDB_DOC_SOURCE_INCLUDES) \
 	$(GDB_DOC_BUILD_INCLUDES)
 
diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
index f4d7085da58..06aaac2598a 100644
--- a/gdb/doc/gdb.texinfo
+++ b/gdb/doc/gdb.texinfo
@@ -10245,6 +10245,7 @@ being passed the type of @var{arg} as the argument.
 * Caching Target Data::         Data caching for targets
 * Searching Memory::            Searching memory for a sequence of bytes
 * Value Sizes::                 Managing memory allocated for values
+* Poke::                        Poking at data using GNU poke.
 @end menu
 
 @node Expressions
@@ -13785,6 +13786,9 @@ Show the maximum size of memory, in bytes, that @value{GDBN} will
 allocate for the contents of a value.
 @end table
 
+@c Poke docs live in a separate file.
+@include poke.texi
+
 @node Optimized Code
 @chapter Debugging Optimized Code
 @cindex optimized code, debugging
diff --git a/gdb/doc/poke.texi b/gdb/doc/poke.texi
new file mode 100644
index 00000000000..d30dd88a141
--- /dev/null
+++ b/gdb/doc/poke.texi
@@ -0,0 +1,373 @@
+@c Copyright (C) 2021 Free Software Foundation, Inc.
+@c Permission is granted to copy, distribute and/or modify this document
+@c under the terms of the GNU Free Documentation License, Version 1.3 or
+@c any later version published by the Free Software Foundation; with the
+@c Invariant Sections being ``Free Software'' and ``Free Software Needs
+@c Free Documentation'', with the Front-Cover Texts being ``A GNU Manual,''
+@c and with the Back-Cover Texts as in (a) below.
+@c
+@c (a) The FSF's Back-Cover Text is: ``You are free to copy and modify
+@c this GNU Manual.  Buying copies from GNU Press supports the FSF in
+@c developing GNU and promoting software freedom.''
+
+@c XXX example: saving some memory in a file (opening other IO spaces)
+
+@node Poke
+@section Poking at data using GNU poke
+@cindex GNU poke
+@cindex poke
+
+@uref{http://jemarch.net/poke.html,GNU poke} is an interactive,
+extensible editor for binary data that implements a full-fledged
+procedural, interactive domain specific language called @dfn{Poke}
+(with big P) that is specifically designed in order to describe the
+layout of structured binary data and to operate on it.
+
+@value{GDBN} integrates with GNU poke by mean of the @file{libpoke}
+library and offers the possibility of executing Poke code from within
+the debugger, to inspect and modify data in the target's memory.  This
+feature is available only if @value{GDBN} was configured using
+@option{--enable-poke}.
+
+As we shall see in the sections below, @value{GDBN} uses the
+integration mechanisms provided by @file{libpoke} in order to make
+certain GDB abstractions (such as symbols and types) visible from the
+Poke side, making the integration bidirectional.
+
+Note that this section documents the integration of GNU poke and
+@value{GDBN} and how to use it, but it doesn't describe GNU poke nor
+the Poke language in detail. @xref{Top,,, poke, The GNU poke Manual}
+for more information.
+
+@menu
+* The @command{poke} Command::  Executing Poke from @value{GDBN}.
+* Poking the Target Memory::    Accessing the target's memory from Poke.
+* @value{GDBN} Types and Poke::         Accessing @value{GDBN} types from Poke.
+* @value{GDBN} Values and Poke::        Accessing @value{GDBN} values from Poke.
+@end menu
+
+@node The @command{poke} Command
+@subsection The @command{poke} Command
+
+The @command{poke} command allows to execute arbitrary Poke code from
+the @value{GDBN} prompt.
+
+@table @code
+@item poke @var{src}
+@var{src} is either a Poke expression, statement or definition.
+@end table
+
+For example, this executes a simple Poke expression and shows the
+result:
+
+@smallexample
+(@value{GDBP}) poke 2 + 3UL
+0x5UL
+@end smallexample
+
+This declares a couple of Poke types and a variable:
+
+@smallexample
+(@value{GDBP}) type byte = uint<8>
+(@value{GDBP}) type Packet = struct @{ byte magic == 0x4a; byte sz; byte[sz] payload; @}
+(@value{GDBP}) poke Packet @{ sz = 4 @}
+Packet @{
+  magic=0x4aUB,
+  sz=0x4UB,
+  payload=[0x0UB,0x0UB,0x0UB,0x0UB]
+@}
+(@value{GDBP}) var p = Packet @{ sz = 4 @}
+(@value{GDBP}) poke p.payload
+[0x0UB,0x0UB,0x0UB,0x0UB]
+@end smallexample
+
+This executes a Poke statement:
+
+@smallexample
+(@value{GDBP}) poke for (i in [1,2,3]) printf "%v\n", i
+0x00000001
+0x00000002
+0x00000003
+@end smallexample
+
+This shows how the Poke incremental compiler handles and reports
+invalid input:
+
+@smallexample
+(@value{GDBP}) poke 2 + fjsdio
+<stdin>:1:5: error: undefined variable 'fjsdio'
+2 + fjsdio;
+    ^~~~~~
+@end smallexample
+
+The standard @command{load} Poke directive loads a Poke source file
+and executes it in the incremental compiler.  The list of directories
+where @command{load} looks for files is in the variable
+@code{load_path}:
+
+@smallexample
+(@value{GDBP}) poke load_path
+".:/home/jemarch/.local/share/poke:%DATADIR%/pickles:%DATADIR%"
+@end smallexample
+
+This loads a file @dfn{foo.pk} if it is found in the load path:
+
+@smallexample
+(@value{GDBP}) poke load foo
+@end smallexample
+
+To Poke source files containing definitions that conceptually apply to
+some definite domain, we call them @dfn{pickles}.  For example,
+@file{elf.pk} is a pickle that provides facilities to poke ELF object
+files.  The GNU poke editor comes with lots of already written pickles
+for many file formats and other domains.  If you happen to have GNU
+poke installed (and not just @file{libpoke}) you can also use the many
+pickles distributed with the editor.  For example:
+
+@smallexample
+(@value{GDBP}) poke load "std-types.pk"
+(@value{GDBP}) poke load elf
+(@value{GDBP}) poke Elf64_Rela @{@}
+Elf64_Rela @{
+  r_offset=0x0UL#B,
+  r_info=Elf64_RelInfo @{
+    r_sym=0x0U,
+    r_type=0x0U
+  @},
+  r_addend=0x0L
+@}
+@end smallexample
+
+The reason why @file{std-types.pk} has to be loaded before
+@file{elf.pk} is explained later in this manual.
+
+@node Poking the Target Memory
+@subsection Poking the Target Memory
+
+@value{GDBN} configures @file{libpoke} to access the target's memory
+as an IO space device called @code{<gdb>}, which is automatically
+opened when the poke incremental compiler is started.
+
+This means that the default IO space in the running poke will provide
+access to the virtual address space of the current @value{GDBN}
+inferior.
+
+For example, suppose that a string table is at offset 0x5ff0 bytes in
+the target's memory.  We could map an array of Poke strings from it by
+issuing:
+
+@smallexample
+(@value{GDBP}) spoke string[3] @@ 0x5ff0#B
+["int", "long", "_pid"]
+@end smallexample
+
+And we can write to the target's memory:
+
+@smallexample
+(@value{GDBP}) poke string[] @@ 0x5ff0#B = ["foo", "bar", "baz"]
+@end smallexample
+
+Note that the fact the current IO space is the GDB target memory
+doesn't mean you cannot access other IO spaces.  This is how you would
+write the string table above to a file @file{strtab.out}:
+
+@smallexample
+(@value{GDBP}) poke var f = open ("strtab.out", IOS_F_WRITE | IOS_F_CREATE)
+(@value{GDBP}) poke string[] @@ f : 0#B = string[3] @@ 0x5ff0#B
+(@value{GDBP}) poke close (f)
+@end smallexample
+
+If you close the default IO space you can re-open the GDB target space
+with @code{open ("<gdb>")}.
+
+@node @value{GDBN} Types and Poke
+@subsection @value{GDBN} Types and Poke
+
+Maybe the strongest side of the Poke language is that it provides a
+very rich and dynamic mechanism to describe the layout of data
+structures.  This is done by defining @dfn{Poke types}.
+
+For example, this is the definition of a signed 13-bit integral type
+that could be used to poke immediate fields in SPARC instructions:
+
+@smallexample
+type simm13 = int<13>;
+@end smallexample
+
+And this is a simplified version of the structure of a 64-bit ELF file
+showing more advanced Poke capabilities like field constraints, field
+labels, absent fields, and methods:
+
+@smallexample
+type Elf64_File =
+  struct
+  @{
+    Elf64_Ehdr ehdr : ehdr.e_ident.ei-mag == [0x7fUB, 'E', 'L', 'F'];
+
+    Elf64_Shdr[ehdr.e_shnum] shdr @@ ehdr.e_shoff
+      if ehdr.e_shnum > 0;
+
+    Elf64_Phdr[ehdr.e_phnum] phdr @@ ehdr.e_phoff
+      if ehdr.e_phnum > 0;
+
+    /* Given an offset into the ELF file's section string table, return
+       the string.  */
+
+    method get_section_name = (offset<Elf_Word,B> offset) string:
+      @{
+        var strtab = ehdr.e_shstrndx;
+        return string @@ (shdr[strtab].sh_offset + offset);
+      @}
+  @};
+@end smallexample
+
+This is all good and well for GNU poke as a standalone binary editor,
+but when it comes to @value{GDBN} we want to poke at data structures
+in the target memory of the debugged program.  These structures are
+described by language-specific types, which @value{GDBN} abstracts as
+@value{GDBN} types, not Poke types.
+
+For example, say we are debugging a C program that contains the
+following type:
+
+@smallexample
+struct person
+@{
+  int age;
+  char *name;
+  char *postal_address;
+@};
+@end smallexample
+
+If we wanted to poke at a struct person from poke, we would need to
+write a Poke struct type that is equivalent to that C type.  This is
+often not trivial, because the physical layout of data structures is
+almost always not well defined in programming languages.
+
+Fortunately, @value{GDBN} provides a few commands to translate
+@value{GDBN} types to Poke types and inspect them.
+
+@table @code
+@item poke-add-type @var{expr}
+@var{expr} is a @value{GDBN} expression that must evaluate to a type.
+
+Translate a @value{GDBN} type to Poke and define it in the running
+poke incremental compiler.  If the given type depends on other types
+that are not known to poke, add these as well.
+
+Types for which @value{GDBN} doesn't know how to create a Poke
+equivalence are simply ignored.
+
+@item poke-add-types @var{regexp}
+@var{regexp} is a regular expression.
+
+Translate all known types whose name matches @var{regexp} to Poke and
+define them in the running poke incremental compiler.  If the matched
+types depend on other types that are not known to poke, add these as
+well.
+
+Types for which @value{GDBN} doesn't know how to create a Poke
+equivalence are simply ignored.
+
+@item poke-dump-types
+Dump the Poke definition of all translated types, one definition per
+line.
+@end table
+
+Using these commands, we can add a type for the @code{struct person} C
+type above like this:
+
+@smallexample
+(@value{GDBN}) poke-add-type struct person
+added type int
+added type struct_person
+@end smallexample
+
+Note how two types are added: the requested @code{struct person} and
+also @code{int}, since the struct contains a field of that basic C
+type. Let's take a look to the type definitions:
+
+@smallexample
+(@value{GDBN}) poke-dump-types
+type int = int<32>;
+type struct_person = struct @{int age; offset<uint<64>,B> name @@ 8#B; offset<uint<64>,B> postal_address;@};
+@end smallexample
+
+If now we want to access a given variable of type @code{struct person}
+in the current target, we just use the created Poke types:
+
+@smallexample
+(@value{GDBN}) poke struct_person @@ 0xf00e#B
+struct_person @{
+  age=0x28,
+  name=0x5555555547b4UL#B,
+  postal_address=0x5555555547c5UL#B
+@}
+(@value{GDBN}) poke string @@ (struct_person @@ 0xf00e#B).postal_address
+"Foo Street number 13"
+@end smallexample
+
+If we wanted to add all the types known to GDB to poke, we could so do
+by:
+
+@smallexample
+(@value{GDBN}) poke-add-types .*
+@end smallexample
+
+The @command{poke-dump-types} is useful to generate Poke files with
+type definitions to be used in GNU poke, like this:
+
+@smallexample
+$ gdb -batch -ex poke-dump-types -ex quit foo.so > foo-types.pk
+@end smallexample
+
+@node @value{GDBN} Values and Poke
+@subsection @value{GDBN} Values and Poke
+
+Poke variables are not the same than GDB symbols, and live in a
+separated world of their own. However, it is possible to refer to GDB
+values by using the @code{$IDENTIFIER} notation in Poke programs.
+
+Consider for example a C program with the following variable:
+
+@smallexample
+short counter;
+@end smallexample
+
+In @value{GDBN} we can access to the value of that variable like this:
+
+@smallexample
+(@value{GDBN}) counter
+$1 = 0
+@end smallexample
+
+And from the poke side:
+
+@smallexample
+(@value{GDBN}) poke $counter
+0x0H
+@end smallexample
+
+Note how the @value{GDBN} value is visible using the right type, in
+the case above a signed 16-bit integer.  If we accessed a C value of a
+pointer type, like @code{char *str;}, we would get an offset with unit
+bytes instead:
+
+@smallexample
+(@value{GDBN}) poke $str
+0x0UL#B
+@end smallexample
+
+Since many @value{GDBN} values are pointers, it is possible to access
+to the address of a value by using the @code{$addr::IDENTIFIER}
+notation.  For example, given the C @code{struct person} defined above
+and a variable @code{struct person jemarch;}:
+
+@smallexample
+(@value{GDBN}) poke struct_person @@ $addr::jemarch
+struct_person @{
+  age=0x28,
+  name=0x5555555547b4UL#B,
+  postal_address=0x5555555547c5UL#B
+@}
+@end smallexample
diff --git a/gdb/poke.c b/gdb/poke.c
new file mode 100644
index 00000000000..31b8460ad1d
--- /dev/null
+++ b/gdb/poke.c
@@ -0,0 +1,762 @@
+/* GDB integration with GNU poke.
+
+   Copyright (C) 2021 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 "command.h"
+#include "arch-utils.h"
+#include "target.h"
+extern "C" {
+#include <libpoke.h>
+}
+#include <ctype.h>
+#include <vector>
+#include <algorithm>
+
+/* Global poke incremental compiler.  */
+
+static pk_compiler poke_compiler;
+static bool poke_compiler_lives = false;
+
+/* Global vector of the Poke code used to define types.  This is
+   filled in by poke_add_type and used by poke_dump_types.  */
+
+static std::vector<std::string> type_poke_strings;
+
+/* Terminal hook that flushes the terminal.  */
+
+static void
+poke_term_flush (void)
+{
+  /* Do nothing here.  */
+}
+
+/* Terminal hook that prints a fixed string.  */
+
+static void
+poke_puts (const char *str)
+{
+  printf_filtered ("%s", str);
+}
+
+/* Terminal hook that prints a formatted string.  */
+
+__attribute__ ((__format__ (__printf__, 1, 2)))
+static void
+poke_printf (const char *format, ...)
+{
+  va_list ap;
+  char *str;
+  int r;
+
+  va_start (ap, format);
+  r = vasprintf (&str, format, ap);
+  if (r == -1)
+    error (_("out of memory in vasprintf")); /* XXX fatal */
+  va_end (ap);
+
+  printf_filtered ("%s", str);
+  free (str);
+}
+
+/* Terminal hook that indents to a given level.  */
+
+static void
+poke_term_indent (unsigned int lvl, unsigned int step)
+{
+  printf_filtered ("\n%*s", (step * lvl), "");
+}
+
+/* Terminal hook that starts a styling class.  */
+
+static void
+poke_term_class (const char *class_name)
+{
+  /* Do nothing here.  */
+}
+
+/* Terminal hook that finishes a styling class.  */
+
+static int
+poke_term_end_class (const char *class_name)
+{
+  /* Just report success.  */
+  return 1;
+}
+
+/* Terminal hook that starts a terminal hyperlink.  */
+
+static void
+poke_term_hyperlink (const char *url, const char *id)
+{
+  /* Do nothing here.  */
+}
+
+/* Terminal hook that finishes a terminal hyperlink.  */
+
+static int
+poke_term_end_hyperlink (void)
+{
+  /* Just report success.  */
+  return 1;
+}
+
+/* Terminal hook that returns the current terminal foreground
+   color.  */
+
+static struct pk_color
+poke_term_get_color (void)
+{
+  /* Just return the default foreground color.  */
+  struct pk_color dfl = {-1,-1,-1};
+  return dfl;
+}
+
+/* Terminal hook that returns the current terminal background
+   color.  */
+
+static struct pk_color
+poke_term_get_bgcolor (void)
+{
+  /* Just return the default background color.  */
+  struct pk_color dfl = {-1,-1,-1};
+  return dfl;
+}
+
+/* Terminal hook that sets the terminal foreground color.  */
+
+static void
+poke_term_set_color (struct pk_color color)
+{
+  /* Do nothing.  */
+}
+
+/* Terminal hook that sets the terminal background color.  */
+
+static void
+poke_term_set_bgcolor (struct pk_color color)
+{
+  /* Do nothing.  */
+}
+
+/* Implementation of the poke terminal interface, that uses the hooks
+   defined above.  */
+
+static struct pk_term_if poke_term_if =
+  {
+    .flush_fn = poke_term_flush,
+    .puts_fn = poke_puts,
+    .printf_fn = poke_printf,
+    .indent_fn = poke_term_indent,
+    .class_fn = poke_term_class,
+    .end_class_fn = poke_term_end_class,
+    .hyperlink_fn = poke_term_hyperlink,
+    .end_hyperlink_fn = poke_term_end_hyperlink,
+    .get_color_fn = poke_term_get_color,
+    .get_bgcolor_fn = poke_term_get_bgcolor,
+    .set_color_fn = poke_term_set_color,
+    .set_bgcolor_fn = poke_term_set_bgcolor,
+  };
+
+/* Foreign IO device hook that returns an unique name identifying the
+   kind of device.  */
+
+static const char *
+iod_get_if_name (void)
+{
+  return "GDB";
+}
+
+/* Foreign IO device hook that recognizes whether a given IO space
+   handler refer to this kind of device, and normalizes it for further
+   use.  */
+
+static char *
+iod_handler_normalize (const char *handler, uint64_t flags, int *error)
+{
+  char *new_handler = NULL;
+
+  if (strcmp (handler, "<gdb>") == 0)
+    new_handler = xstrdup (handler);
+  if (error)
+    *error = PK_IOD_OK;
+
+  return new_handler;
+}
+
+/* Foreign IO device hook that opens a new device.  */
+
+static int iod_opened_p = 0;
+
+static void *
+iod_open (const char *handler, uint64_t flags, int *error)
+{
+  iod_opened_p = 1;
+  return &iod_opened_p;
+}
+
+/* Foreign IO device hook that reads data from a device.  */
+
+static int
+iod_pread (void *dev, void *buf, size_t count, pk_iod_off offset)
+{
+  int ret = target_read_raw_memory (offset, (gdb_byte *) buf, count);
+  return ret == -1 ? PK_IOD_ERROR : PK_IOD_OK;
+}
+
+/* Foreign IO device hook that writes data to a device.  */
+
+static int
+iod_pwrite (void *dev, const void *buf, size_t count, pk_iod_off offset)
+{
+  int ret = target_write_raw_memory (offset, (gdb_byte *) buf, count);
+  return ret == -1 ? PK_IOD_ERROR : PK_IOD_OK;
+}
+
+/* Foreign IO device hook that returns the flags of an IO device. */
+
+static uint64_t
+iod_get_flags (void *dev)
+{
+  return PK_IOS_F_READ | PK_IOS_F_WRITE;
+}
+
+/* Foreign IO device hook that returns the size of an IO device, in
+   bytes.  */
+
+static pk_iod_off
+iod_size (void *dev)
+{
+  return (gdbarch_addr_bit (get_current_arch ()) == 32
+	  ? 0xffffffff : 0xffffffffffffffff);
+}
+
+/* Foreign IO device hook that flushes an IO device.  */
+
+static int
+iod_flush (void *dev, pk_iod_off offset)
+{
+  /* Do nothing here.  */
+  return PK_OK;
+}
+
+/* Foreign IO device hook that closes a given device.  */
+
+static int
+iod_close (void *dev)
+{
+  iod_opened_p = 0;
+  return PK_OK;
+}
+
+/* Implementation of the poke foreign IO device interface, that uses
+   the hooks defined above.  */
+
+static struct pk_iod_if iod_if =
+  {
+    iod_get_if_name,
+    iod_handler_normalize,
+    iod_open,
+    iod_close,
+    iod_pread,
+    iod_pwrite,
+    iod_get_flags,
+    iod_size,
+    iod_flush
+  };
+
+/* Handler for alien tokens.  */
+
+static struct pk_alien_token alien_token;
+
+static struct pk_alien_token *
+poke_alien_token_handler (const char *id, char **errmsg)
+{
+  /* In GDB alien poke tokens with the form $addr::FOO provide the
+     address of the symbol `FOO' as an offset in bytes, i.e. it
+     resolves to the GDB value &foo as a Poke offset with unit bytes.
+
+     $FOO, on the other hand, provide the value of the symbol FOO
+     incarnated in a proper Poke value.  */
+
+  if (strncmp (id, "addr::", 6) == 0)
+    {
+      CORE_ADDR addr;
+
+      std::string expr = "&";
+      expr += id + 6;
+
+      try
+	{
+	  addr = parse_and_eval_address (expr.c_str ());
+	}
+      catch (const gdb_exception_error &except)
+	{
+	  goto error;
+	}
+
+      alien_token.kind = PK_ALIEN_TOKEN_OFFSET;
+      alien_token.value.offset.magnitude = addr;
+      alien_token.value.offset.width = 64;
+      alien_token.value.offset.signed_p = 0;
+      alien_token.value.offset.unit = 8;
+    }
+  else
+    {
+      struct value *value;
+
+      try
+	{
+	  value = parse_and_eval (id);
+	}
+      catch (const gdb_exception_error &except)
+	{
+	  goto error;
+	}
+
+      struct type *type = value_type (value);
+
+      if (can_dereference (type))
+	{
+	  alien_token.kind = PK_ALIEN_TOKEN_OFFSET;
+	  alien_token.value.offset.magnitude
+	    = value_as_address (value);
+	  alien_token.value.offset.width
+	    = TYPE_LENGTH (type) * 8;
+	  alien_token.value.offset.signed_p = 0;
+	  alien_token.value.offset.unit = 8;
+	}
+      else if (is_integral_type (type))
+	{
+	  alien_token.kind = PK_ALIEN_TOKEN_INTEGER;
+	  alien_token.value.integer.magnitude
+	    = value_as_long (value);
+	  alien_token.value.integer.width
+	    = TYPE_LENGTH (type) * 8;
+	  alien_token.value.integer.signed_p
+	    = !type->is_unsigned ();
+	}
+      else
+	goto error;
+    }
+
+  *errmsg = NULL;
+  return &alien_token;
+
+ error:
+  std::string emsg = "can't access GDB variable '";
+  emsg += id;
+  emsg += "'";
+  *errmsg = xstrdup (emsg.c_str ());
+  return NULL;
+}
+
+/* Given a string, prefix it in order ot avoid collision with Poke's
+   keywords.  */
+
+static std::vector<std::string> poke_keywords
+  {
+    "pinned", "struct", "union", "else", "while", "until",
+    "for", "in", "where", "if", "sizeof", "fun", "method",
+    "type", "var", "unit", "break", "continue", "return",
+    "string", "as", "try", "catch", "raise", "void", "any",
+    "print", "printf", "isa", "unmap", "big", "little",
+    "load", "lambda", "assert",
+  };
+
+static std::string
+normalize_poke_identifier (std::string prefix, std::string str)
+{
+  if (std::find (poke_keywords.begin (),
+		 poke_keywords.end (),
+		 str) != poke_keywords.end ())
+    str = prefix + str;
+
+  return str;
+}
+
+/* Given a GDB type name, mangle it to a valid Poke type name.  */
+
+static std::string
+gdb_type_name_to_poke (std::string str, struct type *type = NULL)
+{
+  for (int i = 0; i < str.length (); ++i)
+    if (!(str.begin()[i] == '_'
+	  || (str.begin()[i] >= 'a' && str.begin()[i] <= 'z')
+	  || (str.begin()[i] >= '0' && str.begin()[i] <= '9')
+	  || (str.begin()[i] >= 'A' && str.begin()[i] <= 'Z')))
+      str.begin()[i] = '_';
+
+  if (type)
+    {
+      /* Prepend struct and union tags with suitable prefixes.  This
+	 is to avoid ending with recursive typedefs in C programs.  */
+      if (type->code () == TYPE_CODE_STRUCT)
+	str = "struct_" + str;
+      else if (type->code () == TYPE_CODE_UNION)
+	str = "union_" + str;
+    }
+
+  return str;
+}
+
+#if 0
+/* Command to shut down the poke incremental compiler.  */
+
+static void
+poke_stop_command (const char *args, int from_tty)
+{
+  if (!poke_compiler_live)
+    error (_("The poke incremental compiler is not running."));
+
+  pk_compiler_free (poke_compiler);
+  poke_compiler_live = 0;
+}
+#endif
+
+/* Command to feed the poke compiler with the definition of some given
+   GDB type.  */
+
+static void poke_command (const char *args, int from_tty);
+
+static std::string
+poke_add_type (struct type *type)
+{
+  std::string type_name;
+  std::string str = "";
+
+  if (type != nullptr)
+    {
+      if (type->name ())
+	type_name = type->name ();
+
+      /* Do not try to add a type that is already defined.  */
+      if (type_name != ""
+	  && pk_decl_p (poke_compiler,
+			gdb_type_name_to_poke (type_name, type).c_str (),
+			PK_DECL_KIND_TYPE))
+	return type_name;
+
+      switch (type->code ())
+	{
+	case TYPE_CODE_PTR:
+	  {
+	    str = ("offset<uint<"
+		   + (std::to_string (TYPE_LENGTH (type) * 8))
+		   + ">,B>");
+	    break;
+	  }
+	case TYPE_CODE_TYPEDEF:
+	  {
+	    struct type *target_type = TYPE_TARGET_TYPE (type);
+	    std::string target_type_code = poke_add_type (target_type);
+
+	    if (target_type_code == "")
+	      goto skip;
+
+	    if (target_type->name ())
+	      str += gdb_type_name_to_poke (target_type->name (), target_type);
+	    else
+	      str += target_type_code;
+	    break;
+	  }
+	case TYPE_CODE_INT:
+	  {
+	    size_t type_length = TYPE_LENGTH (type) * 8;
+
+	    if (type_length > 64)
+	      goto skip;
+
+	    if (type->is_unsigned ())
+	      str += "u";
+	    str += "int<";
+	    str += std::to_string (type_length);
+	    str += ">";
+	    break;
+	  }
+	case TYPE_CODE_ARRAY:
+	  {
+	    struct type *target_type = TYPE_TARGET_TYPE (type);
+	    size_t target_type_length = TYPE_LENGTH (target_type);
+	    std::string target_type_code
+	      = poke_add_type (target_type);
+
+	    if (target_type_code == "")
+	      goto skip;
+
+	    if (target_type->name ())
+	      str = gdb_type_name_to_poke (target_type->name (), target_type);
+	    else
+	      str = target_type_code;
+
+	    str += "[";
+	    str += std::to_string (TYPE_LENGTH (type) / target_type_length);
+	    str += "]";
+	    break;
+	  }
+	case TYPE_CODE_STRUCT:
+	  {
+	    size_t natural_bitpos = 0;
+	    str += "struct {";
+
+	    for (int idx = 0; idx < type->num_fields (); idx++)
+	      {
+		std::string field_name
+		  = normalize_poke_identifier ("__f", TYPE_FIELD_NAME (type, idx));
+		struct type *field_type = type->field (idx).type ();
+		size_t field_bitpos = TYPE_FIELD_BITPOS (type, idx);
+
+		if (idx > 0)
+		  str += " ";
+		if (field_type->name ())
+		  {
+		    if (poke_add_type (field_type) == "")
+		      goto skip;
+		    str += gdb_type_name_to_poke (field_type->name (), field_type);
+		  }
+		else
+		  {
+		    std::string pstr = poke_add_type (field_type);
+		    if (pstr == "")
+		      goto skip;
+		    str += pstr;
+		  }
+		str += " ";
+		if (field_name != "")
+		  str += field_name;
+		if (field_bitpos != natural_bitpos)
+		  str += " @ " + (field_bitpos % 8 == 0
+				  ? std::to_string (field_bitpos / 8) + "#B"
+				  : std::to_string (field_bitpos) + "#b");
+		str += ";";
+
+		natural_bitpos = field_bitpos + TYPE_LENGTH (field_type) * 8;
+	      }
+
+	    str += "}";
+	    break;
+	  }
+	default:
+	  goto skip;
+	  break;
+	}
+
+      std::string deftype;
+      if (type_name != "")
+	{
+	  std::string poke_type_name
+	    = gdb_type_name_to_poke (type_name, type);
+
+	  std::string deftype = "type ";
+	  deftype += poke_type_name;
+	  deftype += " = ";
+	  deftype += str;
+
+	  type_poke_strings.push_back (deftype);
+	  poke_command (deftype.c_str(), 0 /* from_tty */);
+	  printf_filtered ("added type %s\n", poke_type_name.c_str ());
+	}
+    }
+
+  return str;
+
+ skip:
+  return "";
+}
+
+/* Start the poke incremental compiler.  */
+
+static void
+start_poke (void)
+{
+  /* Note how we are creating an incremental compiler without the
+     standard Poke types (int, etc) because they collide with the C
+     types.  */
+  poke_compiler = pk_compiler_new_with_flags (&poke_term_if,
+					      PK_F_NOSTDTYPES);
+  if (poke_compiler == NULL)
+    error (_("Couldn't start the poke incremental compiler."));
+  poke_compiler_lives = 1;
+
+  /* Install the handler for alien tokens that recognizes GDB
+     symbols.  */
+  pk_set_alien_token_fn (poke_compiler, poke_alien_token_handler);
+
+  /* Use hexadecimal output by default.  */
+  pk_set_obase (poke_compiler, 16);
+
+  /* Use `tree' printing mode by default.  */
+  pk_set_omode (poke_compiler, PK_PRINT_TREE);
+
+  /* Set default endianness to whatever idem is used by the target
+     architecture.  */
+  enum bfd_endian endian = gdbarch_byte_order (get_current_arch ());
+  pk_set_endian (poke_compiler,
+		 endian == BFD_ENDIAN_BIG ? PK_ENDIAN_MSB : PK_ENDIAN_LSB);
+
+
+  /* Install our foreign IO device interface to access the target's
+     memory, and open it.  */
+  if (pk_register_iod (poke_compiler, &iod_if) != PK_OK)
+    error (_("Could not register the foreign IO device interface in poke."));
+
+  pk_val val;
+  if (pk_compile_statement (poke_compiler, "open (\"<gdb>\");", NULL, &val)
+      != PK_OK)
+    error (_("Could not open <gdb>"));
+}
+
+/* Command to dump the Poke definition of known types.  */
+
+static void
+poke_dump_types (const char *args, int from_tty)
+{
+  if (!poke_compiler_lives)
+    start_poke ();
+
+  for (const std::string &s : type_poke_strings)
+    printf ("%s;\n", s.c_str ());
+}
+
+/* Commands to add GDB types to the running poke compiler.  */
+
+static void
+poke_add_type_command (const char *args, int from_tty)
+{
+  if (!poke_compiler_lives)
+    start_poke ();
+
+  std::string type_name = skip_spaces (args);
+  type_name = gdb_type_name_to_poke (type_name);
+
+  expression_up expr = parse_expression (args);
+  struct value *val = evaluate_type (expr.get ());
+  struct type *type = value_type (val);
+
+  poke_add_type (type);
+}
+
+static void
+poke_add_types (const char *args, int from_tty)
+{
+  if (!poke_compiler_lives)
+    start_poke ();
+
+  std::string symbol_name_regexp = skip_spaces (args);
+  global_symbol_searcher spec (TYPES_DOMAIN, symbol_name_regexp.c_str ());
+  std::vector<symbol_search> symbols = spec.search ();
+  for (const symbol_search &p : symbols)
+    {
+      QUIT;
+
+      struct symbol *sym = p.symbol;
+      struct type *type = SYMBOL_TYPE (sym);
+
+      if (type)
+	poke_add_type (type);
+    }
+}
+
+/* Command to execute a poke statement or declaration.  */
+
+static void
+poke_command (const char *args, int from_tty)
+{
+  if (!poke_compiler_lives)
+    start_poke ();
+
+  int what; /* 0 -> declaration, 1 -> statement */
+  const char *end;
+  std::string cmd;
+
+#define IS_COMMAND(input, cmd) \
+  (strncmp ((input), (cmd), sizeof (cmd) - 1) == 0 \
+   && ((input)[sizeof (cmd) - 1] == ' ' || (input)[sizeof (cmd) - 1] == '\t'))
+
+  args = skip_spaces (args);
+  if (IS_COMMAND (args, "fun"))
+  {
+    what = 0;
+    cmd = args;
+  }
+  else
+    {
+      if (IS_COMMAND (args, "var")
+	  || IS_COMMAND (args, "type")
+	  || IS_COMMAND (args, "unit"))
+	what = 0;
+      else
+	what = 1;
+
+      cmd = args;
+      cmd += ';';
+    }
+
+  pk_set_lexical_cuckolding_p (poke_compiler, 1);
+
+  if (what == 0)
+    {
+      /* Declaration.  */
+      if (pk_compile_buffer (poke_compiler, cmd.c_str (), &end) != PK_OK)
+	goto error;
+    }
+  else
+    {
+      /* Statement.  */
+      pk_val val;
+
+      if (pk_compile_statement (poke_compiler, cmd.c_str (), &end, &val) != PK_OK)
+	goto error;
+
+      if (val != PK_NULL)
+	{
+	  pk_print_val (poke_compiler, val);
+	  poke_puts ("\n");
+	}
+    }
+
+  pk_set_lexical_cuckolding_p (poke_compiler, 0);
+#undef IS_COMMAND
+ error:
+  ;
+}
+
+/* Initialize the poke GDB subsystem.  */
+
+void _initialize_poke (void);
+void
+_initialize_poke ()
+{
+  /* XXX commands to set poke settings, like the output base.  */
+
+  add_com ("poke-add-type", class_vars, poke_add_type_command, _("\
+Make Poke aware of a GDB type given an expression.\n\
+Usage: poke-add-type EXPRESSION\n"));
+
+  add_com ("poke-add-types", class_vars, poke_add_types, _("\
+Make Poke aware of GDB types based on a regexp.\n\
+Usage: poke-add-type REGEXP\n"));
+
+  add_com ("poke-dump-types", class_vars, poke_dump_types, _("\
+Dump the definition of all the GDB types known to poke.\n\
+Usage: poke-dump-types\n"));
+
+  add_com ("poke", class_vars, poke_command, _("\
+Execute a Poke statement or declaration.\n\
+Usage: poke [STMT]\n"));
+}
-- 
2.25.0.2.g232378479e


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

* Re: [PATCH 1/1] Integrate GNU poke in GDB
  2021-05-10 15:10 ` [PATCH 1/1] " Jose E. Marchesi
@ 2021-05-10 16:56   ` Eli Zaretskii
  2021-05-10 18:49     ` Jose E. Marchesi
  2021-05-11  7:33   ` Andrew Burgess
  2021-05-13 16:59   ` Tom Tromey
  2 siblings, 1 reply; 18+ messages in thread
From: Eli Zaretskii @ 2021-05-10 16:56 UTC (permalink / raw)
  To: Jose E. Marchesi; +Cc: gdb-patches

> Date: Mon, 10 May 2021 17:10:44 +0200
> From: "Jose E. Marchesi via Gdb-patches" <gdb-patches@sourceware.org>
> 
> This patch integrates GNU poke (http://jemarch.net/poke) in GDB by
> mean of libpoke.  It allows the GDB user to execute Poke code from
> within the debugger with access to the target memory, types and
> values.

Thanks.

> --- a/gdb/doc/gdb.texinfo
> +++ b/gdb/doc/gdb.texinfo
> @@ -10245,6 +10245,7 @@ being passed the type of @var{arg} as the argument.
>  * Caching Target Data::         Data caching for targets
>  * Searching Memory::            Searching memory for a sequence of bytes
>  * Value Sizes::                 Managing memory allocated for values
> +* Poke::                        Poking at data using GNU poke.
                                                            ^^^^
@samp{poke}, please.

> +@c XXX example: saving some memory in a file (opening other IO spaces)

What's this comment about?

> +@node Poke
> +@section Poking at data using GNU poke

Likewise, please use @samp for "poke" in the section name and in the
text (but not the node name).

> +(with big P) that is specifically designed in order to describe the
         ^^^^^
"capital P"

> +certain GDB abstractions (such as symbols and types) visible from the
           ^^^
@value{GDBN}

> +the Poke language in detail. @xref{Top,,, poke, The GNU poke Manual}
> +for more information.                                               ^

Please add a comma there.

> +* The @command{poke} Command::  Executing Poke from @value{GDBN}.
> +* Poking the Target Memory::    Accessing the target's memory from Poke.
> +* @value{GDBN} Types and Poke::         Accessing @value{GDBN} types from Poke.
> +* @value{GDBN} Values and Poke::        Accessing @value{GDBN} values from Poke.
> +@end menu
> +
> +@node The @command{poke} Command
> +@subsection The @command{poke} Command
> +
> +The @command{poke} command allows to execute arbitrary Poke code from
> +the @value{GDBN} prompt.

@command is wrong for GDB commands.  Our convention is to use @code.

> +(@value{GDBP}) type Packet = struct @{ byte magic == 0x4a; byte sz; byte[sz] payload; @}

This line is too long, it will typeset badly in the printed version.
Please break it into two or more lines.

> +This loads a file @dfn{foo.pk} if it is found in the load path:
                     ^^^^
@file, not @dfn (here and elsewhere).

> +To Poke source files containing definitions that conceptually apply to
> +some definite domain, we call them @dfn{pickles}.  For example,

"To Poke ..., we call them "pickles""?  That sounds incorrect English
to me, do you want to rephrase?

> +(@value{GDBP}) spoke string[3] @@ 0x5ff0#B
                  ^^^^^
"spoke"?

> +Note that the fact the current IO space is the GDB target memory
                                                  ^^^
@value{GDBN}

> +If you close the default IO space you can re-open the GDB target space
                                                         ^^^
Likewise.

> +Note how two types are added: the requested @code{struct person} and
> +also @code{int}, since the struct contains a field of that basic C
> +type. Let's take a look to the type definitions:
       ^^
Two spaces between sentences.

> +type struct_person = struct @{int age; offset<uint<64>,B> name @@ 8#B; offset<uint<64>,B> postal_address;@};

This line is too long.

> +If we wanted to add all the types known to GDB to poke, we could so do
                                              ^^^
@value{GDBN}

> +Poke variables are not the same than GDB symbols, and live in a
                      ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
"not the same as @value{GDBN} symbols"

> +separated world of their own. However, it is possible to refer to GDB
                               ^^                                    ^^^
Two spaces, and @value{GDBN} again.

> +Since many @value{GDBN} values are pointers, it is possible to access
> +to the address of a value by using the @code{$addr::IDENTIFIER} ^^^^^
   ^^^^^^^^^^^^^^
"access the address", without the "to".

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

* Re: [PATCH 0/1] Integrate GNU poke in GDB
  2021-05-10 15:10 [PATCH 0/1] Integrate GNU poke in GDB Jose E. Marchesi
  2021-05-10 15:10 ` [PATCH 1/1] " Jose E. Marchesi
@ 2021-05-10 18:39 ` Simon Marchi
  2021-05-10 20:07   ` Jose E. Marchesi
  2021-05-13 17:04   ` Tom Tromey
  2021-05-11 18:56 ` Tom Tromey
  2 siblings, 2 replies; 18+ messages in thread
From: Simon Marchi @ 2021-05-10 18:39 UTC (permalink / raw)
  To: Jose E. Marchesi, gdb-patches

On 2021-05-10 11:10 a.m., Jose E. Marchesi via Gdb-patches wrote:
> Hi GDB people!
> 
> The patch below integrates GNU poke (http://jemarch.net/poke) in
> GDB by mean of libpoke.
> 
> It allows the GDB user to execute Poke code from within the
> debugger with access to the target memory, types and values.

Awesome!

I'll try to answer a few of your questions, without having read the
patch itself.

> 
> A few notes:
> 
> - The poke support is optional, and built if specified with
>   --enable-poke at configure time.

If I look at this reference [1]:

     --enable-*/--disable-* arguments

	The arguments starting with --enable- prefix are usually used to
	enable features of the program. They usually add or remove
	dependencies only if they are needed for that particular feature
	being enabled or disabled.

    --with-*/--without-* arguments

	The arguments starting with --with- prefix are usually used to
	add or remove dependencies on external projects. These might add
	or remove features from the project.

I don't find this super clear, as we could always see it both ways.  I
want to enable the support for running Python code, so I use
--enable-python, which has the side-effect of pulling libpython.  Or I
want to make my project depend on libpython, so I use --with-python,
which has the side-effect of adding the "can run Python code feature" in
my project.

But most of our dependencies on external libs are expressed with
"--with", so I'd go with "--with-poke".

[1] https://autotools.io/autoconf/arguments.html

> 
> - The poke support requires libpoke from git master (which will
>   become poke 2.x in a few months) since it uses some interfaces
>   that are not available in the released poke 1.x series.
> 
> - Sorry for what is probably an atrocious mixture of C and C++
>   idioms.  I avoid C++ like the pest, but I am all willing to try
>   and write proper C++ for this integration.

No problem, we can help you with that.

> 
> - Eventually we will probably want to ship some prewritten Poke
>   code in a pickle gdb.pk.  Would $pkddatadir/poke/ be a good
>   location for Poke code distributed with GDB?

So, /usr/share/poke?

Would gdb.pk contain some poke code that gdb would use itself, or is the
goal to make that poke code available to other libpoke users (like the
poke cli)?

If it's code that is only meant to be used by gdb, it would make sense
to have it in GDB's own data dir (/usr/share/gdb/poke).  A bit like gdb
has some Python code it uses for itself, in /usr/share/gdb/python.

> 
> And a few questions:
> 
> - Where am I supposed to cleanup and shut down the incremental
>   compiler when GDB exits?  I looked but I cannot find a
>   counterpart to _initialize_FOO, like _finalize_FOO.

See make_final_cleanup.  Or, you can use an std::unique_ptr
specialization that calls your finalize function on destruction.  If
that object is global (or static within a function), it will be
destructed when the program (GDB) exits.  There was a discussion about
exactly this but for debuginfod recently:

https://sourceware.org/pipermail/gdb-patches/2021-May/178578.html

> 
> - There are many global parameters that can be configured in the
>   poke incremental compiler: the numeration base used when
>   printing values, the global endianness, a cut-off value for how
>   many array elements are printed, and the like.  Reasonable
>   defaults for them are set in `start_poke', but I would like the
>   GDB user to be able to change these settings.  I could add a
>   `poke-set SETTING [VALUE]' command, but I was wondering whether
>   there is a better way, like a global register of settings
>   already in GDB?

Without more knowledge of how this all works, I'd say that

   set poke PARAM VALUE
   show poke PARAM

would be more GDB-ish.  Is there a way for GDB to list all poke's
parameters at startup, so it could register them all as a "set/show"
command in GDB?  As a result, the user could do "set poke <TAB><TAB>"
to see all possible parameters, which I would find really nice.

Although if you think we might ever need to add some GDB parameters that
are not poke parameters, but parameters about how GDB uses/integrates
poke, then there is a chance of clash in the "set poke" namespace.  In
that case, we could put all poke's parameters under

    set poke parameter PARAM VALUE
    show poke parameter PARAM

This way, we could have distinct "set poke parameter foo ..." and "set
poke foo ...".

> 
> - I am using target_{read,write}_raw_memory to access the
>   target's memory.  What is the difference between the raw and
>   non-raw accessors?

From target_read_raw_memory's doc:

/* Like target_read_memory, but specify explicitly that this is a read
   from the target's raw memory.  That is, this read bypasses the
   dcache, breakpoint shadowing, etc.  */


So, it bypasses a cache in GDB (not sure why that's important).  But
most importantly, the non-raw version will hide the software breakpoint
instructions, it will replace them with the original memory contents,
whereas the raw version will not.

Let's says that the user decodes some memory where a breakpoint is, using
target_read_raw_memory will return the memory contents with the
breakpoint instruction.  I think that most of the time, the user will
actually want to decode the original contents, so using
target_read_memory would be better.

And if you write memory with target_write_raw_memory, I guess you can
end up overwriting a breakpoint, so that's not good.  So I'd probably
use target_write_memory.

> 
> - How can I demangle C++ identifiers?  And how can I detect when
>   a given type is a C++ one that needs demangling?

I think that we usually rely on the language of the compilation unit, as
given by DWARF, to know that a symbol has a C++ mangled name and needs
demangling.

But otherwise, see the gdb_demangle function.

Simon

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

* Re: [PATCH 1/1] Integrate GNU poke in GDB
  2021-05-10 16:56   ` Eli Zaretskii
@ 2021-05-10 18:49     ` Jose E. Marchesi
  2021-05-10 18:52       ` Eli Zaretskii
  0 siblings, 1 reply; 18+ messages in thread
From: Jose E. Marchesi @ 2021-05-10 18:49 UTC (permalink / raw)
  To: Eli Zaretskii; +Cc: gdb-patches


Hi Eli.

Thanks for your feedback.

I will fix all the points you raise in the next version for the patch.

But I have got one question:

>> +(@value{GDBP}) type Packet = struct @{ byte magic == 0x4a; byte sz; byte[sz] payload; @}
>
> This line is too long, it will typeset badly in the printed version.
> Please break it into two or more lines.

What is the convention to denote one logical line in two physical lines
in verbatim/example sections?  Something like this?

(@value{GDBP}) type Packet = struct @{ byte magic == 0x4a; \
byte sz; byte[sz] payload; @}



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

* Re: [PATCH 1/1] Integrate GNU poke in GDB
  2021-05-10 18:49     ` Jose E. Marchesi
@ 2021-05-10 18:52       ` Eli Zaretskii
  0 siblings, 0 replies; 18+ messages in thread
From: Eli Zaretskii @ 2021-05-10 18:52 UTC (permalink / raw)
  To: Jose E. Marchesi; +Cc: gdb-patches

> From: "Jose E. Marchesi" <jose.marchesi@oracle.com>
> Cc: gdb-patches@sourceware.org
> Date: Mon, 10 May 2021 20:49:20 +0200
> 
> >> +(@value{GDBP}) type Packet = struct @{ byte magic == 0x4a; byte sz; byte[sz] payload; @}
> >
> > This line is too long, it will typeset badly in the printed version.
> > Please break it into two or more lines.
> 
> What is the convention to denote one logical line in two physical lines
> in verbatim/example sections?  Something like this?
> 
> (@value{GDBP}) type Packet = struct @{ byte magic == 0x4a; \
> byte sz; byte[sz] payload; @}

Yes, and indent the second line (and subsequent lines, if any).

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

* Re: [PATCH 0/1] Integrate GNU poke in GDB
  2021-05-10 18:39 ` [PATCH 0/1] " Simon Marchi
@ 2021-05-10 20:07   ` Jose E. Marchesi
  2021-05-11  6:25     ` Andrew Burgess
  2021-05-13 17:04   ` Tom Tromey
  1 sibling, 1 reply; 18+ messages in thread
From: Jose E. Marchesi @ 2021-05-10 20:07 UTC (permalink / raw)
  To: Simon Marchi; +Cc: gdb-patches


Hi Simon.
Thanks for the feedback.

>> A few notes:
>> 
>> - The poke support is optional, and built if specified with
>>   --enable-poke at configure time.
>
> If I look at this reference [1]:
>
>      --enable-*/--disable-* arguments
>
> 	The arguments starting with --enable- prefix are usually used to
> 	enable features of the program. They usually add or remove
> 	dependencies only if they are needed for that particular feature
> 	being enabled or disabled.
>
>     --with-*/--without-* arguments
>
> 	The arguments starting with --with- prefix are usually used to
> 	add or remove dependencies on external projects. These might add
> 	or remove features from the project.
>
> I don't find this super clear, as we could always see it both ways.  I
> want to enable the support for running Python code, so I use
> --enable-python, which has the side-effect of pulling libpython.  Or I
> want to make my project depend on libpython, so I use --with-python,
> which has the side-effect of adding the "can run Python code feature" in
> my project.
>
> But most of our dependencies on external libs are expressed with
> "--with", so I'd go with "--with-poke".

Ok, I will change it :)

>> 
>> - Eventually we will probably want to ship some prewritten Poke
>>   code in a pickle gdb.pk.  Would $pkddatadir/poke/ be a good
>>   location for Poke code distributed with GDB?
>
> So, /usr/share/poke?
>
> Would gdb.pk contain some poke code that gdb would use itself, or is the
> goal to make that poke code available to other libpoke users (like the
> poke cli)?
>
> If it's code that is only meant to be used by gdb, it would make sense
> to have it in GDB's own data dir (/usr/share/gdb/poke).  A bit like gdb
> has some Python code it uses for itself, in /usr/share/gdb/python.

Yeah that makes sense, /usr/share/poke for the first case, and
/usr/share/gdb/poke in the second case.

>> 
>> And a few questions:
>> 
>> - Where am I supposed to cleanup and shut down the incremental
>>   compiler when GDB exits?  I looked but I cannot find a
>>   counterpart to _initialize_FOO, like _finalize_FOO.
>
> See make_final_cleanup.  Or, you can use an std::unique_ptr
> specialization that calls your finalize function on destruction.  If
> that object is global (or static within a function), it will be
> destructed when the program (GDB) exits.  There was a discussion about
> exactly this but for debuginfod recently:
>
> https://sourceware.org/pipermail/gdb-patches/2021-May/178578.html

That unique_ptr approach should work.  Will take a look to that
discussion and DTRT.

>> - There are many global parameters that can be configured in the
>>   poke incremental compiler: the numeration base used when
>>   printing values, the global endianness, a cut-off value for how
>>   many array elements are printed, and the like.  Reasonable
>>   defaults for them are set in `start_poke', but I would like the
>>   GDB user to be able to change these settings.  I could add a
>>   `poke-set SETTING [VALUE]' command, but I was wondering whether
>>   there is a better way, like a global register of settings
>>   already in GDB?
>
> Without more knowledge of how this all works, I'd say that
>
>    set poke PARAM VALUE
>    show poke PARAM
>
> would be more GDB-ish.

Yes that's what I had in mind :)

> Is there a way for GDB to list all poke's parameters at startup, so it
> could register them all as a "set/show" command in GDB?

Yes, that is certainly possible.

> As a result, the user could do "set poke <TAB><TAB>" to see all
> possible parameters, which I would find really nice.

Definitely!

> Although if you think we might ever need to add some GDB parameters that
> are not poke parameters, but parameters about how GDB uses/integrates
> poke, then there is a chance of clash in the "set poke" namespace.  In
> that case, we could put all poke's parameters under
>
>     set poke parameter PARAM VALUE
>     show poke parameter PARAM
>
> This way, we could have distinct "set poke parameter foo ..." and "set
> poke foo ...".

Where is the interface to add the settings?

>> - I am using target_{read,write}_raw_memory to access the
>>   target's memory.  What is the difference between the raw and
>>   non-raw accessors?
>
> From target_read_raw_memory's doc:
>
> /* Like target_read_memory, but specify explicitly that this is a read
>    from the target's raw memory.  That is, this read bypasses the
>    dcache, breakpoint shadowing, etc.  */
>
>
> So, it bypasses a cache in GDB (not sure why that's important).  But
> most importantly, the non-raw version will hide the software breakpoint
> instructions, it will replace them with the original memory contents,
> whereas the raw version will not.
>
> Let's says that the user decodes some memory where a breakpoint is, using
> target_read_raw_memory will return the memory contents with the
> breakpoint instruction.  I think that most of the time, the user will
> actually want to decode the original contents, so using
> target_read_memory would be better.
>
> And if you write memory with target_write_raw_memory, I guess you can
> end up overwriting a breakpoint, so that's not good.  So I'd probably
> use target_write_memory.

Ok, I will change to use the non-raw versions.

>> - How can I demangle C++ identifiers?  And how can I detect when
>>   a given type is a C++ one that needs demangling?
>
> I think that we usually rely on the language of the compilation unit, as
> given by DWARF, to know that a symbol has a C++ mangled name and needs
> demangling.
>
> But otherwise, see the gdb_demangle function.

Will do.
Thanks!

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

* Re: [PATCH 0/1] Integrate GNU poke in GDB
  2021-05-10 20:07   ` Jose E. Marchesi
@ 2021-05-11  6:25     ` Andrew Burgess
  0 siblings, 0 replies; 18+ messages in thread
From: Andrew Burgess @ 2021-05-11  6:25 UTC (permalink / raw)
  To: Jose E. Marchesi; +Cc: Simon Marchi, gdb-patches

* Jose E. Marchesi via Gdb-patches <gdb-patches@sourceware.org> [2021-05-10 22:07:44 +0200]:

> 
> Hi Simon.
> Thanks for the feedback.
> 
> >> A few notes:
> >> 
> >> - The poke support is optional, and built if specified with
> >>   --enable-poke at configure time.
> >
> > If I look at this reference [1]:
> >
> >      --enable-*/--disable-* arguments
> >
> > 	The arguments starting with --enable- prefix are usually used to
> > 	enable features of the program. They usually add or remove
> > 	dependencies only if they are needed for that particular feature
> > 	being enabled or disabled.
> >
> >     --with-*/--without-* arguments
> >
> > 	The arguments starting with --with- prefix are usually used to
> > 	add or remove dependencies on external projects. These might add
> > 	or remove features from the project.
> >
> > I don't find this super clear, as we could always see it both ways.  I
> > want to enable the support for running Python code, so I use
> > --enable-python, which has the side-effect of pulling libpython.  Or I
> > want to make my project depend on libpython, so I use --with-python,
> > which has the side-effect of adding the "can run Python code feature" in
> > my project.
> >
> > But most of our dependencies on external libs are expressed with
> > "--with", so I'd go with "--with-poke".
> 
> Ok, I will change it :)
> 
> >> 
> >> - Eventually we will probably want to ship some prewritten Poke
> >>   code in a pickle gdb.pk.  Would $pkddatadir/poke/ be a good
> >>   location for Poke code distributed with GDB?
> >
> > So, /usr/share/poke?
> >
> > Would gdb.pk contain some poke code that gdb would use itself, or is the
> > goal to make that poke code available to other libpoke users (like the
> > poke cli)?
> >
> > If it's code that is only meant to be used by gdb, it would make sense
> > to have it in GDB's own data dir (/usr/share/gdb/poke).  A bit like gdb
> > has some Python code it uses for itself, in /usr/share/gdb/python.
> 
> Yeah that makes sense, /usr/share/poke for the first case, and
> /usr/share/gdb/poke in the second case.
> 
> >> 
> >> And a few questions:
> >> 
> >> - Where am I supposed to cleanup and shut down the incremental
> >>   compiler when GDB exits?  I looked but I cannot find a
> >>   counterpart to _initialize_FOO, like _finalize_FOO.
> >
> > See make_final_cleanup.  Or, you can use an std::unique_ptr
> > specialization that calls your finalize function on destruction.  If
> > that object is global (or static within a function), it will be
> > destructed when the program (GDB) exits.  There was a discussion about
> > exactly this but for debuginfod recently:
> >
> > https://sourceware.org/pipermail/gdb-patches/2021-May/178578.html
> 
> That unique_ptr approach should work.  Will take a look to that
> discussion and DTRT.
> 
> >> - There are many global parameters that can be configured in the
> >>   poke incremental compiler: the numeration base used when
> >>   printing values, the global endianness, a cut-off value for how
> >>   many array elements are printed, and the like.  Reasonable
> >>   defaults for them are set in `start_poke', but I would like the
> >>   GDB user to be able to change these settings.  I could add a
> >>   `poke-set SETTING [VALUE]' command, but I was wondering whether
> >>   there is a better way, like a global register of settings
> >>   already in GDB?
> >
> > Without more knowledge of how this all works, I'd say that
> >
> >    set poke PARAM VALUE
> >    show poke PARAM
> >
> > would be more GDB-ish.
> 
> Yes that's what I had in mind :)
> 
> > Is there a way for GDB to list all poke's parameters at startup, so it
> > could register them all as a "set/show" command in GDB?
> 
> Yes, that is certainly possible.
> 
> > As a result, the user could do "set poke <TAB><TAB>" to see all
> > possible parameters, which I would find really nice.
> 
> Definitely!
> 
> > Although if you think we might ever need to add some GDB parameters that
> > are not poke parameters, but parameters about how GDB uses/integrates
> > poke, then there is a chance of clash in the "set poke" namespace.  In
> > that case, we could put all poke's parameters under
> >
> >     set poke parameter PARAM VALUE
> >     show poke parameter PARAM
> >
> > This way, we could have distinct "set poke parameter foo ..." and "set
> > poke foo ...".
> 
> Where is the interface to add the settings?

You'd want the add_setshow_* functions declared in command.h.  Calls
to these are usually added to an appropriate _initialize_* function.

Thanks,
Andrew

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

* Re: [PATCH 1/1] Integrate GNU poke in GDB
  2021-05-10 15:10 ` [PATCH 1/1] " Jose E. Marchesi
  2021-05-10 16:56   ` Eli Zaretskii
@ 2021-05-11  7:33   ` Andrew Burgess
  2021-05-11 13:07     ` Jose E. Marchesi
  2021-05-13 16:59   ` Tom Tromey
  2 siblings, 1 reply; 18+ messages in thread
From: Andrew Burgess @ 2021-05-11  7:33 UTC (permalink / raw)
  To: Jose E. Marchesi; +Cc: gdb-patches

* Jose E. Marchesi via Gdb-patches <gdb-patches@sourceware.org> [2021-05-10 17:10:44 +0200]:

> This patch integrates GNU poke (http://jemarch.net/poke) in GDB by
> mean of libpoke.  It allows the GDB user to execute Poke code from
> within the debugger with access to the target memory, types and
> values.
> 
> How this stuff works:
> 
> - GDB links with libpoke.so and uses the interface in libpoke.h.
>   This is also how the GNU poke application (the command-line
>   editor) is implemented.
> 
> - There are three commands:

s/three/four/

> 
>   poke STR
>   poke-add-type EXPR
>   poke-add-types REGEXP
>   poke-dump-types
> 
>   All three commands make sure to start the poke incremental
>   compiler if it isn't running already.

s/three/four/

> 
> - Access to the target's memory is provided by GDB by installing
>   a Foreign IO device in the incremental compiler.  This is
>   `iod_if' in poke.c.
> 
> - Access to the terminal is provided by GDB by providing a
>   pk_term_if implementation to the incremental compiler.  This is
>   `poke_term_if' in poke.c.
> 
> - Access to GDB values is provided by GDB by installing an alien
>   token handler in the incremental compiler.  This is
>   `poke_alien_token_handler' in poke.c.
> 
> gdb/ChangeLog:
> 
> 2021-05-10  Jose E. Marchesi  <jose.marchesi@oracle.com>
> 
> 	* configure.ac: Support --enable-poke.
> 	* configure: Regenerate.
> 	* Makefile.in (POKE_OBS): Define based on @POKE_OBS@.
> 	(DEPFILES): Add POKE_OBS.
> 	* poke.c: New file.
> 
> gdb/doc/ChangeLog:
> 
> 2021-05-10  Jose E. Marchesi  <jose.marchesi@oracle.com>
> 
> 	* Makefile.in (GDB_DOC_FILES): Add poke.texi.
> 	* poke.texi: New file.
> 	* gdb.texinfo (Data): Add meny entry for Poke and @include poke.texi.

I have no objections to merging this functionality, especially as it
is so self-contained, however, I do have a question, or feedback.

As someone who doesn't know poke, after reading the manual, it's still
not clear to me what significant value poke adds to GDB.  A lot of the
examples seem to cover things that GDB already does, mostly examining
data and variables in target memory (though there were a couple of
neat features which aren't so easy to achieve in GDB).

I guess my question, or feedback, would be, what's the killer feature
that adding poke brings to GDB?   Maybe you could find a relatively
small, but powerful example, which you could give in the initial
introduction to poke?

Anyway, that's just my thoughts, something like that would make it
much clearer to me why this is valuable.

I had a couple of other corrections pointed out below.

> ---
>  gdb/ChangeLog       |   8 +
>  gdb/Makefile.in     |   6 +-
>  gdb/configure       |  89 +++++-
>  gdb/configure.ac    |  15 +
>  gdb/doc/ChangeLog   |   6 +
>  gdb/doc/Makefile.in |   1 +
>  gdb/doc/gdb.texinfo |   4 +
>  gdb/doc/poke.texi   | 373 ++++++++++++++++++++++
>  gdb/poke.c          | 762 ++++++++++++++++++++++++++++++++++++++++++++
>  9 files changed, 1257 insertions(+), 7 deletions(-)
>  create mode 100644 gdb/doc/poke.texi
>  create mode 100644 gdb/poke.c
> 
> diff --git a/gdb/ChangeLog b/gdb/ChangeLog
> index 70d4972cd52..00f9a39f9f4 100644
> --- a/gdb/ChangeLog
> +++ b/gdb/ChangeLog
> @@ -1,3 +1,11 @@
> +2021-05-10  Jose E. Marchesi  <jose.marchesi@oracle.com>
> +
> +	* configure.ac: Support --enable-poke.
> +	* configure: Regenerate.
> +	* Makefile.in (POKE_OBS): Define based on @POKE_OBS@.
> +	(DEPFILES): Add POKE_OBS.
> +	* poke.c: New file.
> +
>  2021-05-07  Tom de Vries  <tdevries@suse.de>
>  
>  	PR symtab/26327
> diff --git a/gdb/Makefile.in b/gdb/Makefile.in
> index ba0cabb0216..7085f1196d2 100644
> --- a/gdb/Makefile.in
> +++ b/gdb/Makefile.in
> @@ -670,6 +670,9 @@ SER_HARDWIRE = @SER_HARDWIRE@
>  # This is remote-sim.o if a simulator is to be linked in.
>  SIM_OBS = @SIM_OBS@
>  
> +# Object files to integrate with GNU poke.
> +POKE_OBS = @POKE_OBS@
> +
>  # Target-dependent object files.
>  TARGET_OBS = @TARGET_OBS@
>  
> @@ -1570,7 +1573,8 @@ HFILES_WITH_SRCDIR = \
>  # variables analogous to SER_HARDWIRE which get defaulted in this
>  # Makefile.in
>  
> -DEPFILES = $(TARGET_OBS) $(SER_HARDWIRE) $(NATDEPFILES) $(SIM_OBS)
> +DEPFILES = $(TARGET_OBS) $(SER_HARDWIRE) $(NATDEPFILES) $(SIM_OBS) \
> +           $(POKE_OBS)
>  
>  SOURCES = $(SFILES) $(ALLDEPFILES) $(YYFILES) $(CONFIG_SRCS)
>  # Don't include YYFILES (*.c) because we already include *.y in SFILES,
> diff --git a/gdb/configure b/gdb/configure
> index b47de77bf64..909935ffece 100755
> --- a/gdb/configure
> +++ b/gdb/configure
> @@ -748,6 +748,7 @@ LTLIBICONV
>  LIBICONV
>  zlibinc
>  zlibdir
> +POKE_OBS
>  MIG
>  WINDRES
>  DLLTOOL
> @@ -846,6 +847,7 @@ infodir
>  docdir
>  oldincludedir
>  includedir
> +runstatedir
>  localstatedir
>  sharedstatedir
>  sysconfdir
> @@ -888,6 +890,7 @@ enable_profiling
>  enable_codesign
>  with_pkgversion
>  with_bugurl
> +enable_poke
>  with_system_zlib
>  with_gnu_ld
>  enable_rpath
> @@ -995,6 +998,7 @@ datadir='${datarootdir}'
>  sysconfdir='${prefix}/etc'
>  sharedstatedir='${prefix}/com'
>  localstatedir='${prefix}/var'
> +runstatedir='${localstatedir}/run'
>  includedir='${prefix}/include'
>  oldincludedir='/usr/include'
>  docdir='${datarootdir}/doc/${PACKAGE}'
> @@ -1247,6 +1251,15 @@ do
>    | -silent | --silent | --silen | --sile | --sil)
>      silent=yes ;;
>  
> +  -runstatedir | --runstatedir | --runstatedi | --runstated \
> +  | --runstate | --runstat | --runsta | --runst | --runs \
> +  | --run | --ru | --r)
> +    ac_prev=runstatedir ;;
> +  -runstatedir=* | --runstatedir=* | --runstatedi=* | --runstated=* \
> +  | --runstate=* | --runstat=* | --runsta=* | --runst=* | --runs=* \
> +  | --run=* | --ru=* | --r=*)
> +    runstatedir=$ac_optarg ;;
> +
>    -sbindir | --sbindir | --sbindi | --sbind | --sbin | --sbi | --sb)
>      ac_prev=sbindir ;;
>    -sbindir=* | --sbindir=* | --sbindi=* | --sbind=* | --sbin=* \
> @@ -1384,7 +1397,7 @@ fi
>  for ac_var in	exec_prefix prefix bindir sbindir libexecdir datarootdir \
>  		datadir sysconfdir sharedstatedir localstatedir includedir \
>  		oldincludedir docdir infodir htmldir dvidir pdfdir psdir \
> -		libdir localedir mandir
> +		libdir localedir mandir runstatedir
>  do
>    eval ac_val=\$$ac_var
>    # Remove trailing slashes.
> @@ -1537,6 +1550,7 @@ Fine tuning of the installation directories:
>    --sysconfdir=DIR        read-only single-machine data [PREFIX/etc]
>    --sharedstatedir=DIR    modifiable architecture-independent data [PREFIX/com]
>    --localstatedir=DIR     modifiable single-machine data [PREFIX/var]
> +  --runstatedir=DIR       modifiable per-process data [LOCALSTATEDIR/run]
>    --libdir=DIR            object code libraries [EPREFIX/lib]
>    --includedir=DIR        C header files [PREFIX/include]
>    --oldincludedir=DIR     C header files for non-gcc [/usr/include]
> @@ -1591,6 +1605,7 @@ Optional Features:
>    --enable-gdbtk          enable gdbtk graphical user interface (GUI)
>    --enable-profiling      enable profiling of GDB
>    --enable-codesign=CERT  sign gdb with 'codesign -s CERT'
> +  --enable-poke           Build GDB with poke support (default is NO)
>    --disable-rpath         do not hardcode runtime library paths
>    --enable-source-highlight
>                            enable source-highlight for source listings
> @@ -4852,7 +4867,7 @@ else
>      We can't simply define LARGE_OFF_T to be 9223372036854775807,
>      since some C++ compilers masquerading as C compilers
>      incorrectly reject 9223372036854775807.  */
> -#define LARGE_OFF_T (((off_t) 1 << 62) - 1 + ((off_t) 1 << 62))
> +#define LARGE_OFF_T ((((off_t) 1 << 31) << 31) - 1 + (((off_t) 1 << 31) << 31))
>    int off_t_is_large[(LARGE_OFF_T % 2147483629 == 721
>  		       && LARGE_OFF_T % 2147483647 == 1)
>  		      ? 1 : -1];
> @@ -4898,7 +4913,7 @@ else
>      We can't simply define LARGE_OFF_T to be 9223372036854775807,
>      since some C++ compilers masquerading as C compilers
>      incorrectly reject 9223372036854775807.  */
> -#define LARGE_OFF_T (((off_t) 1 << 62) - 1 + ((off_t) 1 << 62))
> +#define LARGE_OFF_T ((((off_t) 1 << 31) << 31) - 1 + (((off_t) 1 << 31) << 31))
>    int off_t_is_large[(LARGE_OFF_T % 2147483629 == 721
>  		       && LARGE_OFF_T % 2147483647 == 1)
>  		      ? 1 : -1];
> @@ -4922,7 +4937,7 @@ rm -f core conftest.err conftest.$ac_objext conftest.$ac_ext
>      We can't simply define LARGE_OFF_T to be 9223372036854775807,
>      since some C++ compilers masquerading as C compilers
>      incorrectly reject 9223372036854775807.  */
> -#define LARGE_OFF_T (((off_t) 1 << 62) - 1 + ((off_t) 1 << 62))
> +#define LARGE_OFF_T ((((off_t) 1 << 31) << 31) - 1 + (((off_t) 1 << 31) << 31))
>    int off_t_is_large[(LARGE_OFF_T % 2147483629 == 721
>  		       && LARGE_OFF_T % 2147483647 == 1)
>  		      ? 1 : -1];
> @@ -4967,7 +4982,7 @@ else
>      We can't simply define LARGE_OFF_T to be 9223372036854775807,
>      since some C++ compilers masquerading as C compilers
>      incorrectly reject 9223372036854775807.  */
> -#define LARGE_OFF_T (((off_t) 1 << 62) - 1 + ((off_t) 1 << 62))
> +#define LARGE_OFF_T ((((off_t) 1 << 31) << 31) - 1 + (((off_t) 1 << 31) << 31))
>    int off_t_is_large[(LARGE_OFF_T % 2147483629 == 721
>  		       && LARGE_OFF_T % 2147483647 == 1)
>  		      ? 1 : -1];
> @@ -4991,7 +5006,7 @@ rm -f core conftest.err conftest.$ac_objext conftest.$ac_ext
>      We can't simply define LARGE_OFF_T to be 9223372036854775807,
>      since some C++ compilers masquerading as C compilers
>      incorrectly reject 9223372036854775807.  */
> -#define LARGE_OFF_T (((off_t) 1 << 62) - 1 + ((off_t) 1 << 62))
> +#define LARGE_OFF_T ((((off_t) 1 << 31) << 31) - 1 + (((off_t) 1 << 31) << 31))
>    int off_t_is_large[(LARGE_OFF_T % 2147483629 == 721
>  		       && LARGE_OFF_T % 2147483647 == 1)
>  		      ? 1 : -1];
> @@ -8188,6 +8203,68 @@ if test "$ac_res" != no; then :
>  fi
>  
>  
> +# Integration with GNU poke is done through the libpoke library.
> +# Check whether --enable-poke was given.
> +if test "${enable_poke+set}" = set; then :
> +  enableval=$enable_poke; poke_enabled=$enableval
> +else
> +  poke_enabled=no
> +fi
> +
> +if test "x$poke_enabled" = "xyes"; then
> +  # Note that we need a libpoke with support for registering foreign
> +  # IO devices, hence the symbol pk_register_iod.
> +  { $as_echo "$as_me:${as_lineno-$LINENO}: checking for pk_register_iod in -lpoke" >&5
> +$as_echo_n "checking for pk_register_iod in -lpoke... " >&6; }
> +if ${ac_cv_lib_poke_pk_register_iod+:} false; then :
> +  $as_echo_n "(cached) " >&6
> +else
> +  ac_check_lib_save_LIBS=$LIBS
> +LIBS="-lpoke  $LIBS"
> +cat confdefs.h - <<_ACEOF >conftest.$ac_ext
> +/* end confdefs.h.  */
> +
> +/* Override any GCC internal prototype to avoid an error.
> +   Use char because int might match the return type of a GCC
> +   builtin and then its argument prototype would still apply.  */
> +#ifdef __cplusplus
> +extern "C"
> +#endif
> +char pk_register_iod ();
> +int
> +main ()
> +{
> +return pk_register_iod ();
> +  ;
> +  return 0;
> +}
> +_ACEOF
> +if ac_fn_c_try_link "$LINENO"; then :
> +  ac_cv_lib_poke_pk_register_iod=yes
> +else
> +  ac_cv_lib_poke_pk_register_iod=no
> +fi
> +rm -f core conftest.err conftest.$ac_objext \
> +    conftest$ac_exeext conftest.$ac_ext
> +LIBS=$ac_check_lib_save_LIBS
> +fi
> +{ $as_echo "$as_me:${as_lineno-$LINENO}: result: $ac_cv_lib_poke_pk_register_iod" >&5
> +$as_echo "$ac_cv_lib_poke_pk_register_iod" >&6; }
> +if test "x$ac_cv_lib_poke_pk_register_iod" = xyes; then :
> +  cat >>confdefs.h <<_ACEOF
> +#define HAVE_LIBPOKE 1
> +_ACEOF
> +
> +  LIBS="-lpoke $LIBS"
> +
> +fi
> +
> +  POKE_OBS="poke.o"
> +else
> +  POKE_OBS=
> +fi
> +
> +
>  # Link in zlib if we can.  This allows us to read compressed debug sections.
>  
>    # Use the system's zlib library.
> diff --git a/gdb/configure.ac b/gdb/configure.ac
> index acfbe4a9a6c..ea149651419 100644
> --- a/gdb/configure.ac
> +++ b/gdb/configure.ac
> @@ -500,6 +500,21 @@ AC_SEARCH_LIBS(gethostbyname, nsl)
>  # Some systems (e.g. Solaris) have `socketpair' in libsocket.
>  AC_SEARCH_LIBS(socketpair, socket)
>  
> +# Integration with GNU poke is done through the libpoke library.
> +AC_ARG_ENABLE([poke],
> +              AS_HELP_STRING([--enable-poke],
> +                             [Build GDB with poke support (default is NO)]),
> +              [poke_enabled=$enableval], [poke_enabled=no])
> +if test "x$poke_enabled" = "xyes"; then
> +  # Note that we need a libpoke with support for registering foreign
> +  # IO devices, hence the symbol pk_register_iod.
> +  AC_CHECK_LIB(poke, pk_register_iod)
> +  POKE_OBS="poke.o"
> +else
> +  POKE_OBS=
> +fi
> +AC_SUBST(POKE_OBS)
> +
>  # Link in zlib if we can.  This allows us to read compressed debug sections.
>  AM_ZLIB
>  
> diff --git a/gdb/doc/ChangeLog b/gdb/doc/ChangeLog
> index a56735896c5..db4e97c8ad1 100644
> --- a/gdb/doc/ChangeLog
> +++ b/gdb/doc/ChangeLog
> @@ -1,3 +1,9 @@
> +2021-05-10  Jose E. Marchesi  <jose.marchesi@oracle.com>
> +
> +	* Makefile.in (GDB_DOC_FILES): Add poke.texi.
> +	* poke.texi: New file.
> +	* gdb.texinfo (Data): Add meny entry for Poke and @include poke.texi.
> +
>  2021-05-07  Tom de Vries  <tdevries@suse.de>
>  
>  	PR symtab/26327
> diff --git a/gdb/doc/Makefile.in b/gdb/doc/Makefile.in
> index 13b288d3e42..457c332e46d 100644
> --- a/gdb/doc/Makefile.in
> +++ b/gdb/doc/Makefile.in
> @@ -138,6 +138,7 @@ GDB_DOC_FILES = \
>  	$(srcdir)/gdb.texinfo \
>  	$(srcdir)/guile.texi \
>  	$(srcdir)/python.texi \
> +        $(srcdir)/poke.texi \
>  	$(GDB_DOC_SOURCE_INCLUDES) \
>  	$(GDB_DOC_BUILD_INCLUDES)
>  
> diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
> index f4d7085da58..06aaac2598a 100644
> --- a/gdb/doc/gdb.texinfo
> +++ b/gdb/doc/gdb.texinfo
> @@ -10245,6 +10245,7 @@ being passed the type of @var{arg} as the argument.
>  * Caching Target Data::         Data caching for targets
>  * Searching Memory::            Searching memory for a sequence of bytes
>  * Value Sizes::                 Managing memory allocated for values
> +* Poke::                        Poking at data using GNU poke.
>  @end menu
>  
>  @node Expressions
> @@ -13785,6 +13786,9 @@ Show the maximum size of memory, in bytes, that @value{GDBN} will
>  allocate for the contents of a value.
>  @end table
>  
> +@c Poke docs live in a separate file.
> +@include poke.texi
> +
>  @node Optimized Code
>  @chapter Debugging Optimized Code
>  @cindex optimized code, debugging
> diff --git a/gdb/doc/poke.texi b/gdb/doc/poke.texi
> new file mode 100644
> index 00000000000..d30dd88a141
> --- /dev/null
> +++ b/gdb/doc/poke.texi
> @@ -0,0 +1,373 @@
> +@c Copyright (C) 2021 Free Software Foundation, Inc.
> +@c Permission is granted to copy, distribute and/or modify this document
> +@c under the terms of the GNU Free Documentation License, Version 1.3 or
> +@c any later version published by the Free Software Foundation; with the
> +@c Invariant Sections being ``Free Software'' and ``Free Software Needs
> +@c Free Documentation'', with the Front-Cover Texts being ``A GNU Manual,''
> +@c and with the Back-Cover Texts as in (a) below.
> +@c
> +@c (a) The FSF's Back-Cover Text is: ``You are free to copy and modify
> +@c this GNU Manual.  Buying copies from GNU Press supports the FSF in
> +@c developing GNU and promoting software freedom.''
> +
> +@c XXX example: saving some memory in a file (opening other IO spaces)
> +
> +@node Poke
> +@section Poking at data using GNU poke
> +@cindex GNU poke
> +@cindex poke
> +
> +@uref{http://jemarch.net/poke.html,GNU poke} is an interactive,
> +extensible editor for binary data that implements a full-fledged
> +procedural, interactive domain specific language called @dfn{Poke}
> +(with big P) that is specifically designed in order to describe the
> +layout of structured binary data and to operate on it.
> +
> +@value{GDBN} integrates with GNU poke by mean of the @file{libpoke}
> +library and offers the possibility of executing Poke code from within
> +the debugger, to inspect and modify data in the target's memory.  This
> +feature is available only if @value{GDBN} was configured using
> +@option{--enable-poke}.
> +
> +As we shall see in the sections below, @value{GDBN} uses the
> +integration mechanisms provided by @file{libpoke} in order to make
> +certain GDB abstractions (such as symbols and types) visible from the
> +Poke side, making the integration bidirectional.
> +
> +Note that this section documents the integration of GNU poke and
> +@value{GDBN} and how to use it, but it doesn't describe GNU poke nor
> +the Poke language in detail. @xref{Top,,, poke, The GNU poke Manual}
> +for more information.
> +
> +@menu
> +* The @command{poke} Command::  Executing Poke from @value{GDBN}.
> +* Poking the Target Memory::    Accessing the target's memory from Poke.
> +* @value{GDBN} Types and Poke::         Accessing @value{GDBN} types from Poke.
> +* @value{GDBN} Values and Poke::        Accessing @value{GDBN} values from Poke.
> +@end menu
> +
> +@node The @command{poke} Command
> +@subsection The @command{poke} Command
> +
> +The @command{poke} command allows to execute arbitrary Poke code from
> +the @value{GDBN} prompt.
> +
> +@table @code
> +@item poke @var{src}
> +@var{src} is either a Poke expression, statement or definition.
> +@end table
> +
> +For example, this executes a simple Poke expression and shows the
> +result:
> +
> +@smallexample
> +(@value{GDBP}) poke 2 + 3UL
> +0x5UL
> +@end smallexample
> +
> +This declares a couple of Poke types and a variable:
> +
> +@smallexample
> +(@value{GDBP}) type byte = uint<8>
> +(@value{GDBP}) type Packet = struct @{ byte magic == 0x4a; byte sz; byte[sz] payload; @}
> +(@value{GDBP}) poke Packet @{ sz = 4 @}
> +Packet @{
> +  magic=0x4aUB,
> +  sz=0x4UB,
> +  payload=[0x0UB,0x0UB,0x0UB,0x0UB]
> +@}
> +(@value{GDBP}) var p = Packet @{ sz = 4 @}
> +(@value{GDBP}) poke p.payload
> +[0x0UB,0x0UB,0x0UB,0x0UB]
> +@end smallexample
> +
> +This executes a Poke statement:
> +
> +@smallexample
> +(@value{GDBP}) poke for (i in [1,2,3]) printf "%v\n", i
> +0x00000001
> +0x00000002
> +0x00000003
> +@end smallexample
> +
> +This shows how the Poke incremental compiler handles and reports
> +invalid input:
> +
> +@smallexample
> +(@value{GDBP}) poke 2 + fjsdio
> +<stdin>:1:5: error: undefined variable 'fjsdio'
> +2 + fjsdio;
> +    ^~~~~~
> +@end smallexample
> +
> +The standard @command{load} Poke directive loads a Poke source file
> +and executes it in the incremental compiler.  The list of directories
> +where @command{load} looks for files is in the variable
> +@code{load_path}:
> +
> +@smallexample
> +(@value{GDBP}) poke load_path
> +".:/home/jemarch/.local/share/poke:%DATADIR%/pickles:%DATADIR%"
> +@end smallexample
> +
> +This loads a file @dfn{foo.pk} if it is found in the load path:
> +
> +@smallexample
> +(@value{GDBP}) poke load foo
> +@end smallexample
> +
> +To Poke source files containing definitions that conceptually apply to
> +some definite domain, we call them @dfn{pickles}.  For example,
> +@file{elf.pk} is a pickle that provides facilities to poke ELF object
> +files.  The GNU poke editor comes with lots of already written pickles
> +for many file formats and other domains.  If you happen to have GNU
> +poke installed (and not just @file{libpoke}) you can also use the many
> +pickles distributed with the editor.  For example:
> +
> +@smallexample
> +(@value{GDBP}) poke load "std-types.pk"
> +(@value{GDBP}) poke load elf
> +(@value{GDBP}) poke Elf64_Rela @{@}
> +Elf64_Rela @{
> +  r_offset=0x0UL#B,
> +  r_info=Elf64_RelInfo @{
> +    r_sym=0x0U,
> +    r_type=0x0U
> +  @},
> +  r_addend=0x0L
> +@}
> +@end smallexample
> +
> +The reason why @file{std-types.pk} has to be loaded before
> +@file{elf.pk} is explained later in this manual.
> +
> +@node Poking the Target Memory
> +@subsection Poking the Target Memory
> +
> +@value{GDBN} configures @file{libpoke} to access the target's memory
> +as an IO space device called @code{<gdb>}, which is automatically
> +opened when the poke incremental compiler is started.
> +
> +This means that the default IO space in the running poke will provide
> +access to the virtual address space of the current @value{GDBN}
> +inferior.

I would suggest dropping "virtual" here.  GDB doesn't differentiate
between a physical and virtual address space, there's just "the
inferior's address space", and for some (e.g. bare-metal remote
targets), it might not even make sense to talk about the virtual
address space.

> +
> +For example, suppose that a string table is at offset 0x5ff0 bytes in
> +the target's memory.  We could map an array of Poke strings from it by
> +issuing:
> +
> +@smallexample
> +(@value{GDBP}) spoke string[3] @@ 0x5ff0#B
> +["int", "long", "_pid"]
> +@end smallexample
> +
> +And we can write to the target's memory:
> +
> +@smallexample
> +(@value{GDBP}) poke string[] @@ 0x5ff0#B = ["foo", "bar", "baz"]
> +@end smallexample
> +
> +Note that the fact the current IO space is the GDB target memory
> +doesn't mean you cannot access other IO spaces.  This is how you would
> +write the string table above to a file @file{strtab.out}:
> +
> +@smallexample
> +(@value{GDBP}) poke var f = open ("strtab.out", IOS_F_WRITE | IOS_F_CREATE)
> +(@value{GDBP}) poke string[] @@ f : 0#B = string[3] @@ 0x5ff0#B
> +(@value{GDBP}) poke close (f)
> +@end smallexample
> +
> +If you close the default IO space you can re-open the GDB target space
> +with @code{open ("<gdb>")}.
> +
> +@node @value{GDBN} Types and Poke
> +@subsection @value{GDBN} Types and Poke
> +
> +Maybe the strongest side of the Poke language is that it provides a
> +very rich and dynamic mechanism to describe the layout of data
> +structures.  This is done by defining @dfn{Poke types}.
> +
> +For example, this is the definition of a signed 13-bit integral type
> +that could be used to poke immediate fields in SPARC instructions:
> +
> +@smallexample
> +type simm13 = int<13>;
> +@end smallexample
> +
> +And this is a simplified version of the structure of a 64-bit ELF file
> +showing more advanced Poke capabilities like field constraints, field
> +labels, absent fields, and methods:
> +
> +@smallexample
> +type Elf64_File =
> +  struct
> +  @{
> +    Elf64_Ehdr ehdr : ehdr.e_ident.ei-mag == [0x7fUB, 'E', 'L', 'F'];
> +
> +    Elf64_Shdr[ehdr.e_shnum] shdr @@ ehdr.e_shoff
> +      if ehdr.e_shnum > 0;
> +
> +    Elf64_Phdr[ehdr.e_phnum] phdr @@ ehdr.e_phoff
> +      if ehdr.e_phnum > 0;
> +
> +    /* Given an offset into the ELF file's section string table, return
> +       the string.  */
> +
> +    method get_section_name = (offset<Elf_Word,B> offset) string:
> +      @{
> +        var strtab = ehdr.e_shstrndx;
> +        return string @@ (shdr[strtab].sh_offset + offset);
> +      @}
> +  @};
> +@end smallexample
> +
> +This is all good and well for GNU poke as a standalone binary editor,
> +but when it comes to @value{GDBN} we want to poke at data structures
> +in the target memory of the debugged program.  These structures are
> +described by language-specific types, which @value{GDBN} abstracts as
> +@value{GDBN} types, not Poke types.
> +
> +For example, say we are debugging a C program that contains the
> +following type:
> +
> +@smallexample
> +struct person
> +@{
> +  int age;
> +  char *name;
> +  char *postal_address;
> +@};
> +@end smallexample
> +
> +If we wanted to poke at a struct person from poke, we would need to
> +write a Poke struct type that is equivalent to that C type.  This is
> +often not trivial, because the physical layout of data structures is
> +almost always not well defined in programming languages.
> +
> +Fortunately, @value{GDBN} provides a few commands to translate
> +@value{GDBN} types to Poke types and inspect them.
> +
> +@table @code
> +@item poke-add-type @var{expr}
> +@var{expr} is a @value{GDBN} expression that must evaluate to a type.
> +
> +Translate a @value{GDBN} type to Poke and define it in the running
> +poke incremental compiler.  If the given type depends on other types
> +that are not known to poke, add these as well.
> +
> +Types for which @value{GDBN} doesn't know how to create a Poke
> +equivalence are simply ignored.
> +
> +@item poke-add-types @var{regexp}
> +@var{regexp} is a regular expression.
> +
> +Translate all known types whose name matches @var{regexp} to Poke and
> +define them in the running poke incremental compiler.  If the matched
> +types depend on other types that are not known to poke, add these as
> +well.
> +
> +Types for which @value{GDBN} doesn't know how to create a Poke
> +equivalence are simply ignored.
> +
> +@item poke-dump-types
> +Dump the Poke definition of all translated types, one definition per
> +line.
> +@end table
> +
> +Using these commands, we can add a type for the @code{struct person} C
> +type above like this:
> +
> +@smallexample
> +(@value{GDBN}) poke-add-type struct person
> +added type int
> +added type struct_person
> +@end smallexample
> +
> +Note how two types are added: the requested @code{struct person} and
> +also @code{int}, since the struct contains a field of that basic C
> +type. Let's take a look to the type definitions:
> +
> +@smallexample
> +(@value{GDBN}) poke-dump-types
> +type int = int<32>;
> +type struct_person = struct @{int age; offset<uint<64>,B> name @@ 8#B; offset<uint<64>,B> postal_address;@};
> +@end smallexample
> +
> +If now we want to access a given variable of type @code{struct person}
> +in the current target, we just use the created Poke types:
> +
> +@smallexample
> +(@value{GDBN}) poke struct_person @@ 0xf00e#B
> +struct_person @{
> +  age=0x28,
> +  name=0x5555555547b4UL#B,
> +  postal_address=0x5555555547c5UL#B
> +@}
> +(@value{GDBN}) poke string @@ (struct_person @@ 0xf00e#B).postal_address
> +"Foo Street number 13"
> +@end smallexample
> +
> +If we wanted to add all the types known to GDB to poke, we could so do
> +by:
> +
> +@smallexample
> +(@value{GDBN}) poke-add-types .*
> +@end smallexample
> +
> +The @command{poke-dump-types} is useful to generate Poke files with
> +type definitions to be used in GNU poke, like this:
> +
> +@smallexample
> +$ gdb -batch -ex poke-dump-types -ex quit foo.so > foo-types.pk
> +@end smallexample
> +
> +@node @value{GDBN} Values and Poke
> +@subsection @value{GDBN} Values and Poke
> +
> +Poke variables are not the same than GDB symbols, and live in a
> +separated world of their own. However, it is possible to refer to GDB
> +values by using the @code{$IDENTIFIER} notation in Poke programs.
> +
> +Consider for example a C program with the following variable:
> +
> +@smallexample
> +short counter;
> +@end smallexample
> +
> +In @value{GDBN} we can access to the value of that variable like this:
> +
> +@smallexample
> +(@value{GDBN}) counter
> +$1 = 0
> +@end smallexample

Maybe you mean "print counter", the above will just give an error.

Thanks,
Andrew

> +
> +And from the poke side:
> +
> +@smallexample
> +(@value{GDBN}) poke $counter
> +0x0H
> +@end smallexample
> +
> +Note how the @value{GDBN} value is visible using the right type, in
> +the case above a signed 16-bit integer.  If we accessed a C value of a
> +pointer type, like @code{char *str;}, we would get an offset with unit
> +bytes instead:
> +
> +@smallexample
> +(@value{GDBN}) poke $str
> +0x0UL#B
> +@end smallexample
> +
> +Since many @value{GDBN} values are pointers, it is possible to access
> +to the address of a value by using the @code{$addr::IDENTIFIER}
> +notation.  For example, given the C @code{struct person} defined above
> +and a variable @code{struct person jemarch;}:
> +
> +@smallexample
> +(@value{GDBN}) poke struct_person @@ $addr::jemarch
> +struct_person @{
> +  age=0x28,
> +  name=0x5555555547b4UL#B,
> +  postal_address=0x5555555547c5UL#B
> +@}
> +@end smallexample
> diff --git a/gdb/poke.c b/gdb/poke.c
> new file mode 100644
> index 00000000000..31b8460ad1d
> --- /dev/null
> +++ b/gdb/poke.c
> @@ -0,0 +1,762 @@
> +/* GDB integration with GNU poke.
> +
> +   Copyright (C) 2021 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 "command.h"
> +#include "arch-utils.h"
> +#include "target.h"
> +extern "C" {
> +#include <libpoke.h>
> +}
> +#include <ctype.h>
> +#include <vector>
> +#include <algorithm>
> +
> +/* Global poke incremental compiler.  */
> +
> +static pk_compiler poke_compiler;
> +static bool poke_compiler_lives = false;
> +
> +/* Global vector of the Poke code used to define types.  This is
> +   filled in by poke_add_type and used by poke_dump_types.  */
> +
> +static std::vector<std::string> type_poke_strings;
> +
> +/* Terminal hook that flushes the terminal.  */
> +
> +static void
> +poke_term_flush (void)
> +{
> +  /* Do nothing here.  */
> +}
> +
> +/* Terminal hook that prints a fixed string.  */
> +
> +static void
> +poke_puts (const char *str)
> +{
> +  printf_filtered ("%s", str);
> +}
> +
> +/* Terminal hook that prints a formatted string.  */
> +
> +__attribute__ ((__format__ (__printf__, 1, 2)))
> +static void
> +poke_printf (const char *format, ...)
> +{
> +  va_list ap;
> +  char *str;
> +  int r;
> +
> +  va_start (ap, format);
> +  r = vasprintf (&str, format, ap);
> +  if (r == -1)
> +    error (_("out of memory in vasprintf")); /* XXX fatal */
> +  va_end (ap);
> +
> +  printf_filtered ("%s", str);
> +  free (str);
> +}
> +
> +/* Terminal hook that indents to a given level.  */
> +
> +static void
> +poke_term_indent (unsigned int lvl, unsigned int step)
> +{
> +  printf_filtered ("\n%*s", (step * lvl), "");
> +}
> +
> +/* Terminal hook that starts a styling class.  */
> +
> +static void
> +poke_term_class (const char *class_name)
> +{
> +  /* Do nothing here.  */
> +}
> +
> +/* Terminal hook that finishes a styling class.  */
> +
> +static int
> +poke_term_end_class (const char *class_name)
> +{
> +  /* Just report success.  */
> +  return 1;
> +}
> +
> +/* Terminal hook that starts a terminal hyperlink.  */
> +
> +static void
> +poke_term_hyperlink (const char *url, const char *id)
> +{
> +  /* Do nothing here.  */
> +}
> +
> +/* Terminal hook that finishes a terminal hyperlink.  */
> +
> +static int
> +poke_term_end_hyperlink (void)
> +{
> +  /* Just report success.  */
> +  return 1;
> +}
> +
> +/* Terminal hook that returns the current terminal foreground
> +   color.  */
> +
> +static struct pk_color
> +poke_term_get_color (void)
> +{
> +  /* Just return the default foreground color.  */
> +  struct pk_color dfl = {-1,-1,-1};
> +  return dfl;
> +}
> +
> +/* Terminal hook that returns the current terminal background
> +   color.  */
> +
> +static struct pk_color
> +poke_term_get_bgcolor (void)
> +{
> +  /* Just return the default background color.  */
> +  struct pk_color dfl = {-1,-1,-1};
> +  return dfl;
> +}
> +
> +/* Terminal hook that sets the terminal foreground color.  */
> +
> +static void
> +poke_term_set_color (struct pk_color color)
> +{
> +  /* Do nothing.  */
> +}
> +
> +/* Terminal hook that sets the terminal background color.  */
> +
> +static void
> +poke_term_set_bgcolor (struct pk_color color)
> +{
> +  /* Do nothing.  */
> +}
> +
> +/* Implementation of the poke terminal interface, that uses the hooks
> +   defined above.  */
> +
> +static struct pk_term_if poke_term_if =
> +  {
> +    .flush_fn = poke_term_flush,
> +    .puts_fn = poke_puts,
> +    .printf_fn = poke_printf,
> +    .indent_fn = poke_term_indent,
> +    .class_fn = poke_term_class,
> +    .end_class_fn = poke_term_end_class,
> +    .hyperlink_fn = poke_term_hyperlink,
> +    .end_hyperlink_fn = poke_term_end_hyperlink,
> +    .get_color_fn = poke_term_get_color,
> +    .get_bgcolor_fn = poke_term_get_bgcolor,
> +    .set_color_fn = poke_term_set_color,
> +    .set_bgcolor_fn = poke_term_set_bgcolor,
> +  };
> +
> +/* Foreign IO device hook that returns an unique name identifying the
> +   kind of device.  */
> +
> +static const char *
> +iod_get_if_name (void)
> +{
> +  return "GDB";
> +}
> +
> +/* Foreign IO device hook that recognizes whether a given IO space
> +   handler refer to this kind of device, and normalizes it for further
> +   use.  */
> +
> +static char *
> +iod_handler_normalize (const char *handler, uint64_t flags, int *error)
> +{
> +  char *new_handler = NULL;

s/NULL/nullptr/ throughout.

> +
> +  if (strcmp (handler, "<gdb>") == 0)
> +    new_handler = xstrdup (handler);
> +  if (error)
> +    *error = PK_IOD_OK;
> +
> +  return new_handler;
> +}
> +
> +/* Foreign IO device hook that opens a new device.  */
> +
> +static int iod_opened_p = 0;
> +
> +static void *
> +iod_open (const char *handler, uint64_t flags, int *error)
> +{
> +  iod_opened_p = 1;
> +  return &iod_opened_p;
> +}
> +
> +/* Foreign IO device hook that reads data from a device.  */
> +
> +static int
> +iod_pread (void *dev, void *buf, size_t count, pk_iod_off offset)
> +{
> +  int ret = target_read_raw_memory (offset, (gdb_byte *) buf, count);
> +  return ret == -1 ? PK_IOD_ERROR : PK_IOD_OK;
> +}
> +
> +/* Foreign IO device hook that writes data to a device.  */
> +
> +static int
> +iod_pwrite (void *dev, const void *buf, size_t count, pk_iod_off offset)
> +{
> +  int ret = target_write_raw_memory (offset, (gdb_byte *) buf, count);
> +  return ret == -1 ? PK_IOD_ERROR : PK_IOD_OK;
> +}
> +
> +/* Foreign IO device hook that returns the flags of an IO device. */
> +
> +static uint64_t
> +iod_get_flags (void *dev)
> +{
> +  return PK_IOS_F_READ | PK_IOS_F_WRITE;
> +}
> +
> +/* Foreign IO device hook that returns the size of an IO device, in
> +   bytes.  */
> +
> +static pk_iod_off
> +iod_size (void *dev)
> +{
> +  return (gdbarch_addr_bit (get_current_arch ()) == 32
> +	  ? 0xffffffff : 0xffffffffffffffff);
> +}
> +
> +/* Foreign IO device hook that flushes an IO device.  */
> +
> +static int
> +iod_flush (void *dev, pk_iod_off offset)
> +{
> +  /* Do nothing here.  */
> +  return PK_OK;
> +}
> +
> +/* Foreign IO device hook that closes a given device.  */
> +
> +static int
> +iod_close (void *dev)
> +{
> +  iod_opened_p = 0;
> +  return PK_OK;
> +}
> +
> +/* Implementation of the poke foreign IO device interface, that uses
> +   the hooks defined above.  */
> +
> +static struct pk_iod_if iod_if =
> +  {
> +    iod_get_if_name,
> +    iod_handler_normalize,
> +    iod_open,
> +    iod_close,
> +    iod_pread,
> +    iod_pwrite,
> +    iod_get_flags,
> +    iod_size,
> +    iod_flush
> +  };
> +
> +/* Handler for alien tokens.  */
> +
> +static struct pk_alien_token alien_token;
> +
> +static struct pk_alien_token *
> +poke_alien_token_handler (const char *id, char **errmsg)
> +{
> +  /* In GDB alien poke tokens with the form $addr::FOO provide the
> +     address of the symbol `FOO' as an offset in bytes, i.e. it
> +     resolves to the GDB value &foo as a Poke offset with unit bytes.
> +
> +     $FOO, on the other hand, provide the value of the symbol FOO
> +     incarnated in a proper Poke value.  */
> +
> +  if (strncmp (id, "addr::", 6) == 0)

You can use startswith here.

> +    {
> +      CORE_ADDR addr;
> +
> +      std::string expr = "&";
> +      expr += id + 6;
> +
> +      try
> +	{
> +	  addr = parse_and_eval_address (expr.c_str ());
> +	}
> +      catch (const gdb_exception_error &except)
> +	{
> +	  goto error;
> +	}
> +
> +      alien_token.kind = PK_ALIEN_TOKEN_OFFSET;
> +      alien_token.value.offset.magnitude = addr;
> +      alien_token.value.offset.width = 64;

Is this 64 assuming all addresses are 64-bit?  Or an upper bound on
the size of an address?  Would it be better to base this value off of
gdbarch_ptr_bit, using the target_gdbarch?

> +      alien_token.value.offset.signed_p = 0;
> +      alien_token.value.offset.unit = 8;

I wonder what all these '.unit = 8' are representing?  GDB is pretty
much hard coded to assume 8-bit bytes on the host side, but for the
target there has been an attempt to make GDB support non-8-bit-bytes
(though, having just brought up a non-8-bit-byte target, I know there
are lots of places where this is broken currently in GDB).

My question then, is, we do have gdbarch_addressable_memory_unit_size,
which reports the number of host bytes per target byte (i.e. it
reports the target's octets per byte).  Knowing that, would you want
to change the value 8 here?

> +    }
> +  else
> +    {
> +      struct value *value;
> +
> +      try
> +	{
> +	  value = parse_and_eval (id);
> +	}
> +      catch (const gdb_exception_error &except)
> +	{
> +	  goto error;
> +	}

Rather than all of the gotos in this function, you might consider
structuring the code like:

  static struct pk_alien_token *
  poke_alien_token_handler_1 (const char *id)
  {
    ...
  }

  static struct pk_alien_token *
  poke_alien_token_handler (const char *id, char **errmsg)
  {
    try
    {
      struct pk_alien_token *res = poke_alien_token_handler_1 (id);
      if (res != nullptr)
        return res;
    }
    catch (const gdb_exception_error &except)
    {
      /* I'd consider building an error message here that includes the
         output of 'exception.what ()' which is a string describing the
         error, this might be more helpful for the user.  */
    }

    /* Build your plain error message here, we have no exception to
       guide us, but you could always throw anexception from within
       poke_alien_token_handler_1 that includes an error message.  */
  }

Just a thought.

> +
> +      struct type *type = value_type (value);
> +
> +      if (can_dereference (type))
> +	{
> +	  alien_token.kind = PK_ALIEN_TOKEN_OFFSET;
> +	  alien_token.value.offset.magnitude
> +	    = value_as_address (value);
> +	  alien_token.value.offset.width
> +	    = TYPE_LENGTH (type) * 8;
> +	  alien_token.value.offset.signed_p = 0;
> +	  alien_token.value.offset.unit = 8;
> +	}
> +      else if (is_integral_type (type))
> +	{
> +	  alien_token.kind = PK_ALIEN_TOKEN_INTEGER;
> +	  alien_token.value.integer.magnitude
> +	    = value_as_long (value);
> +	  alien_token.value.integer.width
> +	    = TYPE_LENGTH (type) * 8;
> +	  alien_token.value.integer.signed_p
> +	    = !type->is_unsigned ();
> +	}
> +      else
> +	goto error;
> +    }
> +
> +  *errmsg = NULL;
> +  return &alien_token;
> +
> + error:
> +  std::string emsg = "can't access GDB variable '";
> +  emsg += id;
> +  emsg += "'";
> +  *errmsg = xstrdup (emsg.c_str ());
> +  return NULL;
> +}
> +
> +/* Given a string, prefix it in order ot avoid collision with Poke's
> +   keywords.  */
> +
> +static std::vector<std::string> poke_keywords
> +  {
> +    "pinned", "struct", "union", "else", "while", "until",
> +    "for", "in", "where", "if", "sizeof", "fun", "method",
> +    "type", "var", "unit", "break", "continue", "return",
> +    "string", "as", "try", "catch", "raise", "void", "any",
> +    "print", "printf", "isa", "unmap", "big", "little",
> +    "load", "lambda", "assert",
> +  };
> +
> +static std::string
> +normalize_poke_identifier (std::string prefix, std::string str)
> +{
> +  if (std::find (poke_keywords.begin (),
> +		 poke_keywords.end (),
> +		 str) != poke_keywords.end ())
> +    str = prefix + str;
> +
> +  return str;
> +}
> +
> +/* Given a GDB type name, mangle it to a valid Poke type name.  */
> +
> +static std::string
> +gdb_type_name_to_poke (std::string str, struct type *type = NULL)
> +{
> +  for (int i = 0; i < str.length (); ++i)
> +    if (!(str.begin()[i] == '_'
> +	  || (str.begin()[i] >= 'a' && str.begin()[i] <= 'z')
> +	  || (str.begin()[i] >= '0' && str.begin()[i] <= '9')
> +	  || (str.begin()[i] >= 'A' && str.begin()[i] <= 'Z')))
> +      str.begin()[i] = '_';
> +
> +  if (type)
> +    {
> +      /* Prepend struct and union tags with suitable prefixes.  This
> +	 is to avoid ending with recursive typedefs in C programs.  */
> +      if (type->code () == TYPE_CODE_STRUCT)
> +	str = "struct_" + str;
> +      else if (type->code () == TYPE_CODE_UNION)
> +	str = "union_" + str;
> +    }
> +
> +  return str;
> +}
> +
> +#if 0
> +/* Command to shut down the poke incremental compiler.  */
> +
> +static void
> +poke_stop_command (const char *args, int from_tty)
> +{
> +  if (!poke_compiler_live)
> +    error (_("The poke incremental compiler is not running."));
> +
> +  pk_compiler_free (poke_compiler);
> +  poke_compiler_live = 0;
> +}
> +#endif
> +
> +/* Command to feed the poke compiler with the definition of some given
> +   GDB type.  */
> +
> +static void poke_command (const char *args, int from_tty);
> +
> +static std::string
> +poke_add_type (struct type *type)
> +{
> +  std::string type_name;
> +  std::string str = "";
> +
> +  if (type != nullptr)
> +    {
> +      if (type->name ())
> +	type_name = type->name ();
> +
> +      /* Do not try to add a type that is already defined.  */
> +      if (type_name != ""
> +	  && pk_decl_p (poke_compiler,
> +			gdb_type_name_to_poke (type_name, type).c_str (),
> +			PK_DECL_KIND_TYPE))
> +	return type_name;
> +
> +      switch (type->code ())
> +	{
> +	case TYPE_CODE_PTR:
> +	  {
> +	    str = ("offset<uint<"
> +		   + (std::to_string (TYPE_LENGTH (type) * 8))
> +		   + ">,B>");
> +	    break;
> +	  }
> +	case TYPE_CODE_TYPEDEF:
> +	  {
> +	    struct type *target_type = TYPE_TARGET_TYPE (type);
> +	    std::string target_type_code = poke_add_type (target_type);
> +
> +	    if (target_type_code == "")
> +	      goto skip;

Replace these 'goto skip' calls with just 'return {}'.

> +
> +	    if (target_type->name ())
> +	      str += gdb_type_name_to_poke (target_type->name (), target_type);
> +	    else
> +	      str += target_type_code;
> +	    break;
> +	  }
> +	case TYPE_CODE_INT:
> +	  {
> +	    size_t type_length = TYPE_LENGTH (type) * 8;
> +
> +	    if (type_length > 64)
> +	      goto skip;
> +
> +	    if (type->is_unsigned ())
> +	      str += "u";
> +	    str += "int<";
> +	    str += std::to_string (type_length);
> +	    str += ">";
> +	    break;
> +	  }
> +	case TYPE_CODE_ARRAY:
> +	  {
> +	    struct type *target_type = TYPE_TARGET_TYPE (type);
> +	    size_t target_type_length = TYPE_LENGTH (target_type);
> +	    std::string target_type_code
> +	      = poke_add_type (target_type);
> +
> +	    if (target_type_code == "")
> +	      goto skip;
> +
> +	    if (target_type->name ())
> +	      str = gdb_type_name_to_poke (target_type->name (), target_type);
> +	    else
> +	      str = target_type_code;
> +
> +	    str += "[";
> +	    str += std::to_string (TYPE_LENGTH (type) / target_type_length);
> +	    str += "]";
> +	    break;
> +	  }
> +	case TYPE_CODE_STRUCT:
> +	  {
> +	    size_t natural_bitpos = 0;
> +	    str += "struct {";
> +
> +	    for (int idx = 0; idx < type->num_fields (); idx++)
> +	      {
> +		std::string field_name
> +		  = normalize_poke_identifier ("__f", TYPE_FIELD_NAME (type, idx));
> +		struct type *field_type = type->field (idx).type ();
> +		size_t field_bitpos = TYPE_FIELD_BITPOS (type, idx);
> +
> +		if (idx > 0)
> +		  str += " ";
> +		if (field_type->name ())
> +		  {
> +		    if (poke_add_type (field_type) == "")
> +		      goto skip;
> +		    str += gdb_type_name_to_poke (field_type->name (), field_type);
> +		  }
> +		else
> +		  {
> +		    std::string pstr = poke_add_type (field_type);
> +		    if (pstr == "")
> +		      goto skip;
> +		    str += pstr;
> +		  }
> +		str += " ";
> +		if (field_name != "")
> +		  str += field_name;
> +		if (field_bitpos != natural_bitpos)
> +		  str += " @ " + (field_bitpos % 8 == 0
> +				  ? std::to_string (field_bitpos / 8) + "#B"
> +				  : std::to_string (field_bitpos) + "#b");
> +		str += ";";
> +
> +		natural_bitpos = field_bitpos + TYPE_LENGTH (field_type) * 8;
> +	      }
> +
> +	    str += "}";
> +	    break;
> +	  }
> +	default:
> +	  goto skip;
> +	  break;
> +	}
> +
> +      std::string deftype;
> +      if (type_name != "")
> +	{
> +	  std::string poke_type_name
> +	    = gdb_type_name_to_poke (type_name, type);
> +
> +	  std::string deftype = "type ";
> +	  deftype += poke_type_name;
> +	  deftype += " = ";
> +	  deftype += str;
> +
> +	  type_poke_strings.push_back (deftype);
> +	  poke_command (deftype.c_str(), 0 /* from_tty */);
> +	  printf_filtered ("added type %s\n", poke_type_name.c_str ());
> +	}
> +    }
> +
> +  return str;
> +
> + skip:
> +  return "";

I think 'return {}' is possibly more efficient than 'return ""'.

> +}
> +
> +/* Start the poke incremental compiler.  */
> +
> +static void
> +start_poke (void)
> +{
> +  /* Note how we are creating an incremental compiler without the
> +     standard Poke types (int, etc) because they collide with the C
> +     types.  */
> +  poke_compiler = pk_compiler_new_with_flags (&poke_term_if,
> +					      PK_F_NOSTDTYPES);
> +  if (poke_compiler == NULL)
> +    error (_("Couldn't start the poke incremental compiler."));
> +  poke_compiler_lives = 1;
> +
> +  /* Install the handler for alien tokens that recognizes GDB
> +     symbols.  */
> +  pk_set_alien_token_fn (poke_compiler, poke_alien_token_handler);
> +
> +  /* Use hexadecimal output by default.  */
> +  pk_set_obase (poke_compiler, 16);
> +
> +  /* Use `tree' printing mode by default.  */
> +  pk_set_omode (poke_compiler, PK_PRINT_TREE);
> +
> +  /* Set default endianness to whatever idem is used by the target
> +     architecture.  */
> +  enum bfd_endian endian = gdbarch_byte_order (get_current_arch ());
> +  pk_set_endian (poke_compiler,
> +		 endian == BFD_ENDIAN_BIG ? PK_ENDIAN_MSB : PK_ENDIAN_LSB);
> +
> +
> +  /* Install our foreign IO device interface to access the target's
> +     memory, and open it.  */
> +  if (pk_register_iod (poke_compiler, &iod_if) != PK_OK)
> +    error (_("Could not register the foreign IO device interface in poke."));
> +
> +  pk_val val;
> +  if (pk_compile_statement (poke_compiler, "open (\"<gdb>\");", NULL, &val)
> +      != PK_OK)
> +    error (_("Could not open <gdb>"));
> +}
> +
> +/* Command to dump the Poke definition of known types.  */
> +
> +static void
> +poke_dump_types (const char *args, int from_tty)
> +{
> +  if (!poke_compiler_lives)
> +    start_poke ();
> +
> +  for (const std::string &s : type_poke_strings)
> +    printf ("%s;\n", s.c_str ());

This should be printf_filtered I think.

> +}
> +
> +/* Commands to add GDB types to the running poke compiler.  */
> +
> +static void
> +poke_add_type_command (const char *args, int from_tty)
> +{
> +  if (!poke_compiler_lives)
> +    start_poke ();
> +
> +  std::string type_name = skip_spaces (args);
> +  type_name = gdb_type_name_to_poke (type_name);
> +
> +  expression_up expr = parse_expression (args);
> +  struct value *val = evaluate_type (expr.get ());
> +  struct type *type = value_type (val);
> +
> +  poke_add_type (type);
> +}
> +
> +static void
> +poke_add_types (const char *args, int from_tty)
> +{
> +  if (!poke_compiler_lives)
> +    start_poke ();
> +
> +  std::string symbol_name_regexp = skip_spaces (args);
> +  global_symbol_searcher spec (TYPES_DOMAIN, symbol_name_regexp.c_str ());
> +  std::vector<symbol_search> symbols = spec.search ();
> +  for (const symbol_search &p : symbols)
> +    {
> +      QUIT;
> +
> +      struct symbol *sym = p.symbol;
> +      struct type *type = SYMBOL_TYPE (sym);
> +
> +      if (type)
> +	poke_add_type (type);
> +    }
> +}
> +
> +/* Command to execute a poke statement or declaration.  */
> +
> +static void
> +poke_command (const char *args, int from_tty)
> +{
> +  if (!poke_compiler_lives)
> +    start_poke ();
> +
> +  int what; /* 0 -> declaration, 1 -> statement */

Maybe create a local enum instead of using an int:

  enum {
    POKE_DECLARATION = 0,
    POKE_STATEMENT
  } what;

Then the code below can be more descriptive.

> +  const char *end;
> +  std::string cmd;
> +
> +#define IS_COMMAND(input, cmd) \
> +  (strncmp ((input), (cmd), sizeof (cmd) - 1) == 0 \
> +   && ((input)[sizeof (cmd) - 1] == ' ' || (input)[sizeof (cmd) - 1] == '\t'))

Replace the strncmp with 'startswith' ?

And rather than a #define you could just use a local lambda like:

  auto is_command = [] (const char *input, const char *cmd) -> bool {
    /* Write your function here.  */
  }

then just use it as you would your #define.

> +
> +  args = skip_spaces (args);
> +  if (IS_COMMAND (args, "fun"))
> +  {
> +    what = 0;
> +    cmd = args;
> +  }
> +  else
> +    {
> +      if (IS_COMMAND (args, "var")
> +	  || IS_COMMAND (args, "type")
> +	  || IS_COMMAND (args, "unit"))
> +	what = 0;
> +      else
> +	what = 1;
> +
> +      cmd = args;
> +      cmd += ';';
> +    }
> +
> +  pk_set_lexical_cuckolding_p (poke_compiler, 1);
> +
> +  if (what == 0)
> +    {
> +      /* Declaration.  */
> +      if (pk_compile_buffer (poke_compiler, cmd.c_str (), &end) != PK_OK)
> +	goto error;

Replace 'goto error' with just return.

> +    }
> +  else
> +    {
> +      /* Statement.  */
> +      pk_val val;
> +
> +      if (pk_compile_statement (poke_compiler, cmd.c_str (), &end, &val) != PK_OK)
> +	goto error;
> +
> +      if (val != PK_NULL)
> +	{
> +	  pk_print_val (poke_compiler, val);
> +	  poke_puts ("\n");
> +	}
> +    }
> +
> +  pk_set_lexical_cuckolding_p (poke_compiler, 0);
> +#undef IS_COMMAND
> + error:
> +  ;
> +}
> +
> +/* Initialize the poke GDB subsystem.  */
> +
> +void _initialize_poke (void);
> +void
> +_initialize_poke ()
> +{
> +  /* XXX commands to set poke settings, like the output base.  */

s/XXX commands/Commands/

thanks,
Andrew


> +
> +  add_com ("poke-add-type", class_vars, poke_add_type_command, _("\
> +Make Poke aware of a GDB type given an expression.\n\
> +Usage: poke-add-type EXPRESSION\n"));
> +
> +  add_com ("poke-add-types", class_vars, poke_add_types, _("\
> +Make Poke aware of GDB types based on a regexp.\n\
> +Usage: poke-add-type REGEXP\n"));
> +
> +  add_com ("poke-dump-types", class_vars, poke_dump_types, _("\
> +Dump the definition of all the GDB types known to poke.\n\
> +Usage: poke-dump-types\n"));
> +
> +  add_com ("poke", class_vars, poke_command, _("\
> +Execute a Poke statement or declaration.\n\
> +Usage: poke [STMT]\n"));
> +}
> -- 
> 2.25.0.2.g232378479e
> 

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

* Re: [PATCH 1/1] Integrate GNU poke in GDB
  2021-05-11  7:33   ` Andrew Burgess
@ 2021-05-11 13:07     ` Jose E. Marchesi
  2021-05-12  8:52       ` Andrew Burgess
  0 siblings, 1 reply; 18+ messages in thread
From: Jose E. Marchesi @ 2021-05-11 13:07 UTC (permalink / raw)
  To: Andrew Burgess; +Cc: gdb-patches


Hi Andrew.

Thanks for the feedback.

> I have no objections to merging this functionality, especially as it
> is so self-contained, however, I do have a question, or feedback.
>
> As someone who doesn't know poke, after reading the manual, it's still
> not clear to me what significant value poke adds to GDB.  A lot of the
> examples seem to cover things that GDB already does, mostly examining
> data and variables in target memory (though there were a couple of
> neat features which aren't so easy to achieve in GDB).
>
> I guess my question, or feedback, would be, what's the killer feature
> that adding poke brings to GDB?

The most obvious example that quickly comes to mind is to check/debug
the integrity of data in an application's memory.

Suppose you have a program or driver that processes USB packets, and
buffers them in some memory buffer.  The program is malfunctioning and
you suspect (but you don't know for sure) the culprit is some corrupted
USB packet.

So you want to examine the integrity of the USB packets that the program
has buffered at certain point in the program execution.  This is where
poke gets handy: a pickle for USB packets (let's say usb.pk) will
provide types/methods/functions describing the format of USB packets,
including integrity.  It will also likely provide utility functions for
diagnostics, and even fixing invalid packets.

GDB gives you access to the C/whatever-language types, and that is very
good, but:

- C/whatever language types do not include integrity checks.  Poke types
  almost always do.

- Poke types are designed to be used/interpreted/read by persons,
  whereas C/whatever types are designed to be interpreted by programs.
  As such, Poke types tend to "adapt" their form to their contents in
  order to be intelligible and to increase the odds of detecting
  integrity problems.

- If the protocol/format/structure of the data being examined is
  bit-oriented (consider for example data compressed with deflate) the
  C/whatever types usually provide a byte-sized view of the structure
  instead of a meaningful structure that the person using GDB can easily
  understand/manipulate.  Instead, poke can _really_ operate at the bit
  level.  See the following section in the poke manual:
  http://www.jemarch.net/poke-1.2-manual/html_node/Weird-Integers.html#Weird-Integers

For example, consider the following Poke type describing a type in BTF
(the debugging format for BTF types):

  type BTF_Type =
    struct
    {
      offset<uint<32>,B> name;

      struct uint<32>
      {
        uint<1> kind_flag;
        uint<3>;
        uint<4> kind;
        uint<8>;
        uint<16> vlen;

        method _print = void:
          {
            printf ("#<%s,kind_flag:%u32d,vlen:%v>",
                    btf_kind_names[kind], kind_flag, vlen);
          }
      } info;

      union
      {
        offset<uint<32>,B> size : (info.kind in [BTF_KIND_INT,
                                                 BTF_KIND_ENUM,
                                                 BTF_KIND_STRUCT,
                                                 BTF_KIND_UNION]);
        BTF_Type_Id type_id;
      } attrs;

      type BTF_Member =
        struct
        {
          offset<uint<32>,B> name;
          BTF_Type_Id type_id;
          union
          {
            offset<uint<32>,b> member_offset : !info.kind_flag;
            struct uint<32>
            {
              offset<uint<8>,b> bitfield_size;
              offset<uint<24>,b> bit_offset;
            } bitfield;
          } offset;
        };

      type BTF_Func_Proto =
        struct
        {
          BTF_Param[info.vlen] params;
        };

      union
      {
        BTF_Int integer                    : info.kind == BTF_KIND_INT;
        BTF_Array array                    : info.kind == BTF_KIND_ARRAY;
        BTF_Enum[info.vlen] enum           : info.kind == BTF_KIND_ENUM;
        BTF_Func_Proto func_proto          : info.kind == BTF_KIND_FUNC_PROTO;
        BTF_Variable variable              : info.kind == BTF_KIND_VAR;
        BTF_Member[info.vlen] members      : (info.kind == BTF_KIND_UNION
                                              || info.kind == BTF_KIND_STRUCT);
        BTF_Var_SecInfo[info.vlen] datasec : info.kind == BTF_KIND_DATASEC;

        struct {} nothing;
      } data;

      method vararg_p = int:
        {
          var last_param = data.func_proto.params[info.vlen - 1];
          return (last_param.name == 0#B && last_param.param_type == 0);
        }

      method get_kind_name = string:
        {
          return btf_kind_names[info.kind];
        }
    };

If you map a BTF_Type from GDB, it will check the integrity (poke also
support mapping in non-strict mode) and also will build the structure
based on the very contents.  GDB simply can't do the same thing based on
the "equivalent" collection of decoupled C types, which are really not
equivalent at all.

So, back to the debugging of the USB processing program, sure, you could
dump the buffer to some file using GDB commands and then use poke on the
side to poke on it.  But having poke in GDB allows you to inspect the
buffer directly and the integrity of its contents, and also provides
access to the rest of the program memory, which could be also handy to
diagnose the problem.

Now suppose you find something fishy in a USB packet, and what you want
to do is to _fix_ it (or modify it somehow) and then continue the
program to see if that fixes the crash.  Again sure, you could use GDB
commands to load the modified dump.  But having poke in GDB allows you
to just continue.

This as for "what can poke do for GDB".  But there is another aspect to
consider as well: "what can GDB do for poke".

You see, mainly for the sake of completion, we _do_ support a "proc" IO
device in GNU poke (the command-line editor) that provides access to the
memory of a running process.  However, this support is intended only for
the very basics, and:

- It is not portable (only works on GNU/Linux).
- It doesn't know how to recognize stack frames.
- It doesn't know how to unwind.
- It doesn't have a disassembler.
- etc

When people ask me to add features like that to poke (and they do ask) I
always think: why bothering implementing them?  GDB does all these
things very well, and more, and it is portable, and it knows about a
shitload of architectures, and... .

So, in my mind, the right platform to use for poking at the memory of
live (or even dead) processes is GDB.  That's why we took pains to make
it very easy to integrate poke (libpoke) in other applications, and
therefore this proposed patch :)

Another example of integration (which is work in progress) is with the
assembler (GAS).

Instead of writing this:

 .section .text
 .set softfloat
 .ascii "PS-X EXE"
 .byte 0, 0, 0, 0, 0, 0, 0, 0
 .word main
 .word 0
 .word 0x80010000
 .word image_size
 .word 0,0,0,0
 .word 0x8001FFF0
 .word 0,0,0,0,0,0
 .ascii "Sony Computer Entertainment Inc. for zid"

You will be able to write something like:

 .poke load psxexe
 .poke var s = "Sony Computer"
 .poke PS_X_EXE { start = $main, size = $image_size, vendor = s }

(A nice side effect will be that .poke will be a _really portable_ data
 directive, unlike the regular ones, and therefore we will be able to
 write, say, the GDB and linker tests for CTF the right way,
 i.e. without relying on a compiler or copy-pased inintelligible
 .word/.byte blocks.)

I could go on and on but I don't want to pester you people so I better
stop :)

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

* Re: [PATCH 0/1] Integrate GNU poke in GDB
  2021-05-10 15:10 [PATCH 0/1] Integrate GNU poke in GDB Jose E. Marchesi
  2021-05-10 15:10 ` [PATCH 1/1] " Jose E. Marchesi
  2021-05-10 18:39 ` [PATCH 0/1] " Simon Marchi
@ 2021-05-11 18:56 ` Tom Tromey
  2021-05-12  8:06   ` Jose E. Marchesi
  2 siblings, 1 reply; 18+ messages in thread
From: Tom Tromey @ 2021-05-11 18:56 UTC (permalink / raw)
  To: Jose E. Marchesi via Gdb-patches

>>>>> "Jose" == Jose E Marchesi via Gdb-patches <gdb-patches@sourceware.org> writes:

Jose> It allows the GDB user to execute Poke code from within the
Jose> debugger with access to the target memory, types and values.

Jose> - Eventually we will probably want to ship some prewritten Poke
Jose>   code in a pickle gdb.pk.  Would $pkddatadir/poke/ be a good
Jose>   location for Poke code distributed with GDB?

Like what kind of thing are you thinking?

Jose> - How can I demangle C++ identifiers?  And how can I detect when
Jose>   a given type is a C++ one that needs demangling?

Type names normally aren't mangled.

Jose> - There are three commands:

Jose>   poke STR
Jose>   poke-add-type EXPR
Jose>   poke-add-types REGEXP
Jose>   poke-dump-types

Jose>   All three commands make sure to start the poke incremental
Jose>   compiler if it isn't running already.

It's maybe more gdb-ish to make one command and use subcommands.


I wonder if you considered implementing this by writing some Python to
glue Poke into gdb.  This would have some advantages:

* It wouldn't be a configure-time decision by whoever built gdb -- if
  you have Poke, it could "just work".  (Of course this assumes gdb is
  built with Python, but that's the norm for distros.)

* How Poke is glued in and how the commands work would be controlled by
  ordinary Poke patches, rather than having to go through GDB.  This
  would let you evolve the GDB integration along with the library.

Tom

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

* Re: [PATCH 0/1] Integrate GNU poke in GDB
  2021-05-11 18:56 ` Tom Tromey
@ 2021-05-12  8:06   ` Jose E. Marchesi
  2021-05-13 15:52     ` Tom Tromey
  0 siblings, 1 reply; 18+ messages in thread
From: Jose E. Marchesi @ 2021-05-12  8:06 UTC (permalink / raw)
  To: Tom Tromey; +Cc: Jose E. Marchesi via Gdb-patches


> Jose> It allows the GDB user to execute Poke code from within the
> Jose> debugger with access to the target memory, types and values.
>
> Jose> - Eventually we will probably want to ship some prewritten Poke
> Jose>   code in a pickle gdb.pk.  Would $pkddatadir/poke/ be a good
> Jose>   location for Poke code distributed with GDB?
>
> Like what kind of thing are you thinking?

Any Poke code that is specific to GDB in any way.  For example, this
code is part of the initialization and opens the IO space for the
current target provided by GDB:

  open ("<gdb>");

In fact in an initial version I had put that in a gdb.pk file that was
loaded in start_poke.  But since I didn't need to add anything else
there, I decided to just do it from C instead:

  if (pk_compile_statement (poke_compiler, "open (\"<gdb>\");", NULL, &val)
      != PK_OK)
    error (_("Could not open <gdb>"));

> Jose> - There are three commands:
>
> Jose>   poke STR
> Jose>   poke-add-type EXPR
> Jose>   poke-add-types REGEXP
> Jose>   poke-dump-types
>
> Jose>   All three commands make sure to start the poke incremental
> Jose>   compiler if it isn't running already.
>
> It's maybe more gdb-ish to make one command and use subcommands.

What would be the gdb-ish way:

a) poke STR
   poke add-type EXPR
   poke add-types REGEXP
   poke dump-types

or

b) poke STR
   poke add type EXPR
   poke add types REGEXP
   poke dump types

Because a) will be problematic: `add-types' can be a valid Poke
expression if both `add' and `types' are defined as variables.

Doing b) would be ok I think.

> I wonder if you considered implementing this by writing some Python to
> glue Poke into gdb.  This would have some advantages:
>
> * It wouldn't be a configure-time decision by whoever built gdb -- if
>   you have Poke, it could "just work".  (Of course this assumes gdb is
>   built with Python, but that's the norm for distros.)
>
> * How Poke is glued in and how the commands work would be controlled by
>   ordinary Poke patches, rather than having to go through GDB.  This
>   would let you evolve the GDB integration along with the library.

That is a very clever idea that no, I had not considered.

But I am not really looking forward to write Python bindings for libpoke
(or Python for anything for that matter) and even if I could recruit
someone to do that work, the stuff would need to be maintained ... poke
depending on Python, supporting future Python versions and what not,
argh no no no no :)

Also somehow I would not feel comfortable getting into such troubles and
going C++->Python->C->Poke for something that can be simply achieved as
C++->Poke in about 700 lines of C++.

So unless there is a strong feeling about this on the GDB side, I would
much prefer to integrate via libpoke directly.

PS: If GDB supported writing plugins in C or C++ like bash does, I
    wouldn't blink twice before adopting your suggestion!

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

* Re: [PATCH 1/1] Integrate GNU poke in GDB
  2021-05-11 13:07     ` Jose E. Marchesi
@ 2021-05-12  8:52       ` Andrew Burgess
  2021-05-12 10:14         ` Jose E. Marchesi
  0 siblings, 1 reply; 18+ messages in thread
From: Andrew Burgess @ 2021-05-12  8:52 UTC (permalink / raw)
  To: Jose E. Marchesi; +Cc: gdb-patches

* Jose E. Marchesi <jose.marchesi@oracle.com> [2021-05-11 15:07:42 +0200]:

> 
> Hi Andrew.
> 
> Thanks for the feedback.
> 
> > I have no objections to merging this functionality, especially as it
> > is so self-contained, however, I do have a question, or feedback.
> >
> > As someone who doesn't know poke, after reading the manual, it's still
> > not clear to me what significant value poke adds to GDB.  A lot of the
> > examples seem to cover things that GDB already does, mostly examining
> > data and variables in target memory (though there were a couple of
> > neat features which aren't so easy to achieve in GDB).
> >
> > I guess my question, or feedback, would be, what's the killer feature
> > that adding poke brings to GDB?
> 
> The most obvious example that quickly comes to mind is to check/debug
> the integrity of data in an application's memory.
> 
> Suppose you have a program or driver that processes USB packets, and
> buffers them in some memory buffer.  The program is malfunctioning and
> you suspect (but you don't know for sure) the culprit is some corrupted
> USB packet.
> 
> So you want to examine the integrity of the USB packets that the program
> has buffered at certain point in the program execution.  This is where
> poke gets handy: a pickle for USB packets (let's say usb.pk) will
> provide types/methods/functions describing the format of USB packets,
> including integrity.  It will also likely provide utility functions for
> diagnostics, and even fixing invalid packets.
> 
> GDB gives you access to the C/whatever-language types, and that is very
> good, but:
> 
> - C/whatever language types do not include integrity checks.  Poke types
>   almost always do.
> 
> - Poke types are designed to be used/interpreted/read by persons,
>   whereas C/whatever types are designed to be interpreted by programs.
>   As such, Poke types tend to "adapt" their form to their contents in
>   order to be intelligible and to increase the odds of detecting
>   integrity problems.
> 
> - If the protocol/format/structure of the data being examined is
>   bit-oriented (consider for example data compressed with deflate) the
>   C/whatever types usually provide a byte-sized view of the structure
>   instead of a meaningful structure that the person using GDB can easily
>   understand/manipulate.  Instead, poke can _really_ operate at the bit
>   level.  See the following section in the poke manual:
>   http://www.jemarch.net/poke-1.2-manual/html_node/Weird-Integers.html#Weird-Integers
> 
> For example, consider the following Poke type describing a type in BTF
> (the debugging format for BTF types):
> 
>   type BTF_Type =
>     struct
>     {
>       offset<uint<32>,B> name;
> 
>       struct uint<32>
>       {
>         uint<1> kind_flag;
>         uint<3>;
>         uint<4> kind;
>         uint<8>;
>         uint<16> vlen;
> 
>         method _print = void:
>           {
>             printf ("#<%s,kind_flag:%u32d,vlen:%v>",
>                     btf_kind_names[kind], kind_flag, vlen);
>           }
>       } info;
> 
>       union
>       {
>         offset<uint<32>,B> size : (info.kind in [BTF_KIND_INT,
>                                                  BTF_KIND_ENUM,
>                                                  BTF_KIND_STRUCT,
>                                                  BTF_KIND_UNION]);
>         BTF_Type_Id type_id;
>       } attrs;
> 
>       type BTF_Member =
>         struct
>         {
>           offset<uint<32>,B> name;
>           BTF_Type_Id type_id;
>           union
>           {
>             offset<uint<32>,b> member_offset : !info.kind_flag;
>             struct uint<32>
>             {
>               offset<uint<8>,b> bitfield_size;
>               offset<uint<24>,b> bit_offset;
>             } bitfield;
>           } offset;
>         };
> 
>       type BTF_Func_Proto =
>         struct
>         {
>           BTF_Param[info.vlen] params;
>         };
> 
>       union
>       {
>         BTF_Int integer                    : info.kind == BTF_KIND_INT;
>         BTF_Array array                    : info.kind == BTF_KIND_ARRAY;
>         BTF_Enum[info.vlen] enum           : info.kind == BTF_KIND_ENUM;
>         BTF_Func_Proto func_proto          : info.kind == BTF_KIND_FUNC_PROTO;
>         BTF_Variable variable              : info.kind == BTF_KIND_VAR;
>         BTF_Member[info.vlen] members      : (info.kind == BTF_KIND_UNION
>                                               || info.kind == BTF_KIND_STRUCT);
>         BTF_Var_SecInfo[info.vlen] datasec : info.kind == BTF_KIND_DATASEC;
> 
>         struct {} nothing;
>       } data;
> 
>       method vararg_p = int:
>         {
>           var last_param = data.func_proto.params[info.vlen - 1];
>           return (last_param.name == 0#B && last_param.param_type == 0);
>         }
> 
>       method get_kind_name = string:
>         {
>           return btf_kind_names[info.kind];
>         }
>     };
> 
> If you map a BTF_Type from GDB, it will check the integrity (poke also
> support mapping in non-strict mode) and also will build the structure
> based on the very contents.  GDB simply can't do the same thing based on
> the "equivalent" collection of decoupled C types, which are really not
> equivalent at all.

Thanks for this explanation.  For me, personally, I think the manual
entry would be much more powerful if something like the above was
included.  I feel it really gives an idea for the sort of thing that
can be achieved with poke.

Thanks for the great work,

Andrew



> 
> So, back to the debugging of the USB processing program, sure, you could
> dump the buffer to some file using GDB commands and then use poke on the
> side to poke on it.  But having poke in GDB allows you to inspect the
> buffer directly and the integrity of its contents, and also provides
> access to the rest of the program memory, which could be also handy to
> diagnose the problem.
> 
> Now suppose you find something fishy in a USB packet, and what you want
> to do is to _fix_ it (or modify it somehow) and then continue the
> program to see if that fixes the crash.  Again sure, you could use GDB
> commands to load the modified dump.  But having poke in GDB allows you
> to just continue.
> 
> This as for "what can poke do for GDB".  But there is another aspect to
> consider as well: "what can GDB do for poke".
> 
> You see, mainly for the sake of completion, we _do_ support a "proc" IO
> device in GNU poke (the command-line editor) that provides access to the
> memory of a running process.  However, this support is intended only for
> the very basics, and:
> 
> - It is not portable (only works on GNU/Linux).
> - It doesn't know how to recognize stack frames.
> - It doesn't know how to unwind.
> - It doesn't have a disassembler.
> - etc
> 
> When people ask me to add features like that to poke (and they do ask) I
> always think: why bothering implementing them?  GDB does all these
> things very well, and more, and it is portable, and it knows about a
> shitload of architectures, and... .
> 
> So, in my mind, the right platform to use for poking at the memory of
> live (or even dead) processes is GDB.  That's why we took pains to make
> it very easy to integrate poke (libpoke) in other applications, and
> therefore this proposed patch :)
> 
> Another example of integration (which is work in progress) is with the
> assembler (GAS).
> 
> Instead of writing this:
> 
>  .section .text
>  .set softfloat
>  .ascii "PS-X EXE"
>  .byte 0, 0, 0, 0, 0, 0, 0, 0
>  .word main
>  .word 0
>  .word 0x80010000
>  .word image_size
>  .word 0,0,0,0
>  .word 0x8001FFF0
>  .word 0,0,0,0,0,0
>  .ascii "Sony Computer Entertainment Inc. for zid"
> 
> You will be able to write something like:
> 
>  .poke load psxexe
>  .poke var s = "Sony Computer"
>  .poke PS_X_EXE { start = $main, size = $image_size, vendor = s }
> 
> (A nice side effect will be that .poke will be a _really portable_ data
>  directive, unlike the regular ones, and therefore we will be able to
>  write, say, the GDB and linker tests for CTF the right way,
>  i.e. without relying on a compiler or copy-pased inintelligible
>  .word/.byte blocks.)
> 
> I could go on and on but I don't want to pester you people so I better
> stop :)

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

* Re: [PATCH 1/1] Integrate GNU poke in GDB
  2021-05-12  8:52       ` Andrew Burgess
@ 2021-05-12 10:14         ` Jose E. Marchesi
  0 siblings, 0 replies; 18+ messages in thread
From: Jose E. Marchesi @ 2021-05-12 10:14 UTC (permalink / raw)
  To: Andrew Burgess; +Cc: gdb-patches


>> If you map a BTF_Type from GDB, it will check the integrity (poke also
>> support mapping in non-strict mode) and also will build the structure
>> based on the very contents.  GDB simply can't do the same thing based on
>> the "equivalent" collection of decoupled C types, which are really not
>> equivalent at all.
>
> Thanks for this explanation.  For me, personally, I think the manual
> entry would be much more powerful if something like the above was
> included.  I feel it really gives an idea for the sort of thing that
> can be achieved with poke.

Ok I am adding a section to poke.texi with an introduction.
Thanks for the feedback!

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

* Re: [PATCH 0/1] Integrate GNU poke in GDB
  2021-05-12  8:06   ` Jose E. Marchesi
@ 2021-05-13 15:52     ` Tom Tromey
  2021-05-14 20:52       ` Jose E. Marchesi
  0 siblings, 1 reply; 18+ messages in thread
From: Tom Tromey @ 2021-05-13 15:52 UTC (permalink / raw)
  To: Jose E. Marchesi via Gdb-patches; +Cc: Tom Tromey, Jose E. Marchesi

>>>>> "Jose" == Jose E Marchesi via Gdb-patches <gdb-patches@sourceware.org> writes:

Jose> poke STR
Jose> poke-add-type EXPR
Jose> poke-add-types REGEXP
Jose> poke-dump-types

>> It's maybe more gdb-ish to make one command and use subcommands.

Jose> What would be the gdb-ish way:

Jose> a) poke STR
Jose>    poke add-type EXPR
Jose>    poke add-types REGEXP
Jose>    poke dump-types

Jose> or

Jose> b) poke STR
Jose>    poke add type EXPR
Jose>    poke add types REGEXP
Jose>    poke dump types

Jose> Because a) will be problematic: `add-types' can be a valid Poke
Jose> expression if both `add' and `types' are defined as variables.

Jose> Doing b) would be ok I think.

Either is fine, though (b) is more of a pain to implement.  If the plain
prefix command is ambiguous, you can have it work like "set", where "set X"
evaluates X, but "set var" is a subcommand to avoid the ambiguous case.

Jose> But I am not really looking forward to write Python bindings for libpoke
Jose> (or Python for anything for that matter) and even if I could recruit
Jose> someone to do that work, the stuff would need to be maintained ... poke
Jose> depending on Python, supporting future Python versions and what not,
Jose> argh no no no no :)

Just to be clear, gdb often has a high burden to get a patch in.

For example, do you have a gdb copyright assignment in place?  We can't
accept your patch until that's done.

Jose> PS: If GDB supported writing plugins in C or C++ like bash does, I
Jose>     wouldn't blink twice before adopting your suggestion!

I would be opposed to this.  This would require stabilizing some gdb
API, which I think would be difficult and probably counter-productive.

Tom

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

* Re: [PATCH 1/1] Integrate GNU poke in GDB
  2021-05-10 15:10 ` [PATCH 1/1] " Jose E. Marchesi
  2021-05-10 16:56   ` Eli Zaretskii
  2021-05-11  7:33   ` Andrew Burgess
@ 2021-05-13 16:59   ` Tom Tromey
  2 siblings, 0 replies; 18+ messages in thread
From: Tom Tromey @ 2021-05-13 16:59 UTC (permalink / raw)
  To: Jose E. Marchesi via Gdb-patches

>>>>> "Jose" == Jose E Marchesi via Gdb-patches <gdb-patches@sourceware.org> writes:

Hi.  Thanks for the patch.  gdb has its own coding style, not to mention
API peculiarities and surprises, so I've gone through the patch and
mentioned the ones I could find.  I'm sorry it turned out rather long,
don't be discouraged, this is typical for a first gdb patch.

Before accepting this, I think test cases would also be needed; plus a
NEWS entry.

Probably not all the maintainers will have poke, so I'd guess any
changes needed will be done on a best-effort basis.  I see Fedora
doesn't package it, at least as of Fedora 32.

Jose> +# Integration with GNU poke is done through the libpoke library.
Jose> +AC_ARG_ENABLE([poke],
Jose> +              AS_HELP_STRING([--enable-poke],
Jose> +                             [Build GDB with poke support (default is NO)]),
Jose> +              [poke_enabled=$enableval], [poke_enabled=no])

It's normal to also support an 'auto' value, often the default; then
have this mode probe for the library.

Jose> +if test "x$poke_enabled" = "xyes"; then
Jose> +  # Note that we need a libpoke with support for registering foreign
Jose> +  # IO devices, hence the symbol pk_register_iod.
Jose> +  AC_CHECK_LIB(poke, pk_register_iod)
Jose> +  POKE_OBS="poke.o"

Probably the "yes" value should not probe, and just give a compilation
(or configure) error if the library isn't found.  The idea here is that
this is what the user asked for.

Jose> +++ b/gdb/doc/gdb.texinfo
Jose> @@ -10245,6 +10245,7 @@ being passed the type of @var{arg} as the argument.
Jose>  * Caching Target Data::         Data caching for targets
Jose>  * Searching Memory::            Searching memory for a sequence of bytes
Jose>  * Value Sizes::                 Managing memory allocated for values
Jose> +* Poke::                        Poking at data using GNU poke.

I think it would be better if this were conditional on Poke actually
being compiled in.

Jose> +static void
Jose> +poke_term_flush (void)

No need for (void) in C++.

Jose> +/* Terminal hook that prints a fixed string.  */
Jose> +
Jose> +static void
Jose> +poke_puts (const char *str)
Jose> +{
Jose> +  printf_filtered ("%s", str);
Jose> +}

printf_filtered can throw an exception if the user chooses 'q' at a
pagination prompt.  Is this ok for poke?

I don't think other exceptions can be thrown, but it's hard to know for
sure.

Jose> +/* Terminal hook that prints a formatted string.  */
Jose> +
Jose> +__attribute__ ((__format__ (__printf__, 1, 2)))

ATTRIBUTE_PRINTF

Jose> +static void
Jose> +poke_printf (const char *format, ...)
Jose> +{
Jose> +  va_list ap;
Jose> +  char *str;
Jose> +  int r;
Jose> +
Jose> +  va_start (ap, format);
Jose> +  r = vasprintf (&str, format, ap);
Jose> +  if (r == -1)
Jose> +    error (_("out of memory in vasprintf")); /* XXX fatal */
Jose> +  va_end (ap);
Jose> +
Jose> +  printf_filtered ("%s", str);

This can all be replaced with vprintf_filtered.

Jose> +/* Terminal hook that sets the terminal background color.  */
Jose> +
Jose> +static void
Jose> +poke_term_set_bgcolor (struct pk_color color)
Jose> +{
Jose> +  /* Do nothing.  */
Jose> +}

I don't know what this is for, but gdb has a styling system that can be
used.

Jose> +/* Implementation of the poke terminal interface, that uses the hooks
Jose> +   defined above.  */
Jose> +
Jose> +static struct pk_term_if poke_term_if =
Jose> +  {
Jose> +    .flush_fn = poke_term_flush,

I don't think this is C++ syntax.
Maybe it's a GNU extension, but gdb only uses those when it knows it is
being compiled with gcc.

Jose> +static char *
Jose> +iod_handler_normalize (const char *handler, uint64_t flags, int *error)
Jose> +{
Jose> +  char *new_handler = NULL;
Jose> +
Jose> +  if (strcmp (handler, "<gdb>") == 0)
Jose> +    new_handler = xstrdup (handler);
Jose> +  if (error)
Jose> +    *error = PK_IOD_OK;
Jose> +
Jose> +  return new_handler;

If poke owns this memory, then I suspect you'll have problems on
Windows, because IIRC different libraries may get different malloc/free
implementations, and so passing ownership like this isn't ok.

Not sure if that's correct, you may want to double-check.

Jose> +  return (gdbarch_addr_bit (get_current_arch ()) == 32
Jose> +	  ? 0xffffffff : 0xffffffffffffffff);

GNU style here is to add extra parens so that the second line lines up
with the expression on the first line.

Jose> +static struct pk_alien_token *
Jose> +poke_alien_token_handler (const char *id, char **errmsg)
Jose> +{
Jose> +  /* In GDB alien poke tokens with the form $addr::FOO provide the
Jose> +     address of the symbol `FOO' as an offset in bytes, i.e. it
Jose> +     resolves to the GDB value &foo as a Poke offset with unit bytes.
Jose> +
Jose> +     $FOO, on the other hand, provide the value of the symbol FOO
Jose> +     incarnated in a proper Poke value.  */
Jose> +
Jose> +  if (strncmp (id, "addr::", 6) == 0)

startswith

Jose> +    {
Jose> +      CORE_ADDR addr;
Jose> +
Jose> +      std::string expr = "&";
Jose> +      expr += id + 6;
Jose> +
Jose> +      try
Jose> +	{
Jose> +	  addr = parse_and_eval_address (expr.c_str ());
Jose> +	}

"&" is valid in C, but there's no guarantee it is correct for all the
languages gdb supports.  I think it would be better to parse-and-eval,
then use the value API to take the address, the convert using
value_as_address.

Jose> +      catch (const gdb_exception_error &except)
Jose> +	{
Jose> +	  goto error;
Jose> +	}

This will let through gdb_exception_quit, which can happen if the user
types C-c during the evaluation.  You probably want to catch
gdb_exception instead.

Jose> +      if (can_dereference (type))

I didn't know about this function.  It's only used by
annotations... hmm.

Classically gdb lets you dereference a plain int.  Dunno if you want to
support that.

Jose> +  *errmsg = NULL;
Jose> +  return &alien_token;

If this is the only use, the definition might as well be put inside this
function.

Jose> +
Jose> + error:
Jose> +  std::string emsg = "can't access GDB variable '";
Jose> +  emsg += id;
Jose> +  emsg += "'";
Jose> +  *errmsg = xstrdup (emsg.c_str ());

You can use xstrprintf or concat here, instead of a std::string.

Jose> +static std::vector<std::string> poke_keywords
Jose> +  {
Jose> +    "pinned", "struct", "union", "else", "while", "until",
Jose> +    "for", "in", "where", "if", "sizeof", "fun", "method",
Jose> +    "type", "var", "unit", "break", "continue", "return",
Jose> +    "string", "as", "try", "catch", "raise", "void", "any",
Jose> +    "print", "printf", "isa", "unmap", "big", "little",
Jose> +    "load", "lambda", "assert",
Jose> +  };
Jose> +
Jose> +static std::string
Jose> +normalize_poke_identifier (std::string prefix, std::string str)
Jose> +{
Jose> +  if (std::find (poke_keywords.begin (),
Jose> +		 poke_keywords.end (),
Jose> +		 str) != poke_keywords.end ())
Jose> +    str = prefix + str;
Jose> +
Jose> +  return str;
Jose> +}

I think it's probably better to use a plain C array of const char * for
poke_keywords.  Not sure if std::find will work, but an ordinary foreach
is also fine.

I think both arguments can be 'const std::string &' to avoid copying.

Jose> +static std::string
Jose> +gdb_type_name_to_poke (std::string str, struct type *type = NULL)

nullptr

Jose> +{
Jose> +  for (int i = 0; i < str.length (); ++i)
Jose> +    if (!(str.begin()[i] == '_'
Jose> +	  || (str.begin()[i] >= 'a' && str.begin()[i] <= 'z')
Jose> +	  || (str.begin()[i] >= '0' && str.begin()[i] <= '9')
Jose> +	  || (str.begin()[i] >= 'A' && str.begin()[i] <= 'Z')))
Jose> +      str.begin()[i] = '_';

You can just use str[i], no need for .begin().
gdb would use the ctype macros here, or maybe the ones in safe-ctype.h.

Jose> +  if (type)

type != nullptr

Jose> +    {
Jose> +      /* Prepend struct and union tags with suitable prefixes.  This
Jose> +	 is to avoid ending with recursive typedefs in C programs.  */
Jose> +      if (type->code () == TYPE_CODE_STRUCT)
Jose> +	str = "struct_" + str;
Jose> +      else if (type->code () == TYPE_CODE_UNION)
Jose> +	str = "union_" + str;

Before using a type in gdb, you normally have to call check_typedef.
However, I don't know if that's appropriate here or not, since I don't
really know what this is doing.  How does poke handle typedefs?

Jose> +#if 0
Jose> +/* Command to shut down the poke incremental compiler.  */

I guess this is the deletion thing.  Anyway, FAOD, we don't want new
dead code.

What does the incremental compiler do exactly?

Jose> +/* Command to feed the poke compiler with the definition of some given
Jose> +   GDB type.  */
Jose> +
Jose> +static void poke_command (const char *args, int from_tty);
Jose> +
Jose> +static std::string
Jose> +poke_add_type (struct type *type)
Jose> +{

Comment should be just before the function.

Jose> +  std::string str = "";

You can drop the "".

Jose> +      if (type->name ())

!= nullptr

Jose> +	case TYPE_CODE_TYPEDEF:
Jose> +	  {
Jose> +	    struct type *target_type = TYPE_TARGET_TYPE (type);
Jose> +	    std::string target_type_code = poke_add_type (target_type);
Jose> +
Jose> +	    if (target_type_code == "")

Normally you'd use 'target_type_code.empty ()' here.
I think I missed another spot like this.

Jose> +	case TYPE_CODE_INT:

You can probably handle enums like ints.

Jose> +	case TYPE_CODE_ARRAY:
Jose> +	  {
Jose> +	    struct type *target_type = TYPE_TARGET_TYPE (type);
Jose> +	    size_t target_type_length = TYPE_LENGTH (target_type);

You need a check_typedef in here.

Jose> +	    if (target_type->name ())

nullptr

Jose> +	    str += std::to_string (TYPE_LENGTH (type) / target_type_length);

I don't think this will work as you expect.  Normally you need to fetch
the bounds of the array.  This can't always be done, though; for example
in C, a VLA's bounds depend on some value, so the dynamic type can't be
resolved in isolation.

Not sure if this is an issue here or not.  You can probably just skip
dynamically-sized arrays.

Jose> +	case TYPE_CODE_STRUCT:

Don't know if it matters to poke, but unions can often be handled like
structs.

Jose> +	  {
Jose> +	    size_t natural_bitpos = 0;
Jose> +	    str += "struct {";
Jose> +
Jose> +	    for (int idx = 0; idx < type->num_fields (); idx++)
Jose> +	      {
Jose> +		std::string field_name
Jose> +		  = normalize_poke_identifier ("__f", TYPE_FIELD_NAME (type, idx));
Jose> +		struct type *field_type = type->field (idx).type ();
Jose> +		size_t field_bitpos = TYPE_FIELD_BITPOS (type, idx);

You probably want to skip static fields in this loop.

Jose> +		    if (poke_add_type (field_type) == "")
Jose> +		      goto skip;
Jose> +		    str += gdb_type_name_to_poke (field_type->name (), field_type);

I forgot to mention something else ... this seems to assume that a given
name corresponds to a single type.  Is that true?  I wasn't really sure
from the code.  Anyway -- this isn't always the case, it's relatively
normal in C programs to have an opaque type that has a different meaning
in different compilation units.  How will poke handle this?

You may also want this to skip virtual base classes, depending on
whether poke can handle those.

And you may want to skip all dynamic types generally, maybe somewhere a
bit higher up.  Not sure.  These are types whose layout depends on other
values -- not super common in C (just VLA), but relatively more so in
Ada and Rust and maybe Fortran.

Jose> +		natural_bitpos = field_bitpos + TYPE_LENGTH (field_type) * 8;

I didn't understand this.  For example, a bitfield will have an explicit
length that isn't the length of its underlying type.

It's probably better to just remove the "natural_bitpos" logic and defer
to what gdb already knows about the type.  However, I don't know if this
has some kind of bad effect on poke.

Also there may be something weird about bit positions on big-endian systems.
I'm never quite clear on that.

Jose> +	  type_poke_strings.push_back (deftype);
Jose> +	  poke_command (deftype.c_str(), 0 /* from_tty */);

I think it would be better to factor out a common helper instead.

Jose> +	  printf_filtered ("added type %s\n", poke_type_name.c_str ());

Was this some kind of debugging thing, or is it needed?

Jose> + skip:
Jose> +  return "";

I guess all those 'goto's can just be 'return {};'

Jose> +static void
Jose> +start_poke (void)

(void)

Jose> +  if (poke_compiler == NULL)
Jose> +    error (_("Couldn't start the poke incremental compiler."));
Jose> +  poke_compiler_lives = 1;

I was wondering if all the code after this point in this function
matters.  If so, then probably this flag shouldn't be set until then.

Also, it should be bool and use true/false.

Jose> +/* Commands to add GDB types to the running poke compiler.  */
Jose> +
Jose> +static void
Jose> +poke_add_type_command (const char *args, int from_tty)
Jose> +{

Jose> +  std::string type_name = skip_spaces (args);
Jose> +  type_name = gdb_type_name_to_poke (type_name);
Jose> +
Jose> +  expression_up expr = parse_expression (args);

This seems to use 'args' twice for two different things.

Jose> +
Jose> +static void
Jose> +poke_add_types (const char *args, int from_tty)

Intro comment.

Jose> +  std::string symbol_name_regexp = skip_spaces (args);

Better to use a const char * here -- avoids a copy.

Jose> +  int what; /* 0 -> declaration, 1 -> statement */

gdb prefers bool in new code.

Jose> +  const char *end;
Jose> +  std::string cmd;
Jose> +
Jose> +#define IS_COMMAND(input, cmd) \
Jose> +  (strncmp ((input), (cmd), sizeof (cmd) - 1) == 0 \
Jose> +   && ((input)[sizeof (cmd) - 1] == ' ' || (input)[sizeof (cmd) - 1] == '\t'))
Jose> +
Jose> +  args = skip_spaces (args);
Jose> +  if (IS_COMMAND (args, "fun"))
Jose> +  {
Jose> +    what = 0;
Jose> +    cmd = args;
Jose> +  }
Jose> +  else
Jose> +    {
Jose> +      if (IS_COMMAND (args, "var")
Jose> +	  || IS_COMMAND (args, "type")
Jose> +	  || IS_COMMAND (args, "unit"))

One idea here would be to register a command prefix and sub-commands
instead.  This will get completion, abbreviation, and better 'help'
handling.  I get that this is some kind of poke built-in thing, though,
so maybe you'd rather not.

Jose> +
Jose> +  if (what == 0)
Jose> +    {
Jose> +      /* Declaration.  */
Jose> +      if (pk_compile_buffer (poke_compiler, cmd.c_str (), &end) != PK_OK)
Jose> +	goto error;

Normally in gdb, a command will call error() on an error, to stop
execution of command sequences at that point.  Swallowing an error is
sometimes ok, but I wonder if that's the case here.

Does poke itself print something if this failed?
If not, the user should get some indication of the problem.

Jose> +/* Initialize the poke GDB subsystem.  */
Jose> +
Jose> +void _initialize_poke (void);

(void)

Jose> +  /* XXX commands to set poke settings, like the output base.  */

gdb has an output radix, not sure if that's useful.

Tom

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

* Re: [PATCH 0/1] Integrate GNU poke in GDB
  2021-05-10 18:39 ` [PATCH 0/1] " Simon Marchi
  2021-05-10 20:07   ` Jose E. Marchesi
@ 2021-05-13 17:04   ` Tom Tromey
  1 sibling, 0 replies; 18+ messages in thread
From: Tom Tromey @ 2021-05-13 17:04 UTC (permalink / raw)
  To: Simon Marchi via Gdb-patches

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

Simon> I don't find this super clear, as we could always see it both
Simon> ways.

One my pet peeves about autoconf is that the --with/--enable distinction
is too subtle to ever remember or care about.  Like, what would it mean
if a program had both --enable-x and --with-x?  I think it would be
better to just declare them as synonyms.

Simon> From target_read_raw_memory's doc:

Simon> /* Like target_read_memory, but specify explicitly that this is a read
Simon>    from the target's raw memory.  That is, this read bypasses the
Simon>    dcache, breakpoint shadowing, etc.  */

Simon> So, it bypasses a cache in GDB (not sure why that's important).

gdb tries to cache some memory to avoid excess requests to the remote.
This matters if you have a slow connection.

>> - How can I demangle C++ identifiers?  And how can I detect when
>> a given type is a C++ one that needs demangling?

Simon> I think that we usually rely on the language of the compilation unit, as
Simon> given by DWARF, to know that a symbol has a C++ mangled name and needs
Simon> demangling.

I can't remember if I mentioned this elsewhere, but type names aren't
normally mangled.  Symbol names for variables or functions can be, but
gdb already does the demangling if you use the correct accessor.

Tom

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

* Re: [PATCH 0/1] Integrate GNU poke in GDB
  2021-05-13 15:52     ` Tom Tromey
@ 2021-05-14 20:52       ` Jose E. Marchesi
  0 siblings, 0 replies; 18+ messages in thread
From: Jose E. Marchesi @ 2021-05-14 20:52 UTC (permalink / raw)
  To: Tom Tromey; +Cc: Jose E. Marchesi via Gdb-patches


>>>>>> "Jose" == Jose E Marchesi via Gdb-patches <gdb-patches@sourceware.org> writes:
>
> Jose> poke STR
> Jose> poke-add-type EXPR
> Jose> poke-add-types REGEXP
> Jose> poke-dump-types
>
>>> It's maybe more gdb-ish to make one command and use subcommands.
>
> Jose> What would be the gdb-ish way:
>
> Jose> a) poke STR
> Jose>    poke add-type EXPR
> Jose>    poke add-types REGEXP
> Jose>    poke dump-types
>
> Jose> or
>
> Jose> b) poke STR
> Jose>    poke add type EXPR
> Jose>    poke add types REGEXP
> Jose>    poke dump types
>
> Jose> Because a) will be problematic: `add-types' can be a valid Poke
> Jose> expression if both `add' and `types' are defined as variables.
>
> Jose> Doing b) would be ok I think.
>
> Either is fine, though (b) is more of a pain to implement.  If the plain
> prefix command is ambiguous, you can have it work like "set", where "set X"
> evaluates X, but "set var" is a subcommand to avoid the ambiguous case.

Ok, I will go with a) using that strategy.

> Jose> But I am not really looking forward to write Python bindings for libpoke
> Jose> (or Python for anything for that matter) and even if I could recruit
> Jose> someone to do that work, the stuff would need to be maintained ... poke
> Jose> depending on Python, supporting future Python versions and what not,
> Jose> argh no no no no :)
>
> Just to be clear, gdb often has a high burden to get a patch in.

That's pefectly fine.  I have contributed to GDB before.

> For example, do you have a gdb copyright assignment in place?  We can't
> accept your patch until that's done.

I can contribute it under my employer's assignment.

I will sign a personal assignment for GDB anyway; I looked in fencepost
and I don't have one in place.  It is super fast and easy to do so
nowadays using PDF documents (at least from Germany.)

Thanks!

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

end of thread, other threads:[~2021-05-14 20:52 UTC | newest]

Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-05-10 15:10 [PATCH 0/1] Integrate GNU poke in GDB Jose E. Marchesi
2021-05-10 15:10 ` [PATCH 1/1] " Jose E. Marchesi
2021-05-10 16:56   ` Eli Zaretskii
2021-05-10 18:49     ` Jose E. Marchesi
2021-05-10 18:52       ` Eli Zaretskii
2021-05-11  7:33   ` Andrew Burgess
2021-05-11 13:07     ` Jose E. Marchesi
2021-05-12  8:52       ` Andrew Burgess
2021-05-12 10:14         ` Jose E. Marchesi
2021-05-13 16:59   ` Tom Tromey
2021-05-10 18:39 ` [PATCH 0/1] " Simon Marchi
2021-05-10 20:07   ` Jose E. Marchesi
2021-05-11  6:25     ` Andrew Burgess
2021-05-13 17:04   ` Tom Tromey
2021-05-11 18:56 ` Tom Tromey
2021-05-12  8:06   ` Jose E. Marchesi
2021-05-13 15:52     ` Tom Tromey
2021-05-14 20:52       ` Jose E. Marchesi

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