[PATCH v3 2/9] gdb/jit: link to jit_objfile_data directly from the objfile struct
Aktemur, Tankut Baris
tankut.baris.aktemur@intel.com
Tue Jul 21 16:25:38 GMT 2020
On Wednesday, July 15, 2020 4:17 PM, Simon Marchi wrote:
> On 2020-07-15 4:16 a.m., Tankut Baris Aktemur wrote:
> > From: Simon Marchi <simon.marchi@polymtl.ca>
> >
> > Remove the use of objfile_data to associate a jit_objfile_data with an
> > objfile. Instead, directly link to a jit_objfile_data from an objfile
> > struct. The goal is to eliminate unnecessary abstraction.
> >
> > The free_objfile_data function naturally becomes the destructor of
> > jit_objfile_data. However, free_objfile_data accesses the objfile to
> > which the data is attached, which the destructor of jit_objfile_data
> > doesn't have access to. To work around this, add a backlink to the
> > owning objfile in jit_objfile_data. This is however temporary, it goes
> > away in a subsequent patch.
> >
> > gdb/ChangeLog:
> > 2020-MM-DD Simon Marchi <simon.marchi@polymtl.ca>
> >
> > * objfiles.h (struct jit_objfile_data): Migrate to here from
> > jit.c; also add a constructor, destructor, and an objfile* field.
> > (struct objfile) <jit_data>: New field.
> > * jit.c (jit_objfile_data): Remove.
> > (struct jit_objfile_data): Migrate from here to objfiles.h.
> > (jit_objfile_data::~jit_objfile_data): New destructor
> > implementation with code moved from free_objfile_data.
> > (free_objfile_data): Delete.
> > (get_jit_objfile_data): Update to use the jit_data field of objfile.
> > (jit_find_objf_with_entry_addr): Ditto.
> > (jit_inferior_exit_hook): Ditto.
> > (_initialize_jit): Remove the call to
> > register_objfile_data_with_cleanup.
> > ---
> > gdb/jit.c | 89 +++++++++++++-------------------------------------
> > gdb/objfiles.h | 31 ++++++++++++++++++
> > 2 files changed, 54 insertions(+), 66 deletions(-)
> >
> > diff --git a/gdb/jit.c b/gdb/jit.c
> > index 41ed81ab4b0..af0c9299708 100644
> > --- a/gdb/jit.c
> > +++ b/gdb/jit.c
> > @@ -45,8 +45,6 @@
> >
> > static std::string jit_reader_dir;
> >
> > -static const struct objfile_data *jit_objfile_data;
> > -
> > static const char *const jit_break_name = "__jit_debug_register_code";
> >
> > static const char *const jit_descriptor_name = "__jit_debug_descriptor";
> > @@ -265,24 +263,25 @@ struct jit_program_space_data
> >
> > static program_space_key<jit_program_space_data> jit_program_space_key;
> >
> > -/* Per-objfile structure recording the addresses in the program space.
> > - This object serves two purposes: for ordinary objfiles, it may
> > - cache some symbols related to the JIT interface; and for
> > - JIT-created objfiles, it holds some information about the
> > - jit_code_entry. */
> > +/* Destructor for jit_objfile_data. */
> >
> > -struct jit_objfile_data
> > +jit_objfile_data::~jit_objfile_data ()
> > {
> > - /* Symbol for __jit_debug_register_code. */
> > - struct minimal_symbol *register_code;
> > -
> > - /* Symbol for __jit_debug_descriptor. */
> > - struct minimal_symbol *descriptor;
> > + /* Free the data allocated in the jit_program_space_data slot. */
> > + if (this->register_code != NULL)
> > + {
> > + struct jit_program_space_data *ps_data;
> >
> > - /* Address of struct jit_code_entry in this objfile. This is only
> > - non-zero for objfiles that represent code created by the JIT. */
> > - CORE_ADDR addr;
> > -};
> > + ps_data = jit_program_space_key.get (this->objfile->pspace);
> > + if (ps_data != NULL && ps_data->objfile == this->objfile)
> > + {
> > + ps_data->objfile = NULL;
> > + if (ps_data->jit_breakpoint != NULL)
> > + delete_breakpoint (ps_data->jit_breakpoint);
> > + ps_data->cached_code_address = 0;
> > + }
> > + }
> > +}
> >
> > /* Fetch the jit_objfile_data associated with OBJF. If no data exists
> > yet, make a new structure and attach it. */
> > @@ -290,16 +289,10 @@ struct jit_objfile_data
> > static struct jit_objfile_data *
> > get_jit_objfile_data (struct objfile *objf)
> > {
> > - struct jit_objfile_data *objf_data;
> > -
> > - objf_data = (struct jit_objfile_data *) objfile_data (objf, jit_objfile_data);
> > - if (objf_data == NULL)
> > - {
> > - objf_data = XCNEW (struct jit_objfile_data);
> > - set_objfile_data (objf, jit_objfile_data, objf_data);
> > - }
> > + if (objf->jit_data == nullptr)
> > + objf->jit_data.reset (new jit_objfile_data (objf));
> >
> > - return objf_data;
> > + return objf->jit_data.get ();
> > }
> >
> > /* Remember OBJFILE has been created for struct jit_code_entry located
> > @@ -914,14 +907,9 @@ static struct objfile *
> > jit_find_objf_with_entry_addr (CORE_ADDR entry_addr)
> > {
> > for (objfile *objf : current_program_space->objfiles ())
> > - {
> > - struct jit_objfile_data *objf_data;
> > + if (objf->jit_data != nullptr && objf->jit_data->addr == entry_addr)
> > + return objf;
>
> Keep the braces for the for here:
>
> for (objfile *objf : current_program_space->objfiles ())
> {
> if (objf->jit_data != nullptr && objf->jit_data->addr == entry_addr)
> return objf;
> }
>
> as described somewhere here:
>
> https://www.gnu.org/prep/standards/html_node/Syntactic-Conventions.html#Syntactic-
> Conventions
I thought this convention was specifically about nested if-statements.
Fixed in v4.
> >
> > - objf_data
> > - = (struct jit_objfile_data *) objfile_data (objf, jit_objfile_data);
> > - if (objf_data != NULL && objf_data->addr == entry_addr)
> > - return objf;
> > - }
> > return NULL;
> > }
> >
> > @@ -1324,13 +1312,8 @@ static void
> > jit_inferior_exit_hook (struct inferior *inf)
> > {
> > for (objfile *objf : current_program_space->objfiles_safe ())
> > - {
> > - struct jit_objfile_data *objf_data
> > - = (struct jit_objfile_data *) objfile_data (objf, jit_objfile_data);
> > -
> > - if (objf_data != NULL && objf_data->addr != 0)
> > - objf->unlink ();
> > - }
> > + if (objf->jit_data != nullptr && objf->jit_data->addr != 0)
> > + objf->unlink ();
>
> Same here.
Fixed in v4.
> > }
> >
> > void
> > @@ -1371,30 +1354,6 @@ jit_event_handler (gdbarch *gdbarch, objfile *jiter)
> > }
> > }
> >
> > -/* Called to free the data allocated to the jit_program_space_data slot. */
> > -
> > -static void
> > -free_objfile_data (struct objfile *objfile, void *data)
> > -{
> > - struct jit_objfile_data *objf_data = (struct jit_objfile_data *) data;
> > -
> > - if (objf_data->register_code != NULL)
> > - {
> > - struct jit_program_space_data *ps_data;
> > -
> > - ps_data = jit_program_space_key.get (objfile->pspace);
> > - if (ps_data != NULL && ps_data->objfile == objfile)
> > - {
> > - ps_data->objfile = NULL;
> > - if (ps_data->jit_breakpoint != NULL)
> > - delete_breakpoint (ps_data->jit_breakpoint);
> > - ps_data->cached_code_address = 0;
> > - }
> > - }
> > -
> > - xfree (data);
> > -}
> > -
> > /* Initialize the jit_gdbarch_data slot with an instance of struct
> > jit_gdbarch_data_type */
> >
> > @@ -1427,8 +1386,6 @@ _initialize_jit ()
> > gdb::observers::inferior_exit.attach (jit_inferior_exit_hook);
> > gdb::observers::breakpoint_deleted.attach (jit_breakpoint_deleted);
> >
> > - jit_objfile_data =
> > - register_objfile_data_with_cleanup (NULL, free_objfile_data);
> > jit_gdbarch_data = gdbarch_data_register_pre_init (jit_gdbarch_data_init);
> > if (is_dl_available ())
> > {
> > diff --git a/gdb/objfiles.h b/gdb/objfiles.h
> > index 56ff52119dc..6bb4dd0a1d6 100644
> > --- a/gdb/objfiles.h
> > +++ b/gdb/objfiles.h
> > @@ -407,6 +407,34 @@ class separate_debug_range
> > struct objfile *m_objfile;
> > };
> >
> > +/* Per-objfile structure recording the addresses in the program space.
> > + This object serves two purposes: for ordinary objfiles, it may
> > + cache some symbols related to the JIT interface; and for
> > + JIT-created objfiles, it holds some information about the
> > + jit_code_entry. */
> > +
> > +struct jit_objfile_data
> > +{
> > + jit_objfile_data (struct objfile *objfile)
> > + : objfile (objfile)
> > + {}
> > +
> > + ~jit_objfile_data ();
> > +
> > + /* Back-link to the objfile. */
> > + struct objfile *objfile;
> > +
> > + /* Symbol for __jit_debug_register_code. */
> > + minimal_symbol *register_code = nullptr;
> > +
> > + /* Symbol for __jit_debug_descriptor. */
> > + minimal_symbol *descriptor = nullptr;
> > +
> > + /* Address of struct jit_code_entry in this objfile. This is only
> > + non-zero for objfiles that represent code created by the JIT. */
> > + CORE_ADDR addr = 0;
> > +};
>
> This could reside in jit.h, if you don't include `objfiles.h` in `jit.h`
> (which I don't think you need).
I moved this to `jit.h` in v4. Please note that `objfiles.h` will now include
`jit.h` because it needs to have access to `jit_objfile_data` due to the
new unique_ptr field in objfile struct. Since objfile is much of a central
concept and a container of JIT data, this seemed reasonable to me.
> I'm wondering if the JIT data now needs to be explicitly reset when the objfile's
> symbols get re-read (reread_symbols), or when re-running and the base address
> of the objfile changes. "free_objfile_data" might have been automatically invoked
> in either or both of these cases.
>
> A good test would be, with `set disable-randomization off`, try running a JITer program
> twice. On the second run, will the `jit_objfile_data` still hold addresses of the first
> run, which are not good anymore?
>
> And as a follow up test, try rebuilding the JITer program in between the two runs (witout
> restarting GDB) so that addresses of __jit_debug_register_code and __jit_debug_descriptor.
> GDB should call reread_symbols, does it all work as expected?
I did these tests manually. Here is the status.
If the JITer is a linked solib file, the `target_pre_inferior` indirectly causes the
objfile to be unlinked and thus the `jit_objfile_data` associated with it to be
destructed. So, for both scenarios, right before the second run, the JITer objfile is
initialized from fresh.
If the JITer is the main objfile, it is not unlinked. So, its `jit_objfile_data` is
not destroyed. For the second scenario (i.e. reread_symbols), the `objfiles_changed`
function is invoked. It might be a proper spot to reset the JIT data. For the first
scenario, I'm not sure what the best spot for a reset would be.
However, I don't think a reset is needed at all. The `jit_breakpoint_re_set_internal`
function is invoked each time an objfile is loaded. This means, at the beginning of
a run and later when a JITer or a non-JITer solib file is loaded, the symbols will be
looked up again and the addresses will be overwritten. Or am I missing something?
I'll post v4 after this is clarified.
Thanks
-Baris
Intel Deutschland GmbH
Registered Address: Am Campeon 10-12, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de
Managing Directors: Christin Eisenschmid, Gary Kershaw
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928
More information about the Gdb-patches
mailing list