| Message ID | 20150203145649.GG1528@spoyarek.pnq.redhat.com |
|---|---|
| State | New |
| Headers | show |
... and I forgot to add bug-gnulib to cc before I hit send. Siddhesh On Tue, Feb 03, 2015 at 08:26:49PM +0530, Siddhesh Poyarekar wrote: > Hi, > > obstack_init does not completely initialize the obstack structure; it > leaves out the padding bits and valgrind complains about it on s390x: > > ==15793== Memcheck, a memory error detector > ==15793== Copyright (C) 2002-2012, and GNU GPL'd, by Julian Seward et al. > ==15793== Using Valgrind-3.8.1 and LibVEX; rerun with -h for copyright info > ==15793== Command: /root/obstack > ==15793== > ==15793== Conditional jump or move depends on uninitialised value(s) > ==15793== at 0x403E48CA4E: obstack_free (in /lib64/libc-2.12.so) > ==15793== by 0x8000072D: main (obstack.c:12) > ==15793== > ==15793== > ==15793== HEAP SUMMARY: > ==15793== in use at exit: 0 bytes in 0 blocks > ==15793== total heap usage: 1 allocs, 1 frees, 4,064 bytes allocated > ==15793== > ==15793== All heap blocks were freed -- no leaks are possible > ==15793== > ==15793== For counts of detected and suppressed errors, rerun with: -v > ==15793== Use --track-origins=yes to see where uninitialised values come from > ==15793== ERROR SUMMARY: 1 errors from 1 contexts (suppressed: 4 from 4) > > > The fix below (against gnulib, but is identical for glibc) initializes > all of the obstack struct at once. Verified that the valgrind warning > is fixed. OK for 2.22 and gnulib? > > Siddhesh > > ChangeLog for gnulib: > > obstack: Initialize whole obstack structure. > * lib/obstack.c (_obstack_begin): Initialize all of H. > > ChangeLog for glibc: > > [BZ #17919] > * malloc/obstack.c (_obstack_begin): Initialize all of H. > > diff --git a/malloc/obstack.c b/malloc/obstack.c > index 5bb3f0d..c1d6ded 100644 > --- a/lib/obstack.c > +++ b/lib/obstack.c > @@ -148,6 +148,8 @@ _obstack_begin (struct obstack *h, > { > struct _obstack_chunk *chunk; /* points to new chunk */ > > + memset (h, 0, sizeof (struct obstack)); > + > if (alignment == 0) > alignment = DEFAULT_ALIGNMENT; > if (size == 0)
On Tue, Feb 3, 2015 at 7:00 AM, Siddhesh Poyarekar <siddhesh@redhat.com> wrote: > ... and I forgot to add bug-gnulib to cc before I hit send. > > Siddhesh > > On Tue, Feb 03, 2015 at 08:26:49PM +0530, Siddhesh Poyarekar wrote: >> Hi, >> >> obstack_init does not completely initialize the obstack structure; it >> leaves out the padding bits and valgrind complains about it on s390x: >> >> ==15793== Memcheck, a memory error detector >> ==15793== Copyright (C) 2002-2012, and GNU GPL'd, by Julian Seward et al. >> ==15793== Using Valgrind-3.8.1 and LibVEX; rerun with -h for copyright info >> ==15793== Command: /root/obstack >> ==15793== >> ==15793== Conditional jump or move depends on uninitialised value(s) >> ==15793== at 0x403E48CA4E: obstack_free (in /lib64/libc-2.12.so) >> ==15793== by 0x8000072D: main (obstack.c:12) >> ==15793== >> ==15793== >> ==15793== HEAP SUMMARY: >> ==15793== in use at exit: 0 bytes in 0 blocks >> ==15793== total heap usage: 1 allocs, 1 frees, 4,064 bytes allocated >> ==15793== >> ==15793== All heap blocks were freed -- no leaks are possible >> ==15793== >> ==15793== For counts of detected and suppressed errors, rerun with: -v >> ==15793== Use --track-origins=yes to see where uninitialised values come from >> ==15793== ERROR SUMMARY: 1 errors from 1 contexts (suppressed: 4 from 4) >> >> >> The fix below (against gnulib, but is identical for glibc) initializes >> all of the obstack struct at once. Verified that the valgrind warning >> is fixed. OK for 2.22 and gnulib? >> >> Siddhesh >> >> ChangeLog for gnulib: >> >> obstack: Initialize whole obstack structure. >> * lib/obstack.c (_obstack_begin): Initialize all of H. >> >> ChangeLog for glibc: >> >> [BZ #17919] >> * malloc/obstack.c (_obstack_begin): Initialize all of H. >> >> diff --git a/malloc/obstack.c b/malloc/obstack.c >> index 5bb3f0d..c1d6ded 100644 >> --- a/lib/obstack.c >> +++ b/lib/obstack.c >> @@ -148,6 +148,8 @@ _obstack_begin (struct obstack *h, >> { >> struct _obstack_chunk *chunk; /* points to new chunk */ >> >> + memset (h, 0, sizeof (struct obstack)); >> + >> if (alignment == 0) >> alignment = DEFAULT_ALIGNMENT; >> if (size == 0) > > I think you should also remove h->use_extra_arg = 0;
On Tue, Feb 3, 2015 at 7:38 AM, H.J. Lu <hjl.tools@gmail.com> wrote: > On Tue, Feb 3, 2015 at 7:00 AM, Siddhesh Poyarekar <siddhesh@redhat.com> wrote: >> ... and I forgot to add bug-gnulib to cc before I hit send. >> >> Siddhesh >> >> On Tue, Feb 03, 2015 at 08:26:49PM +0530, Siddhesh Poyarekar wrote: >>> Hi, >>> >>> obstack_init does not completely initialize the obstack structure; it >>> leaves out the padding bits and valgrind complains about it on s390x: >>> >>> ==15793== Memcheck, a memory error detector >>> ==15793== Copyright (C) 2002-2012, and GNU GPL'd, by Julian Seward et al. >>> ==15793== Using Valgrind-3.8.1 and LibVEX; rerun with -h for copyright info >>> ==15793== Command: /root/obstack >>> ==15793== >>> ==15793== Conditional jump or move depends on uninitialised value(s) >>> ==15793== at 0x403E48CA4E: obstack_free (in /lib64/libc-2.12.so) >>> ==15793== by 0x8000072D: main (obstack.c:12) >>> ==15793== >>> ==15793== >>> ==15793== HEAP SUMMARY: >>> ==15793== in use at exit: 0 bytes in 0 blocks >>> ==15793== total heap usage: 1 allocs, 1 frees, 4,064 bytes allocated >>> ==15793== >>> ==15793== All heap blocks were freed -- no leaks are possible >>> ==15793== >>> ==15793== For counts of detected and suppressed errors, rerun with: -v >>> ==15793== Use --track-origins=yes to see where uninitialised values come from >>> ==15793== ERROR SUMMARY: 1 errors from 1 contexts (suppressed: 4 from 4) >>> >>> >>> The fix below (against gnulib, but is identical for glibc) initializes >>> all of the obstack struct at once. Verified that the valgrind warning >>> is fixed. OK for 2.22 and gnulib? >>> >>> Siddhesh >>> >>> ChangeLog for gnulib: >>> >>> obstack: Initialize whole obstack structure. >>> * lib/obstack.c (_obstack_begin): Initialize all of H. >>> >>> ChangeLog for glibc: >>> >>> [BZ #17919] >>> * malloc/obstack.c (_obstack_begin): Initialize all of H. >>> >>> diff --git a/malloc/obstack.c b/malloc/obstack.c >>> index 5bb3f0d..c1d6ded 100644 >>> --- a/lib/obstack.c >>> +++ b/lib/obstack.c >>> @@ -148,6 +148,8 @@ _obstack_begin (struct obstack *h, >>> { >>> struct _obstack_chunk *chunk; /* points to new chunk */ >>> >>> + memset (h, 0, sizeof (struct obstack)); >>> + >>> if (alignment == 0) >>> alignment = DEFAULT_ALIGNMENT; >>> if (size == 0) >> >> > > I think you should also remove > > h->use_extra_arg = 0; > And /* The initial chunk now contains no empty object. */ h->maybe_empty_object = 0; h->alloc_failed = 0;
Does this patch fix any actual bug? If so, what's the bug? Is there a faster way to fix it, other than zeroing out the entire struct obstack?
On Tue, Feb 03, 2015 at 08:08:54AM -0800, Paul Eggert wrote: > Does this patch fix any actual bug? If so, what's the bug? Is there a > faster way to fix it, other than zeroing out the entire struct obstack? It fixes a valgrind warning on s390x, but if by bug you mean a user visible problem, then no, there is no such thing. I can't think of a faster way that is not uglier, which is why I took the simpler, more readable approach. Siddhesh
On 02/03/2015 09:56 AM, Siddhesh Poyarekar wrote: > Hi, > > obstack_init does not completely initialize the obstack structure; it > leaves out the padding bits and valgrind complains about it on s390x: > > ==15793== Memcheck, a memory error detector > ==15793== Copyright (C) 2002-2012, and GNU GPL'd, by Julian Seward et al. > ==15793== Using Valgrind-3.8.1 and LibVEX; rerun with -h for copyright info > ==15793== Command: /root/obstack > ==15793== > ==15793== Conditional jump or move depends on uninitialised value(s) > ==15793== at 0x403E48CA4E: obstack_free (in /lib64/libc-2.12.so) > ==15793== by 0x8000072D: main (obstack.c:12) > ==15793== > ==15793== > ==15793== HEAP SUMMARY: > ==15793== in use at exit: 0 bytes in 0 blocks > ==15793== total heap usage: 1 allocs, 1 frees, 4,064 bytes allocated > ==15793== > ==15793== All heap blocks were freed -- no leaks are possible > ==15793== > ==15793== For counts of detected and suppressed errors, rerun with: -v > ==15793== Use --track-origins=yes to see where uninitialised values come from > ==15793== ERROR SUMMARY: 1 errors from 1 contexts (suppressed: 4 from 4) Is this a valgrind bug, false positive in valgrind, or bug in glibc? It clearly says we have a move that depends on an uninitizlied value, so something read the value and tried to do something with it. Cheers, Carlos.
[Also looping in bug-gnulib] On Tue, Feb 03, 2015 at 11:30:17AM -0500, Carlos O'Donell wrote: > Is this a valgrind bug, false positive in valgrind, or bug in glibc? > > It clearly says we have a move that depends on an uninitizlied value, > so something read the value and tried to do something with it. It is a combination of multiple things actually. The uninitialized value here is the padding in the structure and we have fixed such warnings in the past; see the nscd stats warning[1] for example. This warning doesn't trigger on x86 though due to the code that gcc generates for it - it explicitly ANDs the bit field in the struct before testing it. On s390, the optimizer does away with the AND operation and tests the entire thing for some reason - I haven't figured out why but the instruction combiner RTL pass does away with the AND operation. This is also why I started out filing a gcc bug, but then changed my mind and fixed it in glibc instead, since I presumed that the compiler can assume that the padding is also appropriately initialized. But then, I am not completely sure if the compiler is allowed to make that assumption. Siddhesh
Siddhesh Poyarekar <siddhesh@redhat.com> writes: > This is also why I started out filing a gcc bug, but then changed my > mind and fixed it in glibc instead, since I presumed that the compiler > can assume that the padding is also appropriately initialized. No, it can't. Padding must be assumed to be random. Andreas.
On Tue, Feb 3, 2015 at 8:08 AM, Paul Eggert <eggert@cs.ucla.edu> wrote: > Does this patch fix any actual bug? If so, what's the bug? Is there a > faster way to fix it, other than zeroing out the entire struct obstack? I believe that this addresses the MSAN used-uninitialized warning I explored privately a few months ago. Assuming it's the same one, initializing the entire struct obstack is not necessary.
On 02/03/2015 11:46 AM, Andreas Schwab wrote: > Siddhesh Poyarekar <siddhesh@redhat.com> writes: > >> This is also why I started out filing a gcc bug, but then changed my >> mind and fixed it in glibc instead, since I presumed that the compiler >> can assume that the padding is also appropriately initialized. > > No, it can't. Padding must be assumed to be random. Fully agreed. Cheers, Carlos.
On 02/03/2015 11:38 AM, Siddhesh Poyarekar wrote: > [Also looping in bug-gnulib] > > On Tue, Feb 03, 2015 at 11:30:17AM -0500, Carlos O'Donell wrote: >> Is this a valgrind bug, false positive in valgrind, or bug in glibc? >> >> It clearly says we have a move that depends on an uninitizlied value, >> so something read the value and tried to do something with it. > > It is a combination of multiple things actually. The uninitialized > value here is the padding in the structure and we have fixed such > warnings in the past; see the nscd stats warning[1] for example. That case was not as performance sensitive as creating and using obstacks. > This warning doesn't trigger on x86 though due to the code that gcc > generates for it - it explicitly ANDs the bit field in the struct > before testing it. On s390, the optimizer does away with the AND > operation and tests the entire thing for some reason - I haven't > figured out why but the instruction combiner RTL pass does away with > the AND operation. > > This is also why I started out filing a gcc bug, but then changed my > mind and fixed it in glibc instead, since I presumed that the compiler > can assume that the padding is also appropriately initialized. But > then, I am not completely sure if the compiler is allowed to make that > assumption. Thanks for the clarification. IMO zero-initialized padding, for this case, isn't something you can expect. Therefore I think it's a compiler bug. I think it's OK to work around this in glibc, but it needs a comment with a reference to the filed gcc bug. I do not think it is valid for gcc on s390x to use the entire bit field along with padding and I believe it could result in incorrect operation. Cheers, Carlos.
On Tue, Feb 03, 2015 at 11:49:26AM -0500, Carlos O'Donell wrote: > IMO zero-initialized padding, for this case, isn't something you can > expect. Therefore I think it's a compiler bug. Thanks, I've filed a bug now: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=64923 > I think it's OK to work around this in glibc, but it needs a comment > with a reference to the filed gcc bug. I do not think it is valid > for gcc on s390x to use the entire bit field along with padding and > I believe it could result in incorrect operation. Nothing is breaking due to this right now, so we could probably wait and see what the gcc folks think of this. Siddhesh
> On Feb 3, 2015, at 9:05 AM, Siddhesh Poyarekar <siddhesh@redhat.com> wrote: > >> On Tue, Feb 03, 2015 at 11:49:26AM -0500, Carlos O'Donell wrote: >> IMO zero-initialized padding, for this case, isn't something you can >> expect. Therefore I think it's a compiler bug. > > Thanks, I've filed a bug now: > > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=64923 It is not a glibc or a gcc bug but rather a valgrind one. We are comparing against zero since this is the sign bit. Valgrind does not realize that and gives a warning. Thanks, Andrew > >> I think it's OK to work around this in glibc, but it needs a comment >> with a reference to the filed gcc bug. I do not think it is valid >> for gcc on s390x to use the entire bit field along with padding and >> I believe it could result in incorrect operation. > > Nothing is breaking due to this right now, so we could probably wait > and see what the gcc folks think of this. > > Siddhesh
On Tue, Feb 03, 2015 at 09:10:50AM -0800, pinskia@gmail.com wrote: > It is not a glibc or a gcc bug but rather a valgrind one. We are > comparing against zero since this is the sign bit. Valgrind does not > realize that and gives a warning. Of course, thanks for responding. Siddhesh
On 02/03/2015 12:05 PM, Siddhesh Poyarekar wrote: > On Tue, Feb 03, 2015 at 11:49:26AM -0500, Carlos O'Donell wrote: >> IMO zero-initialized padding, for this case, isn't something you can >> expect. Therefore I think it's a compiler bug. > > Thanks, I've filed a bug now: > > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=64923 > >> I think it's OK to work around this in glibc, but it needs a comment >> with a reference to the filed gcc bug. I do not think it is valid >> for gcc on s390x to use the entire bit field along with padding and >> I believe it could result in incorrect operation. > > Nothing is breaking due to this right now, so we could probably wait > and see what the gcc folks think of this. I would check it into 2.22 and reference the GCC PR. However, I see that GCC thinks this is a valgrind bug. If valgrind is simply looking at the comparison to make the warning then it falls into the 'false positive' category. In which case I think Valgrind should set up an exception for this warning on s390. Cheers, Carlos.
On 02/03/2015 01:14 PM, Carlos O'Donell wrote: > On 02/03/2015 12:05 PM, Siddhesh Poyarekar wrote: >> On Tue, Feb 03, 2015 at 11:49:26AM -0500, Carlos O'Donell wrote: >>> IMO zero-initialized padding, for this case, isn't something you can >>> expect. Therefore I think it's a compiler bug. >> >> Thanks, I've filed a bug now: >> >> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=64923 >> >>> I think it's OK to work around this in glibc, but it needs a comment >>> with a reference to the filed gcc bug. I do not think it is valid >>> for gcc on s390x to use the entire bit field along with padding and >>> I believe it could result in incorrect operation. >> >> Nothing is breaking due to this right now, so we could probably wait >> and see what the gcc folks think of this. > > I would check it into 2.22 and reference the GCC PR. > > However, I see that GCC thinks this is a valgrind bug. > > If valgrind is simply looking at the comparison to make > the warning then it falls into the 'false positive' category. > In which case I think Valgrind should set up an exception for > this warning on s390. To be clear, I think nothing needs to be done now except file an upstream valgrind PR. c.
On Tue, 2015-02-03 at 13:14 -0500, Carlos O'Donell wrote: > On 02/03/2015 01:14 PM, Carlos O'Donell wrote: > > However, I see that GCC thinks this is a valgrind bug. > > > > If valgrind is simply looking at the comparison to make > > the warning then it falls into the 'false positive' category. > > In which case I think Valgrind should set up an exception for > > this warning on s390. > > To be clear, I think nothing needs to be done now except file > an upstream valgrind PR. Yes please: https://bugs.kde.org/enter_bug.cgi?product=valgrind&format=guided This sounds a bit like https://bugs.kde.org/show_bug.cgi?id=308427 "s390 memcheck reports tsearch conditional jump or move depends on uninitialized value". But that bug got fixed some time ago and should not be in the latest version of valgrind. It does contain a nice description of how best to report such an issue. Just run under valgrind with --vgdb-error=0 and then attach with gdb, through target remote | vgdb. Please include the disassembly output from gdb around the code that produces the warning. And if possible use the valgrind gdb monitor commands to figure out which bits valgrind thinks are (un)defined. http://valgrind.org/docs/manual/mc-manual.html#mc-manual.monitor-commands Thanks, Mark
On Wed, Feb 04, 2015 at 12:25:18AM +0100, Mark Wielaard wrote: > This sounds a bit like https://bugs.kde.org/show_bug.cgi?id=308427 "s390 > memcheck reports tsearch conditional jump or move depends on > uninitialized value". But that bug got fixed some time ago and should > not be in the latest version of valgrind. It does contain a nice > description of how best to report such an issue. I've filed: https://bugs.kde.org/show_bug.cgi?id=343802 which is a variant of 308427, which fixes the lg+jhe combination, but not the lt+jl combination which the newer gcc produces. Siddhesh
diff --git a/malloc/obstack.c b/malloc/obstack.c index 5bb3f0d..c1d6ded 100644 --- a/lib/obstack.c +++ b/lib/obstack.c @@ -148,6 +148,8 @@ _obstack_begin (struct obstack *h, { struct _obstack_chunk *chunk; /* points to new chunk */ + memset (h, 0, sizeof (struct obstack)); + if (alignment == 0) alignment = DEFAULT_ALIGNMENT; if (size == 0)