[RFC][PATCH 0/7] Patching new code in a binary

[RFC][PATCH 0/7] Patching new code in a binary

[paul-naert]

From: Paul Naert <paul_naert@hotmail.fr>

Hi,
I'm looking for input on a series of patches that I wrote to extend the compile command. At the moment, this commands lets the user type in C/C++ code that is compiled by GCC and then executed in the context of the program.
The idea of the new 'patch' command is to inject the code generated by the compile command directly in the binary instead of executing it once. This makes it possible to dynamically instrument code, either for tracing, checking the behavior of a modification/bug fix without recompiling everything, or creating verification tools based on GDB.

The injection of the code is done in a similar fashion to GDB tracepoints, by replacing the original instruction with a JUMP instruction to a trampoline.
The trampoline :
 - saves the registers
 - loads the register argument of the gdb_expr() function into memory. The gdb_expr() function is the instrumentation code compiled by GCC (I've re-used the infrastructure of the compile command to compile and load the code into memory)
 - Restores the registers
 - Executes the displaced original instruction
 - Jumps to the next instruction.

I have implemented this functionality on the x86_64 architecture. I have tried putting code in the proper files, but I am not exactly sure for everything.
In this series of patches I introduced new commands, but I did not fully comply with the contribution checklist as this is a RFC.
If there is interest in the commands, I will work on the documentation and test cases.

Known issues :
- Vector registers. Even when we ask GCC not to use the vector registers, they are still affected sometimes by the isntrumentation code. This means that we need to save and restore them every time, which brings a performance cost. In this series of patches, saving and restoring vector registers is not included.
- Red zone handling. This complicates the trampoline. I believe that this issue was fixed by the manipulation I did in the trampoline, but this seems to be absent from the GDB tracepoints. Is there something I missed or is this a bug of fast tracepoints?
- Since we save and restore all registers, some instrumentation code may not be able to properly modify variable values.
- In this series of patches, only instructions >= 5 bytes can be instrumented, as is the case with fast tracepoints.

Future work :
If the community is interested in this functionnality, I have also worked on making it more usable by getting rid of the 5 byte limitation using instruction punning. This series of patches works, but is not compliant yet with the GDB standards.

Thanks for your feedback,
Paul NAERT

Paul Naert (1):
  Add the patch command to dynamically inject code in the inferior

paul-naert (6):
  Add addr argument to infcall_mmap
  Change memory protection for compile arguments to RW
  Make create_sals_from_location_default non static
  Add address argument to compile_to_object
  Add the option to defer register storage for compiled object loading. 
  Convert short jumps to long ones in amd64_relocate_instruction    


 gdb/Makefile.in                   |   3 +-
 gdb/amd64-tdep.c                  | 231 ++++++++++++++++++++++++++-
 gdb/arch-utils.c                  |   2 +-
 gdb/arch-utils.h                  |   2 +-
 gdb/breakpoint.c                  |  10 +-
 gdb/breakpoint.h                  |   5 +
 gdb/compile/compile-c-support.c   |   5 +
 gdb/compile/compile-internal.h    |  18 +++
 gdb/compile/compile-object-load.c |  61 +++++---
 gdb/compile/compile-object-load.h |   7 +
 gdb/compile/compile-object-run.c  |   6 +
 gdb/compile/compile-patch.c       | 322 ++++++++++++++++++++++++++++++++++++++
 gdb/compile/compile.c             |  24 ++-
 gdb/defs.h                        |   3 +
 gdb/gdbarch.c                     |  86 +++++++++-
 gdb/gdbarch.h                     |  17 +-
 gdb/gdbarch.sh                    |   3 +-
 gdb/linux-tdep.c                  |   4 +-
 18 files changed, 762 insertions(+), 47 deletions(-)
 create mode 100644 gdb/compile/compile-patch.c

-- 
2.7.4

[RFC][PATCH 1/7] Add addr argument to infcall_mmap

[paul-naert]

The addr argument of mmap was always set to 0 in infcall_mmap.
This allows setting it to another value, making it possible to
map specific pages in memory. This is useful when we need to jump
from the code to the page using a 32bit jump offset for instance,
as we can ask for a page within range.
---
 gdb/arch-utils.c                  | 2 +-
 gdb/arch-utils.h                  | 2 +-
 gdb/compile/compile-object-load.c | 6 +++---
 gdb/gdbarch.c                     | 4 ++--
 gdb/gdbarch.h                     | 4 ++--
 gdb/gdbarch.sh                    | 3 ++-
 gdb/linux-tdep.c                  | 4 ++--
 7 files changed, 13 insertions(+), 12 deletions(-)

diff --git a/gdb/arch-utils.c b/gdb/arch-utils.c
index 2129c3b..5c95ce0 100644
--- a/gdb/arch-utils.c
+++ b/gdb/arch-utils.c
@@ -889,7 +889,7 @@ default_skip_permanent_breakpoint (struct regcache *regcache)
 }
 
 CORE_ADDR
-default_infcall_mmap (CORE_ADDR size, unsigned prot)
+default_infcall_mmap (CORE_ADDR addr, CORE_ADDR size, unsigned prot)
 {
   error (_("This target does not support inferior memory allocation by mmap."));
 }
diff --git a/gdb/arch-utils.h b/gdb/arch-utils.h
index 48ff3bb..bd9d943 100644
--- a/gdb/arch-utils.h
+++ b/gdb/arch-utils.h
@@ -245,7 +245,7 @@ extern void default_skip_permanent_breakpoint (struct regcache *regcache);
 #define GDB_MMAP_PROT_WRITE	0x2	/* Page can be written.  */
 #define GDB_MMAP_PROT_EXEC	0x4	/* Page can be executed.  */
 
-extern CORE_ADDR default_infcall_mmap (CORE_ADDR size, unsigned prot);
+extern CORE_ADDR default_infcall_mmap (CORE_ADDR addr, CORE_ADDR size, unsigned prot);
 extern void default_infcall_munmap (CORE_ADDR addr, CORE_ADDR size);
 extern std::string default_gcc_target_options (struct gdbarch *gdbarch);
 extern const char *default_gnu_triplet_regexp (struct gdbarch *gdbarch);
diff --git a/gdb/compile/compile-object-load.c b/gdb/compile/compile-object-load.c
index a30c557..3078973 100644
--- a/gdb/compile/compile-object-load.c
+++ b/gdb/compile/compile-object-load.c
@@ -129,7 +129,7 @@ setup_sections (bfd *abfd, asection *sect, void *data_voidp)
 
       if (data->last_size != 0)
 	{
-	  addr = gdbarch_infcall_mmap (target_gdbarch (), data->last_size,
+	  addr = gdbarch_infcall_mmap (target_gdbarch (), 0, data->last_size,
 				       data->last_prot);
 	  data->munmap_list->add (addr, data->last_size);
 	  if (compile_debug)
@@ -753,7 +753,7 @@ compile_object_load (const compile_file_names &file_names,
   else
     {
       /* Use read-only non-executable memory protection.  */
-      regs_addr = gdbarch_infcall_mmap (target_gdbarch (),
+      regs_addr = gdbarch_infcall_mmap (target_gdbarch (), 0,
 					TYPE_LENGTH (regs_type),
 					GDB_MMAP_PROT_READ);
       gdb_assert (regs_addr != 0);
@@ -774,7 +774,7 @@ compile_object_load (const compile_file_names &file_names,
       if (out_value_type == NULL)
 	return NULL;
       check_typedef (out_value_type);
-      out_value_addr = gdbarch_infcall_mmap (target_gdbarch (),
+      out_value_addr = gdbarch_infcall_mmap (target_gdbarch (), 0,
 					     TYPE_LENGTH (out_value_type),
 					     (GDB_MMAP_PROT_READ
 					      | GDB_MMAP_PROT_WRITE));
diff --git a/gdb/gdbarch.c b/gdb/gdbarch.c
index fa6be50..d049771 100644
--- a/gdb/gdbarch.c
+++ b/gdb/gdbarch.c
@@ -5010,13 +5010,13 @@ set_gdbarch_vsyscall_range (struct gdbarch *gdbarch,
 }
 
 CORE_ADDR
-gdbarch_infcall_mmap (struct gdbarch *gdbarch, CORE_ADDR size, unsigned prot)
+gdbarch_infcall_mmap (struct gdbarch *gdbarch, CORE_ADDR addr, CORE_ADDR size, unsigned prot)
 {
   gdb_assert (gdbarch != NULL);
   gdb_assert (gdbarch->infcall_mmap != NULL);
   if (gdbarch_debug >= 2)
     fprintf_unfiltered (gdb_stdlog, "gdbarch_infcall_mmap called\n");
-  return gdbarch->infcall_mmap (size, prot);
+  return gdbarch->infcall_mmap (addr, size, prot);
 }
 
 void
diff --git a/gdb/gdbarch.h b/gdb/gdbarch.h
index 01b5aef..054a077 100644
--- a/gdb/gdbarch.h
+++ b/gdb/gdbarch.h
@@ -1582,8 +1582,8 @@ extern void set_gdbarch_vsyscall_range (struct gdbarch *gdbarch, gdbarch_vsyscal
    PROT has GDB_MMAP_PROT_* bitmask format.
    Throw an error if it is not possible.  Returned address is always valid. */
 
-typedef CORE_ADDR (gdbarch_infcall_mmap_ftype) (CORE_ADDR size, unsigned prot);
-extern CORE_ADDR gdbarch_infcall_mmap (struct gdbarch *gdbarch, CORE_ADDR size, unsigned prot);
+typedef CORE_ADDR (gdbarch_infcall_mmap_ftype) (CORE_ADDR addr, CORE_ADDR size, unsigned prot);
+extern CORE_ADDR gdbarch_infcall_mmap (struct gdbarch *gdbarch, CORE_ADDR addr, CORE_ADDR size, unsigned prot);
 extern void set_gdbarch_infcall_mmap (struct gdbarch *gdbarch, gdbarch_infcall_mmap_ftype *infcall_mmap);
 
 /* Deallocate SIZE bytes of memory at ADDR in inferior from gdbarch_infcall_mmap.
diff --git a/gdb/gdbarch.sh b/gdb/gdbarch.sh
index 62f68dc..9030e30 100755
--- a/gdb/gdbarch.sh
+++ b/gdb/gdbarch.sh
@@ -1174,8 +1174,9 @@ m;int;vsyscall_range;struct mem_range *range;range;;default_vsyscall_range;;0
 
 # Allocate SIZE bytes of PROT protected page aligned memory in inferior.
 # PROT has GDB_MMAP_PROT_* bitmask format.
+# mmap uses the addr argument as a hint of where to allocate the page.
 # Throw an error if it is not possible.  Returned address is always valid.
-f;CORE_ADDR;infcall_mmap;CORE_ADDR size, unsigned prot;size, prot;;default_infcall_mmap;;0
+f;CORE_ADDR;infcall_mmap;CORE_ADDR addr, CORE_ADDR size, unsigned prot;addr, size, prot;;default_infcall_mmap;;0
 
 # Deallocate SIZE bytes of memory at ADDR in inferior from gdbarch_infcall_mmap.
 # Print a warning if it is not possible.
diff --git a/gdb/linux-tdep.c b/gdb/linux-tdep.c
index 567b01c..83f4b6e 100644
--- a/gdb/linux-tdep.c
+++ b/gdb/linux-tdep.c
@@ -2317,7 +2317,7 @@ linux_vsyscall_range (struct gdbarch *gdbarch, struct mem_range *range)
 /* See gdbarch.sh 'infcall_mmap'.  */
 
 static CORE_ADDR
-linux_infcall_mmap (CORE_ADDR size, unsigned prot)
+linux_infcall_mmap (CORE_ADDR addr, CORE_ADDR size, unsigned prot)
 {
   struct objfile *objf;
   /* Do there still exist any Linux systems without "mmap64"?
@@ -2333,7 +2333,7 @@ linux_infcall_mmap (CORE_ADDR size, unsigned prot)
   struct value *arg[ARG_LAST];
 
   arg[ARG_ADDR] = value_from_pointer (builtin_type (gdbarch)->builtin_data_ptr,
-				      0);
+				      addr);
   /* Assuming sizeof (unsigned long) == sizeof (size_t).  */
   arg[ARG_LENGTH] = value_from_ulongest
 		    (builtin_type (gdbarch)->builtin_unsigned_long, size);
-- 
2.7.4

[RFC][PATCH 2/7] Change memory protection for compile arguments to RW

[paul-naert]

The registers need to be updated when gdb_expr is not executed instantly
after being compiled, to reflect their values at execution time.
---
 gdb/compile/compile-object-load.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/gdb/compile/compile-object-load.c b/gdb/compile/compile-object-load.c
index 3078973..964356c 100644
--- a/gdb/compile/compile-object-load.c
+++ b/gdb/compile/compile-object-load.c
@@ -752,10 +752,11 @@ compile_object_load (const compile_file_names &file_names,
     regs_addr = 0;
   else
     {
-      /* Use read-only non-executable memory protection.  */
+      /* Use read-write non-executable memory protection.
+         We may need to update the registers when calling _gdb_expr_. */
       regs_addr = gdbarch_infcall_mmap (target_gdbarch (), 0,
 					TYPE_LENGTH (regs_type),
-					GDB_MMAP_PROT_READ);
+					GDB_MMAP_PROT_READ | GDB_MMAP_PROT_WRITE);
       gdb_assert (regs_addr != 0);
       setup_sections_data.munmap_list->add (regs_addr, TYPE_LENGTH (regs_type));
       if (compile_debug)
-- 
2.7.4

[RFC][PATCH 3/7] Make create_sals_from_location_default non static

[paul-naert]

This function can be used to translate a location to SAL in other files.
---
 gdb/breakpoint.c | 10 ++--------
 gdb/breakpoint.h |  5 +++++
 2 files changed, 7 insertions(+), 8 deletions(-)

diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index c9587ff..4f90d42 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -88,11 +88,6 @@ static void map_breakpoint_numbers (const char *,
 
 static void breakpoint_re_set_default (struct breakpoint *);
 
-static void
-  create_sals_from_location_default (const struct event_location *location,
-				     struct linespec_result *canonical,
-				     enum bptype type_wanted);
-
 static void create_breakpoints_sal_default (struct gdbarch *,
 					    struct linespec_result *,
 					    gdb::unique_xmalloc_ptr<char>,
@@ -13640,10 +13635,9 @@ breakpoint_re_set_default (struct breakpoint *b)
   update_breakpoint_locations (b, filter_pspace, expanded, expanded_end);
 }
 
-/* Default method for creating SALs from an address string.  It basically
-   calls parse_breakpoint_sals.  Return 1 for success, zero for failure.  */
+/* See breakpoint.h.  */
 
-static void
+void
 create_sals_from_location_default (const struct event_location *location,
 				   struct linespec_result *canonical,
 				   enum bptype type_wanted)
diff --git a/gdb/breakpoint.h b/gdb/breakpoint.h
index da26f64..378283c 100644
--- a/gdb/breakpoint.h
+++ b/gdb/breakpoint.h
@@ -648,6 +648,11 @@ struct breakpoint_ops
   void (*after_condition_true) (struct bpstats *bs);
 };
 
+/* Create SALs from location, storing the result in linespec_result.  */
+void create_sals_from_location_default (const struct event_location *location,
+                                        struct linespec_result *canonical,
+                                        enum bptype type_wanted);
+
 /* Helper for breakpoint_ops->print_recreate implementations.  Prints
    the "thread" or "task" condition of B, and then a newline.
 
-- 
2.7.4

[RFC][PATCH 4/7] Add address argument to compile_to_object

[paul-naert]

This enables the caller to have the compiler look at locals from a different frame than the current one.
This function is also set as non static to make it available for the patch command.
---
 gdb/compile/compile-internal.h | 11 +++++++++++
 gdb/compile/compile.c          | 24 ++++++++++++++++--------
 2 files changed, 27 insertions(+), 8 deletions(-)

diff --git a/gdb/compile/compile-internal.h b/gdb/compile/compile-internal.h
index 9c0e989..648b2ea 100644
--- a/gdb/compile/compile-internal.h
+++ b/gdb/compile/compile-internal.h
@@ -207,4 +207,15 @@ private:
   std::string m_object_file;
 };
 
+/* Process the compilation request.  On success it returns the object
+   and source file names.  On an error condition, error () is
+   called.
+   if addr is set to 0, the program is compiled in the scope of the
+   current frame. Otherwise it uses the frame containing the
+   instruction at address addr.  */
+compile_file_names compile_to_object(struct command_line *cmd,
+                                     const char *cmd_string,
+                                     enum compile_i_scope_types scope,
+                                     CORE_ADDR addr);
+
 #endif /* COMPILE_COMPILE_INTERNAL_H */
diff --git a/gdb/compile/compile.c b/gdb/compile/compile.c
index 94942db..ce01a08 100644
--- a/gdb/compile/compile.c
+++ b/gdb/compile/compile.c
@@ -667,13 +667,12 @@ print_callback (void *ignore, const char *message)
   fputs_filtered (message, gdb_stderr);
 }
 
-/* Process the compilation request.  On success it returns the object
-   and source file names.  On an error condition, error () is
-   called.  */
 
-static compile_file_names
+/* See compile-internal.h.  */
+
+compile_file_names
 compile_to_object (struct command_line *cmd, const char *cmd_string,
-		   enum compile_i_scope_types scope)
+		   enum compile_i_scope_types scope, CORE_ADDR address)
 {
   const struct block *expr_block;
   CORE_ADDR trash_pc, expr_pc;
@@ -687,8 +686,17 @@ compile_to_object (struct command_line *cmd, const char *cmd_string,
     error (_("The program must be running for the compile command to "\
 	     "work."));
 
-  expr_block = get_expr_block_and_pc (&trash_pc);
-  expr_pc = get_frame_address_in_block (get_selected_frame (NULL));
+  if(address == 0)
+    {
+      /* Use current address.  */
+      expr_block = get_expr_block_and_pc (&trash_pc);
+      expr_pc = get_frame_address_in_block (get_selected_frame (NULL));
+    }
+  else
+    {
+      expr_block =  block_for_pc(address);
+      expr_pc = address;
+    }
 
   /* Set up instance and context for the compiler.  */
   if (current_language->la_get_compile_instance == NULL)
@@ -824,7 +832,7 @@ eval_compile_command (struct command_line *cmd, const char *cmd_string,
 {
   struct compile_module *compile_module;
 
-  compile_file_names fnames = compile_to_object (cmd, cmd_string, scope);
+  compile_file_names fnames = compile_to_object (cmd, cmd_string, scope, 0);
 
   gdb::unlinker object_remover (fnames.object_file ());
   gdb::unlinker source_remover (fnames.source_file ());
-- 
2.7.4

[RFC][PATCH 5/7] Add the option to defer register storage for compiled object loading

[paul-naert]

Currently, required register values are saved to
memory when the compile object is loaded. This modification allows storing
the metadata of which registers should be saved instead. They can then be
saved when the gdb_expr function is actually executed.
---
 gdb/compile/compile-c-support.c   |  5 ++++
 gdb/compile/compile-internal.h    |  7 ++++++
 gdb/compile/compile-object-load.c | 50 +++++++++++++++++++++++++++++----------
 gdb/compile/compile-object-load.h |  7 ++++++
 gdb/compile/compile-object-run.c  |  6 +++++
 gdb/defs.h                        |  3 +++
 6 files changed, 65 insertions(+), 13 deletions(-)

diff --git a/gdb/compile/compile-c-support.c b/gdb/compile/compile-c-support.c
index 9de827a..34ad568 100644
--- a/gdb/compile/compile-c-support.c
+++ b/gdb/compile/compile-c-support.c
@@ -320,6 +320,7 @@ struct c_add_code_header
     switch (type)
       {
       case COMPILE_I_SIMPLE_SCOPE:
+      case COMPILE_I_PATCH_SCOPE:
 	fputs_unfiltered ("void "
 			  GCC_FE_WRAPPER_FUNCTION
 			  " (struct "
@@ -367,6 +368,7 @@ struct c_add_code_footer
     switch (type)
       {
       case COMPILE_I_SIMPLE_SCOPE:
+      case COMPILE_I_PATCH_SCOPE:
       case COMPILE_I_PRINT_ADDRESS_SCOPE:
       case COMPILE_I_PRINT_VALUE_SCOPE:
 	fputs_unfiltered ("}\n", buf);
@@ -444,6 +446,7 @@ struct cplus_add_code_header
   switch (type)
     {
     case COMPILE_I_SIMPLE_SCOPE:
+    case COMPILE_I_PATCH_SCOPE:
       fputs_unfiltered ("void "
 			GCC_FE_WRAPPER_FUNCTION
 			" (struct "
@@ -601,6 +604,7 @@ public:
     AddCodeHeaderPolicy::add_code_header (m_instance->scope (), &buf);
 
     if (m_instance->scope () == COMPILE_I_SIMPLE_SCOPE
+	|| m_instance->scope () == COMPILE_I_PATCH_SCOPE
 	|| m_instance->scope () == COMPILE_I_PRINT_ADDRESS_SCOPE
 	|| m_instance->scope () == COMPILE_I_PRINT_VALUE_SCOPE)
       {
@@ -630,6 +634,7 @@ public:
       buf.puts ("}\n");
 
     if (m_instance->scope () == COMPILE_I_SIMPLE_SCOPE
+  || m_instance->scope () == COMPILE_I_PATCH_SCOPE
 	|| m_instance->scope () == COMPILE_I_PRINT_ADDRESS_SCOPE
 	|| m_instance->scope () == COMPILE_I_PRINT_VALUE_SCOPE)
       PopUserExpressionPolicy::pop_user_expression (&buf);
diff --git a/gdb/compile/compile-internal.h b/gdb/compile/compile-internal.h
index 648b2ea..14a41a8 100644
--- a/gdb/compile/compile-internal.h
+++ b/gdb/compile/compile-internal.h
@@ -151,6 +151,13 @@ protected:
   htab_up m_symbol_err_map;
 };
 
+/* Stores data needed to save registers for the patch command.  */
+struct regs_store_data
+{
+  int regnum;
+  ULONGEST reg_offset;
+};
+
 /* Define header and footers for different scopes.  */
 
 /* A simple scope just declares a function named "_gdb_expr", takes no
diff --git a/gdb/compile/compile-object-load.c b/gdb/compile/compile-object-load.c
index 964356c..743b2b7 100644
--- a/gdb/compile/compile-object-load.c
+++ b/gdb/compile/compile-object-load.c
@@ -528,7 +528,7 @@ get_regs_type (struct symbol *func_sym, struct objfile *objfile)
    starting at inferior address REGS_BASE.  */
 
 static void
-store_regs (struct type *regs_type, CORE_ADDR regs_base)
+store_regs (struct type *regs_type, CORE_ADDR regs_base, regs_store_data *deferred_store)
 {
   struct gdbarch *gdbarch = target_gdbarch ();
   int fieldno;
@@ -546,6 +546,12 @@ store_regs (struct type *regs_type, CORE_ADDR regs_base)
       struct value *regval;
       CORE_ADDR inferior_addr;
 
+      /* Initialize deferred registers storage data.  */
+      if (deferred_store != NULL)
+	{
+	  deferred_store[fieldno] = {-1, 0};
+	}
+
       if (strcmp (reg_name, COMPILE_I_SIMPLE_REGISTER_DUMMY) == 0)
 	continue;
 
@@ -561,17 +567,27 @@ store_regs (struct type *regs_type, CORE_ADDR regs_base)
 
       regnum = compile_register_name_demangle (gdbarch, reg_name);
 
-      regval = value_from_register (reg_type, regnum, get_current_frame ());
-      if (value_optimized_out (regval))
-	error (_("Register \"%s\" is optimized out."), reg_name);
-      if (!value_entirely_available (regval))
-	error (_("Register \"%s\" is not available."), reg_name);
-
-      inferior_addr = regs_base + reg_offset;
-      if (0 != target_write_memory (inferior_addr, value_contents (regval),
-				    reg_size))
-	error (_("Cannot write register \"%s\" to inferior memory at %s."),
-	       reg_name, paddress (gdbarch, inferior_addr));
+      /* If deferred store is null, registers are loaded instantly.
+	 Otherwise we store the offset.  */
+      if (deferred_store == NULL)
+	{
+	  regval = value_from_register (reg_type, regnum, get_current_frame ());
+	  if (value_optimized_out (regval))
+	    error (_ ("Register \"%s\" is optimized out."), reg_name);
+	  if (!value_entirely_available (regval))
+	    error (_ ("Register \"%s\" is not available."), reg_name);
+
+	  inferior_addr = regs_base + reg_offset;
+	  if (0
+	      != target_write_memory (inferior_addr, value_contents (regval),
+				      reg_size))
+	    error (_ ("Cannot write register \"%s\" to inferior memory at %s."),
+		   reg_name, paddress (gdbarch, inferior_addr));
+	}
+      else
+	{
+	  deferred_store[fieldno] = {regnum, reg_offset};
+	}
     }
 }
 
@@ -599,6 +615,7 @@ compile_object_load (const compile_file_names &file_names,
   struct objfile *objfile;
   int expect_parameters;
   struct type *expect_return_type;
+  struct regs_store_data *deferred_regs_store = NULL;
 
   gdb::unique_xmalloc_ptr<char> filename
     (tilde_expand (file_names.object_file ()));
@@ -654,6 +671,7 @@ compile_object_load (const compile_file_names &file_names,
   switch (scope)
     {
     case COMPILE_I_SIMPLE_SCOPE:
+    case COMPILE_I_PATCH_SCOPE:
       expect_parameters = 1;
       expect_return_type = builtin_type (target_gdbarch ())->builtin_void;
       break;
@@ -765,7 +783,11 @@ compile_object_load (const compile_file_names &file_names,
 			    paddress (target_gdbarch (),
 				      TYPE_LENGTH (regs_type)),
 			    paddress (target_gdbarch (), regs_addr));
-      store_regs (regs_type, regs_addr);
+      if (scope == COMPILE_I_PATCH_SCOPE)
+      {
+        deferred_regs_store = new regs_store_data[TYPE_NFIELDS(regs_type)];
+      }
+      store_regs (regs_type, regs_addr, deferred_regs_store);
     }
 
   if (scope == COMPILE_I_PRINT_ADDRESS_SCOPE
@@ -795,6 +817,8 @@ compile_object_load (const compile_file_names &file_names,
   retval->source_file = xstrdup (file_names.source_file ());
   retval->func_sym = func_sym;
   retval->regs_addr = regs_addr;
+  retval->deferred_regs_store = deferred_regs_store;
+  retval->regs_store_num = TYPE_NFIELDS (regs_type);
   retval->scope = scope;
   retval->scope_data = scope_data;
   retval->out_value_type = out_value_type;
diff --git a/gdb/compile/compile-object-load.h b/gdb/compile/compile-object-load.h
index 730164f..7016370 100644
--- a/gdb/compile/compile-object-load.h
+++ b/gdb/compile/compile-object-load.h
@@ -59,6 +59,13 @@ struct compile_module
      require any.  */
   CORE_ADDR regs_addr;
 
+  /* Offsets needed to store the registers for the patch
+     command. NULL for the compile command.  */
+  struct regs_store_data *deferred_regs_store;
+
+  /* The number of registers to be stored.  */
+  int regs_store_num;
+
   /* The "scope" of this compilation.  */
   enum compile_i_scope_types scope;
 
diff --git a/gdb/compile/compile-object-run.c b/gdb/compile/compile-object-run.c
index 32e46f9..07aeceb 100644
--- a/gdb/compile/compile-object-run.c
+++ b/gdb/compile/compile-object-run.c
@@ -52,6 +52,9 @@ struct do_module_cleanup
 
   /* objfile_name of our objfile.  */
   char objfile_name_string[1];
+
+  /* Deferred store registers.  */
+  regs_store_data *deferred_regs_store;
 };
 
 /* Cleanup everything after the inferior function dummy frame gets
@@ -100,6 +103,9 @@ do_module_cleanup (void *arg, int registers_valid)
 
   delete data->munmap_list_head;
 
+  /* Delete the deferred registers storage data.  */
+  delete[] data->deferred_regs_store;
+
   /* Delete the .o file.  */
   unlink (data->objfile_name_string);
   xfree (data);
diff --git a/gdb/defs.h b/gdb/defs.h
index f12ba36..4ce885a 100644
--- a/gdb/defs.h
+++ b/gdb/defs.h
@@ -80,6 +80,9 @@ enum compile_i_scope_types
        name already specifies its address.  See get_out_value_type.  */
     COMPILE_I_PRINT_ADDRESS_SCOPE,
     COMPILE_I_PRINT_VALUE_SCOPE,
+    
+    /* A simple scope with deferred register storing.  */
+    COMPILE_I_PATCH_SCOPE,
   };
 
 
-- 
2.7.4

[RFC][PATCH 6/7] Convert short jumps to long ones in amd64_relocate_instruction

[paul-naert]

When relocating a short jump, the range is often
too short to correctly point to the original position from the displaced one.
We replace short jump instructions by 5 bytes jumps, meaning we now have a
32bit possible offset instead of 8.
---
 gdb/amd64-tdep.c | 33 ++++++++++++++++++++++++++++++---
 1 file changed, 30 insertions(+), 3 deletions(-)

diff --git a/gdb/amd64-tdep.c b/gdb/amd64-tdep.c
index ca9b909..a1604a8 100644
--- a/gdb/amd64-tdep.c
+++ b/gdb/amd64-tdep.c
@@ -1835,6 +1835,8 @@ amd64_relocate_instruction (struct gdbarch *gdbarch,
   gdb_byte *buf = (gdb_byte *) xmalloc (len + fixup_sentinel_space);
   struct amd64_insn insn_details;
   int offset = 0;
+  int jmp_arg_len = 4;
+  int opcode_len_diff = 0;
   LONGEST rel32, newrel;
   gdb_byte *insn;
   int insn_length;
@@ -1924,19 +1926,44 @@ amd64_relocate_instruction (struct gdbarch *gdbarch,
   offset = rip_relative_offset (&insn_details);
   if (!offset)
     {
+      /* Replace short jumps by long ones */
+      if (insn[0] == 0xeb)
+	{
+	  jmp_arg_len = 1;
+	  insn[0] = 0xe9;
+	  insn_length = 5;
+	}
       /* Adjust jumps with 32-bit relative addresses.  Calls are
 	 already handled above.  */
       if (insn[0] == 0xe9)
 	offset = 1;
+
+      /* Replace short conditional jumps by long ones */
+      if ((insn[0] & 0xf0) == 0x70)
+	{
+	  jmp_arg_len = 1;
+	  insn[2] = insn[1];
+	  insn[1] = insn[0] + 0x10;
+	  insn[0] = 0x0f;
+	  insn_length = 6;
+	  opcode_len_diff = 1;
+	}
       /* Adjust conditional jumps.  */
-      else if (insn[0] == 0x0f && (insn[1] & 0xf0) == 0x80)
+      if (insn[0] == 0x0f && (insn[1] & 0xf0) == 0x80)
 	offset = 2;
     }
 
   if (offset)
     {
-      rel32 = extract_signed_integer (insn_details.raw_insn + offset, 4, byte_order);
-      newrel = (oldloc - *to) + rel32;
+      rel32 = extract_signed_integer (insn_details.raw_insn + offset, jmp_arg_len, byte_order);
+      /* The length of the opcode and offset of the jump may be different. */
+      newrel = (oldloc - *to) + rel32 - (4 + opcode_len_diff - jmp_arg_len);
+      if (newrel < INT32_MIN || newrel > INT32_MAX)
+	{
+	  /* Overflowing the 32 bit jump */
+	  if (debug_displaced)
+	    error (_ ("Overflowing of int32 for jump instruction relocation"));
+	}
       store_signed_integer (insn_details.raw_insn + offset, 4, byte_order, newrel);
       if (debug_displaced)
 	fprintf_unfiltered (gdb_stdlog,
-- 
2.7.4

[RFC][PATCH 7/7] Add the patch command to dynamically inject code in the inferior

[paul-naert]

From: Paul Naert <paul.naert@polymtl.ca>

The goal of this command is to compile and inject code in the inferior.
This commit adds two commands : "patch code" and "patch file", which respectively take as input C/C++ code and a source file.
Compared to the compile command, these new commands leave the code in the binary, so that it can be executed several times.
The injection of code is done through a trampoline in a similar fashion to GDB fast tracepoints.
The original instruction is replaced with a jump to the trampoline which calls the gdb_expr() function generated by gcc (same process as in the compile command).
---
 gdb/Makefile.in             |   3 +-
 gdb/amd64-tdep.c            | 198 +++++++++++++++++++++++++++
 gdb/compile/compile-patch.c | 322 ++++++++++++++++++++++++++++++++++++++++++++
 gdb/gdbarch.c               |  82 +++++++++++
 gdb/gdbarch.h               |  13 ++
 5 files changed, 617 insertions(+), 1 deletion(-)
 create mode 100644 gdb/compile/compile-patch.c

diff --git a/gdb/Makefile.in b/gdb/Makefile.in
index b6caa2c..737be8e 100644
--- a/gdb/Makefile.in
+++ b/gdb/Makefile.in
@@ -315,7 +315,8 @@ SUBDIR_GCC_COMPILE_SRCS = \
 	compile/compile-cplus-types.c \
 	compile/compile-loc2c.c \
 	compile/compile-object-load.c \
-	compile/compile-object-run.c
+	compile/compile-object-run.c\
+	compile/compile-patch.c
 
 SUBDIR_GCC_COMPILE_OBS = $(patsubst %.c,%.o,$(filter %.c,$(SUBDIR_GCC_COMPILE_SRCS)))
 
diff --git a/gdb/amd64-tdep.c b/gdb/amd64-tdep.c
index a1604a8..d180875 100644
--- a/gdb/amd64-tdep.c
+++ b/gdb/amd64-tdep.c
@@ -49,6 +49,7 @@
 #include "gdbsupport/byte-vector.h"
 #include "osabi.h"
 #include "x86-tdep.h"
+#include "compile/compile-object-load.h"
 
 /* Note that the AMD64 architecture was previously known as x86-64.
    The latter is (forever) engraved into the canonical system name as
@@ -3119,6 +3120,198 @@ amd64_in_indirect_branch_thunk (struct gdbarch *gdbarch, CORE_ADDR pc)
 				       AMD64_RAX_REGNUM,
 				       AMD64_RIP_REGNUM);
 }
+/* Fills the trampoline buffer with instructions to store the register referenced by reg_to_store.  */
+static int 
+amd64_store_reg(unsigned char *trampoline_instr, struct regs_store_data reg_to_store)
+{
+  /* The numbering of registers needs to be adjusted to simplify machine instructions production.  */
+  int correct[16]={0,3,1,2,6,7,5,4,8,9,10,11,12,13,14,15};
+  ULONGEST offset = reg_to_store.reg_offset;
+  int regnum = reg_to_store.regnum;
+  if(regnum == -1)
+  {
+    /* Dummy register.  */
+    return 0;
+  }
+  if(regnum>15 || regnum < 0)
+  {
+    fprintf_filtered(gdb_stdlog, "Expected register number < 16\n");
+    fprintf_filtered(gdb_stdlog, "Got number %d \n", regnum);
+    return 0;
+  }
+  regnum = correct[regnum];
+
+  int i = 0;
+  /* Move register value to *rdi + offset.  */
+  if(regnum<8)
+    trampoline_instr[i++] = '\x48';
+  else
+    trampoline_instr[i++] = '\x4c';
+  trampoline_instr[i++] = '\x89';
+  trampoline_instr[i++] = '\x87'+'\x8'*(regnum%8);
+  int32_t offset32 = (int32_t) offset;
+  memcpy(trampoline_instr+i, &offset32, 4);
+  i+=4;
+  return i;
+}
+
+/* This fills the trampoline_instr buffer with instructions to save
+   and restore the registers, but does not relocate the replaced 
+   instruction. It returns the length of the trampoline.  */
+static int
+amd64_fill_trampoline_buffer (unsigned char *trampoline_instr, 
+                       struct compile_module *module)
+{
+  int i = 0;
+  CORE_ADDR called = BLOCK_ENTRY_PC (SYMBOL_BLOCK_VALUE (module->func_sym));
+  CORE_ADDR regs_struct_addr = module->regs_addr;
+  struct regs_store_data *regs_to_store = module->deferred_regs_store;
+  int regs_store_num = module->regs_store_num;
+  /* save registers */
+
+  /* Store rax at rsp -0xc8 */
+  trampoline_instr[i++] = 0x48;
+  trampoline_instr[i++] = 0x89;
+  trampoline_instr[i++] = 0x84;
+  trampoline_instr[i++] = 0x24;
+  trampoline_instr[i++] = 0x38;
+  trampoline_instr[i++] = 0xff;
+  trampoline_instr[i++] = 0xff;
+  trampoline_instr[i++] = 0xff;
+  
+  /* Move flags to ah */
+  trampoline_instr[i++] = 0x9f; /* lahf */
+
+  /* rsp -= 0x80 for the red zone*/
+  trampoline_instr[i++] = 0x48; 
+  trampoline_instr[i++] = 0x83; 
+  trampoline_instr[i++] = 0xc4; 
+  trampoline_instr[i++] = 0x80; 
+  
+  trampoline_instr[i++] = 0x50; /* push %rax i.e. flags */
+  trampoline_instr[i++] = 0x54; /* push %rsp */
+  trampoline_instr[i++] = 0x55; /* push %rbp */
+  trampoline_instr[i++] = 0x57; /* push %rdi */
+  trampoline_instr[i++] = 0x56; /* push %rsi */
+  trampoline_instr[i++] = 0x52; /* push %rdx */
+  trampoline_instr[i++] = 0x51; /* push %rcx */
+  trampoline_instr[i++] = 0x53; /* push %rbx */
+  trampoline_instr[i++] = 0x48; /* skip this position (old rax is there) */
+  trampoline_instr[i++] = 0x83; 
+  trampoline_instr[i++] = 0xc4;
+  trampoline_instr[i++] = 0xf8;
+  trampoline_instr[i++] = 0x41;
+  trampoline_instr[i++] = 0x57; /* push %r15 */
+  trampoline_instr[i++] = 0x41;
+  trampoline_instr[i++] = 0x56; /* push %r14 */
+  trampoline_instr[i++] = 0x41;
+  trampoline_instr[i++] = 0x55; /* push %r13 */
+  trampoline_instr[i++] = 0x41;
+  trampoline_instr[i++] = 0x54; /* push %r12 */
+  trampoline_instr[i++] = 0x41;
+  trampoline_instr[i++] = 0x53; /* push %r11 */
+  trampoline_instr[i++] = 0x41;
+  trampoline_instr[i++] = 0x52; /* push %r10 */
+  trampoline_instr[i++] = 0x41;
+  trampoline_instr[i++] = 0x51; /* push %r9 */
+  trampoline_instr[i++] = 0x41;
+  trampoline_instr[i++] = 0x50; /* push %r8 */
+
+
+
+  /* Provide gdb_expr () arguments.  */
+  trampoline_instr[i++] = 0x48; /* movabs <arg>,%rdi */
+  trampoline_instr[i++] = 0xbf;
+  memcpy (trampoline_instr + i, &regs_struct_addr, 8);
+  i += 8;
+  /* Store required register values into *rdi.  */
+  for(int j =0; j < regs_store_num; j++)
+  {
+    i += amd64_store_reg(trampoline_instr+i, regs_to_store[j]);
+  }
+
+  /* call gdb_expr () */
+  trampoline_instr[i++] = 0x48; /* movabs <called>,%rax */
+  trampoline_instr[i++] = 0xb8;
+  memcpy (trampoline_instr + i, &called, 8);
+  i += 8;
+  trampoline_instr[i++] = 0xff; /* callq *%rax */
+  trampoline_instr[i++] = 0xd0;
+
+  /* restore registers */
+  trampoline_instr[i++] = 0x41;
+  trampoline_instr[i++] = 0x58; /* pop %r8 */
+  trampoline_instr[i++] = 0x41;
+  trampoline_instr[i++] = 0x59; /* pop %r9 */
+  trampoline_instr[i++] = 0x41;
+  trampoline_instr[i++] = 0x5a; /* pop %r10 */
+  trampoline_instr[i++] = 0x41;
+  trampoline_instr[i++] = 0x5b; /* pop %r11 */
+  trampoline_instr[i++] = 0x41;
+  trampoline_instr[i++] = 0x5c; /* pop %r12 */
+  trampoline_instr[i++] = 0x41;
+  trampoline_instr[i++] = 0x5d; /* pop %r13 */
+  trampoline_instr[i++] = 0x41;
+  trampoline_instr[i++] = 0x5e; /* pop %r14 */
+  trampoline_instr[i++] = 0x41;
+  trampoline_instr[i++] = 0x5f; /* pop %r15 */
+  trampoline_instr[i++] = 0x58; /* pop %rax (will be overwritten right after) */
+  trampoline_instr[i++] = 0x5b; /* pop %rbx */
+  trampoline_instr[i++] = 0x59; /* pop %rcx */
+  trampoline_instr[i++] = 0x5a; /* pop %rdx */
+  trampoline_instr[i++] = 0x5e; /* pop %rsi */
+  trampoline_instr[i++] = 0x5f; /* pop %rdi */
+  trampoline_instr[i++] = 0x5d; /* pop %rbp */
+  trampoline_instr[i++] = 0x5c; /* pop %rsp */
+  trampoline_instr[i++] = 0x58; /* pop %rax */
+  /* rsp += 0x80 for the red zone.  */
+  trampoline_instr[i++] = 0x48; 
+  trampoline_instr[i++] = 0x81; 
+  trampoline_instr[i++] = 0xc4; 
+  trampoline_instr[i++] = 0x80; 
+  trampoline_instr[i++] = 0x00; 
+  trampoline_instr[i++] = 0x00; 
+  trampoline_instr[i++] = 0x00; 
+
+  trampoline_instr[i++] = 0x9e; /* sahf */
+
+  /* Restore rax from rsp -0xc8 */
+  trampoline_instr[i++] = 0x48;
+  trampoline_instr[i++] = 0x8B;
+  trampoline_instr[i++] = 0x84;
+  trampoline_instr[i++] = 0x24;
+  trampoline_instr[i++] = 0x38;
+  trampoline_instr[i++] = 0xff;
+  trampoline_instr[i++] = 0xff;
+  trampoline_instr[i++] = 0xff;
+
+  return i;
+}
+
+/* Inserts a jump from ''from'' to ''to''. If fill not is set to 1,
+   this functions sets the bytes after the jump to NOP to clarify the code.  */
+bool
+amd64_patch_jump (struct gdbarch *gdbarch, CORE_ADDR from, CORE_ADDR to)
+{
+  int64_t long_jump_offset = to - (from + 5);
+  if (long_jump_offset > INT_MAX || long_jump_offset < INT_MIN)
+    {
+      fprintf_filtered (
+          gdb_stderr,
+          "E.Jump pad too far from instruction for jump (offset 0x%" PRIx64
+          " > int32). \n",
+          long_jump_offset);
+      return 0;
+    }
+
+  int32_t jump_offset = (int32_t)long_jump_offset;
+  fprintf_filtered (gdb_stdlog, "Patched in a jump from 0x%lx to 0x%lx \n",
+                    from, to);
+  gdb_byte jump_insn[] = { 0xe9, 0, 0, 0, 0 };
+  memcpy (jump_insn + 1, &jump_offset, 4);
+  target_write_memory (from, jump_insn, 5);
+  return 1;
+}
 
 void
 amd64_init_abi (struct gdbarch_info info, struct gdbarch *gdbarch,
@@ -3225,6 +3418,11 @@ amd64_init_abi (struct gdbarch_info info, struct gdbarch *gdbarch,
   set_gdbarch_ps_regnum (gdbarch, AMD64_EFLAGS_REGNUM); /* %eflags */
   set_gdbarch_fp0_regnum (gdbarch, AMD64_ST0_REGNUM); /* %st(0) */
 
+  /* Functions necessary for the patch command.  */
+  set_gdbarch_fill_trampoline_buffer(gdbarch, amd64_fill_trampoline_buffer);
+  set_gdbarch_patch_jump(gdbarch, amd64_patch_jump);
+  set_gdbarch_jmp_insn_length(gdbarch);
+
   /* The "default" register numbering scheme for AMD64 is referred to
      as the "DWARF Register Number Mapping" in the System V psABI.
      The preferred debugging format for all known AMD64 targets is
diff --git a/gdb/compile/compile-patch.c b/gdb/compile/compile-patch.c
new file mode 100644
index 0000000..4088a80
--- /dev/null
+++ b/gdb/compile/compile-patch.c
@@ -0,0 +1,322 @@
+#include "defs.h"
+#include "gdbcmd.h"
+#include "arch-utils.h"
+#include "gdbsupport/gdb_unlinker.h"
+#include "gdbsupport/pathstuff.h"
+#include "linespec.h"
+#include "objfiles.h"
+#include "compile.h"
+#include "compile-object-load.h"
+#include "observable.h"
+#include <map>
+
+#define PAGE_SIZE sysconf (_SC_PAGE_SIZE)
+#define PAGE_ADDRESS(addr) (addr / PAGE_SIZE) * PAGE_SIZE
+#define TP_MAX_SIZE 0x100
+
+/* Hold "patch" commands.  */
+
+static struct cmd_list_element *compile_patch_command_list;
+
+/* Finds the return address for a trampoline placed at insn_addr.  */
+static CORE_ADDR
+find_return_address (struct gdbarch *gdbarch, CORE_ADDR insn_addr, bool verbose)
+{
+  /* In this version, we only check if we have enough room to put a jump.  */
+  if (gdb_insn_length (gdbarch, insn_addr)
+      < gdbarch_jmp_insn_length (gdbarch, 0))
+    {
+      return 0;
+    }
+  CORE_ADDR return_address = insn_addr + gdb_insn_length (gdbarch, insn_addr);
+  return return_address;
+}
+
+/* Is the page containing addr available ? If so map it.  */
+static CORE_ADDR
+try_map_page (gdbarch *gdbarch, CORE_ADDR trampoline_addr, int trampoline_size)
+{
+  int page_size = PAGE_SIZE;
+  const int prot
+    = GDB_MMAP_PROT_READ | GDB_MMAP_PROT_WRITE | GDB_MMAP_PROT_EXEC;
+
+  CORE_ADDR mapped_page
+    = gdbarch_infcall_mmap (gdbarch, trampoline_addr, 1, prot);
+
+  if (mapped_page != PAGE_ADDRESS (trampoline_addr))
+    {
+      gdbarch_infcall_munmap (gdbarch, mapped_page, page_size);
+      return (CORE_ADDR) 0;
+    }
+  /* Check if the trampoline overlaps several pages.  */
+  if (mapped_page + page_size < trampoline_addr + trampoline_size)
+    {
+      CORE_ADDR second_page
+	= gdbarch_infcall_mmap (gdbarch, mapped_page + page_size, page_size,
+				prot);
+      if (second_page == mapped_page + page_size)
+	{
+	  return mapped_page;
+	}
+      else
+	{
+	  gdbarch_infcall_munmap (gdbarch, mapped_page, page_size);
+	  gdbarch_infcall_munmap (gdbarch, second_page, page_size);
+	  return (CORE_ADDR) 0;
+	}
+    }
+  return mapped_page;
+}
+
+/* This function aims to find the next available chunk of memory around
+   insn_address where we have enough room to put a trampoline of size
+   trampoline_size. It keeps a map pointing to the next available portion of
+   memory given a minimum address.  */
+static CORE_ADDR
+allocate_trampoline (gdbarch *gdbarch, CORE_ADDR insn_address,
+		     int trampoline_size)
+{
+  int max_pages_searched = 100;
+  int page_size = PAGE_SIZE;
+  static std::map<CORE_ADDR, CORE_ADDR> current_stack_top;
+
+  /* We do not handle the case where the trampoline is larger than a page.  */
+  gdb_assert (trampoline_size < page_size);
+
+  /* Return value.  */
+  int bit_shift = gdbarch_jmp_insn_length (gdbarch, 1) * 8 - 3;
+
+  CORE_ADDR trampoline_address
+    = ((insn_address >> bit_shift) > 0
+	 ? (((insn_address >> bit_shift) - 1) << bit_shift)
+	 : 100 * PAGE_SIZE);
+
+  if (current_stack_top.find (trampoline_address) != current_stack_top.end ())
+    {
+      trampoline_address = current_stack_top[trampoline_address];
+      CORE_ADDR next_page = PAGE_ADDRESS (trampoline_address) + page_size;
+      CORE_ADDR trampoline_end = trampoline_address + trampoline_size;
+      if (trampoline_end <= next_page)
+	{
+	  current_stack_top[trampoline_address] = trampoline_end;
+	  return trampoline_address;
+	}
+      /* We don't have enought room to put all of the trampoline
+	on this page but the next one is available.  */
+      if (try_map_page (gdbarch, next_page, 0) != 0)
+	{
+	  current_stack_top[trampoline_address] = trampoline_end;
+	  return trampoline_address;
+	}
+    }
+
+  for (int i = 0; i < max_pages_searched; i++)
+    {
+      if (try_map_page (gdbarch, trampoline_address, trampoline_size) != 0)
+	{
+	  current_stack_top[trampoline_address]
+	    = trampoline_address + trampoline_size;
+	  return trampoline_address;
+	}
+      trampoline_address = PAGE_ADDRESS (trampoline_address + page_size);
+    }
+
+  return 0;
+}
+
+static CORE_ADDR
+build_compile_trampoline (struct gdbarch *gdbarch,
+			  struct compile_module *module, CORE_ADDR insn_addr,
+			  CORE_ADDR return_address)
+{
+  /* Allocate memory for the trampoline in the inferior.  */
+  CORE_ADDR trampoline = allocate_trampoline (gdbarch, insn_addr, TP_MAX_SIZE);
+
+  if (trampoline == 0)
+    {
+      return 0;
+    }
+
+  /* Build trampoline.  */
+  gdb_byte trampoline_instr[TP_MAX_SIZE];
+  int trampoline_size
+    = gdbarch_fill_trampoline_buffer (gdbarch, trampoline_instr, module);
+
+  gdb_assert (trampoline_size < TP_MAX_SIZE);
+
+  /* Copy content of trampoline_instr to inferior memory.  */
+  target_write_memory (trampoline, trampoline_instr, trampoline_size);
+
+  /* Relocate replaced instruction */
+  CORE_ADDR trampoline_end = trampoline + trampoline_size;
+
+  gdbarch_relocate_instruction (gdbarch, &trampoline_end, insn_addr);
+
+  gdb_assert (trampoline_end - trampoline + gdbarch_jmp_insn_length (gdbarch, 0)
+	      < TP_MAX_SIZE);
+
+  /* Jump back to return address.  */
+  gdbarch_patch_jump (gdbarch, trampoline_end, return_address);
+
+  return trampoline;
+}
+
+/* Convert a string describing a location to an instruction address.
+   Here we assume the location to correspond to only one pc. */
+static CORE_ADDR
+location_to_pc (const char *location)
+{
+  struct linespec_result canonical;
+
+  event_location_up event_location
+    = string_to_event_location (&location, current_language);
+  create_sals_from_location_default (event_location.get (), &canonical,
+				     bp_breakpoint);
+  CORE_ADDR addr = canonical.lsals[0].sals[0].pc;
+  return addr;
+}
+
+/* The central function for the patch command. */
+static void
+patch_code (const char *location, const char *code)
+{
+  struct gdbarch *gdbarch = target_gdbarch ();
+
+  /* Convert location string to an instruction address.  */
+  CORE_ADDR insn_addr = location_to_pc (location);
+
+  /* Compile code.  */
+  enum compile_i_scope_types scope = COMPILE_I_PATCH_SCOPE;
+  compile_file_names fnames = compile_to_object (NULL, code, scope, insn_addr);
+  gdb::unlinker object_remover (fnames.object_file ());
+  gdb::unlinker source_remover (fnames.source_file ());
+
+  /* Load compiled code into memory.  */
+  struct compile_module *compile_module
+    = compile_object_load (fnames, scope, NULL);
+
+  /* Build a trampoline which calls the compiled code.  */
+  CORE_ADDR return_address = find_return_address (gdbarch, insn_addr, true);
+
+  if (return_address != 0)
+    {
+      CORE_ADDR trampoline_address
+	= build_compile_trampoline (gdbarch, compile_module, insn_addr,
+				    return_address);
+
+      if (trampoline_address == 0)
+	{
+	  fprintf_filtered (gdb_stderr, "Unable to build a trampoline.\n");
+    unlink (compile_module->source_file);
+    xfree (compile_module->source_file);
+    unlink (objfile_name (compile_module->objfile));
+    xfree (compile_module);
+    return;
+	}
+
+      /* Patch in the jump to the trampoline.  */
+      bool success
+	= gdbarch_patch_jump (gdbarch, insn_addr, trampoline_address);
+      if (!success)
+	{
+	  fprintf_filtered (gdb_stderr,
+			    "Unable to insert the code at the given location.\n\
+Make sure the instruction is long enough \n\
+to be replaced by a jump instruction.\n");
+	}
+    }
+  /* Free unused memory */
+  /* Some memory is left allocated in the inferior because
+     we still need to access it to execute the compiled code.  */
+  unlink (compile_module->source_file);
+  xfree (compile_module->source_file);
+  unlink (objfile_name (compile_module->objfile));
+  xfree (compile_module);
+}
+
+/* Handle the input from the 'patch code' command.  The
+   "patch code" command is used to patch in the code an expression
+   containing calls to the GCC compiler.  The language expected in this
+   command is the language currently set in GDB.  */
+static void
+compile_patch_code_command (const char *arg, int from_tty)
+{
+  if (arg == NULL)
+    {
+      error ("No arguments were entered for the patch code command.");
+    }
+  char *dup = strdup (arg);
+  const char *location = strtok (dup, " ");
+  const char *code = strtok (NULL, "\0");
+  if (code == NULL)
+    {
+      free (dup);
+      error ("Missing the code argument for the patch code command.");
+    }
+  patch_code (location, code);
+  free (dup);
+}
+
+/* Handle the input from the 'patch file' command.  The
+   "patch file" command is used to patch in the code an expression
+   containing calls to the GCC compiler. It takes as argument
+   a source file.  The language expected in this command
+   is the language currently set in GDB. */
+static void
+compile_patch_file_command (const char *arg, int from_tty)
+{
+  if (arg == NULL)
+    {
+      error ("No arguments were entered for the patch file command.");
+    }
+  char *dup = strdup (arg);
+  const char *location = strtok (dup, " ");
+  const char *source_file = strtok (NULL, " ");
+  if (source_file == NULL)
+    {
+      free (dup);
+      error ("Missing the second argument for the patch file command.");
+    }
+  gdb::unique_xmalloc_ptr<char> abspath = gdb_abspath (source_file);
+  std::string code_buf = string_printf ("#include \"%s\"\n", abspath.get ());
+  patch_code (location, code_buf.c_str ());
+  free (dup);
+}
+
+/* The patch command without a suffix is interpreted as patch code.  */
+static void
+compile_patch_command (const char *arg, int from_tty)
+{
+  compile_patch_code_command (arg, from_tty);
+}
+
+void _initialize_compile_patch();
+static void
+_initialize_compile_patch (void)
+{
+  struct cmd_list_element *c = NULL;
+
+  compile_cmd_element
+    = add_prefix_cmd ("patch", class_obscure, compile_patch_command, _ ("\
+Command to compile source code and patch it into the inferior."),
+		      &compile_patch_command_list, "patch ", 1, &cmdlist);
+
+  add_cmd ("code", class_obscure, compile_patch_code_command, _ ("\
+Compile, and patch code at location.\n\
+\n\
+Usage: patch code [LOCATION] [CODE]\n\
+\n\
+The source code may be specified as a simple one line expression, e.g.:\n\
+\n\
+    patch code main:2 printf(\"Hello world\\n\");\n\
+\n\
+It will be executed each time the instruction at location is hit."),
+	   &compile_patch_command_list);
+
+  c = add_cmd ("file", class_obscure, compile_patch_file_command, _ ("\
+Compile and patch in a file containing source code.\n\
+\n\
+Usage: compile patch file [LOCATION] [FILENAME]"),
+	       &compile_patch_command_list);
+  set_cmd_completer (c, filename_completer);
+}
diff --git a/gdb/gdbarch.c b/gdb/gdbarch.c
index d049771..9adb77a 100644
--- a/gdb/gdbarch.c
+++ b/gdb/gdbarch.c
@@ -359,6 +359,10 @@ struct gdbarch
   const disasm_options_and_args_t * valid_disassembler_options;
   gdbarch_type_align_ftype *type_align;
   gdbarch_get_pc_address_flags_ftype *get_pc_address_flags;
+  gdbarch_fill_trampoline_buffer_ftype *fill_trampoline_buffer;
+  gdbarch_patch_jump_ftype *patch_jump;
+  int jmp_insn_length;
+  int jmp_opcode_length;
 };
 
 /* Create a new ``struct gdbarch'' based on information provided by
@@ -475,6 +479,10 @@ gdbarch_alloc (const struct gdbarch_info *info,
   gdbarch->addressable_memory_unit_size = default_addressable_memory_unit_size;
   gdbarch->type_align = default_type_align;
   gdbarch->get_pc_address_flags = default_get_pc_address_flags;
+  gdbarch->fill_trampoline_buffer = NULL;
+  gdbarch->patch_jump = NULL;
+  gdbarch->jmp_insn_length = -1;
+  gdbarch->jmp_opcode_length = -1;
   /* gdbarch_alloc() */
 
   return gdbarch;
@@ -724,6 +732,10 @@ verify_gdbarch (struct gdbarch *gdbarch)
   /* Skip verify of valid_disassembler_options, invalid_p == 0 */
   /* Skip verify of type_align, invalid_p == 0 */
   /* Skip verify of get_pc_address_flags, invalid_p == 0 */
+  /* Skip verify of fill_trampoline_buffer, invalid_p == 0 */
+  /* Skip verify of patch_jump, invalid_p == 0 */
+  /* Skip verify of jmp_insn_length, invalid_p == 0 */
+  /* Skip verify of jmp_opcode_length, invalid_p == 0 */
   if (!log.empty ())
     internal_error (__FILE__, __LINE__,
                     _("verify_gdbarch: the following are invalid ...%s"),
@@ -1003,6 +1015,18 @@ gdbarch_dump (struct gdbarch *gdbarch, struct ui_file *file)
                       "gdbarch_dump: fetch_tls_load_module_address = <%s>\n",
                       host_address_to_string (gdbarch->fetch_tls_load_module_address));
   fprintf_unfiltered (file,
+                      "gdbarch_dump: fill_trampoline_buffer = <%s>\n",
+                      host_address_to_string (gdbarch->fill_trampoline_buffer));
+  fprintf_unfiltered (file,
+                      "gdbarch_dump: patch_jump = <%s>\n",
+                      host_address_to_string (gdbarch->patch_jump));
+  fprintf_unfiltered (file,
+                      "gdbarch_dump: jmp_insn_length = <%d>\n",
+                      gdbarch->jmp_insn_length);
+  fprintf_unfiltered (file,
+                      "gdbarch_dump: jmp_opcode_length = <%d>\n",
+                      gdbarch->jmp_opcode_length);
+  fprintf_unfiltered (file,
                       "gdbarch_dump: gdbarch_find_memory_regions_p() = %d\n",
                       gdbarch_find_memory_regions_p (gdbarch));
   fprintf_unfiltered (file,
@@ -4054,6 +4078,64 @@ set_gdbarch_relocate_instruction (struct gdbarch *gdbarch,
   gdbarch->relocate_instruction = relocate_instruction;
 }
 
+void
+set_gdbarch_fill_trampoline_buffer (struct gdbarch *gdbarch,
+                                  gdbarch_fill_trampoline_buffer_ftype fill_trampoline_buffer)
+{
+  gdbarch->fill_trampoline_buffer = fill_trampoline_buffer;
+}
+
+int 
+gdbarch_fill_trampoline_buffer (struct gdbarch *gdbarch,
+                             unsigned char *trampoline_instr,
+                             compile_module *module)
+{
+  gdb_assert (gdbarch != NULL);
+  gdb_assert (gdbarch->fill_trampoline_buffer != NULL);
+  if (gdbarch_debug >= 2)
+    fprintf_unfiltered (gdb_stdlog, "gdbarch_fill_trampoline_buffer called\n");
+  return gdbarch->fill_trampoline_buffer(trampoline_instr, module);
+}
+
+void
+set_gdbarch_patch_jump (struct gdbarch *gdbarch,
+                        gdbarch_patch_jump_ftype patch_jump)
+{
+  gdbarch->patch_jump = patch_jump;
+}
+
+bool 
+gdbarch_patch_jump (struct gdbarch *gdbarch,
+                        CORE_ADDR from,
+                        CORE_ADDR to)
+{
+  gdb_assert (gdbarch != NULL);
+  gdb_assert (gdbarch->patch_jump != NULL);
+  if (gdbarch_debug >= 2)
+    fprintf_unfiltered (gdb_stdlog, "gdbarch_patch_jump called\n");
+  return gdbarch->patch_jump(gdbarch, from, to);
+}
+
+void
+set_gdbarch_jmp_insn_length (struct gdbarch *gdbarch)
+{
+  gdbarch->jmp_insn_length = 5;
+  gdbarch->jmp_opcode_length = 1;
+}
+
+int
+gdbarch_jmp_insn_length(struct gdbarch *gdbarch, bool without_opcode)
+{
+  gdb_assert (gdbarch != NULL);
+  gdb_assert (gdbarch->jmp_insn_length != -1);
+  gdb_assert (gdbarch->jmp_opcode_length != -1);
+  if (gdbarch_debug >= 2)
+    fprintf_unfiltered (gdb_stdlog, "gdbarch_jmp_insn_length called\n");
+  if(without_opcode)
+    return gdbarch->jmp_insn_length - gdbarch->jmp_opcode_length;
+  return gdbarch->jmp_insn_length;
+}
+
 int
 gdbarch_overlay_update_p (struct gdbarch *gdbarch)
 {
diff --git a/gdb/gdbarch.h b/gdb/gdbarch.h
index 054a077..d1b9f18 100644
--- a/gdb/gdbarch.h
+++ b/gdb/gdbarch.h
@@ -66,6 +66,7 @@ struct mem_range;
 struct syscalls_info;
 struct thread_info;
 struct ui_out;
+struct compile_module;
 
 #include "regcache.h"
 
@@ -1648,6 +1649,18 @@ extern void set_gdbarch_get_pc_address_flags (struct gdbarch *gdbarch, gdbarch_g
 
 extern struct gdbarch_tdep *gdbarch_tdep (struct gdbarch *gdbarch);
 
+/* Fill a trampoline by saving and restoring registers and calling the function from the compile module.  */
+typedef int (gdbarch_fill_trampoline_buffer_ftype) (unsigned char *trampoline_instr, compile_module *module);
+extern int gdbarch_fill_trampoline_buffer (struct gdbarch *gdbarch, unsigned char *trampoline_instr, compile_module *module);
+extern void set_gdbarch_fill_trampoline_buffer (struct gdbarch *gdbarch, gdbarch_fill_trampoline_buffer_ftype *fill_trampoline_buffer);
+
+/* Insert a jump instruction from ''from'' to ''to'' */
+typedef bool (gdbarch_patch_jump_ftype) (struct gdbarch *gdbarch, CORE_ADDR from, CORE_ADDR to);
+extern bool gdbarch_patch_jump (struct gdbarch *gdbarch, CORE_ADDR from, CORE_ADDR to);
+extern void set_gdbarch_patch_jump (struct gdbarch *gdbarch, gdbarch_patch_jump_ftype *patch_jump);
+
+extern int gdbarch_jmp_insn_length(gdbarch *gdbarch, bool without_opcode);
+extern void set_gdbarch_jmp_insn_length(gdbarch *gdbarch);
 
 /* Mechanism for co-ordinating the selection of a specific
    architecture.
-- 
2.7.4

Re: [RFC][PATCH 2/7] Change memory protection for compile arguments to RW

[Tom Tromey]

>>>>> ">" == paul-naert via Gdb-patches <gdb-patches@sourceware.org> writes:

>> The registers need to be updated when gdb_expr is not executed instantly
>> after being compiled, to reflect their values at execution time.

I don't understand the connection between this commit message and what
the patch actually does:

>> +      /* Use read-write non-executable memory protection.
>> +         We may need to update the registers when calling _gdb_expr_. */
>>        regs_addr = gdbarch_infcall_mmap (target_gdbarch (), 0,
>>  					TYPE_LENGTH (regs_type),
>> -					GDB_MMAP_PROT_READ);
>> +					GDB_MMAP_PROT_READ | GDB_MMAP_PROT_WRITE);

Tom

Re: [RFC][PATCH 5/7] Add the option to defer register storage for compiled object loading

[Tom Tromey]

>>>>> ">" == paul-naert via Gdb-patches <gdb-patches@sourceware.org> writes:

>> +      if (scope == COMPILE_I_PATCH_SCOPE)
>> +      {
>> +        deferred_regs_store = new regs_store_data[TYPE_NFIELDS(regs_type)];
>> +      }

We'd probably use a vector here, though I guess the way these data
structures are allocated makes that more of a pain.

>> +  /* The number of registers to be stored.  */
>> +  int regs_store_num;

This would be redundant with a vector.

Tom

Re: [RFC][PATCH 6/7] Convert short jumps to long ones in amd64_relocate_instruction

[Tom Tromey]

>>>>> ">" == paul-naert via Gdb-patches <gdb-patches@sourceware.org> writes:

>> +      if (newrel < INT32_MIN || newrel > INT32_MAX)
>> +	{
>> +	  /* Overflowing the 32 bit jump */
>> +	  if (debug_displaced)
>> +	    error (_ ("Overflowing of int32 for jump instruction relocation"));
>> +	}

It seems like the error ought not be conditional on debug_displaced.
Wouldn't this result in silently doing something incorrect?

Tom

Re: [RFC][PATCH 7/7] Add the patch command to dynamically inject code in the inferior

[Tom Tromey]

>>>>> ">" == paul-naert via Gdb-patches <gdb-patches@sourceware.org> writes:

>> +  /* The numbering of registers needs to be adjusted to simplify machine instructions production.  */
>> +  int correct[16]={0,3,1,2,6,7,5,4,8,9,10,11,12,13,14,15};

It seems like nearly any register could be seen here, so this code would
have to adapt a bit more.

>> +  if(regnum>15 || regnum < 0)
>> +  {
>> +    fprintf_filtered(gdb_stdlog, "Expected register number < 16\n");
>> +    fprintf_filtered(gdb_stdlog, "Got number %d \n", regnum);
>> +    return 0;

It's better in gdb to call error in these situations.
Then you also can just change the return type to void.

>> +  set_gdbarch_jmp_insn_length(gdbarch);

This should take a constant as an argument.

>> +void _initialize_compile_patch();
>> +static void
>> +_initialize_compile_patch (void)

The "static" and "void" here should be removed.

Tom

Re: [RFC][PATCH 0/7] Patching new code in a binary

[Tom Tromey]

>>>>> ">" == paul-naert via Gdb-patches <gdb-patches@sourceware.org> writes:

>> I'm looking for input on a series of patches that I wrote to extend
>> the compile command. At the moment, this commands lets the user type
>> in C/C++ code that is compiled by GCC and then executed in the
>> context of the program.

This is fantastic.

>> If there is interest in the commands, I will work on the
>> documentation and test cases.

Yes, please.  I think this is very interesting work.

>> - Red zone handling. This complicates the trampoline. I believe that
>> this issue was fixed by the manipulation I did in the trampoline, but
>> this seems to be absent from the GDB tracepoints. Is there something
>> I missed or is this a bug of fast tracepoints?

I don't know, sorry.

>> - Since we save and restore all registers, some instrumentation code
>> may not be able to properly modify variable values.

The original idea in the compile approach was that the code could write
into the structure-of-register-values, and then on return these values
would be loaded back into the registers.  So, in patch 7, this could be
done after calling the patched function, but before executing the
displaced instruction.

>> If the community is interested in this functionnality, I have also
>> worked on making it more usable by getting rid of the 5 byte
>> limitation using instruction punning. This series of patches works,
>> but is not compliant yet with the GDB standards.

I didn't go through and mention the coding style stuff, but there's a
fair amount of changes to be done there.  Also ChangeLogs are required
for a final patch.

I think this is very promising, though.

One related thing I'm interested in is extending this to compile
breakpoint conditions.

Tom