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

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

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

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

Today I took Xavier's patch and instrumented it with some printfs.

What happens here is that at startup, darwin_solib_create_inferior_hook
clears the notifier_set flag:

+              notifier += dyld_relocated_base_address;
+              info->notifier_set = false;

... but it still creates an solib event breakpoint.

This later causes darwin_handle_solib_event to be called.

In my case the address of the solib event breakpoint is at the same
location in both cases.  But, there is also this comment:

+	 (Apparently dyld doesn't need to relocate itself on x86-64 darwin,
+	 but don't assume that).

What this says to me is that perhaps there is an architecture where
darwin_handle_solib_event computes a different solib breakpoint address.

My conclusion is that the patch is generally ok (certainly it works)
and, while this one part is unusual, it isn't fatally so.

>> +  /* True when a breakpoint has been added on dyld notifier.  */
>> +  unsigned int notifier_set;

Simon> Use bool?

I did this.

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

I don't think this is quite accurate; and the existing comments were
enough for me.

But there's one case where it sets notifier=0 -- this is the one I don't
understand.

>> +static gdb_bfd_ref_ptr
>> +darwin_get_dyld_bfd (void)

Simon> The comment above the function would need to be updated.

I did it.

>> {
>> 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;

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

I didn't really understand this choice either, but all the calls to
darwin_get_dyld_bfd ultimately come from
darwin_solib_create_inferior_hook anyway, so maybe it is intentional.

>> +  CORE_ADDR notifier;

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

I made this change too.

Tom

commit 71b7becde98dd066de4e1e3a5142233c4dd16581
Author: Xavier Roirand <roirand@adacore.com>
Date:   Wed Aug 22 12:11:14 2018 +0200

    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

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

Commit Message

Comments

Patch