This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: Ping: [RFA] Add support of shared lib for Darwin
- From: Joel Brobecker <brobecker at adacore dot com>
- To: Tristan Gingold <gingold at adacore dot com>
- Cc: gdb-patches at sourceware dot org
- Date: Mon, 2 Feb 2009 18:04:01 -0800
- Subject: Re: Ping: [RFA] Add support of shared lib for Darwin
- References: <20090108140918.GA70183@ulanbator.act-europe.fr> <2BC5ADA3-B225-4760-822B-C8E31FD999A2@adacore.com>
No review of this patch so far, so I took a look :)
> >2009-01-08 Tristan Gingold <gingold@adacore.com>
> >
> > * machoread.c (macho_symfile_read): Read minsymtab also from
> > shared libraries.
> > (macho_symfile_read): Try to read dwarf2 frame info from main
> > object file, but not from OSO files.
> > (macho_symfile_offsets): Update section names for latest BFD
> > changes.
> > * i386-darwin-tdep.c (i386_darwin_init_abi): Call set_solib_ops.
> > (x86_darwin_init_abi_64): Ditto.
> > * configure.tgt: Add solib.o solib-darwin.o for Darwin.
Generally speaking, this looks fine. I have a few minor comments...
> >+/* Read dyld_all_image from inferior. */
> >+static void
> >+darwin_load_image_infos (void)
> >+{
> >+ gdb_byte buf[24];
I'm always nervous when I see hard-coded constants like this in
buffer declarations? Would it make sense to use alloca? Or maybe
add an assertion that len <= sizeof (buf)?
> >+ len = 4 + 4 + 2 * ptr_type->length;
Can you explain the computation using little comments, maybe?
> >+/* Return non-zero if GDB_SO_NAME and INFERIOR_SO_NAME represent
> >+ the same shared library. */
> >+
> >+static int
> >+darwin_same (struct so_list *gdb, struct so_list *inferior)
> >+{
> >+ return strcmp (gdb->so_original_name, inferior->so_original_name)
> >== 0;
> >+}
I think that this function is not necessary. if so_ops.same is set to NULL,
then GDB falls back to using strcmp like you did... Perhaps we could add
a comment about that in solist.h, in fact.
> >+/* Lookup the value for a specific symbol. */
of?
> >+static CORE_ADDR
> >+bfd_lookup_symbol (bfd *abfd, char *symname)
The name of this function annoys me a little. With GDB's current
conventions, it seems to suggest that this function is part of bfd.
Can we call is darwin_lookup_symbol or darwin_lookup_symbol_from_bfd?
> >+/* Return program interpreter string. */
> >+static gdb_byte *
> >+find_program_interpreter (void)
> >+{
[...]
> >+ /* If we didn't find it, read from memory.
> >+ FIXME: todo. */
Would it be complicated to do this now? I'm OK with looking at this
later, if you think it's easier. I suppose this only really matter
in the "attach" case, right?
> >+/* Build a list of currently loaded shared objects. See solib-
> >svr4.c */
> >+static struct so_list *
> >+darwin_current_sos (void)
> >+{
[...]
> >+ /* Read infos for each solib. */
> >+ for (i = 0; i < dyld_all_image.count; i++)
> >+ {
> >+ CORE_ADDR info = dyld_all_image.info + i * image_info_size;
> >+ char buf[image_info_size];
> >+ CORE_ADDR load_addr;
[...]
> >+
> >+ /* Read image info from inferior. */
> >+ if (target_read_memory (info, buf, image_info_size))
> >+ break;
> >+
> >+ load_addr = extract_typed_address (buf, ptr_type);
> >+ path_addr = extract_typed_address (buf + ptr_len, ptr_type);
> >+
> >+ target_read_string (path_addr, &file_path,
> >+ SO_NAME_MAX_PATH_SIZE - 1, &errcode);
> >+ if (errcode)
> >+ break;
> >+
> >+ /* Ignore first entry as this is the executable itself. */
> >+ if (i == 0)
> >+ continue;
Is there a reason for reading the info about the first entry at all?
Can we for instance start the loop with i = 1?
> >+ if (!inf->attach_flag)
> >+ {
> >+ /* We find the dynamic linker's base address by examining
> >+ the current pc (which should point at the entry point for the
> >+ dynamic linker) and subtracting the offset of the entry point. */
> >+ load_addr = (read_pc () - bfd_get_start_address (dyld_bfd));
> >+ }
> >+ else
> >+ {
> >+ /* FIXME: todo.
> >+ Get address of __DATA.__dyld in exec_bfd, read address at offset 0
> >+ */
> >+ xfree (interp_name);
> >+ return;
> >+ }
Can we implement this part as well? Same remark as above. OK to
push to a separate patch if it helps, but might as well if it's easy.
> >+extern initialize_file_ftype _initialize_svr4_solib; /* -Wmissing-
> >prototypes */
Looks like an unused declaration. Unwanted copy/paste? The
corresponding advance prototype for _initialize_darwin_solib
really isn't necessary - I think. We have lots of files that
don't provide this advance declaration.
--
Joel