This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: [PATCH] Support running the selftests under valgrind
- From: David Malcolm <dmalcolm at redhat dot com>
- To: Andrew Pinski <pinskia at gmail dot com>
- Cc: GCC Patches <gcc-patches at gcc dot gnu dot org>
- Date: Sat, 09 Jul 2016 09:21:39 -0400
- Subject: Re: [PATCH] Support running the selftests under valgrind
- Authentication-results: sourceware.org; auth=none
- References: <1468007197-11819-1-git-send-email-dmalcolm@redhat.com> <CA+=Sn1mtF_sdEk-r1JSvV=hFiKsfQSgOEh0B6hiGVd-ThCuaYA@mail.gmail.com>
On Fri, 2016-07-08 at 22:55 -0700, Andrew Pinski wrote:
> On Fri, Jul 8, 2016 at 12:46 PM, David Malcolm <dmalcolm@redhat.com>
> wrote:
> > This patch adds a new phony target to gcc/Makefile.in to make it
> > easy
> > to run the selftests under valgrind, via "make selftest-valgrind".
> > This phony target isn't a dependency of anything; it's purely for
> > convenience (it takes about 4-5 seconds on my box).
> >
> > Doing so uncovered a few leaks in the selftest suite, which the
> > patch also fixes, so that it runs cleanly under valgrind (on
> > x86_64-pc-linux-gnu, configured with --enable-valgrind-annotations,
> > at least).
> >
> > Successfully bootstrapped®rtested on x86_64-pc-linux-gnu.
> > Manually verified that the valgrind output is "clean" on
> > x86_64-pc-linux-gnu [1].
> >
> > OK for trunk?
>
> I think this is a good idea. I assume this not turned on by default.
> valgrind is still not fully working on aarch64 :).
Correct; that's what I was trying to express by saying it isn't a
dependency of anything. I wouldn't want to make that a requirement.
FWIW, it's very useful when writing new selftests; I was using it when
writing the string literal location patch I just posted. It caught a
memory leak when tracking string concatenation, for the case of
concatenating a string that's fully <= LINE_MAP_MAX_LOCATION_WITH_COLS
with a string that is at least partly beyond that boundary. This leak
would have been hard to find using our traditional test approach. (the
version of the string literal patch that I posted is clean under "make
selftest-valgrind").
> Thanks,
> Andrew
>
> >
> > [1]:
> >
> > HEAP SUMMARY:
> > in use at exit: 1,203,983 bytes in 2,114 blocks
> > total heap usage: 4,545 allocs, 2,431 frees, 3,212,841 bytes
> > allocated
> >
> > LEAK SUMMARY:
> > definitely lost: 0 bytes in 0 blocks
> > indirectly lost: 0 bytes in 0 blocks
> > possibly lost: 0 bytes in 0 blocks
> > still reachable: 1,203,983 bytes in 2,114 blocks
> > suppressed: 0 bytes in 0 blocks
> > Reachable blocks (those to which a pointer was found) are not
> > shown.
> > To see them, rerun with: --leak-check=full --show-leak-kinds=all
> >
> > For counts of detected and suppressed errors, rerun with: -v
> > ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 2 from 2)
> >
> > gcc/ChangeLog:
> > * Makefile.in (selftest-valgrind): New phony target.
> > * function-tests.c (selftest::build_cfg): Delete pass
> > instances
> > created by the test.
> > (selftest::convert_to_ssa): Likewise.
> > (selftest::test_expansion_to_rtl): Likewise.
> > * tree-cfg.c (selftest::test_linear_chain): Release
> > dominator
> > vectors.
> > (selftest::test_diamond): Likewise.
> > ---
> > gcc/Makefile.in | 6 ++++++
> > gcc/function-tests.c | 4 ++++
> > gcc/tree-cfg.c | 6 ++++++
> > 3 files changed, 16 insertions(+)
> >
> > diff --git a/gcc/Makefile.in b/gcc/Makefile.in
> > index 5e7422d..1a4b5d7 100644
> > --- a/gcc/Makefile.in
> > +++ b/gcc/Makefile.in
> > @@ -1869,6 +1869,12 @@ s-selftest: $(GCC_PASSES) cc1$(exeext) stmp
> > -int-hdrs
> > selftest-gdb: $(GCC_PASSES) cc1$(exeext) stmp-int-hdrs
> > $(GCC_FOR_TARGET) -xc -S -c /dev/null -fself-test -wrapper
> > gdb,--args
> >
> > +# Convenience method for running selftests under valgrind:
> > +.PHONY: selftest-valgrind
> > +selftest-valgrind: $(GCC_PASSES) cc1$(exeext) stmp-int-hdrs
> > + $(GCC_FOR_TARGET) -xc -S -c /dev/null -fself-test \
> > + -wrapper valgrind,--leak-check=full
> > +
> > # Recompile all the language-independent object files.
> > # This is used only if the user explicitly asks for it.
> > compilations: $(BACKEND)
> > diff --git a/gcc/function-tests.c b/gcc/function-tests.c
> > index c8188e7..edd355f 100644
> > --- a/gcc/function-tests.c
> > +++ b/gcc/function-tests.c
> > @@ -296,6 +296,7 @@ build_cfg (tree fndecl)
> > push_cfun (fun);
> > lower_cf_pass->execute (fun);
> > pop_cfun ();
> > + delete lower_cf_pass;
> >
> > /* We can now convert to CFG form; for our trivial test function
> > this
> > gives us:
> > @@ -310,6 +311,7 @@ build_cfg (tree fndecl)
> > push_cfun (fun);
> > build_cfg_pass->execute (fun);
> > pop_cfun ();
> > + delete build_cfg_pass;
> > }
> >
> > /* Convert a gimple+CFG function to SSA form. */
> > @@ -325,6 +327,7 @@ convert_to_ssa (tree fndecl)
> > push_cfun (fun);
> > build_ssa_pass->execute (fun);
> > pop_cfun ();
> > + delete build_ssa_pass;
> > }
> >
> > /* Assuming we have a simple 3-block CFG like this:
> > @@ -594,6 +597,7 @@ test_expansion_to_rtl ()
> > init_function_start (fndecl);
> > expand_pass->execute (fun);
> > pop_cfun ();
> > + delete expand_pass;
> >
> > /* On x86_64, I get this:
> > (note 3 1 2 2 [bb 2] NOTE_INSN_BASIC_BLOCK)
> > diff --git a/gcc/tree-cfg.c b/gcc/tree-cfg.c
> > index 0fac49c..6d69435 100644
> > --- a/gcc/tree-cfg.c
> > +++ b/gcc/tree-cfg.c
> > @@ -9276,6 +9276,7 @@ test_linear_chain ()
> > ASSERT_EQ (1, dom_by_b.length ());
> > ASSERT_EQ (bb_c, dom_by_b[0]);
> > free_dominance_info (CDI_DOMINATORS);
> > + dom_by_b.release ();
> >
> > /* Similarly for post-dominance: each BB in our chain is post
> > -dominated
> > by the one after it. */
> > @@ -9286,6 +9287,7 @@ test_linear_chain ()
> > ASSERT_EQ (1, postdom_by_b.length ());
> > ASSERT_EQ (bb_a, postdom_by_b[0]);
> > free_dominance_info (CDI_POST_DOMINATORS);
> > + postdom_by_b.release ();
> >
> > pop_cfun ();
> > }
> > @@ -9346,8 +9348,10 @@ test_diamond ()
> > ASSERT_EQ (bb_a, get_immediate_dominator (CDI_DOMINATORS,
> > bb_d));
> > vec<basic_block> dom_by_a = get_dominated_by (CDI_DOMINATORS,
> > bb_a);
> > ASSERT_EQ (3, dom_by_a.length ()); /* B, C, D, in some order.
> > */
> > + dom_by_a.release ();
> > vec<basic_block> dom_by_b = get_dominated_by (CDI_DOMINATORS,
> > bb_b);
> > ASSERT_EQ (0, dom_by_b.length ());
> > + dom_by_b.release ();
> > free_dominance_info (CDI_DOMINATORS);
> >
> > /* Similarly for post-dominance. */
> > @@ -9357,8 +9361,10 @@ test_diamond ()
> > ASSERT_EQ (bb_d, get_immediate_dominator (CDI_POST_DOMINATORS,
> > bb_c));
> > vec<basic_block> postdom_by_d = get_dominated_by
> > (CDI_POST_DOMINATORS, bb_d);
> > ASSERT_EQ (3, postdom_by_d.length ()); /* A, B, C in some order.
> > */
> > + postdom_by_d.release ();
> > vec<basic_block> postdom_by_b = get_dominated_by
> > (CDI_POST_DOMINATORS, bb_b);
> > ASSERT_EQ (0, postdom_by_b.length ());
> > + postdom_by_b.release ();
> > free_dominance_info (CDI_POST_DOMINATORS);
> >
> > pop_cfun ();
> > --
> > 1.8.5.3
> >