[PATCH] Remove __warn_memset_zero_len [bz #25399]

[PATCH] Remove __warn_memset_zero_len [bz #25399]

[Siddhesh Poyarekar]

Non-gcc compilers (clang and possibly other compilers that do not
masquerade as gcc 5.0 or later) are unable to use
__warn_memset_zero_len since the symbol is no longer available on
glibc built with gcc 5.0 or later.  While it was likely an oversight
that caused this omission, the fact that it wasn't noticed until
recently (when clang closed the gap on _FORTIFY_SUPPORT) that the
symbol was missing.

Given that both gcc and clang are capable of doing this check in the
compiler, drop all signs of __warn_memset_zero_len from glibc.

The __warndecl macro remains in there unused in the event that we need
it to add a similar check in future.
---
 string/bits/string_fortified.h | 15 ---------------
 1 file changed, 15 deletions(-)

diff --git a/string/bits/string_fortified.h b/string/bits/string_fortified.h
index 309d0f39b2..c8d3051af8 100644
--- a/string/bits/string_fortified.h
+++ b/string/bits/string_fortified.h
@@ -22,11 +22,6 @@
 # error "Never use <bits/string_fortified.h> directly; include <string.h> instead."
 #endif
 
-#if !__GNUC_PREREQ (5,0)
-__warndecl (__warn_memset_zero_len,
-	    "memset used with constant zero length parameter; this could be due to transposed parameters");
-#endif
-
 __fortify_function void *
 __NTH (memcpy (void *__restrict __dest, const void *__restrict __src,
 	       size_t __len))
@@ -58,16 +53,6 @@ __NTH (mempcpy (void *__restrict __dest, const void *__restrict __src,
 __fortify_function void *
 __NTH (memset (void *__dest, int __ch, size_t __len))
 {
-  /* GCC-5.0 and newer implements these checks in the compiler, so we don't
-     need them here.  */
-#if !__GNUC_PREREQ (5,0)
-  if (__builtin_constant_p (__len) && __len == 0
-      && (!__builtin_constant_p (__ch) || __ch != 0))
-    {
-      __warn_memset_zero_len ();
-      return __dest;
-    }
-#endif
   return __builtin___memset_chk (__dest, __ch, __len, __bos0 (__dest));
 }
 
-- 
2.26.2

Re: [PATCH] Remove __warn_memset_zero_len [bz #25399]

[Florian Weimer]

* Siddhesh Poyarekar:

> Non-gcc compilers (clang and possibly other compilers that do not
> masquerade as gcc 5.0 or later) are unable to use
> __warn_memset_zero_len since the symbol is no longer available on
> glibc built with gcc 5.0 or later.  While it was likely an oversight
> that caused this omission, the fact that it wasn't noticed until
> recently (when clang closed the gap on _FORTIFY_SUPPORT) that the
> symbol was missing.
>
> Given that both gcc and clang are capable of doing this check in the
> compiler, drop all signs of __warn_memset_zero_len from glibc.
>
> The __warndecl macro remains in there unused in the event that we need
> it to add a similar check in future.

Nit: We tend to write: [BZ #25399]
(But the Git update hook does not react to the commit subject these days.)

Patch looks good.  Please remove the last paragraph of the commit
message and send a separate patch to remove __warndecl. 8-)

Thanks,
FLorian
-- 
Red Hat GmbH, https://de.redhat.com/ , Registered seat: Grasbrunn,
Commercial register: Amtsgericht Muenchen, HRB 153243,
Managing Directors: Charles Cachera, Brian Klemm, Laurie Krebs, Michael O'Neill

Re: [PATCH] Remove __warn_memset_zero_len [bz #25399]

[Jakub Jelinek]

On Wed, Nov 04, 2020 at 10:19:25AM +0100, Florian Weimer wrote:
> * Siddhesh Poyarekar:
> 
> > Non-gcc compilers (clang and possibly other compilers that do not
> > masquerade as gcc 5.0 or later) are unable to use
> > __warn_memset_zero_len since the symbol is no longer available on
> > glibc built with gcc 5.0 or later.  While it was likely an oversight
> > that caused this omission, the fact that it wasn't noticed until
> > recently (when clang closed the gap on _FORTIFY_SUPPORT) that the
> > symbol was missing.
> >
> > Given that both gcc and clang are capable of doing this check in the
> > compiler, drop all signs of __warn_memset_zero_len from glibc.
> >
> > The __warndecl macro remains in there unused in the event that we need
> > it to add a similar check in future.
> 
> Nit: We tend to write: [BZ #25399]
> (But the Git update hook does not react to the commit subject these days.)
> 
> Patch looks good.  Please remove the last paragraph of the commit
> message and send a separate patch to remove __warndecl. 8-)

I think it can't be really removed.
It is ok if it doesn't warn anymore, and it is ok if it is not in the
headers anymore, but one can still have object files or *.a libraries compiled with
gcc 4.9 and earlier against older glibc and that will now fail to link
altogether.

	Jakub

Re: [PATCH] Remove __warn_memset_zero_len [bz #25399]

[Florian Weimer]

* Jakub Jelinek:

> It is ok if it doesn't warn anymore, and it is ok if it is not in the
> headers anymore, but one can still have object files or *.a libraries
> compiled with gcc 4.9 and earlier against older glibc and that will
> now fail to link altogether.

In general, that is not supported.  We have recently made an exception
for __xstat.  But in this case, linking after removal will only fail if
there was an annoying linker warning before, so I think it's going to be
very rare that this makes a difference.

Thanks,
Florian
-- 
Red Hat GmbH, https://de.redhat.com/ , Registered seat: Grasbrunn,
Commercial register: Amtsgericht Muenchen, HRB 153243,
Managing Directors: Charles Cachera, Brian Klemm, Laurie Krebs, Michael O'Neill

Re: [PATCH] Remove __warn_memset_zero_len [bz #25399]

[Siddhesh Poyarekar]

On 11/4/20 3:10 PM, Florian Weimer via Libc-alpha wrote:
> * Jakub Jelinek:
> 
>> It is ok if it doesn't warn anymore, and it is ok if it is not in the
>> headers anymore, but one can still have object files or *.a libraries
>> compiled with gcc 4.9 and earlier against older glibc and that will
>> now fail to link altogether.
> 
> In general, that is not supported.  We have recently made an exception
> for __xstat.  But in this case, linking after removal will only fail if
> there was an annoying linker warning before, so I think it's going to be
> very rare that this makes a difference.

I suppose another way to look at this is that the patch does not fix the 
link failure, it only ensures that the symbol doesn't appear in the 
future by accident.

We can then look at fixing the link failure by adding the symbol in if 
someone actually cares enough to file another bug.  Does that sound 
reasonable?

Siddhesh

Re: [PATCH] Remove __warn_memset_zero_len [bz #25399]

[Florian Weimer]

* Siddhesh Poyarekar:

> On 11/4/20 3:10 PM, Florian Weimer via Libc-alpha wrote:
>> * Jakub Jelinek:
>> 
>>> It is ok if it doesn't warn anymore, and it is ok if it is not in the
>>> headers anymore, but one can still have object files or *.a libraries
>>> compiled with gcc 4.9 and earlier against older glibc and that will
>>> now fail to link altogether.
>> In general, that is not supported.  We have recently made an
>> exception
>> for __xstat.  But in this case, linking after removal will only fail if
>> there was an annoying linker warning before, so I think it's going to be
>> very rare that this makes a difference.
>
> I suppose another way to look at this is that the patch does not fix
> the link failure, it only ensures that the symbol doesn't appear in
> the future by accident.
>
> We can then look at fixing the link failure by adding the symbol in if
> someone actually cares enough to file another bug.  Does that sound 
> reasonable?

Eh, right.  The __warn_memset_zero_len symbol is currently not part of
libc_nonshared.a.  (Our downstream build of 2.17 still has it, but the
one based on 2.28 does not, due to a different GCC version being used.)
So there is no new backwards compatibility impact from remove the
__warndecl stuff.

Thanks,
Florian
-- 
Red Hat GmbH, https://de.redhat.com/ , Registered seat: Grasbrunn,
Commercial register: Amtsgericht Muenchen, HRB 153243,
Managing Directors: Charles Cachera, Brian Klemm, Laurie Krebs, Michael O'Neill

Re: [PATCH] Remove __warn_memset_zero_len [bz #25399]

[Siddhesh Poyarekar]

On 11/4/20 3:26 PM, Florian Weimer via Libc-alpha wrote:
> Eh, right.  The __warn_memset_zero_len symbol is currently not part of
> libc_nonshared.a.  (Our downstream build of 2.17 still has it, but the
> one based on 2.28 does not, due to a different GCC version being used.)
> So there is no new backwards compatibility impact from remove the
> __warndecl stuff.

I have pushed this now.

Siddhesh