[1/5] posix: Sync regex code with gnulib

  It sync with gnulib commit 43ee1a6bf.  The main change is 9682f18e9.
(which does not have a meaniful description).

Checked on x86_64-linux-gnu.
---
 posix/regcomp.c        |  2 +-
 posix/regex.h          | 17 ++++++++++++-----
 posix/regex_internal.c | 19 ++++++++++---------
 posix/regex_internal.h | 16 ++++++++++++----
 4 files changed, 35 insertions(+), 19 deletions(-)
  

This patch looks good to me, thanks.
  

After this commit [1], I am seeing  -Werror=undef when bootstapping glibc on aarch64-none-linux-gnu :

In file included from regex.c:70:0:
regex_internal.h:852:26: error: "__clang_major__" is not defined [-Werror=undef]
 # if (__GNUC__ >= 7) || (__clang_major__ >= 10)
                          ^~~~~~~~~~~~~~~

The build/host/target setup is:
Build: aarch64-none-linux-gnu (Ubuntu 14.04)
Host: aarch64-none-linux-gnu
Target: aarch64-none-linux-gnu

[1]
commit c2a150d089fa096cb5f9e342da80fb30dc0d1953
Author:     Adhemerval Zanella <adhemerval.zanella@linaro.org>
AuthorDate: Tue Dec 29 17:32:25 2020 -0300
Commit:     Adhemerval Zanella <adhemerval.zanella@linaro.org>
CommitDate: Mon Jan 4 08:38:52 2021 -0300

    posix: Sync regex code with gnulib
    
    It sync with gnulib commit 43ee1a6bf.  The main change is 9682f18e9.
    (which does not have a meaniful description).
    
    Checked on x86_64-linux-gnu.




On 30/12/2020, 20:15, "Libc-alpha on behalf of Adhemerval Zanella via Libc-alpha" <libc-alpha-bounces@sourceware.org on behalf of libc-alpha@sourceware.org> wrote:

    It sync with gnulib commit 43ee1a6bf.  The main change is 9682f18e9.
    (which does not have a meaniful description).

    Checked on x86_64-linux-gnu.
    ---
     posix/regcomp.c        |  2 +-
     posix/regex.h          | 17 ++++++++++++-----
     posix/regex_internal.c | 19 ++++++++++---------
     posix/regex_internal.h | 16 ++++++++++++----
     4 files changed, 35 insertions(+), 19 deletions(-)

    diff --git a/posix/regcomp.c b/posix/regcomp.c
    index 93bb0a0538..692928b0db 100644
    --- a/posix/regcomp.c
    +++ b/posix/regcomp.c
    @@ -558,7 +558,7 @@ weak_alias (__regerror, regerror)
     static const bitset_t utf8_sb_map =
     {
       /* Set the first 128 bits.  */
    -# if defined __GNUC__ && !defined __STRICT_ANSI__
    +# if (defined __GNUC__ || __clang_major__ >= 4) && !defined __STRICT_ANSI__
       [0 ... 0x80 / BITSET_WORD_BITS - 1] = BITSET_WORD_MAX
     # else
     #  if 4 * BITSET_WORD_BITS < ASCII_CHARS
    diff --git a/posix/regex.h b/posix/regex.h
    index 5fe41c8685..7418e6c76f 100644
    --- a/posix/regex.h
    +++ b/posix/regex.h
    @@ -612,7 +612,9 @@ extern int re_exec (const char *);
        'configure' might #define 'restrict' to those words, so pick a
        different name.  */
     #ifndef _Restrict_
    -# if defined __restrict || 2 < __GNUC__ + (95 <= __GNUC_MINOR__)
    +# if defined __restrict \
    +     || 2 < __GNUC__ + (95 <= __GNUC_MINOR__) \
    +     || __clang_major__ >= 3
     #  define _Restrict_ __restrict
     # elif 199901L <= __STDC_VERSION__ || defined restrict
     #  define _Restrict_ restrict
    @@ -620,13 +622,18 @@ extern int re_exec (const char *);
     #  define _Restrict_
     # endif
     #endif
    -/* For [restrict], use glibc's __restrict_arr if available.
    -   Otherwise, GCC 3.1 (not in C++ mode) and C99 support [restrict].  */
    +/* For the ISO C99 syntax
    +     array_name[restrict]
    +   use glibc's __restrict_arr if available.
    +   Otherwise, GCC 3.1 and clang support this syntax (but not in C++ mode).
    +   Other ISO C99 compilers support it as well.  */
     #ifndef _Restrict_arr_
     # ifdef __restrict_arr
     #  define _Restrict_arr_ __restrict_arr
    -# elif ((199901L <= __STDC_VERSION__ || 3 < __GNUC__ + (1 <= __GNUC_MINOR__)) \
    -        && !defined __GNUG__)
    +# elif ((199901L <= __STDC_VERSION__ \
    +         || 3 < __GNUC__ + (1 <= __GNUC_MINOR__) \
    +         || __clang_major__ >= 3) \
    +        && !defined __cplusplus)
     #  define _Restrict_arr_ _Restrict_
     # else
     #  define _Restrict_arr_
    diff --git a/posix/regex_internal.c b/posix/regex_internal.c
    index e1b6b4d5af..ed0a13461b 100644
    --- a/posix/regex_internal.c
    +++ b/posix/regex_internal.c
    @@ -300,18 +300,20 @@ build_wcs_upper_buffer (re_string_t *pstr)
           while (byte_idx < end_idx)
     	{
     	  wchar_t wc;
    +	  unsigned char ch = pstr->raw_mbs[pstr->raw_mbs_idx + byte_idx];

    -	  if (isascii (pstr->raw_mbs[pstr->raw_mbs_idx + byte_idx])
    -	      && mbsinit (&pstr->cur_state))
    +	  if (isascii (ch) && mbsinit (&pstr->cur_state))
     	    {
    -	      /* In case of a singlebyte character.  */
    -	      pstr->mbs[byte_idx]
    -		= toupper (pstr->raw_mbs[pstr->raw_mbs_idx + byte_idx]);
     	      /* The next step uses the assumption that wchar_t is encoded
     		 ASCII-safe: all ASCII values can be converted like this.  */
    -	      pstr->wcs[byte_idx] = (wchar_t) pstr->mbs[byte_idx];
    -	      ++byte_idx;
    -	      continue;
    +	      wchar_t wcu = __towupper (ch);
    +	      if (isascii (wcu))
    +		{
    +		  pstr->mbs[byte_idx] = wcu;
    +		  pstr->wcs[byte_idx] = wcu;
    +		  byte_idx++;
    +		  continue;
    +		}
     	    }

     	  remain_len = end_idx - byte_idx;
    @@ -348,7 +350,6 @@ build_wcs_upper_buffer (re_string_t *pstr)
     	    {
     	      /* It is an invalid character, an incomplete character
     		 at the end of the string, or '\0'.  Just use the byte.  */
    -	      int ch = pstr->raw_mbs[pstr->raw_mbs_idx + byte_idx];
     	      pstr->mbs[byte_idx] = ch;
     	      /* And also cast it to wide char.  */
     	      pstr->wcs[byte_idx++] = (wchar_t) ch;
    diff --git a/posix/regex_internal.h b/posix/regex_internal.h
    index 8c42586c42..4a3cf779bf 100644
    --- a/posix/regex_internal.h
    +++ b/posix/regex_internal.h
    @@ -77,6 +77,14 @@
     # define isblank(ch) ((ch) == ' ' || (ch) == '\t')
     #endif

    +/* regex code assumes isascii has its usual numeric meaning,
    +   even if the portable character set uses EBCDIC encoding,
    +   and even if wint_t is wider than int.  */
    +#ifndef _LIBC
    +# undef isascii
    +# define isascii(c) (((c) & ~0x7f) == 0)
    +#endif
    +
     #ifdef _LIBC
     # ifndef _RE_DEFINE_LOCALE_FUNCTIONS
     #  define _RE_DEFINE_LOCALE_FUNCTIONS 1
    @@ -335,7 +343,7 @@ typedef struct
         Idx idx;			/* for BACK_REF */
         re_context_type ctx_type;	/* for ANCHOR */
       } opr;
    -#if __GNUC__ >= 2 && !defined __STRICT_ANSI__
    +#if (__GNUC__ >= 2 || defined __clang__) && !defined __STRICT_ANSI__
       re_token_type_t type : 8;
     #else
       re_token_type_t type;
    @@ -841,10 +849,10 @@ re_string_elem_size_at (const re_string_t *pstr, Idx idx)
     #endif /* RE_ENABLE_I18N */

     #ifndef FALLTHROUGH
    -# if __GNUC__ < 7
    -#  define FALLTHROUGH ((void) 0)
    -# else
    +# if (__GNUC__ >= 7) || (__clang_major__ >= 10)
     #  define FALLTHROUGH __attribute__ ((__fallthrough__))
    +# else
    +#  define FALLTHROUGH ((void) 0)
     # endif
     #endif

    -- 
    2.25.1
  

On 19/01/2021 11:16, Vaseeharan Vinayagamoorthy wrote:
> After this commit [1], I am seeing  -Werror=undef when bootstapping glibc on aarch64-none-linux-gnu :
> 
> In file included from regex.c:70:0:
> regex_internal.h:852:26: error: "__clang_major__" is not defined [-Werror=undef]
>  # if (__GNUC__ >= 7) || (__clang_major__ >= 10)
>                           ^~~~~~~~~~~~~~~
> 
> The build/host/target setup is:
> Build: aarch64-none-linux-gnu (Ubuntu 14.04)
> Host: aarch64-none-linux-gnu
> Target: aarch64-none-linux-gnu
> 
> [1]
> commit c2a150d089fa096cb5f9e342da80fb30dc0d1953
> Author:     Adhemerval Zanella <adhemerval.zanella@linaro.org>
> AuthorDate: Tue Dec 29 17:32:25 2020 -0300
> Commit:     Adhemerval Zanella <adhemerval.zanella@linaro.org>
> CommitDate: Mon Jan 4 08:38:52 2021 -0300
> 
>     posix: Sync regex code with gnulib
>     
>     It sync with gnulib commit 43ee1a6bf.  The main change is 9682f18e9.
>     (which does not have a meaniful description).
>     
>     Checked on x86_64-linux-gnu.

Does the following help:

--
diff --git a/posix/regex_internal.h b/posix/regex_internal.h
index e31ac92674..944de219c3 100644
--- a/posix/regex_internal.h
+++ b/posix/regex_internal.h
@@ -849,7 +849,7 @@ re_string_elem_size_at (const re_string_t *pstr, Idx idx)
 #endif /* RE_ENABLE_I18N */
 
 #ifndef FALLTHROUGH
-# if (__GNUC__ >= 7) || (__clang_major__ >= 10)
+# if (__GNUC__ >= 7) || (defined __clang_major__ &&__clang_major__ >= 10)
 #  define FALLTHROUGH __attribute__ ((__fallthrough__))
 # else
 #  define FALLTHROUGH ((void) 0)
--

Paul, this seemed to a common pattern scatter on multiple file in gnulib.
Wouldn't be better to consolidate it on cdefs.h?

> On 30/12/2020, 20:15, "Libc-alpha on behalf of Adhemerval Zanella via Libc-alpha" <libc-alpha-bounces@sourceware.org on behalf of libc-alpha@sourceware.org> wrote:
> 
>     It sync with gnulib commit 43ee1a6bf.  The main change is 9682f18e9.
>     (which does not have a meaniful description).
> 
>     Checked on x86_64-linux-gnu.
>     ---
>      posix/regcomp.c        |  2 +-
>      posix/regex.h          | 17 ++++++++++++-----
>      posix/regex_internal.c | 19 ++++++++++---------
>      posix/regex_internal.h | 16 ++++++++++++----
>      4 files changed, 35 insertions(+), 19 deletions(-)
> 
>     diff --git a/posix/regcomp.c b/posix/regcomp.c
>     index 93bb0a0538..692928b0db 100644
>     --- a/posix/regcomp.c
>     +++ b/posix/regcomp.c
>     @@ -558,7 +558,7 @@ weak_alias (__regerror, regerror)
>      static const bitset_t utf8_sb_map =
>      {
>        /* Set the first 128 bits.  */
>     -# if defined __GNUC__ && !defined __STRICT_ANSI__
>     +# if (defined __GNUC__ || __clang_major__ >= 4) && !defined __STRICT_ANSI__
>        [0 ... 0x80 / BITSET_WORD_BITS - 1] = BITSET_WORD_MAX
>      # else
>      #  if 4 * BITSET_WORD_BITS < ASCII_CHARS
>     diff --git a/posix/regex.h b/posix/regex.h
>     index 5fe41c8685..7418e6c76f 100644
>     --- a/posix/regex.h
>     +++ b/posix/regex.h
>     @@ -612,7 +612,9 @@ extern int re_exec (const char *);
>         'configure' might #define 'restrict' to those words, so pick a
>         different name.  */
>      #ifndef _Restrict_
>     -# if defined __restrict || 2 < __GNUC__ + (95 <= __GNUC_MINOR__)
>     +# if defined __restrict \
>     +     || 2 < __GNUC__ + (95 <= __GNUC_MINOR__) \
>     +     || __clang_major__ >= 3
>      #  define _Restrict_ __restrict
>      # elif 199901L <= __STDC_VERSION__ || defined restrict
>      #  define _Restrict_ restrict
>     @@ -620,13 +622,18 @@ extern int re_exec (const char *);
>      #  define _Restrict_
>      # endif
>      #endif
>     -/* For [restrict], use glibc's __restrict_arr if available.
>     -   Otherwise, GCC 3.1 (not in C++ mode) and C99 support [restrict].  */
>     +/* For the ISO C99 syntax
>     +     array_name[restrict]
>     +   use glibc's __restrict_arr if available.
>     +   Otherwise, GCC 3.1 and clang support this syntax (but not in C++ mode).
>     +   Other ISO C99 compilers support it as well.  */
>      #ifndef _Restrict_arr_
>      # ifdef __restrict_arr
>      #  define _Restrict_arr_ __restrict_arr
>     -# elif ((199901L <= __STDC_VERSION__ || 3 < __GNUC__ + (1 <= __GNUC_MINOR__)) \
>     -        && !defined __GNUG__)
>     +# elif ((199901L <= __STDC_VERSION__ \
>     +         || 3 < __GNUC__ + (1 <= __GNUC_MINOR__) \
>     +         || __clang_major__ >= 3) \
>     +        && !defined __cplusplus)
>      #  define _Restrict_arr_ _Restrict_
>      # else
>      #  define _Restrict_arr_
>     diff --git a/posix/regex_internal.c b/posix/regex_internal.c
>     index e1b6b4d5af..ed0a13461b 100644
>     --- a/posix/regex_internal.c
>     +++ b/posix/regex_internal.c
>     @@ -300,18 +300,20 @@ build_wcs_upper_buffer (re_string_t *pstr)
>            while (byte_idx < end_idx)
>      	{
>      	  wchar_t wc;
>     +	  unsigned char ch = pstr->raw_mbs[pstr->raw_mbs_idx + byte_idx];
> 
>     -	  if (isascii (pstr->raw_mbs[pstr->raw_mbs_idx + byte_idx])
>     -	      && mbsinit (&pstr->cur_state))
>     +	  if (isascii (ch) && mbsinit (&pstr->cur_state))
>      	    {
>     -	      /* In case of a singlebyte character.  */
>     -	      pstr->mbs[byte_idx]
>     -		= toupper (pstr->raw_mbs[pstr->raw_mbs_idx + byte_idx]);
>      	      /* The next step uses the assumption that wchar_t is encoded
>      		 ASCII-safe: all ASCII values can be converted like this.  */
>     -	      pstr->wcs[byte_idx] = (wchar_t) pstr->mbs[byte_idx];
>     -	      ++byte_idx;
>     -	      continue;
>     +	      wchar_t wcu = __towupper (ch);
>     +	      if (isascii (wcu))
>     +		{
>     +		  pstr->mbs[byte_idx] = wcu;
>     +		  pstr->wcs[byte_idx] = wcu;
>     +		  byte_idx++;
>     +		  continue;
>     +		}
>      	    }
> 
>      	  remain_len = end_idx - byte_idx;
>     @@ -348,7 +350,6 @@ build_wcs_upper_buffer (re_string_t *pstr)
>      	    {
>      	      /* It is an invalid character, an incomplete character
>      		 at the end of the string, or '\0'.  Just use the byte.  */
>     -	      int ch = pstr->raw_mbs[pstr->raw_mbs_idx + byte_idx];
>      	      pstr->mbs[byte_idx] = ch;
>      	      /* And also cast it to wide char.  */
>      	      pstr->wcs[byte_idx++] = (wchar_t) ch;
>     diff --git a/posix/regex_internal.h b/posix/regex_internal.h
>     index 8c42586c42..4a3cf779bf 100644
>     --- a/posix/regex_internal.h
>     +++ b/posix/regex_internal.h
>     @@ -77,6 +77,14 @@
>      # define isblank(ch) ((ch) == ' ' || (ch) == '\t')
>      #endif
> 
>     +/* regex code assumes isascii has its usual numeric meaning,
>     +   even if the portable character set uses EBCDIC encoding,
>     +   and even if wint_t is wider than int.  */
>     +#ifndef _LIBC
>     +# undef isascii
>     +# define isascii(c) (((c) & ~0x7f) == 0)
>     +#endif
>     +
>      #ifdef _LIBC
>      # ifndef _RE_DEFINE_LOCALE_FUNCTIONS
>      #  define _RE_DEFINE_LOCALE_FUNCTIONS 1
>     @@ -335,7 +343,7 @@ typedef struct
>          Idx idx;			/* for BACK_REF */
>          re_context_type ctx_type;	/* for ANCHOR */
>        } opr;
>     -#if __GNUC__ >= 2 && !defined __STRICT_ANSI__
>     +#if (__GNUC__ >= 2 || defined __clang__) && !defined __STRICT_ANSI__
>        re_token_type_t type : 8;
>      #else
>        re_token_type_t type;
>     @@ -841,10 +849,10 @@ re_string_elem_size_at (const re_string_t *pstr, Idx idx)
>      #endif /* RE_ENABLE_I18N */
> 
>      #ifndef FALLTHROUGH
>     -# if __GNUC__ < 7
>     -#  define FALLTHROUGH ((void) 0)
>     -# else
>     +# if (__GNUC__ >= 7) || (__clang_major__ >= 10)
>      #  define FALLTHROUGH __attribute__ ((__fallthrough__))
>     +# else
>     +#  define FALLTHROUGH ((void) 0)
>      # endif
>      #endif
> 
>     -- 
>     2.25.1
> 
>
  

Adhemerval Zanella wrote:
> -# if (__GNUC__ >= 7) || (__clang_major__ >= 10)
> +# if (__GNUC__ >= 7) || (defined __clang_major__ &&__clang_major__ >= 10)

I would write it as:

+# if (__GNUC__ >= 7) || (defined __clang__ && __clang_major__ >= 10)

because 'defined __clang__' is the widely known way to test for a clang
(or derivate) compiler.

Bruno
  

* Adhemerval Zanella via Libc-alpha:

> Paul, this seemed to a common pattern scatter on multiple file in gnulib.
> Wouldn't be better to consolidate it on cdefs.h?

gnulib shouldn't be using cdefs.h on glibc systems, so I think a
separate header would be needed.

Thanks,
Florian
  

On 19/01/2021 13:52, Florian Weimer wrote:
> * Adhemerval Zanella via Libc-alpha:
> 
>> Paul, this seemed to a common pattern scatter on multiple file in gnulib.
>> Wouldn't be better to consolidate it on cdefs.h?
> 
> gnulib shouldn't be using cdefs.h on glibc systems, so I think a
> separate header would be needed.

What is the problem of using cdefs.h for internal implementation?
  

* Adhemerval Zanella:

> On 19/01/2021 13:52, Florian Weimer wrote:
>> * Adhemerval Zanella via Libc-alpha:
>> 
>>> Paul, this seemed to a common pattern scatter on multiple file in gnulib.
>>> Wouldn't be better to consolidate it on cdefs.h?
>> 
>> gnulib shouldn't be using cdefs.h on glibc systems, so I think a
>> separate header would be needed.
>
> What is the problem of using cdefs.h for internal implementation? 

Isn't regex_internal.h sync'ed with gnulib?

Thanks,
Florian
  

On 19/01/2021 14:16, Florian Weimer wrote:
> * Adhemerval Zanella:
> 
>> On 19/01/2021 13:52, Florian Weimer wrote:
>>> * Adhemerval Zanella via Libc-alpha:
>>>
>>>> Paul, this seemed to a common pattern scatter on multiple file in gnulib.
>>>> Wouldn't be better to consolidate it on cdefs.h?
>>>
>>> gnulib shouldn't be using cdefs.h on glibc systems, so I think a
>>> separate header would be needed.
>>
>> What is the problem of using cdefs.h for internal implementation? 
> 
> Isn't regex_internal.h sync'ed with gnulib?

Yes, and gnulib also has an internal cdefs (which we should sync btw).
  

On 1/19/21 7:43 AM, Bruno Haible wrote:
> Adhemerval Zanella wrote:
>> -# if (__GNUC__ >= 7) || (__clang_major__ >= 10)
>> +# if (__GNUC__ >= 7) || (defined __clang_major__ &&__clang_major__ >= 10)
> I would write it as:
> 
> +# if (__GNUC__ >= 7) || (defined __clang__ && __clang_major__ >= 10)

This line should be used only if _LIBC is defined, so we can simplify it 
to just "#if __GNUC__ >= 7" and thus not worry about Clang.

I see that Gnulib wasn't consistent about this, so I installed the 
attached patch to Gnulib to fix the issue here and elsewhere. The idea 
is that the Gnulib regex_internal.h can be copied back to glibc, and 
that the other uses in Gnulib should be similar.

On 1/19/21 6:43 AM, Adhemerval Zanella wrote:

> Paul, this seemed to a common pattern scatter on multiple file in gnulib.
> Wouldn't be better to consolidate it on cdefs.h?

We could append something like the following to cdefs.h, and switch to 
__attribute_fallthrough__ for modules shared between the two systems. Is 
that something you'd like to pursue? (We should also sync Gnulib cdefs.h 
back to glibc of course.)

#if defined __STDC_VERSION__ && 201710L < __STDC_VERSION__
# define __attribute_fallthrough__ [[__fallthrough__]]
#elif __GNUC_PREREQ (7, 0) || __glibc_has_attribute (__fallthrough__)
# define __attribute_fallthrough__ __attribute__ ((__fallthrough__))
#else
# define __attribute_fallthrough__ ((void) 0)
#endif
  

On 19/01/2021 23:55, Paul Eggert wrote:
> On 1/19/21 7:43 AM, Bruno Haible wrote:
>> Adhemerval Zanella wrote:
>>> -# if (__GNUC__ >= 7) || (__clang_major__ >= 10)
>>> +# if (__GNUC__ >= 7) || (defined __clang_major__ &&__clang_major__ >= 10)
>> I would write it as:
>>
>> +# if (__GNUC__ >= 7) || (defined __clang__ && __clang_major__ >= 10)
> 
> This line should be used only if _LIBC is defined, so we can simplify it to just "#if __GNUC__ >= 7" and thus not worry about Clang.
> 
> I see that Gnulib wasn't consistent about this, so I installed the attached patch to Gnulib to fix the issue here and elsewhere. The idea is that the Gnulib regex_internal.h can be copied back to glibc, and that the other uses in Gnulib should be similar.

Thanks, I will check with a bootstrap build and sync regex_internal.h
with glibc.

> 
> On 1/19/21 6:43 AM, Adhemerval Zanella wrote:
> 
>> Paul, this seemed to a common pattern scatter on multiple file in gnulib.
>> Wouldn't be better to consolidate it on cdefs.h?
> 
> We could append something like the following to cdefs.h, and switch to __attribute_fallthrough__ for modules shared between the two systems. Is that something you'd like to pursue? (We should also sync Gnulib cdefs.h back to glibc of course.)
> 
> #if defined __STDC_VERSION__ && 201710L < __STDC_VERSION__
> # define __attribute_fallthrough__ [[__fallthrough__]]
> #elif __GNUC_PREREQ (7, 0) || __glibc_has_attribute (__fallthrough__)
> # define __attribute_fallthrough__ __attribute__ ((__fallthrough__))
> #else
> # define __attribute_fallthrough__ ((void) 0)
> #endif

Yes, for 2.34 I will send some patches to sync the remaining gnulib
files and document from our part the shared files.
  

Thanks for the fix, Adhemerval Zanella.
I assume you would fix this in other files too, for example I think a similar patch is needed for posix/fnmatch.c.

fnmatch.c:67:26: error: "__clang_major__" is not defined [-Werror=undef]
 # if (__GNUC__ >= 7) || (__clang_major__ >= 10)
                          ^~~~~~~~~~~~~~~



On 20/01/2021, 11:27, "Adhemerval Zanella" <adhemerval.zanella@linaro.org> wrote:



    On 19/01/2021 23:55, Paul Eggert wrote:
    > On 1/19/21 7:43 AM, Bruno Haible wrote:
    >> Adhemerval Zanella wrote:
    >>> -# if (__GNUC__ >= 7) || (__clang_major__ >= 10)
    >>> +# if (__GNUC__ >= 7) || (defined __clang_major__ &&__clang_major__ >= 10)
    >> I would write it as:
    >>
    >> +# if (__GNUC__ >= 7) || (defined __clang__ && __clang_major__ >= 10)
    > 
    > This line should be used only if _LIBC is defined, so we can simplify it to just "#if __GNUC__ >= 7" and thus not worry about Clang.
    > 
    > I see that Gnulib wasn't consistent about this, so I installed the attached patch to Gnulib to fix the issue here and elsewhere. The idea is that the Gnulib regex_internal.h can be copied back to glibc, and that the other uses in Gnulib should be similar.

    Thanks, I will check with a bootstrap build and sync regex_internal.h
    with glibc.

    > 
    > On 1/19/21 6:43 AM, Adhemerval Zanella wrote:
    > 
    >> Paul, this seemed to a common pattern scatter on multiple file in gnulib.
    >> Wouldn't be better to consolidate it on cdefs.h?
    > 
    > We could append something like the following to cdefs.h, and switch to __attribute_fallthrough__ for modules shared between the two systems. Is that something you'd like to pursue? (We should also sync Gnulib cdefs.h back to glibc of course.)
    > 
    > #if defined __STDC_VERSION__ && 201710L < __STDC_VERSION__
    > # define __attribute_fallthrough__ [[__fallthrough__]]
    > #elif __GNUC_PREREQ (7, 0) || __glibc_has_attribute (__fallthrough__)
    > # define __attribute_fallthrough__ __attribute__ ((__fallthrough__))
    > #else
    > # define __attribute_fallthrough__ ((void) 0)
    > #endif

    Yes, for 2.34 I will send some patches to sync the remaining gnulib
    files and document from our part the shared files.
  

Right, this did not showed up on build-many-glibc.py.  My plan is to touch
as little code as possible since we are in slush freeze and sync again with
gnulib once we set development for 2.34.

I will fix this as well.

On 20/01/2021 12:32, Vaseeharan Vinayagamoorthy wrote:
> Thanks for the fix, Adhemerval Zanella.
> I assume you would fix this in other files too, for example I think a similar patch is needed for posix/fnmatch.c.
> 
> fnmatch.c:67:26: error: "__clang_major__" is not defined [-Werror=undef]
>  # if (__GNUC__ >= 7) || (__clang_major__ >= 10)
>                           ^~~~~~~~~~~~~~~
> 
> 
> 
> On 20/01/2021, 11:27, "Adhemerval Zanella" <adhemerval.zanella@linaro.org> wrote:
> 
> 
> 
>     On 19/01/2021 23:55, Paul Eggert wrote:
>     > On 1/19/21 7:43 AM, Bruno Haible wrote:
>     >> Adhemerval Zanella wrote:
>     >>> -# if (__GNUC__ >= 7) || (__clang_major__ >= 10)
>     >>> +# if (__GNUC__ >= 7) || (defined __clang_major__ &&__clang_major__ >= 10)
>     >> I would write it as:
>     >>
>     >> +# if (__GNUC__ >= 7) || (defined __clang__ && __clang_major__ >= 10)
>     > 
>     > This line should be used only if _LIBC is defined, so we can simplify it to just "#if __GNUC__ >= 7" and thus not worry about Clang.
>     > 
>     > I see that Gnulib wasn't consistent about this, so I installed the attached patch to Gnulib to fix the issue here and elsewhere. The idea is that the Gnulib regex_internal.h can be copied back to glibc, and that the other uses in Gnulib should be similar.
> 
>     Thanks, I will check with a bootstrap build and sync regex_internal.h
>     with glibc.
> 
>     > 
>     > On 1/19/21 6:43 AM, Adhemerval Zanella wrote:
>     > 
>     >> Paul, this seemed to a common pattern scatter on multiple file in gnulib.
>     >> Wouldn't be better to consolidate it on cdefs.h?
>     > 
>     > We could append something like the following to cdefs.h, and switch to __attribute_fallthrough__ for modules shared between the two systems. Is that something you'd like to pursue? (We should also sync Gnulib cdefs.h back to glibc of course.)
>     > 
>     > #if defined __STDC_VERSION__ && 201710L < __STDC_VERSION__
>     > # define __attribute_fallthrough__ [[__fallthrough__]]
>     > #elif __GNUC_PREREQ (7, 0) || __glibc_has_attribute (__fallthrough__)
>     > # define __attribute_fallthrough__ __attribute__ ((__fallthrough__))
>     > #else
>     > # define __attribute_fallthrough__ ((void) 0)
>     > #endif
> 
>     Yes, for 2.34 I will send some patches to sync the remaining gnulib
>     files and document from our part the shared files.
>
  

On 1/20/21 3:27 AM, Adhemerval Zanella wrote:
>> #if defined __STDC_VERSION__ && 201710L < __STDC_VERSION__
>> # define __attribute_fallthrough__ [[__fallthrough__]]
>> #elif __GNUC_PREREQ (7, 0) || __glibc_has_attribute (__fallthrough__)
>> # define __attribute_fallthrough__ __attribute__ ((__fallthrough__))
>> #else
>> # define __attribute_fallthrough__ ((void) 0)
>> #endif
> Yes, for 2.34 I will send some patches to sync the remaining gnulib
> files and document from our part the shared files.

On thinking about it more (for 2.34), I find that I would prefer the 
current Gnulib practice of using 'FALLTHROUGH;' to using 
'__attribute_fallthrough__;' in code, and so suggest Gnulib's approach 
of having an attribute.h file (purely for internal glibc use) that has 
"#define FALLTHROUGH __attribute__ (__fallthrough__)' etc.