Re: Module suggestion: Atomic operations

[Bruno Haible]

Hi Marc,

> C11 has introduced atomic types and atomic operations.  When they are not
> available, one can use locks/mutexes instead.
> 
> It would be nice if there was a Gnulib module that abstracts over this,
> much like the threadlib module and friends abstract over a specific
> threading implementation.
> 
> What I am thinking of is the following: Given a type T, a new Gnulib module
> atomic allows the declaration of an atomic version of type T.  This is
> straightforward on a platform that has .  Otherwise the atomic
> version of T would be a struct consisting of an object of type T together
> with a lock.
> 
> The rest of the module would then provide some simple atomic primitives
> like fetch_and_add, etc. that are either mapped to the C11 stdatomic
> counterparts or are implemented using the lock.

Yes, given that the platform support for these atomic types/operations is
increasing, it would accelerate the adoption if there was a Gnulib module,
like you describe it. Program developers could then adopt 
without losing portability to a number of platforms.

Personally I'm not very motivated to work on that (because in most algorithms
I've seen, mutexes/locks are the way to go, and because I find the memory_order
stuff hard to understand). But if you want to work on that, it will be
welcome!

Bruno

Re: stack module

[Bruno Haible]

Paul Eggert wrote:
> I don't want to encourage programmers to supply an E with side effects, as 
> side
> effects are trouble here.

+1

Re: portability of fopen and 'e' (O_CLOEXEC) flag

[Bruno Haible]

Tim Rühsen wrote on 2020-05-12:
> > How about using open() with O_CLOEXEC, and then fdopen()?
> 
> Thanks. Sure, this is possible. Doing so at dozens of places (or more)
> asks for an implementation in gnulib :)

Additionally, gnulib is the right place to do this because - as Eric said -
the 'e' flag is already scheduled for being added to the next POSIX version:
https://www.austingroupbugs.net/view.php?id=411

My evaluation of current platform support was incorrect: Current versions of
FreeBSD, NetBSD, OpenBSD, Solaris, Cygwin, and even Minix already support it.


2020-05-24  Bruno Haible  

fopen-gnu: Add tests.
* tests/test-fopen-gnu.c: New file.
* modules/fopen-gnu-tests: New file.

fopen-gnu: New module.
Suggested by Tim Rühsen  in
.
* lib/fopen.c (rpl_fopen): When the fopen-gnu module is enabled and the
mode contains an 'x' or 'e' flag, use open() followed by fdopen().
* m4/fopen.m4 (gl_FUNC_FOPEN_GNU): New macro.
* modules/fopen-gnu: New file.
* doc/posix-functions/fopen.texi: Document the 'fopen-gnu' module.

From 10cb4be2a2114dd6fff347acc9841d7904636adf Mon Sep 17 00:00:00 2001
From: Bruno Haible 
Date: Sun, 24 May 2020 20:38:53 +0200
Subject: [PATCH 1/2] fopen-gnu: New module.
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Suggested by Tim Rühsen  in
.

* lib/fopen.c (rpl_fopen): When the fopen-gnu module is enabled and the
mode contains an 'x' or 'e' flag, use open() followed by fdopen().
* m4/fopen.m4 (gl_FUNC_FOPEN_GNU): New macro.
* modules/fopen-gnu: New file.
* doc/posix-functions/fopen.texi: Document the 'fopen-gnu' module.
---
 ChangeLog  | 11 +
 doc/posix-functions/fopen.texi | 19 -
 lib/fopen.c| 92 --
 m4/fopen.m4| 87 ++-
 modules/fopen-gnu  | 27 +
 5 files changed, 229 insertions(+), 7 deletions(-)
 create mode 100644 modules/fopen-gnu

diff --git a/ChangeLog b/ChangeLog
index ae9fae4..fab0d87 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,5 +1,16 @@
 2020-05-24  Bruno Haible  
 
+	fopen-gnu: New module.
+	Suggested by Tim Rühsen  in
+	.
+	* lib/fopen.c (rpl_fopen): When the fopen-gnu module is enabled and the
+	mode contains an 'x' or 'e' flag, use open() followed by fdopen().
+	* m4/fopen.m4 (gl_FUNC_FOPEN_GNU): New macro.
+	* modules/fopen-gnu: New file.
+	* doc/posix-functions/fopen.texi: Document the 'fopen-gnu' module.
+
+2020-05-24  Bruno Haible  
+
 	open, openat: Really support O_CLOEXEC.
 	* lib/open.c (open): When have_cloexec is still undecided, do pass a
 	O_CLOEXEC flag to orig_open.
diff --git a/doc/posix-functions/fopen.texi b/doc/posix-functions/fopen.texi
index 308d676..6c562a6 100644
--- a/doc/posix-functions/fopen.texi
+++ b/doc/posix-functions/fopen.texi
@@ -4,9 +4,9 @@
 
 POSIX specification:@* @url{https://pubs.opengroup.org/onlinepubs/9699919799/functions/fopen.html}
 
-Gnulib module: fopen
+Gnulib module: fopen or fopen-gnu
 
-Portability problems fixed by Gnulib:
+Portability problems fixed by either Gnulib module @code{fopen} or @code{fopen-gnu}:
 @itemize
 @item
 This function does not fail when the file name argument ends in a slash
@@ -21,6 +21,21 @@ On Windows platforms (excluding Cygwin), this function does usually not
 recognize the @file{/dev/null} filename.
 @end itemize
 
+Portability problems fixed by Gnulib module @code{fopen-gnu}:
+@itemize
+@item
+This function does not support the mode character
+@samp{x} (corresponding to @code{O_EXCL}), introduced in ISO C11,
+on some platforms:
+FreeBSD 8.2, NetBSD 6.1, OpenBSD 5.6, Minix 3.2, AIX 6.1, HP-UX 11.31, IRIX 6.5, Solaris 11.3, Cygwin 1.7.16 (2012), mingw, MSVC 14.
+@item
+This function does not support the mode character
+@samp{e} (corresponding to @code{O_CLOEXEC}),
+introduced into a future POSIX revision through
+@url{https://www.austingroupbugs.net/view.php?id=411}, on some platforms:
+glibc 2.6, Mac OS X 10.13, FreeBSD 9.0, NetBSD 5.1, OpenBSD 5.6, Minix 3.2, AIX 7.2, HP-UX 11.31, IRIX 6.5, Solaris 11.3, Cygwin 1.7.16 (2012), mingw, MSVC 14.
+@end itemize
+
 Portability problems not fixed by Gnulib:
 @itemize
 @item
diff --git a/lib/fopen.c b/lib/fopen.c
index ad6511d..20065e4 100644
--- a/lib/fopen.c
+++ b/lib/fopen.c
@@ -49,6 +49,12 @@ rpl_fopen (const char *filename, const char *mode)
 {
   int open_direction;
   int open_flags_standard;
+#if GNULIB_FOPEN_GNU
+  int open_flags_gnu;
+# define BUF_SIZE 80
+  char fdopen_mode_buf[BUF_SIZE + 1];
+#endif
+  int open_flags;
 
 #if defined _WIN32 && ! defined __CYGWIN__
   if (strcmp (filename, "/dev/null") == 0)
@@ -58,35 +64,88 @@ rpl_fopen 

Re: stack module

[Paul Eggert]

On 5/24/20 1:17 AM, Marc Nieper-Wißkirchen wrote:
> You
> wrote for the affirm macro that if NDEBUG is defined that the behavior
> is undefined if E has side effects. That's not true as long as E does
> not evaluate to false.

We can *make* it true, by fiat. :-)

I don't want to encourage programmers to supply an E with side effects, as side
effects are trouble here. Even if side effects happen to work in the current
implementation when E is true, I don't want to suggest to programmers that
they'll continue to work in future implementations; we may come up with a
different implementation that evaluates E twice, for example.

open, openat: really support O_CLOEXEC

[Bruno Haible]

While working on support for mode 'e' in fopen(), I noticed that on macOS 10.13
open with O_CLOEXEC creates a file descriptor that does *not* have the
FD_CLOEXEC flag set.

This was meant to be implemented since 2017-08-14, but it does not work.

The problem is in this code (lib/open.c):

  fd = orig_open (filename,
  flags & ~(have_cloexec <= 0 ? O_CLOEXEC : 0), mode);

  if (flags & O_CLOEXEC)
{
  if (! have_cloexec)
{
  if (0 <= fd)
have_cloexec = 1;
  else if (errno == EINVAL)
{
  fd = orig_open (filename, flags & ~O_CLOEXEC, mode);
  have_cloexec = -1;
}
}
  if (have_cloexec < 0 && 0 <= fd)
set_cloexec_flag (fd, true);
}

At the first invocation:
  - have_cloexec is 0,
  - thus the flags passed to orig_open() do NOT include O_CLOEXEC,
  - thus orig_open() returns an fd >= 0,
  - thus have_cloexec gets set to 1.
The detection of a platform without O_CLOEXEC support is not working.

In fact, when I extend the unit test of open(), I see it fail:

$ ./test-open
../../gltests/test-open.h:100: assertion '(flags & FD_CLOEXEC) != 0' failed
Abort trap: 6
$ ./test-openat
../../gltests/test-open.h:100: assertion '(flags & FD_CLOEXEC) != 0' failed
Abort trap: 6


This patch fixes the issue and adds the corresponding unit test.


2020-05-24  Bruno Haible  

open, openat: Really support O_CLOEXEC.
* lib/open.c (open): When have_cloexec is still undecided, do pass a
O_CLOEXEC flag to orig_open.
* lib/openat.c (rpl_openat): When have_cloexec is still undecided, do
pass a O_CLOEXEC flag to orig_openat.
* tests/test-open.h (test_open): Verify that O_CLOEXEC is honoured.
* modules/open-tests (Depends-on): Add fcntl.
* modules/openat-tests (Depends-on): Likewise.
* modules/fcntl-safer-tests (Depends-on): Likewise.

diff --git a/lib/open.c b/lib/open.c
index bb180fd..751b42d 100644
--- a/lib/open.c
+++ b/lib/open.c
@@ -124,7 +124,7 @@ open (const char *filename, int flags, ...)
 #endif
 
   fd = orig_open (filename,
-  flags & ~(have_cloexec <= 0 ? O_CLOEXEC : 0), mode);
+  flags & ~(have_cloexec < 0 ? O_CLOEXEC : 0), mode);
 
   if (flags & O_CLOEXEC)
 {
diff --git a/lib/openat.c b/lib/openat.c
index 974f1a8..fbe1d2e 100644
--- a/lib/openat.c
+++ b/lib/openat.c
@@ -114,7 +114,7 @@ rpl_openat (int dfd, char const *filename, int flags, ...)
 # endif
 
   fd = orig_openat (dfd, filename,
-flags & ~(have_cloexec <= 0 ? O_CLOEXEC : 0), mode);
+flags & ~(have_cloexec < 0 ? O_CLOEXEC : 0), mode);
 
   if (flags & O_CLOEXEC)
 {
diff --git a/tests/test-open.h b/tests/test-open.h
index c57054f..862c8df 100644
--- a/tests/test-open.h
+++ b/tests/test-open.h
@@ -88,6 +88,26 @@ test_open (int (*func) (char const *, int, ...), bool print)
   ASSERT (0 <= fd);
   ASSERT (close (fd) == 0);
 
+  /* O_CLOEXEC must be honoured.  */
+  if (O_CLOEXEC)
+{
+  /* Since the O_CLOEXEC handling goes through a special code path at its
+ first invocation, test it twice.  */
+  int i;
+
+  for (i = 0; i < 2; i++)
+{
+  int flags;
+
+  fd = func (BASE "file", O_CLOEXEC | O_RDONLY);
+  ASSERT (0 <= fd);
+  flags = fcntl (fd, F_GETFD);
+  ASSERT (flags >= 0);
+  ASSERT ((flags & FD_CLOEXEC) != 0);
+  ASSERT (close (fd) == 0);
+}
+}
+
   /* Symlink handling, where supported.  */
   if (symlink (BASE "file", BASE "link") != 0)
 {
diff --git a/modules/fcntl-safer-tests b/modules/fcntl-safer-tests
index cb35aed..b967c8a 100644
--- a/modules/fcntl-safer-tests
+++ b/modules/fcntl-safer-tests
@@ -5,6 +5,7 @@ tests/macros.h
 
 Depends-on:
 stdbool
+fcntl
 symlink
 
 configure.ac:
diff --git a/modules/open-tests b/modules/open-tests
index 5bc4e0f..b2b6710 100644
--- a/modules/open-tests
+++ b/modules/open-tests
@@ -6,6 +6,7 @@ tests/macros.h
 
 Depends-on:
 stdbool
+fcntl
 symlink
 
 configure.ac:
diff --git a/modules/openat-tests b/modules/openat-tests
index 0b7370d..c4a72f5 100644
--- a/modules/openat-tests
+++ b/modules/openat-tests
@@ -5,6 +5,7 @@ tests/signature.h
 tests/macros.h
 
 Depends-on:
+fcntl
 symlink
 
 configure.ac:

Re: find_in_given_path(): not handling directories properly

[Paul Smith]

On Fri, 2020-05-22 at 01:05 -0400, Paul Smith wrote:
> However, the same issue exists in lib/findprog-in.c, in the
> find_in_given_path() (which is what GNU make actually uses), and that
> function wasn't updated by the above commit.

Thanks for addressing this, Bruno!

Update to docs etc.

[Paul Smith]

It's been pointed out on the GNU make lists that by including gnulib
content we've effectively updated our minimum required compiler from
C90 to C99.

That may be acceptable, but I think the gnulib docs (at least), if not
gnulib-tool, should make clear that AC_PROG_CC_C99 (at least) needs to
be added to to configure.ac.

In GNU make we only have AC_PROG_CC which was fine before we started
using gnulib but now causes failures on systems where this gives a C89
compiler, without extra options needed to choose C99.

Gnulib macros may also need to fail the configure if the compiler
cannot support C99.

Module suggestion: Atomic operations

[Marc Nieper-Wißkirchen]

C11 has introduced atomic types and atomic operations.  When they are not
available, one can use locks/mutexes instead.

It would be nice if there was a Gnulib module that abstracts over this,
much like the threadlib module and friends abstract over a specific
threading implementation.

What I am thinking of is the following: Given a type T, a new Gnulib module
atomic allows the declaration of an atomic version of type T.  This is
straightforward on a platform that has .  Otherwise the atomic
version of T would be a struct consisting of an object of type T together
with a lock.

The rest of the module would then provide some simple atomic primitives
like fetch_and_add, etc. that are either mapped to the C11 stdatomic
counterparts or are implemented using the lock.

Marc

fopen: Fix the trailing slash workaround

[Bruno Haible]

When I add a couple of more unit tests to the 'fopen' tests, I see a test
failure on Solaris 9:

$ ./test-fopen
../../gltests/test-fopen.h:74: assertion 'fopen ("./", "r+") == NULL' failed
Abort (core dumped)

This patch fixes it.


2020-05-24  Bruno Haible  

fopen: Fix the trailing slash workaround.
* lib/fopen.c (rpl_fopen): Parse the mode string. Recognize "r+" as a
write access. Pass the right flags to open().
* tests/test-fopen.h (test_fopen): Add a few more tests on directories.

diff --git a/lib/fopen.c b/lib/fopen.c
index a3128fa..ad6511d 100644
--- a/lib/fopen.c
+++ b/lib/fopen.c
@@ -47,11 +47,47 @@ orig_fopen (const char *filename, const char *mode)
 FILE *
 rpl_fopen (const char *filename, const char *mode)
 {
+  int open_direction;
+  int open_flags_standard;
+
 #if defined _WIN32 && ! defined __CYGWIN__
   if (strcmp (filename, "/dev/null") == 0)
 filename = "NUL";
 #endif
 
+  /* Parse the mode.  */
+  open_direction = 0;
+  open_flags_standard = 0;
+  {
+const char *m;
+
+for (m = mode; *m != '\0'; m++)
+  {
+switch (*m)
+  {
+  case 'r':
+open_direction = O_RDONLY;
+continue;
+  case 'w':
+open_direction = O_WRONLY;
+open_flags_standard |= O_CREAT | O_TRUNC;
+continue;
+  case 'a':
+open_direction = O_WRONLY;
+open_flags_standard |= O_CREAT | O_APPEND;
+continue;
+  case 'b':
+continue;
+  case '+':
+open_direction = O_RDWR;
+continue;
+  default:
+break;
+  }
+break;
+  }
+  }
+
 #if FOPEN_TRAILING_SLASH_BUG
   /* Fail if the mode requires write access and the filename ends in a slash,
  as POSIX says such a filename must name a directory
@@ -74,13 +110,13 @@ rpl_fopen (const char *filename, const char *mode)
 struct stat statbuf;
 FILE *fp;
 
-if (mode[0] == 'w' || mode[0] == 'a')
+if (open_direction != O_RDONLY)
   {
 errno = EISDIR;
 return NULL;
   }
 
-fd = open (filename, O_RDONLY);
+fd = open (filename, open_direction | open_flags_standard);
 if (fd < 0)
   return NULL;
 
@@ -101,7 +137,7 @@ rpl_fopen (const char *filename, const char *mode)
 return fp;
   }
   }
-# endif
+#endif
 
   return orig_fopen (filename, mode);
 }
diff --git a/tests/test-fopen.h b/tests/test-fopen.h
index fb036af..875cdaf 100644
--- a/tests/test-fopen.h
+++ b/tests/test-fopen.h
@@ -47,6 +47,10 @@ test_fopen (void)
   ASSERT (fopen (BASE "file/", "r") == NULL);
   ASSERT (errno == ENOTDIR || errno == EISDIR || errno == EINVAL);
 
+  errno = 0;
+  ASSERT (fopen (BASE "file/", "r+") == NULL);
+  ASSERT (errno == ENOTDIR || errno == EISDIR || errno == EINVAL);
+
   /* Cannot create a directory.  */
   errno = 0;
   ASSERT (fopen ("nonexist.ent/", "w") == NULL);
@@ -58,6 +62,18 @@ test_fopen (void)
   ASSERT (fopen (".", "w") == NULL);
   ASSERT (errno == EISDIR || errno == EINVAL || errno == EACCES);
 
+  errno = 0;
+  ASSERT (fopen ("./", "w") == NULL);
+  ASSERT (errno == EISDIR || errno == EINVAL || errno == EACCES);
+
+  errno = 0;
+  ASSERT (fopen (".", "r+") == NULL);
+  ASSERT (errno == EISDIR || errno == EINVAL || errno == EACCES);
+
+  errno = 0;
+  ASSERT (fopen ("./", "r+") == NULL);
+  ASSERT (errno == EISDIR || errno == EINVAL || errno == EACCES);
+
   /* /dev/null must exist, and be writable.  */
   f = fopen ("/dev/null", "r");
   ASSERT (f);

Re: Bug

[Bruno Haible]

[CCing bug-m4]

Hi,

Rahmad Alamsyah Nazaruddin wrote:
> I got this issue '#error "please port gnulib freadhead.c to your platform!
> Look at the definition of fflush, fread, ungetc on your system'
> How can i fix this ? I fail install on M4

Thank you for reporting this.

https://lists.gnu.org/archive/html/bug-m4/2020-03/msg0.html tells you
where you can download a working, up-to-date tarball for GNU m4.

Bruno

Bug

[Rahmad Alamsyah Nazaruddin]

Hello,
I got this issue '#error "please port gnulib freadhead.c to your platform!
Look at the definition of fflush, fread, ungetc on your system'
How can i fix this ? I fail install on M4

Re: Fix memleak in getdelim.m4

[Tim Rühsen]

Hi Bruno,

On 23.05.20 22:47, Bruno Haible wrote:
> Tim Rühsen wrote:
>> - clang-10 with unset CFLAGS (*.clang-10)
>>
>> And the same set with -fsanitize... (*.sanitize).
> 
> The other differences between the configure results without and with 
> sanitizers
> can be explained by the fact that these -fsanitize options have the effect 
> that
> the resulting executables are linked with
>   - libpthread,
>   - librt
>   - libm
>   - libdl
> by default.

...

> 
> So, all in all, it's more harmless that it looked at first sight. But
> it showed 9 bugs in gnulib, which are now all fixed.

Thank you so much for investigating and fixing the issues !

Regards, Tim



signature.asc
Description: OpenPGP digital signature

Bundling Gnulib tests with an application

[Marc Nieper-Wißkirchen]

When an application bundles Gnulib tests in its distribution, say in a
folder named gnulib-tests, and has its own suite of Automake tests in the
folder tests, the console output by Automake when running

make check

is suboptimal because it will print something like


Testsuite summary for MyProgram 1.0


twice, once when processing the tests folder and once when processing the
gnulib-tests folder.

Is there a way to change the title that is printed by Automake?

Marc

Re: stack module

[Marc Nieper-Wißkirchen]

Thank you very much, Paul!

One bit of the commentary is still misleading, though, I think. You
wrote for the affirm macro that if NDEBUG is defined that the behavior
is undefined if E has side effects. That's not true as long as E does
not evaluate to false.

Marc

Am So., 24. Mai 2020 um 04:14 Uhr schrieb Paul Eggert :
>
> On 5/23/20 3:06 PM, Bruno Haible wrote:
> > How about this instead?
>
> Thanks, good point about the danger. Also, I forgot to include verify.h.
>
> I tightened up the commentary, folded in Marc's suggestion, and installed the
> attached.

Re: Fix calloc.m4 test

[Bruno Haible]

> > Does this look like a reportable bug? :-)
> 
> Absolutely!
> 

I've reported it at .

Bruno