[PATCH] add attribute none to pthread_setspecific (BZ #27714)

[PATCH] add attribute none to pthread_setspecific (BZ #27714)

[Martin Sebor via Libc-alpha]

[-- Attachment #1: Type: text/plain, Size: 750 bytes --]

GCC 11 warns when a pointer to an uninitialized object is passed
to a function that takes a const-qualified argument.  This is done
on the assumption that most such functions read from the object.
For the rare case of a function that doesn't, GCC 11 extended
attribute access to add a new mode called none.

POSIX pthread_setspecific() is one such rare function that takes
a const void* argument but that doesn't read from the object it
points to.  To suppress the -Wmaybe-uninitialized issued by GCC
11 when the address of an uninitialized object is passed to it
(e.g., the result of malloc()), the attached patch #defines
__attr_access_none in cdefs.h and uses the macro on the function
in sysdeps/htl/pthread.h and sysdeps/nptl/pthread.h.

Martin

[-- Attachment #2: glibc-bz27714.diff --]
[-- Type: text/x-patch, Size: 3453 bytes --]

diff --git a/misc/sys/cdefs.h b/misc/sys/cdefs.h
index 8e244a77cf..ac56be4d87 100644
--- a/misc/sys/cdefs.h
+++ b/misc/sys/cdefs.h
@@ -592,8 +592,14 @@ _Static_assert (0, "IEEE 128-bits long double requires redirection on this platf
    array according to access mode, or at least one element when
    size-index is not provided:
      access (access-mode, <ref-index> [, <size-index>])  */
-#define __attr_access(x) __attribute__ ((__access__ x))
+#  define __attr_access(x) __attribute__ ((__access__ x))
+#  if __GNUC_PREREQ (11, 0)
+#    define __attr_access_none(pos) __attribute__ ((__access__ (__none__, pos)))
+#  endif
 #else
 #  define __attr_access(x)
+#  define __attr_access_none(pos)
+#endif
+
 
 /* Specify that a function such as setjmp or vfork may return
diff --git a/nptl/tst-thread-setspecific.c b/nptl/tst-thread-setspecific.c
new file mode 100644
index 0000000000..bda61c6333
--- /dev/null
+++ b/nptl/tst-thread-setspecific.c
@@ -0,0 +1,43 @@
+/* Test to verify that passing a pointer to an uninitialized object
+   to pthread_setspecific doesn't trigger bogus uninitialized warnings.
+   Copyright (C) 2021 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 <pthread.h>
+#include <stdlib.h>
+
+/* Turn uninitialized warnings into errors to detect the problem.
+   See BZ #27714.  */
+
+#pragma GCC diagnostic push
+#pragma GCC diagnostic error "-Wmaybe-uninitialized"
+#pragma GCC diagnostic error "-Wuninitialized"
+
+int do_test (void)
+{
+  void *p = malloc (1);   /* Deliberately uninitialized.  */
+  pthread_setspecific (pthread_self (), p);
+
+  void *q = pthread_getspecific (pthread_self ());
+
+  return p == q;
+}
+
+#pragma GCC diagnostic pop
+
+#define TEST_FUNCTION do_test ()
+#include "../test-skeleton.c"
diff --git a/sysdeps/htl/pthread.h b/sysdeps/htl/pthread.h
index 0923ad0002..6bcf97d692 100644
--- a/sysdeps/htl/pthread.h
+++ b/sysdeps/htl/pthread.h
@@ -822,7 +822,7 @@ extern void *pthread_getspecific (pthread_key_t __key) __THROW;
 
 /* Set the caller thread's thread specific value of KEY to VALUE.  */
 extern int pthread_setspecific (pthread_key_t __key, const void *__value)
-	__THROW;
+	__THROW __attr_access_none (2);
 \f
 
 /* Dynamic package initialization.  */
diff --git a/sysdeps/nptl/pthread.h b/sysdeps/nptl/pthread.h
index 23bcd51d91..7c14d0fef7 100644
--- a/sysdeps/nptl/pthread.h
+++ b/sysdeps/nptl/pthread.h
@@ -1171,7 +1171,8 @@ extern void *pthread_getspecific (pthread_key_t __key) __THROW;
 
 /* Store POINTER in the thread-specific data slot identified by KEY. */
 extern int pthread_setspecific (pthread_key_t __key,
-				const void *__pointer) __THROW ;
+				const void *__pointer)
+  __THROW __attr_access_none (2);
 
 
 #ifdef __USE_XOPEN2K

Re: [PATCH] add attribute none to pthread_setspecific (BZ #27714)

[Martin Sebor via Libc-alpha]

On 4/22/21 3:30 PM, Martin Sebor wrote:
> GCC 11 warns when a pointer to an uninitialized object is passed
> to a function that takes a const-qualified argument.  This is done
> on the assumption that most such functions read from the object.
> For the rare case of a function that doesn't, GCC 11 extended
> attribute access to add a new mode called none.
> 
> POSIX pthread_setspecific() is one such rare function that takes
> a const void* argument but that doesn't read from the object it
> points to.  To suppress the -Wmaybe-uninitialized issued by GCC
> 11 when the address of an uninitialized object is passed to it
> (e.g., the result of malloc()), the attached patch #defines
> __attr_access_none in cdefs.h and uses the macro on the function
> in sysdeps/htl/pthread.h and sysdeps/nptl/pthread.h.
> 
> Martin

The patch is missing the nptl/Makefile change below to add the test:

diff --git a/nptl/Makefile b/nptl/Makefile
index 294bb2faa4..294591bddf 100644
--- a/nptl/Makefile
+++ b/nptl/Makefile
@@ -316,6 +316,7 @@ tests = tst-attr2 tst-attr3 tst-default-attr \
         tst-pthread-gdb-attach tst-pthread-gdb-attach-static \
         tst-pthread_exit-nothreads \
         tst-pthread_exit-nothreads-static \
+       tst-thread-setspecific

  tests-nolibpthread = \
    tst-pthread_exit-nothreads \

Re: [PATCH] add attribute none to pthread_setspecific (BZ #27714)

[Paul Eggert]

[-- Attachment #1: Type: text/plain, Size: 590 bytes --]

On 4/22/21 2:30 PM, Martin Sebor via Libc-alpha wrote:

> -	__THROW;
> +	__THROW __attr_access_none (2);

Instead of inventing a new __attr_access_none macro that developers will 
need to remember, why not add support to the existing __attr_access 
macro? That is, uses can look like this:

      __THROW __attr_access ((__none__, 2));

if we define __attr_access with something like the attached patch.

Alternatively, one could keep both cdefs.h and the callers simple by 
doing access attribute checking only for GCC 11 and later. That'd be 
good enough in the long run.

[-- Attachment #2: cdefs.diff --]
[-- Type: text/x-patch, Size: 1437 bytes --]

diff --git a/misc/sys/cdefs.h b/misc/sys/cdefs.h
index 8e244a77cf..db3283ec7f 100644
--- a/misc/sys/cdefs.h
+++ b/misc/sys/cdefs.h
@@ -586,15 +586,26 @@ _Static_assert (0, "IEEE 128-bits long double requires redirection on this platf
 # define __HAVE_GENERIC_SELECTION 0
 #endif
 
-#if __GNUC_PREREQ (10, 0)
 /* Designates a 1-based positional argument ref-index of pointer type
    that can be used to access size-index elements of the pointed-to
    array according to access mode, or at least one element when
    size-index is not provided:
      access (access-mode, <ref-index> [, <size-index>])  */
-#define __attr_access(x) __attribute__ ((__access__ x))
+#if __GNUC_PREREQ (11, 0)
+# define __attr_access(x) __attribute__ ((__access__ x))
+#elif __GNUC_PREREQ (10, 0)
+# define __attr_access(x) __attr_access1 x
+# define __attr_access1(access_mode, ...) \
+    __attr_access##access_mode (__VA_ARGS__)
+# define __attr_access__none__(...)
+# define __attr_access__read_only__(...) \
+    __attribute__ ((__access__ (__read_only__, __VA_ARGS__)
+# define __attr_access__read_write__(...) \
+    __attribute__ ((__access__ (__read_write__, __VA_ARGS__)
+# define __attr_access__write_only__(...) \
+    __attribute__ ((__access__ (__write_only__, __VA_ARGS__)
 #else
-#  define __attr_access(x)
+# define __attr_access(x)
 #endif
 
 /* Specify that a function such as setjmp or vfork may return

Re: [PATCH] add attribute none to pthread_setspecific (BZ #27714)

[Martin Sebor via Libc-alpha]

On 4/22/21 6:11 PM, Paul Eggert wrote:
> On 4/22/21 2:30 PM, Martin Sebor via Libc-alpha wrote:
> 
>> -    __THROW;
>> +    __THROW __attr_access_none (2);
> 
> Instead of inventing a new __attr_access_none macro that developers will 
> need to remember, why not add support to the existing __attr_access 
> macro? That is, uses can look like this:
> 
>       __THROW __attr_access ((__none__, 2));
> 
> if we define __attr_access with something like the attached patch.

I don't have a preference for how to define the macro.  I went with
a new one because Joseph suggested that approach in hist comments
on the mismatched allocation patch (for the __attr_dealloc macros).
The other approach I considered was using the __attr_access macro
but guarding it for GCC 11 in situ.  Since functions like
pthread_setspecific are exceeingly rare, I don't expect the new
attribute to be used very much at all (if you do know of other
such functions, though, please let me know so I can annotate them
as well).

As for your suggested change. I think we considered variadic macros
when we first introduced the attribute but rejected it for some
reason that I'm not sure I remember.  Maybe because they're a C99
feature and Glibc supports older compilers?

If this isn't the case and others share your preference for this
approach I don't mind changing it.

> 
> Alternatively, one could keep both cdefs.h and the callers simple by 
> doing access attribute checking only for GCC 11 and later. That'd be 
> good enough in the long run.

I'd view disabling the checking with older GCC releases as a QoI
regression, so I'm not in favor of this alternative.

Martin

Re: [PATCH] add attribute none to pthread_setspecific (BZ #27714)

[Paul Eggert]

[-- Attachment #1: Type: text/plain, Size: 714 bytes --]

On 4/23/21 8:24 AM, Martin Sebor wrote:
> 
> I think we considered variadic macros
> when we first introduced the attribute but rejected it for some
> reason that I'm not sure I remember.  Maybe because they're a C99
> feature and Glibc supports older compilers?

That shouldn't be an issue here, since the suggested change uses 
variadic macros for GCC 10 only.

Unless people were worried about running something like 'gcc -ansi' or 
'gcc -std=c89? To head that off at the pass, we can do the GCC 10 stuff 
only if !__STRICT_ANSI__. Also, while we're at it we should be 
C99-compatible in the variadic part (i.e., at least one named argument). 
Something like the attached (untested) patch, say.

[-- Attachment #2: cdefs-2.diff --]
[-- Type: text/x-patch, Size: 1368 bytes --]

diff --git a/misc/sys/cdefs.h b/misc/sys/cdefs.h
index 8e244a77cf..b3b5456efd 100644
--- a/misc/sys/cdefs.h
+++ b/misc/sys/cdefs.h
@@ -586,15 +586,23 @@ _Static_assert (0, "IEEE 128-bits long double requires redirection on this platf
 # define __HAVE_GENERIC_SELECTION 0
 #endif
 
-#if __GNUC_PREREQ (10, 0)
 /* Designates a 1-based positional argument ref-index of pointer type
    that can be used to access size-index elements of the pointed-to
    array according to access mode, or at least one element when
    size-index is not provided:
      access (access-mode, <ref-index> [, <size-index>])  */
-#define __attr_access(x) __attribute__ ((__access__ x))
+#if __GNUC_PREREQ (11, 0)
+# define __attr_access(x) __attribute__ ((__access__ x))
+#elif __GNUC_PREREQ (10, 0) && !defined __STRICT_ANSI__
+# define __attr_access(x) __attr_access1 x
+# define __attr_access1(mode, ...) __attr_access##mode (mode, __VA_ARGS__)
+# define __attr_access__none__(mode, ...)
+# define __attr_access__read_only__(mode, ...) \
+    __attribute__ ((__access__ (mode, __VA_ARGS__)))
+# define __attr_access__read_write__ __attr_access__read_only__
+# define __attr_access__write_only__ __attr_access__read_only__
 #else
-#  define __attr_access(x)
+# define __attr_access(x)
 #endif
 
 /* Specify that a function such as setjmp or vfork may return

Re: [PATCH] add attribute none to pthread_setspecific (BZ #27714)

[Martin Sebor via Libc-alpha]

On 4/23/21 2:19 PM, Paul Eggert wrote:
> On 4/23/21 8:24 AM, Martin Sebor wrote:
>>
>> I think we considered variadic macros
>> when we first introduced the attribute but rejected it for some
>> reason that I'm not sure I remember.  Maybe because they're a C99
>> feature and Glibc supports older compilers?
> 
> That shouldn't be an issue here, since the suggested change uses 
> variadic macros for GCC 10 only.
> 
> Unless people were worried about running something like 'gcc -ansi' or 
> 'gcc -std=c89? To head that off at the pass, we can do the GCC 10 stuff 
> only if !__STRICT_ANSI__. Also, while we're at it we should be 
> C99-compatible in the variadic part (i.e., at least one named argument). 
> Something like the attached (untested) patch, say.

I'm open to your suggestion to use the variadic macros if no one
objects to it.

GCC doesn't let language conformance modes affect unrelated warnings
(like -Wuninitialized) and I am not in favor of introducing such
a distinction in Glibc.  We routinely get bug reports about false
negatives that boil down to the sensitivity of these warnings
(especially -Wuninitialized) to optimization settings.  I wouldn't
want to compound the problem.

Martin

Re: [PATCH] add attribute none to pthread_setspecific (BZ #27714)

[Paul Eggert]

On 4/23/21 2:29 PM, Martin Sebor wrote:
> GCC doesn't let language conformance modes affect unrelated warnings
> (like -Wuninitialized) and I am not in favor of introducing such
> a distinction in Glibc.

If this is talking about __STRICT_ANSI__, misc/sys/cdefs.h uses that 
macro already when defining _Static_assert on older compilers. The use 
is not to avoid unrelated warnings; it's to conform to the relevant 
standard on request.

Re: [PATCH] add attribute none to pthread_setspecific (BZ #27714)

[Martin Sebor via Libc-alpha]

On 4/23/21 6:27 PM, Paul Eggert wrote:
> On 4/23/21 2:29 PM, Martin Sebor wrote:
>> GCC doesn't let language conformance modes affect unrelated warnings
>> (like -Wuninitialized) and I am not in favor of introducing such
>> a distinction in Glibc.
> 
> If this is talking about __STRICT_ANSI__, misc/sys/cdefs.h uses that 
> macro already when defining _Static_assert on older compilers. The use 
> is not to avoid unrelated warnings; it's to conform to the relevant 
> standard on request.

I am saying that that introducing a dependency on __STRICT_ANSI__
as you suggested would have the effect of disabling access warnings
when the -ansi option is set and would be a QoI bug/regression and
so is not in my view suitable.

I've tested your suggested change to the __attr_access macro without
the dependency on __STRICT_ANSI__ in all language conformance modes,
both with and without -ansi, both in C and C++, and with all of GCC
9, 10, and 11.  The only diagnostics I saw involving the new
attribute definition were -Wvariadic-macros with GCC 10 in C++ 98
and C++ 11 modes with -Wpedantic and -Wsystem-headers:

../misc/sys/cdefs.h:598:31: warning: anonymous variadic macros were 
introduced in C++11 [-Wvariadic-macros]
   598 | # define __attr_access1(mode, ...) __attr_access##mode (mode, 
__VA_ARGS__)
       |                               ^~~
../misc/sys/cdefs.h:599:38: warning: anonymous variadic macros were 
introduced in C++11 [-Wvariadic-macros]
   599 | # define __attr_access__none__(mode, ...)
       |                                      ^~~
../misc/sys/cdefs.h:600:43: warning: anonymous variadic macros were 
introduced in C++11 [-Wvariadic-macros]
   600 | # define __attr_access__read_only__(mode, ...) \
       |                                           ^~~

My test includes all the standard C headers that use the attribute
(and a few others).

I don't know what Glibc's policy is regarding -Wsystem-headers but
the uses of variadic macros in other Glibc headers show that they
are guarded with #if !__cplusplus, presumably to avoid the same
warnings.  In light of that, I'm not comfortable introducing
the variadic macro in a fix for a false positive when a simpler
fix is available that doesn't trigger other warnings.

If you have a strong desire to redefine the macro in some way
please do it separately of the fix for the false positive.  That
way, if there's any unwelcome fallout, the change can be reverted
without reintroducing the false positive.

I plan to commit my originally proposed patch this week unless there
are objections.

Martin

Re: [PATCH] add attribute none to pthread_setspecific (BZ #27714)

[Florian Weimer via Libc-alpha]

* Martin Sebor via Libc-alpha:

> diff --git a/misc/sys/cdefs.h b/misc/sys/cdefs.h
> index 8e244a77cf..ac56be4d87 100644
> --- a/misc/sys/cdefs.h
> +++ b/misc/sys/cdefs.h
> @@ -592,8 +592,14 @@ _Static_assert (0, "IEEE 128-bits long double requires redirection on this platf
>     array according to access mode, or at least one element when
>     size-index is not provided:
>       access (access-mode, <ref-index> [, <size-index>])  */
> -#define __attr_access(x) __attribute__ ((__access__ x))
> +#  define __attr_access(x) __attribute__ ((__access__ x))
> +#  if __GNUC_PREREQ (11, 0)
> +#    define __attr_access_none(pos) __attribute__ ((__access__ (__none__, pos)))
> +#  endif
>  #else
>  #  define __attr_access(x)
> +#  define __attr_access_none(pos)
> +#endif

I don't think this works because __attr_access_none is not defined for
GCC 10 due to the way the definitions are nested.  I think you should
move the __GNUC_PREREQ (11, 0) check to the top level.

It might be consistent with fewer attribute access extensions to write
__attr_access_none ((__none__, 2)) instead of __attr_access_none (2).

Thanks,
Florian

Re: [PATCH] add attribute none to pthread_setspecific (BZ #27714)

[Martin Sebor via Libc-alpha]

On 4/26/21 10:41 PM, Florian Weimer wrote:
> * Martin Sebor via Libc-alpha:
> 
>> diff --git a/misc/sys/cdefs.h b/misc/sys/cdefs.h
>> index 8e244a77cf..ac56be4d87 100644
>> --- a/misc/sys/cdefs.h
>> +++ b/misc/sys/cdefs.h
>> @@ -592,8 +592,14 @@ _Static_assert (0, "IEEE 128-bits long double requires redirection on this platf
>>      array according to access mode, or at least one element when
>>      size-index is not provided:
>>        access (access-mode, <ref-index> [, <size-index>])  */
>> -#define __attr_access(x) __attribute__ ((__access__ x))
>> +#  define __attr_access(x) __attribute__ ((__access__ x))
>> +#  if __GNUC_PREREQ (11, 0)
>> +#    define __attr_access_none(pos) __attribute__ ((__access__ (__none__, pos)))
>> +#  endif
>>   #else
>>   #  define __attr_access(x)
>> +#  define __attr_access_none(pos)
>> +#endif
> 
> I don't think this works because __attr_access_none is not defined for
> GCC 10 due to the way the definitions are nested.  I think you should
> move the __GNUC_PREREQ (11, 0) check to the top level.

You're right.  I had inadvertently reverted the pthread.h changes so
my testing didn't expose it.  Too many patches in the same tree...
I've committed a1561c3bbe with the missing definition (and retesting
the macro).

> 
> It might be consistent with fewer attribute access extensions to write
> __attr_access_none ((__none__, 2)) instead of __attr_access_none (2).

Repeating the "none" part doesn't seem in any way useful to me.  It
wouldn't reduce the number of macros, existing or new.

Martin

> 
> Thanks,
> Florian
> 

Re: [PATCH] add attribute none to pthread_setspecific (BZ #27714)

[Joseph Myers]

On Tue, 27 Apr 2021, Martin Sebor via Libc-alpha wrote:

> You're right.  I had inadvertently reverted the pthread.h changes so
> my testing didn't expose it.  Too many patches in the same tree...
> I've committed a1561c3bbe with the missing definition (and retesting
> the macro).

How was that commit tested?  It breaks the glibc testsuite build for me 
with GCC 11 (on x86_64, also seen on lots of other architectures with 
build-many-glibcs.py).

tst-tsd3.c: In function 'tf':
tst-tsd3.c:71:7: error: 'pthread_setspecific' expecting 1 byte in a region of size 0 [-Werror=stringop-overread]
   71 |   if (pthread_setspecific (key1, (void *) 1l) != 0
      |       ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
In file included from ../include/pthread.h:1,
                 from tst-tsd3.c:20:
../sysdeps/nptl/pthread.h:1184:12: note: in a call to function 'pthread_setspecific' declared with attribute 'access (none, 2)'
 1184 | extern int pthread_setspecific (pthread_key_t __key,
      |            ^~~~~~~~~~~~~~~~~~~
tst-tsd3.c:72:10: error: 'pthread_setspecific' expecting 1 byte in a region of size 0 [-Werror=stringop-overread]
   72 |       || pthread_setspecific (key2, (void *) 1l) != 0)
      |          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
In file included from ../include/pthread.h:1,
                 from tst-tsd3.c:20:
../sysdeps/nptl/pthread.h:1184:12: note: in a call to function 'pthread_setspecific' declared with attribute 'access (none, 2)'
 1184 | extern int pthread_setspecific (pthread_key_t __key,
      |            ^~~~~~~~~~~~~~~~~~~
tst-tsd3.c: In function 'destr2':
tst-tsd3.c:56:11: error: 'pthread_setspecific' expecting 1 byte in a region of size 0 [-Werror=stringop-overread]
   56 |       if (pthread_setspecific (key1, (void *) 1l) != 0)
      |           ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
In file included from ../include/pthread.h:1,
                 from tst-tsd3.c:20:
../sysdeps/nptl/pthread.h:1184:12: note: in a call to function 'pthread_setspecific' declared with attribute 'access (none, 2)'
 1184 | extern int pthread_setspecific (pthread_key_t __key,
      |            ^~~~~~~~~~~~~~~~~~~
tst-tsd3.c: In function 'destr1':
tst-tsd3.c:40:11: error: 'pthread_setspecific' expecting 1 byte in a region of size 0 [-Werror=stringop-overread]
   40 |       if (pthread_setspecific (key2, (void *) 1l) != 0)
      |           ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
In file included from ../include/pthread.h:1,
                 from tst-tsd3.c:20:
../sysdeps/nptl/pthread.h:1184:12: note: in a call to function 'pthread_setspecific' declared with attribute 'access (none, 2)'
 1184 | extern int pthread_setspecific (pthread_key_t __key,
      |            ^~~~~~~~~~~~~~~~~~~

(I also see similar errors building tst-tsd4.c.  The testsuite builds 
cleanly with the same compiler and the previous glibc commit.)

-- 
Joseph S. Myers
joseph@codesourcery.com

Re: [PATCH] add attribute none to pthread_setspecific (BZ #27714)

[Martin Sebor via Libc-alpha]

On 4/27/21 3:07 PM, Joseph Myers wrote:
> On Tue, 27 Apr 2021, Martin Sebor via Libc-alpha wrote:
> 
>> You're right.  I had inadvertently reverted the pthread.h changes so
>> my testing didn't expose it.  Too many patches in the same tree...
>> I've committed a1561c3bbe with the missing definition (and retesting
>> the macro).
> 
> How was that commit tested?  It breaks the glibc testsuite build for me
> with GCC 11 (on x86_64, also seen on lots of other architectures with
> build-many-glibcs.py).

These are warnings in my build (I've seen a few others scroll by
and have always assumed they were expected(*)).  Those you pasted
below are intended: the none mode implies that const void* pointer
should point to an object 1 byte in size (that excludes valid,
past-the-end pointers, something I've been meaning to add
an extension for).  The test should change to use a valid pointer.
I test by simply running make check.  I can make the change to
the test if you expect warning-free test builds.

Martin

[*] Here's an example of a warning I just noticed while rerunning
make check:

tst-chk1.c: In function ‘do_test’:
../bits/select.h:37:51: warning: value computed is not used [-Wunused-value]
    37 |   ((__FDS_BITS (s)[__FD_ELT (d)] & __FD_MASK (d)) != 0)
       |   ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~
../misc/sys/select.h:87:33: note: in expansion of macro ‘__FD_ISSET’
    87 | #define FD_ISSET(fd, fdsetp)    __FD_ISSET (fd, fdsetp)
       |                                 ^~~~~~~~~~
tst-chk1.c:1681:3: note: in expansion of macro ‘FD_ISSET’
  1681 |   FD_ISSET (FD_SETSIZE - 1, &s);
       |   ^~~~~~~~

> 
> tst-tsd3.c: In function 'tf':
> tst-tsd3.c:71:7: error: 'pthread_setspecific' expecting 1 byte in a region of size 0 [-Werror=stringop-overread]
>     71 |   if (pthread_setspecific (key1, (void *) 1l) != 0
>        |       ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> In file included from ../include/pthread.h:1,
>                   from tst-tsd3.c:20:
> ../sysdeps/nptl/pthread.h:1184:12: note: in a call to function 'pthread_setspecific' declared with attribute 'access (none, 2)'
>   1184 | extern int pthread_setspecific (pthread_key_t __key,
>        |            ^~~~~~~~~~~~~~~~~~~
> tst-tsd3.c:72:10: error: 'pthread_setspecific' expecting 1 byte in a region of size 0 [-Werror=stringop-overread]
>     72 |       || pthread_setspecific (key2, (void *) 1l) != 0)
>        |          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> In file included from ../include/pthread.h:1,
>                   from tst-tsd3.c:20:
> ../sysdeps/nptl/pthread.h:1184:12: note: in a call to function 'pthread_setspecific' declared with attribute 'access (none, 2)'
>   1184 | extern int pthread_setspecific (pthread_key_t __key,
>        |            ^~~~~~~~~~~~~~~~~~~
> tst-tsd3.c: In function 'destr2':
> tst-tsd3.c:56:11: error: 'pthread_setspecific' expecting 1 byte in a region of size 0 [-Werror=stringop-overread]
>     56 |       if (pthread_setspecific (key1, (void *) 1l) != 0)
>        |           ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> In file included from ../include/pthread.h:1,
>                   from tst-tsd3.c:20:
> ../sysdeps/nptl/pthread.h:1184:12: note: in a call to function 'pthread_setspecific' declared with attribute 'access (none, 2)'
>   1184 | extern int pthread_setspecific (pthread_key_t __key,
>        |            ^~~~~~~~~~~~~~~~~~~
> tst-tsd3.c: In function 'destr1':
> tst-tsd3.c:40:11: error: 'pthread_setspecific' expecting 1 byte in a region of size 0 [-Werror=stringop-overread]
>     40 |       if (pthread_setspecific (key2, (void *) 1l) != 0)
>        |           ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> In file included from ../include/pthread.h:1,
>                   from tst-tsd3.c:20:
> ../sysdeps/nptl/pthread.h:1184:12: note: in a call to function 'pthread_setspecific' declared with attribute 'access (none, 2)'
>   1184 | extern int pthread_setspecific (pthread_key_t __key,
>        |            ^~~~~~~~~~~~~~~~~~~
> 
> (I also see similar errors building tst-tsd4.c.  The testsuite builds
> cleanly with the same compiler and the previous glibc commit.)
> 

Re: [PATCH] add attribute none to pthread_setspecific (BZ #27714)

[Joseph Myers]

On Tue, 27 Apr 2021, Martin Sebor via Libc-alpha wrote:

> These are warnings in my build (I've seen a few others scroll by
> and have always assumed they were expected(*)).  Those you pasted

Most warnings are errors by default (unless you use --disable-werror, 
which should never normally be used in glibc development unless you're 
e.g. reviewing the warnings you get if you add extra warning options to 
those with which glibc is built by default, in order to fix those warnings 
before adding the extra options).

> I test by simply running make check.  I can make the change to
> the test if you expect warning-free test builds.

We expect builds, for all glibc ABIs, free from compile errors or failures 
of tests that can run without needing to execute any code for the glibc 
architecture.  That means no warnings that are turned into errors by 
-Werror.  There are some warnings for which -Wno-error or pragmas are used 
to stop them being errors; all other warnings are disallowed.

> [*] Here's an example of a warning I just noticed while rerunning
> make check:
> 
> tst-chk1.c: In function ‘do_test’:

That's an example covered by -Wno-error.

# We know these tests have problems with format strings, this is what
# we are testing.  Disable that warning.  They are also testing
# deprecated functions (notably gets) so disable that warning as well.
# And they also generate warnings from warning attributes, which
# cannot be disabled via pragmas, so require -Wno-error to be used.
CFLAGS-tst-chk1.c += -Wno-format -Wno-deprecated-declarations -Wno-error

(Any code disabling any warnings or disabling -Werror for them is expected 
to have a comment explaining why it's OK to do so in that case.)

-- 
Joseph S. Myers
joseph@codesourcery.com

Re: [PATCH] add attribute none to pthread_setspecific (BZ #27714)

[Martin Sebor via Libc-alpha]

[-- Attachment #1: Type: text/plain, Size: 2122 bytes --]

On 4/27/21 3:58 PM, Joseph Myers wrote:
> On Tue, 27 Apr 2021, Martin Sebor via Libc-alpha wrote:
> 
>> These are warnings in my build (I've seen a few others scroll by
>> and have always assumed they were expected(*)).  Those you pasted
> 
> Most warnings are errors by default (unless you use --disable-werror,
> which should never normally be used in glibc development unless you're
> e.g. reviewing the warnings you get if you add extra warning options to
> those with which glibc is built by default, in order to fix those warnings
> before adding the extra options).

Ah, that's what my build script does and I keep forgetting to
change that when working on Glibc (as opposed to testing new
GCC warnings).  My bad.  The attached diff patches up the tests
to pass the function a valid argument.  I'll plan to commit it
shortly unless you prefer some other solution.

Martin

> 
>> I test by simply running make check.  I can make the change to
>> the test if you expect warning-free test builds.
> 
> We expect builds, for all glibc ABIs, free from compile errors or failures
> of tests that can run without needing to execute any code for the glibc
> architecture.  That means no warnings that are turned into errors by
> -Werror.  There are some warnings for which -Wno-error or pragmas are used
> to stop them being errors; all other warnings are disallowed.
> 
>> [*] Here's an example of a warning I just noticed while rerunning
>> make check:
>>
>> tst-chk1.c: In function ‘do_test’:
> 
> That's an example covered by -Wno-error.
> 
> # We know these tests have problems with format strings, this is what
> # we are testing.  Disable that warning.  They are also testing
> # deprecated functions (notably gets) so disable that warning as well.
> # And they also generate warnings from warning attributes, which
> # cannot be disabled via pragmas, so require -Wno-error to be used.
> CFLAGS-tst-chk1.c += -Wno-format -Wno-deprecated-declarations -Wno-error
> 
> (Any code disabling any warnings or disabling -Werror for them is expected
> to have a comment explaining why it's OK to do so in that case.)
> 


[-- Attachment #2: glibc-tests-pthread_setspecific.diff --]
[-- Type: text/x-patch, Size: 2305 bytes --]

diff --git a/nptl/tst-tsd3.c b/nptl/tst-tsd3.c
index 0dd39ccb2b..45c7e4e1ea 100644
--- a/nptl/tst-tsd3.c
+++ b/nptl/tst-tsd3.c
@@ -37,7 +37,8 @@ destr1 (void *arg)
     {
       puts ("set key2");
 
-      if (pthread_setspecific (key2, (void *) 1l) != 0)
+      /* Use an arbirary but valid pointer to avoid GCC warnings.  */
+      if (pthread_setspecific (key2, (void *) &left) != 0)
 	{
 	  puts ("destr1: setspecific failed");
 	  exit (1);
@@ -53,7 +54,8 @@ destr2 (void *arg)
     {
       puts ("set key1");
 
-      if (pthread_setspecific (key1, (void *) 1l) != 0)
+      /* Use an arbirary but valid pointer to avoid GCC warnings.  */
+      if (pthread_setspecific (key1, (void *) &left) != 0)
 	{
 	  puts ("destr2: setspecific failed");
 	  exit (1);
@@ -68,8 +70,9 @@ tf (void *arg)
   /* Let the destructors work.  */
   left = 7;
 
-  if (pthread_setspecific (key1, (void *) 1l) != 0
-      || pthread_setspecific (key2, (void *) 1l) != 0)
+  /* Use an arbirary but valid pointer to avoid GCC warnings.  */
+  if (pthread_setspecific (key1, (void *) &left) != 0
+      || pthread_setspecific (key2, (void *) &left) != 0)
     {
       puts ("tf: setspecific failed");
       exit (1);
diff --git a/nptl/tst-tsd4.c b/nptl/tst-tsd4.c
index cc1ada4d4d..f45cf70e37 100644
--- a/nptl/tst-tsd4.c
+++ b/nptl/tst-tsd4.c
@@ -34,7 +34,8 @@ destr (void *arg)
 {
   ++rounds;
 
-  if (pthread_setspecific (key, (void *) 1l) != 0)
+  /* Use an arbirary but valid pointer to avoid GCC warnings.  */
+  if (pthread_setspecific (key, (void *) &rounds) != 0)
     {
       puts ("destr: setspecific failed");
       exit (1);
@@ -45,7 +46,7 @@ destr (void *arg)
 static void *
 tf (void *arg)
 {
-  if (pthread_setspecific (key, (void *) 1l) != 0)
+  if (pthread_setspecific (key, (void *) &rounds) != 0)
     {
       puts ("tf: setspecific failed");
       exit (1);
diff --git a/sysdeps/pthread/tst-key2.c b/sysdeps/pthread/tst-key2.c
index 6828873e41..5662842035 100644
--- a/sysdeps/pthread/tst-key2.c
+++ b/sysdeps/pthread/tst-key2.c
@@ -56,7 +56,7 @@ tf (void *arg)
 {
   pthread_key_t *key = (pthread_key_t *) arg;
 
-  if (pthread_setspecific (*key, (void *) -1l) != 0)
+  if (pthread_setspecific (*key, (void *) arg) != 0)
     {
       write_message ("setspecific failed\n");
       _exit (1);

Re: [PATCH] add attribute none to pthread_setspecific (BZ #27714)

[Martin Sebor via Libc-alpha]

[-- Attachment #1: Type: text/plain, Size: 2423 bytes --]

On 4/27/21 4:57 PM, Martin Sebor wrote:
> On 4/27/21 3:58 PM, Joseph Myers wrote:
>> On Tue, 27 Apr 2021, Martin Sebor via Libc-alpha wrote:
>>
>>> These are warnings in my build (I've seen a few others scroll by
>>> and have always assumed they were expected(*)).  Those you pasted
>>
>> Most warnings are errors by default (unless you use --disable-werror,
>> which should never normally be used in glibc development unless you're
>> e.g. reviewing the warnings you get if you add extra warning options to
>> those with which glibc is built by default, in order to fix those 
>> warnings
>> before adding the extra options).
> 
> Ah, that's what my build script does and I keep forgetting to
> change that when working on Glibc (as opposed to testing new
> GCC warnings).  My bad.  The attached diff patches up the tests
> to pass the function a valid argument.  I'll plan to commit it
> shortly unless you prefer some other solution.

More testing exposed a few more of these pthread_setspecific()
calls with invalid pointers.  I've committed the attached patch
in rb25b067491.  Let me know if you see something else.

> 
> Martin
> 
>>
>>> I test by simply running make check.  I can make the change to
>>> the test if you expect warning-free test builds.
>>
>> We expect builds, for all glibc ABIs, free from compile errors or 
>> failures
>> of tests that can run without needing to execute any code for the glibc
>> architecture.  That means no warnings that are turned into errors by
>> -Werror.  There are some warnings for which -Wno-error or pragmas are 
>> used
>> to stop them being errors; all other warnings are disallowed.
>>
>>> [*] Here's an example of a warning I just noticed while rerunning
>>> make check:
>>>
>>> tst-chk1.c: In function ‘do_test’:
>>
>> That's an example covered by -Wno-error.
>>
>> # We know these tests have problems with format strings, this is what
>> # we are testing.  Disable that warning.  They are also testing
>> # deprecated functions (notably gets) so disable that warning as well.
>> # And they also generate warnings from warning attributes, which
>> # cannot be disabled via pragmas, so require -Wno-error to be used.
>> CFLAGS-tst-chk1.c += -Wno-format -Wno-deprecated-declarations -Wno-error
>>
>> (Any code disabling any warnings or disabling -Werror for them is 
>> expected
>> to have a comment explaining why it's OK to do so in that case.)
>>
> 


[-- Attachment #2: glibc-tests-pthread_setspecific.diff --]
[-- Type: text/x-patch, Size: 5987 bytes --]

diff --git a/nptl/tst-tsd3.c b/nptl/tst-tsd3.c
index 0dd39ccb2b..45c7e4e1ea 100644
--- a/nptl/tst-tsd3.c
+++ b/nptl/tst-tsd3.c
@@ -37,7 +37,8 @@ destr1 (void *arg)
     {
       puts ("set key2");
 
-      if (pthread_setspecific (key2, (void *) 1l) != 0)
+      /* Use an arbirary but valid pointer to avoid GCC warnings.  */
+      if (pthread_setspecific (key2, (void *) &left) != 0)
 	{
 	  puts ("destr1: setspecific failed");
 	  exit (1);
@@ -53,7 +54,8 @@ destr2 (void *arg)
     {
       puts ("set key1");
 
-      if (pthread_setspecific (key1, (void *) 1l) != 0)
+      /* Use an arbirary but valid pointer to avoid GCC warnings.  */
+      if (pthread_setspecific (key1, (void *) &left) != 0)
 	{
 	  puts ("destr2: setspecific failed");
 	  exit (1);
@@ -68,8 +70,9 @@ tf (void *arg)
   /* Let the destructors work.  */
   left = 7;
 
-  if (pthread_setspecific (key1, (void *) 1l) != 0
-      || pthread_setspecific (key2, (void *) 1l) != 0)
+  /* Use an arbirary but valid pointer to avoid GCC warnings.  */
+  if (pthread_setspecific (key1, (void *) &left) != 0
+      || pthread_setspecific (key2, (void *) &left) != 0)
     {
       puts ("tf: setspecific failed");
       exit (1);
diff --git a/nptl/tst-tsd4.c b/nptl/tst-tsd4.c
index cc1ada4d4d..d72219466f 100644
--- a/nptl/tst-tsd4.c
+++ b/nptl/tst-tsd4.c
@@ -34,7 +34,8 @@ destr (void *arg)
 {
   ++rounds;
 
-  if (pthread_setspecific (key, (void *) 1l) != 0)
+  /* Use an arbirary but valid pointer to avoid GCC warnings.  */
+  if (pthread_setspecific (key, (void *) &rounds) != 0)
     {
       puts ("destr: setspecific failed");
       exit (1);
@@ -45,7 +46,8 @@ destr (void *arg)
 static void *
 tf (void *arg)
 {
-  if (pthread_setspecific (key, (void *) 1l) != 0)
+  /* Use an arbirary but valid pointer to avoid GCC warnings.  */
+  if (pthread_setspecific (key, (void *) &rounds) != 0)
     {
       puts ("tf: setspecific failed");
       exit (1);
diff --git a/sysdeps/pthread/tst-key2.c b/sysdeps/pthread/tst-key2.c
index 6828873e41..28ba547073 100644
--- a/sysdeps/pthread/tst-key2.c
+++ b/sysdeps/pthread/tst-key2.c
@@ -56,7 +56,8 @@ tf (void *arg)
 {
   pthread_key_t *key = (pthread_key_t *) arg;
 
-  if (pthread_setspecific (*key, (void *) -1l) != 0)
+  /* Use an arbirary but valid pointer to avoid GCC warnings.  */
+  if (pthread_setspecific (*key, arg) != 0)
     {
       write_message ("setspecific failed\n");
       _exit (1);
diff --git a/sysdeps/pthread/tst-key3.c b/sysdeps/pthread/tst-key3.c
index 5cf5dcf06f..b6cc8c49a3 100644
--- a/sysdeps/pthread/tst-key3.c
+++ b/sysdeps/pthread/tst-key3.c
@@ -59,7 +59,7 @@ tf (void *arg)
 {
   pthread_key_t *key = (pthread_key_t *) arg;
 
-  if (pthread_setspecific (*key, (void *) -1l) != 0)
+  if (pthread_setspecific (*key, arg) != 0)
     {
       write_message ("setspecific failed\n");
       _exit (1);
diff --git a/sysdeps/pthread/tst-tsd1.c b/sysdeps/pthread/tst-tsd1.c
index eeabfb9012..37b4aef27e 100644
--- a/sysdeps/pthread/tst-tsd1.c
+++ b/sysdeps/pthread/tst-tsd1.c
@@ -27,6 +27,9 @@ do_test (void)
   pthread_key_t key1;
   pthread_key_t key2;
   void *value;
+  /* Addresses of val1 and val2 are used as arbitrary but valid pointers
+     in calls to pthread_setspecific to avoid GCC warnings.  */
+  char val1 = 0, val2 = 0;
   int result = 0;
   int err;
 
@@ -45,7 +48,7 @@ do_test (void)
       result = 1;
     }
 
-  err = pthread_setspecific (key1, (void *) -2l);
+  err = pthread_setspecific (key1, (void *) &val1);
   if (err != 0)
     {
       printf ("1st setspecific failed: %s\n", strerror (err));
@@ -58,13 +61,13 @@ do_test (void)
       puts ("2nd getspecific == NULL\n");
       result = 1;
     }
-  else if (value != (void *) -2l)
+  else if (value != (void *) &val1)
     {
-      puts ("2nd getspecific != -2l\n");
+      puts ("2nd getspecific != &val1l\n");
       result = 1;
     }
 
-  err = pthread_setspecific (key1, (void *) -3l);
+  err = pthread_setspecific (key1, (void *) &val2);
   if (err != 0)
     {
       printf ("2nd setspecific failed: %s\n", strerror (err));
@@ -77,9 +80,9 @@ do_test (void)
       puts ("3rd getspecific == NULL\n");
       result = 1;
     }
-  else if (value != (void *) -3l)
+  else if (value != (void *) &val2)
     {
-      puts ("3rd getspecific != -2l\n");
+      puts ("3rd getspecific != &val2\n");
       result = 1;
     }
 
diff --git a/sysdeps/pthread/tst-tsd2.c b/sysdeps/pthread/tst-tsd2.c
index 275bbccfdd..6b4ca7c49f 100644
--- a/sysdeps/pthread/tst-tsd2.c
+++ b/sysdeps/pthread/tst-tsd2.c
@@ -27,7 +27,7 @@ static int result;
 static void
 destr (void *arg)
 {
-  if (arg != (void *) -2l)
+  if (arg != (void *) &result)
     result = 2;
   else
     result = 0;
@@ -40,7 +40,8 @@ tf (void *arg)
   pthread_key_t key = (pthread_key_t) (long int) arg;
   int err;
 
-  err = pthread_setspecific (key, (void *) -2l);
+  /* Use an arbirary but valid pointer to avoid GCC warnings.  */
+  err = pthread_setspecific (key, &result);
   if (err != 0)
     result = 3;
 
diff --git a/sysdeps/pthread/tst-tsd5.c b/sysdeps/pthread/tst-tsd5.c
index 5f62b5a6e8..e875863892 100644
--- a/sysdeps/pthread/tst-tsd5.c
+++ b/sysdeps/pthread/tst-tsd5.c
@@ -53,7 +53,8 @@ do_test (void)
       puts ("key_create failed");
       return 1;
     }
-  if (pthread_setspecific (k, (void *) 1) != 0)
+  /* Use an arbitrary but valid pointer as the value.  */
+  if (pthread_setspecific (k, (void *) &k) != 0)
     {
       puts ("setspecific failed");
       return 1;
diff --git a/sysdeps/pthread/tst-tsd6.c b/sysdeps/pthread/tst-tsd6.c
index debb1dd367..5f2aa42e98 100644
--- a/sysdeps/pthread/tst-tsd6.c
+++ b/sysdeps/pthread/tst-tsd6.c
@@ -17,7 +17,8 @@ tf (void *arg)
   for (int i = 0; i < NKEYS; ++i)
     {
       void *p = pthread_getspecific (keys[i]);
-      pthread_setspecific (keys[i], (void *) 7);
+      /* Use an arbitrary but valid pointer as the value.  */
+      pthread_setspecific (keys[i], (void *) keys);
       if (p != NULL)
 	res = p;
     }

Re: [PATCH] add attribute none to pthread_setspecific (BZ #27714)

[Florian Weimer via Libc-alpha]

* Martin Sebor:

> diff --git a/nptl/tst-tsd3.c b/nptl/tst-tsd3.c
> index 0dd39ccb2b..45c7e4e1ea 100644
> --- a/nptl/tst-tsd3.c
> +++ b/nptl/tst-tsd3.c
> @@ -37,7 +37,8 @@ destr1 (void *arg)
>      {
>        puts ("set key2");
>  
> -      if (pthread_setspecific (key2, (void *) 1l) != 0)
> +      /* Use an arbirary but valid pointer to avoid GCC warnings.  */
> +      if (pthread_setspecific (key2, (void *) &left) != 0)
>  	{
>  	  puts ("destr1: setspecific failed");
>  	  exit (1);
> @@ -53,7 +54,8 @@ destr2 (void *arg)
>      {
>        puts ("set key1");
>  
> -      if (pthread_setspecific (key1, (void *) 1l) != 0)
> +      /* Use an arbirary but valid pointer to avoid GCC warnings.  */
> +      if (pthread_setspecific (key1, (void *) &left) != 0)
>  	{
>  	  puts ("destr2: setspecific failed");
>  	  exit (1);

Sorry, this is clearly a bug in attribute access (none).  No access
should mean no access, not access to one byte, as the warning currently
implies.

Please fix this for GCC 11.2 and adjust the glibc version check for the
none variant of the attribute.

Thanks,
Florian

Re: [PATCH] add attribute none to pthread_setspecific (BZ #27714)

[Martin Sebor via Libc-alpha]

On 4/28/21 1:32 AM, Florian Weimer wrote:
> * Martin Sebor:
> 
>> diff --git a/nptl/tst-tsd3.c b/nptl/tst-tsd3.c
>> index 0dd39ccb2b..45c7e4e1ea 100644
>> --- a/nptl/tst-tsd3.c
>> +++ b/nptl/tst-tsd3.c
>> @@ -37,7 +37,8 @@ destr1 (void *arg)
>>       {
>>         puts ("set key2");
>>   
>> -      if (pthread_setspecific (key2, (void *) 1l) != 0)
>> +      /* Use an arbirary but valid pointer to avoid GCC warnings.  */
>> +      if (pthread_setspecific (key2, (void *) &left) != 0)
>>   	{
>>   	  puts ("destr1: setspecific failed");
>>   	  exit (1);
>> @@ -53,7 +54,8 @@ destr2 (void *arg)
>>       {
>>         puts ("set key1");
>>   
>> -      if (pthread_setspecific (key1, (void *) 1l) != 0)
>> +      /* Use an arbirary but valid pointer to avoid GCC warnings.  */
>> +      if (pthread_setspecific (key1, (void *) &left) != 0)
>>   	{
>>   	  puts ("destr2: setspecific failed");
>>   	  exit (1);
> 
> Sorry, this is clearly a bug in attribute access (none).  No access
> should mean no access, not access to one byte, as the warning currently
> implies.

It's not a bug.  Access none was introduced as a check whether
the object has the expected size without assuming it's written
to or read from (each access mode has its own checks).  Pointers
to void are treated as char*.  This was documented in the GCC 10
manual when attribute access was first added and hasn't changed
with GCC 11.

The motivating use case for access none was a Linux kernel API
(I think check_copy_size) used to verify that a pointer points
to an object with the expected size.

> 
> Please fix this for GCC 11.2 and adjust the glibc version check for the
> none variant of the attribute.

I'll see if I can come up with a solution for
the pthread_setspecific use case.

Martin

Re: [PATCH] add attribute none to pthread_setspecific (BZ #27714)

[Florian Weimer via Libc-alpha]

* Martin Sebor:

> On 4/28/21 1:32 AM, Florian Weimer wrote:
>> * Martin Sebor:
>> 
>>> diff --git a/nptl/tst-tsd3.c b/nptl/tst-tsd3.c
>>> index 0dd39ccb2b..45c7e4e1ea 100644
>>> --- a/nptl/tst-tsd3.c
>>> +++ b/nptl/tst-tsd3.c
>>> @@ -37,7 +37,8 @@ destr1 (void *arg)
>>>       {
>>>         puts ("set key2");
>>>   -      if (pthread_setspecific (key2, (void *) 1l) != 0)
>>> +      /* Use an arbirary but valid pointer to avoid GCC warnings.  */
>>> +      if (pthread_setspecific (key2, (void *) &left) != 0)
>>>   	{
>>>   	  puts ("destr1: setspecific failed");
>>>   	  exit (1);
>>> @@ -53,7 +54,8 @@ destr2 (void *arg)
>>>       {
>>>         puts ("set key1");
>>>   -      if (pthread_setspecific (key1, (void *) 1l) != 0)
>>> +      /* Use an arbirary but valid pointer to avoid GCC warnings.  */
>>> +      if (pthread_setspecific (key1, (void *) &left) != 0)
>>>   	{
>>>   	  puts ("destr2: setspecific failed");
>>>   	  exit (1);
>> Sorry, this is clearly a bug in attribute access (none).  No access
>> should mean no access, not access to one byte, as the warning currently
>> implies.
>
> It's not a bug.  Access none was introduced as a check whether
> the object has the expected size without assuming it's written
> to or read from (each access mode has its own checks).  Pointers
> to void are treated as char*.  This was documented in the GCC 10
> manual when attribute access was first added and hasn't changed
> with GCC 11.
>
> The motivating use case for access none was a Linux kernel API
> (I think check_copy_size) used to verify that a pointer points
> to an object with the expected size.

But pthread_setspecific does not perform any address range validity
checks, so an attribute that implies some validation of the pointer
target is not correct.

Is it possible to set the data size as 0?

Thanks,
Florian

Re: [PATCH] add attribute none to pthread_setspecific (BZ #27714)

[Martin Sebor via Libc-alpha]

On 4/29/21 1:45 AM, Florian Weimer wrote:
> * Martin Sebor:
> 
>> On 4/28/21 1:32 AM, Florian Weimer wrote:
>>> * Martin Sebor:
>>>
>>>> diff --git a/nptl/tst-tsd3.c b/nptl/tst-tsd3.c
>>>> index 0dd39ccb2b..45c7e4e1ea 100644
>>>> --- a/nptl/tst-tsd3.c
>>>> +++ b/nptl/tst-tsd3.c
>>>> @@ -37,7 +37,8 @@ destr1 (void *arg)
>>>>        {
>>>>          puts ("set key2");
>>>>    -      if (pthread_setspecific (key2, (void *) 1l) != 0)
>>>> +      /* Use an arbirary but valid pointer to avoid GCC warnings.  */
>>>> +      if (pthread_setspecific (key2, (void *) &left) != 0)
>>>>    	{
>>>>    	  puts ("destr1: setspecific failed");
>>>>    	  exit (1);
>>>> @@ -53,7 +54,8 @@ destr2 (void *arg)
>>>>        {
>>>>          puts ("set key1");
>>>>    -      if (pthread_setspecific (key1, (void *) 1l) != 0)
>>>> +      /* Use an arbirary but valid pointer to avoid GCC warnings.  */
>>>> +      if (pthread_setspecific (key1, (void *) &left) != 0)
>>>>    	{
>>>>    	  puts ("destr2: setspecific failed");
>>>>    	  exit (1);
>>> Sorry, this is clearly a bug in attribute access (none).  No access
>>> should mean no access, not access to one byte, as the warning currently
>>> implies.
>>
>> It's not a bug.  Access none was introduced as a check whether
>> the object has the expected size without assuming it's written
>> to or read from (each access mode has its own checks).  Pointers
>> to void are treated as char*.  This was documented in the GCC 10
>> manual when attribute access was first added and hasn't changed
>> with GCC 11.
>>
>> The motivating use case for access none was a Linux kernel API
>> (I think check_copy_size) used to verify that a pointer points
>> to an object with the expected size.
> 
> But pthread_setspecific does not perform any address range validity
> checks, so an attribute that implies some validation of the pointer
> target is not correct.

C (and by extension POSIX) require that pointer arguments to library
functions be valid so the incorrect aspect of applying the attribute
here is that it causes warnings for valid past-the-end pointers.
I don't expect there's more code like that in the wild than there
is code that calls the function with a pointer to uninitialized
memory (e.g., the result of malloc).  So while I agree that
the result of the new annotation isn't perfect I think it will
cause fewer false positives that without it.

> 
> Is it possible to set the data size as 0?

There's no way to do that with the existing attribute (in GCC 11.1).
But a solution might be to change GCC 11.2 to treat access none with
void* and no size as size zero.  I'll look into that.

Martin

> 
> Thanks,
> Florian
> 

Re: [PATCH] add attribute none to pthread_setspecific (BZ #27714)

[Florian Weimer via Libc-alpha]

* Martin Sebor:

>> Is it possible to set the data size as 0?
>
> There's no way to do that with the existing attribute (in GCC 11.1).
> But a solution might be to change GCC 11.2 to treat access none with
> void* and no size as size zero.  I'll look into that.

Thanks, looking forward to it.

Florian

Re: [PATCH] add attribute none to pthread_setspecific (BZ #27714)

[H.J. Lu via Libc-alpha]

On Tue, Apr 27, 2021 at 6:00 PM Martin Sebor via Libc-alpha
<libc-alpha@sourceware.org> wrote:
>
> On 4/27/21 3:58 PM, Joseph Myers wrote:
> > On Tue, 27 Apr 2021, Martin Sebor via Libc-alpha wrote:
> >
> >> These are warnings in my build (I've seen a few others scroll by
> >> and have always assumed they were expected(*)).  Those you pasted
> >
> > Most warnings are errors by default (unless you use --disable-werror,
> > which should never normally be used in glibc development unless you're
> > e.g. reviewing the warnings you get if you add extra warning options to
> > those with which glibc is built by default, in order to fix those warnings
> > before adding the extra options).
>
> Ah, that's what my build script does and I keep forgetting to
> change that when working on Glibc (as opposed to testing new
> GCC warnings).  My bad.  The attached diff patches up the tests
> to pass the function a valid argument.  I'll plan to commit it
> shortly unless you prefer some other solution.
>

It doesn't work:

https://sourceware.org/bugzilla/show_bug.cgi?id=27714#c3

-- 
H.J.