diff mbox

[(updated)] Convert symtab, cgraph and varpool nodes into a real class hierarchy

Message ID 1383015457.29677.50.camel@surprise
State New
Headers show

Commit Message

David Malcolm Oct. 29, 2013, 2:57 a.m. UTC 
On Fri, 2013-09-20 at 17:33 +0200, Jan Hubicka wrote:
> > This patch is the handwritten part of the conversion of these types
> > to C++; it requires the followup patch, which is autogenerated.
> > 
> > It converts:
> >   struct symtab_node_base
> > to:
> >   class symtab_node_base
> > 
> > and converts:
> >   struct cgraph_node
> > to:
> >   struct cgraph_node : public symtab_node_base
> > and:
> >   struct varpool_node
> > to:
> >   class varpool_node : public symtab_node_base
> > 
> > dropping the symtab_node_def union.
> 
> Yep, incrementally we should continue with the grand renaming 
> retiring symtab_node_base and getting things symmetric.
> > 
> > 	* cgraph.h (symtab_node_base): Convert to a class;
> > 	add GTY((desc ("%h.type"), tag ("SYMTAB_SYMBOL"))).
> > 	(cgraph_node): Inherit from symtab_node; add GTY option
> > 	tag ("SYMTAB_FUNCTION").
> > 	(varpool_node): Inherit from symtab_node; add GTY option
> > 	tag ("SYMTAB_VARIABLE").
> > 	(symtab_node_def): Remove.
> > 	(is_a_helper <cgraph_node>::test (symtab_node_def *)): Convert to...
> > 	(is_a_helper <cgraph_node>::test (symtab_node_base *)): ...this.
> > 	(is_a_helper <varpool_node>::test (symtab_node_def *)): Convert to...
> > 	(is_a_helper <varpool_node>::test (symtab_node_base *)): ...this.
> > 
> > 	* ipa-ref.h (symtab_node_def): Drop.
> > 	(symtab_node): Change underlying type from symtab_node_def to
> > 	symtab_node_base.
> > 	(const_symtab_node): Likwise.
> > 
> > 	* is-a.h: Update examples in comment.
> > 
> > 	* symtab.c (symtab_hash): Change symtab_node_def to symtab_node_base.
> > 	(assembler_name_hash): Likewise.
> 
> This patch is OK.  Thanks for working on this!

These symtab changes were dependent on having gengtype support for
inheritance, which is now in trunk, so I'm now revisiting these patches.

The above patch hasn't bitrotted, though the autogenerated one that goes
with it needed regenerating.

A new version of the autogenerated patch can be seen at:
http://dmalcolm.fedorapeople.org/gcc/large-patches/eaba9669644c84592ea32be2dcd19ba92beca381-0003-Autogenerated-fixes-of-symbol.-to.patch
Is that new patch OK?  (it's 450KB so one's eyes tend to glaze over
after a while, but FWIW you approved an earlier version of that in:
  http://gcc.gnu.org/ml/gcc-patches/2013-09/msg00730.html
and the test suite for the script that generated the patch can be seen
at:
https://github.com/davidmalcolm/gcc-refactoring-scripts/blob/master/test_refactor_symtab.py )

Also, I noticed when reviewing the autogenerated marking routines in
gtype-desc.c that the symtab_node_def union had chain_next/chain_prev
markings, and that my patch above effectively dropped them.

The attached patch reinstates them, albeit to the symtab_node_base base
class, and inspection of the generated gtype-desc.c shows that the
generated code does the right thing.

Is this change also OK for trunk?

(I'm still bootstrapping/regtesting the patch series now, so the above
is all conditional on that, so my apologies if this turns out to be
premature).

Dave

Comments

Jeff Law Oct. 29, 2013, 5:37 a.m. UTC | #1 
On 10/28/13 20:57, David Malcolm wrote:
>>> 	* cgraph.h (symtab_node_base): Convert to a class;
>>> 	add GTY((desc ("%h.type"), tag ("SYMTAB_SYMBOL"))).
>>> 	(cgraph_node): Inherit from symtab_node; add GTY option
>>> 	tag ("SYMTAB_FUNCTION").
>>> 	(varpool_node): Inherit from symtab_node; add GTY option
>>> 	tag ("SYMTAB_VARIABLE").
>>> 	(symtab_node_def): Remove.
>>> 	(is_a_helper <cgraph_node>::test (symtab_node_def *)): Convert to...
>>> 	(is_a_helper <cgraph_node>::test (symtab_node_base *)): ...this.
>>> 	(is_a_helper <varpool_node>::test (symtab_node_def *)): Convert to...
>>> 	(is_a_helper <varpool_node>::test (symtab_node_base *)): ...this.
>>>
>>> 	* ipa-ref.h (symtab_node_def): Drop.
>>> 	(symtab_node): Change underlying type from symtab_node_def to
>>> 	symtab_node_base.
>>> 	(const_symtab_node): Likwise.
>>>
>>> 	* is-a.h: Update examples in comment.
>>>
>>> 	* symtab.c (symtab_hash): Change symtab_node_def to symtab_node_base.
>>> 	(assembler_name_hash): Likewise.
>>
>> This patch is OK.  Thanks for working on this!
>
> These symtab changes were dependent on having gengtype support for
> inheritance, which is now in trunk, so I'm now revisiting these patches.
>
> The above patch hasn't bitrotted, though the autogenerated one that goes
> with it needed regenerating.
>
> A new version of the autogenerated patch can be seen at:
> http://dmalcolm.fedorapeople.org/gcc/large-patches/eaba9669644c84592ea32be2dcd19ba92beca381-0003-Autogenerated-fixes-of-symbol.-to.patch
> Is that new patch OK?  (it's 450KB so one's eyes tend to glaze over
> after a while, but FWIW you approved an earlier version of that in:
>    http://gcc.gnu.org/ml/gcc-patches/2013-09/msg00730.html
> and the test suite for the script that generated the patch can be seen
> at:
> https://github.com/davidmalcolm/gcc-refactoring-scripts/blob/master/test_refactor_symtab.py )
Given it's already approved and hasn't changed in any substantial way, 
it's OK for the trunk once you get through the bootstrap and regression 
testing.

jeff
David Malcolm Oct. 29, 2013, 7:05 p.m. UTC | #2 
On Mon, 2013-10-28 at 23:37 -0600, Jeff Law wrote:
> On 10/28/13 20:57, David Malcolm wrote:
> >>> 	* cgraph.h (symtab_node_base): Convert to a class;
> >>> 	add GTY((desc ("%h.type"), tag ("SYMTAB_SYMBOL"))).
> >>> 	(cgraph_node): Inherit from symtab_node; add GTY option
> >>> 	tag ("SYMTAB_FUNCTION").
> >>> 	(varpool_node): Inherit from symtab_node; add GTY option
> >>> 	tag ("SYMTAB_VARIABLE").
> >>> 	(symtab_node_def): Remove.
> >>> 	(is_a_helper <cgraph_node>::test (symtab_node_def *)): Convert to...
> >>> 	(is_a_helper <cgraph_node>::test (symtab_node_base *)): ...this.
> >>> 	(is_a_helper <varpool_node>::test (symtab_node_def *)): Convert to...
> >>> 	(is_a_helper <varpool_node>::test (symtab_node_base *)): ...this.
> >>>
> >>> 	* ipa-ref.h (symtab_node_def): Drop.
> >>> 	(symtab_node): Change underlying type from symtab_node_def to
> >>> 	symtab_node_base.
> >>> 	(const_symtab_node): Likwise.
> >>>
> >>> 	* is-a.h: Update examples in comment.
> >>>
> >>> 	* symtab.c (symtab_hash): Change symtab_node_def to symtab_node_base.
> >>> 	(assembler_name_hash): Likewise.
> >>
> >> This patch is OK.  Thanks for working on this!
> >
> > These symtab changes were dependent on having gengtype support for
> > inheritance, which is now in trunk, so I'm now revisiting these patches.
> >
> > The above patch hasn't bitrotted, though the autogenerated one that goes
> > with it needed regenerating.
> >
> > A new version of the autogenerated patch can be seen at:
> > http://dmalcolm.fedorapeople.org/gcc/large-patches/eaba9669644c84592ea32be2dcd19ba92beca381-0003-Autogenerated-fixes-of-symbol.-to.patch
> > Is that new patch OK?  (it's 450KB so one's eyes tend to glaze over
> > after a while, but FWIW you approved an earlier version of that in:
> >    http://gcc.gnu.org/ml/gcc-patches/2013-09/msg00730.html
> > and the test suite for the script that generated the patch can be seen
> > at:
> > https://github.com/davidmalcolm/gcc-refactoring-scripts/blob/master/test_refactor_symtab.py )
> Given it's already approved and hasn't changed in any substantial way, 
> it's OK for the trunk once you get through the bootstrap and regression 
> testing.
Thanks.

The conversion of the symtab types from inheritance-in-C to a C++ class
hierarchy is now in trunk: I committed the manual parts of the
conversion as r204170, and the automated part as r204171.  I then
noticed that this broke the gdb debugging hooks for printing a
(cgraph_node *).  I tested and committed the attached fix for them as
r204174.

As a followup, I'm next looking at the renaming of the symtab types
previously discussed with Honza (on 2013-09-10).

Dave
Jan Hubicka Oct. 29, 2013, 7:24 p.m. UTC | #3 
> 
> The conversion of the symtab types from inheritance-in-C to a C++ class
> hierarchy is now in trunk: I committed the manual parts of the
> conversion as r204170, and the automated part as r204171.  I then
> noticed that this broke the gdb debugging hooks for printing a
> (cgraph_node *).  I tested and committed the attached fix for them as
> r204174.
> 
> As a followup, I'm next looking at the renaming of the symtab types
> previously discussed with Honza (on 2013-09-10).

Thank you, David!

Honza
> 
> Dave

> Index: gcc/ChangeLog
> ===================================================================
> --- gcc/ChangeLog	(revision 204173)
> +++ gcc/ChangeLog	(revision 204174)
> @@ -1,3 +1,10 @@
> +2013-10-29  David Malcolm  <dmalcolm@redhat.com>
> +
> +	* gdbhooks.py (CGraphNodePrinter.to_string): Update gdb
> +	prettyprinter for cgraph_node to reflect the conversion of the
> +	symtable types to a C++ class hierarchy: it now *is* a
> +	symtab_node_base, rather than having one (named "symbol").
> +
>  2013-10-29  Balaji V. Iyer  <balaji.v.iyer@intel.com>
>  
>  	* builtins.c (is_builtin_name): Added a check for __cilkrts_detach and
> Index: gcc/gdbhooks.py
> ===================================================================
> --- gcc/gdbhooks.py	(revision 204173)
> +++ gcc/gdbhooks.py	(revision 204174)
> @@ -226,8 +226,7 @@
>              # symtab_node_name calls lang_hooks.decl_printable_name
>              # default implementation (lhd_decl_printable_name) is:
>              #    return IDENTIFIER_POINTER (DECL_NAME (decl));
> -            symbol = self.gdbval['symbol']
> -            tree_decl = Tree(symbol['decl'])
> +            tree_decl = Tree(self.gdbval['decl'])
>              result += ' "%s"' % tree_decl.DECL_NAME().IDENTIFIER_POINTER()
>          result += '>'
>          return result
diff mbox

Patch

From f070f1ecc2baa536d121f2942bd8af750dbc2f02 Mon Sep 17 00:00:00 2001
From: David Malcolm <dmalcolm@redhat.com>
Date: Mon, 28 Oct 2013 22:25:38 -0400
Subject: [PATCH 2/3] Add chain_next and chain_prev options back to
 symtab_node_base

gcc/
	* cgraph.h (symtab_node_base): Add missing chain_next and
	chain_prev GTY options.
---
 gcc/cgraph.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/gcc/cgraph.h b/gcc/cgraph.h
index 5a582d8..35f79ec 100644
--- a/gcc/cgraph.h
+++ b/gcc/cgraph.h
@@ -39,7 +39,7 @@  enum symtab_type
 
 /* Base of all entries in the symbol table.
    The symtab_node is inherited by cgraph and varpol nodes.  */
-class GTY((desc ("%h.type"), tag ("SYMTAB_SYMBOL"))) symtab_node_base
+class GTY((desc ("%h.type"), tag ("SYMTAB_SYMBOL"), chain_next ("%h.next"), chain_prev ("%h.previous"))) symtab_node_base
 {
 public:
   /* Type of the symbol.  */
-- 
1.7.11.7