Re: Ping: [RFA] Add support of shared lib for Darwin

[Joel Brobecker <brobecker@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