sparc: fix for missing include file
diff mbox

Message ID 20140913113918.GA22732@waldemar-brodkorb.de
State New
Headers show

Commit Message

Waldemar Brodkorb Sept. 13, 2014, 11:39 a.m. UTC
Fix idea from Roland McGrath <roland@hack.frob.com>
Fixes following compile error targeting sparc32 v8

../sysdeps/sparc/sparc32/sem_trywait.c: In function '__new_sem_trywait':
../sysdeps/sparc/sparc32/sem_trywait.c:35:11: error: dereferencing
pointer to incomplete type
   if (isem->value > 0)
           ^
In file included from ../include/list.h:45:0,
                 from ../sysdeps/sparc/nptl/tls.h:28,
                 from ../include/errno.h:27,
                 from ../sysdeps/sparc/sparc32/sem_trywait.c:20:
../sysdeps/sparc/sparc32/sem_trywait.c:38:43: error: dereferencing
pointer to incomplete type
  val = atomic_decrement_if_positive (&isem->value);
                                           ^
../include/atomic.h:372:18: note: in definition of macro

Signed-off-by: Waldemar Brodkorb <wbx@openadk.org>
---
 sysdeps/sparc/sparc32/sem_trywait.c | 1 +
 1 file changed, 1 insertion(+)

Comments

David Miller Sept. 13, 2014, 9:38 p.m. UTC | #1
From: Waldemar Brodkorb <wbx@openadk.org>
Date: Sat, 13 Sep 2014 13:39:19 +0200

> Fix idea from Roland McGrath <roland@hack.frob.com>
> Fixes following compile error targeting sparc32 v8
> 
> ../sysdeps/sparc/sparc32/sem_trywait.c: In function '__new_sem_trywait':
> ../sysdeps/sparc/sparc32/sem_trywait.c:35:11: error: dereferencing
> pointer to incomplete type
>    if (isem->value > 0)
>            ^
> In file included from ../include/list.h:45:0,
>                  from ../sysdeps/sparc/nptl/tls.h:28,
>                  from ../include/errno.h:27,
>                  from ../sysdeps/sparc/sparc32/sem_trywait.c:20:
> ../sysdeps/sparc/sparc32/sem_trywait.c:38:43: error: dereferencing
> pointer to incomplete type
>   val = atomic_decrement_if_positive (&isem->value);
>                                            ^
> ../include/atomic.h:372:18: note: in definition of macro
> 
> Signed-off-by: Waldemar Brodkorb <wbx@openadk.org>

This is fine, thanks for fixing this.
Ondřej Bílka Dec. 11, 2014, 1:57 p.m. UTC | #2
This was not commited yet, David could you commit it?

On Sat, Sep 13, 2014 at 01:39:19PM +0200, Waldemar Brodkorb wrote:
> Fix idea from Roland McGrath <roland@hack.frob.com>
> Fixes following compile error targeting sparc32 v8
> 
> ../sysdeps/sparc/sparc32/sem_trywait.c: In function '__new_sem_trywait':
> ../sysdeps/sparc/sparc32/sem_trywait.c:35:11: error: dereferencing
> pointer to incomplete type
>    if (isem->value > 0)
>            ^
> In file included from ../include/list.h:45:0,
>                  from ../sysdeps/sparc/nptl/tls.h:28,
>                  from ../include/errno.h:27,
>                  from ../sysdeps/sparc/sparc32/sem_trywait.c:20:
> ../sysdeps/sparc/sparc32/sem_trywait.c:38:43: error: dereferencing
> pointer to incomplete type
>   val = atomic_decrement_if_positive (&isem->value);
>                                            ^
> ../include/atomic.h:372:18: note: in definition of macro
> 
> Signed-off-by: Waldemar Brodkorb <wbx@openadk.org>
> 
> ---
> sysdeps/sparc/sparc32/sem_trywait.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/sysdeps/sparc/sparc32/sem_trywait.c b/sysdeps/sparc/sparc32/sem_trywait.c
> index 7d0fc55..ad9b4ad 100644
> --- a/sysdeps/sparc/sparc32/sem_trywait.c
> +++ b/sysdeps/sparc/sparc32/sem_trywait.c
> @@ -22,6 +22,7 @@
>  #include <lowlevellock.h>
>  #include <internaltypes.h>
>  #include <semaphore.h>
> +#include <sparc-nptl.h>
>  
>  #include <shlib-compat.h>
>
David Miller Dec. 11, 2014, 8:05 p.m. UTC | #3
From: Ondřej Bílka <neleai@seznam.cz>

Date: Thu, 11 Dec 2014 14:57:16 +0100

> This was not commited yet, David could you commit it?


I don't want to commit a change into a tree that doesn't even
build successfully for me:

malloc.c: In function ‘_int_malloc’:
malloc.c:1858:1: error: inlining failed in call to ‘alloc_perturb.part.5’: call is unlikely and code size would grow [-Werror=inline]
malloc.c:3292:1: error: called from here [-Werror=inline]
malloc.c:1858:1: error: inlining failed in call to ‘alloc_perturb.part.5’: call is unlikely and code size would grow [-Werror=inline]
malloc.c:3292:1: error: called from here [-Werror=inline]
malloc.c:1858:1: error: inlining failed in call to ‘alloc_perturb.part.5’: call is unlikely and code size would grow [-Werror=inline]
malloc.c:3292:1: error: called from here [-Werror=inline]
malloc.c:1858:1: error: inlining failed in call to ‘alloc_perturb.part.5’: call is unlikely and code size would grow [-Werror=inline]
malloc.c:3292:1: error: called from here [-Werror=inline]
malloc.c:1858:1: error: inlining failed in call to ‘alloc_perturb.part.5’: call is unlikely and code size would grow [-Werror=inline]
malloc.c:3292:1: error: called from here [-Werror=inline]
malloc.c:1858:1: error: inlining failed in call to ‘alloc_perturb.part.5’: call is unlikely and code size would grow [-Werror=inline]
malloc.c:3292:1: error: called from here [-Werror=inline]
malloc.c:1858:1: error: inlining failed in call to ‘alloc_perturb.part.5’: call is unlikely and code size would grow [-Werror=inline]
malloc.c:3292:1: error: called from here [-Werror=inline]
malloc.c:1858:1: error: inlining failed in call to ‘alloc_perturb.part.5’: call is unlikely and code size would grow [-Werror=inline]
malloc.c:3292:1: error: called from here [-Werror=inline]
malloc.c: In function ‘_int_malloc’:
malloc.c:1858:1: error: inlining failed in call to ‘alloc_perturb.part.5’: call is unlikely and code size would grow [-Werror=inline]
malloc.c:3292:1: error: called from here [-Werror=inline]
malloc.c:1858:1: error: inlining failed in call to ‘alloc_perturb.part.5’: call is unlikely and code size would grow [-Werror=inline]
malloc.c:3292:1: error: called from here [-Werror=inline]
malloc.c:1858:1: error: inlining failed in call to ‘alloc_perturb.part.5’: call is unlikely and code size would grow [-Werror=inline]
malloc.c:3292:1: error: called from here [-Werror=inline]
malloc.c:1858:1: error: inlining failed in call to ‘alloc_perturb.part.5’: call is unlikely and code size would grow [-Werror=inline]
malloc.c:3292:1: error: called from here [-Werror=inline]
malloc.c:1858:1: error: inlining failed in call to ‘alloc_perturb.part.5’: call is unlikely and code size would grow [-Werror=inline]
malloc.c:3292:1: error: called from here [-Werror=inline]
malloc.c:1858:1: error: inlining failed in call to ‘alloc_perturb.part.5’: call is unlikely and code size would grow [-Werror=inline]
malloc.c:3292:1: error: called from here [-Werror=inline]
malloc.c:1858:1: error: inlining failed in call to ‘alloc_perturb.part.5’: call is unlikely and code size would grow [-Werror=inline]
malloc.c:3292:1: error: called from here [-Werror=inline]
malloc.c:1858:1: error: inlining failed in call to ‘alloc_perturb.part.5’: call is unlikely and code size would grow [-Werror=inline]
malloc.c:3292:1: error: called from here [-Werror=inline]
cc1: all warnings being treated as errors
David Miller Dec. 13, 2014, 6 p.m. UTC | #4
From: David Miller <davem@davemloft.net>

Date: Thu, 11 Dec 2014 15:05:35 -0500 (EST)

> From: Ondřej Bílka <neleai@seznam.cz>

> Date: Thu, 11 Dec 2014 14:57:16 +0100

> 

>> This was not commited yet, David could you commit it?

> 

> I don't want to commit a change into a tree that doesn't even

> build successfully for me:

> 

> malloc.c: In function ‘_int_malloc’:


So I removed the inlines just to make forward progress, then got:

strncat.c: In function ‘strncat’:
strncat.c:76:6: error: ‘c’ may be used uninitialized in this function [-Werror=uninitialized]

This is the generic C strncat.c in string/strncat.c, and actually the
compiler is right here in that when 'n' is zero 'c' will in fact be
tested without being initialized to anything.
Paul Eggert Dec. 13, 2014, 7:08 p.m. UTC | #5
David Miller wrote:
> strncat.c: In function ‘strncat’:
> strncat.c:76:6: error: ‘c’ may be used uninitialized in this function [-Werror=uninitialized]
>
> This is the generic C strncat.c in string/strncat.c, and actually the
> compiler is right here in that when 'n' is zero 'c' will in fact be
> tested without being initialized to anything.

That's amusing, since the code works regardless of whether 'c' is initialized 
properly, assuming that using an uninitialized value doesn't trap or have 
similarly bad behavior.  So this looks like another candidate for adding pragmas 
to suppress the diagnostic.  In Gnulib we'd say 'char c IF_LINT (= 0);'.
David Miller Dec. 13, 2014, 7:32 p.m. UTC | #6
From: Paul Eggert <eggert@cs.ucla.edu>

Date: Sat, 13 Dec 2014 11:08:47 -0800

> David Miller wrote:

>> strncat.c: In function ‘strncat’:

>> strncat.c:76:6: error: ‘c’ may be used uninitialized in this function

>> [-Werror=uninitialized]

>>

>> This is the generic C strncat.c in string/strncat.c, and actually the

>> compiler is right here in that when 'n' is zero 'c' will in fact be

>> tested without being initialized to anything.

> 

> That's amusing, since the code works regardless of whether 'c' is

> initialized properly, assuming that using an uninitialized value

> doesn't trap or have similarly bad behavior.  So this looks like

> another candidate for adding pragmas to suppress the diagnostic.  In

> Gnulib we'd say 'char c IF_LINT (= 0);'.


How does the code "work"?  The test of 'c' against '\0' at the end of
the function happens even if 'n' is passed as zero in which case 'c'
will not be initialized at all.
Paul Eggert Dec. 13, 2014, 8 p.m. UTC | #7
David Miller wrote:

>>> >>strncat.c: In function ‘strncat’:
>>> >>strncat.c:76:6: error: ‘c’ may be used uninitialized in this function
>>> >>[-Werror=uninitialized]
>> ...
>> >That's amusing, since the code works regardless of whether 'c' is
>> >initialized properly, assuming that using an uninitialized value
>> >doesn't trap or have similarly bad behavior....
> How does the code "work"?  The test of 'c' against '\0' at the end of
> the function happens even if 'n' is passed as zero in which case 'c'
> will not be initialized at all.

Sure, but the result of the test doesn't matter, because in that case the code:

   if (c != '\0')
     *++s1 = '\0';

possibly assigns zero to a location that is already zero.  That is, in the case 
you mention this code is a no-op no matter what c's value is.  It's pretty funny.

True, the code is relying on behavior that is undefined, since the C standard 
says using an uninitialized variable has undefined behavior.  But I don't know 
of any libc platforms where the code doesn't work, and since our general rule 
has been to not slow down glibc to pacify GCC, the usual gaggle of pragmas are 
needed here.

Another amusing tidbit: this is not the only part of string/strncat.c that 
relies on undefined behavior.  The earlier statement 's1 -= 1;' does so as well, 
it's just that GCC doesn't diagnose the earlier statement so we don't need to 
pragma-ize it.
David Miller Dec. 13, 2014, 8:31 p.m. UTC | #8
From: Paul Eggert <eggert@cs.ucla.edu>

Date: Sat, 13 Dec 2014 12:00:09 -0800

> David Miller wrote:

> 

>>>> >>strncat.c: In function ‘strncat’:

>>>> >>strncat.c:76:6: error: ‘c’ may be used uninitialized in this function

>>>> >>[-Werror=uninitialized]

>>> ...

>>> >That's amusing, since the code works regardless of whether 'c' is

>>> >initialized properly, assuming that using an uninitialized value

>>> >doesn't trap or have similarly bad behavior....

>> How does the code "work"?  The test of 'c' against '\0' at the end of

>> the function happens even if 'n' is passed as zero in which case 'c'

>> will not be initialized at all.

> 

> Sure, but the result of the test doesn't matter, because in that case

> the code:

> 

>   if (c != '\0')

>     *++s1 = '\0';

> 

> possibly assigns zero to a location that is already zero.  That is, in

> the case you mention this code is a no-op no matter what c's value is.

> It's pretty funny.


The destination is not necessarily NULL terminated, I see nothing
in the definition of this function that says so.

In fact, if anything, we are required to execute this step and put
that sole and only NULL terminating charater into the destination if
'n' is passed as zero.

The only reasonable thing to do, therefore, is to explicitly
initialize 'c' to '\0', and not with some conditional LINT'ish
thing.
Andreas Schwab Dec. 13, 2014, 8:43 p.m. UTC | #9
David Miller <davem@davemloft.net> writes:

> The destination is not necessarily NULL terminated,

This is strn*cat*, the destination must be a string.  Otherwise, what
would strlen (s1) be?

Andreas.
David Miller Dec. 13, 2014, 8:48 p.m. UTC | #10
From: Andreas Schwab <schwab@linux-m68k.org>
Date: Sat, 13 Dec 2014 21:43:14 +0100

> David Miller <davem@davemloft.net> writes:
> 
>> The destination is not necessarily NULL terminated,
> 
> This is strn*cat*, the destination must be a string.  Otherwise, what
> would strlen (s1) be?

Indeed, thanks for the clarification.
Will Newton Dec. 15, 2014, 9:34 a.m. UTC | #11
On 13 December 2014 at 20:00, Paul Eggert <eggert@cs.ucla.edu> wrote:
> David Miller wrote:
>
>>>> >>strncat.c: In function ‘strncat’:
>>>> >>strncat.c:76:6: error: ‘c’ may be used uninitialized in this function
>>>> >>[-Werror=uninitialized]
>>>
>>> ...
>>> >That's amusing, since the code works regardless of whether 'c' is
>>> >initialized properly, assuming that using an uninitialized value
>>> >doesn't trap or have similarly bad behavior....
>>
>> How does the code "work"?  The test of 'c' against '\0' at the end of
>> the function happens even if 'n' is passed as zero in which case 'c'
>> will not be initialized at all.
>
>
> Sure, but the result of the test doesn't matter, because in that case the
> code:
>
>   if (c != '\0')
>     *++s1 = '\0';
>
> possibly assigns zero to a location that is already zero.  That is, in the
> case you mention this code is a no-op no matter what c's value is.  It's
> pretty funny.
>
> True, the code is relying on behavior that is undefined, since the C
> standard says using an uninitialized variable has undefined behavior.  But I
> don't know of any libc platforms where the code doesn't work, and since our
> general rule has been to not slow down glibc to pacify GCC, the usual gaggle
> of pragmas are needed here.

Loading a zero constant into c is cheap and the side effect of the
undefined behaviour is potentially a write to memory so it is not
always going to be a performance win to do this. Also potentially
runtime analysis tools (e.g. sanitizers, valgrind) may trip over this
so it seems to me a better idea just to not invoke undefined
behaviours.
Richard Henderson Dec. 15, 2014, 6:04 p.m. UTC | #12
On 12/15/2014 03:34 AM, Will Newton wrote:
> Loading a zero constant into c is cheap and the side effect of the
> undefined behaviour is potentially a write to memory so it is not
> always going to be a performance win to do this. Also potentially
> runtime analysis tools (e.g. sanitizers, valgrind) may trip over this
> so it seems to me a better idea just to not invoke undefined
> behaviours.

My thoughts exactly.


r~
Paul Eggert Dec. 15, 2014, 8:37 p.m. UTC | #13
On 12/15/2014 01:34 AM, Will Newton wrote:
> Loading a zero constant into c is cheap and the side effect of the
> undefined behaviour is potentially a write to memory so it is not
> always going to be a performance win to do this.

It's going to be a performance win in the normal case where n != 0. It'd 
take a reeeaally weird usage pattern to make clearing c a performance win.

> runtime analysis tools (e.g. sanitizers, valgrind) may trip over this
>
This is the real question.  In many places, glibc is not safe in the 
presence of sanitizers that insert undefined behavior whenever the C 
standard allows them to.  Current practice is for these sanitizers to 
maintain a list of exceptions, so that they don't cry wolf in situations 
like these.  Should we continue this practice, or should we strive to 
make glibc safe even in the presence of a purposely-finicky implementation?

For strncat it doesn't matter: nobody with a clue ever uses strncat, so 
the only thing that matters about its performance is how many code bytes 
its instructions waste.  For other parts of glibc, though, this question 
can be a big deal.
Torvald Riegel Dec. 15, 2014, 9:32 p.m. UTC | #14
On Mon, 2014-12-15 at 12:37 -0800, Paul Eggert wrote:
> On 12/15/2014 01:34 AM, Will Newton wrote:

> > runtime analysis tools (e.g. sanitizers, valgrind) may trip over this
> >
> This is the real question.  In many places, glibc is not safe in the 
> presence of sanitizers that insert undefined behavior whenever the C 
> standard allows them to.  Current practice is for these sanitizers to 
> maintain a list of exceptions, so that they don't cry wolf in situations 
> like these.  Should we continue this practice, or should we strive to 
> make glibc safe even in the presence of a purposely-finicky implementation?

I think we should try to avoid undefined behavior, unless we really need
it.  Making it harder to use those tools won't be helpful in the long
term, in my opinion.  The less "special" glibc code is the better,
probably.

Patch
diff mbox

diff --git a/sysdeps/sparc/sparc32/sem_trywait.c b/sysdeps/sparc/sparc32/sem_trywait.c
index 7d0fc55..ad9b4ad 100644
--- a/sysdeps/sparc/sparc32/sem_trywait.c
+++ b/sysdeps/sparc/sparc32/sem_trywait.c
@@ -22,6 +22,7 @@ 
 #include <lowlevellock.h>
 #include <internaltypes.h>
 #include <semaphore.h>
+#include <sparc-nptl.h>
 
 #include <shlib-compat.h>