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

[Tristan Gingold <gingold@adacore.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.