JIT read_debug_info callback takes target symfile addr
Commit Message
Hi,
The attached patch makes an incompatible change to the JIT reader
interface to pass the target address of the symfile to the
read_debug_info, instead of eagerly copying the symfile to GDB and
passing the local address to the .so. In many cases this allows the
target to be simpler, as in many cases it's not necessary to allocate a
symfile object at all. In the case where you do need the memory in the
GDB address space, well the target_read() callback is there for you.
I have a patch coming that will allow JIT readers to be implemented in
Guile. It uses the JIT reader interface, though loaded from an
extension instead of a shared library. This patch would help extensions
to be able to do JIT unwinding, as extensions have a much richer
interface to GDB and in particular can query types and sizes from the
inferior, without having to hard-code them from the reader side. What
you want is the target address, not the target's memory.
WDYT?
Comments
Andy Wingo <wingo@igalia.com> writes:
> Hi,
>
> The attached patch makes an incompatible change to the JIT reader
> interface to pass the target address of the symfile to the
> read_debug_info, instead of eagerly copying the symfile to GDB and
> passing the local address to the .so. In many cases this allows the
> target to be simpler, as in many cases it's not necessary to allocate a
> symfile object at all. In the case where you do need the memory in the
> GDB address space, well the target_read() callback is there for you.
>
> I have a patch coming that will allow JIT readers to be implemented in
> Guile. It uses the JIT reader interface, though loaded from an
> extension instead of a shared library. This patch would help extensions
> to be able to do JIT unwinding, as extensions have a much richer
> interface to GDB and in particular can query types and sizes from the
> inferior, without having to hard-code them from the reader side. What
> you want is the target address, not the target's memory.
>
> WDYT?
>
>
> From 983216b213af8c2cf5853a814d2559062018fce4 Mon Sep 17 00:00:00 2001
> From: Andy Wingo <wingo@igalia.com>
> Date: Mon, 23 Feb 2015 15:06:09 +0100
> Subject: [PATCH] JIT read_debug_info callback takes target symfile address
>
> gdb/ChangeLog:
> 2015-02-23 Andy Wingo <wingo@igalia.com>
>
> * NEWS: Announce JIT reader interface change.
> * jit-reader.in (GDB_READER_INTERFACE_VERSION): Bump to 2.
> (gdb_read_debug_info): Update typedef to express the memory as a
> target address, and update the comment.
> * jit.c (jit_reader_try_read_symtab): Don't read the symfile from
> target memory. The JIT reader can do that if it likes using the
> gdb_symbol_callbacks structure.
>
> gdb/testsuite/ChangeLog:
> 2015-02-23 Andy Wingo <wingo@igalia.com>
>
> * gdb.base/jitreader.c (read_debug_info): Update for changes in
> JIT reader interface and jithost.c test.
> * gdb.base/jithost.c (main): No need to allocate symfile objects
> with the new GDB_READER_INTERFACE_VERSION.
> * gdb.base/jithost.h: Removed.
Hi. A couple of high level comments.
1) My understanding is that we still need to support older versions of
the interface, at least for awhile.
Our currently deprecation policy is to never break anything
(or something within epsilon of that).
This doesn't scale. I'd like to see a formal policy where things
can live in a deprecated state for pre-specified awhile, after which
they can be deleted.
2) It's not clear to me whether we want to continue extending the JIT
interface, or just move it to the extension languages.
We'd still keep the existing interface, deprecated.
I'm not proposing we actually do this, at least not today.
Hi Doug,
On Wed 25 Feb 2015 19:17, Doug Evans <xdje42@gmail.com> writes:
> Hi. A couple of high level comments.
>
> 1) My understanding is that we still need to support older versions of
> the interface, at least for awhile.
> Our currently deprecation policy is to never break anything
> (or something within epsilon of that).
Cool. It's not a particular problem to support an older interface.
> This doesn't scale. I'd like to see a formal policy where things
> can live in a deprecated state for pre-specified awhile, after which
> they can be deleted.
ACK.
> 2) It's not clear to me whether we want to continue extending the JIT
> interface, or just move it to the extension languages.
> We'd still keep the existing interface, deprecated.
> I'm not proposing we actually do this, at least not today.
Yeah, I know what you mean. After having some experience with this
(https://groups.google.com/forum/#!topic/v8-users/y18bmM2I5L0) I don't
think the JIT interface is really the right thing for the future.
For C JIT reader extensions, you really don't have enough of an
interface to do many interesting things.
For extension languages, it doesn't feel quite right; you can use
make-value and value-cast and all those things to turn the spare
interface of the custom JIT debuginfo reader into something that feels
more like the normal extension language, but you still lack a lot of
information from the inferior. For example, the architecture seems to
be a property of the frame, but you can't get at the current frame
object from the extension. Likewise specifying registers by DWARF ID is
not so nice. It would be nice if the read-register interface would
return values of the proper size, as proper GDB values -- in my patch I
am returning them as bytevectors because hey, registers are different
sizes, but then you still don't have information on how they should be
interpreted. But that's not something you want to force an extension
writer to know about.
The JIT interface also seems to encourage native (not cross)
development, because you have little information about the target.
Finally the JIT reader callbacks are awkward. I wish actually that I
could unwind frames from the target without requiring the target to
integrate with GDB in any way. I could do that from Guile for V8, for
example, at least for the V8 part of the stack (see mail I linked
above). What I would really like would be a callback to ask the
Guile/Python extension "is this a frame you know about?" and then if so,
let the extension unwind it. Then I wouldn't have to modify the target
in any way, which would be a relief.
What is the right mailing list to discuss these things? Who are the
right people? I'm happy to do work off and on around these things, if
you are interested.
Andy
Hi Andy,
Many thanks for working on this whole problem.
On 02/26/2015 08:58 AM, Andy Wingo wrote:
> What is the right mailing list to discuss these things?
For the GDB side, either here or gdb@ are fine, me thinks.
> Who are the right people? I'm happy to do work off and on around these things, if
> you are interested.
Looks like you could be coordinating your ideas with
Alexander Smundak (added to CC), which is wanting similar
things for OpenJDK:
https://sourceware.org/ml/gdb-patches/2015-02/msg00735.html
Alexander, this thread is here:
https://sourceware.org/ml/gdb-patches/2015-02/msg00739.html
Thanks,
Pedro Alves
On Thu 26 Feb 2015 12:49, Pedro Alves <palves@redhat.com> writes:
> On 02/26/2015 08:58 AM, Andy Wingo wrote:
>
>> Who are the right people? I'm happy to do work off and on around these things, if
>> you are interested.
>
> Looks like you could be coordinating your ideas with
> Alexander Smundak (added to CC), which is wanting similar
> things for OpenJDK:
>
> https://sourceware.org/ml/gdb-patches/2015-02/msg00735.html
>
> Alexander, this thread is here:
>
> https://sourceware.org/ml/gdb-patches/2015-02/msg00739.html
Oh neat. That approach looks more aligned to V8's needs. I will see if
I can cook up a patch based on Alexander's work that does the same thing
in Guile and write an unwinder for V8. I will let you know when I have
this done. Of course I do not intend to block Alexander's work in any
way, but perhaps this could provide additional feedback about the use
case.
Alexander do you have your work in a git repository anywhere? It might
be easier to coordinate, if that's a thing you are interested in, and to
track changes in your work as you react to Doug et al's feedback. I
sometimes sync up to this tree, FWIW:
https://github.com/andywingo/binutils-gdb/commits/master
Regards,
Andy
My GDB repository is internal, sorry. I still hope my patch will be
reviewed soon :-)
How set are you on implementing the unwinder for V8 in Guile rather
than in Python? Having unwinders for different mixed language
environment might help reveal API deficiencies sooner.
Regards
Alexander
On Thu, Feb 26, 2015 at 4:51 AM, Andy Wingo <wingo@igalia.com> wrote:
> On Thu 26 Feb 2015 12:49, Pedro Alves <palves@redhat.com> writes:
>
>> On 02/26/2015 08:58 AM, Andy Wingo wrote:
>>
>>> Who are the right people? I'm happy to do work off and on around these things, if
>>> you are interested.
>>
>> Looks like you could be coordinating your ideas with
>> Alexander Smundak (added to CC), which is wanting similar
>> things for OpenJDK:
>>
>> https://sourceware.org/ml/gdb-patches/2015-02/msg00735.html
>>
>> Alexander, this thread is here:
>>
>> https://sourceware.org/ml/gdb-patches/2015-02/msg00739.html
>
> Oh neat. That approach looks more aligned to V8's needs. I will see if
> I can cook up a patch based on Alexander's work that does the same thing
> in Guile and write an unwinder for V8. I will let you know when I have
> this done. Of course I do not intend to block Alexander's work in any
> way, but perhaps this could provide additional feedback about the use
> case.
>
> Alexander do you have your work in a git repository anywhere? It might
> be easier to coordinate, if that's a thing you are interested in, and to
> track changes in your work as you react to Doug et al's feedback. I
> sometimes sync up to this tree, FWIW:
>
> https://github.com/andywingo/binutils-gdb/commits/master
>
> Regards,
>
> Andy
Hi,
On Thu 26 Feb 2015 22:50, Alexander Smundak <asmundak@google.com> writes:
> How set are you on implementing the unwinder for V8 in Guile rather
> than in Python? Having unwinders for different mixed language
> environment might help reveal API deficiencies sooner.
Which mix -- target or extension language? I assume you refer to the
target (JS/V8 versus Java/OpenJDK). In any case the API that Python and
Guile would expose would be very similar, so I would think that the
feedback on the API doesn't depend much on the extension language.
That said, as I get to choose, I choose Guile :) My V8 debugging tools
are already in Guile, FWIW.
Andy
From 983216b213af8c2cf5853a814d2559062018fce4 Mon Sep 17 00:00:00 2001
From: Andy Wingo <wingo@igalia.com>
Date: Mon, 23 Feb 2015 15:06:09 +0100
Subject: [PATCH] JIT read_debug_info callback takes target symfile address
gdb/ChangeLog:
2015-02-23 Andy Wingo <wingo@igalia.com>
* NEWS: Announce JIT reader interface change.
* jit-reader.in (GDB_READER_INTERFACE_VERSION): Bump to 2.
(gdb_read_debug_info): Update typedef to express the memory as a
target address, and update the comment.
* jit.c (jit_reader_try_read_symtab): Don't read the symfile from
target memory. The JIT reader can do that if it likes using the
gdb_symbol_callbacks structure.
gdb/testsuite/ChangeLog:
2015-02-23 Andy Wingo <wingo@igalia.com>
* gdb.base/jitreader.c (read_debug_info): Update for changes in
JIT reader interface and jithost.c test.
* gdb.base/jithost.c (main): No need to allocate symfile objects
with the new GDB_READER_INTERFACE_VERSION.
* gdb.base/jithost.h: Removed.
---
gdb/ChangeLog | 10 ++++++++++
gdb/NEWS | 9 +++++++++
gdb/jit-reader.in | 15 +++++++++------
gdb/jit.c | 34 +++++++++++-----------------------
gdb/testsuite/ChangeLog | 8 ++++++++
gdb/testsuite/gdb.base/jithost.c | 11 +++--------
gdb/testsuite/gdb.base/jithost.h | 27 ---------------------------
gdb/testsuite/gdb.base/jitreader.c | 22 +++++++++++++---------
8 files changed, 63 insertions(+), 73 deletions(-)
delete mode 100644 gdb/testsuite/gdb.base/jithost.h
@@ -1,3 +1,13 @@
+2015-02-23 Andy Wingo <wingo@igalia.com>
+
+ * NEWS: Announce JIT reader interface change.
+ * jit-reader.in (GDB_READER_INTERFACE_VERSION): Bump to 2.
+ (gdb_read_debug_info): Update typedef to express the memory as a
+ target address, and update the comment.
+ * jit.c (jit_reader_try_read_symtab): Don't read the symfile from
+ target memory. The JIT reader can do that if it likes using the
+ gdb_symbol_callbacks structure.
+
2015-02-20 Andy Wingo <wingo@igalia.com>
* guile/scm-value.c (gdbscm_value_dynamic_type): Fix typo in which
@@ -3,6 +3,15 @@
*** Changes since GDB 7.9
+* The JIT debug info reader interface has changed incompatibly: the
+ "gdb_read_debug_info" callback now receives the symfile_addr argument
+ in the target address space, not GDB's address space.
+ Correspondingly, the GDB_READER_INTERFACE_VERSION has been bumped to
+ 2, which should cause any attempt to load an existing jit-reader
+ shared object to fail. Likewise the gdb_read_debug_info typedef has
+ changed, so simple recompilation will cause an error, which is good as
+ the JIT readers will need to be fixed manually.
+
* The "info source" command now displays the producer string if it was
present in the debug info. This typically includes the compiler version
and may include things like its command line arguments.
@@ -26,7 +26,7 @@ extern "C" {
/* Versioning information. See gdb_reader_funcs. */
-#define GDB_READER_INTERFACE_VERSION 1
+#define GDB_READER_INTERFACE_VERSION 2
/* Readers must be released under a GPL compatible license. To
declare that the reader is indeed released under a GPL compatible
@@ -279,17 +279,20 @@ struct gdb_unwind_callbacks
struct gdb_reader_funcs;
-/* Parse the debug info off a block of memory, pointed to by MEMORY
- (already copied to GDB's address space) and MEMORY_SZ bytes long.
- The implementation has to use the functions in CB to actually emit
- the parsed data into GDB. SELF is the same structure returned by
+/* Parse the debug info from the SYMFILE_SIZE bytes of memory located at
+ SYMFILE_ADDR on the target. These values are taken from the
+ corresponding "struct jit_code_entry" node that the target has
+ registered using the GDB JIT compilation interface. The
+ implementation has to use the functions in CB to actually emit the
+ parsed data into GDB. SELF is the same structure returned by
gdb_init_reader.
Return GDB_FAIL on failure and GDB_SUCCESS on success. */
typedef enum gdb_status (gdb_read_debug_info) (struct gdb_reader_funcs *self,
struct gdb_symbol_callbacks *cb,
- void *memory, long memory_sz);
+ GDB_CORE_ADDR symfile_addr,
+ long symfile_size);
/* Unwind the current frame, CB is the set of unwind callbacks that
are to be used to do this.
@@ -849,10 +849,12 @@ jit_reader_try_read_symtab (struct jit_code_entry *code_entry,
CORE_ADDR entry_addr)
{
void *gdb_mem;
- int status;
+ enum gdb_status status;
jit_dbg_reader_data priv_data;
struct gdb_reader_funcs *funcs;
volatile struct gdb_exception e;
+ GDB_CORE_ADDR symfile_addr;
+ long symfile_size;
struct gdb_symbol_callbacks callbacks =
{
jit_object_open_impl,
@@ -867,34 +869,20 @@ jit_reader_try_read_symtab (struct jit_code_entry *code_entry,
&priv_data
};
- priv_data = entry_addr;
-
if (!jit_reader_loaded ())
return 0;
- gdb_mem = xmalloc (code_entry->symfile_size);
-
- status = 1;
- TRY_CATCH (e, RETURN_MASK_ALL)
- if (target_read_memory (code_entry->symfile_addr, gdb_mem,
- code_entry->symfile_size))
- status = 0;
- if (e.reason < 0)
- status = 0;
-
- if (status)
- {
- funcs = loaded_jit_reader->functions;
- if (funcs->read (funcs, &callbacks, gdb_mem, code_entry->symfile_size)
- != GDB_SUCCESS)
- status = 0;
- }
+ priv_data = entry_addr;
+ funcs = loaded_jit_reader->functions;
+ symfile_addr = code_entry->symfile_addr;
+ symfile_size = code_entry->symfile_size;
+ status = funcs->read (funcs, &callbacks, symfile_addr, symfile_size);
- xfree (gdb_mem);
- if (jit_debug && status == 0)
+ if (jit_debug && status == GDB_FAIL)
fprintf_unfiltered (gdb_stdlog,
"Could not read symtab using the loaded JIT reader.\n");
- return status;
+
+ return status == GDB_SUCCESS;
}
/* Try to read CODE_ENTRY using BFD. ENTRY_ADDR is the address of the
@@ -1,3 +1,11 @@
+2015-02-23 Andy Wingo <wingo@igalia.com>
+
+ * gdb.base/jitreader.c (read_debug_info): Update for changes in
+ JIT reader interface and jithost.c test.
+ * gdb.base/jithost.c (main): No need to allocate symfile objects
+ with the new GDB_READER_INTERFACE_VERSION.
+ * gdb.base/jithost.h: Removed.
+
2015-02-23 Sanjoy Das <sanjoy@playingwithpointers.com>
* gdb.base/jit-reader.exp: New file. Test case for the jit-reader
@@ -1,4 +1,4 @@
-/* Copyright (C) 2009-2012 Free Software Foundation, Inc.
+/* Copyright (C) 2009-2012, 2015 Free Software Foundation, Inc.
This file is part of GDB.
@@ -23,7 +23,6 @@
#include <sys/mman.h>
#include JIT_READER_H /* Please see jit-reader.exp for an explanation. */
-#include "jithost.h"
#include "jit-protocol.h"
void __attribute__((noinline)) __jit_debug_register_code () { }
@@ -42,12 +41,8 @@ int main (int argc, char **argv)
code[0] = 0xcc; /* SIGTRAP */
code[1] = 0xc3; /* RET */
- struct jithost_abi *symfile = malloc (sizeof (struct jithost_abi));
- symfile->begin = code;
- symfile->end = code + 2;
-
- only_entry.symfile_addr = symfile;
- only_entry.symfile_size = sizeof (struct jithost_abi);
+ only_entry.symfile_addr = code;
+ only_entry.symfile_size = 2;
__jit_debug_descriptor.first_entry = &only_entry;
__jit_debug_descriptor.relevant_entry = &only_entry;
deleted file mode 100644
@@ -1,27 +0,0 @@
-/* Copyright (C) 2009-2012 Free Software Foundation, Inc.
-
- This file is part of GDB.
-
- This program is free software; you can redistribute it and/or modify
- it under the terms of the GNU General Public License as published by
- the Free Software Foundation; either version 3 of the License, or
- (at your option) any later version.
-
- This program is distributed in the hope that it will be useful,
- but WITHOUT ANY WARRANTY; without even the implied warranty of
- MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
- GNU General Public License for more details.
-
- You should have received a copy of the GNU General Public License
- along with this program. If not, see <http://www.gnu.org/licenses/>. */
-
-#ifndef JITHOST_H
-#define JITHOST_H
-
-struct jithost_abi
-{
- const char *begin;
- const char *end;
-};
-
-#endif /* JITHOST_H */
@@ -1,4 +1,4 @@
-/* Copyright (C) 2009-2012 Free Software Foundation, Inc.
+/* Copyright (C) 2009-2012, 2015 Free Software Foundation, Inc.
This file is part of GDB.
@@ -21,7 +21,6 @@
#include <string.h>
#include JIT_READER_H /* Please see jit-reader.exp for an explanation. */
-#include "jithost.h"
GDB_DECLARE_GPL_COMPATIBLE_READER;
@@ -40,18 +39,23 @@ struct reader_state
static enum gdb_status
read_debug_info (struct gdb_reader_funcs *self,
struct gdb_symbol_callbacks *cbs,
- void *memory, long memory_sz)
+ GDB_CORE_ADDR symfile_addr, long symfile_size)
{
- struct jithost_abi *symfile = memory;
- struct gdb_object *object = cbs->object_open (cbs);
- struct gdb_symtab *symtab = cbs->symtab_open (cbs, object, "");
- GDB_CORE_ADDR begin = (GDB_CORE_ADDR) symfile->begin;
- GDB_CORE_ADDR end = (GDB_CORE_ADDR) symfile->end;
+ struct gdb_object *object;
+ struct gdb_symtab *symtab;
+ GDB_CORE_ADDR begin, end;
- cbs->block_open (cbs, symtab, NULL, begin, end, "jit_function_00");
+ /* For this test, there is no additional metadata in a symbol, so it the code
+ itself can serve as the symfile. */
+ begin = symfile_addr;
+ end = symfile_addr + symfile_size;
+ object = cbs->object_open (cbs);
+ symtab = cbs->symtab_open (cbs, object, "");
+ cbs->block_open (cbs, symtab, NULL, begin, end, "jit_function_00");
cbs->symtab_close (cbs, symtab);
cbs->object_close (cbs, object);
+
return GDB_SUCCESS;
}
--
2.1.4