[PATCH 0/6] Fix -Os build

[PATCH 0/6] Fix -Os build

[Adhemerval Zanella]

This patchset fix the glibc build including tests with -Os.  I checked
with GCC 12 for all supported ABI and run a full make check for x86_64
and i686 without any regressions.

Adhemerval Zanella (5):
  posix: Suppress -Os warnings on fnmatch
  posix: Suppress -Os may be used uninitialized warnings on regexec
  rt: Initialize mq_send input on tst-mqueue{5,6}
  sunrpc: Suppress GCC -Os warning on user2netname
  x86: Fix -Os build (BZ #29576)

Martin Jansa (1):
  locale: prevent maybe-uninitialized errors with -Os [BZ #19444]

 locale/weight.h                        |  7 ++++++
 posix/fnmatch_loop.c                   | 31 ++++++++++++++++++++++++++
 posix/regexec.c                        |  6 +++++
 rt/tst-mqueue5.c                       |  2 +-
 rt/tst-mqueue6.c                       |  2 +-
 sunrpc/netname.c                       |  6 +++++
 sysdeps/x86_64/multiarch/rtld-strcpy.S | 18 +++++++++++++++
 7 files changed, 70 insertions(+), 2 deletions(-)
 create mode 100644 sysdeps/x86_64/multiarch/rtld-strcpy.S

-- 
2.34.1

[PATCH 1/6] locale: prevent maybe-uninitialized errors with -Os [BZ #19444]

[Adhemerval Zanella]

From: Martin Jansa <Martin.Jansa@gmail.com>

Fixes following error when building  with -Os:
| In file included from strcoll_l.c:43:
| strcoll_l.c: In function '__strcoll_l':
| ../locale/weight.h:31:26: error: 'seq2.back_us' may be used
uninitialized in this function [-Werror=maybe-uninitialized]
|    int_fast32_t i = table[*(*cpp)++];
|                           ^~~~~~~~~
| strcoll_l.c:304:18: note: 'seq2.back_us' was declared here
|    coll_seq seq1, seq2;
|                   ^~~~
| In file included from strcoll_l.c:43:
| ../locale/weight.h:31:26: error: 'seq1.back_us' may be used
uninitialized in this function [-Werror=maybe-uninitialized]
|    int_fast32_t i = table[*(*cpp)++];
|                           ^~~~~~~~~
| strcoll_l.c:304:12: note: 'seq1.back_us' was declared here
|    coll_seq seq1, seq2;
|             ^~~~
---
 locale/weight.h | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/locale/weight.h b/locale/weight.h
index 8be2d220f8..4a4d5aa6b2 100644
--- a/locale/weight.h
+++ b/locale/weight.h
@@ -27,7 +27,14 @@ findidx (const int32_t *table,
 	 const unsigned char *extra,
 	 const unsigned char **cpp, size_t len)
 {
+  /* With GCC 8 when compiling with -Os the compiler warns that
+     seq1.back_us and seq2.back_us might be used uninitialized.
+     This uninitialized use is impossible for the same reason
+     as described in comments in locale/weightwc.h.  */
+  DIAG_PUSH_NEEDS_COMMENT;
+  DIAG_IGNORE_Os_NEEDS_COMMENT (8, "-Wmaybe-uninitialized");
   int32_t i = table[*(*cpp)++];
+  DIAG_POP_NEEDS_COMMENT;
   const unsigned char *cp;
   const unsigned char *usrc;
 
-- 
2.34.1

[PATCH 2/6] posix: Suppress -Os warnings on fnmatch

[Adhemerval Zanella]

GCC with -Os issues some may uninitialized warnings on fnmatch code.
All of the variables are already set when they are accessed on the
loop prior.

Checked on x86_64-linux-gnu and i686-linux-gnu.
---
 posix/fnmatch_loop.c | 31 +++++++++++++++++++++++++++++++
 1 file changed, 31 insertions(+)

diff --git a/posix/fnmatch_loop.c b/posix/fnmatch_loop.c
index 9445ed9c58..23eeffc79c 100644
--- a/posix/fnmatch_loop.c
+++ b/posix/fnmatch_loop.c
@@ -530,6 +530,14 @@ FCT (const CHAR *pattern, const CHAR *string, const CHAR *string_end,
                               {
                                 /* Compare the byte sequence but only if
                                    this is not part of a range.  */
+
+				/* The compiler might warn that idx may be
+				   used uninitialized, however it will be
+				   reached iff elem < table_size which means
+				   that it was properly set in the loop
+				   above.   */
+                                DIAG_PUSH_NEEDS_COMMENT;
+                                DIAG_IGNORE_Os_NEEDS_COMMENT (8, "-Wmaybe-uninitialized");
                                 if (! is_range
 
 # if WIDE_CHAR_VERSION
@@ -542,11 +550,19 @@ FCT (const CHAR *pattern, const CHAR *string, const CHAR *string_end,
                                     n += c1 - 1;
                                     goto matched;
                                   }
+                                DIAG_POP_NEEDS_COMMENT;
 
                                 /* Get the collation sequence value.  */
                                 is_seqval = true;
 # if WIDE_CHAR_VERSION
+                                /* The compile might warn that wextra may be
+				   used uninitialized and similar to 'idx'
+				   above it will be properly set by the loop.
+				   */
+                                DIAG_PUSH_NEEDS_COMMENT;
+                                DIAG_IGNORE_Os_NEEDS_COMMENT (8, "-Wmaybe-uninitialized");
                                 cold = wextra[1 + wextra[0]];
+                                DIAG_POP_NEEDS_COMMENT;
 # else
                                 idx += 1 + extra[idx];
                                 /* Adjust for the alignment.  */
@@ -723,9 +739,24 @@ FCT (const CHAR *pattern, const CHAR *string, const CHAR *string_end,
                                     /* Get the collation sequence value.  */
                                     is_seqval = true;
 # if WIDE_CHAR_VERSION
+                                    /* The compiler might warn that wextra may
+                                       be used uninitialized, however it will
+                                       be reached iff elem < table_size which
+                                       means that it was properly set in the
+                                       loop above.   */
+                                    DIAG_PUSH_NEEDS_COMMENT;
+                                    DIAG_IGNORE_Os_NEEDS_COMMENT (8, "-Wmaybe-uninitialized");
                                     cend = wextra[1 + wextra[0]];
+                                    DIAG_POP_NEEDS_COMMENT;
 # else
+				    /* The compile might warn that idx may
+				       be used uninitialized and similar to
+				       wextra above it will be properly set by
+				       the loop.   */
+                                    DIAG_PUSH_NEEDS_COMMENT;
+                                    DIAG_IGNORE_Os_NEEDS_COMMENT (8, "-Wmaybe-uninitialized");
                                     idx += 1 + extra[idx];
+                                    DIAG_POP_NEEDS_COMMENT;
                                     /* Adjust for the alignment.  */
                                     idx = (idx + 3) & ~3;
                                     cend = *((int32_t *) &extra[idx]);
-- 
2.34.1

[PATCH 3/6] posix: Suppress -Os may be used uninitialized warnings on regexec

[Adhemerval Zanella]

GCC with -Os issues may uninitialized warnings on regexec code.

Checked on x86_64-linux-gnu and i686-linux-gnu.
---
 posix/regexec.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/posix/regexec.c b/posix/regexec.c
index cffeaf2845..386a757f35 100644
--- a/posix/regexec.c
+++ b/posix/regexec.c
@@ -3768,7 +3768,13 @@ check_node_accept_bytes (const re_dfa_t *dfa, Idx node_idx,
 	      _NL_CURRENT (LC_COLLATE, _NL_COLLATE_SYMB_EXTRAMB);
 	  for (i = 0; i < cset->ncoll_syms; ++i)
 	    {
+	      /* The compiler might warn that extra may be used uninitialized,
+		 however the loop will be executed iff ncoll_syms is larger
+		 than 0,which means extra will be already initialized.  */
+	      DIAG_PUSH_NEEDS_COMMENT;
+	      DIAG_IGNORE_Os_NEEDS_COMMENT (8, "-Wmaybe-uninitialized");
 	      const unsigned char *coll_sym = extra + cset->coll_syms[i];
+	      DIAG_POP_NEEDS_COMMENT;
 	      /* Compare the length of input collating element and
 		 the length of current collating element.  */
 	      if (*coll_sym != elem_len)
-- 
2.34.1

[PATCH 4/6] rt: Initialize mq_send input on tst-mqueue{5,6}

[Adhemerval Zanella]

GCC with -Os warns that the mq_send input may be used uninitialized.
Although for the tests the data content sent is not important, since
both tests checks only if mq_notify was properly set, the warning is
correct and data is indeed uninitialized.

Checked on x86_64-linux-gnu and i686-linux-gnu.
---
 rt/tst-mqueue5.c | 2 +-
 rt/tst-mqueue6.c | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/rt/tst-mqueue5.c b/rt/tst-mqueue5.c
index 70d97a36c2..2b19b6a031 100644
--- a/rt/tst-mqueue5.c
+++ b/rt/tst-mqueue5.c
@@ -58,7 +58,7 @@ rtmin_handler (int sig, siginfo_t *info, void *ctx)
 static int
 (mqsend) (mqd_t q, int line)
 {
-  char c;
+  char c = 0;
   if (mq_send (q, &c, 1, 1) != 0)
     {
       printf ("mq_send on line %d failed with: %m\n", line);
diff --git a/rt/tst-mqueue6.c b/rt/tst-mqueue6.c
index bc875f101e..a22ac05aca 100644
--- a/rt/tst-mqueue6.c
+++ b/rt/tst-mqueue6.c
@@ -40,7 +40,7 @@
 static int
 (mqsend) (mqd_t q, int line)
 {
-  char c;
+  char c = 0;
   if (mq_send (q, &c, 1, 1) != 0)
     {
       printf ("mq_send on line %d failed with: %m\n", line);
-- 
2.34.1

[PATCH 5/6] sunrpc: Suppress GCC -Os warning on user2netname

[Adhemerval Zanella]

GCC with -Os warns that sprint might overflow:

  netname.c: In function ‘user2netname’:
  netname.c:51:28: error: ‘%s’ directive writing up to 255 bytes into a
  region of size between 239 and 249 [-Werror=format-overflow=]
     51 |   sprintf (netname, "%s.%d@%s", OPSYS, uid, dfltdom);
        |                            ^~               ~~~~~~~
  netname.c:51:3: note: ‘sprintf’ output between 8 and 273 bytes into a
  destination of size 256
     51 |   sprintf (netname, "%s.%d@%s", OPSYS, uid, dfltdom);
        |   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  cc1: all warnings being treated as errors

However the code does test prior the sprintf call that dfltdom plus
the required extra space for OPSYS, uid, and extra character will not
overflow and return 0 instead.

Checked on x86_64-linux-gnu and i686-linux-gnu.
---
 sunrpc/netname.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/sunrpc/netname.c b/sunrpc/netname.c
index bf7f0b81c4..c1d1c43e50 100644
--- a/sunrpc/netname.c
+++ b/sunrpc/netname.c
@@ -20,6 +20,7 @@
 #include <string.h>
 #include <rpc/rpc.h>
 #include <shlib-compat.h>
+#include <libc-diag.h>
 
 #include "nsswitch.h"
 
@@ -48,7 +49,12 @@ user2netname (char netname[MAXNETNAMELEN + 1], const uid_t uid,
   if ((strlen (dfltdom) + OPSYS_LEN + 3 + MAXIPRINT) > (size_t) MAXNETNAMELEN)
     return 0;
 
+  /* GCC with -Os warns that sprint might overflow while handling dfltdom,
+     however the above test does check if an overflow would happen.  */
+  DIAG_PUSH_NEEDS_COMMENT;
+  DIAG_IGNORE_Os_NEEDS_COMMENT (8, "-Wformat-overflow");
   sprintf (netname, "%s.%d@%s", OPSYS, uid, dfltdom);
+  DIAG_POP_NEEDS_COMMENT;
   i = strlen (netname);
   if (netname[i - 1] == '.')
     netname[i - 1] = '\0';
-- 
2.34.1

[PATCH 6/6] x86: Fix -Os build (BZ #29576)

[Adhemerval Zanella]

The compiler might transform __stpcpy calls (which are routed to
__builtin_stpcpy as an optimization) to strcpy and x86_64 strcpy
multiarch implementation does not build any working symbol due
ISA_SHOULD_BUILD not being evaluated for IS_IN(rtld).

Checked on x86_64-linux-gnu.
---
 sysdeps/x86_64/multiarch/rtld-strcpy.S | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)
 create mode 100644 sysdeps/x86_64/multiarch/rtld-strcpy.S

diff --git a/sysdeps/x86_64/multiarch/rtld-strcpy.S b/sysdeps/x86_64/multiarch/rtld-strcpy.S
new file mode 100644
index 0000000000..19439c553d
--- /dev/null
+++ b/sysdeps/x86_64/multiarch/rtld-strcpy.S
@@ -0,0 +1,18 @@
+/* Copyright (C) 2022 Free Software Foundation, Inc.
+   This file is part of the GNU C Library.
+
+   The GNU C Library is free software; you can redistribute it and/or
+   modify it under the terms of the GNU Lesser General Public
+   License as published by the Free Software Foundation; either
+   version 2.1 of the License, or (at your option) any later version.
+
+   The GNU C Library is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+   Lesser General Public License for more details.
+
+   You should have received a copy of the GNU Lesser General Public
+   License along with the GNU C Library; if not, see
+   <https://www.gnu.org/licenses/>.  */
+
+#include "../strcpy.S"
-- 
2.34.1

Re: [PATCH 1/6] locale: prevent maybe-uninitialized errors with -Os [BZ #19444]

[Carlos O'Donell]

On Wed, Sep 21, 2022 at 10:51:03AM -0300, Adhemerval Zanella via Libc-alpha wrote:
> From: Martin Jansa <Martin.Jansa@gmail.com>
> 
> Fixes following error when building  with -Os:
> | In file included from strcoll_l.c:43:
> | strcoll_l.c: In function '__strcoll_l':
> | ../locale/weight.h:31:26: error: 'seq2.back_us' may be used
> uninitialized in this function [-Werror=maybe-uninitialized]
> |    int_fast32_t i = table[*(*cpp)++];
> |                           ^~~~~~~~~
> | strcoll_l.c:304:18: note: 'seq2.back_us' was declared here
> |    coll_seq seq1, seq2;
> |                   ^~~~
> | In file included from strcoll_l.c:43:
> | ../locale/weight.h:31:26: error: 'seq1.back_us' may be used
> uninitialized in this function [-Werror=maybe-uninitialized]
> |    int_fast32_t i = table[*(*cpp)++];
> |                           ^~~~~~~~~
> | strcoll_l.c:304:12: note: 'seq1.back_us' was declared here
> |    coll_seq seq1, seq2;
> |             ^~~~

LGTM.

No regresions on x86_64.

Reviewed-by: Carlos O'Donell <carlos@redhat.com>
Tested-by: Carlos O'Donell <carlos@redhat.com>

> ---
>  locale/weight.h | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/locale/weight.h b/locale/weight.h
> index 8be2d220f8..4a4d5aa6b2 100644
> --- a/locale/weight.h
> +++ b/locale/weight.h
> @@ -27,7 +27,14 @@ findidx (const int32_t *table,
>  	 const unsigned char *extra,
>  	 const unsigned char **cpp, size_t len)
>  {
> +  /* With GCC 8 when compiling with -Os the compiler warns that
> +     seq1.back_us and seq2.back_us might be used uninitialized.
> +     This uninitialized use is impossible for the same reason
> +     as described in comments in locale/weightwc.h.  */

OK. Agreed.

> +  DIAG_PUSH_NEEDS_COMMENT;
> +  DIAG_IGNORE_Os_NEEDS_COMMENT (8, "-Wmaybe-uninitialized");
>    int32_t i = table[*(*cpp)++];
> +  DIAG_POP_NEEDS_COMMENT;
>    const unsigned char *cp;
>    const unsigned char *usrc;
>  
> -- 
> 2.34.1
> 

Re: [PATCH 0/6] Fix -Os build

[Carlos O'Donell]

On Wed, Sep 21, 2022 at 10:51:02AM -0300, Adhemerval Zanella via Libc-alpha wrote:
> This patchset fix the glibc build including tests with -Os.  I checked
> with GCC 12 for all supported ABI and run a full make check for x86_64
> and i686 without any regressions.

Thanks for looking into this, I think that both -O2 and -Os are key
baselines that we need to be testing. I should setup a CI/CD builder
that checks -Os.

> Adhemerval Zanella (5):
>   posix: Suppress -Os warnings on fnmatch
>   posix: Suppress -Os may be used uninitialized warnings on regexec
>   rt: Initialize mq_send input on tst-mqueue{5,6}
>   sunrpc: Suppress GCC -Os warning on user2netname
>   x86: Fix -Os build (BZ #29576)
> 
> Martin Jansa (1):
>   locale: prevent maybe-uninitialized errors with -Os [BZ #19444]
> 
>  locale/weight.h                        |  7 ++++++
>  posix/fnmatch_loop.c                   | 31 ++++++++++++++++++++++++++
>  posix/regexec.c                        |  6 +++++
>  rt/tst-mqueue5.c                       |  2 +-
>  rt/tst-mqueue6.c                       |  2 +-
>  sunrpc/netname.c                       |  6 +++++
>  sysdeps/x86_64/multiarch/rtld-strcpy.S | 18 +++++++++++++++
>  7 files changed, 70 insertions(+), 2 deletions(-)
>  create mode 100644 sysdeps/x86_64/multiarch/rtld-strcpy.S
> 
> -- 
> 2.34.1
> 

Re: [PATCH 2/6] posix: Suppress -Os warnings on fnmatch

[Carlos O'Donell]

On Wed, Sep 21, 2022 at 10:51:04AM -0300, Adhemerval Zanella via Libc-alpha wrote:
> GCC with -Os issues some may uninitialized warnings on fnmatch code.
> All of the variables are already set when they are accessed on the
> loop prior.
> 
> Checked on x86_64-linux-gnu and i686-linux-gnu.

LGTM.

This touches gnulib code and may require merging upstream or harmonizing
in a different way. For now this looks ok. There are some alternatives,
but this is OK.

Reviewed-by: Carlos O'Donell <carlos@redhat.com>
Tested-by: Carlos O'Donell <carlos@redhat.com>

> ---
>  posix/fnmatch_loop.c | 31 +++++++++++++++++++++++++++++++
>  1 file changed, 31 insertions(+)
> 
> diff --git a/posix/fnmatch_loop.c b/posix/fnmatch_loop.c
> index 9445ed9c58..23eeffc79c 100644
> --- a/posix/fnmatch_loop.c
> +++ b/posix/fnmatch_loop.c
> @@ -530,6 +530,14 @@ FCT (const CHAR *pattern, const CHAR *string, const CHAR *string_end,
>                                {
>                                  /* Compare the byte sequence but only if
>                                     this is not part of a range.  */
> +
> +				/* The compiler might warn that idx may be
> +				   used uninitialized, however it will be
> +				   reached iff elem < table_size which means
> +				   that it was properly set in the loop
> +				   above.   */
> +                                DIAG_PUSH_NEEDS_COMMENT;
> +                                DIAG_IGNORE_Os_NEEDS_COMMENT (8, "-Wmaybe-uninitialized");
>                                  if (! is_range
>  
>  # if WIDE_CHAR_VERSION
> @@ -542,11 +550,19 @@ FCT (const CHAR *pattern, const CHAR *string, const CHAR *string_end,
>                                      n += c1 - 1;
>                                      goto matched;
>                                    }
> +                                DIAG_POP_NEEDS_COMMENT;

OK.

>  
>                                  /* Get the collation sequence value.  */
>                                  is_seqval = true;
>  # if WIDE_CHAR_VERSION
> +                                /* The compile might warn that wextra may be
> +				   used uninitialized and similar to 'idx'
> +				   above it will be properly set by the loop.
> +				   */
> +                                DIAG_PUSH_NEEDS_COMMENT;
> +                                DIAG_IGNORE_Os_NEEDS_COMMENT (8, "-Wmaybe-uninitialized");
>                                  cold = wextra[1 + wextra[0]];
> +                                DIAG_POP_NEEDS_COMMENT;

OK.

>  # else
>                                  idx += 1 + extra[idx];
>                                  /* Adjust for the alignment.  */
> @@ -723,9 +739,24 @@ FCT (const CHAR *pattern, const CHAR *string, const CHAR *string_end,
>                                      /* Get the collation sequence value.  */
>                                      is_seqval = true;
>  # if WIDE_CHAR_VERSION
> +                                    /* The compiler might warn that wextra may
> +                                       be used uninitialized, however it will
> +                                       be reached iff elem < table_size which
> +                                       means that it was properly set in the
> +                                       loop above.   */
> +                                    DIAG_PUSH_NEEDS_COMMENT;
> +                                    DIAG_IGNORE_Os_NEEDS_COMMENT (8, "-Wmaybe-uninitialized");
>                                      cend = wextra[1 + wextra[0]];
> +                                    DIAG_POP_NEEDS_COMMENT;

OK.

>  # else
> +				    /* The compile might warn that idx may
> +				       be used uninitialized and similar to
> +				       wextra above it will be properly set by
> +				       the loop.   */
> +                                    DIAG_PUSH_NEEDS_COMMENT;
> +                                    DIAG_IGNORE_Os_NEEDS_COMMENT (8, "-Wmaybe-uninitialized");
>                                      idx += 1 + extra[idx];
> +                                    DIAG_POP_NEEDS_COMMENT;

OK.

>                                      /* Adjust for the alignment.  */
>                                      idx = (idx + 3) & ~3;
>                                      cend = *((int32_t *) &extra[idx]);
> -- 
> 2.34.1
> 

Re: [PATCH 3/6] posix: Suppress -Os may be used uninitialized warnings on regexec

[Carlos O'Donell]

On Wed, Sep 21, 2022 at 10:51:05AM -0300, Adhemerval Zanella via Libc-alpha wrote:
> GCC with -Os issues may uninitialized warnings on regexec code.

LGTM.

This is also gnulib code. Needs eventual harmonization.

Reviewed-by: Carlos O'Donell <carlos@redhat.com>
Tested-by: Carlos O'Donell <carlos@redhat.com>

> Checked on x86_64-linux-gnu and i686-linux-gnu.
> ---
>  posix/regexec.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/posix/regexec.c b/posix/regexec.c
> index cffeaf2845..386a757f35 100644
> --- a/posix/regexec.c
> +++ b/posix/regexec.c
> @@ -3768,7 +3768,13 @@ check_node_accept_bytes (const re_dfa_t *dfa, Idx node_idx,
>  	      _NL_CURRENT (LC_COLLATE, _NL_COLLATE_SYMB_EXTRAMB);
>  	  for (i = 0; i < cset->ncoll_syms; ++i)
>  	    {
> +	      /* The compiler might warn that extra may be used uninitialized,
> +		 however the loop will be executed iff ncoll_syms is larger
> +		 than 0,which means extra will be already initialized.  */
> +	      DIAG_PUSH_NEEDS_COMMENT;
> +	      DIAG_IGNORE_Os_NEEDS_COMMENT (8, "-Wmaybe-uninitialized");
>  	      const unsigned char *coll_sym = extra + cset->coll_syms[i];
> +	      DIAG_POP_NEEDS_COMMENT;

OK. Agreed.

>  	      /* Compare the length of input collating element and
>  		 the length of current collating element.  */
>  	      if (*coll_sym != elem_len)
> -- 
> 2.34.1
> 

Re: [PATCH 4/6] rt: Initialize mq_send input on tst-mqueue{5,6}

[Carlos O'Donell]

On Wed, Sep 21, 2022 at 10:51:06AM -0300, Adhemerval Zanella via Libc-alpha wrote:
> GCC with -Os warns that the mq_send input may be used uninitialized.
> Although for the tests the data content sent is not important, since
> both tests checks only if mq_notify was properly set, the warning is
> correct and data is indeed uninitialized.

LGTM.

No regressions on x86_64. Pre-commit CI didn't run for these patches.

Reviewed-by: Carlos O'Donell <carlos@redhat.com>
Tested-by: Carlos O'Donell <carlos@redhat.com>

> Checked on x86_64-linux-gnu and i686-linux-gnu.
> ---
>  rt/tst-mqueue5.c | 2 +-
>  rt/tst-mqueue6.c | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/rt/tst-mqueue5.c b/rt/tst-mqueue5.c
> index 70d97a36c2..2b19b6a031 100644
> --- a/rt/tst-mqueue5.c
> +++ b/rt/tst-mqueue5.c
> @@ -58,7 +58,7 @@ rtmin_handler (int sig, siginfo_t *info, void *ctx)
>  static int
>  (mqsend) (mqd_t q, int line)
>  {
> -  char c;
> +  char c = 0;

OK. Agreed. Data needs to be initialized.

>    if (mq_send (q, &c, 1, 1) != 0)
>      {
>        printf ("mq_send on line %d failed with: %m\n", line);
> diff --git a/rt/tst-mqueue6.c b/rt/tst-mqueue6.c
> index bc875f101e..a22ac05aca 100644
> --- a/rt/tst-mqueue6.c
> +++ b/rt/tst-mqueue6.c
> @@ -40,7 +40,7 @@
>  static int
>  (mqsend) (mqd_t q, int line)
>  {
> -  char c;
> +  char c = 0;

OK. Agreed. Data needs to be initialized.

>    if (mq_send (q, &c, 1, 1) != 0)
>      {
>        printf ("mq_send on line %d failed with: %m\n", line);
> -- 
> 2.34.1
> 

Re: [PATCH 5/6] sunrpc: Suppress GCC -Os warning on user2netname

[Carlos O'Donell]

On Wed, Sep 21, 2022 at 10:51:07AM -0300, Adhemerval Zanella via Libc-alpha wrote:
> GCC with -Os warns that sprint might overflow:
> 
>   netname.c: In function ‘user2netname’:
>   netname.c:51:28: error: ‘%s’ directive writing up to 255 bytes into a
>   region of size between 239 and 249 [-Werror=format-overflow=]
>      51 |   sprintf (netname, "%s.%d@%s", OPSYS, uid, dfltdom);
>         |                            ^~               ~~~~~~~
>   netname.c:51:3: note: ‘sprintf’ output between 8 and 273 bytes into a
>   destination of size 256
>      51 |   sprintf (netname, "%s.%d@%s", OPSYS, uid, dfltdom);
>         |   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>   cc1: all warnings being treated as errors
> 
> However the code does test prior the sprintf call that dfltdom plus
> the required extra space for OPSYS, uid, and extra character will not
> overflow and return 0 instead.

LGTM.

No regressions on x86_64.

Reviewed-by: Carlos O'Donell <carlos@redhat.com>
Tested-by: Carlos O'Donell <carlos@redhat.com>

> Checked on x86_64-linux-gnu and i686-linux-gnu.
> ---
>  sunrpc/netname.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/sunrpc/netname.c b/sunrpc/netname.c
> index bf7f0b81c4..c1d1c43e50 100644
> --- a/sunrpc/netname.c
> +++ b/sunrpc/netname.c
> @@ -20,6 +20,7 @@
>  #include <string.h>
>  #include <rpc/rpc.h>
>  #include <shlib-compat.h>
> +#include <libc-diag.h>

OK.

>  
>  #include "nsswitch.h"
>  
> @@ -48,7 +49,12 @@ user2netname (char netname[MAXNETNAMELEN + 1], const uid_t uid,
>    if ((strlen (dfltdom) + OPSYS_LEN + 3 + MAXIPRINT) > (size_t) MAXNETNAMELEN)
>      return 0;
>  
> +  /* GCC with -Os warns that sprint might overflow while handling dfltdom,
> +     however the above test does check if an overflow would happen.  */
> +  DIAG_PUSH_NEEDS_COMMENT;
> +  DIAG_IGNORE_Os_NEEDS_COMMENT (8, "-Wformat-overflow");
>    sprintf (netname, "%s.%d@%s", OPSYS, uid, dfltdom);
> +  DIAG_POP_NEEDS_COMMENT;

OK. Agreed. The code above does that check.

>    i = strlen (netname);
>    if (netname[i - 1] == '.')
>      netname[i - 1] = '\0';
> -- 
> 2.34.1
> 

Re: [PATCH 6/6] x86: Fix -Os build (BZ #29576)

[Carlos O'Donell]

On Wed, Sep 21, 2022 at 10:51:08AM -0300, Adhemerval Zanella via Libc-alpha wrote:
> The compiler might transform __stpcpy calls (which are routed to
> __builtin_stpcpy as an optimization) to strcpy and x86_64 strcpy
> multiarch implementation does not build any working symbol due
> ISA_SHOULD_BUILD not being evaluated for IS_IN(rtld).

Ohhhhhh... that is interesting. This changes the strcpy used in rtld for
all x86_64 build options, and I'm going to ACK this, but we may need to
revisit this if it shows up in a profile.

CC'ing HJ here in case he wants to comment as a machine maintainer.

LGTM.

No regressions on x86_64.

Reviewed-by: Carlos O'Donell <carlos@redhat.com>
Tested-by: Carlos O'Donell <carlos@redhat.com>
 
> Checked on x86_64-linux-gnu.
> ---
>  sysdeps/x86_64/multiarch/rtld-strcpy.S | 18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)
>  create mode 100644 sysdeps/x86_64/multiarch/rtld-strcpy.S
> 
> diff --git a/sysdeps/x86_64/multiarch/rtld-strcpy.S b/sysdeps/x86_64/multiarch/rtld-strcpy.S
> new file mode 100644
> index 0000000000..19439c553d
> --- /dev/null
> +++ b/sysdeps/x86_64/multiarch/rtld-strcpy.S
> @@ -0,0 +1,18 @@
> +/* Copyright (C) 2022 Free Software Foundation, Inc.
> +   This file is part of the GNU C Library.
> +
> +   The GNU C Library is free software; you can redistribute it and/or
> +   modify it under the terms of the GNU Lesser General Public
> +   License as published by the Free Software Foundation; either
> +   version 2.1 of the License, or (at your option) any later version.
> +
> +   The GNU C Library is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> +   Lesser General Public License for more details.
> +
> +   You should have received a copy of the GNU Lesser General Public
> +   License along with the GNU C Library; if not, see
> +   <https://www.gnu.org/licenses/>.  */
> +
> +#include "../strcpy.S"

OK. Uses Makeconfig:sysd-rules-patterns to capture rtld-*.

> -- 
> 2.34.1
> 

Re: [PATCH 6/6] x86: Fix -Os build (BZ #29576)

[H.J. Lu]

On Wed, Oct 5, 2022 at 7:10 AM Carlos O'Donell <carlos@redhat.com> wrote:
>
> On Wed, Sep 21, 2022 at 10:51:08AM -0300, Adhemerval Zanella via Libc-alpha wrote:
> > The compiler might transform __stpcpy calls (which are routed to
> > __builtin_stpcpy as an optimization) to strcpy and x86_64 strcpy
> > multiarch implementation does not build any working symbol due
> > ISA_SHOULD_BUILD not being evaluated for IS_IN(rtld).
>
> Ohhhhhh... that is interesting. This changes the strcpy used in rtld for
> all x86_64 build options, and I'm going to ACK this, but we may need to
> revisit this if it shows up in a profile.

Will this lead to both strcpy and stpcpy in ld.so? Currently there is only
stpcpy in ld.so.

> CC'ing HJ here in case he wants to comment as a machine maintainer.
>
> LGTM.
>
> No regressions on x86_64.
>
> Reviewed-by: Carlos O'Donell <carlos@redhat.com>
> Tested-by: Carlos O'Donell <carlos@redhat.com>
>
> > Checked on x86_64-linux-gnu.
> > ---
> >  sysdeps/x86_64/multiarch/rtld-strcpy.S | 18 ++++++++++++++++++
> >  1 file changed, 18 insertions(+)
> >  create mode 100644 sysdeps/x86_64/multiarch/rtld-strcpy.S
> >
> > diff --git a/sysdeps/x86_64/multiarch/rtld-strcpy.S b/sysdeps/x86_64/multiarch/rtld-strcpy.S
> > new file mode 100644
> > index 0000000000..19439c553d
> > --- /dev/null
> > +++ b/sysdeps/x86_64/multiarch/rtld-strcpy.S
> > @@ -0,0 +1,18 @@
> > +/* Copyright (C) 2022 Free Software Foundation, Inc.
> > +   This file is part of the GNU C Library.
> > +
> > +   The GNU C Library is free software; you can redistribute it and/or
> > +   modify it under the terms of the GNU Lesser General Public
> > +   License as published by the Free Software Foundation; either
> > +   version 2.1 of the License, or (at your option) any later version.
> > +
> > +   The GNU C Library is distributed in the hope that it will be useful,
> > +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> > +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> > +   Lesser General Public License for more details.
> > +
> > +   You should have received a copy of the GNU Lesser General Public
> > +   License along with the GNU C Library; if not, see
> > +   <https://www.gnu.org/licenses/>.  */
> > +
> > +#include "../strcpy.S"
>
> OK. Uses Makeconfig:sysd-rules-patterns to capture rtld-*.
>
> > --
> > 2.34.1
> >
>


-- 
H.J.

Re: [PATCH 6/6] x86: Fix -Os build (BZ #29576)

[Adhemerval Zanella Netto]

On 05/10/22 14:21, H.J. Lu wrote:
> On Wed, Oct 5, 2022 at 7:10 AM Carlos O'Donell <carlos@redhat.com> wrote:
>>
>> On Wed, Sep 21, 2022 at 10:51:08AM -0300, Adhemerval Zanella via Libc-alpha wrote:
>>> The compiler might transform __stpcpy calls (which are routed to
>>> __builtin_stpcpy as an optimization) to strcpy and x86_64 strcpy
>>> multiarch implementation does not build any working symbol due
>>> ISA_SHOULD_BUILD not being evaluated for IS_IN(rtld).
>>
>> Ohhhhhh... that is interesting. This changes the strcpy used in rtld for
>> all x86_64 build options, and I'm going to ACK this, but we may need to
>> revisit this if it shows up in a profile.
> 
> Will this lead to both strcpy and stpcpy in ld.so? Currently there is only
> stpcpy in ld.so.

I think that is expected behavior if compiler creates a reference for a
supported string function in the loader (rtld build pulls the
implementation).

> 
>> CC'ing HJ here in case he wants to comment as a machine maintainer.
>>
>> LGTM.
>>
>> No regressions on x86_64.
>>
>> Reviewed-by: Carlos O'Donell <carlos@redhat.com>
>> Tested-by: Carlos O'Donell <carlos@redhat.com>
>>
>>> Checked on x86_64-linux-gnu.
>>> ---
>>>  sysdeps/x86_64/multiarch/rtld-strcpy.S | 18 ++++++++++++++++++
>>>  1 file changed, 18 insertions(+)
>>>  create mode 100644 sysdeps/x86_64/multiarch/rtld-strcpy.S
>>>
>>> diff --git a/sysdeps/x86_64/multiarch/rtld-strcpy.S b/sysdeps/x86_64/multiarch/rtld-strcpy.S
>>> new file mode 100644
>>> index 0000000000..19439c553d
>>> --- /dev/null
>>> +++ b/sysdeps/x86_64/multiarch/rtld-strcpy.S
>>> @@ -0,0 +1,18 @@
>>> +/* Copyright (C) 2022 Free Software Foundation, Inc.
>>> +   This file is part of the GNU C Library.
>>> +
>>> +   The GNU C Library is free software; you can redistribute it and/or
>>> +   modify it under the terms of the GNU Lesser General Public
>>> +   License as published by the Free Software Foundation; either
>>> +   version 2.1 of the License, or (at your option) any later version.
>>> +
>>> +   The GNU C Library is distributed in the hope that it will be useful,
>>> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
>>> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
>>> +   Lesser General Public License for more details.
>>> +
>>> +   You should have received a copy of the GNU Lesser General Public
>>> +   License along with the GNU C Library; if not, see
>>> +   <https://www.gnu.org/licenses/>.  */
>>> +
>>> +#include "../strcpy.S"
>>
>> OK. Uses Makeconfig:sysd-rules-patterns to capture rtld-*.
>>
>>> --
>>> 2.34.1
>>>
>>
> 
> 

Re: [PATCH 6/6] x86: Fix -Os build (BZ #29576)

[H.J. Lu]

On Wed, Oct 5, 2022 at 10:38 AM Adhemerval Zanella Netto
<adhemerval.zanella@linaro.org> wrote:
>
>
>
> On 05/10/22 14:21, H.J. Lu wrote:
> > On Wed, Oct 5, 2022 at 7:10 AM Carlos O'Donell <carlos@redhat.com> wrote:
> >>
> >> On Wed, Sep 21, 2022 at 10:51:08AM -0300, Adhemerval Zanella via Libc-alpha wrote:
> >>> The compiler might transform __stpcpy calls (which are routed to
> >>> __builtin_stpcpy as an optimization) to strcpy and x86_64 strcpy
> >>> multiarch implementation does not build any working symbol due
> >>> ISA_SHOULD_BUILD not being evaluated for IS_IN(rtld).
> >>
> >> Ohhhhhh... that is interesting. This changes the strcpy used in rtld for
> >> all x86_64 build options, and I'm going to ACK this, but we may need to
> >> revisit this if it shows up in a profile.
> >
> > Will this lead to both strcpy and stpcpy in ld.so? Currently there is only
> > stpcpy in ld.so.
>
> I think that is expected behavior if compiler creates a reference for a
> supported string function in the loader (rtld build pulls the
> implementation).

strcpy is only generated in dl-profile.os:

   text    data     bss     dec     hex filename
   2716       0      72    2788     ae4 dl-profile.os (-O2)
   2265       0      72    2337     921 dl-profile.os (-Os)
   1840       0       0    1840     730  strcpy.os

Should we compile dl-profile.c with -O2 with

CFLAGS-dl-profile.c += $(if $(findstring -Os,$(+cflags)), -O2)

It will create a smaller ld.so.

> >
> >> CC'ing HJ here in case he wants to comment as a machine maintainer.
> >>
> >> LGTM.
> >>
> >> No regressions on x86_64.
> >>
> >> Reviewed-by: Carlos O'Donell <carlos@redhat.com>
> >> Tested-by: Carlos O'Donell <carlos@redhat.com>
> >>
> >>> Checked on x86_64-linux-gnu.
> >>> ---
> >>>  sysdeps/x86_64/multiarch/rtld-strcpy.S | 18 ++++++++++++++++++
> >>>  1 file changed, 18 insertions(+)
> >>>  create mode 100644 sysdeps/x86_64/multiarch/rtld-strcpy.S
> >>>
> >>> diff --git a/sysdeps/x86_64/multiarch/rtld-strcpy.S b/sysdeps/x86_64/multiarch/rtld-strcpy.S
> >>> new file mode 100644
> >>> index 0000000000..19439c553d
> >>> --- /dev/null
> >>> +++ b/sysdeps/x86_64/multiarch/rtld-strcpy.S
> >>> @@ -0,0 +1,18 @@
> >>> +/* Copyright (C) 2022 Free Software Foundation, Inc.
> >>> +   This file is part of the GNU C Library.
> >>> +
> >>> +   The GNU C Library is free software; you can redistribute it and/or
> >>> +   modify it under the terms of the GNU Lesser General Public
> >>> +   License as published by the Free Software Foundation; either
> >>> +   version 2.1 of the License, or (at your option) any later version.
> >>> +
> >>> +   The GNU C Library is distributed in the hope that it will be useful,
> >>> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> >>> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> >>> +   Lesser General Public License for more details.
> >>> +
> >>> +   You should have received a copy of the GNU Lesser General Public
> >>> +   License along with the GNU C Library; if not, see
> >>> +   <https://www.gnu.org/licenses/>.  */
> >>> +
> >>> +#include "../strcpy.S"
> >>
> >> OK. Uses Makeconfig:sysd-rules-patterns to capture rtld-*.
> >>
> >>> --
> >>> 2.34.1
> >>>
> >>
> >
> >



-- 
H.J.

Re: [PATCH 2/6] posix: Suppress -Os warnings on fnmatch

[Paul Eggert]

On 10/5/22 06:47, Carlos O'Donell via Libc-alpha wrote:
> This touches gnulib code and may require merging upstream or harmonizing
> in a different way.

Shouldn't require any extra work as far as glibc goes, as Gnulib 
generally disables those diagnostics at the GCC command line level, 
because c'mon, they're more trouble than their worth.

We'll put in no-op macro definitions somewhere else in Gnulib, if 
needed. The files touched by this patch shouldn't need any further change.

Re: [PATCH 6/6] x86: Fix -Os build (BZ #29576)

[Adhemerval Zanella Netto]

On 05/10/22 15:06, H.J. Lu wrote:
> On Wed, Oct 5, 2022 at 10:38 AM Adhemerval Zanella Netto
> <adhemerval.zanella@linaro.org> wrote:
>>
>>
>>
>> On 05/10/22 14:21, H.J. Lu wrote:
>>> On Wed, Oct 5, 2022 at 7:10 AM Carlos O'Donell <carlos@redhat.com> wrote:
>>>>
>>>> On Wed, Sep 21, 2022 at 10:51:08AM -0300, Adhemerval Zanella via Libc-alpha wrote:
>>>>> The compiler might transform __stpcpy calls (which are routed to
>>>>> __builtin_stpcpy as an optimization) to strcpy and x86_64 strcpy
>>>>> multiarch implementation does not build any working symbol due
>>>>> ISA_SHOULD_BUILD not being evaluated for IS_IN(rtld).
>>>>
>>>> Ohhhhhh... that is interesting. This changes the strcpy used in rtld for
>>>> all x86_64 build options, and I'm going to ACK this, but we may need to
>>>> revisit this if it shows up in a profile.
>>>
>>> Will this lead to both strcpy and stpcpy in ld.so? Currently there is only
>>> stpcpy in ld.so.
>>
>> I think that is expected behavior if compiler creates a reference for a
>> supported string function in the loader (rtld build pulls the
>> implementation).
> 
> strcpy is only generated in dl-profile.os:
> 
>    text    data     bss     dec     hex filename
>    2716       0      72    2788     ae4 dl-profile.os (-O2)
>    2265       0      72    2337     921 dl-profile.os (-Os)
>    1840       0       0    1840     730  strcpy.os
> 
> Should we compile dl-profile.c with -O2 with
> 
> CFLAGS-dl-profile.c += $(if $(findstring -Os,$(+cflags)), -O2)
> 
> It will create a smaller ld.so.

It might be an option to a subsequent patch, but I would avoid trying to outsmart
the compiler here since it might generate a smaller in other version and it would
most likely be arch-specific (adding even more building complexity).

Re: [PATCH 6/6] x86: Fix -Os build (BZ #29576)

[H.J. Lu]

On Wed, Oct 5, 2022 at 1:43 PM Adhemerval Zanella Netto
<adhemerval.zanella@linaro.org> wrote:
>
>
>
> On 05/10/22 15:06, H.J. Lu wrote:
> > On Wed, Oct 5, 2022 at 10:38 AM Adhemerval Zanella Netto
> > <adhemerval.zanella@linaro.org> wrote:
> >>
> >>
> >>
> >> On 05/10/22 14:21, H.J. Lu wrote:
> >>> On Wed, Oct 5, 2022 at 7:10 AM Carlos O'Donell <carlos@redhat.com> wrote:
> >>>>
> >>>> On Wed, Sep 21, 2022 at 10:51:08AM -0300, Adhemerval Zanella via Libc-alpha wrote:
> >>>>> The compiler might transform __stpcpy calls (which are routed to
> >>>>> __builtin_stpcpy as an optimization) to strcpy and x86_64 strcpy
> >>>>> multiarch implementation does not build any working symbol due
> >>>>> ISA_SHOULD_BUILD not being evaluated for IS_IN(rtld).
> >>>>
> >>>> Ohhhhhh... that is interesting. This changes the strcpy used in rtld for
> >>>> all x86_64 build options, and I'm going to ACK this, but we may need to
> >>>> revisit this if it shows up in a profile.
> >>>
> >>> Will this lead to both strcpy and stpcpy in ld.so? Currently there is only
> >>> stpcpy in ld.so.
> >>
> >> I think that is expected behavior if compiler creates a reference for a
> >> supported string function in the loader (rtld build pulls the
> >> implementation).
> >
> > strcpy is only generated in dl-profile.os:
> >
> >    text    data     bss     dec     hex filename
> >    2716       0      72    2788     ae4 dl-profile.os (-O2)
> >    2265       0      72    2337     921 dl-profile.os (-Os)
> >    1840       0       0    1840     730  strcpy.os
> >
> > Should we compile dl-profile.c with -O2 with
> >
> > CFLAGS-dl-profile.c += $(if $(findstring -Os,$(+cflags)), -O2)
> >
> > It will create a smaller ld.so.
>
> It might be an option to a subsequent patch, but I would avoid trying to outsmart
> the compiler here since it might generate a smaller in other version and it would
> most likely be arch-specific (adding even more building complexity).
>

Fair enough.

LGTM.

Thanks.

-- 
H.J.