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

[Tristan Gingold <gingold at adacore dot com>]

On Feb 3, 2009, at 3:04 AM, Joel Brobecker wrote:

> No review of this patch so far, so I took a look :)

Thank you for reviewing.

> > > +/* 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)?

I will add the assertion.

> > > +  len = 4 + 4 + 2 * ptr_type->length;
>
> Can you explain the computation using little comments, maybe?

Ok.

> > > +/* 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.

Ok.

> > > +/* 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?

Ok, I changed it to 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?

I think this case won't happen once pid_to_exec_file is implemented  
(and I have an implementation for this
target op).

> > > +/* 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?

Yes.

> > > +  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.

Unfortunately this doesn't look very easy.  This method is not  
documented and I had to read the sources
of the dynamic loader to find it.  I really prefer to postpone the  
implementation.

> > > +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.

Oops, this is a left over previous cleanups.

I plan to re-submit the patch soon.

Tristan.