| Message ID | 1468007197-11819-1-git-send-email-dmalcolm@redhat.com |
|---|---|
| State | New |
| Headers | show |
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 :). 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 >
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 > >
On 07/08/2016 01:46 PM, David Malcolm 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? > > [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. OK. jeff
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 ();
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? [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(+)