Re: [PATCH] stdlib: avoid canonicalize_file_name contradiction

[Bruno Haible <bruno@clisp.org>]

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

Hi Jim,

> stdlib: avoid canonicalize_file_name contradiction
> * lib/stdlib.in.h (canonicalize_file_name): Remove the nonnull
> attribute from its declaration.

I'm not sure this is right. The patch removes a diagnostic (from GCC and
possibly other static analyzers) when some code is, by mistake, passing
NULL to canonicalize_file_name.

The question is: Is passing NULL to canonicalize_file_name valid? If not,
then the nonnull attribute should stay.

On one hand, in glibc's stdlib.h we have:

extern char *canonicalize_file_name (const char *__name)
     __THROW __nonnull ((1)) __wur;

On the other hand, in https://gcc.gnu.org/bugzilla/show_bug.cgi?id=93156#c3
you say "Note that at least some versions of canonicalize_file_name *can*
take a NULL argument". What are there versions? Even if Cygwin and/or
Solaris 11 have a function of the same name which allows passing NULL,
portable code should not pass NULL since that would not work on glibc
systems. Therefore the diagnostic is useful.

> tests/test-canonicalize-lgpl.c
> passes null_ptr () to it, which (via this contradiction) would
> provoke a segfault from GCC 10. See a small reproducer and
> discussion in https://gcc.gnu.org/bugzilla/show_bug.cgi?id=93156

I would prefer to modify the tests, to avoid GCC's over-optimization.
There are two ways to do that:

  * Use 'volatile', as shown by Andrew Pinski in
    https://gcc.gnu.org/bugzilla/show_bug.cgi?id=93156#c2
    I am not in favour of this, because the semantics of 'volatile'
    changes over time. Currently there is a proposed change in
    http://www.open-std.org/jtc1/sc22/wg14/www/docs/n2244.htm#dr_476 .

  * The usual technique for this, that we already use in
      lib/canonicalize-lgpl.c
      lib/getaddrinfo.c
      lib/getdelim.c
      lib/glob.c
      lib/random_r.c
      lib/setenv.c
      lib/tsearch.c
      lib/unsetenv.c
    , is to add a line
      #define _GL_ARG_NONNULL(params)
    before '#include <config.h>'.

Does the attached patch fix the problem for you?

Bruno

[-- Attachment #2: 0001-tests-Avoid-GCC-over-optimization-caused-by-_GL_ARG_.patch --]
[-- Type: text/x-patch, Size: 3073 bytes --]

From 826a16e3da3d42454417e433774dda6e5003ad04 Mon Sep 17 00:00:00 2001
From: Bruno Haible <bruno@clisp.org>
Date: Sun, 5 Jan 2020 09:13:25 +0100
Subject: [PATCH] tests: Avoid GCC over-optimization caused by _GL_ARG_NONNULL
 attributes.

Reported by Jim Meyering in
<https://lists.gnu.org/archive/html/bug-gnulib/2020-01/msg00040.html>.

* tests/test-canonicalize.c (_GL_ARG_NONNULL): Define to empty.
* tests/test-canonicalize-lgpl.c (_GL_ARG_NONNULL): Likewise.
* tests/test-ptsname_r.c (_GL_ARG_NONNULL): Likewise.
---
 ChangeLog                      | 9 +++++++++
 tests/test-canonicalize-lgpl.c | 5 +++++
 tests/test-canonicalize.c      | 5 +++++
 tests/test-ptsname_r.c         | 5 +++++
 4 files changed, 24 insertions(+)

diff --git a/ChangeLog b/ChangeLog
index 6d9ea9e..556549c 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,12 @@
+2020-01-05  Bruno Haible  <bruno@clisp.org>
+
+	tests: Avoid GCC over-optimization caused by _GL_ARG_NONNULL attributes.
+	Reported by Jim Meyering in
+	<https://lists.gnu.org/archive/html/bug-gnulib/2020-01/msg00040.html>.
+	* tests/test-canonicalize.c (_GL_ARG_NONNULL): Define to empty.
+	* tests/test-canonicalize-lgpl.c (_GL_ARG_NONNULL): Likewise.
+	* tests/test-ptsname_r.c (_GL_ARG_NONNULL): Likewise.
+
 2020-01-04  Bruno Haible  <bruno@clisp.org>
 
 	mbsnrtoc32s: Add tests.
diff --git a/tests/test-canonicalize-lgpl.c b/tests/test-canonicalize-lgpl.c
index b46ad87..2c743af 100644
--- a/tests/test-canonicalize-lgpl.c
+++ b/tests/test-canonicalize-lgpl.c
@@ -16,6 +16,11 @@
 
 /* Written by Bruno Haible <bruno@clisp.org>, 2007.  */
 
+/* Don't use __attribute__ __nonnull__ in this compilation unit.  Otherwise gcc
+   may "optimize" the null_ptr function, when its result gets passed to a
+   function that has an argument declared as _GL_ARG_NONNULL.  */
+#define _GL_ARG_NONNULL(params)
+
 #include <config.h>
 
 #include <stdlib.h>
diff --git a/tests/test-canonicalize.c b/tests/test-canonicalize.c
index e7138a6..1df3092 100644
--- a/tests/test-canonicalize.c
+++ b/tests/test-canonicalize.c
@@ -16,6 +16,11 @@
 
 /* Written by Bruno Haible <bruno@clisp.org>, 2007.  */
 
+/* Don't use __attribute__ __nonnull__ in this compilation unit.  Otherwise gcc
+   may "optimize" the null_ptr function, when its result gets passed to a
+   function that has an argument declared as _GL_ARG_NONNULL.  */
+#define _GL_ARG_NONNULL(params)
+
 #include <config.h>
 
 #include "canonicalize.h"
diff --git a/tests/test-ptsname_r.c b/tests/test-ptsname_r.c
index 19e33ea..9e1471c 100644
--- a/tests/test-ptsname_r.c
+++ b/tests/test-ptsname_r.c
@@ -14,6 +14,11 @@
    You should have received a copy of the GNU General Public License
    along with this program.  If not, see <https://www.gnu.org/licenses/>.  */
 
+/* Don't use __attribute__ __nonnull__ in this compilation unit.  Otherwise gcc
+   may "optimize" the null_ptr function, when its result gets passed to a
+   function that has an argument declared as _GL_ARG_NONNULL.  */
+#define _GL_ARG_NONNULL(params)
+
 #include <config.h>
 
 #include <stdlib.h>
-- 
2.7.4