[RFA 0/5] Fix some bugs on macOS

[RFA 0/5] Fix some bugs on macOS

[Xavier Roirand]

Hello,

I'm sending a couple of patches which make GDB working better on Sierra and later.

Xavier

Xavier Roirand (5):
  Darwin: fix bad loop incrementation
  Darwin: Handle unrelocated dyld.
  Darwin: set startup-with-shell to off on Sierra and later.
  Darwin: fix thread ptid started by fork_inferior
  Darwin: fix SIGTRAP when debugging

 gdb/darwin-nat.c   |  54 ++++++++++++++++++++++++--
 gdb/machoread.c    |   2 +-
 gdb/solib-darwin.c | 111 ++++++++++++++++++++++++++++++++++++++++++++++++-----
 3 files changed, 154 insertions(+), 13 deletions(-)

-- 
2.7.4

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

[Xavier Roirand]

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(-)

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;
 };
 
 /* 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)
 {
   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;
 
   /* 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;
 
   info->all_image_addr = 0;
 
@@ -528,10 +574,6 @@ darwin_solib_create_inferior_hook (int from_tty)
       return;
     }
 
-  /* Add the breakpoint which is hit by dyld when the list of solib is
-     modified.  */
-  create_solib_event_breakpoint (target_gdbarch (), info->all_image.notifier);
-
   if (info->all_image.count != 0)
     {
       /* Possible relocate the main executable (PIE).  */
@@ -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);
 }
 
 static void
@@ -658,4 +750,5 @@ _initialize_darwin_solib (void)
   darwin_so_ops.in_dynsym_resolve_code = darwin_in_dynsym_resolve_code;
   darwin_so_ops.lookup_lib_global_symbol = darwin_lookup_lib_symbol;
   darwin_so_ops.bfd_open = darwin_bfd_open;
+  darwin_so_ops.handle_event = darwin_handle_solib_event;
 }
-- 
2.7.4

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

[Simon Marchi]

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

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

[Tom Tromey]

>>>>> "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

diff --git a/gdb/solib-darwin.c b/gdb/solib-darwin.c
index ed8e0c13365..2633dc04d43 100644
--- a/gdb/solib-darwin.c
+++ b/gdb/solib-darwin.c
@@ -77,6 +77,10 @@ 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, or when
+     an attempt to do so has been made and failed.  */
+  bool notifier_set;
 };
 
 /* Per-program-space data key.  */
@@ -429,23 +433,21 @@ gdb_bfd_mach_o_fat_extract (bfd *abfd, bfd_format format,
   return gdb_bfd_ref_ptr (result);
 }
 
-/* Extract dyld_all_image_addr when the process was just created, assuming the
-   current PC is at the entry of the dynamic linker.  */
+/* Return the BFD for the program interpreter.  */
 
-static void
-darwin_solib_get_all_image_info_addr_at_init (struct darwin_info *info)
+static gdb_bfd_ref_ptr
+darwin_get_dyld_bfd ()
 {
   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;
 
   /* 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 = true;
+
+  /* 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
@@ -528,10 +573,6 @@ darwin_solib_create_inferior_hook (int from_tty)
       return;
     }
 
-  /* Add the breakpoint which is hit by dyld when the list of solib is
-     modified.  */
-  create_solib_event_breakpoint (target_gdbarch (), info->all_image.notifier);
-
   if (info->all_image.count != 0)
     {
       /* Possible relocate the main executable (PIE).  */
@@ -558,6 +599,54 @@ 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).  */
+  CORE_ADDR notifier = info->all_image.notifier;
+  info->notifier_set = true;
+
+  if (info->all_image.count == 0)
+    {
+      /* 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.  */
+      CORE_ADDR 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 = false;
+            }
+        }
+    }
+
+  /* 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);
 }
 
 static void
@@ -658,4 +747,5 @@ _initialize_darwin_solib (void)
   darwin_so_ops.in_dynsym_resolve_code = darwin_in_dynsym_resolve_code;
   darwin_so_ops.lookup_lib_global_symbol = darwin_lookup_lib_symbol;
   darwin_so_ops.bfd_open = darwin_bfd_open;
+  darwin_so_ops.handle_event = darwin_handle_solib_event;
 }

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

[Joel Brobecker]

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

I am wondering whether the difference in what you are seeing
might be explained by a difference in MacOS X version; if I were
to guess, I would say that Xavier was running on Mac OS X Sierra.
What version were you running on?

Or perhaps the intent is to be extra careful meaning that while
today the relocation is not necessary, we still handle it so that
it continues working the day it becomes so?

If the comment above is confusing, I would vote for removing it.
To me, this is like Windows, where DLLs have prefered base addresses
where they get loaded, but we still need to do the reloc just in case,
because the loader may have to load it elsewhere. So what this is
doing here is somewhat "classic".

That makes me realize (again) that, for MacOS X, we should be more
proactive at specificying which version a patch we are submitting
was tested on, and some information about which versions of MacOS X
a given patch helps. A fair amount of work that Tristan did once
the initial port was created was to adapt it to subsequent versions
of Darwin. Nearly every new version of Darwin introduced its new
set of changes requiring additional adaptations.


-- 
Joel

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

[Simon Marchi]

On 2018-09-19 09:40, Joel Brobecker wrote:
> That makes me realize (again) that, for MacOS X, we should be more
> proactive at specificying which version a patch we are submitting
> was tested on, and some information about which versions of MacOS X
> a given patch helps. A fair amount of work that Tristan did once
> the initial port was created was to adapt it to subsequent versions
> of Darwin. Nearly every new version of Darwin introduced its new
> set of changes requiring additional adaptations.

Given the low amount of resources we have to work on GDB for macOS and 
the fact that it's difficult for one person to test on different 
versions of macOS (you would need multiple Mac computers, AFAIK), I 
suggest that we only aim at supporting the last two released versions of 
macOS.  I say two, because there will always be an overlap after a new 
version is released, where some people use the new version and others 
have not upgraded yet.

If some people use older versions of macOS and want to contribute 
patches and help testing, that's fine too, but if everybody contributing 
is on latest or latest-1, we can't realistically ensure it works on 
previous versions.

That said, I agree that it would be very helpful if commit messages and 
comments in the code mentioned the version of macOS that was used at the 
time that commit message or comment was written.

Simon

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

[Joel Brobecker]

> Given the low amount of resources we have to work on GDB for macOS and the
> fact that it's difficult for one person to test on different versions of
> macOS (you would need multiple Mac computers, AFAIK), I suggest that we only
> aim at supporting the last two released versions of macOS.  I say two,
> because there will always be an overlap after a new version is released,
> where some people use the new version and others have not upgraded yet.

That makes a lot of sense to me.

If we agree, let's document that somewhere. I'm not sure where
the best place might be. Maybe this wiki page?

        https://sourceware.org/gdb/wiki/BuildingOnDarwin

> If some people use older versions of macOS and want to contribute patches
> and help testing, that's fine too, but if everybody contributing is on
> latest or latest-1, we can't realistically ensure it works on previous
> versions.
> 
> That said, I agree that it would be very helpful if commit messages and
> comments in the code mentioned the version of macOS that was used at the
> time that commit message or comment was written.

-- 
Joel

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

[Tom Tromey]

>>>>> "Joel" == Joel Brobecker <brobecker@adacore.com> writes:

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

Joel> I am wondering whether the difference in what you are seeing
Joel> might be explained by a difference in MacOS X version; if I were
Joel> to guess, I would say that Xavier was running on Mac OS X Sierra.
Joel> What version were you running on?

High Sierra.

Joel> Or perhaps the intent is to be extra careful meaning that while
Joel> today the relocation is not necessary, we still handle it so that
Joel> it continues working the day it becomes so?

I couldn't really say, I just concluded what I did based on the mention
of the specific architecture there.

Joel> If the comment above is confusing, I would vote for removing it.

I think we need more information.

If we remove the comment because we think it is untrue, then Simon's
original critique of the patch -- that there is no reason for a second
solib breakpoint -- seems correct.  And in this case we should remove a
chunk of the patch.

Based on my testing thus far, this would be fine.  But I don't know what
testing Xavier and Tristan did, or with what architectures.

Also my testing hasn't been exactly exhaustive.  I just try simple
things because today those break.

Now, for my purposes, it would be fine to land the more minimal patch.
That would not preclude adding this code back later.  If that seems ok,
I'm happy to do it.  I'd like to get this working so I can make some
progress on other patches, and anyway un-break the Mac port.

Joel> That makes me realize (again) that, for MacOS X, we should be more
Joel> proactive at specificying which version a patch we are submitting
Joel> was tested on, and some information about which versions of MacOS X
Joel> a given patch helps. A fair amount of work that Tristan did once
Joel> the initial port was created was to adapt it to subsequent versions
Joel> of Darwin. Nearly every new version of Darwin introduced its new
Joel> set of changes requiring additional adaptations.

Wholly agreed.

Tom

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

[Simon Marchi]

On 2018-09-19 10:36, Tom Tromey wrote:
>>>>>> "Joel" == Joel Brobecker <brobecker@adacore.com> writes:
> 
>>> +	 (Apparently dyld doesn't need to relocate itself on x86-64 darwin,
>>> +	 but don't assume that).
> 
> Joel> I am wondering whether the difference in what you are seeing
> Joel> might be explained by a difference in MacOS X version; if I were
> Joel> to guess, I would say that Xavier was running on Mac OS X Sierra.
> Joel> What version were you running on?
> 
> High Sierra.
> 
> Joel> Or perhaps the intent is to be extra careful meaning that while
> Joel> today the relocation is not necessary, we still handle it so that
> Joel> it continues working the day it becomes so?
> 
> I couldn't really say, I just concluded what I did based on the mention
> of the specific architecture there.
> 
> Joel> If the comment above is confusing, I would vote for removing it.
> 
> I think we need more information.
> 
> If we remove the comment because we think it is untrue, then Simon's
> original critique of the patch -- that there is no reason for a second
> solib breakpoint -- seems correct.  And in this case we should remove a
> chunk of the patch.
> 
> Based on my testing thus far, this would be fine.  But I don't know 
> what
> testing Xavier and Tristan did, or with what architectures.
> 
> Also my testing hasn't been exactly exhaustive.  I just try simple
> things because today those break.
> 
> Now, for my purposes, it would be fine to land the more minimal patch.
> That would not preclude adding this code back later.  If that seems ok,
> I'm happy to do it.  I'd like to get this working so I can make some
> progress on other patches, and anyway un-break the Mac port.
> 
> Joel> That makes me realize (again) that, for MacOS X, we should be 
> more
> Joel> proactive at specificying which version a patch we are submitting
> Joel> was tested on, and some information about which versions of MacOS 
> X
> Joel> a given patch helps. A fair amount of work that Tristan did once
> Joel> the initial port was created was to adapt it to subsequent 
> versions
> Joel> of Darwin. Nearly every new version of Darwin introduced its new
> Joel> set of changes requiring additional adaptations.
> 
> Wholly agreed.
> 
> Tom

I would vote for only checking in the code you know is necessary for 
now, otherwise it will just be more confusing in the future, trying to 
figure out what is needed and what isn't.

Simon

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

[Joel Brobecker]

> I would vote for only checking in the code you know is necessary for now,
> otherwise it will just be more confusing in the future, trying to figure out
> what is needed and what isn't.

I agree.

-- 
Joel

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

[Tom Tromey]

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

Simon> I would vote for only checking in the code you know is necessary for
Simon> now, otherwise it will just be more confusing in the future, trying to
Simon> figure out what is needed and what isn't.

Here is a more minimal version of the patch.  This one seems to work for
me on High Sierra.  I tried running a "hello world" program -- this
previously failed, but now works.  It's good enough that I could run
gdb.cp/*.exp -- lots of fails but no crashes or mystery problems.

Tom

commit 114a1aae792443d72f1438dbc979b42a39c5b780
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 was tested on High Sierra by trying to run a simple "hello world"
    program.
    
    gdb/ChangeLog:
    
            * solib-darwin.c (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.
    
    Change-Id: I7dde5008c9158f17b78dc89bd7f4bd8a12d4a6e1

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 328d48eeeb9..804aaf78e91 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,9 @@
+2018-09-18  Xavier Roirand  <roirand@adacore.com>
+
+          * solib-darwin.c (darwin_get_dyld_bfd): New function.
+          (darwin_solib_get_all_image_info_addr_at_init): Update call.
+          (darwin_solib_create_inferior_hook): Handle unrelocated dyld.
+
 2018-09-18  Tom Tromey  <tom@tromey.com>
 
 	* compile/compile-object-load.c (struct
diff --git a/gdb/solib-darwin.c b/gdb/solib-darwin.c
index ed8e0c13365..1877ec0839d 100644
--- a/gdb/solib-darwin.c
+++ b/gdb/solib-darwin.c
@@ -429,23 +429,21 @@ gdb_bfd_mach_o_fat_extract (bfd *abfd, bfd_format format,
   return gdb_bfd_ref_ptr (result);
 }
 
-/* Extract dyld_all_image_addr when the process was just created, assuming the
-   current PC is at the entry of the dynamic linker.  */
+/* Return the BFD for the program interpreter.  */
 
-static void
-darwin_solib_get_all_image_info_addr_at_init (struct darwin_info *info)
+static gdb_bfd_ref_ptr
+darwin_get_dyld_bfd ()
 {
   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;
 
   /* 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 +457,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;
 
@@ -528,10 +538,6 @@ darwin_solib_create_inferior_hook (int from_tty)
       return;
     }
 
-  /* Add the breakpoint which is hit by dyld when the list of solib is
-     modified.  */
-  create_solib_event_breakpoint (target_gdbarch (), info->all_image.notifier);
-
   if (info->all_image.count != 0)
     {
       /* Possible relocate the main executable (PIE).  */
@@ -558,6 +564,49 @@ 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).  */
+  CORE_ADDR notifier = info->all_image.notifier;
+
+  if (info->all_image.count == 0)
+    {
+      /* Dyld hasn't yet relocated itself, so the notifier address may
+	 be incorrect (as it has to be relocated).  */
+      CORE_ADDR 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;
+            }
+        }
+    }
+
+  /* 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);
 }
 
 static void

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

[Simon Marchi]

On 2018-09-19 15:15, Tom Tromey wrote:
>>>>>> "Simon" == Simon Marchi <simon.marchi@polymtl.ca> writes:
> 
> Simon> I would vote for only checking in the code you know is necessary 
> for
> Simon> now, otherwise it will just be more confusing in the future, 
> trying to
> Simon> figure out what is needed and what isn't.
> 
> Here is a more minimal version of the patch.  This one seems to work 
> for
> me on High Sierra.  I tried running a "hello world" program -- this
> previously failed, but now works.  It's good enough that I could run
> gdb.cp/*.exp -- lots of fails but no crashes or mystery problems.
> 
> Tom
> 
> commit 114a1aae792443d72f1438dbc979b42a39c5b780
> 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 was tested on High Sierra by trying to run a simple "hello 
> world"
>     program.

Works for me, thanks!  I just noted some nits.

> @@ -459,6 +457,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 ());

Use =.

> +
>    if (dyld_bfd == NULL)
>      return;
> 
> @@ -528,10 +538,6 @@ darwin_solib_create_inferior_hook (int from_tty)
>        return;
>      }
> 
> -  /* Add the breakpoint which is hit by dyld when the list of solib is
> -     modified.  */
> -  create_solib_event_breakpoint (target_gdbarch (), 
> info->all_image.notifier);
> -
>    if (info->all_image.count != 0)
>      {
>        /* Possible relocate the main executable (PIE).  */
> @@ -558,6 +564,49 @@ 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).  */
> +  CORE_ADDR notifier = info->all_image.notifier;
> +
> +  if (info->all_image.count == 0)
> +    {
> +      /* Dyld hasn't yet relocated itself, so the notifier address may
> +	 be incorrect (as it has to be relocated).  */
> +      CORE_ADDR start = bfd_get_start_address (exec_bfd);
> +      if (start == 0)
> +	notifier = 0;
> +      else
> +        {
> +          gdb_bfd_ref_ptr dyld_bfd (darwin_get_dyld_bfd ());

Here too.

Simon

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

[Xavier Roirand]

Hello,

Thanks Tom and Simon for all the work you've done on this patch. I was 
quite busy last week and did not follow this thread.

Le 9/19/18 à 9:15 PM, Tom Tromey a écrit :
>>>>>> "Simon" == Simon Marchi <simon.marchi@polymtl.ca> writes:
> 
> Simon> I would vote for only checking in the code you know is necessary for
> Simon> now, otherwise it will just be more confusing in the future, trying to
> Simon> figure out what is needed and what isn't.
> 
> Here is a more minimal version of the patch.  This one seems to work for
> me on High Sierra.  I tried running a "hello world" program -- this
> previously failed, but now works.  It's good enough that I could run
> gdb.cp/*.exp -- lots of fails but no crashes or mystery problems.
> 
> Tom
> 
> commit 114a1aae792443d72f1438dbc979b42a39c5b780
> 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 was tested on High Sierra by trying to run a simple "hello world"
>      program.
>      
>      gdb/ChangeLog:
>      
>              * solib-darwin.c (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.
>      
>      Change-Id: I7dde5008c9158f17b78dc89bd7f4bd8a12d4a6e1
> 
> diff --git a/gdb/ChangeLog b/gdb/ChangeLog
> index 328d48eeeb9..804aaf78e91 100644
> --- a/gdb/ChangeLog
> +++ b/gdb/ChangeLog
> @@ -1,3 +1,9 @@
> +2018-09-18  Xavier Roirand  <roirand@adacore.com>
> +
> +          * solib-darwin.c (darwin_get_dyld_bfd): New function.
> +          (darwin_solib_get_all_image_info_addr_at_init): Update call.
> +          (darwin_solib_create_inferior_hook): Handle unrelocated dyld.
> +
>   2018-09-18  Tom Tromey  <tom@tromey.com>
>   
>   	* compile/compile-object-load.c (struct
> diff --git a/gdb/solib-darwin.c b/gdb/solib-darwin.c
> index ed8e0c13365..1877ec0839d 100644
> --- a/gdb/solib-darwin.c
> +++ b/gdb/solib-darwin.c
> @@ -429,23 +429,21 @@ gdb_bfd_mach_o_fat_extract (bfd *abfd, bfd_format format,
>     return gdb_bfd_ref_ptr (result);
>   }
>   
> -/* Extract dyld_all_image_addr when the process was just created, assuming the
> -   current PC is at the entry of the dynamic linker.  */
> +/* Return the BFD for the program interpreter.  */
>   
> -static void
> -darwin_solib_get_all_image_info_addr_at_init (struct darwin_info *info)
> +static gdb_bfd_ref_ptr
> +darwin_get_dyld_bfd ()
>   {
>     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;
>   
>     /* 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 +457,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;
>   
> @@ -528,10 +538,6 @@ darwin_solib_create_inferior_hook (int from_tty)
>         return;
>       }
>   
> -  /* Add the breakpoint which is hit by dyld when the list of solib is
> -     modified.  */
> -  create_solib_event_breakpoint (target_gdbarch (), info->all_image.notifier);
> -
>     if (info->all_image.count != 0)
>       {
>         /* Possible relocate the main executable (PIE).  */
> @@ -558,6 +564,49 @@ 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).  */
> +  CORE_ADDR notifier = info->all_image.notifier;
> +
> +  if (info->all_image.count == 0)
> +    {
> +      /* Dyld hasn't yet relocated itself, so the notifier address may
> +	 be incorrect (as it has to be relocated).  */
> +      CORE_ADDR 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;
> +            }
> +        }
> +    }
> +
> +  /* 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);
>   }
>   
>   static void
> 

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

[Tom Tromey]

>>>>> "Xavier" == Xavier Roirand <roirand@adacore.com> writes:

Xavier> Thanks Tom and Simon for all the work you've done on this patch. I was
Xavier> quite busy last week and did not follow this thread.

Thank you for writing it in the first place.
Also it is not too late for your review.  If you think what went in
needs some change, let me know.

Tom

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

[Simon Marchi]

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

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

[Tom Tromey]

>>>>> "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

[RFA 4/5] Darwin: fix thread ptid started by fork_inferior

[Xavier Roirand]

When debugging a program on Mac OS X Darwin, gdb stops with:

Temporary breakpoint 1 at 0x100000fb4: file /tmp/helloworld.c, line 1.
Starting program: /private/tmp/helloworld
[New Thread 0xb03 of process 65066]
[New Thread 0xd03 of process 65066]
During startup program terminated with signal SIGTRAP, Trace/breakpoint trap.

When the inferior is started a thread with lwp=tid=0 is created
and has to be fixed later by darwin_init_thread_list(). Because
this is not done, GDB does not understand that the SIGTRAP is
coming from GDB and not the program itself.

gdb/ChangeLog:

        * darwin-nat.c (darwin_check_new_threads): Check if first
        silent thread case.
        (darwin_init_thread_list): Fixup thread ptid.
        (darwin_ptrace_him): Add call.
        (darwin_nat_target::attach): Add call.

Change-Id: Ie8ad7edfff167a1f95140907c75af45ceb3fae14
---
 gdb/darwin-nat.c | 31 ++++++++++++++++++++++++++++---
 1 file changed, 28 insertions(+), 3 deletions(-)

diff --git a/gdb/darwin-nat.c b/gdb/darwin-nat.c
index 96f70cf..9ad4a87 100644
--- a/gdb/darwin-nat.c
+++ b/gdb/darwin-nat.c
@@ -344,8 +344,22 @@ darwin_check_new_threads (struct inferior *inf)
 	  pti->gdb_port = new_id;
 	  pti->msg_state = DARWIN_RUNNING;
 
-	  /* Add the new thread.  */
-	  add_thread_with_info (ptid_t (inf->pid, 0, new_id), pti);
+	  if (old_nbr == 0 && new_ix == 0)
+            {
+	      /* A ptid is created when the inferior is started (see
+                 fork-child.c) with lwp=tid=0.  This ptid will be renamed
+                 later by darwin_init_thread_list (), so find this previous
+                 thread silently added.  */
+
+              struct thread_info *tp = find_thread_ptid (ptid_t (inf->pid, 0, 0));
+              tp->priv.reset (pti);
+            }
+          else
+            {
+              /* Add the new thread.  */
+              add_thread_with_info (ptid_t (inf->pid, 0, new_id), pti);
+             }
+
 	  new_thread_vec.push_back (pti);
 	  new_ix++;
 	  continue;
@@ -1733,6 +1747,8 @@ thread_info_from_private_thread_info (darwin_thread_info *pti)
 static void
 darwin_init_thread_list (struct inferior *inf)
 {
+  ptid_t new_ptid;
+
   darwin_check_new_threads (inf);
 
   darwin_inferior *priv = get_darwin_inferior (inf);
@@ -1743,7 +1759,11 @@ darwin_init_thread_list (struct inferior *inf)
   struct thread_info *first_thread
     = thread_info_from_private_thread_info (first_pti);
 
-  inferior_ptid = first_thread->ptid;
+  /* Note: fork_inferior automatically add a thread but it uses a wrong ptid.
+     Fix up.  */
+  new_ptid = ptid_t (inf->pid, 0, first_pti->gdb_port);
+  thread_change_ptid (inferior_ptid, new_ptid);
+  inferior_ptid = new_ptid;
 }
 
 /* The child must synchronize with gdb: gdb must set the exception port
@@ -1806,6 +1826,9 @@ darwin_ptrace_him (int pid)
   int traps_expected;
   struct inferior *inf = current_inferior ();
 
+  inferior_ptid = ptid_t (pid);
+  add_thread_silent (inferior_ptid);
+
   darwin_attach_pid (inf);
 
   /* Let's the child run.  */
@@ -1933,6 +1956,8 @@ darwin_nat_target::attach (const char *args, int from_tty)
   inferior_appeared (inf, pid);
   inf->attach_flag = 1;
 
+  add_thread_silent (inferior_ptid);
+
   darwin_attach_pid (inf);
 
   darwin_suspend_inferior (inf);
-- 
2.7.4

Re: [RFA 4/5] Darwin: fix thread ptid started by fork_inferior

[Simon Marchi]

On 2018-08-22 06:11, Xavier Roirand wrote:
> When debugging a program on Mac OS X Darwin, gdb stops with:
> 
> Temporary breakpoint 1 at 0x100000fb4: file /tmp/helloworld.c, line 1.
> Starting program: /private/tmp/helloworld
> [New Thread 0xb03 of process 65066]
> [New Thread 0xd03 of process 65066]
> During startup program terminated with signal SIGTRAP, Trace/breakpoint 
> trap.
> 
> When the inferior is started a thread with lwp=tid=0 is created
> and has to be fixed later by darwin_init_thread_list(). Because
> this is not done, GDB does not understand that the SIGTRAP is
> coming from GDB and not the program itself.

I think I have seen this error in my testing yesterday, though it seemed 
intermittent.  If I started a few times in a row, I would often get that 
SIGTRAP, but it would eventually work...  I can't really comment on the 
validity of the fix, so I will assume it's right (I am not at work right 
now so I can't test on the Mac).  But linux-nat does something similar 
(add a pid-only thread, then change it to its real ptid), so there is a 
precedent.

> diff --git a/gdb/darwin-nat.c b/gdb/darwin-nat.c
> index 96f70cf..9ad4a87 100644
> --- a/gdb/darwin-nat.c
> +++ b/gdb/darwin-nat.c
> @@ -344,8 +344,22 @@ darwin_check_new_threads (struct inferior *inf)
>  	  pti->gdb_port = new_id;
>  	  pti->msg_state = DARWIN_RUNNING;
> 
> -	  /* Add the new thread.  */
> -	  add_thread_with_info (ptid_t (inf->pid, 0, new_id), pti);
> +	  if (old_nbr == 0 && new_ix == 0)
> +            {
> +	      /* A ptid is created when the inferior is started (see
> +                 fork-child.c) with lwp=tid=0.  This ptid will be 
> renamed
> +                 later by darwin_init_thread_list (), so find this 
> previous
> +                 thread silently added.  */
> +
> +              struct thread_info *tp = find_thread_ptid (ptid_t
> (inf->pid, 0, 0));
> +              tp->priv.reset (pti);

Put a gdb_assert (tp != nullptr), so that if for some reason the thread 
is not found (because of a GDB bug), we fail with a failed assertion 
rather than a segfault.

> +            }
> +          else
> +            {
> +              /* Add the new thread.  */
> +              add_thread_with_info (ptid_t (inf->pid, 0, new_id), 
> pti);
> +             }
> +
>  	  new_thread_vec.push_back (pti);
>  	  new_ix++;
>  	  continue;
> @@ -1733,6 +1747,8 @@ thread_info_from_private_thread_info
> (darwin_thread_info *pti)
>  static void
>  darwin_init_thread_list (struct inferior *inf)
>  {
> +  ptid_t new_ptid;
> +
>    darwin_check_new_threads (inf);
> 
>    darwin_inferior *priv = get_darwin_inferior (inf);
> @@ -1743,7 +1759,11 @@ darwin_init_thread_list (struct inferior *inf)
>    struct thread_info *first_thread
>      = thread_info_from_private_thread_info (first_pti);
> 
> -  inferior_ptid = first_thread->ptid;
> +  /* Note: fork_inferior automatically add a thread but it uses a 
> wrong ptid.
> +     Fix up.  */
> +  new_ptid = ptid_t (inf->pid, 0, first_pti->gdb_port);
> +  thread_change_ptid (inferior_ptid, new_ptid);
> +  inferior_ptid = new_ptid;

You can declare new_ptid when initializing it.

Simon

Re: [RFA 4/5] Darwin: fix thread ptid started by fork_inferior

[Pedro Alves]

On 08/22/2018 11:11 AM, Xavier Roirand wrote:

> 
> diff --git a/gdb/darwin-nat.c b/gdb/darwin-nat.c
> index 96f70cf..9ad4a87 100644
> --- a/gdb/darwin-nat.c
> +++ b/gdb/darwin-nat.c
> @@ -344,8 +344,22 @@ darwin_check_new_threads (struct inferior *inf)
>  	  pti->gdb_port = new_id;
>  	  pti->msg_state = DARWIN_RUNNING;
>  
> -	  /* Add the new thread.  */
> -	  add_thread_with_info (ptid_t (inf->pid, 0, new_id), pti);
> +	  if (old_nbr == 0 && new_ix == 0)
> +            {
> +	      /* A ptid is created when the inferior is started (see
> +                 fork-child.c) with lwp=tid=0.  

It looks like this patch was written against an older gdb,
because fork-child.c doesn't add a thread nowadays.  For GNU/Linux, it's
inf-ptrace.c that adds the initial thread, but only after
fork_inferior returns (inf_ptrace_target::create_inferior).

But were is that equivalent code in darwin-nat.c?

/me looks.

Answer: it's nowhere.  It does not exist.

So, when then shared fork-child.c was created a while ago,
the add_thread call was moved to darwin-nat.c's target_create_inferior
implementation.  But, later on, Simon removed that add_thread call with:

 commit db665f427ca781d631d9e29b1bb744fb11ffcbba
 Author:     Simon Marchi <simon.marchi@ericsson.com>
 AuthorDate: Tue Jun 27 10:55:36 2017 +0200
 Commit:     Simon Marchi <simon.marchi@ericsson.com>
 CommitDate: Tue Jun 27 10:56:53 2017 +0200

    darwin: Do not add a dummy thread

(Weird, I can't find that patch on the list's archives, even
though I received a local copy.)

It sounds to me like you need to reevaluate the patch from
scratch, because its premise is invalid.

> @@ -1933,6 +1956,8 @@ darwin_nat_target::attach (const char *args, int from_tty)
>    inferior_appeared (inf, pid);
>    inf->attach_flag = 1;
>  
> +  add_thread_silent (inferior_ptid);
> +
>    darwin_attach_pid (inf);
>  
>    darwin_suspend_inferior (inf);

This surely is not related to "run" and/or fork-inferior.c

Thanks,
Pedro Alves

Re: [RFA 4/5] Darwin: fix thread ptid started by fork_inferior

[Simon Marchi]

On 2018-08-22 12:10, Pedro Alves wrote:
> On 08/22/2018 11:11 AM, Xavier Roirand wrote:
> 
>> 
>> diff --git a/gdb/darwin-nat.c b/gdb/darwin-nat.c
>> index 96f70cf..9ad4a87 100644
>> --- a/gdb/darwin-nat.c
>> +++ b/gdb/darwin-nat.c
>> @@ -344,8 +344,22 @@ darwin_check_new_threads (struct inferior *inf)
>>  	  pti->gdb_port = new_id;
>>  	  pti->msg_state = DARWIN_RUNNING;
>> 
>> -	  /* Add the new thread.  */
>> -	  add_thread_with_info (ptid_t (inf->pid, 0, new_id), pti);
>> +	  if (old_nbr == 0 && new_ix == 0)
>> +            {
>> +	      /* A ptid is created when the inferior is started (see
>> +                 fork-child.c) with lwp=tid=0.
> 
> It looks like this patch was written against an older gdb,
> because fork-child.c doesn't add a thread nowadays.  For GNU/Linux, 
> it's
> inf-ptrace.c that adds the initial thread, but only after
> fork_inferior returns (inf_ptrace_target::create_inferior).
> 
> But were is that equivalent code in darwin-nat.c?
> 
> /me looks.
> 
> Answer: it's nowhere.  It does not exist.
> 
> So, when then shared fork-child.c was created a while ago,
> the add_thread call was moved to darwin-nat.c's target_create_inferior
> implementation.  But, later on, Simon removed that add_thread call 
> with:
> 
>  commit db665f427ca781d631d9e29b1bb744fb11ffcbba
>  Author:     Simon Marchi <simon.marchi@ericsson.com>
>  AuthorDate: Tue Jun 27 10:55:36 2017 +0200
>  Commit:     Simon Marchi <simon.marchi@ericsson.com>
>  CommitDate: Tue Jun 27 10:56:53 2017 +0200
> 
>     darwin: Do not add a dummy thread
> 
> (Weird, I can't find that patch on the list's archives, even
> though I received a local copy.)
> 
> It sounds to me like you need to reevaluate the patch from
> scratch, because its premise is invalid.

Hmm I had not considered that, I did that a while ago and had completely 
forgotten about it.  I'll try to take a look tomorrow, when I have 
access to the Mac machine.  Maybe the spurious SIGTRAP I saw when 
running was caused by the bug fixed by patch 5/5.

Simon

Re: [RFA 4/5] Darwin: fix thread ptid started by fork_inferior

[Tom Tromey]

>>>>> "Xavier" == Xavier Roirand <roirand@adacore.com> writes:

Xavier> When debugging a program on Mac OS X Darwin, gdb stops with:
Xavier> Temporary breakpoint 1 at 0x100000fb4: file /tmp/helloworld.c, line 1.
Xavier> Starting program: /private/tmp/helloworld
Xavier> [New Thread 0xb03 of process 65066]
Xavier> [New Thread 0xd03 of process 65066]
Xavier> During startup program terminated with signal SIGTRAP, Trace/breakpoint trap.

I applied patch #2 from this series locally, but not patch #4.  Then I
ran gdb on a simple test program.  This worked fine.  So at least here
this patch seems not to be needed now.

Tom

[RFA 1/5] Darwin: fix bad loop incrementation

[Xavier Roirand]

When reading symbols from the vector of oso files on Mac OS X
Darwin, a previous commit introduce a change in the loop and add
an increment at each loop iteration whereas this incrementation is
not needed since the increment or set of the loop control variable
is already done in the loop.

gdb/ChangeLog:

        * machoread.c (macho_symfile_read_all_oso): Remove uneeded
        incrementation.

Change-Id: I3a5a6deb4e9d834ee7d4217a62d90c2ffb7241bc
---
 gdb/machoread.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/gdb/machoread.c b/gdb/machoread.c
index 0cbd209..3040fe7 100644
--- a/gdb/machoread.c
+++ b/gdb/machoread.c
@@ -615,7 +615,7 @@ macho_symfile_read_all_oso (std::vector<oso_el> *oso_vector_ptr,
   std::sort (oso_vector_ptr->begin (), oso_vector_ptr->end (),
 	     oso_el_compare_name);
 
-  for (ix = 0; ix < oso_vector_ptr->size (); ++ix)
+  for (ix = 0; ix < oso_vector_ptr->size ();)
     {
       int pfx_len;
 
-- 
2.7.4

Re: [RFA 1/5] Darwin: fix bad loop incrementation

[Simon Marchi]

On 2018-08-22 06:11, Xavier Roirand wrote:
> When reading symbols from the vector of oso files on Mac OS X
> Darwin, a previous commit introduce a change in the loop and add
> an increment at each loop iteration whereas this incrementation is
> not needed since the increment or set of the loop control variable
> is already done in the loop.
> 
> gdb/ChangeLog:
> 
>         * machoread.c (macho_symfile_read_all_oso): Remove uneeded
>         incrementation.
> 
> Change-Id: I3a5a6deb4e9d834ee7d4217a62d90c2ffb7241bc
> ---
>  gdb/machoread.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/gdb/machoread.c b/gdb/machoread.c
> index 0cbd209..3040fe7 100644
> --- a/gdb/machoread.c
> +++ b/gdb/machoread.c
> @@ -615,7 +615,7 @@ macho_symfile_read_all_oso (std::vector<oso_el>
> *oso_vector_ptr,
>    std::sort (oso_vector_ptr->begin (), oso_vector_ptr->end (),
>  	     oso_el_compare_name);
> 
> -  for (ix = 0; ix < oso_vector_ptr->size (); ++ix)
> +  for (ix = 0; ix < oso_vector_ptr->size ();)
>      {
>        int pfx_len;

I compared this with the original code (before 2cc9b3048b), that LGTM.

Simon

Re: [RFA 1/5] Darwin: fix bad loop incrementation

[Simon Marchi]

On 2018-08-22 09:14, Simon Marchi wrote:
> On 2018-08-22 06:11, Xavier Roirand wrote:
>> When reading symbols from the vector of oso files on Mac OS X
>> Darwin, a previous commit introduce a change in the loop and add
>> an increment at each loop iteration whereas this incrementation is
>> not needed since the increment or set of the loop control variable
>> is already done in the loop.
>> 
>> gdb/ChangeLog:
>> 
>>         * machoread.c (macho_symfile_read_all_oso): Remove uneeded
>>         incrementation.
>> 
>> Change-Id: I3a5a6deb4e9d834ee7d4217a62d90c2ffb7241bc
>> ---
>>  gdb/machoread.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>> 
>> diff --git a/gdb/machoread.c b/gdb/machoread.c
>> index 0cbd209..3040fe7 100644
>> --- a/gdb/machoread.c
>> +++ b/gdb/machoread.c
>> @@ -615,7 +615,7 @@ macho_symfile_read_all_oso (std::vector<oso_el>
>> *oso_vector_ptr,
>>    std::sort (oso_vector_ptr->begin (), oso_vector_ptr->end (),
>>  	     oso_el_compare_name);
>> 
>> -  for (ix = 0; ix < oso_vector_ptr->size (); ++ix)
>> +  for (ix = 0; ix < oso_vector_ptr->size ();)
>>      {
>>        int pfx_len;
> 
> I compared this with the original code (before 2cc9b3048b), that LGTM.
> 
> Simon

I took the liberty of pushing this patch to master as well as 
gdb-8.2-branch, since it is a regression since 8.1.

Thanks,

Simon

[RFA 3/5] Darwin: set startup-with-shell to off on Sierra and later.

[Xavier Roirand]

On Mac OS X Sierra and later, the shell is not allowed to be
debug so add a check and disable startup with shell in that
case.

gdb/ChangeLog:
    * darwin-nat.c (disable_startup_with_shell): New function.
    (_initialize_darwin_inferior): Add call.

Change-Id: Ia3cbeaa89b2b44a173b93ee22cce0d3884a16924
---
 gdb/darwin-nat.c | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/gdb/darwin-nat.c b/gdb/darwin-nat.c
index be80163..96f70cf 100644
--- a/gdb/darwin-nat.c
+++ b/gdb/darwin-nat.c
@@ -2362,6 +2362,26 @@ darwin_nat_target::supports_multi_process ()
   return true;
 }
 
+/* Read kernel version, and set startup-with-shell to false on Sierra or
+   later.  */
+
+void
+disable_startup_with_shell ()
+{
+  char str[16];
+    size_t sz = sizeof (str);
+    int ret;
+    unsigned long ver;
+
+    ret = sysctlbyname ("kern.osrelease", str, &sz, NULL, 0);
+    if (ret == 0 && sz < sizeof (str))
+      {
+       ver = strtoul (str, NULL, 10);
+       if (ver >= 16)
+         startup_with_shell = 0;
+      }
+}
+
 void
 _initialize_darwin_nat ()
 {
@@ -2396,4 +2416,6 @@ When this mode is on, all low level exceptions are reported before being\n\
 reported by the kernel."),
 			   &set_enable_mach_exceptions, NULL,
 			   &setlist, &showlist);
+
+  disable_startup_with_shell ();
 }
-- 
2.7.4

Re: [RFA 3/5] Darwin: set startup-with-shell to off on Sierra and later.

[Simon Marchi]

On 2018-08-22 06:11, Xavier Roirand wrote:
> On Mac OS X Sierra and later, the shell is not allowed to be
> debug so add a check and disable startup with shell in that
> case.

Ah, that's a really good idea to do it automatically, instead of asking 
the user to do it.

> gdb/ChangeLog:
>     * darwin-nat.c (disable_startup_with_shell): New function.
>     (_initialize_darwin_inferior): Add call.
> 
> Change-Id: Ia3cbeaa89b2b44a173b93ee22cce0d3884a16924
> ---
>  gdb/darwin-nat.c | 22 ++++++++++++++++++++++
>  1 file changed, 22 insertions(+)
> 
> diff --git a/gdb/darwin-nat.c b/gdb/darwin-nat.c
> index be80163..96f70cf 100644
> --- a/gdb/darwin-nat.c
> +++ b/gdb/darwin-nat.c
> @@ -2362,6 +2362,26 @@ darwin_nat_target::supports_multi_process ()
>    return true;
>  }
> 
> +/* Read kernel version, and set startup-with-shell to false on Sierra 
> or
> +   later.  */
> +
> +void
> +disable_startup_with_shell ()

This function should be static.

> +{
> +  char str[16];
> +    size_t sz = sizeof (str);
> +    int ret;
> +    unsigned long ver;
> +
> +    ret = sysctlbyname ("kern.osrelease", str, &sz, NULL, 0);
> +    if (ret == 0 && sz < sizeof (str))
> +      {
> +       ver = strtoul (str, NULL, 10);
> +       if (ver >= 16)
> +         startup_with_shell = 0;
> +      }
> +}

The indentation is not quite right.  You can also declare variables when 
they are used/initialized.

> +
>  void
>  _initialize_darwin_nat ()
>  {
> @@ -2396,4 +2416,6 @@ When this mode is on, all low level exceptions
> are reported before being\n\
>  reported by the kernel."),
>  			   &set_enable_mach_exceptions, NULL,
>  			   &setlist, &showlist);
> +
> +  disable_startup_with_shell ();
>  }

I don't think we should do that in _initialize_darwin_nat.  Since 
startup-with-shell is supported with remote debugging, you could still 
use it when starting a Linux remote program from a macOS host.

Instead, we should probably only disable it at the moment we create a 
new inferior in the darwin_nat target, so in 
darwin_nat_target::create_inferior.  We also don't want to permanently 
change the setting, so we should restore the value.  Presumably, putting 
something like this in darwin_nat_target::create_inferior should work (I 
haven't tested it, and sorry for the potential formatting mess my email 
client will do):

   gdb::optional<scoped_restore_tmpl<int>> restore_startup_with_shell;
   if (startup_with_shell && should_disable_startup_with_shell ())
     {
       warning (_("startup-with-shell not supported on this macOS 
version, disabling it."));
       restore_startup_with_shell.emplace (&startup_with_shell, 0);
     }

Instead of setting/restoring startup_with_shell, it might be better if 
its value was passed as a parameter all the way down instead of having 
functions read the global variable, but that's a bigger job.

Simon

Re: [RFA 3/5] Darwin: set startup-with-shell to off on Sierra and later.

[Pedro Alves]

On 08/22/2018 03:20 PM, Simon Marchi wrote:
> On 2018-08-22 06:11, Xavier Roirand wrote:
>> On Mac OS X Sierra and later, the shell is not allowed to be
>> debug so add a check and disable startup with shell in that
>> case.
> 
> Ah, that's a really good idea to do it automatically, instead of asking the user to do it.

See also:

  https://sourceware.org/ml/gdb-patches/2018-06/msg00734.html

BTW, on IRC, Tromey and I discussed how to make startup-with-shell
work, which led to:

  https://sourceware.org/bugzilla/show_bug.cgi?id=23364

which points at what seems like the simplest solution (copy the shell
to /tmp).  

An alternative would be to use a similar approach to
lldb's -- run a helper tool (lldb-argdumper) via the shell whose only
purpose is to return back its shell-expanded argv[0..N].  That would work
because gdb wouldn't debug that program, just run it normally, so it
wouldn't run into the SIP restrictions.  Instead of a separate helper tool, I would
imagine we could also build the helper functionality inside gdb itself -- we'd
make gdb behave in that "expand args" mode via an environment
variable, for example.

> 
>> gdb/ChangeLog:
>>     * darwin-nat.c (disable_startup_with_shell): New function.
>>     (_initialize_darwin_inferior): Add call.
>>
>> Change-Id: Ia3cbeaa89b2b44a173b93ee22cce0d3884a16924
>> ---
>>  gdb/darwin-nat.c | 22 ++++++++++++++++++++++
>>  1 file changed, 22 insertions(+)
>>
>> diff --git a/gdb/darwin-nat.c b/gdb/darwin-nat.c
>> index be80163..96f70cf 100644
>> --- a/gdb/darwin-nat.c
>> +++ b/gdb/darwin-nat.c
>> @@ -2362,6 +2362,26 @@ darwin_nat_target::supports_multi_process ()
>>    return true;
>>  }
>>
>> +/* Read kernel version, and set startup-with-shell to false on Sierra or
>> +   later.  */
>> +
>> +void
>> +disable_startup_with_shell ()
> 
> This function should be static.
> 
>> +{
>> +  char str[16];
>> +    size_t sz = sizeof (str);
>> +    int ret;
>> +    unsigned long ver;
>> +
>> +    ret = sysctlbyname ("kern.osrelease", str, &sz, NULL, 0);
>> +    if (ret == 0 && sz < sizeof (str))
>> +      {
>> +       ver = strtoul (str, NULL, 10);
>> +       if (ver >= 16)
>> +         startup_with_shell = 0;
>> +      }
>> +}
> 
> The indentation is not quite right.  You can also declare variables when they are used/initialized.
> 
>> +
>>  void
>>  _initialize_darwin_nat ()
>>  {
>> @@ -2396,4 +2416,6 @@ When this mode is on, all low level exceptions
>> are reported before being\n\
>>  reported by the kernel."),
>>                 &set_enable_mach_exceptions, NULL,
>>                 &setlist, &showlist);
>> +
>> +  disable_startup_with_shell ();
>>  }
> 
> I don't think we should do that in _initialize_darwin_nat.  Since startup-with-shell is supported with remote debugging, you could still use it when starting a Linux remote program from a macOS host.
> 
> Instead, we should probably only disable it at the moment we create a new inferior in the darwin_nat target, so in darwin_nat_target::create_inferior.  We also don't want to permanently change the setting, so we should restore the value.  Presumably, putting something like this in darwin_nat_target::create_inferior should work (I haven't tested it, and sorry for the potential formatting mess my email client will do):
> 
>   gdb::optional<scoped_restore_tmpl<int>> restore_startup_with_shell;
>   if (startup_with_shell && should_disable_startup_with_shell ())
>     {
>       warning (_("startup-with-shell not supported on this macOS version, disabling it."));
>       restore_startup_with_shell.emplace (&startup_with_shell, 0);
>     }
> 
> Instead of setting/restoring startup_with_shell, it might be better if its value was passed as a parameter all the way down instead of having functions read the global variable, but that's a bigger job.
Thanks,
Pedro Alves

Re: [RFA 3/5] Darwin: set startup-with-shell to off on Sierra and later.

[Xavier Roirand]

Hello,

Le 8/22/18 à 4:20 PM, Simon Marchi a écrit :
> On 2018-08-22 06:11, Xavier Roirand wrote:
>> On Mac OS X Sierra and later, the shell is not allowed to be
>> debug so add a check and disable startup with shell in that
>> case.
> 
> Ah, that's a really good idea to do it automatically, instead of asking 
> the user to do it.
> 
>> gdb/ChangeLog:
>>     * darwin-nat.c (disable_startup_with_shell): New function.
>>     (_initialize_darwin_inferior): Add call.
>>
>> Change-Id: Ia3cbeaa89b2b44a173b93ee22cce0d3884a16924
>> ---
>>  gdb/darwin-nat.c | 22 ++++++++++++++++++++++
>>  1 file changed, 22 insertions(+)
>>
>> diff --git a/gdb/darwin-nat.c b/gdb/darwin-nat.c
>> index be80163..96f70cf 100644
>> --- a/gdb/darwin-nat.c
>> +++ b/gdb/darwin-nat.c
>> @@ -2362,6 +2362,26 @@ darwin_nat_target::supports_multi_process ()
>>    return true;
>>  }
>>
>> +/* Read kernel version, and set startup-with-shell to false on Sierra or
>> +   later.  */
>> +
>> +void
>> +disable_startup_with_shell ()
> 
> This function should be static.
> 
>> +{
>> +  char str[16];
>> +    size_t sz = sizeof (str);
>> +    int ret;
>> +    unsigned long ver;
>> +
>> +    ret = sysctlbyname ("kern.osrelease", str, &sz, NULL, 0);
>> +    if (ret == 0 && sz < sizeof (str))
>> +      {
>> +       ver = strtoul (str, NULL, 10);
>> +       if (ver >= 16)
>> +         startup_with_shell = 0;
>> +      }
>> +}
> 
> The indentation is not quite right.  You can also declare variables when 
> they are used/initialized.
> 
>> +
>>  void
>>  _initialize_darwin_nat ()
>>  {
>> @@ -2396,4 +2416,6 @@ When this mode is on, all low level exceptions
>> are reported before being\n\
>>  reported by the kernel."),
>>                 &set_enable_mach_exceptions, NULL,
>>                 &setlist, &showlist);
>> +
>> +  disable_startup_with_shell ();
>>  }
> 
> I don't think we should do that in _initialize_darwin_nat.  Since 
> startup-with-shell is supported with remote debugging, you could still 
> use it when starting a Linux remote program from a macOS host.
> 
> Instead, we should probably only disable it at the moment we create a 
> new inferior in the darwin_nat target, so in 
> darwin_nat_target::create_inferior.  We also don't want to permanently 
> change the setting, so we should restore the value.  Presumably, putting 
> something like this in darwin_nat_target::create_inferior should work (I 
> haven't tested it, and sorry for the potential formatting mess my email 
> client will do):
> 
>    gdb::optional<scoped_restore_tmpl<int>> restore_startup_with_shell;
>    if (startup_with_shell && should_disable_startup_with_shell ())
>      {
>        warning (_("startup-with-shell not supported on this macOS 
> version, disabling it."));
>        restore_startup_with_shell.emplace (&startup_with_shell, 0);
>      }
> 

Thanks for the remarks, I'll update my patch accordingly.

Regards.

> Instead of setting/restoring startup_with_shell, it might be better if 
> its value was passed as a parameter all the way down instead of having 
> functions read the global variable, but that's a bigger job.
> 
> Simon

Re: [RFA 3/5] Darwin: set startup-with-shell to off on Sierra and later.

[Tom Tromey]

>>>>> "Xavier" == Xavier Roirand <roirand@adacore.com> writes:

Xavier> On Mac OS X Sierra and later, the shell is not allowed to be
Xavier> debug so add a check and disable startup with shell in that
Xavier> case.

I learned recently that System Integrity Protection was introduced in
El Capitan:

    https://support.apple.com/en-us/HT204899

So, I'm curious why this patch checks for Sierra.

I thought maybe it could just be a bug, but also that maybe the ptrace
changes were added in Sierra.

Do you know?  I don't have an older machine to try on.  I think it would
be good to document or fix this decision, depending on what's going on.

Tom

[RFA 5/5] Darwin: fix SIGTRAP when debugging

[Xavier Roirand]

Debugging a program under Darwin does not work:

(gdb) start
Temporary breakpoint 1 at 0x100000fb4: file /tmp/helloworld.c, line 1.
Starting program: /private/tmp/helloworld
[New Thread 0x2903 of process 60326]
During startup program terminated with signal SIGTRAP, Trace/breakpoint trap.

Field signaled from darwin_thread_info is not initialized thus signal sent to
the debuggee is considered as not sent by GDB whereas it should.

This patch fixes this problem.

gdb/ChangeLog:

    * darwin-nat.c (darwin_check_new_threads): Initialize signaled
    field.

Change-Id: I02052473f1f8d28106cc343f440fa784b110bbf5
---
 gdb/darwin-nat.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/gdb/darwin-nat.c b/gdb/darwin-nat.c
index 9ad4a87..bbd2700 100644
--- a/gdb/darwin-nat.c
+++ b/gdb/darwin-nat.c
@@ -341,6 +341,7 @@ darwin_check_new_threads (struct inferior *inf)
 	  /* A thread was created.  */
 	  darwin_thread_info *pti = new darwin_thread_info;
 
+	  pti->signaled = 0;
 	  pti->gdb_port = new_id;
 	  pti->msg_state = DARWIN_RUNNING;
 
-- 
2.7.4

Re: [RFA 5/5] Darwin: fix SIGTRAP when debugging

[Simon Marchi]

On 2018-08-22 06:11, Xavier Roirand wrote:
> Debugging a program under Darwin does not work:
> 
> (gdb) start
> Temporary breakpoint 1 at 0x100000fb4: file /tmp/helloworld.c, line 1.
> Starting program: /private/tmp/helloworld
> [New Thread 0x2903 of process 60326]
> During startup program terminated with signal SIGTRAP, Trace/breakpoint 
> trap.
> 
> Field signaled from darwin_thread_info is not initialized thus signal 
> sent to
> the debuggee is considered as not sent by GDB whereas it should.
> 
> This patch fixes this problem.
> 
> gdb/ChangeLog:
> 
>     * darwin-nat.c (darwin_check_new_threads): Initialize signaled
>     field.
> 
> Change-Id: I02052473f1f8d28106cc343f440fa784b110bbf5
> ---
>  gdb/darwin-nat.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/gdb/darwin-nat.c b/gdb/darwin-nat.c
> index 9ad4a87..bbd2700 100644
> --- a/gdb/darwin-nat.c
> +++ b/gdb/darwin-nat.c
> @@ -341,6 +341,7 @@ darwin_check_new_threads (struct inferior *inf)
>  	  /* A thread was created.  */
>  	  darwin_thread_info *pti = new darwin_thread_info;
> 
> +	  pti->signaled = 0;
>  	  pti->gdb_port = new_id;
>  	  pti->msg_state = DARWIN_RUNNING;

Oh, good catch.  Though instead, can we initialize the fields directly 
in the class declaration, so if we ever create darwin_thread_info at 
some other place, the object will be correctly initialized?  In C++11, 
you can initialize the fields directly like this:

   unsigned char signaled = 0;

I think we might as well initialize all the fields to a sensible value, 
and you could also change the "char" fields to "bool" at the same time.

Thanks,

Simon

Re: [RFA 0/5] Fix some bugs on macOS

[Tom Tromey]

>>>>> "Xavier" == Xavier Roirand <roirand@adacore.com> writes:

Xavier> I'm sending a couple of patches which make GDB working better on Sierra and later.

Hi Xavier.  I had a little time recently to work on some macOS patches,
and I noticed that some of your patches have not yet gone in.  As far as
I know they are all approved.  Is there something I can do to help land
them more quickly?  It would be convenient for me if they were already
in.

thanks,
Tom

Re: [RFA 0/5] Fix some bugs on macOS

[Joel Brobecker]

Hi Tom,

> Xavier> I'm sending a couple of patches which make GDB working better
> on Sierra and later.
> 
> Hi Xavier.  I had a little time recently to work on some macOS patches,
> and I noticed that some of your patches have not yet gone in.  As far as
> I know they are all approved.  Is there something I can do to help land
> them more quickly?  It would be convenient for me if they were already
> in.

Not speaking for Xavier specifically, but more in general; if patches
are approved, but somehow the contributor takes time to actually push,
I am been known to push the patch myself if it is a patch that I need.
I would say you should feel free to do the same; or else let me know
which patches (I lost track a bit), and I will push them for him --
I think Xavier is going to be busy this week. I am quite happy to
cover for him.

-- 
Joel

Re: [RFA 0/5] Fix some bugs on macOS

[Tom Tromey]

>>>>> "Joel" == Joel Brobecker <brobecker@adacore.com> writes:

Joel> Not speaking for Xavier specifically, but more in general; if patches
Joel> are approved, but somehow the contributor takes time to actually push,
Joel> I am been known to push the patch myself if it is a patch that I need.
Joel> I would say you should feel free to do the same; or else let me know
Joel> which patches (I lost track a bit), and I will push them for him --
Joel> I think Xavier is going to be busy this week. I am quite happy to
Joel> cover for him.

Thanks Joel.  Or I can also do it I just wanted to check in first -- I
didn't want to surprise anybody.

Tom