[review] jit: enhance test suite

[review] jit: enhance test suite

[Mihails Strasuns (Code Review)]

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/757
......................................................................

jit: enhance test suite

Refactoring and enhancement of jit-main.c related facilities which:

1) Enables calling a specified function from the pseudo-jit elf
   binary.
2) Does simple adjustment of debug info by replacing all mentions of
   called function original address with relocated address. It does not
   fully fix the debug information (most notably line info remains
   unusable) but it makes possible to put a breakpoint on such function.
3) Splits functions for loading and adjusting an elf binary into a new
   header file, "jit-elf.h", so that it can be reused later in other
   test cases. Splis jit-related definitions into "jit-defs.h" for the
   same purpose.

This commit does not modify existing testing scenarios and is intended
to serve as a base for further jit test additions.

gdb/testsuite/ChangeLog:
2019-12-18  Mihails Strasuns  <mihails.strasuns@intel.com>
	* jit-elf.h, jit-defs.h: new file
	* jit-main.c: refactoring to use "jit-elf.h" and "jit-defs.h",
	  now will also execute the registered jit function

Signed-off-by: Mihails Strasuns <mihails.strasuns@intel.com>
Change-Id: I5f9d56e8eaaa7cf88881eda6a92a72962dd3f066
---
A gdb/testsuite/gdb.base/jit-defs.h
A gdb/testsuite/gdb.base/jit-elf.h
M gdb/testsuite/gdb.base/jit-main.c
3 files changed, 253 insertions(+), 118 deletions(-)



diff --git a/gdb/testsuite/gdb.base/jit-defs.h b/gdb/testsuite/gdb.base/jit-defs.h
new file mode 100644
index 0000000..d349d28
--- /dev/null
+++ b/gdb/testsuite/gdb.base/jit-defs.h
@@ -0,0 +1,50 @@
+/* This test program is part of GDB, the GNU debugger.
+
+   Copyright 2019 Free Software Foundation, Inc.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+#include <stdint.h>
+
+typedef enum
+{
+  JIT_NOACTION = 0,
+  JIT_REGISTER_FN,
+  JIT_UNREGISTER_FN
+} jit_actions_t;
+
+struct jit_code_entry
+{
+  struct jit_code_entry *next_entry;
+  struct jit_code_entry *prev_entry;
+  const char *symfile_addr;
+  uint64_t symfile_size;
+};
+
+struct jit_descriptor
+{
+  uint32_t version;
+  /* This type should be jit_actions_t, but we use uint32_t
+     to be explicit about the bitwidth.  */
+  uint32_t action_flag;
+  struct jit_code_entry *relevant_entry;
+  struct jit_code_entry *first_entry;
+};
+
+/* GDB puts a breakpoint in this function.  */
+void __attribute__((noinline)) __jit_debug_register_code () { }
+
+/* Make sure to specify the version statically, because the
+   debugger may check the version before we can set it.  */
+struct jit_descriptor __jit_debug_descriptor = { 1, 0, 0, 0 };
\ No newline at end of file
diff --git a/gdb/testsuite/gdb.base/jit-elf.h b/gdb/testsuite/gdb.base/jit-elf.h
new file mode 100644
index 0000000..61de3b4
--- /dev/null
+++ b/gdb/testsuite/gdb.base/jit-elf.h
@@ -0,0 +1,184 @@
+/* This test program is part of GDB, the GNU debugger.
+
+   Copyright 2019 Free Software Foundation, Inc.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+/* Simulates loading of JIT code by memory mapping a compiled
+   shared library binary and doing minimal post-processing.  */
+
+#include <elf.h>
+#include <errno.h>
+#include <fcntl.h>
+#include <link.h>
+#include <stdint.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+#include <sys/mman.h>
+#include <sys/stat.h>
+#include <unistd.h>
+
+/* ElfW is coming from linux. On other platforms it does not exist.
+   Let us define it here. */
+#ifndef ElfW
+#if (defined(_LP64) || defined(__LP64__))
+#define WORDSIZE 64
+#else
+#define WORDSIZE 32
+#endif /* _LP64 || __LP64__  */
+#define ElfW(type) _ElfW (Elf, WORDSIZE, type)
+#define _ElfW(e, w, t) _ElfW_1 (e, w, _##t)
+#define _ElfW_1(e, w, t) e##w##t
+#endif /* !ElfW  */
+
+/* Update section addresses to use `addr` as a base.
+   If `rename_num` is >= 0, look into string tables for entry
+   "jit_function_XXXX" and update it to use the supplied number. */
+static void
+update_locations (ElfW (Addr) addr, int rename_num)
+{
+  const ElfW (Ehdr) *const ehdr = (ElfW (Ehdr) *) addr;
+  ElfW (Shdr) *const shdr = (ElfW (Shdr) *) ((char *) addr + ehdr->e_shoff);
+  ElfW (Phdr) *const phdr = (ElfW (Phdr) *) ((char *) addr + ehdr->e_phoff);
+
+  /* Update .p_vaddr and .sh_addr as if the code was JITted to ADDR.  */
+
+  for (int i = 0; i < ehdr->e_phnum; ++i)
+    if (phdr[i].p_type == PT_LOAD)
+      phdr[i].p_vaddr += addr;
+
+  for (int i = 0; i < ehdr->e_shnum; ++i)
+    {
+      if (rename_num >= 0 && shdr[i].sh_type == SHT_STRTAB)
+	{
+	  /* Note: we update both .strtab and .dynstr.  The latter would
+	     not be correct if this were a regular shared library (.hash
+	     would be wrong), but this is a simulation -- the library is
+	     never exposed to the dynamic loader, so it all ends up ok.  */
+	  char *const strtab = (char *) (addr + shdr[i].sh_offset);
+	  char *const strtab_end = strtab + shdr[i].sh_size;
+	  char *p;
+
+	  for (p = strtab; p < strtab_end; p += strlen (p) + 1)
+	    if (strcmp (p, "jit_function_XXXX") == 0)
+	      sprintf (p, "jit_function_%04d", rename_num);
+	}
+
+      if (shdr[i].sh_flags & SHF_ALLOC)
+	shdr[i].sh_addr += addr;
+    }
+}
+
+/* Find symbol with the name `sym_name`, relocate it to
+   use `addr` as a base and update all mentions to use the
+   new address. */
+static ElfW (Addr) load_symbol (ElfW (Addr) addr, const char *sym_name)
+{
+  const ElfW (Ehdr) *const ehdr = (ElfW (Ehdr) *) addr;
+  ElfW (Shdr) *const shdr = (ElfW (Shdr) *) ((char *) addr + ehdr->e_shoff);
+
+  ElfW (Addr) sym_old_addr = 0;
+  ElfW (Addr) sym_new_addr = 0;
+
+  /* Find `func_name` in symbol_table and adjust it from the addr */
+
+  for (int i = 0; i < ehdr->e_shnum; ++i)
+    {
+      if (shdr[i].sh_type == SHT_SYMTAB)
+	{
+	  ElfW (Sym) *symtab = (ElfW (Sym) *) (addr + shdr[i].sh_offset);
+	  ElfW (Sym) *symtab_end
+	      = (ElfW (Sym) *) (addr + shdr[i].sh_offset + shdr[i].sh_size);
+	  char *const strtab
+	      = (char *) (addr + shdr[shdr[i].sh_link].sh_offset);
+
+	  for (ElfW (Sym) *p = symtab; p < symtab_end; p += 1)
+	    {
+	      const char *s = strtab + p->st_name;
+	      if (strcmp (s, sym_name) == 0)
+		{
+		  sym_old_addr = p->st_value;
+		  p->st_value += addr;
+		  sym_new_addr = p->st_value;
+		  break;
+		}
+	    }
+
+	  if (sym_new_addr != 0)
+	    break;
+	}
+    }
+
+  if (sym_new_addr == 0)
+    {
+      fprintf (stderr, "symbol '%s' not found\n", sym_name);
+      exit (1);
+    }
+
+  /* Find all mentions of `func_name` old address in debug sections and adjust
+   * values */
+
+  for (int i = 0; i < ehdr->e_shnum; ++i)
+    {
+      if (shdr[i].sh_type == SHT_PROGBITS)
+	{
+	  size_t *start = (size_t *) (addr + shdr[i].sh_offset);
+	  size_t *end
+	      = (size_t *) (addr + shdr[i].sh_offset + shdr[i].sh_size);
+	  for (size_t *p = start; p < end; ++p)
+	    if (*p == (size_t) sym_old_addr)
+	      *p = (size_t) sym_new_addr;
+	}
+    }
+
+  return sym_new_addr;
+}
+
+/* Open an elf binary file and memory map it
+   with execution flag enabled. */
+ElfW (Addr) load_elf (const char *libname, size_t *size, ElfW (Addr) old_addr)
+{
+  int fd;
+  struct stat st;
+
+  if ((fd = open (libname, O_RDONLY)) == -1)
+    {
+      fprintf (stderr, "open (\"%s\", O_RDONLY): %s\n", libname,
+	       strerror (errno));
+      exit (1);
+    }
+
+  if (fstat (fd, &st) != 0)
+    {
+      fprintf (stderr, "fstat (\"%d\"): %s\n", fd, strerror (errno));
+      exit (1);
+    }
+
+  void *addr = mmap ((void *) old_addr, st.st_size,
+		     PROT_READ | PROT_WRITE | PROT_EXEC,
+		     old_addr ? MAP_PRIVATE | MAP_FIXED : MAP_PRIVATE, fd, 0);
+  close (fd);
+
+  if (addr == MAP_FAILED)
+    {
+      fprintf (stderr, "mmap: %s\n", strerror (errno));
+      exit (1);
+    }
+
+  if (size != NULL)
+    *size = st.st_size;
+
+  return (ElfW (Addr)) addr;
+}
\ No newline at end of file
diff --git a/gdb/testsuite/gdb.base/jit-main.c b/gdb/testsuite/gdb.base/jit-main.c
index c8e177c..4a82793 100644
--- a/gdb/testsuite/gdb.base/jit-main.c
+++ b/gdb/testsuite/gdb.base/jit-main.c
@@ -17,62 +17,8 @@
 
 /* Simulate loading of JIT code.  */
 
-#include <elf.h>
-#include <link.h>
-#include <errno.h>
-#include <fcntl.h>
-#include <stdint.h>
-#include <stdio.h>
-#include <stdlib.h>
-#include <string.h>
-#include <sys/mman.h>
-#include <sys/stat.h>
-#include <unistd.h>
-
-/* ElfW is coming from linux. On other platforms it does not exist.
-   Let us define it here. */
-#ifndef ElfW
-# if (defined  (_LP64) || defined (__LP64__)) 
-#   define WORDSIZE 64
-# else
-#   define WORDSIZE 32
-# endif /* _LP64 || __LP64__  */
-#define ElfW(type)      _ElfW (Elf, WORDSIZE, type)
-#define _ElfW(e,w,t)    _ElfW_1 (e, w, _##t)
-#define _ElfW_1(e,w,t)  e##w##t
-#endif /* !ElfW  */
-
-typedef enum
-{
-  JIT_NOACTION = 0,
-  JIT_REGISTER_FN,
-  JIT_UNREGISTER_FN
-} jit_actions_t;
-
-struct jit_code_entry
-{
-  struct jit_code_entry *next_entry;
-  struct jit_code_entry *prev_entry;
-  const char *symfile_addr;
-  uint64_t symfile_size;
-};
-
-struct jit_descriptor
-{
-  uint32_t version;
-  /* This type should be jit_actions_t, but we use uint32_t
-     to be explicit about the bitwidth.  */
-  uint32_t action_flag;
-  struct jit_code_entry *relevant_entry;
-  struct jit_code_entry *first_entry;
-};
-
-/* GDB puts a breakpoint in this function.  */
-void __attribute__((noinline)) __jit_debug_register_code () { }
-
-/* Make sure to specify the version statically, because the
-   debugger may check the version before we can set it.  */
-struct jit_descriptor __jit_debug_descriptor = { 1, 0, 0, 0 };
+#include "jit-elf.h"
+#include "jit-defs.h"
 
 static void
 usage (const char *const argv0)
@@ -81,42 +27,6 @@
   exit (1);
 }
 
-/* Update .p_vaddr and .sh_addr as if the code was JITted to ADDR.  */
-
-static void
-update_locations (const void *const addr, int idx)
-{
-  const ElfW (Ehdr) *const ehdr = (ElfW (Ehdr) *)addr;
-  ElfW (Shdr) *const shdr = (ElfW (Shdr) *)((char *)addr + ehdr->e_shoff);
-  ElfW (Phdr) *const phdr = (ElfW (Phdr) *)((char *)addr + ehdr->e_phoff);
-  int i;
-
-  for (i = 0; i < ehdr->e_phnum; ++i)
-    if (phdr[i].p_type == PT_LOAD)
-      phdr[i].p_vaddr += (ElfW (Addr))addr;
-
-  for (i = 0; i < ehdr->e_shnum; ++i)
-    {
-      if (shdr[i].sh_type == SHT_STRTAB)
-        {
-          /* Note: we update both .strtab and .dynstr.  The latter would
-             not be correct if this were a regular shared library (.hash
-             would be wrong), but this is a simulation -- the library is
-             never exposed to the dynamic loader, so it all ends up ok.  */
-          char *const strtab = (char *)((ElfW (Addr))addr + shdr[i].sh_offset);
-          char *const strtab_end = strtab + shdr[i].sh_size;
-          char *p;
-
-          for (p = strtab; p < strtab_end; p += strlen (p) + 1)
-            if (strcmp (p, "jit_function_XXXX") == 0)
-              sprintf (p, "jit_function_%04d", idx);
-        }
-
-      if (shdr[i].sh_flags & SHF_ALLOC)
-        shdr[i].sh_addr += (ElfW (Addr))addr;
-    }
-}
-
 /* Defined by the .exp file if testing attach.  */
 #ifndef ATTACH
 #define ATTACH 0
@@ -138,8 +48,7 @@
 {
   /* These variables are here so they can easily be set from jit.exp.  */
   const char *libname = NULL;
-  int count = 0, i, fd;
-  struct stat st;
+  int count = 0, i;
 
   alarm (300);
 
@@ -164,36 +73,22 @@
   printf ("%s:%d: libname = %s, count = %d\n", __FILE__, __LINE__,
 	  libname, count);
 
-  if ((fd = open (libname, O_RDONLY)) == -1)
-    {
-      fprintf (stderr, "open (\"%s\", O_RDONLY): %s\n", libname,
-	       strerror (errno));
-      exit (1);
-    }
-
-  if (fstat (fd, &st) != 0)
-    {
-      fprintf (stderr, "fstat (\"%d\"): %s\n", fd, strerror (errno));
-      exit (1);
-    }
-
   for (i = 0; i < count; ++i)
     {
-      const void *const addr = mmap (0, st.st_size, PROT_READ|PROT_WRITE,
-				     MAP_PRIVATE, fd, 0);
-      struct jit_code_entry *const entry = calloc (1, sizeof (*entry));
-
-      if (addr == MAP_FAILED)
-	{
-	  fprintf (stderr, "mmap: %s\n", strerror (errno));
-	  exit (1);
-	}
-
+      size_t obj_size;
+      ElfW (Addr) addr = load_elf (libname, &obj_size);
       update_locations (addr, i);
 
+      char name[32];
+      sprintf (name, "jit_function_%04d", i);
+      int (*jit_function) () = (int (*) ()) load_symbol (addr, name);
+
+      struct jit_code_entry *const entry
+	  = (struct jit_code_entry *) calloc (1, sizeof (*entry));
+
       /* Link entry at the end of the list.  */
       entry->symfile_addr = (const char *)addr;
-      entry->symfile_size = st.st_size;
+      entry->symfile_size = obj_size;
       entry->prev_entry = __jit_debug_descriptor.relevant_entry;
       __jit_debug_descriptor.relevant_entry = entry;
 
@@ -205,6 +100,12 @@
       /* Notify GDB.  */
       __jit_debug_descriptor.action_flag = JIT_REGISTER_FN;
       __jit_debug_register_code ();
+
+      if (jit_function() != 42)
+	{
+	  fprintf (stderr, "unexpected return value\n");
+	  exit (1);
+	}
     }
 
   WAIT_FOR_GDB; i = 0;  /* gdb break here 1 */

-- 
Gerrit-Project: binutils-gdb
Gerrit-Branch: master
Gerrit-Change-Id: I5f9d56e8eaaa7cf88881eda6a92a72962dd3f066
Gerrit-Change-Number: 757
Gerrit-PatchSet: 1
Gerrit-Owner: Mihails Strasuns <mihails.strasuns@intel.com>
Gerrit-MessageType: newchange

[review] jit: enhance test suite

[Mihails Strasuns (Code Review)]

Mihails Strasuns has posted comments on this change.

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/757
......................................................................


Patch Set 1:

Ping. Are there any concerns regarding this proposal?


-- 
Gerrit-Project: binutils-gdb
Gerrit-Branch: master
Gerrit-Change-Id: I5f9d56e8eaaa7cf88881eda6a92a72962dd3f066
Gerrit-Change-Number: 757
Gerrit-PatchSet: 1
Gerrit-Owner: Mihails Strasuns <mihails.strasuns@intel.com>
Gerrit-Reviewer: Mihails Strasuns <mihails.strasuns@intel.com>
Gerrit-Reviewer: Simon Marchi <simon.marchi@polymtl.ca>
Gerrit-Comment-Date: Wed, 29 Jan 2020 15:33:47 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: No
Gerrit-MessageType: comment

[review] jit: enhance test suite

[Simon Marchi (Code Review)]

Simon Marchi has posted comments on this change.

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/757
......................................................................


Patch Set 1:

> Patch Set 1:
> 
> Ping. Are there any concerns regarding this proposal?

Sorry about that, I haven't had time to come back to the JIT stuff (which includes this).  I'll try to look at it soon, feel free to keep pinging weekly if I don't.


-- 
Gerrit-Project: binutils-gdb
Gerrit-Branch: master
Gerrit-Change-Id: I5f9d56e8eaaa7cf88881eda6a92a72962dd3f066
Gerrit-Change-Number: 757
Gerrit-PatchSet: 1
Gerrit-Owner: Mihails Strasuns <mihails.strasuns@intel.com>
Gerrit-Reviewer: Mihails Strasuns <mihails.strasuns@intel.com>
Gerrit-Reviewer: Simon Marchi <simon.marchi@polymtl.ca>
Gerrit-Comment-Date: Wed, 29 Jan 2020 15:50:45 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: No
Gerrit-MessageType: comment

[review] jit: enhance test suite

[Simon Marchi (Code Review)]

Simon Marchi has posted comments on this change.

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/757
......................................................................


Patch Set 1:

(7 comments)

As mentioned in my commit in jit-elf.h, I don't think this is a very good way to go.  I'm afraid it will lead to a very fragile test.

What about this: the -Ttext-segment=org linker option allows us to define the base of the text segment of the generated binary.  We could use that to generate a binary whose text is specifically made to live at a certain address (say, 0x800000), and the generated DWARF information will automatically be generated based on that address.

I checked ld.bfd (GNU ld), ld.gold and ld.lld, and they all support this option, so it would be a relatively portable solution, I think.

| --- /dev/null
| +++ gdb/testsuite/gdb.base/jit-elf.h
| @@ -1,0 +39,19 @@ #else
| +#define WORDSIZE 32
| +#endif /* _LP64 || __LP64__  */
| +#define ElfW(type) _ElfW (Elf, WORDSIZE, type)
| +#define _ElfW(e, w, t) _ElfW_1 (e, w, _##t)
| +#define _ElfW_1(e, w, t) e##w##t
| +#endif /* !ElfW  */
| +
| +/* Update section addresses to use `addr` as a base.
| +   If `rename_num` is >= 0, look into string tables for entry
| +   "jit_function_XXXX" and update it to use the supplied number. */

PS1, Line 48:

We use two spaces after periods (even those at the end of the
comment).

| +static void
| +update_locations (ElfW (Addr) addr, int rename_num)
| +{
| +  const ElfW (Ehdr) *const ehdr = (ElfW (Ehdr) *) addr;
| +  ElfW (Shdr) *const shdr = (ElfW (Shdr) *) ((char *) addr + ehdr->e_shoff);
| +  ElfW (Phdr) *const phdr = (ElfW (Phdr) *) ((char *) addr + ehdr->e_phoff);
| +
| +  /* Update .p_vaddr and .sh_addr as if the code was JITted to ADDR.  */
| +

 ...

| @@ -1,0 +78,19 @@ update_locations (ElfW (Addr) addr, int rename_num)
| +
| +      if (shdr[i].sh_flags & SHF_ALLOC)
| +	shdr[i].sh_addr += addr;
| +    }
| +}
| +
| +/* Find symbol with the name `sym_name`, relocate it to
| +   use `addr` as a base and update all mentions to use the
| +   new address. */
| +static ElfW (Addr) load_symbol (ElfW (Addr) addr, const char *sym_name)

PS1, Line 87:

The function name should be at column 0, on the next line.

| +{
| +  const ElfW (Ehdr) *const ehdr = (ElfW (Ehdr) *) addr;
| +  ElfW (Shdr) *const shdr = (ElfW (Shdr) *) ((char *) addr + ehdr->e_shoff);
| +
| +  ElfW (Addr) sym_old_addr = 0;
| +  ElfW (Addr) sym_new_addr = 0;
| +
| +  /* Find `func_name` in symbol_table and adjust it from the addr */
| +

 ...

| @@ -1,0 +98,19 @@ static ElfW (Addr) load_symbol (ElfW (Addr) addr, const char *sym_name)
| +    {
| +      if (shdr[i].sh_type == SHT_SYMTAB)
| +	{
| +	  ElfW (Sym) *symtab = (ElfW (Sym) *) (addr + shdr[i].sh_offset);
| +	  ElfW (Sym) *symtab_end
| +	      = (ElfW (Sym) *) (addr + shdr[i].sh_offset + shdr[i].sh_size);
| +	  char *const strtab
| +	      = (char *) (addr + shdr[shdr[i].sh_link].sh_offset);
| +
| +	  for (ElfW (Sym) *p = symtab; p < symtab_end; p += 1)

PS1, Line 107:

p++?

| +	    {
| +	      const char *s = strtab + p->st_name;
| +	      if (strcmp (s, sym_name) == 0)
| +		{
| +		  sym_old_addr = p->st_value;
| +		  p->st_value += addr;
| +		  sym_new_addr = p->st_value;
| +		  break;
| +		}

 ...

| @@ -1,0 +122,28 @@ static ElfW (Addr) load_symbol (ElfW (Addr) addr, const char *sym_name)
| +    }
| +
| +  if (sym_new_addr == 0)
| +    {
| +      fprintf (stderr, "symbol '%s' not found\n", sym_name);
| +      exit (1);
| +    }
| +
| +  /* Find all mentions of `func_name` old address in debug sections and adjust
| +   * values */

PS1, Line 131:

End with a period (and two spaces).

| +
| +  for (int i = 0; i < ehdr->e_shnum; ++i)
| +    {
| +      if (shdr[i].sh_type == SHT_PROGBITS)
| +	{
| +	  size_t *start = (size_t *) (addr + shdr[i].sh_offset);
| +	  size_t *end
| +	      = (size_t *) (addr + shdr[i].sh_offset + shdr[i].sh_size);
| +	  for (size_t *p = start; p < end; ++p)

PS1, Line 140:

I'm not super comfortable with this.  On one hand, I understand that
it would be unthinkable to actually parse and modify the DWARF.
However, I'm afraid that blindly updating bytes like this may lead to
false positives and false negatives, leading to test instability
(remember that this runs on many architectures).

For example, we might update bytes that happen to match the address
but aren't actually the address.  Also, in DWARF, numbers aren't
always expressed directly as 4 or 8 consecutive bytes.  For example,
there's LEB128 (not sure if that is actually used to represent
addresses though).  Also, I don't think that the addresses you'll want
to replace will always end up aligned on a multiple of `size_t`, which
this code seems to assume.

| +	    if (*p == (size_t) sym_old_addr)
| +	      *p = (size_t) sym_new_addr;
| +	}
| +    }
| +
| +  return sym_new_addr;
| +}
| +
| +/* Open an elf binary file and memory map it
| --- gdb/testsuite/gdb.base/jit-main.c
| +++ gdb/testsuite/gdb.base/jit-main.c
| @@ -184,10 +78,11 @@ MAIN (int argc, char *argv[])
| -      struct jit_code_entry *const entry = calloc (1, sizeof (*entry));
| -
| -      if (addr == MAP_FAILED)
| -	{
| -	  fprintf (stderr, "mmap: %s\n", strerror (errno));
| -	  exit (1);
| -	}
| -
| +      size_t obj_size;
| +      ElfW (Addr) addr = load_elf (libname, &obj_size);

PS1, Line 79:

This is missing an argument:

 jit-main.c:79:26: error: too few arguments to function ‘load_elf’
    79 |       ElfW (Addr) addr = load_elf (libname, &obj_size);
       |                          ^~~~~~~~

|        update_locations (addr, i);
|  
| +      char name[32];
| +      sprintf (name, "jit_function_%04d", i);
| +      int (*jit_function) () = (int (*) ()) load_symbol (addr, name);
| +
| +      struct jit_code_entry *const entry
| +	  = (struct jit_code_entry *) calloc (1, sizeof (*entry));
| +

 ...

| @@ -200,13 +95,19 @@ MAIN (int argc, char *argv[])
|        if (entry->prev_entry != NULL)
|  	entry->prev_entry->next_entry = entry;
|        else
|  	__jit_debug_descriptor.first_entry = entry;
|  
|        /* Notify GDB.  */
|        __jit_debug_descriptor.action_flag = JIT_REGISTER_FN;
|        __jit_debug_register_code ();
| +
| +      if (jit_function() != 42)

PS1, Line 104:

Missing space before the parentheses.

| +	{
| +	  fprintf (stderr, "unexpected return value\n");
| +	  exit (1);
| +	}
|      }
|  
|    WAIT_FOR_GDB; i = 0;  /* gdb break here 1 */
|  
|    /* Now unregister them all in reverse order.  */

-- 
Gerrit-Project: binutils-gdb
Gerrit-Branch: master
Gerrit-Change-Id: I5f9d56e8eaaa7cf88881eda6a92a72962dd3f066
Gerrit-Change-Number: 757
Gerrit-PatchSet: 1
Gerrit-Owner: Mihails Strasuns <mihails.strasuns@intel.com>
Gerrit-Reviewer: Mihails Strasuns <mihails.strasuns@intel.com>
Gerrit-Reviewer: Simon Marchi <simon.marchi@polymtl.ca>
Gerrit-Comment-Date: Wed, 05 Feb 2020 03:27:34 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment

[review] jit: enhance test suite

[Mihails Strasuns (Code Review)]

Mihails Strasuns has posted comments on this change.

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/757
......................................................................


Patch Set 1:

> Patch Set 1:
> What about this: the -Ttext-segment=org linker option allows us to define the base of the text segment of the generated binary.  We could use that to generate a binary whose text is specifically made to live at a certain address (say, 0x800000), and the generated DWARF information will automatically be generated based on that address.

I like this a lot and from a quick experiment it does seem to work as desired.

One clarification about existing jit.exp tests though - they currently try to load the same library multiple times in a loop and register it as different jit instances. If dynamic binary patching is replaced with -Ttext-segment approach, it won't work anymore. I am currently planning to change it to compile separate binaries each with own fixed base address. Is that OK?


-- 
Gerrit-Project: binutils-gdb
Gerrit-Branch: master
Gerrit-Change-Id: I5f9d56e8eaaa7cf88881eda6a92a72962dd3f066
Gerrit-Change-Number: 757
Gerrit-PatchSet: 1
Gerrit-Owner: Mihails Strasuns <mihails.strasuns@intel.com>
Gerrit-Reviewer: Mihails Strasuns <mihails.strasuns@intel.com>
Gerrit-Reviewer: Simon Marchi <simon.marchi@polymtl.ca>
Gerrit-Comment-Date: Thu, 06 Feb 2020 14:07:14 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: No
Gerrit-MessageType: comment

[review] jit: enhance test suite

[Simon Marchi (Code Review)]

Simon Marchi has posted comments on this change.

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/757
......................................................................


Patch Set 1:

> Patch Set 1:
> 
> > Patch Set 1:
> > What about this: the -Ttext-segment=org linker option allows us to define the base of the text segment of the generated binary.  We could use that to generate a binary whose text is specifically made to live at a certain address (say, 0x800000), and the generated DWARF information will automatically be generated based on that address.
> 
> I like this a lot and from a quick experiment it does seem to work as desired.
> 
> One clarification about existing jit.exp tests though - they currently try to load the same library multiple times in a loop and register it as different jit instances. If dynamic binary patching is replaced with -Ttext-segment approach, it won't work anymore. I am currently planning to change it to compile separate binaries each with own fixed base address. Is that OK?

Yeah, I think it would make sense.


-- 
Gerrit-Project: binutils-gdb
Gerrit-Branch: master
Gerrit-Change-Id: I5f9d56e8eaaa7cf88881eda6a92a72962dd3f066
Gerrit-Change-Number: 757
Gerrit-PatchSet: 1
Gerrit-Owner: Mihails Strasuns <mihails.strasuns@intel.com>
Gerrit-Reviewer: Mihails Strasuns <mihails.strasuns@intel.com>
Gerrit-Reviewer: Simon Marchi <simon.marchi@polymtl.ca>
Gerrit-Comment-Date: Thu, 06 Feb 2020 14:22:15 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: No
Gerrit-MessageType: comment

[review] jit: enhance test suite

[Mihails Strasuns (Code Review)]

Mihails Strasuns has abandoned this change. ( https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/757 )

Change subject: jit: enhance test suite
......................................................................


Abandoned

https://sourceware.org/ml/gdb-patches/2020-02/msg00716.html
-- 
Gerrit-Project: binutils-gdb
Gerrit-Branch: master
Gerrit-Change-Id: I5f9d56e8eaaa7cf88881eda6a92a72962dd3f066
Gerrit-Change-Number: 757
Gerrit-PatchSet: 1
Gerrit-Owner: Mihails Strasuns <mihails.strasuns@intel.com>
Gerrit-Reviewer: Mihails Strasuns <mihails.strasuns@intel.com>
Gerrit-Reviewer: Simon Marchi <simon.marchi@polymtl.ca>
Gerrit-MessageType: abandon