diff mbox series

[v3,02/10] Add mcheck tests to malloc

Message ID 20210702023546.3081774-3-siddhesh@sourceware.org
State Committed
Commit 784fff6ea553da551b6a4989c94c66a69c43201d
Headers show
Series Remove malloc hooks | expand

Checks

Context Check Description
dj/TryBot-apply_patch success Patch applied to master at the time it was sent

Commit Message

Siddhesh Poyarekar July 2, 2021, 2:35 a.m. UTC
Like malloc-check, add generic rules to run all tests in malloc by
linking with libmcheck.a so as to provide coverage for mcheck().
Currently the following 12 tests fail:

FAIL: malloc/tst-malloc-backtrace-mcheck
FAIL: malloc/tst-malloc-fork-deadlock-mcheck
FAIL: malloc/tst-malloc-stats-cancellation-mcheck
FAIL: malloc/tst-malloc-tcache-leak-mcheck
FAIL: malloc/tst-malloc-thread-exit-mcheck
FAIL: malloc/tst-malloc-thread-fail-mcheck
FAIL: malloc/tst-malloc-usable-static-mcheck
FAIL: malloc/tst-malloc-usable-static-tunables-mcheck
FAIL: malloc/tst-malloc-usable-tunables-mcheck
FAIL: malloc/tst-malloc_info-mcheck
FAIL: malloc/tst-memalign-mcheck
FAIL: malloc/tst-posix_memalign-mcheck

and they have been added to tests-exclude-mcheck for now to keep
status quo.  At least the last two can be attributed to bugs in
mcheck() but I haven't fixed them here since they should be fixed by
removing malloc hooks.  Others need to be triaged to check if they're
due to mcheck bugs or due to actual bugs.

Reviewed-by: DJ Delorie <dj@redhat.com>
---
 Rules           | 15 ++++++++++++++-
 malloc/Makefile | 35 +++++++++++++++++++++++++++++++++++
 2 files changed, 49 insertions(+), 1 deletion(-)

Comments

Stefan Liebler July 6, 2021, 2:50 p.m. UTC | #1
On 02/07/2021 04:35, Siddhesh Poyarekar via Libc-alpha wrote:
> Like malloc-check, add generic rules to run all tests in malloc by
> linking with libmcheck.a so as to provide coverage for mcheck().
> Currently the following 12 tests fail:
> 
> FAIL: malloc/tst-malloc-backtrace-mcheck
> FAIL: malloc/tst-malloc-fork-deadlock-mcheck
> FAIL: malloc/tst-malloc-stats-cancellation-mcheck
> FAIL: malloc/tst-malloc-tcache-leak-mcheck
> FAIL: malloc/tst-malloc-thread-exit-mcheck
> FAIL: malloc/tst-malloc-thread-fail-mcheck
> FAIL: malloc/tst-malloc-usable-static-mcheck
> FAIL: malloc/tst-malloc-usable-static-tunables-mcheck
> FAIL: malloc/tst-malloc-usable-tunables-mcheck
> FAIL: malloc/tst-malloc_info-mcheck
> FAIL: malloc/tst-memalign-mcheck
> FAIL: malloc/tst-posix_memalign-mcheck
> 
> and they have been added to tests-exclude-mcheck for now to keep
> status quo.  At least the last two can be attributed to bugs in
> mcheck() but I haven't fixed them here since they should be fixed by
> removing malloc hooks.  Others need to be triaged to check if they're
> due to mcheck bugs or due to actual bugs.
> 
> Reviewed-by: DJ Delorie <dj@redhat.com>

Hi Siddhesh,

starting with this commit, malloc/tst-realloc-mcheck fails if build with
gcc 7.5 on s390x:
Error: realloc (NULL, 0) returned NULL.

With gcc 7.5:
 p = realloc (NULL, 0);
realloc is really called, which is using reallochook, which always
returns NULL:
static void *
reallochook (void *ptr, size_t size, const void *caller)
{
  if (size == 0)
    {
      freehook (ptr, caller);
      return NULL;
    }

With e.g. gcc 9.1:
malloc(0) is called, which is using mallochook. It is using
hdr = (struct hdr *) malloc (sizeof (struct hdr) + size + 1);
...
return (void *) (hdr + 1);
=> != NULL

Can you please have a look? And perhaps also filter out this test?

Bye,
Stefan
Siddhesh Poyarekar July 6, 2021, 3:54 p.m. UTC | #2
On 7/6/21 8:20 PM, Stefan Liebler via Libc-alpha wrote:
> Hi Siddhesh,
> 
> starting with this commit, malloc/tst-realloc-mcheck fails if build with
> gcc 7.5 on s390x:
> Error: realloc (NULL, 0) returned NULL.
> 
> With gcc 7.5:
>   p = realloc (NULL, 0);
> realloc is really called, which is using reallochook, which always
> returns NULL:
> static void *
> reallochook (void *ptr, size_t size, const void *caller)
> {
>    if (size == 0)
>      {
>        freehook (ptr, caller);
>        return NULL;
>      }
> 
> With e.g. gcc 9.1:
> malloc(0) is called, which is using mallochook. It is using
> hdr = (struct hdr *) malloc (sizeof (struct hdr) + size + 1);
> ...
> return (void *) (hdr + 1);
> => != NULL
> 
> Can you please have a look? And perhaps also filter out this test?

Thanks, that is an mcheck bug that gcc seems to have masked.  I'll push 
a patch filter it out.  I'll also need to update my hooks patch to fix 
this bug in mcheck.

Thanks,
Siddhesh
H.J. Lu July 8, 2021, 11:37 p.m. UTC | #3
On Tue, Jul 6, 2021 at 8:54 AM Siddhesh Poyarekar <siddhesh@gotplt.org> wrote:
>
> On 7/6/21 8:20 PM, Stefan Liebler via Libc-alpha wrote:
> > Hi Siddhesh,
> >
> > starting with this commit, malloc/tst-realloc-mcheck fails if build with
> > gcc 7.5 on s390x:
> > Error: realloc (NULL, 0) returned NULL.
> >
> > With gcc 7.5:
> >   p = realloc (NULL, 0);
> > realloc is really called, which is using reallochook, which always
> > returns NULL:
> > static void *
> > reallochook (void *ptr, size_t size, const void *caller)
> > {
> >    if (size == 0)
> >      {
> >        freehook (ptr, caller);
> >        return NULL;
> >      }
> >
> > With e.g. gcc 9.1:
> > malloc(0) is called, which is using mallochook. It is using
> > hdr = (struct hdr *) malloc (sizeof (struct hdr) + size + 1);
> > ...
> > return (void *) (hdr + 1);
> > => != NULL
> >
> > Can you please have a look? And perhaps also filter out this test?
>
> Thanks, that is an mcheck bug that gcc seems to have masked.  I'll push
> a patch filter it out.  I'll also need to update my hooks patch to fix
> this bug in mcheck.
>
> Thanks,
> Siddhesh

It also caused:

https://sourceware.org/bugzilla/show_bug.cgi?id=28068

on x32.
H.J. Lu July 9, 2021, 12:06 a.m. UTC | #4
On Thu, Jul 8, 2021 at 4:37 PM H.J. Lu <hjl.tools@gmail.com> wrote:
>
> On Tue, Jul 6, 2021 at 8:54 AM Siddhesh Poyarekar <siddhesh@gotplt.org> wrote:
> >
> > On 7/6/21 8:20 PM, Stefan Liebler via Libc-alpha wrote:
> > > Hi Siddhesh,
> > >
> > > starting with this commit, malloc/tst-realloc-mcheck fails if build with
> > > gcc 7.5 on s390x:
> > > Error: realloc (NULL, 0) returned NULL.
> > >
> > > With gcc 7.5:
> > >   p = realloc (NULL, 0);
> > > realloc is really called, which is using reallochook, which always
> > > returns NULL:
> > > static void *
> > > reallochook (void *ptr, size_t size, const void *caller)
> > > {
> > >    if (size == 0)
> > >      {
> > >        freehook (ptr, caller);
> > >        return NULL;
> > >      }
> > >
> > > With e.g. gcc 9.1:
> > > malloc(0) is called, which is using mallochook. It is using
> > > hdr = (struct hdr *) malloc (sizeof (struct hdr) + size + 1);
> > > ...
> > > return (void *) (hdr + 1);
> > > => != NULL
> > >
> > > Can you please have a look? And perhaps also filter out this test?
> >
> > Thanks, that is an mcheck bug that gcc seems to have masked.  I'll push
> > a patch filter it out.  I'll also need to update my hooks patch to fix
> > this bug in mcheck.
> >
> > Thanks,
> > Siddhesh
>
> It also caused:
>
> https://sourceware.org/bugzilla/show_bug.cgi?id=28068
>
> on x32.

mcheck.c doesn't take MALLOC_ALIGNMENT into account.   Will it be removed
or fixed?
Siddhesh Poyarekar July 9, 2021, 1:46 a.m. UTC | #5
On 7/9/21 5:36 AM, H.J. Lu wrote:
>> It also caused:
>>
>> https://sourceware.org/bugzilla/show_bug.cgi?id=28068
>>
>> on x32.
> 
> mcheck.c doesn't take MALLOC_ALIGNMENT into account.   Will it be removed
> or fixed?

Very interesting, I think it's best to exclude the test for mcheck on 
x32 for now.  I'll fix mcheck once the malloc hooks patchset[1] is in.

On a separate note, this test probably deserves to be run on all 
architectures for MALLOC_ALIGNMENT.

Siddhesh
Matheus Castanho July 12, 2021, 6:02 p.m. UTC | #6
Siddhesh Poyarekar <siddhesh@gotplt.org> writes:

> On 7/9/21 5:36 AM, H.J. Lu wrote:
>>> It also caused:
>>>
>>> https://sourceware.org/bugzilla/show_bug.cgi?id=28068
>>>
>>> on x32.
>> mcheck.c doesn't take MALLOC_ALIGNMENT into account.   Will it be removed
>> or fixed?
>
> Very interesting, I think it's best to exclude the test for mcheck on x32 for
> now.  I'll fix mcheck once the malloc hooks patchset[1] is in.
>

I just noticed this test is also failing on powerpc32. Similar to
x32. Does it make sense to exclude it there too?

--
Matheus Castanho
Siddhesh Poyarekar July 13, 2021, 1:06 a.m. UTC | #7
On 7/12/21 11:32 PM, Matheus Castanho wrote:
> I just noticed this test is also failing on powerpc32. Similar to
> x32. Does it make sense to exclude it there too?

I just acked HJ's patch that fixes this.

Siddhesh
Matheus Castanho July 13, 2021, 12:11 p.m. UTC | #8
Siddhesh Poyarekar <siddhesh@gotplt.org> writes:

> On 7/12/21 11:32 PM, Matheus Castanho wrote:
>> I just noticed this test is also failing on powerpc32. Similar to
>> x32. Does it make sense to exclude it there too?
>
> I just acked HJ's patch that fixes this.
>
> Siddhesh

Nice, I just saw it. I confirm it fixes the problem on ppc32 as
well. Thanks!

--
Matheus Castanho
diff mbox series

Patch

diff --git a/Rules b/Rules
index c6b635c3f7..ba13598df6 100644
--- a/Rules
+++ b/Rules
@@ -155,6 +155,7 @@  xtests: tests $(xtests-special)
 else
 tests: $(tests:%=$(objpfx)%.out) $(tests-internal:%=$(objpfx)%.out) \
        $(tests-container:%=$(objpfx)%.out) \
+       $(tests-mcheck:%=$(objpfx)%-mcheck.out) \
        $(tests-malloc-check:%=$(objpfx)%-malloc-check.out) \
        $(tests-special) $(tests-printers-out)
 xtests: tests $(xtests:%=$(objpfx)%.out) $(xtests-special)
@@ -166,7 +167,8 @@  ifeq ($(run-built-tests),no)
 tests-expected =
 else
 tests-expected = $(tests) $(tests-internal) $(tests-printers) \
-	$(tests-container) $(tests-malloc-check:%=%-malloc-check)
+	$(tests-container) $(tests-malloc-check:%=%-malloc-check) \
+	$(tests-mcheck:%=%-mcheck)
 endif
 tests:
 	$(..)scripts/merge-test-results.sh -s $(objpfx) $(subdir) \
@@ -192,6 +194,7 @@  else
 binaries-pie-tests =
 binaries-pie-notests =
 endif
+binaries-mcheck-tests = $(tests-mcheck:%=%-mcheck)
 binaries-malloc-check-tests = $(tests-malloc-check:%=%-malloc-check)
 else
 binaries-all-notests =
@@ -202,6 +205,7 @@  binaries-static-tests =
 binaries-static =
 binaries-pie-tests =
 binaries-pie-notests =
+binaries-mcheck-tests =
 binaries-malloc-check-tests =
 endif
 
@@ -226,6 +230,15 @@  $(addprefix $(objpfx),$(binaries-shared-tests)): %: %.o \
 	$(+link-tests)
 endif
 
+ifneq "$(strip $(binaries-mcheck-tests))" ""
+$(addprefix $(objpfx),$(binaries-mcheck-tests)): %-mcheck: %.o \
+  $(link-extra-libs-tests) \
+  $(sort $(filter $(common-objpfx)lib%,$(link-libc))) \
+  $(common-objpfx)malloc/libmcheck.a \
+  $(addprefix $(csu-objpfx),start.o) $(+preinit) $(+postinit)
+	$(+link-tests)
+endif
+
 ifneq "$(strip $(binaries-malloc-check-tests))" ""
 $(addprefix $(objpfx),$(binaries-malloc-check-tests)): %-malloc-check: %.o \
   $(link-extra-libs-tests) \
diff --git a/malloc/Makefile b/malloc/Makefile
index 9bc2e50a9a..c8256abbbf 100644
--- a/malloc/Makefile
+++ b/malloc/Makefile
@@ -77,6 +77,26 @@  tests-exclude-malloc-check = tst-malloc-check tst-malloc-usable \
 # Run all tests with MALLOC_CHECK_=3
 tests-malloc-check = $(filter-out $(tests-exclude-malloc-check),$(tests))
 
+# Tests that don't play well with mcheck.  They are either bugs in mcheck or
+# the tests expect specific internal behavior that is changed due to linking to
+# libmcheck.a.
+tests-exclude-mcheck = tst-mallocstate \
+	tst-safe-linking tst-malloc-usable \
+	tst-malloc-backtrace \
+	tst-malloc-fork-deadlock \
+	tst-malloc-stats-cancellation \
+	tst-malloc-tcache-leak \
+	tst-malloc-thread-exit \
+	tst-malloc-thread-fail \
+	tst-malloc-usable-static \
+	tst-malloc-usable-static-tunables \
+	tst-malloc-usable-tunables \
+	tst-malloc_info \
+	tst-memalign \
+	tst-posix_memalign
+
+tests-mcheck = $(filter-out $(tests-exclude-mcheck), $(tests))
+
 routines = malloc morecore mcheck mtrace obstack reallocarray \
   scratch_buffer_dupfree \
   scratch_buffer_grow scratch_buffer_grow_preserve \
@@ -118,6 +138,11 @@  $(objpfx)tst-mallocfork3: $(shared-thread-library)
 $(objpfx)tst-mallocfork3-mcheck: $(shared-thread-library)
 $(objpfx)tst-malloc-fork-deadlock: $(shared-thread-library)
 $(objpfx)tst-malloc-stats-cancellation: $(shared-thread-library)
+$(objpfx)tst-malloc-backtrace-mcheck: $(shared-thread-library)
+$(objpfx)tst-malloc-thread-exit-mcheck: $(shared-thread-library)
+$(objpfx)tst-malloc-thread-fail-mcheck: $(shared-thread-library)
+$(objpfx)tst-malloc-fork-deadlock-mcheck: $(shared-thread-library)
+$(objpfx)tst-malloc-stats-cancellation-mcheck: $(shared-thread-library)
 $(objpfx)tst-malloc-backtrace-malloc-check: $(shared-thread-library)
 $(objpfx)tst-malloc-thread-exit-malloc-check: $(shared-thread-library)
 $(objpfx)tst-malloc-thread-fail-malloc-check: $(shared-thread-library)
@@ -253,17 +278,24 @@  $(foreach o,$(all-object-suffixes),$(objpfx)malloc$(o)): arena.c hooks.c
 $(tests:%=$(objpfx)%.o): CPPFLAGS += -DTEST_NO_MALLOPT
 
 $(objpfx)tst-interpose-nothread: $(objpfx)tst-interpose-aux-nothread.o
+$(objpfx)tst-interpose-nothread-mcheck: $(objpfx)tst-interpose-aux-nothread.o
 $(objpfx)tst-interpose-nothread-malloc-check: \
 	$(objpfx)tst-interpose-aux-nothread.o
 $(objpfx)tst-interpose-thread: \
   $(objpfx)tst-interpose-aux-thread.o $(shared-thread-library)
+$(objpfx)tst-interpose-thread-mcheck: \
+  $(objpfx)tst-interpose-aux-thread.o $(shared-thread-library)
 $(objpfx)tst-interpose-thread-malloc-check: \
   $(objpfx)tst-interpose-aux-thread.o $(shared-thread-library)
 $(objpfx)tst-interpose-static-nothread: $(objpfx)tst-interpose-aux-nothread.o
+$(objpfx)tst-interpose-static-nothread-mcheck: \
+	$(objpfx)tst-interpose-aux-nothread.o
 $(objpfx)tst-interpose-static-nothread-malloc-check: \
 	$(objpfx)tst-interpose-aux-nothread.o
 $(objpfx)tst-interpose-static-thread: \
   $(objpfx)tst-interpose-aux-thread.o $(static-thread-library)
+$(objpfx)tst-interpose-static-thread-mcheck: \
+  $(objpfx)tst-interpose-aux-thread.o $(static-thread-library)
 $(objpfx)tst-interpose-static-thread-malloc-check: \
   $(objpfx)tst-interpose-aux-thread.o $(static-thread-library)
 
@@ -280,6 +312,9 @@  $(objpfx)tst-dynarray-fail-mem.out: $(objpfx)tst-dynarray-fail.out
 $(objpfx)tst-malloc-tcache-leak: $(shared-thread-library)
 $(objpfx)tst-malloc_info: $(shared-thread-library)
 $(objpfx)tst-mallocfork2: $(shared-thread-library)
+$(objpfx)tst-malloc-tcache-leak-mcheck: $(shared-thread-library)
+$(objpfx)tst-malloc_info-mcheck: $(shared-thread-library)
+$(objpfx)tst-mallocfork2-mcheck: $(shared-thread-library)
 $(objpfx)tst-malloc-tcache-leak-malloc-check: $(shared-thread-library)
 $(objpfx)tst-malloc_info-malloc-check: $(shared-thread-library)
 $(objpfx)tst-mallocfork2-malloc-check: $(shared-thread-library)