[RFA] Make macOS build warning-free

[RFA] Make macOS build warning-free

[Tom Tromey]

I noticed thousands of warnings, mostly from the _() macro, when
building on macOS.  This patch makes the macOS build be warning-free,
without requiring --disable-nls.

The warning.m4 change should be self-explanatory.

The symfile.c change fixes a clang warning coming from
-Wmissing-braces.

Tested by rebuilding on macOS and on x86-64 Fedora 28.

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

	* symfile.c (set_objfile_default_section_offset): Use extra braces
	around initializer.
	* warning.m4 (AM_GDB_WARNINGS): Use -Wno-deprecated-declarations,
	-Wno-format-nonliteral, and -Wno-format-security on *-*-darwin*.
	* configure: Rebuild.
---
 gdb/ChangeLog  |  8 ++++++++
 gdb/configure  | 11 +++++++++++
 gdb/symfile.c  |  2 +-
 gdb/warning.m4 | 11 +++++++++++
 4 files changed, 31 insertions(+), 1 deletion(-)

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 4c04d0ba728..df573d35394 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,11 @@
+2018-06-29  Tom Tromey  <tom@tromey.com>
+
+	* symfile.c (set_objfile_default_section_offset): Use extra braces
+	around initializer.
+	* warning.m4 (AM_GDB_WARNINGS): Use -Wno-deprecated-declarations,
+	-Wno-format-nonliteral, and -Wno-format-security on *-*-darwin*.
+	* configure: Rebuild.
+
 2018-06-28  Tom Tromey  <tom@tromey.com>
 
 	* NEWS: Mention --enable-codesign.
diff --git a/gdb/configure b/gdb/configure
index 28756ed9826..a4d0d0a6cda 100755
--- a/gdb/configure
+++ b/gdb/configure
@@ -15488,6 +15488,17 @@ case "${host}" in
     build_warnings="$build_warnings -Wno-unknown-pragmas"
     # Solaris 11 <unistd.h> marks vfork deprecated.
     build_warnings="$build_warnings -Wno-deprecated-declarations" ;;
+  *-*-darwin*)
+    # macOS deprecates syscall (needed by darwin-nat.c) and
+    # sbrk (used only for some maint commands).
+    build_warnings="$build_warnings -Wno-deprecated-declarations"
+    # -Wformat-nonliteral doesn't work properly on macOS -- apparently
+    # it does not interact properly with the format_arg attribute.
+    # (libgnuintl.h disables this attribute for macOS, but re-enabling
+    # it there did not work.)
+    build_warnings="$build_warnings -Wno-format-nonliteral"
+    build_warnings="$build_warnings -Wno-format-security"
+    ;;
   *) build_warnings="$build_warnings -Wformat-nonliteral" ;;
 esac
 
diff --git a/gdb/symfile.c b/gdb/symfile.c
index 48eca5cc0ea..62b38bd6182 100644
--- a/gdb/symfile.c
+++ b/gdb/symfile.c
@@ -2088,7 +2088,7 @@ set_objfile_default_section_offset (struct objfile *objf,
 {
   /* Add OFFSET to all sections by default.  */
   std::vector<struct section_offsets> offsets (objf->num_sections,
-					       { offset });
+					       { { offset } });
 
   /* Create sorted lists of all sections in ADDRS as well as all
      sections in OBJF.  */
diff --git a/gdb/warning.m4 b/gdb/warning.m4
index 632cc214ac0..17afc5455a1 100644
--- a/gdb/warning.m4
+++ b/gdb/warning.m4
@@ -58,6 +58,17 @@ case "${host}" in
     build_warnings="$build_warnings -Wno-unknown-pragmas"
     # Solaris 11 <unistd.h> marks vfork deprecated.
     build_warnings="$build_warnings -Wno-deprecated-declarations" ;;
+  *-*-darwin*)
+    # macOS deprecates syscall (needed by darwin-nat.c) and
+    # sbrk (used only for some maint commands).
+    build_warnings="$build_warnings -Wno-deprecated-declarations"
+    # -Wformat-nonliteral doesn't work properly on macOS -- apparently
+    # it does not interact properly with the format_arg attribute.
+    # (libgnuintl.h disables this attribute for macOS, but re-enabling
+    # it there did not work.)
+    build_warnings="$build_warnings -Wno-format-nonliteral"
+    build_warnings="$build_warnings -Wno-format-security"
+    ;;
   *) build_warnings="$build_warnings -Wformat-nonliteral" ;;
 esac
 
-- 
2.17.1

Re: [RFA] Make macOS build warning-free

[Simon Marchi]

On 2018-06-29 12:54 PM, Tom Tromey wrote:
> diff --git a/gdb/warning.m4 b/gdb/warning.m4
> index 632cc214ac0..17afc5455a1 100644
> --- a/gdb/warning.m4
> +++ b/gdb/warning.m4
> @@ -58,6 +58,17 @@ case "${host}" in
>      build_warnings="$build_warnings -Wno-unknown-pragmas"
>      # Solaris 11 <unistd.h> marks vfork deprecated.
>      build_warnings="$build_warnings -Wno-deprecated-declarations" ;;
> +  *-*-darwin*)
> +    # macOS deprecates syscall (needed by darwin-nat.c) and
> +    # sbrk (used only for some maint commands).
> +    build_warnings="$build_warnings -Wno-deprecated-declarations"
> +    # -Wformat-nonliteral doesn't work properly on macOS -- apparently
> +    # it does not interact properly with the format_arg attribute.
> +    # (libgnuintl.h disables this attribute for macOS, but re-enabling
> +    # it there did not work.)
> +    build_warnings="$build_warnings -Wno-format-nonliteral"
> +    build_warnings="$build_warnings -Wno-format-security"
> +    ;;
>    *) build_warnings="$build_warnings -Wformat-nonliteral" ;;
>  esac

About the gettext / non-literal warning, this is the patch I carry.  I tried
to dig up why gettext doesn't define _INTL_MAY_RETURN_STRING_ARG for __APPLE_CC__
but couldn't find it.  My guess is that in a distant past, the Apple compiler
did not know about that attribute.  You need another change to get rid of the warning
though.  Either:

1. Apply _INTL_MAY_RETURN_STRING_ARG to the gettext function (see patch below)
2. Remove the other __APPLE_CC__ guard that protects the _INTL_REDIRECT_ASM definition.
   It will make it so the used gettext declaration will be the one at line 138, which
   already has _INTL_MAY_RETURN_STRING_ARG.

Here's the diff I've been carrying for a while:

From c40fdef23fb3c85e94454bb2e3e9559df02cb43c Mon Sep 17 00:00:00 2001
From: Simon Marchi <simon.marchi@polymtl.ca>
Date: Fri, 29 Jun 2018 17:20:06 -0400
Subject: [PATCH 1/2] Fix gettext warnings

---
 intl/libgnuintl.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/intl/libgnuintl.h b/intl/libgnuintl.h
index acc9093a9d..aae7947b84 100644
--- a/intl/libgnuintl.h
+++ b/intl/libgnuintl.h
@@ -115,7 +115,7 @@ extern "C" {
 /* _INTL_MAY_RETURN_STRING_ARG(n) declares that the given function may return
    its n-th argument literally.  This enables GCC to warn for example about
    printf (gettext ("foo %y")).  */
-#if __GNUC__ >= 3 && !(__APPLE_CC__ > 1 && defined __cplusplus)
+#if __GNUC__ >= 3
 # define _INTL_MAY_RETURN_STRING_ARG(n) __attribute__ ((__format_arg__ (n)))
 #else
 # define _INTL_MAY_RETURN_STRING_ARG(n)
@@ -127,7 +127,7 @@ extern "C" {
 #ifdef _INTL_REDIRECT_INLINE
 extern char *libintl_gettext (const char *__msgid)
        _INTL_MAY_RETURN_STRING_ARG (1);
-static inline char *gettext (const char *__msgid)
+static inline _INTL_MAY_RETURN_STRING_ARG (1) char *gettext (const char *__msgid)
 {
   return libintl_gettext (__msgid);
 }
-- 
2.13.1


Simon

Re: [RFA] Make macOS build warning-free

[Tom Tromey]

>>>>> "Simon" == Simon Marchi <simon.marchi@ericsson.com> writes:

Simon> About the gettext / non-literal warning, this is the patch I carry.  I tried
Simon> to dig up why gettext doesn't define _INTL_MAY_RETURN_STRING_ARG for __APPLE_CC__
Simon> but couldn't find it.  My guess is that in a distant past, the Apple compiler
Simon> did not know about that attribute.  You need another change to get rid of the warning
Simon> though.

Thanks.

I looked into this a little more.  It seems like a huge effort, because,
first, upstream doesn't have this fix; and second, it seems like it
would have to be imported into gcc first.

Fixing this upstream would probably require the same thing for other
functions, not just gettext.

I think in the meantime I'll just --disable-nls.  Perhaps you wouldn't
mind approving the symfile.c change in isolation.

Tom

Re: [RFA] Make macOS build warning-free

[Simon Marchi]

On 2018-07-02 10:58 AM, Tom Tromey wrote:
>>>>>> "Simon" == Simon Marchi <simon.marchi@ericsson.com> writes:
> 
> Simon> About the gettext / non-literal warning, this is the patch I carry.  I tried
> Simon> to dig up why gettext doesn't define _INTL_MAY_RETURN_STRING_ARG for __APPLE_CC__
> Simon> but couldn't find it.  My guess is that in a distant past, the Apple compiler
> Simon> did not know about that attribute.  You need another change to get rid of the warning
> Simon> though.
> 
> Thanks.
> 
> I looked into this a little more.  It seems like a huge effort, because,
> first, upstream doesn't have this fix; and second, it seems like it
> would have to be imported into gcc first.
> 
> Fixing this upstream would probably require the same thing for other
> functions, not just gettext.

_INTL_MAY_RETURN_STRING_ARG thing is a gcc/binutils/gdb addition, on top of the
v0.12.1 version of gettext.  So we shouldn't have to deal with upstream gettext
to fix this, since it's just the gcc/binutils/gdb addition that needs to be fixed.
Getting the fixed accepted in gcc shouldn't take took long.

> I think in the meantime I'll just --disable-nls.  Perhaps you wouldn't
> mind approving the symfile.c change in isolation.

That bit looks good to me, at least to shut up the compiler.  But I'm wondering
if we still need that section_offsets structure, or if we could just replace
it with std::vector<CORE_ADDR>/gdb:array_view<CORE_ADDR>.

Simon

Re: [RFA] Make macOS build warning-free

[Tom Tromey]

>>>>> "Simon" == Simon Marchi <simark@simark.ca> writes:

Simon> _INTL_MAY_RETURN_STRING_ARG thing is a gcc/binutils/gdb addition, on top of the
Simon> v0.12.1 version of gettext.  So we shouldn't have to deal with upstream gettext
Simon> to fix this, since it's just the gcc/binutils/gdb addition that needs to be fixed.

It's upstream as well though.  Maybe it was upstreamed from gcc to
gettext?  I don't really know the history there.

I'll send a note to the gettext list.

Simon> That bit looks good to me, at least to shut up the compiler.  But I'm wondering
Simon> if we still need that section_offsets structure, or if we could just replace
Simon> it with std::vector<CORE_ADDR>/gdb:array_view<CORE_ADDR>.

I think it's a good idea to rewrite section_offsets, but that seems more involved.
Also the way that this code is using std::vector and then turning it
into a section_offsets* seems incorrect.

I think the symfile.c change isn't enough, btw; the
-Wno-deprecated-declarations bit is also required.  Otherwise there is at
least one error (about the use of syscall).  But maybe I could disable that
one with a #pragma instead - what do you think?

Tom

Re: [RFA] Make macOS build warning-free

[Simon Marchi]

On 2018-07-03 09:17 AM, Tom Tromey wrote:
>>>>>> "Simon" == Simon Marchi <simark@simark.ca> writes:
> 
> Simon> _INTL_MAY_RETURN_STRING_ARG thing is a gcc/binutils/gdb addition, on top of the
> Simon> v0.12.1 version of gettext.  So we shouldn't have to deal with upstream gettext
> Simon> to fix this, since it's just the gcc/binutils/gdb addition that needs to be fixed.
> 
> It's upstream as well though.  Maybe it was upstreamed from gcc to
> gettext?  I don't really know the history there.
> 
> I'll send a note to the gettext list.

Ah ok I see, it is present in later versions of gettext.  I will follow the discussion on the
gettext mailing to see where it goes.

Grepping and git-blaming the clang source code, it looks like clang has supported the attribute
since at least 2009, and clang has been the default compiler on Mac since about that time too.
So it should be safe to just remove the __APPLE_CC__ #if completely.  If this is fixed upstream,
I think we could just mirror the change in our local copy of libgnuintl.h, without importing a
whole new version of gettext (which would probably be a lot of work).

> Simon> That bit looks good to me, at least to shut up the compiler.  But I'm wondering
> Simon> if we still need that section_offsets structure, or if we could just replace
> Simon> it with std::vector<CORE_ADDR>/gdb:array_view<CORE_ADDR>.
> 
> I think it's a good idea to rewrite section_offsets, but that seems more involved.
> Also the way that this code is using std::vector and then turning it
> into a section_offsets* seems incorrect.

Indeed.  I started a prototype, it uncovered an issue about reread_symbol accessing
objfile::section_offsets out of bounds, so I'm stuck on that.

> I think the symfile.c change isn't enough, btw; the
> -Wno-deprecated-declarations bit is also required.  Otherwise there is at
> least one error (about the use of syscall).  But maybe I could disable that
> one with a #pragma instead - what do you think?

About this part, I would prefer if we disabled the warning at the call sites
instead of disabling the warning across the board, since the warning could be relevant
sometimes.  Or maybe we should look at using a non-deprecated alternative...

Here's the other patch I've been carrying.  If you think it's good, I can finish
it and push it, or you can steal it.


From e458931de993f87702384591701b21edf8299279 Mon Sep 17 00:00:00 2001
From: Simon Marchi <simon.marchi@polymtl.ca>
Date: Sun, 25 Jun 2017 15:17:43 -0400
Subject: [PATCH 2/2] Ignore deprecation of sbrk and syscall

---
 gdb/darwin-nat.c      | 3 +++
 gdb/main.c            | 3 +++
 gdb/maint.c           | 6 ++++++
 include/diagnostics.h | 6 ++++++
 4 files changed, 18 insertions(+)

diff --git a/gdb/darwin-nat.c b/gdb/darwin-nat.c
index 7dccce7392..b46d8b5632 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/gdb/main.c b/gdb/main.c
index 9694af2426..0c597685f5 100644
--- a/gdb/main.c
+++ b/gdb/main.c
@@ -488,8 +488,11 @@ captured_main_1 (struct captured_main_args *context)
   struct objfile *objfile;

 #ifdef HAVE_SBRK
+DIAGNOSTIC_PUSH
+DIAGNOSTIC_IGNORE_DEPRECATED_DECLARATIONS
   /* Set this before constructing scoped_command_stats.  */
   lim_at_start = (char *) sbrk (0);
+DIAGNOSTIC_POP
 #endif

   scoped_command_stats stat_reporter (false);
diff --git a/gdb/maint.c b/gdb/maint.c
index a8a1fcbc29..0b3a3e04e6 100644
--- a/gdb/maint.c
+++ b/gdb/maint.c
@@ -829,7 +829,10 @@ scoped_command_stats::~scoped_command_stats ()
   if (m_space_enabled && per_command_space)
     {
 #ifdef HAVE_SBRK
+DIAGNOSTIC_PUSH
+DIAGNOSTIC_IGNORE_DEPRECATED_DECLARATIONS
       char *lim = (char *) sbrk (0);
+DIAGNOSTIC_POP

       long space_now = lim - lim_at_start;
       long space_diff = space_now - m_start_space;
@@ -867,7 +870,10 @@ scoped_command_stats::scoped_command_stats (bool msg_type)
   if (!m_msg_type || per_command_space)
     {
 #ifdef HAVE_SBRK
+DIAGNOSTIC_PUSH
+DIAGNOSTIC_IGNORE_DEPRECATED_DECLARATIONS
       char *lim = (char *) sbrk (0);
+DIAGNOSTIC_POP
       m_start_space = lim - lim_at_start;
       m_space_enabled = 1;
 #endif
diff --git a/include/diagnostics.h b/include/diagnostics.h
index 4a674106dc..ecf77969f9 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 \
@@ -72,4 +74,8 @@
 # define DIAGNOSTIC_IGNORE_STRINGOP_TRUNCATION
 #endif

+#ifndef DIAGNOSTIC_IGNORE_DEPRECATED_DECLARATIONS
+# define DIAGNOSTIC_IGNORE_DEPRECATED_DECLARATIONS
+#endif
+
 #endif /* DIAGNOSTICS_H */
-- 
2.18.0

Re: [RFA] Make macOS build warning-free

[Tom Tromey]

>>>>> "Simon" == Simon Marchi <simon.marchi@ericsson.com> writes:

Simon> Indeed.  I started a prototype, it uncovered an issue about reread_symbol accessing
Simon> objfile::section_offsets out of bounds, so I'm stuck on that.

That sounds bad!

>> I think the symfile.c change isn't enough, btw; the
>> -Wno-deprecated-declarations bit is also required.  Otherwise there is at
>> least one error (about the use of syscall).  But maybe I could disable that
>> one with a #pragma instead - what do you think?

Simon> About this part, I would prefer if we disabled the warning at the call sites
Simon> instead of disabling the warning across the board, since the warning could be relevant
Simon> sometimes.  Or maybe we should look at using a non-deprecated alternative...

Simon> Here's the other patch I've been carrying.  If you think it's good, I can finish
Simon> it and push it, or you can steal it.

Simon> +	  DIAGNOSTIC_PUSH
Simon> +	  DIAGNOSTIC_IGNORE_DEPRECATED_DECLARATIONS
Simon>  	  res = syscall (SYS___pthread_kill, thread->gdb_port, nsignal);
Simon> +	  DIAGNOSTIC_POP

I like it but I wonder if it would work with ";" on those lines.
That would make indentation work more nicely in Emacs :)

Simon>  #ifdef HAVE_SBRK
Simon> +DIAGNOSTIC_PUSH
Simon> +DIAGNOSTIC_IGNORE_DEPRECATED_DECLARATIONS
Simon>    /* Set this before constructing scoped_command_stats.  */
Simon>    lim_at_start = (char *) sbrk (0);
Simon> +DIAGNOSTIC_POP
Simon>  #endif

Perhaps we need an sbrk wrapper so we can limit the damage to a single spot.

Tom

Re: [RFA] Make macOS build warning-free

[Tom Tromey]

>>>>> "Simon" == Simon Marchi <simark@simark.ca> writes:

>> I think in the meantime I'll just --disable-nls.  Perhaps you wouldn't
>> mind approving the symfile.c change in isolation.

Simon> That bit looks good to me, at least to shut up the compiler.

I forgot about this for a while, but here is the patch I am checking in.

Tom

commit 027a4c30d03e3f434ddfa61e7f026e3b15f83daa
Author: Tom Tromey <tom@tromey.com>
Date:   Fri Jul 13 11:14:57 2018 -0600

    Remove a warning on macOS
    
    This removes a clang warning coming from -Wmissing-braces.
    
    ChangeLog
    2018-07-13  Tom Tromey  <tom@tromey.com>
    
            * symfile.c (set_objfile_default_section_offset): Use extra braces
            around initializer.

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 7b808eb3a76..66b7c3681e2 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,8 @@
+2018-07-13  Tom Tromey  <tom@tromey.com>
+
+	* symfile.c (set_objfile_default_section_offset): Use extra braces
+	around initializer.
+
 2018-07-13  Andreas Arnez  <arnez@linux.vnet.ibm.com>
 
 	* s390-tdep.c (s390_displaced_step_fixup): Adjust PC for a
diff --git a/gdb/symfile.c b/gdb/symfile.c
index 48eca5cc0ea..62b38bd6182 100644
--- a/gdb/symfile.c
+++ b/gdb/symfile.c
@@ -2088,7 +2088,7 @@ set_objfile_default_section_offset (struct objfile *objf,
 {
   /* Add OFFSET to all sections by default.  */
   std::vector<struct section_offsets> offsets (objf->num_sections,
-					       { offset });
+					       { { offset } });
 
   /* Create sorted lists of all sections in ADDRS as well as all
      sections in OBJF.  */