[PATCH 2/2] Use C11 _Alignas on scratch_buffer internal buffer

[Paul Eggert]

Now back to the patch topic, I think the first union version [1] should
the most suitable one and it is what I am intended to push.
[1]https://sourceware.org/ml/libc-alpha/2017-09/msg00730.html

The code for that looks fine for Gnulib. However, the commit message could be
improved. It needs a summary line that does not mention _Alignas. "an union"
should be "a union". There is no need to cite every use affected by the change;
just say "All uses changed". It would be nice to credit Florian for pointing out
the problem and Andreas for suggesting a solution.

Attached is a proposed patch to Gnulib, which uses the same code as the
abovementioned glibc patch, but which has an improved commit message; please use
it as a guideline for the glibc commit.

One more thing. Gnulib and other GNU projects are switching to using https: URLs
to gnu.org and to fsf.org, and this causes the shared glob sources to differ in
one line. Glibc should be fixed to use https: instead of http: for citations to
GNU and FSF web sites. That should be a separate patch, of course. I'll look
into it.

[Adhemerval Zanella]

Now back to the patch topic, I think the first union version [1] should
the most suitable one and it is what I am intended to push.
[1]https://sourceware.org/ml/libc-alpha/2017-09/msg00730.html

The code for that looks fine for Gnulib. However, the commit message could be improved. It needs a summary line that does not mention _Alignas. "an union" should be "a union". There is no need to cite every use affected by the change; just say "All uses changed". It would be nice to credit Florian for pointing out the problem and Andreas for suggesting a solution.
Attached is a proposed patch to Gnulib, which uses the same code as the abovementioned glibc patch, but which has an improved commit message; please use it as a guideline for the glibc commit.

Fair enough, I indeed had changes commit message but I hadn't the trouble
to copied it on the message. I will use your suggestion.

One more thing. Gnulib and other GNU projects are switching to using https: URLs to gnu.org and to fsf.org, and this causes the shared glob sources to differ in one line. Glibc should be fixed to use https: instead of http: for citations to GNU and FSF web sites. That should be a separate patch, of course. I'll look into it.

[Paul Eggert]

+ ((flags & (GLOB_ERR | GLOB_NOESCAPE | GLOB_ALTDIRFUNC))
+ | GLOB_NOSORT | GLOB_ONLYDIR),

The indenting is not quite right here: the "|" should be under the 2nd
"(" in the previous line, not under the 1st.

+ result = __glob (onealt,
+ ((flags & ~(GLOB_NOCHECK | GLOB_NOMAGIC))
+ | GLOB_APPEND), errfunc, pglob);

As long as we're changing indenting anyway, we should fix the indenting
here to match the usual GNU style. Something like this:

              result = __glob (onealt,
                               ((flags & ~(GLOB_NOCHECK | GLOB_NOMAGIC))
                                | GLOB_APPEND),
                               errfunc, pglob);

Other than these minor indenting things it looks OK. glibc's glob.c is
wrongly indented elsewhere, but that can be fixed separately.

Proposed corresponding gnulib patch attached; it would keep gnulib
glob.c in sync with glibc's, except for whitespace and the https: thing.
CC'ing this to bug-gnulib.

[Adhemerval Zanella]

Post by Paul Eggert

+               ((flags & (GLOB_ERR | GLOB_NOESCAPE | GLOB_ALTDIRFUNC))
+               | GLOB_NOSORT | GLOB_ONLYDIR),

The indenting is not quite right here: the "|" should be under the 2nd
"(" in the previous line, not under the 1st.

Ack.

Post by Paul Eggert

+          result = __glob (onealt,
+                   ((flags & ~(GLOB_NOCHECK | GLOB_NOMAGIC))
+                   | GLOB_APPEND), errfunc, pglob);

As long as we're changing indenting anyway, we should fix the
              result = __glob (onealt,
                               ((flags & ~(GLOB_NOCHECK | GLOB_NOMAGIC))
                                | GLOB_APPEND),
                               errfunc, pglob);

Ack.

Post by Paul Eggert
Other than these minor indenting things it looks OK. glibc's glob.c is
wrongly indented elsewhere, but that can be fixed separately.
Proposed corresponding gnulib patch attached; it would keep gnulib
thing. CC'ing this to bug-gnulib.

I will commit both patch shortly, thanks for the review.

[Andreas Schwab]

Current version of make won't build against this (undefined reference to
__alloca from included glob sources).

Andreas.

[Adhemerval Zanella]

Post by Andreas Schwab
Current version of make won't build against this (undefined reference to
__alloca from included glob sources).
Andreas.

I am not very familiar of the process to incorporate gnulib code
in external projects, but I see other possible issues that would
need to be fixed as well:

* FLEXIBLE_ARRAY_MEMBER definition for !__LIBC.
* __glob_pattern_type duplicated prototype.
* __set_errno and mempcpy definition.

I could build gnumake with forced internal glob implementation
(make_cv_sys_gnu_glob=no) with the patch following patch. Looking
gnulib I am not sure if the correct way to use mempcpy for !_LIBC.

---

diff --git a/posix/glob.c b/posix/glob.c
index 98122dac88..31e3aba4dd 100644
--- a/posix/glob.c
+++ b/posix/glob.c
@@ -65,11 +65,15 @@
# define __stat64(fname, buf) stat (fname, buf)
# define __fxstatat64(_, d, f, st, flag) fstatat (d, f, st, flag)
# define struct_stat64 struct stat
-# ifndef __MVS__
-# define __alloca alloca
-# endif
+# define __alloca alloca
# define __readdir readdir
+# define FLEXIBLE_ARRAY_MEMBER 0
# define COMPILE_GLOB64
+static inline void *
+mempcpy (void *dest, const void *src, size_t n)
+{
+ return memcpy (dest, src, n) + n;
+}
#endif /* _LIBC */

#include <fnmatch.h>
@@ -230,8 +234,6 @@ glob_use_alloca (size_t alloca_used, size_t len)
static int glob_in_dir (const char *pattern, const char *directory,
int flags, int (*errfunc) (const char *, int),
glob_t *pglob, size_t alloca_used);
-extern int __glob_pattern_type (const char *pattern, int quote)
- attribute_hidden;

static int prefix_array (const char *prefix, char **array, size_t n) __THROWNL;
static int collated_compare (const void *, const void *) __THROWNL;

[Paul Eggert]

Post by Adhemerval Zanella
I see other possible issues that would
* FLEXIBLE_ARRAY_MEMBER definition for !__LIBC.
* __glob_pattern_type duplicated prototype.
* __set_errno and mempcpy definition.

Thanks for reporting the duplicated prototype; that is a portability
issue that I fixed in Gnulib by applying the attached patch to Gnulib
master. Gnulib glob.c already handles FLEXIBLE_ARRAY_MEMBER,
__set_errno, and mempcpy automatically, because its 'glob' module has
the modules 'flexmember', 'libc-config', and 'mempcpy' as prerequisites,
and these supply the necessary definitions.

You can test Gnulib glob.c's portabililty by running this shell command
in Gnulib's top directory:

./gnulib-tool --create-testdir --dir foo glob

and then by copying the newly-created 'foo' directory to the platform of
your choice and running './configure; make check' there.

Gnu Make does not use Gnulib. Instead, it ships an old version of
glob.c, which it took directly from glibc in 1999; this all predates
Gnulib. If GNU Make ever updates to a more-modern glob.c I suggest that
it use the Gnulib glob module, along with that module's prerequisites. I
could help do that, if the GNU Make maintainer wants to go that route.
[I'll CC: this to bug-make to give its maintainer a heads-up about this
libc-alpha discussion, which can be followed from here:

https://sourceware.org/ml/libc-alpha/2017-09/msg00972.html

]

Post by Adhemerval Zanella
I could build gnumake with forced internal glob implementation
(make_cv_sys_gnu_glob=no) with the patch following patch. Looking
gnulib I am not sure if the correct way to use mempcpy for !_LIBC.

Sorry, but I'm confused by that patch (quoted below). It is a patch to
glibc, so at first I assumed that you applied it to a copy of the glibc
source, built a new glibc with it, and then used the newly-built glibc
to configure and build GNU Make 4.2.1.  But that assumption cannot be
right, since the patch modifies only code that is unused when building
glibc. And one cannot simply copy current or patched glibc posix/glob.c
into make's lib/glob.c, as (among other things) glibc's posix/glob.c
includes <flexmember.h> when compiled outside of glibc, and GNU Make
does not have that include file. So how did you use this patch?

Post by Adhemerval Zanella
diff --git a/posix/glob.c b/posix/glob.c
index 98122dac88..31e3aba4dd 100644
--- a/posix/glob.c
+++ b/posix/glob.c
@@ -65,11 +65,15 @@
# define __stat64(fname, buf) stat (fname, buf)
# define __fxstatat64(_, d, f, st, flag) fstatat (d, f, st, flag)
# define struct_stat64 struct stat
-# ifndef __MVS__
-# define __alloca alloca
-# endif
+# define __alloca alloca
# define __readdir readdir
+# define FLEXIBLE_ARRAY_MEMBER 0
# define COMPILE_GLOB64
+static inline void *
+mempcpy (void *dest, const void *src, size_t n)
+{
+ return memcpy (dest, src, n) + n;
+}
#endif /* _LIBC */
#include <fnmatch.h>
@@ -230,8 +234,6 @@ glob_use_alloca (size_t alloca_used, size_t len)
static int glob_in_dir (const char *pattern, const char *directory,
int flags, int (*errfunc) (const char *, int),
glob_t *pglob, size_t alloca_used);
-extern int __glob_pattern_type (const char *pattern, int quote)
- attribute_hidden;
static int prefix_array (const char *prefix, char **array, size_t n) __THROWNL;
static int collated_compare (const void *, const void *) __THROWNL;

[Adhemerval Zanella]

Post by Paul Eggert

Post by Adhemerval Zanella
I see other possible issues that would
   * FLEXIBLE_ARRAY_MEMBER definition for !__LIBC.
   * __glob_pattern_type duplicated prototype.
   * __set_errno and mempcpy definition.

Thanks for reporting the duplicated prototype; that is a portability
issue that I fixed in Gnulib by applying the attached patch to Gnulib
master. Gnulib glob.c already handles FLEXIBLE_ARRAY_MEMBER,
__set_errno, and mempcpy automatically, because its 'glob' module has
the modules 'flexmember', 'libc-config', and 'mempcpy' as
prerequisites, and these supply the necessary definitions.

Thanks, I will sync this change with glibc.

Post by Paul Eggert
You can test Gnulib glob.c's portabililty by running this shell
./gnulib-tool --create-testdir --dir foo glob
and then by copying the newly-created 'foo' directory to the platform
of your choice and running './configure; make check' there.
Gnu Make does not use Gnulib. Instead, it ships an old version of
glob.c, which it took directly from glibc in 1999; this all predates
Gnulib. If GNU Make ever updates to a more-modern glob.c I suggest
that it use the Gnulib glob module, along with that module's
prerequisites. I could help do that, if the GNU Make maintainer wants
to go that route. [I'll CC: this to bug-make to give its maintainer a
heads-up about this libc-alpha discussion, which can be followed from
https://sourceware.org/ml/libc-alpha/2017-09/msg00972.html

Besides update GNU make glob implementation I think it should also update
its configure.ac to accept not only _GNU_GLOB_INTERFACE_VERSION equal
to 1, but also _GNU_GLOB_INTERFACE_VERSION to 2 now that your suggestion [1]
has been fixed.

[1] http://lists.gnu.org/archive/html/bug-make/2017-09/msg00014.html

Post by Paul Eggert
]

Post by Adhemerval Zanella
I could build gnumake with forced internal glob implementation
(make_cv_sys_gnu_glob=no) with the patch following patch.  Looking
gnulib I am not sure if the correct way to use mempcpy for !_LIBC.

Sorry, but I'm confused by that patch (quoted below). It is a patch to
glibc, so at first I assumed that you applied it to a copy of the
glibc source, built a new glibc with it, and then used the newly-built
glibc to configure and build GNU Make 4.2.1.  But that assumption
cannot be right, since the patch modifies only code that is unused
when building glibc. And one cannot simply copy current or patched
glibc posix/glob.c into make's lib/glob.c, as (among other things)
glibc's posix/glob.c includes <flexmember.h> when compiled outside of
glibc, and GNU Make does not have that include file. So how did you
use this patch?

Sorry if I were not explicit, the patch is indeed against glibc and what
I did was
basically copy glob.c and all required files (flexmember.h and
glob_internal)
on glob folder. It is just a ad hoc way to verify if its build correctly.

Post by Paul Eggert

Post by Adhemerval Zanella
diff --git a/posix/glob.c b/posix/glob.c
index 98122dac88..31e3aba4dd 100644
--- a/posix/glob.c
+++ b/posix/glob.c
@@ -65,11 +65,15 @@
  # define __stat64(fname, buf)   stat (fname, buf)
  # define __fxstatat64(_, d, f, st, flag) fstatat (d, f, st, flag)
  # define struct_stat64          struct stat
-# ifndef __MVS__
-#  define __alloca              alloca
-# endif
+# define __alloca               alloca
  # define __readdir              readdir
+# define FLEXIBLE_ARRAY_MEMBER 0
  # define COMPILE_GLOB64
+static inline void *
+mempcpy (void *dest, const void *src, size_t n)
+{
+  return memcpy (dest, src, n) + n;
+}
  #endif /* _LIBC */
    #include <fnmatch.h>
@@ -230,8 +234,6 @@ glob_use_alloca (size_t alloca_used, size_t len)
  static int glob_in_dir (const char *pattern, const char *directory,
                         int flags, int (*errfunc) (const char *, int),
                         glob_t *pglob, size_t alloca_used);
-extern int __glob_pattern_type (const char *pattern, int quote)
-    attribute_hidden;
    static int prefix_array (const char *prefix, char **array, size_t
n) __THROWNL;
  static int collated_compare (const void *, const void *) __THROWNL;

[Andreas Schwab]

I can confirm that make works properly with the compat glob symbol.

Andreas.