Re: master a86c25c91f3 11/13: Prefer -I to -isystem

Re: master a86c25c91f3 11/13: Prefer -I to -isystem

[Eli Zaretskii]

> branch: master
> commit a86c25c91f39a25a93d18c9267e024822c1bc43e
> Author: Paul Eggert <eggert@cs.ucla.edu>
> 
>     Prefer -I to -isystem
>     
>     * configure.ac: Simplify configuration by using -I instead of
>     -isystem, as -isystem is no longer helpful for suppressing
>     diagnostics (and likely has not been helpful for years).
>     Do not suppress -Wsystem-headers, as Gnulib no longer enables it.

Paul,

Contrary to your "famous last words" above, this change caused the
build issue a useless diagnostic in image.c, in a 32-bit build:

  d:/usr/include/glib-2.0/gobject/gparam.h:157:35: warning: result of '1 << 31' requires 33 bits to represent, but 'int' only has 32 bits [-Wshift-overflow=]
    157 |   G_PARAM_DEPRECATED          = 1 << 31
	|                                   ^~

While arguably a bug in Glib, the diagnostic is an annoyance, so I'd
like to avoid it.  I've verified that -Wno-system-headers doesn't
remove it.  -Wno-shift-overflow does, but I don't think suppressing
that globally is a good idea.

Re: master a86c25c91f3 11/13: Prefer -I to -isystem

[Paul Eggert]

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

On 2024-07-17 06:44, Eli Zaretskii wrote:
> this change caused the
> build issue a useless diagnostic in image.c, in a 32-bit build:

Ouch, my bad; I confused -Wsystem-headers with something else. (I tested 
32-bit builds but not with that image library.) I installed the attached 
to fix the mistake. Thanks for reporting it.

[-- Attachment #2: 0001-Go-back-to-preferring-isystem-to-I.patch --]
[-- Type: text/x-patch, Size: 3253 bytes --]

From 3a790abd869ddadc343710deb0c4368227ba6611 Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Wed, 17 Jul 2024 08:13:31 -0700
Subject: [PATCH] Go back to preferring -isystem to -I
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

* configure.ac: Go back to preferring -isystem to -I,
as headers like <gobject/gparam.h> still need it.  This
reverts almost all of 2024-07-16T02:25:44!eggert@cs.ucla.edu,
except that the ‘nw="$nw -Wsystem-headers"’ line continues to
be removed as it is no longer needed due to recent Gnulib changes.
Problem reported by Eli Zaretskii in:
https://lists.gnu.org/r/emacs-devel/2024-07/msg00756.html
---
 configure.ac | 18 +++++++++++++-----
 1 file changed, 13 insertions(+), 5 deletions(-)

diff --git a/configure.ac b/configure.ac
index e2b6dc2fc4d..b6acdf2e456 100644
--- a/configure.ac
+++ b/configure.ac
@@ -1759,8 +1759,11 @@ AC_DEFUN
 fi
 
 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],
  [
+  isystem='-I'
   AS_IF([test "$emacs_cv_clang" = yes],
    [
      # Turn off some warnings if supported.
@@ -1770,6 +1773,8 @@ AC_DEFUN
      gl_WARN_ADD([-Wno-unknown-pragmas])
    ])
  ],[
+  isystem='-isystem '
+
   # This, $nw, is the list of warnings we disable.
   nw=
 
@@ -1909,6 +1914,9 @@ AC_DEFUN
 
 edit_cflags="
   s,///*,/,g
+  s/^/ /
+  s/ -I/ $isystem/g
+  s/^ //
 "
 
 AC_ARG_ENABLE([link-time-optimization],
@@ -2816,7 +2824,7 @@ AC_DEFUN
 AC_SUBST([LD_SWITCH_X_SITE_RPATH])
 
 if test "${x_includes}" != NONE && test -n "${x_includes}"; then
-  C_SWITCH_X_SITE=-I`AS_ECHO(["$x_includes"]) | sed -e "s/:/ -I/g"`
+  C_SWITCH_X_SITE=$isystem`AS_ECHO(["$x_includes"]) | sed -e "s/:/ $isystem/g"`
 fi
 
 if test x"${x_includes}" = x; then
@@ -2882,8 +2890,8 @@ AC_DEFUN
        GNUSTEP_LOCAL_HEADERS="-I${GNUSTEP_LOCAL_HEADERS}"
      test "x${GNUSTEP_LOCAL_LIBRARIES}" != "x" && \
        GNUSTEP_LOCAL_LIBRARIES="-L${GNUSTEP_LOCAL_LIBRARIES}"
-     CPPFLAGS="$CPPFLAGS -I ${GNUSTEP_SYSTEM_HEADERS} ${GNUSTEP_LOCAL_HEADERS}"
-     CFLAGS="$CFLAGS -I ${GNUSTEP_SYSTEM_HEADERS} ${GNUSTEP_LOCAL_HEADERS}"
+     CPPFLAGS="$CPPFLAGS -isystem ${GNUSTEP_SYSTEM_HEADERS} ${GNUSTEP_LOCAL_HEADERS}"
+     CFLAGS="$CFLAGS -isystem ${GNUSTEP_SYSTEM_HEADERS} ${GNUSTEP_LOCAL_HEADERS}"
      LDFLAGS="$LDFLAGS -L${GNUSTEP_SYSTEM_LIBRARIES} ${GNUSTEP_LOCAL_LIBRARIES}"
      LIBS_GNUSTEP="-lgnustep-gui -lgnustep-base -lobjc -lpthread"
      dnl GNUstep defines BASE_NATIVE_OBJC_EXCEPTIONS to 0 or 1.
@@ -5787,13 +5795,13 @@ AC_DEFUN
 	xcsdkdir="" ;;
       esac
     fi
-    CPPFLAGS="$CPPFLAGS -I${xcsdkdir}/usr/include/libxml2"
+    CPPFLAGS="$CPPFLAGS -isystem${xcsdkdir}/usr/include/libxml2"
     AC_CHECK_HEADER([libxml/HTMLparser.h],
       [AC_CHECK_DECL([HTML_PARSE_RECOVER], [HAVE_LIBXML2=yes], [],
 		     [#include <libxml/HTMLparser.h>])])
     CPPFLAGS="$SAVE_CPPFLAGS"
     if test "${HAVE_LIBXML2}" = "yes"; then
-      LIBXML2_CFLAGS="-I${xcsdkdir}/usr/include/libxml2"
+      LIBXML2_CFLAGS="-isystem${xcsdkdir}/usr/include/libxml2"
       LIBXML2_LIBS="-lxml2"
     fi
   fi
-- 
2.43.0

Re: master a86c25c91f3 11/13: Prefer -I to -isystem

[Eli Zaretskii]

> Date: Wed, 17 Jul 2024 08:31:08 -0700
> Cc: emacs-devel@gnu.org
> From: Paul Eggert <eggert@cs.ucla.edu>
> 
> On 2024-07-17 06:44, Eli Zaretskii wrote:
> > this change caused the
> > build issue a useless diagnostic in image.c, in a 32-bit build:
> 
> Ouch, my bad; I confused -Wsystem-headers with something else. (I tested 
> 32-bit builds but not with that image library.) I installed the attached 
> to fix the mistake. Thanks for reporting it.

Thanks, the warning is gone now.

Re: master a86c25c91f3 11/13: Prefer -I to -isystem

[Collin Funk]

Eli Zaretskii <eliz@gnu.org> writes:

>   d:/usr/include/glib-2.0/gobject/gparam.h:157:35: warning: result of '1 << 31' requires 33 bits to represent, but 'int' only has 32 bits [-Wshift-overflow=]
>     157 |   G_PARAM_DEPRECATED          = 1 << 31
> 	|                                   ^~
>
> While arguably a bug in Glib, the diagnostic is an annoyance, so I'd
> like to avoid it.  I've verified that -Wno-system-headers doesn't
> remove it.  -Wno-shift-overflow does, but I don't think suppressing
> that globally is a good idea.

It is a bug since (1 * 2^31) is equal to INT_MAX + 1 on most platforms.
C23 § 6.5.7 says this is undefined behavior [1].

GCC and Clang both seem to handle it fine but the warning is correct.
Although it is annoying when it occurs in included headers. I ran into a
similar issue with GCC intrinsic doing (1 << 31) too. The glib bug was
fixed at some point since my system has:

  G_PARAM_DEPRECATED          = (gint)(1u << 31)

Which is the same as I did for GCC.

Collin

[1] https://www.open-std.org/jtc1/sc22/wg14/www/docs/n3096.pdf