[PATCH 1/4] localename: -Wtautological-pointer-compare

[PATCH 1/4] localename: -Wtautological-pointer-compare

[Paul Eggert]

Problem found by xlclang 16.1 on AIX 7.2.
* lib/localename.c (duplocale, freelocale):
Omit unnecessary comparison of non-null args to NULL.
---
 ChangeLog        | 7 +++++++
 lib/localename.c | 6 +-----
 2 files changed, 8 insertions(+), 5 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index 2485ffc919..916ed890c7 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,10 @@
+2023-01-13  Paul Eggert  <eggert@cs.ucla.edu>
+
+	localename: -Wtautological-pointer-compare
+	Problem found by xlclang 16.1 on AIX 7.2.
+	* lib/localename.c (duplocale, freelocale):
+	Omit unnecessary comparison of non-null args to NULL.
+
 2023-01-13  Bruno Haible  <bruno@clisp.org>
 
 	login_tty tests: Be more verbose when the test fails.
diff --git a/lib/localename.c b/lib/localename.c
index a0e774ccea..5a178c68fe 100644
--- a/lib/localename.c
+++ b/lib/localename.c
@@ -2967,10 +2967,6 @@ duplocale (locale_t locale)
   struct locale_hash_node *node;
   locale_t result;
 
-  if (locale == NULL)
-    /* Invalid argument.  */
-    abort ();
-
   node = (struct locale_hash_node *) malloc (sizeof (struct locale_hash_node));
   if (node == NULL)
     /* errno is set to ENOMEM.  */
@@ -3056,7 +3052,7 @@ void
 freelocale (locale_t locale)
 #undef freelocale
 {
-  if (locale == NULL || locale == LC_GLOBAL_LOCALE)
+  if (locale == LC_GLOBAL_LOCALE)
     /* Invalid argument.  */
     abort ();
 
-- 
2.37.2

[PATCH 2/4] Don’t use alloc_size with xlclang 16.1

[Paul Eggert]

* m4/gnulib-common.m4 (_GL_HAS_ATTRIBUTE): Require Clang 5 instead
of 3.5 for this, to pacify xlclang 16.1.0 on AIX 7.2, which
advertises itself as clang 4.1.0, and which otherwise issues
warnings like “./xalloc.h:141:3: warning: 1540-2990 The attribute
"__attribute__((alloc_size(2, 3)))" is not supported.  The
attribute is ignored.” when building bleeding-edge GNU grep.
---
 ChangeLog           | 8 ++++++++
 m4/gnulib-common.m4 | 2 +-
 2 files changed, 9 insertions(+), 1 deletion(-)

diff --git a/ChangeLog b/ChangeLog
index 916ed890c7..866a405d4a 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,5 +1,13 @@
 2023-01-13  Paul Eggert  <eggert@cs.ucla.edu>
 
+	Don’t use alloc_size with xlclang 16.1
+	* m4/gnulib-common.m4 (_GL_HAS_ATTRIBUTE): Require Clang 5 instead
+	of 3.5 for this, to pacify xlclang 16.1.0 on AIX 7.2, which
+	advertises itself as clang 4.1.0, and which otherwise issues
+	warnings like “./xalloc.h:141:3: warning: 1540-2990 The attribute
+	"__attribute__((alloc_size(2, 3)))" is not supported.  The
+	attribute is ignored.” when building bleeding-edge GNU grep.
+
 	localename: -Wtautological-pointer-compare
 	Problem found by xlclang 16.1 on AIX 7.2.
 	* lib/localename.c (duplocale, freelocale):
diff --git a/m4/gnulib-common.m4 b/m4/gnulib-common.m4
index 26239caa2b..2db3376b01 100644
--- a/m4/gnulib-common.m4
+++ b/m4/gnulib-common.m4
@@ -71,7 +71,7 @@ AC_DEFUN([gl_COMMON_BODY], [
      && (!defined __clang_minor__ \
          || (defined __apple_build_version__ \
              ? 6000000 <= __apple_build_version__ \
-             : 3 < __clang_major__ + (5 <= __clang_minor__))))
+             : 5 <= __clang_major__)))
 # define _GL_HAS_ATTRIBUTE(attr) __has_attribute (__##attr##__)
 #else
 # define _GL_HAS_ATTRIBUTE(attr) _GL_ATTR_##attr
-- 
2.37.2

[PATCH 3/4] assert-h: fix configure comment-out

[Paul Eggert]

* m4/assert_h.m4 (gl_ASSERT_H): Also break apart "#undef
static_assert" with /**/.  Problem discovered with xlclang 16.1,
though it does not directly affect xlclang 16.1.
---
 ChangeLog      | 5 +++++
 m4/assert_h.m4 | 2 +-
 2 files changed, 6 insertions(+), 1 deletion(-)

diff --git a/ChangeLog b/ChangeLog
index 866a405d4a..b3b8ce5b9f 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,5 +1,10 @@
 2023-01-13  Paul Eggert  <eggert@cs.ucla.edu>
 
+	assert-h: fix configure comment-out
+	* m4/assert_h.m4 (gl_ASSERT_H): Also break apart "#undef
+	static_assert" with /**/.  Problem discovered with xlclang 16.1,
+	though it does not directly affect xlclang 16.1.
+
 	Don’t use alloc_size with xlclang 16.1
 	* m4/gnulib-common.m4 (_GL_HAS_ATTRIBUTE): Require Clang 5 instead
 	of 3.5 for this, to pacify xlclang 16.1.0 on AIX 7.2, which
diff --git a/m4/assert_h.m4 b/m4/assert_h.m4
index 6275f633a6..abba4fa3a1 100644
--- a/m4/assert_h.m4
+++ b/m4/assert_h.m4
@@ -60,7 +60,7 @@ AC_DEFUN([gl_ASSERT_H],
  /* Solaris 11.4 <assert.h> defines static_assert as a macro with 2 arguments.
     We need it also to be invocable with a single argument.  */
  #if defined __sun && (__STDC_VERSION__ - 0 >= 201112L) && !defined __cplusplus
-  #undef static_assert
+  #undef/**/static_assert
   #define static_assert _Static_assert
  #endif
 #endif])
-- 
2.37.2

[PATCH 4/4] assert-h: suppress xlclang 16.1 false alarms

[Paul Eggert]

* m4/assert_h.m4 (gl_ASSERT_H): Pacify older clangs too,
with regard to single-argument static_assert.
Problem found with xlclang 16.1 on AIX 7.2.
---
 ChangeLog      | 5 +++++
 m4/assert_h.m4 | 2 +-
 2 files changed, 6 insertions(+), 1 deletion(-)

diff --git a/ChangeLog b/ChangeLog
index b3b8ce5b9f..eb19f7dc0f 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,5 +1,10 @@
 2023-01-13  Paul Eggert  <eggert@cs.ucla.edu>
 
+	assert-h: suppress xlclang 16.1 false alarms
+	* m4/assert_h.m4 (gl_ASSERT_H): Pacify older clangs too,
+	with regard to single-argument static_assert.
+	Problem found with xlclang 16.1 on AIX 7.2.
+
 	assert-h: fix configure comment-out
 	* m4/assert_h.m4 (gl_ASSERT_H): Also break apart "#undef
 	static_assert" with /**/.  Problem discovered with xlclang 16.1,
diff --git a/m4/assert_h.m4 b/m4/assert_h.m4
index abba4fa3a1..3801452ef0 100644
--- a/m4/assert_h.m4
+++ b/m4/assert_h.m4
@@ -18,7 +18,7 @@ AC_DEFUN([gl_ASSERT_H],
        [AC_LANG_PROGRAM(
           [[#if defined __clang__ && __STDC_VERSION__ < 202311
              #pragma clang diagnostic error "-Wc2x-extensions"
-             #pragma clang diagnostic error "-Wc++17-extensions"
+             #pragma clang diagnostic error "-Wc++1z-extensions"
             #endif
             #ifdef INCLUDE_ASSERT_H
              #include <assert.h>
-- 
2.37.2

Re: [PATCH 1/4] localename: -Wtautological-pointer-compare

[Bruno Haible]

Paul Eggert wrote:
> Problem found by xlclang 16.1 on AIX 7.2.
> * lib/localename.c (duplocale, freelocale):
> Omit unnecessary comparison of non-null args to NULL.

I disagree with this patch.

Compiler warnings are supposed to help us improve the code.

Replacing a function that starts with an entry check — which is a good
practice [1] — with one that operates in garbage-in - garbage-out fashion
is not an improvement; quite the opposite.

We already know how to handle this situation: see
  canonicalize-lgpl.c
  execl.c
  execle.c
  execlp.c
  execve.c
  execvpe.c
  getaddrinfo.c
  getdelim.c
  getpass.c
  glob.c
  random_r.c
  setenv.c
  tsearch.c
  unsetenv.c

[1] https://cwe.mitre.org/data/definitions/20.html


2023-01-13  Bruno Haible  <bruno@clisp.org>

	localename: Fix -Wtautological-pointer-compare warning in a better way.
	* lib/localename.c (duplocale, freelocale): Revert last patch.
	(_GL_ARG_NONNULL): Define to empty.

diff --git a/lib/localename.c b/lib/localename.c
index 5a178c68fe..8fe90e0bf2 100644
--- a/lib/localename.c
+++ b/lib/localename.c
@@ -18,6 +18,12 @@
 /* Native Windows code written by Tor Lillqvist <tml@iki.fi>.  */
 /* Mac OS X code written by Bruno Haible <bruno@clisp.org>.  */
 
+/* Don't use __attribute__ __nonnull__ in this compilation unit.  Otherwise gcc
+   optimizes away the locale == NULL tests below in duplocale() and freelocale(),
+   or xlclang reports -Wtautological-pointer-compare warnings for these tests.
+ */
+#define _GL_ARG_NONNULL(params)
+
 #include <config.h>
 
 /* Specification.  */
@@ -2967,6 +2973,10 @@ duplocale (locale_t locale)
   struct locale_hash_node *node;
   locale_t result;
 
+  if (locale == NULL)
+    /* Invalid argument.  */
+    abort ();
+
   node = (struct locale_hash_node *) malloc (sizeof (struct locale_hash_node));
   if (node == NULL)
     /* errno is set to ENOMEM.  */
@@ -3052,7 +3062,7 @@ void
 freelocale (locale_t locale)
 #undef freelocale
 {
-  if (locale == LC_GLOBAL_LOCALE)
+  if (locale == NULL || locale == LC_GLOBAL_LOCALE)
     /* Invalid argument.  */
     abort ();
 

Re: [PATCH 1/4] localename: -Wtautological-pointer-compare

[Paul Eggert]

On 2023-01-13 14:59, Bruno Haible wrote:
> Replacing a function that starts with an entry check — which is a good
> practice [1]

It's a good practice in some contexts, bad in others.

In this particular case, on practical Gnulib targets the input should be 
checked anyway, both statically by the compiler at the call point and 
dynamically by the MMU.

There is a downside of the extra runtime check, in that if static 
checking is disabled (a mistake if you ask me, but many people do it), 
then the calling code won't immediately crash like it should. Instead, 
the function simply sets errno and returns, and the calling code might 
go on to do the wrong thing because there's a lot of sloppy calling code 
that either doesn't check errno or that has never been tested to do the 
right thing when the function fails.

So my guess is that for this particular case on practical hosts, the 
additional runtime check is more likely to introduce a security bug, 
than to prevent one. Of course this is just a guess. But that's why I 
wrote the patch the way I did.

Re: [PATCH 1/4] localename: -Wtautological-pointer-compare

[Bruno Haible]

Paul Eggert wrote:
> > ... starts with an entry check — which is a good practice [1]
> 
> It's a good practice in some contexts, bad in others.

It's a good practice at least when
  - the function is non-static (and therefore the callers are not all known),
  - checking the parameters is fast,
  - the code for checking the parameters is small in size, so that it does
    not hamper maintainability.

I wrote

  if (locale == NULL)
    /* Invalid argument.  */
    abort ();

for three reasons:

  * To avoid garbage-in - garbage-out behaviour, which in general encourages
    the presence of bugs.

  * So that when an invalid argument gets passed and the developer happened
    to compile with -g, the debugger will point to the exact line of the
    abort(). When you say "the input should be checked anyway ... dynamically
    by the MMU", what the user would see is a SIGSEGV, and would start wondering
    whether the bug is in his code or in our code.

  * For documentation purposes: So that it's clear that we have considered
    the doc of the function, and NULL at this particular place is invalid.

    (This is not evident: For example, does glibc's error_at_line() support a
    NULL fname argument? Hard to say from
    <https://www.gnu.org/software/libc/manual/html_node/Error-Messages.html>.)

> In this particular case, on practical Gnulib targets the input should be 
> checked anyway, both statically by the compiler at the call point and 
> dynamically by the MMU.
> 
> There is a downside of the extra runtime check, in that if static 
> checking is disabled (a mistake if you ask me, but many people do it), 
> then the calling code won't immediately crash like it should. Instead, 
> the function simply sets errno and returns, and the calling code might 
> go on to do the wrong thing because there's a lot of sloppy calling code 
> that either doesn't check errno or that has never been tested to do the 
> right thing when the function fails.
> 
> So my guess is that for this particular case on practical hosts, the 
> additional runtime check is more likely to introduce a security bug, 
> than to prevent one.

I'm not sure I understand what you write here. Do you mean that adding
  #define _GL_ARG_NONNULL(params)
disables useful static checking on this compilation unit? If so, my
counter-arguments are:

  * A runtime check catches all occasions of passing an invalid argument.
    A static check catches only those where the compiler/tool (gcc,
    clang, -fanalyzer, coverity, ...) has a reason to distinguish NULL and
    non-NULL. For example, when the argument is the value of some variable
    or some function's return, the compiler/tool will not diagnose anything,
    since that would lead to too many diagnostics.

    So, a runtime abort() will catch 100% of the invalid arguments, whereas
    the compiler/tool will catch maybe (wild guess) 25% of them.

    Therefore, eliminating a runtime check to preserve a static check is not
    beneficial. It is more important to preserve the runtime check.

  * We have 15 files in lib/ which disable _GL_ARG_NONNULL. That is less than
    2% of these source files. We don't lose much.

Still, if that is what you are worried about, we can reduce the situations
in which we disable the compiler's NULL argument checking: If I
  - replace all _GL_ARG_NONNULL(params) annotations with
    _GL_FUNC_ARG_NONNULL(func, params)
  - add to localename.c definitions
      #define THIS_COMPILATION_UNIT_DEFINES_duplocale 1
      #define THIS_COMPILATION_UNIT_DEFINES_freelocale 1
  - implement _GL_FUNC_ARG_NONNULL(func, params) so that in this context
    it expands to empty for func being duplocale or freelocale, but to
    _GL_ARG_NONNULL(params) for all other function names,
then most of the __attribute__ __non_null__ annotations are still present,
except for precisely those that we want to eliminate for this precise
compilation unit.

Is that what you are worried about? Should I work on this?

Bruno

Re: [PATCH 1/4] localename: -Wtautological-pointer-compare

[Paul Eggert]

On 2023-01-14 03:00, Bruno Haible wrote:
> Is that what you are worried about? Should I work on this?

No, please don't bother. I was partly confused by the situation and your 
email helped clear up some of it. Hope you don't mind this followup.

My confusion arose partly because I am accustomed to languages where the 
distinction between null and non-null pointers is checked statically and 
reliably, and I keep forgetting that with C, GCC and Clang are only poor 
approximations to that - though I hope the approximations are slowly 
getting better.


>   * To avoid garbage-in - garbage-out behaviour, which in general encourages
>     the presence of bugs.

Yes, that's a good thing, though in this particular case abort and 
dereferencing NULL have similar effects on typical platforms so this 
doesn't argue for one method or the other.


>   * So that when an invalid argument gets passed and the developer happened
>     to compile with -g, the debugger will point to the exact line of the
>     abort(). When you say "the input should be checked anyway ... dynamically
>     by the MMU", what the user would see is a SIGSEGV, and would start wondering
>     whether the bug is in his code or in our code.

In my experience users don't care whether the application died due to 
SIGABRT or to SIGSEGV.

Also, in my experience the debugger doesn't always point to the exact 
line of the abort(). For example, if there are two abort() calls in the 
same function they are routinely coalesced. And come to think of it, I 
have somewhat better luck with gdb's reports of SIGSEGV from 
dereferencing null pointers (though there are obviously issues there 
too) than I do from abort() calls.

To give a different example: I wouldn't bother with the following code 
(where M and N are int arguments to a function):

     if (n == 0)
       abort ();
     if (n == -1 && m < -INT_MAX)
       abort ();
     return m / n;

and would instead write this:

     return m / n;

as the user and debugging experiences are similar and the shorter form 
simplifies code maintenance. Sure, the longer form is safer for oddball 
platforms, but it's not worth the aggravation.


>   * For documentation purposes: So that it's clear that we have considered
>     the doc of the function, and NULL at this particular place is invalid.

I'm willing to trust the intelligence of the reader to know that if a 
function computes *P, then it's invalid for P to be null.

Admittedly there's a lot of low-quality code out there where this isn't 
true, but here I'm focusing on the relatively high-quality code in 
Gnulib and GNU apps.


>     (This is not evident: For example, does glibc's error_at_line() support a
>     NULL fname argument? Hard to say from
>     <https://www.gnu.org/software/libc/manual/html_node/Error-Messages.html>.)

The documentation is indeed unclear there, but this sort of thing is 
inevitable in any large system using C. For each such function where the 
spec is unclear, we should document whether a null pointer arg is 
allowed, and if it's not allowed the implementation of the function 
shouldn't need to worry about null pointer args (implementers already 
have too much to worry about, and part of the point of an API is 
simplification).

Re: [PATCH 1/4] localename: -Wtautological-pointer-compare

[Bruno Haible]

Hi Paul,

> My confusion arose partly because I am accustomed to languages where the 
> distinction between null and non-null pointers is checked statically and 
> reliably, and I keep forgetting that with C, GCC and Clang are only poor 
> approximations to that

Oh, now I understand. May I guess the language: Haskell, OCaml, TypeScript,
Rust?

> - though I hope the approximations are slowly getting better.

Still it will take a lot of time. The following steps need to happen:
  1. Standards need to define a notation for declaring a non-null type value,
     non-null argument, or non-null return value. (Partially done.)
  2. Compilers need to diagnose places where a non-null declaration could be
     added, like they do for function attributes __pure__ and __const__.
  3. Developers need to add such declarations to their .h files.
Then only such static checking can happen, without producing floods of
diagnostics that developers would discard.

> Also, in my experience the debugger doesn't always point to the exact 
> line of the abort(). For example, if there are two abort() calls in the 
> same function they are routinely coalesced.

True :( I should have mentioned to compile with "-g -O0", not just "-g".

> To give a different example: I wouldn't bother with the following code 
> (where M and N are int arguments to a function):
> 
>      if (n == 0)
>        abort ();
>      if (n == -1 && m < -INT_MAX)
>        abort ();
>      return m / n;
> 
> and would instead write this:
> 
>      return m / n;
> 
> as the user and debugging experiences are similar and the shorter form 
> simplifies code maintenance.

Unfortunately, this is an excellent example for a portability problem:
The division yields a SIGFPE on x86, x86_64, alpha, m68k, and s390/s390x
CPU, but not on other architectures.

> Sure, the longer form is safer for oddball 
> platforms, but it's not worth the aggravation.

Some distros feel differently. I have such code in GNU gettext, and
optimize away the entry checks on platforms where I know it yields a SIGFPE.
And some distros disabled these optimizations, i.e. enabled the entry checks
always...

Bruno

Re: [PATCH 1/4] localename: -Wtautological-pointer-compare

[Paul Eggert]

On 2023-01-15 14:03, Bruno Haible wrote:

>> My confusion arose partly because I am accustomed to languages where the
>> distinction between null and non-null pointers is checked statically ...
> 
> Oh, now I understand. May I guess the language: Haskell, OCaml, TypeScript,
> Rust?

These days OCaml and maybe Rust, though the language that first comes to 
my mind is one I designed in the 1970s and never got off the ground: B 
Pascal.


> Unfortunately, this is an excellent example for a portability problem:
> The division yields a SIGFPE on x86, x86_64, alpha, m68k, and s390/s390x
> CPU, but not on other architectures.

I suppose I should have written that my assumption was that the code is 
executed in a context where the divisor must be positive. (This is 
pretty common when calculating indexes from sizes.)

In other contexts the extra checking may be necessary, or at least helpful.