[RFA,2/5] Darwin: Handle unrelocated dyld.

  On Darwin, debugging an helloworld program with GDB does
not work and ends with:

  (gdb) set startup-with-shell off
  (gdb) start
  Temporary breakpoint 1 at 0x100000fb4: file /tmp/helloworld.c, line 1.
  Starting program: /private/tmp/helloworld
  [New Thread 0x2703 of process 18906]
  [New Thread 0x2603 of process 18906]

  [1]+  Stopped                 ./gdb/gdb /tmp/helloworld

When debugging with lldb, instead of having the STOP signal, we can
see that a breakpoint is not set to a proper location:

  Warning:
  Cannot insert breakpoint -1.
  Cannot access memory at address 0xf726

  Command aborted.

The inserted breakpoint is the one used when GDB has to stop the target
when a shared library is loaded or unloaded. The notifier address used
for adding the breakpoint is wrong thus the above failure.
This notifier address is an offset relative to dyld base address, so
the value calculation has to be updated to reflect this.
This patch fix this.

gdb/ChangeLog:

        * solib-darwin.c (struct darwin_info): Add notifier_set field.
        (darwin_get_dyld_bfd): New function.
        (darwin_solib_get_all_image_info_addr_at_init): Update call.
        (darwin_handle_solib_event): New function.
        (darwin_solib_create_inferior_hook): Handle unrelocated dyld.
        (_initialize_darwin_solib): Add solib_event.

Change-Id: I7dde5008c9158f17b78dc89bd7f4bd8a12d4a6e1
---
 gdb/solib-darwin.c | 111 ++++++++++++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 102 insertions(+), 9 deletions(-)
  

On 2018-08-22 06:11, Xavier Roirand wrote:
> On Darwin, debugging an helloworld program with GDB does
> not work and ends with:
> 
>   (gdb) set startup-with-shell off
>   (gdb) start
>   Temporary breakpoint 1 at 0x100000fb4: file /tmp/helloworld.c, line 
> 1.
>   Starting program: /private/tmp/helloworld
>   [New Thread 0x2703 of process 18906]
>   [New Thread 0x2603 of process 18906]
> 
>   [1]+  Stopped                 ./gdb/gdb /tmp/helloworld
> 
> When debugging with lldb, instead of having the STOP signal, we can
> see that a breakpoint is not set to a proper location:
> 
>   Warning:
>   Cannot insert breakpoint -1.
>   Cannot access memory at address 0xf726
> 
>   Command aborted.
> 
> The inserted breakpoint is the one used when GDB has to stop the target
> when a shared library is loaded or unloaded. The notifier address used
> for adding the breakpoint is wrong thus the above failure.
> This notifier address is an offset relative to dyld base address, so
> the value calculation has to be updated to reflect this.
> This patch fix this.

We just got a Mac at work, so I started to toy with GDB on it yesterday, 
this is one of the first problem I got.  I also had the intuition that 
the address of that breakpoint was wrong, so I patched GDB to add a 
fixed 0x100000000, and that made it work.

With this patch, there seems to be two way the notifier breakpoint can 
be set: in darwin_solib_create_inferior_hook and in 
darwin_handle_solib_event.  Can you explain why do we need both?

In particular, I don't understand when the one in 
darwin_handle_solib_event can be useful.  There are two ways 
darwin_handle_solib_event can be called:

  - By bpstat_stop_status in breakpoint.c.  For this to happen, we need 
to hit a breakpoint of type bp_shlib_event. If we have such a 
breakpoint, it means we already have set our notifier breakpoint, so why 
would we need to set it again?
  - By handle_inferior_event_1 if ecs->ws.kind is TARGET_WAITKIND_LOADED. 
  darwin-nat.c never seems to report this stop kind.

> diff --git a/gdb/solib-darwin.c b/gdb/solib-darwin.c
> index ed8e0c1..aad3c44 100644
> --- a/gdb/solib-darwin.c
> +++ b/gdb/solib-darwin.c
> @@ -77,6 +77,9 @@ struct darwin_info
> 
>    /* Gdb copy of dyld_all_info_infos.  */
>    struct gdb_dyld_all_image_infos all_image;
> +
> +  /* True when a breakpoint has been added on dyld notifier.  */
> +  unsigned int notifier_set;

Use bool?

Also, from what I understand from the rest of the patch (I might be 
wrong), this is also set if we have tried to add it but failed (because 
we failed to get info about the dynamic loader, for example).  If this 
is right, can you update the comment to reflect this?

>  };
> 
>  /* Per-program-space data key.  */
> @@ -432,20 +435,19 @@ gdb_bfd_mach_o_fat_extract (bfd *abfd, bfd_format 
> format,
>  /* Extract dyld_all_image_addr when the process was just created, 
> assuming the
>     current PC is at the entry of the dynamic linker.  */
> 
> -static void
> -darwin_solib_get_all_image_info_addr_at_init (struct darwin_info 
> *info)
> +static gdb_bfd_ref_ptr
> +darwin_get_dyld_bfd (void)

The comment above the function would need to be updated.

>  {
>    char *interp_name;
> -  CORE_ADDR load_addr = 0;
> 
>    /* This method doesn't work with an attached process.  */
>    if (current_inferior ()->attach_flag)
> -    return;
> +    return NULL;

Should this last snippet (check for attach_flag) be here or in 
darwin_solib_get_all_image_info_addr_at_init?  From what I understand, 
there is nothing preventing darwin_get_dyld_bfd to work when working 
with an attached process.

> 
>    /* Find the program interpreter.  */
>    interp_name = find_program_interpreter ();
>    if (!interp_name)
> -    return;
> +    return NULL;
> 
>    /* Create a bfd for the interpreter.  */
>    gdb_bfd_ref_ptr dyld_bfd (gdb_bfd_open (interp_name, gnutarget, 
> -1));
> @@ -459,6 +461,18 @@ darwin_solib_get_all_image_info_addr_at_init
> (struct darwin_info *info)
>        else
>  	dyld_bfd.release ();
>      }
> +  return dyld_bfd;
> +}
> +
> +/* Extract dyld_all_image_addr when the process was just created, 
> assuming the
> +   current PC is at the entry of the dynamic linker.  */
> +
> +static void
> +darwin_solib_get_all_image_info_addr_at_init (struct darwin_info 
> *info)
> +{
> +  CORE_ADDR load_addr = 0;
> +  gdb_bfd_ref_ptr dyld_bfd (darwin_get_dyld_bfd ());
> +
>    if (dyld_bfd == NULL)
>      return;
> 
> @@ -502,6 +516,37 @@ darwin_solib_read_all_image_info_addr (struct
> darwin_info *info)
>    info->all_image_addr = extract_unsigned_integer (buf, len, 
> BFD_ENDIAN_BIG);
>  }
> 
> +/* Callback called on solib event.
> +   Used when a breakpoint has to be added on the entry point, to wait 
> for
> +   dyld initialization.  */
> +
> +static void
> +darwin_handle_solib_event (void)
> +{
> +  struct darwin_info *info = get_darwin_info ();
> +
> +  /* Nothing to do if notifier was already set.  */
> +  if (info->notifier_set)
> +    return;
> +  info->notifier_set = 1;
> +
> +  /* Remove breakpoint on entry, but not now (as our caller is 
> iterating on
> +     breakpoints).  */
> +  remove_solib_event_breakpoints_at_next_stop ();
> +
> +  /* Reload dyld infos.  */
> +  darwin_load_image_infos (info);
> +
> +  if (info->all_image.count == 0)
> +    {
> +      /* Not expected: no dylibs.  */
> +      return;
> +    }
> +
> +  /* Insert the real solib event on the dyld notifier.  */
> +  create_solib_event_breakpoint (target_gdbarch (), 
> info->all_image.notifier);
> +}
> +
>  /* Shared library startup support.  See documentation in solib-svr4.c. 
>  */
> 
>  static void
> @@ -509,6 +554,7 @@ darwin_solib_create_inferior_hook (int from_tty)
>  {
>    struct darwin_info *info = get_darwin_info ();
>    CORE_ADDR load_addr;
> +  CORE_ADDR notifier;

Style nit: you can declare variables where you are using them the first 
time.

Simon
  

On 2018-08-22 06:11, Xavier Roirand wrote:
> @@ -558,6 +600,56 @@ darwin_solib_create_inferior_hook (int from_tty)
>        if (vmaddr != load_addr)
>  	objfile_rebase (symfile_objfile, load_addr - vmaddr);
>      }
> +
> +  /* Set solib notifier (to reload list of shared libraries).  */
> +  notifier = info->all_image.notifier;
> +  info->notifier_set = 1;
> +
> +  if (info->all_image.count == 0)
> +    {
> +      CORE_ADDR start;
> +
> +      /* Dyld hasn't yet relocated itself, so the notifier address may 
> be
> +        incorrect (as it has to be relocated).
> +        (Apparently dyld doesn't need to relocate itself on x86-64 
> darwin,
> +        but don't assume that).
> +        Set an event breakpoint at the entry point.  */
> +      start = bfd_get_start_address (exec_bfd);
> +      if (start == 0)
> +       notifier = 0;
> +      else
> +        {
> +          gdb_bfd_ref_ptr dyld_bfd (darwin_get_dyld_bfd ());
> +          if (dyld_bfd != NULL)
> +            {
> +              CORE_ADDR dyld_bfd_start_address;
> +              CORE_ADDR dyld_relocated_base_address;
> +              CORE_ADDR pc;
> +
> +              dyld_bfd_start_address = bfd_get_start_address 
> (dyld_bfd.get());
> +
> +              /* 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.  */
> +
> +              pc = regcache_read_pc (get_current_regcache ());
> +              dyld_relocated_base_address = pc - 
> dyld_bfd_start_address;
> +
> +              /* We get the proper notifier relocated address by
> +                 adding the dyld relocated base address to the current
> +                 notifier offset value.  */
> +
> +              notifier += dyld_relocated_base_address;
> +              info->notifier_set = 0;
> +            }
> +        }
> +    }
> +
> +  /* Add the breakpoint which is hit by dyld when the list of solib is
> +     modified.  */
> +  if (notifier != 0)
> +    create_solib_event_breakpoint (target_gdbarch (), notifier);
>  }

Also, if the process of finding the notifier address fails at any point, 
could we display a warning?

Simon
  

>>>>> "Simon" == Simon Marchi <simon.marchi@polymtl.ca> writes:

>> +      if (start == 0)
>> +       notifier = 0;
[...]
>> +  /* Add the breakpoint which is hit by dyld when the list of solib is
>> +     modified.  */
>> +  if (notifier != 0)
>> +    create_solib_event_breakpoint (target_gdbarch (), notifier);
>> }

Simon> Also, if the process of finding the notifier address fails at any
Simon> point, could we display a warning?

This is the one spot -- but I didn't know what to put in the comment,
because I don't know the circumstances under which start==0 can happen.

Tom