[PATCH 1/2] darwin: Silence syscall deprecated declaration warning

[PATCH 1/2] darwin: Silence syscall deprecated declaration warning

[Simon Marchi]

This patch silences this warning:

/Users/simark/src/binutils-gdb/gdb/darwin-nat.c:839:10: error: 'syscall' is deprecated: first deprecated in macOS 10.12 - syscall(2) is unsupported; please switch to a supported interface. For SYS_kdebug_trace use kdebug_signpost(). [-Werror,-Wdeprecated-declarations]
          res = syscall (SYS___pthread_kill, thread->gdb_port, nsignal);
                ^
/usr/include/unistd.h:745:6: note: 'syscall' has been explicitly marked deprecated here
int      syscall(int, ...);
         ^

I guess it would be good to find a non-deprecated alternative for
sending that signal to a specific thread, but I have not idea what we
could use instead (not sure if plain kill would do the trick).

gdb/ChangeLog:

	* darwin-nat.c (darwin_resume_thread): Silence syscall
	deprecated declaration warning.
---
 gdb/darwin-nat.c      | 3 +++
 include/diagnostics.h | 6 ++++++
 2 files changed, 9 insertions(+)

diff --git a/gdb/darwin-nat.c b/gdb/darwin-nat.c
index 8104de53e7f8..95b89aaae302 100644
--- a/gdb/darwin-nat.c
+++ b/gdb/darwin-nat.c
@@ -836,7 +836,10 @@ darwin_resume_thread (struct inferior *inf, darwin_thread_t *thread,
 	{
 	  /* Note: ptrace is allowed only if the process is stopped.
 	     Directly send the signal to the thread.  */
+	  DIAGNOSTIC_PUSH;
+	  DIAGNOSTIC_IGNORE_DEPRECATED_DECLARATIONS;
 	  res = syscall (SYS___pthread_kill, thread->gdb_port, nsignal);
+	  DIAGNOSTIC_POP;
 	  inferior_debug (4, _("darwin_resume_thread: kill 0x%x %d: %d\n"),
 			  thread->gdb_port, nsignal, res);
 	  thread->signaled = 1;
diff --git a/include/diagnostics.h b/include/diagnostics.h
index 4a674106dc01..34fc01b85bd4 100644
--- a/include/diagnostics.h
+++ b/include/diagnostics.h
@@ -35,6 +35,8 @@
 #if defined (__clang__) /* clang */
 
 # define DIAGNOSTIC_IGNORE_SELF_MOVE DIAGNOSTIC_IGNORE ("-Wself-move")
+# define DIAGNOSTIC_IGNORE_DEPRECATED_DECLARATIONS \
+  DIAGNOSTIC_IGNORE ("-Wdeprecated-declarations")
 # define DIAGNOSTIC_IGNORE_DEPRECATED_REGISTER \
   DIAGNOSTIC_IGNORE ("-Wdeprecated-register")
 # define DIAGNOSTIC_IGNORE_UNUSED_FUNCTION \
@@ -56,6 +58,10 @@
 # define DIAGNOSTIC_IGNORE_SELF_MOVE
 #endif
 
+#ifndef DIAGNOSTIC_IGNORE_DEPRECATED_DECLARATIONS
+# define DIAGNOSTIC_IGNORE_DEPRECATED_DECLARATIONS
+#endif
+
 #ifndef DIAGNOSTIC_IGNORE_DEPRECATED_REGISTER
 # define DIAGNOSTIC_IGNORE_DEPRECATED_REGISTER
 #endif
-- 
2.17.1

[PATCH 2/2] darwin: Don't use sbrk

[Simon Marchi]

This patch gets rid of this warning on macOS:

    CXX    main.o
  /Users/simark/src/binutils-gdb/gdb/main.c:492:27: error: 'sbrk' is deprecated [-Werror,-Wdeprecated-declarations]
    lim_at_start = (char *) sbrk (0);
                            ^
  /usr/include/unistd.h:585:1: note: 'sbrk' has been explicitly marked deprecated here
  __deprecated __WATCHOS_PROHIBITED __TVOS_PROHIBITED
  ^
  /usr/include/sys/cdefs.h:176:37: note: expanded from macro '__deprecated'
  #define __deprecated    __attribute__((deprecated))
                                         ^

sbrk on macOS is not useful for our purposes, since sbrk(0) always
returns the same value.  From what I read, brk/sbrk on macOS is just an
emulation, it always returns a pointer in a 4MB section reserved for
that.

So instead of letting users use "maint set per-command space on" and
print silly results, I think we should just disable that feature for
this platform (as we do for platforms that don't have sbrk).

I defined a HAVE_USEFUL_SBRK macro and used that instead of HAVE_SBRK.

gdb/ChangeLog:

	* common/common-defs.h (HAVE_USEFUL_SBRK): Define.
	* main.c: Use HAVE_USEFUL_SBRK instead of HAVE_SBRK.
	* maint.c: Likewise.
	* top.c: Likewise.
---
 gdb/common/common-defs.h | 4 ++++
 gdb/main.c               | 2 +-
 gdb/maint.c              | 4 ++--
 gdb/top.c                | 2 +-
 4 files changed, 8 insertions(+), 4 deletions(-)

diff --git a/gdb/common/common-defs.h b/gdb/common/common-defs.h
index eb0ec214926c..8209e13943d7 100644
--- a/gdb/common/common-defs.h
+++ b/gdb/common/common-defs.h
@@ -105,4 +105,8 @@
 /* String containing the current directory (what getwd would return).  */
 extern char *current_directory;
 
+#if defined (HAVE_SBRK) && !__APPLE__
+#define HAVE_USEFUL_SBRK 1
+#endif
+
 #endif /* COMMON_DEFS_H */
diff --git a/gdb/main.c b/gdb/main.c
index 9694af242699..e925128a4263 100644
--- a/gdb/main.c
+++ b/gdb/main.c
@@ -487,7 +487,7 @@ captured_main_1 (struct captured_main_args *context)
   int save_auto_load;
   struct objfile *objfile;
 
-#ifdef HAVE_SBRK
+#ifdef HAVE_USEFUL_SBRK
   /* Set this before constructing scoped_command_stats.  */
   lim_at_start = (char *) sbrk (0);
 #endif
diff --git a/gdb/maint.c b/gdb/maint.c
index a8a1fcbc2996..5d4701cfacca 100644
--- a/gdb/maint.c
+++ b/gdb/maint.c
@@ -828,7 +828,7 @@ scoped_command_stats::~scoped_command_stats ()
 
   if (m_space_enabled && per_command_space)
     {
-#ifdef HAVE_SBRK
+#ifdef HAVE_USEFUL_SBRK
       char *lim = (char *) sbrk (0);
 
       long space_now = lim - lim_at_start;
@@ -866,7 +866,7 @@ scoped_command_stats::scoped_command_stats (bool msg_type)
 {
   if (!m_msg_type || per_command_space)
     {
-#ifdef HAVE_SBRK
+#ifdef HAVE_USEFUL_SBRK
       char *lim = (char *) sbrk (0);
       m_start_space = lim - lim_at_start;
       m_space_enabled = 1;
diff --git a/gdb/top.c b/gdb/top.c
index fdef3e0d0bf2..de1a335e40a4 100644
--- a/gdb/top.c
+++ b/gdb/top.c
@@ -171,7 +171,7 @@ int remote_timeout = 2;
 int remote_debug = 0;
 
 /* Sbrk location on entry to main.  Used for statistics only.  */
-#ifdef HAVE_SBRK
+#ifdef HAVE_USEFUL_SBRK
 char *lim_at_start;
 #endif
 
-- 
2.17.1

Re: [PATCH 2/2] darwin: Don't use sbrk

[Pedro Alves]

On 07/04/2018 05:30 AM, Simon Marchi wrote:
> This patch gets rid of this warning on macOS:
> 

> sbrk on macOS is not useful for our purposes, since sbrk(0) always
> returns the same value.  From what I read, brk/sbrk on macOS is just an
> emulation, it always returns a pointer in a 4MB section reserved for
> that.
> 

> --- a/gdb/common/common-defs.h
> +++ b/gdb/common/common-defs.h
> @@ -105,4 +105,8 @@
>  /* String containing the current directory (what getwd would return).  */
>  extern char *current_directory;
>  

Suggest adding a comment here based on what you had in the intro:

/* sbrk on macOS is not useful for our purposes, since sbrk(0) always
   returns the same value.  brk/sbrk on macOS is just an emulation
   that always returns a pointer to a 4MB section reserved for
   that.  */

> +#if defined (HAVE_SBRK) && !__APPLE__
> +#define HAVE_USEFUL_SBRK 1
> +#endif
Thanks,
Pedro Alves

Re: [PATCH 2/2] darwin: Don't use sbrk

[Simon Marchi]

On 2018-07-04 06:49, Pedro Alves wrote:
> On 07/04/2018 05:30 AM, Simon Marchi wrote:
>> This patch gets rid of this warning on macOS:
>> 
> 
>> sbrk on macOS is not useful for our purposes, since sbrk(0) always
>> returns the same value.  From what I read, brk/sbrk on macOS is just 
>> an
>> emulation, it always returns a pointer in a 4MB section reserved for
>> that.
>> 
> 
>> --- a/gdb/common/common-defs.h
>> +++ b/gdb/common/common-defs.h
>> @@ -105,4 +105,8 @@
>>  /* String containing the current directory (what getwd would return). 
>>  */
>>  extern char *current_directory;
>> 
> 
> Suggest adding a comment here based on what you had in the intro:
> 
> /* sbrk on macOS is not useful for our purposes, since sbrk(0) always
>    returns the same value.  brk/sbrk on macOS is just an emulation
>    that always returns a pointer to a 4MB section reserved for
>    that.  */

Of course, that's a good idea.  Otherwise somebody will curse in 10 
years, wondering why we did this, just like we did for the #if 
__APPLE_CC__ in gettext :).  I'm pushing with that added, thanks for 
providing the comment.

Simon

Re: [PATCH 1/2] darwin: Silence syscall deprecated declaration warning

[Pedro Alves]

On 07/04/2018 05:30 AM, Simon Marchi wrote:
> This patch silences this warning:
> 
> /Users/simark/src/binutils-gdb/gdb/darwin-nat.c:839:10: error: 'syscall' is deprecated: first deprecated in macOS 10.12 - syscall(2) is unsupported; please switch to a supported interface. For SYS_kdebug_trace use kdebug_signpost(). [-Werror,-Wdeprecated-declarations]
>           res = syscall (SYS___pthread_kill, thread->gdb_port, nsignal);
>                 ^
> /usr/include/unistd.h:745:6: note: 'syscall' has been explicitly marked deprecated here
> int      syscall(int, ...);
>          ^
> 
> I guess it would be good to find a non-deprecated alternative for
> sending that signal to a specific thread, but I have not idea what we
> could use instead (not sure if plain kill would do the trick).

Plain kill probably would not, as that it not directed to a specific
thread -- any thread could dequeue it.

My immediate question when reading this was "why not use the
pthread_kill C function instead of calling the syscall directly?"

I then followed the rabbit down the hole, starting here:

 https://stackoverflow.com/questions/44990839/pthread-kill-to-a-gcd-managed-thread

which lead to pthread_kill's sources, here:

  https://github.com/apple/darwin-libpthread/blob/768e71947f16da454ea05c4a98f77a0f14358f21/src/pthread.c#L1412

So it's very likely we use the syscall directly instead of pthread_kill because
we want to be able to send a signal to all kinds of threads, including
worker threads underlying GCD.  I wish there was a comment to the effect here.

I also peeked at debugserver's sources in lldb, and it seems to me that
lldb doesn't send a signal to the thread unless it was stopped on a mach
exception?  

Actually, I wonder if that syscall in gdb is really ever reached...  We
shouldn't be calling darwin_resume_thread on non-resumed threads, AFAICT,
so why do we handle that?  See comment underlined below -- when isn't
the process stopped when we get there?

static void
darwin_resume_thread (struct inferior *inf, darwin_thread_t *thread,
		      int step, int nsignal)
{
..
  switch (thread->msg_state)
    {
    case DARWIN_MESSAGE:
      if (thread->event.ex_type == EXC_SOFTWARE
	  && thread->event.ex_data[0] == EXC_SOFT_SIGNAL)
	{
...
	}
      else if (nsignal)
	{
	  /* Note: ptrace is allowed only if the process is stopped.
             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
	     Directly send the signal to the thread.  */
	  res = syscall (SYS___pthread_kill, thread->gdb_port, nsignal);
          ...
	}

Thanks,
Pedro Alves

Re: [PATCH 1/2] darwin: Silence syscall deprecated declaration warning

[Simon Marchi]

On 2018-07-04 06:30, Pedro Alves wrote:
> Plain kill probably would not, as that it not directed to a specific
> thread -- any thread could dequeue it.
> 
> My immediate question when reading this was "why not use the
> pthread_kill C function instead of calling the syscall directly?"
> 
> I then followed the rabbit down the hole, starting here:
> 
> 
> https://stackoverflow.com/questions/44990839/pthread-kill-to-a-gcd-managed-thread
> 
> which lead to pthread_kill's sources, here:
> 
> 
> https://github.com/apple/darwin-libpthread/blob/768e71947f16da454ea05c4a98f77a0f14358f21/src/pthread.c#L1412
> 
> So it's very likely we use the syscall directly instead of pthread_kill 
> because
> we want to be able to send a signal to all kinds of threads, including
> worker threads underlying GCD.  I wish there was a comment to the 
> effect here.

Indeed, I saw that too.  I can only assume that this was Tristan's 
intention when writing the code.  I can add the comment if we're 
confident enough that it's the case.

> I also peeked at debugserver's sources in lldb, and it seems to me that
> lldb doesn't send a signal to the thread unless it was stopped on a 
> mach
> exception?
> 
> Actually, I wonder if that syscall in gdb is really ever reached...  We
> shouldn't be calling darwin_resume_thread on non-resumed threads, 
> AFAICT,
> so why do we handle that?  See comment underlined below -- when isn't
> the process stopped when we get there?
> 
> static void
> darwin_resume_thread (struct inferior *inf, darwin_thread_t *thread,
> 		      int step, int nsignal)
> {
> ..
>   switch (thread->msg_state)
>     {
>     case DARWIN_MESSAGE:
>       if (thread->event.ex_type == EXC_SOFTWARE
> 	  && thread->event.ex_data[0] == EXC_SOFT_SIGNAL)
> 	{
> ...
> 	}
>       else if (nsignal)
> 	{
> 	  /* Note: ptrace is allowed only if the process is stopped.
>              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> 	     Directly send the signal to the thread.  */
> 	  res = syscall (SYS___pthread_kill, thread->gdb_port, nsignal);
>           ...
> 	}

Just guessing here.  It looks like when the thread generates mach 
exception (breakpoint, unix signal, bad instruction, etc), it "sends" a 
message (in Darwin jargon) and is stuck waiting for a reply.  GDB needs 
to reply to it to unblock the thread (the call to darwin_send_reply).  
In this case, the thread might be stopped from the point of view of the 
user and GDB, but not from the point of view of the OS.  So that's why, 
if we want to inject a signal, we have to use __pthread_kill.  I don't 
know why the (ex_type == EXC_SOFTWARE && ex_data[0] == EXC_SOFT_SIGNAL) 
is treated differently though.

In any case, changing this would require a deeper analysis and knowledge 
of the Darwin kernel and some time, and I don't have either :).  Would 
you still be fine with silencing the warning for now to get it out of 
the way?

Simon

Re: [PATCH 1/2] darwin: Silence syscall deprecated declaration warning

[Pedro Alves]

On 07/04/2018 05:39 PM, Simon Marchi wrote:
> On 2018-07-04 06:30, Pedro Alves wrote:
>> Plain kill probably would not, as that it not directed to a specific
>> thread -- any thread could dequeue it.
>>
>> My immediate question when reading this was "why not use the
>> pthread_kill C function instead of calling the syscall directly?"
>>
>> I then followed the rabbit down the hole, starting here:
>>
>>
>> https://stackoverflow.com/questions/44990839/pthread-kill-to-a-gcd-managed-thread
>>
>> which lead to pthread_kill's sources, here:
>>
>>
>> https://github.com/apple/darwin-libpthread/blob/768e71947f16da454ea05c4a98f77a0f14358f21/src/pthread.c#L1412
>>
>> So it's very likely we use the syscall directly instead of pthread_kill because
>> we want to be able to send a signal to all kinds of threads, including
>> worker threads underlying GCD.  I wish there was a comment to the effect here.
> 
> Indeed, I saw that too.  I can only assume that this was Tristan's intention when writing the code.  I can add the comment if we're confident enough that it's the case.

I'm reasonably confident.

> 
>> I also peeked at debugserver's sources in lldb, and it seems to me that
>> lldb doesn't send a signal to the thread unless it was stopped on a mach
>> exception?
>>
>> Actually, I wonder if that syscall in gdb is really ever reached...  We
>> shouldn't be calling darwin_resume_thread on non-resumed threads, AFAICT,
>> so why do we handle that?  See comment underlined below -- when isn't
>> the process stopped when we get there?
>>
>> static void
>> darwin_resume_thread (struct inferior *inf, darwin_thread_t *thread,
>>               int step, int nsignal)
>> {
>> ..
>>   switch (thread->msg_state)
>>     {
>>     case DARWIN_MESSAGE:
>>       if (thread->event.ex_type == EXC_SOFTWARE
>>       && thread->event.ex_data[0] == EXC_SOFT_SIGNAL)
>>     {
>> ...
>>     }
>>       else if (nsignal)
>>     {
>>       /* Note: ptrace is allowed only if the process is stopped.
>>              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>>          Directly send the signal to the thread.  */
>>       res = syscall (SYS___pthread_kill, thread->gdb_port, nsignal);
>>           ...
>>     }
> 
> Just guessing here.  It looks like when the thread generates mach exception (breakpoint, unix signal, bad instruction, etc), it "sends" a message (in Darwin jargon) and is stuck waiting for a reply.  GDB needs to reply to it to unblock the thread (the call to darwin_send_reply).  In this case, the thread might be stopped from the point of view of the user and GDB, but not from the point of view of the OS.  So that's why, if we want to inject a signal, we have to use __pthread_kill.  I don't know why the (ex_type == EXC_SOFTWARE && ex_data[0] == EXC_SOFT_SIGNAL) is treated differently though.

If a thread was suspended by GDB, with task_suspend, I'd assume
that the thread's msg_state would not be DARWIN_MESSAGE, so we wouldn't
reach here.  But just guessing too.

> 
> In any case, changing this would require a deeper analysis and knowledge of the Darwin kernel and some time, and I don't have either :).  Would you still be fine with silencing the warning for now to get it out of the way?
Yes, of course.  I'd suggest wrapping this in a function though
(darwin_pthread_kill or some such), so that the silencing
would be contained there out of view.  We can also mention why we
need the function in its intro comment too.

Thanks,
Pedro Alves