[PATCH] test-skeleton.c: Do not enable M_PERTURB

[PATCH] test-skeleton.c: Do not enable M_PERTURB

[Florian Weimer]

Over all, this decreases the realism of the tests because
it ensures that freshly allocated memory has a well-defined
bit pattern.  It also causes malloc to take internal paths
different from regular application usage, and therefore
reduces malloc test coverage.

2016-06-23  Florian Weimer  <fweimer@redhat.com>

	* test-skeleton.c (main): Do not enable M_PERTURB.

diff --git a/test-skeleton.c b/test-skeleton.c
index d9bf989..e462eee 100644
--- a/test-skeleton.c
+++ b/test-skeleton.c
@@ -346,9 +346,6 @@ main (int argc, char *argv[])
   unsigned int timeoutfactor = 1;
   pid_t termpid;
 
-  /* Make uses of freed and uninitialized memory known.  */
-  mallopt (M_PERTURB, 42);
-
 #ifdef STDOUT_UNBUFFERED
   setbuf (stdout, NULL);
 #endif

Re: [PATCH] test-skeleton.c: Do not enable M_PERTURB

[Siddhesh Poyarekar]

On Thu, Jun 23, 2016 at 04:40:42PM +0200, Florian Weimer wrote:
> Over all, this decreases the realism of the tests because
> it ensures that freshly allocated memory has a well-defined
> bit pattern.  It also causes malloc to take internal paths
> different from regular application usage, and therefore
> reduces malloc test coverage.

The well-defined bit pattern is more likely to catch any bad tests
though, which might make it valuable.  However, I agree that it makes
sense to remove it for malloc tests.

Siddhesh

Re: [PATCH] test-skeleton.c: Do not enable M_PERTURB

[Florian Weimer]

On 06/23/2016 04:49 PM, Siddhesh Poyarekar wrote:
> On Thu, Jun 23, 2016 at 04:40:42PM +0200, Florian Weimer wrote:
>> Over all, this decreases the realism of the tests because
>> it ensures that freshly allocated memory has a well-defined
>> bit pattern.  It also causes malloc to take internal paths
>> different from regular application usage, and therefore
>> reduces malloc test coverage.
>
> The well-defined bit pattern is more likely to catch any bad tests
> though, which might make it valuable.

It could also cover up bugs which would otherwise be visible with fresh 
allocations which contain only zeros.

Ideally, we'd run all tests twice, with different settings from the 
environment (and also with valgrind).

Thanks,
Florian

Re: [PATCH] test-skeleton.c: Do not enable M_PERTURB

[Siddhesh Poyarekar]

On Thu, Jun 23, 2016 at 05:05:33PM +0200, Florian Weimer wrote:
> It could also cover up bugs which would otherwise be visible with fresh
> allocations which contain only zeros.
> 
> Ideally, we'd run all tests twice, with different settings from the
> environment (and also with valgrind).

Yeah, if you were to do that kind of coverage at the distribution
level (i.e. run the tests twice, once with and then without valgrind)
that the change makes sense to make the tests behave as closely as
possible to the real world.

In short, LGTM.

Siddhesh

Re: [PATCH] test-skeleton.c: Do not enable M_PERTURB

[Zack Weinberg]

On Thu, Jun 23, 2016 at 12:00 PM, Siddhesh Poyarekar
<siddhesh@sourceware.org> wrote:
> On Thu, Jun 23, 2016 at 05:05:33PM +0200, Florian Weimer wrote:
>> It could also cover up bugs which would otherwise be visible with fresh
>> allocations which contain only zeros.
>>
>> Ideally, we'd run all tests twice, with different settings from the
>> environment (and also with valgrind).
>
> Yeah, if you were to do that kind of coverage at the distribution
> level (i.e. run the tests twice, once with and then without valgrind)
> that the change makes sense to make the tests behave as closely as
> possible to the real world.

Is there a well-documented way to run the tests under valgrind?

Re: [PATCH] test-skeleton.c: Do not enable M_PERTURB

[Siddhesh Poyarekar]

On Thu, Jun 23, 2016 at 12:57:17PM -0400, Zack Weinberg wrote:
> Is there a well-documented way to run the tests under valgrind?

I don't think so, but it ought to be sufficient to run valgrind
--trace-children=yes for all of make check.  Dead slow, but I don't
see why it shouldn't work.

Maybe this is a good candidate for enhancement in glibc to allow
running individual tests under valgrind, something like
'make valgrind-check'.

Siddhesh

Re: [PATCH] test-skeleton.c: Do not enable M_PERTURB

[Florian Weimer]

On 06/23/2016 07:16 PM, Siddhesh Poyarekar wrote:
> On Thu, Jun 23, 2016 at 12:57:17PM -0400, Zack Weinberg wrote:
>> Is there a well-documented way to run the tests under valgrind?
>
> I don't think so, but it ought to be sufficient to run valgrind
> --trace-children=yes for all of make check.  Dead slow, but I don't
> see why it shouldn't work.
>
> Maybe this is a good candidate for enhancement in glibc to allow
> running individual tests under valgrind, something like
> 'make valgrind-check'.

Running tests under valgrind is one thing.  Interpreting the errors is 
another.

On some architectures, running valgrind on a non-installed libc gives 
rather poor results.  Some versions do not recognize malloc as such, and 
on some architectures, the suppression files are inactive because the 
libc paths do not match the hard-coded expectations of valgrind.

With certain tests, we have the problem that we have memory leaks when 
subprocesses call exit, without freeing data, which is marked as a leak 
in the error log.

Thread stack deallocation is generally racy, and stacks may be reported 
by valgrind as a memory leak.

These are just the issues I have seen so far when running valgrind 
manually.  I expect there are more.

Thanks,
Florian

Re: [PATCH] test-skeleton.c: Do not enable M_PERTURB

[Mike Frysinger]

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

On 23 Jun 2016 19:25, Florian Weimer wrote:
> On 06/23/2016 07:16 PM, Siddhesh Poyarekar wrote:
> > On Thu, Jun 23, 2016 at 12:57:17PM -0400, Zack Weinberg wrote:
> >> Is there a well-documented way to run the tests under valgrind?
> >
> > I don't think so, but it ought to be sufficient to run valgrind
> > --trace-children=yes for all of make check.  Dead slow, but I don't
> > see why it shouldn't work.
> >
> > Maybe this is a good candidate for enhancement in glibc to allow
> > running individual tests under valgrind, something like
> > 'make valgrind-check'.
> 
> Running tests under valgrind is one thing.  Interpreting the errors is 
> another.
> 
> On some architectures, running valgrind on a non-installed libc gives 
> rather poor results.  Some versions do not recognize malloc as such, and 
> on some architectures, the suppression files are inactive because the 
> libc paths do not match the hard-coded expectations of valgrind.

not only that, but valgrind frequently needs to be updated whenever a new
glibc release is made (to tweak internal false positives).  if we tried to
use that against git master, i'm afraid it'd always be broken.
-mike

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH] test-skeleton.c: Do not enable M_PERTURB

[Mike Frysinger]

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

On 23 Jun 2016 17:05, Florian Weimer wrote:
> On 06/23/2016 04:49 PM, Siddhesh Poyarekar wrote:
> > On Thu, Jun 23, 2016 at 04:40:42PM +0200, Florian Weimer wrote:
> >> Over all, this decreases the realism of the tests because
> >> it ensures that freshly allocated memory has a well-defined
> >> bit pattern.  It also causes malloc to take internal paths
> >> different from regular application usage, and therefore
> >> reduces malloc test coverage.
> >
> > The well-defined bit pattern is more likely to catch any bad tests
> > though, which might make it valuable.
> 
> It could also cover up bugs which would otherwise be visible with fresh 
> allocations which contain only zeros.

while certainly true, i think this is much less common of an edge case.
code that happens to work because it happened to get zero-ed memory is,
in my experience, way less common than code that happens to work because
it happened to get garbage initially.
-mike

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH] test-skeleton.c: Do not enable M_PERTURB

[Florian Weimer]

On 06/23/2016 09:31 PM, Mike Frysinger wrote:
> On 23 Jun 2016 17:05, Florian Weimer wrote:
>> On 06/23/2016 04:49 PM, Siddhesh Poyarekar wrote:
>>> On Thu, Jun 23, 2016 at 04:40:42PM +0200, Florian Weimer wrote:
>>>> Over all, this decreases the realism of the tests because
>>>> it ensures that freshly allocated memory has a well-defined
>>>> bit pattern.  It also causes malloc to take internal paths
>>>> different from regular application usage, and therefore
>>>> reduces malloc test coverage.
>>>
>>> The well-defined bit pattern is more likely to catch any bad tests
>>> though, which might make it valuable.
>>
>> It could also cover up bugs which would otherwise be visible with fresh
>> allocations which contain only zeros.
>
> while certainly true, i think this is much less common of an edge case.
> code that happens to work because it happened to get zero-ed memory is,
> in my experience, way less common than code that happens to work because
> it happened to get garbage initially.

Okay, patch withdrawn.

Thanks,
Florian