[RFC] Make "run" work on macOS 10.13

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

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

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
  

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
  

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

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