Re: [PATCH (for next stage 1)] Add return type to gimple function dumps

[David Malcolm <dmalcolm at redhat dot com>]

On Fri, 2014-05-16 at 12:17 -0400, David Malcolm wrote:
> On Fri, 2014-05-16 at 14:59 +0200, Richard Biener wrote:
> > On Tue, Apr 29, 2014 at 5:01 PM, David Malcolm <dmalcolm@redhat.com> wrote:
> > > On Tue, 2014-04-29 at 11:16 +0200, Richard Biener wrote:
> > >> On Tue, Apr 29, 2014 at 2:58 AM, David Malcolm <dmalcolm@redhat.com> wrote:
> > >> > On Thu, 2014-04-24 at 15:46 -0600, Jeff Law wrote:
> > >> >> On 03/10/14 13:22, David Malcolm wrote:
> > >> >> > Gimple function dumps contain the types of parameters, but not of the
> > >> >> > return type.
> > >> >> >
> > >> >> > The attached patch fixes this omission; here's an example of the
> > >> >> > before/after diff:
> > >> >> > $ diff -up /tmp/pr23401.c.004t.gimple.old /tmp/pr23401.c.004t.gimple.new
> > >> >> > --- /tmp/pr23401.c.004t.gimple.old      2014-03-10 13:40:08.972063541 -0400
> > >> >> > +++ /tmp/pr23401.c.004t.gimple.new      2014-03-10 13:39:49.346515464 -0400
> > >> >> > @@ -1,3 +1,4 @@
> > >> >> > +int
> > >> >> >   ffff (int i)
> > >> >> >   {
> > >> >> >     int D.1731;
> > >> >> >
> > >> >> >
> > >> >> > Successfully bootstrapped and regrtested on x86_64 Linux (Fedora 20).
> > >> >> >
> > >> >> > A couple of test cases needed tweaking, since they were counting the
> > >> >> > number of occurrences of "int" in the gimple dump, which thus changed
> > >> >> > for functions returning int (like the one above).
> > >> >> >
> > >> >> > OK for next stage 1?
> > >> >> Conceptually OK.  As Richi notes, the work here is in fixing up the
> > >> >> testsuite.  I didn't see a reply to Richi's question, particularly WRT
> > >> >> the Fortran testsuite.
> > >> >
> > >> > I'm attaching a revised version of the patch which adds the use of
> > >> > TDF_SLIM (though it didn't appear to be necessary in the test I did of a
> > >> > function returning a struct).
> > >> >
> > >> > Successfully bootstrapped & regrtested on x86_64 Linux (Fedora 20),
> > >> > using:
> > >> >   --enable-languages=c,c++,objc,obj-c++,java,fortran,ada,go,lto
> > >> >
> > >> > I didn't see any new failures from this in the testsuite, in particular
> > >> > gfortran.sum.  Here's a comparison of the before/after test results,
> > >> > generated using my "jamais-vu" tool [1], with comments added by me
> > >> > inline:
> > >> >
> > >> > Comparing 16 common .sum files
> > >> > ------------------------------
> > >> >
> > >> >  gcc/testsuite/ada/acats/acats.sum : total: 2320 PASS: 2320
> > >> >  gcc/testsuite/g++/g++.sum : total: 90421 FAIL: 3 PASS: 86969 XFAIL: 445 UNSUPPORTED: 3004
> > >> >  gcc/testsuite/gcc/gcc.sum : total: 110458 FAIL: 45 PASS: 108292 XFAIL: 265 XPASS: 33 UNSUPPORTED: 1823
> > >> >  gcc/testsuite/gfortran/gfortran.sum : total: 45717 PASS: 45600 XFAIL: 52 UNSUPPORTED: 65
> > >> >  gcc/testsuite/gnat/gnat.sum : total: 1255 PASS: 1234 XFAIL: 18 UNSUPPORTED: 3
> > >> >  gcc/testsuite/go/go.sum : total: 7266 PASS: 7258 XFAIL: 1 UNTESTED: 6 UNSUPPORTED: 1
> > >> >  gcc/testsuite/obj-c++/obj-c++.sum : total: 1450 PASS: 1354 XFAIL: 10 UNSUPPORTED: 86
> > >> >  gcc/testsuite/objc/objc.sum : total: 2973 PASS: 2893 XFAIL: 6 UNSUPPORTED: 74
> > >> >  x86_64-unknown-linux-gnu/boehm-gc/testsuite/boehm-gc.sum : total: 13 PASS: 12 UNSUPPORTED: 1
> > >> >  x86_64-unknown-linux-gnu/libatomic/testsuite/libatomic.sum : total: 54 PASS: 54
> > >> >  x86_64-unknown-linux-gnu/libffi/testsuite/libffi.sum : total: 1856 PASS: 1801 UNSUPPORTED: 55
> > >> >  x86_64-unknown-linux-gnu/libgo/libgo.sum : total: 122 PASS: 122
> > >> >  x86_64-unknown-linux-gnu/libgomp/testsuite/libgomp.sum : total: 2420 PASS: 2420
> > >> >  x86_64-unknown-linux-gnu/libitm/testsuite/libitm.sum : total: 30 PASS: 26 XFAIL: 3 UNSUPPORTED: 1
> > >> >  x86_64-unknown-linux-gnu/libjava/testsuite/libjava.sum : total: 2586 PASS: 2582 XFAIL: 4
> > >> >  x86_64-unknown-linux-gnu/libstdc++-v3/testsuite/libstdc++.sum : total: 10265 PASS: 10000 XFAIL: 41 UNSUPPORTED: 224
> > >> >
> > >> > (...i.e. the totals were unchanged between unpatched/patched for all of
> > >> > the .sum files; and yes, Fortran was tested.  Should there be a
> > >> > gcj.sum?)
> > >> >
> > >> > Tests that went away in gcc/testsuite/gcc/gcc.sum: 2
> > >> > ----------------------------------------------------
> > >> >
> > >> >  PASS: gcc.dg/tree-ssa/pr23401.c scan-tree-dump-times gimple "int" 5
> > >> >  PASS: gcc.dg/tree-ssa/pr27810.c scan-tree-dump-times gimple "int" 3
> > >> >
> > >> > Tests appeared in gcc/testsuite/gcc/gcc.sum: 2
> > >> > ----------------------------------------------
> > >> >
> > >> >  PASS: gcc.dg/tree-ssa/pr23401.c scan-tree-dump-times gimple "int" 6
> > >> >  PASS: gcc.dg/tree-ssa/pr27810.c scan-tree-dump-times gimple "int" 4
> > >> >
> > >> >
> > >> > (...my comparison tool isn't smart enough yet to tie these "went
> > >> > away"/"appeared" results together; they reflect the fixups from the
> > >> > patch).
> > >> >
> > >> > Tests that went away in gcc/testsuite/go/go.sum: 2
> > >> > --------------------------------------------------
> > >> >
> > >> >  PASS: go.test/test/dwarf/dwarf.dir/main.go (lots of refs to path of build) compilation,  -O2 -g
> > >> >  PASS: go.test/test/dwarf/dwarf.dir/main.go (lots of refs to path of build) execution,  -O2 -g
> > >> >
> > >> > Tests appeared in gcc/testsuite/go/go.sum: 2
> > >> > --------------------------------------------
> > >> >
> > >> >  PASS: go.test/test/dwarf/dwarf.dir/main.go (lots of refs to path of build) compilation,  -O2 -g
> > >> >  PASS: go.test/test/dwarf/dwarf.dir/main.go (lots of refs to path of build) execution,  -O2 -g
> > >> >
> > >> > (...I hand edited the above, this main.go test embeds numerous paths,
> > >> > which change between the two builds; so nothing really changed here).
> > >> >
> > >> >
> > >> > Are the above results sane?
> > >>
> > >> Yes.
> > >>
> > >> > I'm not sure why I didn't see the failures Richi described; the patch
> > >> > does appear to work (though again, should there be a gcj.sum? Did I miss
> > >> > any frontends?)
> > >>
> > >> Maybe I dumped
> > >>
> > >> int foo (...
> > >>
> > >> vs. your
> > >>
> > >> int
> > >> foo (...
> > >>
> > >> and that made the difference.
> > >>
> > >> > OK for trunk?
> > >>
> > >> Ok.
> > >
> > > Thanks; committed to trunk as r209902.
> > 
> > Btw, I now see
> > 
> > int
> > int integer_zerop(const_tree) (const union tree_node * expr)
> > {
> >   union tree_node * D.86619;
> >   union tree_node * exp;
> > 
> > thus duplicated return type.
> > 
> > So - can you either revert or fix that?
> > 
> > Thanks,
> > Richard.
> 
> Sorry about this.
> 
> I dug into the decl-related dump output:
> 
>   int
>   int integer_zerop(const_tree) (const union tree_node * expr)
> 
> to see where each component comes from:
> 
>   int
>   ^^^ from r209902
>   int integer_zerop(const_tree) (const union tree_node * expr)
>   ^^^^^^^function_name ()^^^^^^ ^^^dump_function_to_file ()^^^
> 
> Clearly I didn't test enough with C++; what I committed works with C,
> but function_name calls fndecl_name which calls
> lang_hooks.decl_printable_name with verbosity 2.
> 
> The C++ langhook for decl_printable_name includes the return type at
> v=2, whereas the default lhd_decl_printable_name merely returns
>   IDENTIFIER_POINTER (DECL_NAME (decl))
> 
> hence the duplicated return type for C++ (and possibly java), and single
> copy of the return type for C.
> 
> Note also that the C++ langhook at v=2 also includes the parameter
> types, which is why the C++ dump contains the param types twice; i.e.:
> 
>   int
>   int integer_zerop(const_tree) (const union tree_node * expr)
>                    ^^^^types^^^ ^^^^^^types and names^^^^^^^^^
> 
> Is the duplication of the parameter signature regarded as an issue [1],
> or expected behavior given that language's support for overloading?  (I
> see this duplication with e.g. 4.8, fwiw).

In the meantime, I've reverted this, as r210533.

> Dave
> 
> [1] if so, perhaps some kind of tweaking of the verbosity used when
> calling decl_printable_name from dump_function_to_file could fix this?
> though I imagine the redundant info can make debugging easier