| Message ID | 20140913113918.GA22732@waldemar-brodkorb.de |
|---|---|
| State | New |
| Headers | show |
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.
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> >
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
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.
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);'.
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.
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.
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.
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.
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.
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.
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~
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.
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.
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>
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(+)