[RFC] Make "run" work on macOS 10.13

[RFC] Make "run" work on macOS 10.13

[Tom Tromey]

I would like some feedback on this patch.

On macOS 10.13.5, "run" does not work in gdb.  There are two cases:

1. If I forget to "set startup-with-shell off", then gdb will fail due
   to the system integrity protection feature.  I believe this happens
   because gdb is not allowed to debug the shell.

   You can find many sites advocating "set startup-with-shell off",
   but it seems to me that it is friendlier for gdb to simply do it by
   default.

   One option here might be to do this conditionally based on the
   version of the OS.

2. I found that gdb was setting the solib breakpoint incorrectly,
   causing a failure.  Adding the load address to the notifier address
   makes this work for me.  I suspect this would regress earlier
   versions of macOS, but I have no way to test that; one idea might
   be to only do this when gdb_dyld_all_image_infos::version == 15.

gdb/ChangeLog
2018-06-29  Tom Tromey  <tom@tromey.com>

	* solib-darwin.c (darwin_solib_create_inferior_hook): Create solib
	breakpoint later.  Add load_addr to the notifier address.
	* darwin-nat.c (darwin_nat_target::create_inferior): Bind
	startup_with_shell to 0.
---
 gdb/ChangeLog      | 7 +++++++
 gdb/darwin-nat.c   | 5 +++++
 gdb/solib-darwin.c | 9 +++++----
 3 files changed, 17 insertions(+), 4 deletions(-)

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 4c04d0ba728..c6462259fe0 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,10 @@
+2018-06-29  Tom Tromey  <tom@tromey.com>
+
+	* solib-darwin.c (darwin_solib_create_inferior_hook): Create solib
+	breakpoint later.  Add load_addr to the notifier address.
+	* darwin-nat.c (darwin_nat_target::create_inferior): Bind
+	startup_with_shell to 0.
+
 2018-06-28  Tom Tromey  <tom@tromey.com>
 
 	* NEWS: Mention --enable-codesign.
diff --git a/gdb/darwin-nat.c b/gdb/darwin-nat.c
index 7dccce73926..542c8389ef0 100644
--- a/gdb/darwin-nat.c
+++ b/gdb/darwin-nat.c
@@ -1809,6 +1809,11 @@ darwin_nat_target::create_inferior (const char *exec_file,
 				    const std::string &allargs,
 				    char **env, int from_tty)
 {
+  /* Starting with Sierra, SIP prevents gdb from attaching to the
+     shell, so users have to disable startup-with-shell.  */
+  scoped_restore save_startup
+    = make_scoped_restore (&startup_with_shell, 0);
+
   /* Do the hard work.  */
   fork_inferior (exec_file, allargs, env, darwin_ptrace_me,
 		 darwin_ptrace_him, darwin_pre_ptrace, NULL,
diff --git a/gdb/solib-darwin.c b/gdb/solib-darwin.c
index ed8e0c13365..a4e15dc6b5b 100644
--- a/gdb/solib-darwin.c
+++ b/gdb/solib-darwin.c
@@ -528,10 +528,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).  */
@@ -547,6 +543,11 @@ darwin_solib_create_inferior_hook (int from_tty)
       load_addr = darwin_read_exec_load_addr_at_init (info);
     }
 
+  /* 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 + load_addr);
+
   if (load_addr != 0 && symfile_objfile != NULL)
     {
       CORE_ADDR vmaddr;
-- 
2.17.1

Re: [RFC] Make "run" work on macOS 10.13

[Tom Tromey]

>>>>> "Tom" == Tom Tromey <tom@tromey.com> writes:

Tom> I would like some feedback on this patch.
Tom> On macOS 10.13.5, "run" does not work in gdb.

FWIW this improved gdb.cp/*.exp from 1359 passes to 3296 passes.

Tom

Re: [RFC] Make "run" work on macOS 10.13

[Joel Brobecker]

[-- Attachment #1: Type: text/plain, Size: 4006 bytes --]

Hi Tom,

> On macOS 10.13.5, "run" does not work in gdb.  There are two cases:
> 
> 1. If I forget to "set startup-with-shell off", then gdb will fail due
>    to the system integrity protection feature.  I believe this happens
>    because gdb is not allowed to debug the shell.
> 
>    You can find many sites advocating "set startup-with-shell off",
>    but it seems to me that it is friendlier for gdb to simply do it by
>    default.
> 
>    One option here might be to do this conditionally based on the
>    version of the OS.

Attached is a patch that Tristan wrote to make that automatic.
We were hoping to contribute that patch a while back, but we got
busy working on other targets that needed our immediate attention.

This patch has been working for us, but we wanted to review it
internally before submitting it, because we wanted to make sure
that changing the value of a global like that during _init
was the best way to do this.

> 2. I found that gdb was setting the solib breakpoint incorrectly,
>    causing a failure.  Adding the load address to the notifier address
>    makes this work for me.  I suspect this would regress earlier
>    versions of macOS, but I have no way to test that; one idea might
>    be to only do this when gdb_dyld_all_image_infos::version == 15.

Xavier, do you think you could help Tom to answer his question, please?

> gdb/ChangeLog
> 2018-06-29  Tom Tromey  <tom@tromey.com>
> 
> 	* solib-darwin.c (darwin_solib_create_inferior_hook): Create solib
> 	breakpoint later.  Add load_addr to the notifier address.
> 	* darwin-nat.c (darwin_nat_target::create_inferior): Bind
> 	startup_with_shell to 0.
> ---
>  gdb/ChangeLog      | 7 +++++++
>  gdb/darwin-nat.c   | 5 +++++
>  gdb/solib-darwin.c | 9 +++++----
>  3 files changed, 17 insertions(+), 4 deletions(-)
> 
> diff --git a/gdb/ChangeLog b/gdb/ChangeLog
> index 4c04d0ba728..c6462259fe0 100644
> --- a/gdb/ChangeLog
> +++ b/gdb/ChangeLog
> @@ -1,3 +1,10 @@
> +2018-06-29  Tom Tromey  <tom@tromey.com>
> +
> +	* solib-darwin.c (darwin_solib_create_inferior_hook): Create solib
> +	breakpoint later.  Add load_addr to the notifier address.
> +	* darwin-nat.c (darwin_nat_target::create_inferior): Bind
> +	startup_with_shell to 0.
> +
>  2018-06-28  Tom Tromey  <tom@tromey.com>
>  
>  	* NEWS: Mention --enable-codesign.
> diff --git a/gdb/darwin-nat.c b/gdb/darwin-nat.c
> index 7dccce73926..542c8389ef0 100644
> --- a/gdb/darwin-nat.c
> +++ b/gdb/darwin-nat.c
> @@ -1809,6 +1809,11 @@ darwin_nat_target::create_inferior (const char *exec_file,
>  				    const std::string &allargs,
>  				    char **env, int from_tty)
>  {
> +  /* Starting with Sierra, SIP prevents gdb from attaching to the
> +     shell, so users have to disable startup-with-shell.  */
> +  scoped_restore save_startup
> +    = make_scoped_restore (&startup_with_shell, 0);
> +
>    /* Do the hard work.  */
>    fork_inferior (exec_file, allargs, env, darwin_ptrace_me,
>  		 darwin_ptrace_him, darwin_pre_ptrace, NULL,
> diff --git a/gdb/solib-darwin.c b/gdb/solib-darwin.c
> index ed8e0c13365..a4e15dc6b5b 100644
> --- a/gdb/solib-darwin.c
> +++ b/gdb/solib-darwin.c
> @@ -528,10 +528,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).  */
> @@ -547,6 +543,11 @@ darwin_solib_create_inferior_hook (int from_tty)
>        load_addr = darwin_read_exec_load_addr_at_init (info);
>      }
>  
> +  /* 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 + load_addr);
> +
>    if (load_addr != 0 && symfile_objfile != NULL)
>      {
>        CORE_ADDR vmaddr;
> -- 
> 2.17.1

-- 
Joel

[-- Attachment #2: 0001-Darwin-set-startup-with-shell-to-off-by-default-on-S.patch --]
[-- Type: text/x-diff, Size: 1213 bytes --]

From 770848bb83d170ea2def48e02f7ea93f772124e4 Mon Sep 17 00:00:00 2001
From: Tristan Gingold <gingold@adacore.com>
Date: Wed, 15 Mar 2017 14:10:17 +0100
Subject: [PATCH] Darwin: set startup-with-shell to off by default on Sierra
 and later.

... as they aren't allowed to be debugger.

gdb/ChangeLog:
	* darwin-nat.c (_initialize_darwin_inferior): Clear
	startup_with_shell on Sierra and later.

---
 gdb/darwin-nat.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/gdb/darwin-nat.c b/gdb/darwin-nat.c
index e3d058d15e8..013f8c08a41 100644
--- a/gdb/darwin-nat.c
+++ b/gdb/darwin-nat.c
@@ -2383,6 +2383,23 @@ _initialize_darwin_inferior (void)
       MACH_CHECK_ERROR (kret);
     }
 
+  /* Read kernel version, and set startup-with-shell to false on Sierra or
+     later.  */
+  {
+    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;
+      }
+  }
+
   darwin_ops = inf_child_target ();
 
   darwin_ops->to_create_inferior = darwin_create_inferior;
-- 
2.17.1

Re: [RFC] Make "run" work on macOS 10.13

[Pedro Alves]

On 06/29/2018 09:55 PM, Tom Tromey wrote:
> I would like some feedback on this patch.
> 
> On macOS 10.13.5, "run" does not work in gdb.  There are two cases:
> 
> 1. If I forget to "set startup-with-shell off", then gdb will fail due
>    to the system integrity protection feature.  I believe this happens
>    because gdb is not allowed to debug the shell.
> 
>    You can find many sites advocating "set startup-with-shell off",
>    but it seems to me that it is friendlier for gdb to simply do it by
>    default.

Judging from stackoverflow, input redirection is a common-ish thing
for users to do.  Maybe we could reuse Eli's input redirection
emulation for Windows here:

 https://sourceware.org/ml/gdb-patches/2016-10/msg00832.html

Doesn't have to be you or in this patch of course.  Just a suggestion.

> 
>    One option here might be to do this conditionally based on the
>    version of the OS.
> 
> 2. I found that gdb was setting the solib breakpoint incorrectly,
>    causing a failure.  Adding the load address to the notifier address
>    makes this work for me.  I suspect this would regress earlier
>    versions of macOS, but I have no way to test that; one idea might
>    be to only do this when gdb_dyld_all_image_infos::version == 15.

14 vs 15 behaving differently does sound consistent with:

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

Thanks,
Pedro Alves

Re: [RFC] Make "run" work on macOS 10.13

[Simon Marchi]

On 2018-06-29 16:55, Tom Tromey wrote:
> I would like some feedback on this patch.
> 
> On macOS 10.13.5, "run" does not work in gdb.  There are two cases:
> 
> 1. If I forget to "set startup-with-shell off", then gdb will fail due
>    to the system integrity protection feature.  I believe this happens
>    because gdb is not allowed to debug the shell.
> 
>    You can find many sites advocating "set startup-with-shell off",
>    but it seems to me that it is friendlier for gdb to simply do it by
>    default.
> 
>    One option here might be to do this conditionally based on the
>    version of the OS.
> 
> 2. I found that gdb was setting the solib breakpoint incorrectly,
>    causing a failure.  Adding the load address to the notifier address
>    makes this work for me.  I suspect this would regress earlier
>    versions of macOS, but I have no way to test that; one idea might
>    be to only do this when gdb_dyld_all_image_infos::version == 15.
> 
> gdb/ChangeLog
> 2018-06-29  Tom Tromey  <tom@tromey.com>
> 
> 	* solib-darwin.c (darwin_solib_create_inferior_hook): Create solib
> 	breakpoint later.  Add load_addr to the notifier address.
> 	* darwin-nat.c (darwin_nat_target::create_inferior): Bind
> 	startup_with_shell to 0.
> ---
>  gdb/ChangeLog      | 7 +++++++
>  gdb/darwin-nat.c   | 5 +++++
>  gdb/solib-darwin.c | 9 +++++----
>  3 files changed, 17 insertions(+), 4 deletions(-)
> 
> diff --git a/gdb/ChangeLog b/gdb/ChangeLog
> index 4c04d0ba728..c6462259fe0 100644
> --- a/gdb/ChangeLog
> +++ b/gdb/ChangeLog
> @@ -1,3 +1,10 @@
> +2018-06-29  Tom Tromey  <tom@tromey.com>
> +
> +	* solib-darwin.c (darwin_solib_create_inferior_hook): Create solib
> +	breakpoint later.  Add load_addr to the notifier address.
> +	* darwin-nat.c (darwin_nat_target::create_inferior): Bind
> +	startup_with_shell to 0.
> +
>  2018-06-28  Tom Tromey  <tom@tromey.com>
> 
>  	* NEWS: Mention --enable-codesign.
> diff --git a/gdb/darwin-nat.c b/gdb/darwin-nat.c
> index 7dccce73926..542c8389ef0 100644
> --- a/gdb/darwin-nat.c
> +++ b/gdb/darwin-nat.c
> @@ -1809,6 +1809,11 @@ darwin_nat_target::create_inferior (const char
> *exec_file,
>  				    const std::string &allargs,
>  				    char **env, int from_tty)
>  {
> +  /* Starting with Sierra, SIP prevents gdb from attaching to the
> +     shell, so users have to disable startup-with-shell.  */
> +  scoped_restore save_startup
> +    = make_scoped_restore (&startup_with_shell, 0);
> +
>    /* Do the hard work.  */
>    fork_inferior (exec_file, allargs, env, darwin_ptrace_me,

I think this part is good.  I would suggest printing a message/warnings 
to indicate that we are disabling startup-with-shell (only if 
startup_with_shell is 1 in the first place).

>  		 darwin_ptrace_him, darwin_pre_ptrace, NULL,
> diff --git a/gdb/solib-darwin.c b/gdb/solib-darwin.c
> index ed8e0c13365..a4e15dc6b5b 100644
> --- a/gdb/solib-darwin.c
> +++ b/gdb/solib-darwin.c
> @@ -528,10 +528,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).  */
> @@ -547,6 +543,11 @@ darwin_solib_create_inferior_hook (int from_tty)
>        load_addr = darwin_read_exec_load_addr_at_init (info);
>      }
> 
> +  /* 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 + load_addr);
> +
>    if (load_addr != 0 && symfile_objfile != NULL)
>      {
>        CORE_ADDR vmaddr;

About the dynamic loader relocation, I am trying to compare your 
approach with Xavier's approach here:

https://sourceware.org/ml/gdb-patches/2018-08/msg00519.html

If I print the resulting notifier address, I get two different values:

With Tom's patch: 0x10000f782
With Xavier's patch: 0x100012782

The unrelocated value of the symbol is 0xf782.  That breakpoint is used 
for "set stop-on-solib-events", it seems, so I tried to enable that with 
both of your patches.  I got a stop with Xavier's patch and none with 
Tom's, which leads me to think that Xavier's patch gets it right.  I 
think you may be using the executable base address, while we actually 
want to use dyld's base address?  This is not very clear to me yet.

Simon

Re: [RFC] Make "run" work on macOS 10.13

[Tom Tromey]

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

>> {
>> +  /* Starting with Sierra, SIP prevents gdb from attaching to the
>> +     shell, so users have to disable startup-with-shell.  */
>> +  scoped_restore save_startup
>> +    = make_scoped_restore (&startup_with_shell, 0);
>> +
>> /* Do the hard work.  */
>> fork_inferior (exec_file, allargs, env, darwin_ptrace_me,

Simon> I think this part is good.  I would suggest printing a
Simon> message/warnings to indicate that we are disabling startup-with-shell
Simon> (only if startup_with_shell is 1 in the first place).

See the bug and also Pedro's comments on Xavier's similar patch --
there are other, probably better, ideas here.

Simon> The unrelocated value of the symbol is 0xf782.  That breakpoint is
Simon> used for "set stop-on-solib-events", it seems, so I tried to enable
Simon> that with both of your patches.  I got a stop with Xavier's patch and
Simon> none with Tom's, which leads me to think that Xavier's patch gets it
Simon> right.  I think you may be using the executable base address, while we
Simon> actually want to use dyld's base address?  This is not very clear to
Simon> me yet.

I think we want Xavier's patch and not mine.  Mine was more of a stab in
the dark.

Tom

Re: [RFC] Make "run" work on macOS 10.13

[Simon Marchi]

On 2018-08-23 16:03, Tom Tromey wrote:
>>>>>> "Simon" == Simon Marchi <simon.marchi@polymtl.ca> writes:
> 
>>> {
>>> +  /* Starting with Sierra, SIP prevents gdb from attaching to the
>>> +     shell, so users have to disable startup-with-shell.  */
>>> +  scoped_restore save_startup
>>> +    = make_scoped_restore (&startup_with_shell, 0);
>>> +
>>> /* Do the hard work.  */
>>> fork_inferior (exec_file, allargs, env, darwin_ptrace_me,
> 
> Simon> I think this part is good.  I would suggest printing a
> Simon> message/warnings to indicate that we are disabling 
> startup-with-shell
> Simon> (only if startup_with_shell is 1 in the first place).
> 
> See the bug and also Pedro's comments on Xavier's similar patch --
> there are other, probably better, ideas here.

I have seen those, but I think your change would improve the situation 
in the immediate, so I would vote for merging it (and even 
cherry-picking to 8.2).

> Simon> The unrelocated value of the symbol is 0xf782.  That breakpoint 
> is
> Simon> used for "set stop-on-solib-events", it seems, so I tried to 
> enable
> Simon> that with both of your patches.  I got a stop with Xavier's 
> patch and
> Simon> none with Tom's, which leads me to think that Xavier's patch 
> gets it
> Simon> right.  I think you may be using the executable base address, 
> while we
> Simon> actually want to use dyld's base address?  This is not very 
> clear to
> Simon> me yet.
> 
> I think we want Xavier's patch and not mine.  Mine was more of a stab 
> in
> the dark.

Ok, thanks, so we'll continue with Xavier's lead.

Simon