27bb4de72b * Port cleanup attribute to Oracle Studio 12.5

27bb4de72b * Port cleanup attribute to Oracle Studio 12.5

[Philipp Stephani]

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

Hi,

the commit message says "The C compiler should check the cleanup attribute
in the next line anyway." But that's not the case: unknown attributes are
silently ignored, at least in Clang. The verify(__has_attribute(cleanup))
or equivalent is absolutely required here. Can we revert that commit?

[-- Attachment #2: Type: text/html, Size: 358 bytes --]

Re: 27bb4de72b * Port cleanup attribute to Oracle Studio 12.5

[Paul Eggert]

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

On 06/14/2017 03:29 AM, Philipp Stephani wrote:
>
> the commit message says "The C compiler should check the cleanup 
> attribute in the next line anyway." But that's not the case: unknown 
> attributes are silently ignored, at least in Clang. The 
> verify(__has_attribute(cleanup)) or equivalent is absolutely required 
> here. Can we revert that commit?

No, because the 'verify' breaks the build with Oracle Studio 12.5, where 
__has_attribute works only inside preprocessor conditionals. I installed 
the attached patch, which checks for __attribute__ (cleanup) in a 
different way. But while we're on the subject, wouldn't it be better if 
emacs-module.c were made to work (albeit perhaps less efficiently) even 
on compilers that do not support this nonstandard C extension?

Another thing. Have you had a chance to think about related questions I 
asked about recently installed portability changes? Here are the URLs:

http://lists.gnu.org/archive/html/emacs-devel/2017-06/msg00225.html

http://lists.gnu.org/archive/html/emacs-devel/2017-06/msg00226.html

Since then I see that you installed another patch (commit 
32d8dba625fc50ccbe28e35afcf1f0529d611e00) to pacify Clang on macOS; this 
patch unfortunately could cause trouble on non-POSIX platforms where 
rlim_t is signed. Pacifying Clang shouldn't be at the cost of 
portability or unnecessary complexity.

And I'm still puzzled as to why you're getting the Clang warnings but I 
am not. Are you using an older Clang? Are you passing it extra warning 
options?


[-- Attachment #2: 0001-Port-cleanup-check-to-Oracle-Studio-12.5.patch --]
[-- Type: text/x-patch, Size: 1982 bytes --]

From 80f8f15b5e9d6776948adedc8b1ab25c2a41df6e Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Wed, 14 Jun 2017 11:01:02 -0700
Subject: [PATCH] Port cleanup check to Oracle Studio 12.5

* src/conf_post.h (__has_attribute_cleanup): Resurrect.
* src/emacs-module.c: Verify __has_attribute (cleanup), but in an
#if this time.
---
 src/conf_post.h    | 5 ++++-
 src/emacs-module.c | 4 ++++
 2 files changed, 8 insertions(+), 1 deletion(-)

diff --git a/src/conf_post.h b/src/conf_post.h
index 18b096e..e1d6a93 100644
--- a/src/conf_post.h
+++ b/src/conf_post.h
@@ -57,10 +57,13 @@ typedef bool bool_bf;
 #endif
 
 /* Simulate __has_attribute on compilers that lack it.  It is used only
-   on arguments like alloc_size that are handled in this simulation.  */
+   on arguments like alloc_size that are handled in this simulation.
+   __has_attribute should be used only in #if expressions, as Oracle
+   Studio 12.5's __has_attribute does not work in plain code.  */
 #ifndef __has_attribute
 # define __has_attribute(a) __has_attribute_##a
 # define __has_attribute_alloc_size GNUC_PREREQ (4, 3, 0)
+# define __has_attribute_cleanup GNUC_PREREQ (3, 4, 0)
 # define __has_attribute_externally_visible GNUC_PREREQ (4, 1, 0)
 # define __has_attribute_no_address_safety_analysis false
 # define __has_attribute_no_sanitize_address GNUC_PREREQ (4, 8, 0)
diff --git a/src/emacs-module.c b/src/emacs-module.c
index de62329..5c413ee 100644
--- a/src/emacs-module.c
+++ b/src/emacs-module.c
@@ -170,6 +170,10 @@ static struct emacs_env_private global_env_private;
 		   internal_handler_##handlertype,			       \
 		   internal_cleanup_##handlertype)
 
+#if !__has_attribute (cleanup)
+ #error "__attribute__ ((cleanup)) not supported by this compiler; try GCC"
+#endif
+
 /* It is very important that pushing the handler doesn't itself raise
    a signal.  Install the cleanup only after the handler has been
    pushed.  Use __attribute__ ((cleanup)) to avoid
-- 
2.9.4

Re: 27bb4de72b * Port cleanup attribute to Oracle Studio 12.5

[Philipp Stephani]

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

Paul Eggert <eggert@cs.ucla.edu> schrieb am Mi., 14. Juni 2017 um 20:26 Uhr:

> On 06/14/2017 03:29 AM, Philipp Stephani wrote:
> >
> > the commit message says "The C compiler should check the cleanup
> > attribute in the next line anyway." But that's not the case: unknown
> > attributes are silently ignored, at least in Clang. The
> > verify(__has_attribute(cleanup)) or equivalent is absolutely required
> > here. Can we revert that commit?
>
> No, because the 'verify' breaks the build with Oracle Studio 12.5, where
> __has_attribute works only inside preprocessor conditionals. I installed
> the attached patch, which checks for __attribute__ (cleanup) in a
> different way.


Thanks.


> But while we're on the subject, wouldn't it be better if
> emacs-module.c were made to work (albeit perhaps less efficiently) even
> on compilers that do not support this nonstandard C extension?
>

I've considered a couple of options. I think the simplest and most portable
one would be to compile as C++, which has destructors built into the
language.
If that's not possible for whatever reason, we could introduce a macro that
would create an entire wrapper function, which could implement the cleanup
as normal C function. That would require some preprocessor metaprogramming,
though, or more boilerplate.
Alternatively we could inline most of the macros and require discipline
from the developers editing that code.


>
> Another thing. Have you had a chance to think about related questions I
> asked about recently installed portability changes? Here are the URLs:
>

Yes, sorry for the slow response, I'm working through these now.


>
> http://lists.gnu.org/archive/html/emacs-devel/2017-06/msg00225.html
>
> http://lists.gnu.org/archive/html/emacs-devel/2017-06/msg00226.html
>
> Since then I see that you installed another patch (commit
> 32d8dba625fc50ccbe28e35afcf1f0529d611e00) to pacify Clang on macOS; this
> patch unfortunately could cause trouble on non-POSIX platforms where
> rlim_t is signed. Pacifying Clang shouldn't be at the cost of
> portability or unnecessary complexity.
>

Agreed. I wasn't aware of such platforms.
How about using #pragma clang diagnostic push/pop/ignore to ignore the
warnings in the specific statements where they arise and we know that they
are false positives? I'd much prefer that over disabling them globally in
configure, because most of the time the warnings are useful.


>
> And I'm still puzzled as to why you're getting the Clang warnings but I
> am not. Are you using an older Clang? Are you passing it extra warning
> options?
>
>
I'm using the Apple fork on macOS. It's mostly identical to upstream Clang
and compiles Emacs just fine, but it is a fork and not 100% identical. I
also get some of the warnings only when building with -O3 (haven't checked
other optimization levels).

[-- Attachment #2: Type: text/html, Size: 4154 bytes --]

Re: 27bb4de72b * Port cleanup attribute to Oracle Studio 12.5

[Paul Eggert]

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

On 06/15/2017 05:54 AM, Philipp Stephani wrote:
> I've considered a couple of options. I think the simplest and most 
> portable one would be to compile as C++, which has destructors built 
> into the language.

That would require some autoconf and makefile hacking, but it would 
work. That is, if modules are enabled, and __attribute__ ((cleanup)) is 
not available but a C++ compiler is available, then the build process 
could use the C++ compiler. Admittedly this is low priority.

> How about using #pragma clang diagnostic push/pop/ignore to ignore the 
> warnings in the specific statements where they arise and we know that 
> they are false positives? I'd much prefer that over disabling them 
> globally in configure, because most of the time the warnings are useful.

Although many warnings are useful, in my experience these particular 
warnings are not useful for Emacs. That is, the hassle they cause by 
false alarms costs more than the benefits of actual bugs that they fix. 
It's OK to disable such warnings globally.

>
>     And I'm still puzzled as to why you're getting the Clang warnings
>     but I
>     am not. Are you using an older Clang? Are you passing it extra warning
>     options?
>
>
> I'm using the Apple fork on macOS. It's mostly identical to upstream 
> Clang and compiles Emacs just fine, but it is a fork and not 100% 
> identical. I also get some of the warnings only when building with -O3 
> (haven't checked other optimization levels).

I just now applied the attached patch, configured with:

./configure --enable-gcc-warnings CC=clang CFLAGS='-O3'

and had no problems on Fedora 25. If you have a problem with this patch, 
can you please send relevant output from 'clang --version' and 'make 
V=1'? Here's the output for me:

clang version 3.9.1 (tags/RELEASE_391/final)
Target: x86_64-unknown-linux-gnu
Thread model: posix
InstalledDir: /usr/bin

make -C lib all
make[1]: Entering directory '/home/eggert/src/gnu/emacs/static-checking/lib'
clang -c    -MMD -MF deps/strftime.d -MP -fno-common -W -Wabi -Waddress 
-Wall -Wattributes -Wbuiltin-macro-redefined -Wchar-subscripts -Wcomment 
-Wcomments -Wdangling-else -Wdate-time -Wdeprecated 
-Wdeprecated-declarations -Wdisabled-optimization -Wdiv-by-zero 
-Wduplicate-decl-specifier -Wempty-body -Wendif-labels -Wenum-compare 
-Wexpansion-to-defined -Wextra -Wformat-extra-args -Wformat-security 
-Wformat-y2k -Wformat-zero-length -Wignored-attributes 
-Wignored-qualifiers -Wimplicit -Wimplicit-function-declaration 
-Wimplicit-int -Wincompatible-pointer-types -Winit-self -Wint-conversion 
-Wint-to-pointer-cast -Winvalid-pch -Wlogical-not-parentheses -Wmain 
-Wmissing-declarations -Wmissing-include-dirs -Wmissing-prototypes 
-Wmultichar -Wnarrowing -Wnested-externs -Wnonnull -Wnull-dereference 
-Wodr -Wold-style-definition -Woverflow -Wpacked -Wparentheses 
-Wpointer-arith -Wpointer-sign -Wpointer-to-int-cast -Wpragmas 
-Wreturn-type -Wsequence-point -Wshift-count-negative 
-Wshift-count-overflow -Wshift-negative-value -Wsizeof-array-argument 
-Wsizeof-pointer-memaccess -Wstrict-aliasing -Wstrict-prototypes 
-Wswitch-bool -Wtautological-compare -Wtrigraphs -Wuninitialized 
-Wunknown-pragmas -Wunused -Wunused-function -Wunused-label 
-Wunused-local-typedefs -Wunused-result -Wunused-value -Wunused-variable 
-Wvarargs -Wvariadic-macros -Wvolatile-register-var -Wwrite-strings 
-Wno-missing-field-initializers -Wredundant-decls 
-Wno-missing-field-initializers -Wno-sign-compare -Wno-type-limits 
-Wno-unused-parameter -Wno-format-nonliteral -Wno-missing-braces 
-Wno-tautological-compare 
-Wno-tautological-constant-out-of-range-compare -Werror  -O3 -I. 
-I../src -I. -I./../src  strftime.c
[no warnings]
...
clang   -fno-common -W -Wabi -Waddress -Wall -Wattributes 
-Wbuiltin-macro-redefined -Wchar-subscripts -Wcomment -Wcomments 
-Wdangling-else -Wdate-time -Wdeprecated -Wdeprecated-declarations 
-Wdisabled-optimization -Wdiv-by-zero -Wduplicate-decl-specifier 
-Wempty-body -Wendif-labels -Wenum-compare -Wexpansion-to-defined 
-Wextra -Wformat-extra-args -Wformat-security -Wformat-y2k 
-Wformat-zero-length -Wignored-attributes -Wignored-qualifiers 
-Wimplicit -Wimplicit-function-declaration -Wimplicit-int 
-Wincompatible-pointer-types -Winit-self -Wint-conversion 
-Wint-to-pointer-cast -Winvalid-pch -Wlogical-not-parentheses -Wmain 
-Wmissing-declarations -Wmissing-include-dirs -Wmissing-prototypes 
-Wmultichar -Wnarrowing -Wnested-externs -Wnonnull -Wnull-dereference 
-Wodr -Wold-style-definition -Woverflow -Wpacked -Wparentheses 
-Wpointer-arith -Wpointer-sign -Wpointer-to-int-cast -Wpragmas 
-Wreturn-type -Wsequence-point -Wshift-count-negative 
-Wshift-count-overflow -Wshift-negative-value -Wsizeof-array-argument 
-Wsizeof-pointer-memaccess -Wstrict-aliasing -Wstrict-prototypes 
-Wswitch-bool -Wtautological-compare -Wtrigraphs -Wuninitialized 
-Wunknown-pragmas -Wunused -Wunused-function -Wunused-label 
-Wunused-local-typedefs -Wunused-result -Wunused-value -Wunused-variable 
-Wvarargs -Wvariadic-macros -Wvolatile-register-var -Wwrite-strings 
-Wno-missing-field-initializers -Wredundant-decls 
-Wno-missing-field-initializers -Wno-sign-compare -Wno-type-limits 
-Wno-unused-parameter -Wno-format-nonliteral -Wno-missing-braces 
-Wno-tautological-compare 
-Wno-tautological-constant-out-of-range-compare -Werror -I. -I../src 
-I../lib -I. -I./../src -I./../lib    -O3 make-docfile.c 
../lib/libgnu.a  -o make-docfile
[no warnings]
...
make[1]: Leaving directory 
'/home/eggert/src/gnu/emacs/static-checking/lib-src'
make -C src VCSWITNESS='$(srcdir)/../.git/logs/HEAD' all
make[1]: Entering directory '/home/eggert/src/gnu/emacs/static-checking/src'
...
clang -c  -Demacs  -I. -I. -I../lib -I../lib   -pthread -isystem 
/usr/include/gtk-3.0 -isystem /usr/include/at-spi2-atk/2.0 -isystem 
/usr/include/at-spi-2.0 -isystem /usr/include/dbus-1.0 -isystem 
/usr/lib64/dbus-1.0/include -isystem /usr/include/gtk-3.0 -isystem 
/usr/include/gio-unix-2.0/ -isystem /usr/include/cairo -isystem 
/usr/include/pango-1.0 -isystem /usr/include/harfbuzz -isystem 
/usr/include/pango-1.0 -isystem /usr/include/atk-1.0 -isystem 
/usr/include/cairo -isystem /usr/include/pixman-1 -isystem 
/usr/include/freetype2 -isystem /usr/include/libpng16 -isystem 
/usr/include/freetype2 -isystem /usr/include/libdrm -isystem 
/usr/include/libpng16 -isystem /usr/include/gdk-pixbuf-2.0 -isystem 
/usr/include/libpng16 -isystem /usr/include/glib-2.0 -isystem 
/usr/lib64/glib-2.0/include -isystem /usr/include/freetype2 -isystem 
/usr/include/libpng16 -isystem /usr/include/freetype2 -isystem 
/usr/include/libpng16  -isystem /usr/include/alsa -pthread -isystem 
/usr/include/librsvg-2.0 -isystem /usr/include/gdk-pixbuf-2.0 -isystem 
/usr/include/libpng16 -isystem /usr/include/cairo -isystem 
/usr/include/glib-2.0 -isystem /usr/lib64/glib-2.0/include -isystem 
/usr/include/pixman-1 -isystem /usr/include/freetype2 -isystem 
/usr/include/libpng16 -isystem /usr/include/freetype2 -isystem 
/usr/include/libdrm -isystem /usr/include/libpng16  -isystem 
/usr/include/libpng16 -isystem /usr/include/libxml2 -isystem 
/usr/include/dbus-1.0 -isystem /usr/lib64/dbus-1.0/include -pthread 
-isystem /usr/include/glib-2.0 -isystem /usr/lib64/glib-2.0/include 
-pthread -isystem /usr/include/gconf/2 -isystem /usr/include/dbus-1.0 
-isystem /usr/lib64/dbus-1.0/include -isystem /usr/include/glib-2.0 
-isystem /usr/lib64/glib-2.0/include -isystem /usr/include/glib-2.0 
-isystem /usr/lib64/glib-2.0/include -isystem /usr/include/freetype2 
-isystem /usr/include/libpng16 -isystem /usr/include/freetype2 -isystem 
/usr/include/libpng16 -isystem /usr/include/freetype2 -isystem 
/usr/include/libpng16 -isystem /usr/include/freetype2 -isystem 
/usr/include/libpng16 -MMD -MF deps/emacs.d -MP  -isystem 
/usr/include/p11-kit-1 -Werror -fno-common -W -Wabi -Waddress -Wall 
-Wattributes -Wbuiltin-macro-redefined -Wchar-subscripts -Wcomment 
-Wcomments -Wdangling-else -Wdate-time -Wdeprecated 
-Wdeprecated-declarations -Wdisabled-optimization -Wdiv-by-zero 
-Wduplicate-decl-specifier -Wempty-body -Wendif-labels -Wenum-compare 
-Wexpansion-to-defined -Wextra -Wformat-extra-args -Wformat-security 
-Wformat-y2k -Wformat-zero-length -Wignored-attributes 
-Wignored-qualifiers -Wimplicit -Wimplicit-function-declaration 
-Wimplicit-int -Wincompatible-pointer-types -Winit-self -Wint-conversion 
-Wint-to-pointer-cast -Winvalid-pch -Wlogical-not-parentheses -Wmain 
-Wmissing-declarations -Wmissing-include-dirs -Wmissing-prototypes 
-Wmultichar -Wnarrowing -Wnested-externs -Wnonnull -Wnull-dereference 
-Wodr -Wold-style-definition -Woverflow -Wpacked -Wparentheses 
-Wpointer-arith -Wpointer-sign -Wpointer-to-int-cast -Wpragmas 
-Wreturn-type -Wsequence-point -Wshift-count-negative 
-Wshift-count-overflow -Wshift-negative-value -Wsizeof-array-argument 
-Wsizeof-pointer-memaccess -Wstrict-aliasing -Wstrict-prototypes 
-Wswitch-bool -Wtautological-compare -Wtrigraphs -Wuninitialized 
-Wunknown-pragmas -Wunused -Wunused-function -Wunused-label 
-Wunused-local-typedefs -Wunused-result -Wunused-value -Wunused-variable 
-Wvarargs -Wvariadic-macros -Wvolatile-register-var -Wwrite-strings 
-Wno-missing-field-initializers -Wredundant-decls 
-Wno-missing-field-initializers -Wno-sign-compare -Wno-type-limits 
-Wno-unused-parameter -Wno-format-nonliteral -Wno-missing-braces 
-Wno-tautological-compare 
-Wno-tautological-constant-out-of-range-compare -O3  emacs.c
[no warnings]
...


[-- Attachment #2: nowarnings.patch --]
[-- Type: text/x-patch, Size: 2493 bytes --]

diff --git a/lib-src/make-docfile.c b/lib-src/make-docfile.c
index 85bcc8b..9470bd6 100644
--- a/lib-src/make-docfile.c
+++ b/lib-src/make-docfile.c
@@ -224,11 +224,7 @@ put_filename (char *filename)
 
   for (tmp = filename; *tmp; tmp++)
     {
-      /* Use separate variable to silence a Clang warning on macOS.
-         Clang takes offence of the additional set of parantheses
-         generated by the macro.  */
-      bool is_sep = IS_DIRECTORY_SEP (*tmp);
-      if (is_sep)
+      if (IS_DIRECTORY_SEP (*tmp))
 	filename = tmp + 1;
     }
 
diff --git a/lib/strftime.c b/lib/strftime.c
index 18c899d..99bee4e 100644
--- a/lib/strftime.c
+++ b/lib/strftime.c
@@ -1123,23 +1123,18 @@ __strftime_internal (STREAM_OR_CHAR_T *s, STRFTIME_ARG (size_t maxsize)
           if (modifier == L_('E'))
             goto bad_format;
 
-          {
-            /* Use a new variable here instead of reusing number_value
-               because Clang complains about the self-assignment
-               generated by DO_NUMBER.  */
-            ptrdiff_t n = ns;
-            if (width == -1)
-              width = 9;
-            else
-              {
-                /* Take an explicit width less than 9 as a precision.  */
-                int j;
-                for (j = width; j < 9; j++)
-                  n /= 10;
-              }
+          number_value = ns;
+          if (width == -1)
+            width = 9;
+          else
+            {
+              /* Take an explicit width less than 9 as a precision.  */
+              int j;
+              for (j = width; j < 9; j++)
+                number_value /= 10;
+            }
 
-            DO_NUMBER (width, n);
-          }
+          DO_NUMBER (width, number_value);
 #endif
 
         case L_('n'):
diff --git a/src/emacs.c b/src/emacs.c
index 08430de..da8df1b 100644
--- a/src/emacs.c
+++ b/src/emacs.c
@@ -832,7 +832,7 @@ main (int argc, char **argv)
      (https://www.cygwin.com/ml/cygwin/2015-07/msg00096.html).  */
   struct rlimit rlim;
   if (getrlimit (RLIMIT_STACK, &rlim) == 0
-      && rlim.rlim_cur <= LONG_MAX)
+      && 0 <= rlim.rlim_cur && rlim.rlim_cur <= LONG_MAX)
     {
       rlim_t lim = rlim.rlim_cur;
 
@@ -866,7 +866,7 @@ main (int argc, char **argv)
 	     right thing anyway.  */
 	  long pagesize = getpagesize ();
 	  newlim += pagesize - 1;
-	  if (rlim.rlim_max < newlim)
+	  if (0 <= rlim.rlim_max && rlim.rlim_max < newlim)
 	    newlim = rlim.rlim_max;
 	  newlim -= newlim % pagesize;
 

Re: 27bb4de72b * Port cleanup attribute to Oracle Studio 12.5

[Philipp Stephani]

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

Paul Eggert <eggert@cs.ucla.edu> schrieb am Do., 15. Juni 2017 um 20:46 Uhr:

> On 06/15/2017 05:54 AM, Philipp Stephani wrote:
> > I've considered a couple of options. I think the simplest and most
> > portable one would be to compile as C++, which has destructors built
> > into the language.
>
> That would require some autoconf and makefile hacking, but it would
> work. That is, if modules are enabled, and __attribute__ ((cleanup)) is
> not available but a C++ compiler is available, then the build process
> could use the C++ compiler. Admittedly this is low priority.
>

Alternatively, we might even compile emacs-modules.c as C++
unconditionally. I guess by now every system that has a C compiler and
supports shared libraries also has a C++ compiler.


>
> > How about using #pragma clang diagnostic push/pop/ignore to ignore the
> > warnings in the specific statements where they arise and we know that
> > they are false positives? I'd much prefer that over disabling them
> > globally in configure, because most of the time the warnings are useful.
>
> Although many warnings are useful, in my experience these particular
> warnings are not useful for Emacs. That is, the hassle they cause by
> false alarms costs more than the benefits of actual bugs that they fix.
> It's OK to disable such warnings globally.
>
> >
> >     And I'm still puzzled as to why you're getting the Clang warnings
> >     but I
> >     am not. Are you using an older Clang? Are you passing it extra
> warning
> >     options?
> >
> >
> > I'm using the Apple fork on macOS. It's mostly identical to upstream
> > Clang and compiles Emacs just fine, but it is a fork and not 100%
> > identical. I also get some of the warnings only when building with -O3
> > (haven't checked other optimization levels).
>
> I just now applied the attached patch, configured with:
>
> ./configure --enable-gcc-warnings CC=clang CFLAGS='-O3'
>
> and had no problems on Fedora 25. If you have a problem with this patch,
> can you please send relevant output from 'clang --version' and 'make
> V=1'?


On macOS 10.12.5, current master, and the rlim_t patch reverted:

$ git rev-parse HEAD
1ac8c9bb9b6ba44585ed68b03ebbce659a777041

$ gcc -c  -Demacs  -I. -I. -I../lib -I../lib   -I/usr/X11/include
 -I/usr/local/Cellar/dbus/1.10.18/include/dbus-1.0
-I/usr/local/Cellar/dbus/1.10.18/lib/dbus-1.0/include           -MMD -MF
deps/emacs.d -MP  -I/usr/local/Cellar/gnutls/3.5.13/include
-I/usr/local/Cellar/nettle/3.3/include
-I/usr/local/Cellar/libtasn1/4.12/include
-I/usr/local/Cellar/p11-kit/0.23.7/include/p11-kit-1    -Wno-switch
-Wno-tautological-constant-out-of-range-compare -Wno-pointer-sign
-Wno-string-plus-int -Wno-unknown-attributes -ggdb3 -O0  emacs.c

*emacs.c:835:12: **warning: **comparison of 0 <= unsigned expression is
always true*

*      [-Wtautological-compare]*

      && 0 <= rlim.rlim_cur && rlim.rlim_cur <= LONG_MAX)

*         ~ ^  ~~~~~~~~~~~~~*

*emacs.c:869:10: **warning: **comparison of 0 <= unsigned expression is
always true*

*      [-Wtautological-compare]*

          if (0 <= rlim.rlim_max && rlim.rlim_max < newlim)

*              ~ ^  ~~~~~~~~~~~~~*

2 warnings generated.


$ ./config.status --config

'--with-modules' '--without-xml2' '--without-pop' '--with-mailutils'
'--enable-checking' '--enable-check-lisp-object-type'
'MAKEINFO=/usr/local/opt/texinfo/bin/makeinfo' 'CFLAGS=-ggdb3 -O0'


(i.e. this doesn't even need --enable-gcc-warnings)



The other case, same repo version, but the other commit reverted:


$ make -C lib-src make-docfile V=1

gcc   -Wno-switch -Wno-tautological-constant-out-of-range-compare
-Wno-pointer-sign -Wno-string-plus-int -Wno-unknown-attributes  -I.
-I../src -I../lib -I. -I./../src -I./../lib    -O3 -save-temps
make-docfile.c  ../lib/libgnu.a  -o make-docfile

*make-docfile.c:227:19: **warning: **equality comparison with extraneous
parentheses*

*      [-Wparentheses-equality]*

      if (((*tmp) == '/'))

*           ~~~~~~~^~~~~~*

*make-docfile.c:227:19: **note: *remove extraneous parentheses around the
comparison

      to silence this warning

      if (((*tmp) == '/'))

*          ~       ^     ~*

*make-docfile.c:227:19: **note: *use '=' to turn this equality comparison
into an

      assignment

      if (((*tmp) == '/'))

*                  ^~*

                  =

1 warning generated.


$ ./config.status --config

'--with-modules' '--without-xml2' '--without-pop' '--with-mailutils'
'--enable-checking' '--enable-check-lisp-object-type'
'MAKEINFO=/usr/local/opt/texinfo/bin/makeinfo' 'CFLAGS=-O3 -save-temps'


Note that the -save-temps is crucial here, without it the compiler doesn't
look at the preprocessed output.

It's weird that -save-temps generates more warnings, but GCC has similar
issues (https://gcc.gnu.org/bugzilla/show_bug.cgi?id=57201). -save-temps
also generates tons of warnings for other files, even without
--enable-gcc-warnings.

Not sure what to do about the -save-temps problem. Maybe that's just
something to document somewhere, because there seems to be little we can do
about it.


$ gcc --version

Configured with: --prefix=/Applications/Xcode.app/Contents/Developer/usr
--with-gxx-include-dir=/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.12.sdk/usr/include/c++/4.2.1

Apple LLVM version 8.1.0 (clang-802.0.42)

Target: x86_64-apple-darwin16.6.0

Thread model: posix

InstalledDir:
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin

[-- Attachment #2: Type: text/html, Size: 12465 bytes --]

Re: 27bb4de72b * Port cleanup attribute to Oracle Studio 12.5

[Paul Eggert]

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

On 06/15/2017 12:20 PM, Philipp Stephani wrote:
> we might even compile emacs-modules.c as C++ unconditionally. I guess 
> by now every system that has a C compiler and supports shared 
> libraries also has a C++ compiler.

Although C++ compilers may be available, they (or their libraries) are 
not necessarily installed properly, and this can be a problem at runtime 
as well as at compile-time. So it may be better to stick with C if it works.

> (i.e. this doesn't even need --enable-gcc-warnings)
>

Ah, that's key. Although 'configure' suppresses that false alarm if 
--enable-gcc-warnings is specified, it also needs to suppress it when 
--enable-gcc-warnings is *not* specified. I installed the attached, 
which I hope fixes the warnings that were annoying you (assuming we are 
not using -save-temps, which is a reasonable assumption).

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Pacify-clang-without-munging-C-source.patch --]
[-- Type: text/x-patch; name="0001-Pacify-clang-without-munging-C-source.patch", Size: 6513 bytes --]

From a8ce87fa150b318134aed4a97d1b1a8c2757c805 Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Thu, 15 Jun 2017 13:29:04 -0700
Subject: [PATCH] Pacify clang without munging C source
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

* configure.ac (WARN_CFLAGS): With Clang, use
-Wno-tautological-compare regardless of --enable-gcc-warnings.
(WERROR_CFLAGS): Simplify assignments, and guarantee it’s always set.
* lib/strftime.c: Copy from gnulib, reverting Clang-specific
change which I hope is no longer needed.
* src/emacs.c (main): Revert rlim_t change, as rlim_t is signed on
some older non-POSIX hosts.
---
 configure.ac   | 38 +++++++++++++++++++++-----------------
 lib/strftime.c | 27 +++++++++++----------------
 src/emacs.c    |  4 ++--
 3 files changed, 34 insertions(+), 35 deletions(-)

diff --git a/configure.ac b/configure.ac
index 459e314..9069e5b 100644
--- a/configure.ac
+++ b/configure.ac
@@ -891,6 +891,7 @@ AC_DEFUN
      [emacs_cv_clang=yes],
      [emacs_cv_clang=no])])
 
+WERROR_CFLAGS=
 # When compiling with GCC, prefer -isystem to -I when including system
 # include files, to avoid generating useless diagnostics for the files.
 AS_IF([test $gl_gcc_warnings = no],
@@ -900,7 +901,6 @@ AC_DEFUN
    [
      # Turn off some warnings if supported.
      gl_WARN_ADD([-Wno-switch])
-     gl_WARN_ADD([-Wno-tautological-constant-out-of-range-compare])
      gl_WARN_ADD([-Wno-pointer-sign])
      gl_WARN_ADD([-Wno-string-plus-int])
      gl_WARN_ADD([-Wno-unknown-attributes])
@@ -918,8 +918,7 @@ AC_DEFUN
        ;;
   esac
   AS_IF([test $gl_gcc_warnings = yes],
-    [gl_WARN_ADD([-Werror], [WERROR_CFLAGS])])
-  AC_SUBST([WERROR_CFLAGS])
+    [WERROR_CFLAGS=-Werror])
 
   nw="$nw -Wduplicated-branches"    # Too many false alarms
   nw="$nw -Wformat-overflow=2"      # False alarms due to GCC bug 80776
@@ -961,7 +960,7 @@ AC_DEFUN
   nw="$nw -Wtype-limits"
   nw="$nw -Wunused-parameter"
 
-  if test $emacs_cv_clang = yes; then
+  if test "$emacs_cv_clang" = yes; then
     nw="$nw -Wcast-align"
     nw="$nw -Wdouble-promotion"
     nw="$nw -Wmissing-braces"
@@ -984,11 +983,9 @@ AC_DEFUN
   gl_WARN_ADD([-Wno-unused-parameter]) # Too many warnings for now
   gl_WARN_ADD([-Wno-format-nonliteral])
 
-  # More things that clang is unduly picky about.
-  if test $emacs_cv_clang = yes; then
+  # clang is unduly picky about braces.
+  if test "$emacs_cv_clang" = yes; then
     gl_WARN_ADD([-Wno-missing-braces])
-    gl_WARN_ADD([-Wno-tautological-compare])
-    gl_WARN_ADD([-Wno-tautological-constant-out-of-range-compare])
   fi
 
   # This causes too much noise in the MinGW build
@@ -1006,15 +1003,22 @@ AC_DEFUN
    # define _FORTIFY_SOURCE 2
    #endif
   ])
+ ])
 
-  # We use a slightly smaller set of warning options for lib/.
-  # Remove the following and save the result in GNULIB_WARN_CFLAGS.
-  nw=
-  nw="$nw -Wunused-macros"
+# clang is unduly picky about these regardless of whether
+# --enable-gcc-warnings is specified.
+if test "$emacs_cv_clang" = yes; then
+  gl_WARN_ADD([-Wno-tautological-compare])
+  gl_WARN_ADD([-Wno-tautological-constant-out-of-range-compare])
+fi
 
-  gl_MANYWARN_COMPLEMENT([GNULIB_WARN_CFLAGS], [$WARN_CFLAGS], [$nw])
-  AC_SUBST([GNULIB_WARN_CFLAGS])
- ])
+# Use a slightly smaller set of warning options for lib/.
+nw=
+nw="$nw -Wunused-macros"
+gl_MANYWARN_COMPLEMENT([GNULIB_WARN_CFLAGS], [$WARN_CFLAGS], [$nw])
+
+AC_SUBST([WERROR_CFLAGS])
+AC_SUBST([GNULIB_WARN_CFLAGS])
 
 edit_cflags="
   s,///*,/,g
@@ -1033,7 +1037,7 @@ AC_DEFUN
 		 recommended for typical use.])],
 if test "${enableval}" != "no"; then
    ac_lto_supported=no
-   if test $emacs_cv_clang = yes; then
+   if test "$emacs_cv_clang" = yes; then
       AC_MSG_CHECKING([whether link-time optimization is supported by clang])
       GOLD_PLUGIN=`$CC -print-file-name=LLVMgold.so 2>/dev/null`
       if test -x "$GOLD_PLUGIN"; then
@@ -1062,7 +1066,7 @@ AC_DEFUN
    AC_MSG_RESULT([$ac_lto_supported])
    if test "$ac_lto_supported" = "yes"; then
       CFLAGS="$CFLAGS $LTO"
-      if test x$emacs_cv_clang = xyes; then
+      if test "$emacs_cv_clang" = yes; then
 	 AC_MSG_WARN([Please read INSTALL before using link-time optimization with clang])
 	 # WARNING: 'ar --plugin ...' doesn't work without
 	 # command, so plugin name is appended to ARFLAGS.
diff --git a/lib/strftime.c b/lib/strftime.c
index 18c899d..99bee4e 100644
--- a/lib/strftime.c
+++ b/lib/strftime.c
@@ -1123,23 +1123,18 @@ __strftime_internal (STREAM_OR_CHAR_T *s, STRFTIME_ARG (size_t maxsize)
           if (modifier == L_('E'))
             goto bad_format;
 
-          {
-            /* Use a new variable here instead of reusing number_value
-               because Clang complains about the self-assignment
-               generated by DO_NUMBER.  */
-            ptrdiff_t n = ns;
-            if (width == -1)
-              width = 9;
-            else
-              {
-                /* Take an explicit width less than 9 as a precision.  */
-                int j;
-                for (j = width; j < 9; j++)
-                  n /= 10;
-              }
+          number_value = ns;
+          if (width == -1)
+            width = 9;
+          else
+            {
+              /* Take an explicit width less than 9 as a precision.  */
+              int j;
+              for (j = width; j < 9; j++)
+                number_value /= 10;
+            }
 
-            DO_NUMBER (width, n);
-          }
+          DO_NUMBER (width, number_value);
 #endif
 
         case L_('n'):
diff --git a/src/emacs.c b/src/emacs.c
index 08430de..da8df1b 100644
--- a/src/emacs.c
+++ b/src/emacs.c
@@ -832,7 +832,7 @@ main (int argc, char **argv)
      (https://www.cygwin.com/ml/cygwin/2015-07/msg00096.html).  */
   struct rlimit rlim;
   if (getrlimit (RLIMIT_STACK, &rlim) == 0
-      && rlim.rlim_cur <= LONG_MAX)
+      && 0 <= rlim.rlim_cur && rlim.rlim_cur <= LONG_MAX)
     {
       rlim_t lim = rlim.rlim_cur;
 
@@ -866,7 +866,7 @@ main (int argc, char **argv)
 	     right thing anyway.  */
 	  long pagesize = getpagesize ();
 	  newlim += pagesize - 1;
-	  if (rlim.rlim_max < newlim)
+	  if (0 <= rlim.rlim_max && rlim.rlim_max < newlim)
 	    newlim = rlim.rlim_max;
 	  newlim -= newlim % pagesize;
 
-- 
2.9.4

Re: 27bb4de72b * Port cleanup attribute to Oracle Studio 12.5

[Richard Stallman]

[[[ To any NSA and FBI agents reading my email: please consider    ]]]
[[[ whether defending the US Constitution against all enemies,     ]]]
[[[ foreign or domestic, requires you to follow Snowden's example. ]]]

I am very strongly against making Emacs require a C++ compiler.

-- 
Dr Richard Stallman
President, Free Software Foundation (gnu.org, fsf.org)
Internet Hall-of-Famer (internethalloffame.org)
Skype: No way! See stallman.org/skype.html.

Re: 27bb4de72b * Port cleanup attribute to Oracle Studio 12.5

[Philipp Stephani]

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

Paul Eggert <eggert@cs.ucla.edu> schrieb am Do., 15. Juni 2017 um 22:33 Uhr:

> On 06/15/2017 12:20 PM, Philipp Stephani wrote:
> > we might even compile emacs-modules.c as C++ unconditionally. I guess
> > by now every system that has a C compiler and supports shared
> > libraries also has a C++ compiler.
>
> Although C++ compilers may be available, they (or their libraries) are
> not necessarily installed properly, and this can be a problem at runtime
> as well as at compile-time. So it may be better to stick with C if it
> works.
>

OK, let's keep the code as is for now; once we find a platform that causes
problems, we can reevaluate the options.


>
> > (i.e. this doesn't even need --enable-gcc-warnings)
> >
>
> Ah, that's key. Although 'configure' suppresses that false alarm if
> --enable-gcc-warnings is specified, it also needs to suppress it when
> --enable-gcc-warnings is *not* specified. I installed the attached,
> which I hope fixes the warnings that were annoying you (assuming we are
> not using -save-temps, which is a reasonable assumption).
>

OK, thanks.

[-- Attachment #2: Type: text/html, Size: 1588 bytes --]

Re: 27bb4de72b * Port cleanup attribute to Oracle Studio 12.5

[Paul Eggert]

On 06/15/2017 08:19 PM, Richard Stallman wrote:
> I am very strongly against making Emacs require a C++ compiler.

Yes, we've ruled that out.