add attribute none to pthread_setspecific (BZ #27714)

  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
  

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 \
  

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.
  

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
  

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.
  

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
  

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.
  

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
  

* 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
  

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
>
  

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.)
  

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.)
>
  

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.)
  

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.)
>
  

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.)
>>
>
  

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
  

* 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
  

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
  

* 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
  

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
>
  

* 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