Re: [PATCH/Merge Request] Vtable Verification feature.

[David Malcolm <dmalcolm at redhat dot com>]

On Mon, 2013-08-05 at 15:24 -0700, Diego Novillo wrote:
> This looks almost ready to commit.  Some comments below:

[...]

> > +/* Definition of this optimization pass.  */
> > +
> > +struct gimple_opt_pass pass_vtable_verify =
> > +{
> > + {
> > +  GIMPLE_PASS,
> > +  "vtable-verify",                      /* name */
> > +  OPTGROUP_NONE,                        /* optinfo_flags */
> > +  gate_tree_vtable_verify,              /* gate */
> > +  vtable_verify_main,                   /* execute */
> > +  NULL,                                 /* sub */
> > +  NULL,                                 /* next */
> > +  0,                                    /* static_pass_number */
> > +  TV_VTABLE_VERIFICATION,               /* tv_id */
> > +  PROP_cfg | PROP_ssa,                  /* properties_required */
> > +  0,                                    /* properties_provided */
> > +  0,                                    /* properties_destroyed */
> > +  0,                                    /* todo_flags_start */
> > +  TODO_update_ssa                       /* todo_flags_finish */
> > + }
> > +};
> 
> So, David M (CC'd) has just pulled the rug from under you a little
(Looks like I wasn't CCed, but I happened to see this given we were
chatting on IRC about it; have added myself to CC now)

> bit.  The pass manager now has a different API.  The changes are not
> too drastic, but you'll need to rework the registration of the pass.
> (http://gcc.gnu.org/ml/gcc-patches/2013-08/msg00231.html).

I'm sorry for the inconvenience; a lot changed in trunk with these
changes.

A description of the API change can be seen in this mail:
http://gcc.gnu.org/ml/gcc-patches/2013-07/msg01259.html
and perhaps by reading:
https://github.com/davidmalcolm/gcc-refactoring-scripts/blob/master/test_refactor_passes.py

> David wrote a refactoring tool to use, but this being a single pass it
> should be easy to convert it manually.  See the other passes, and I'm
> sure David will be only too happy to help you if you run into trouble.

The script is "refactor_passes.py" within:
https://github.com/davidmalcolm/gcc-refactoring-scripts

I had a go at running the script on your branch; currently it fails
with:
  File "refactor_passes.py", line 372, in make_replacement2
    assert extra != d[extra]
AssertionError
which is an overly terse way of reporting that a callback in an IPA pass
has the same identifier name as the name of the corresponding field,
which leads to the code breaking under the new API: specifically, I
believe the branch is missing the commit that's r201020 on trunk:

    2013-07-18  David Malcolm  <dmalcolm@redhat.com>
    
        * ipa-pure-const.c (generate_summary): Rename to...
        (pure_const_generate_summary): ... this.

(see http://gcc.gnu.org/ml/gcc-patches/2013-07/msg00687.html for more
details on this one).

On running it on just vtable-verify.c thusly:
  $ python refactor_passes.py ../src/gcc/vtable-verify.c 
it generated the attached patch, which looks correct to me (I haven't
attempted to remerge the branches to see if it compiles, but it looks
like it should); I kept the generated ChangeLog in the patchfile.

You'll also need to change the extern decl in tree-pass.h:
  extern struct gimple_opt_pass pass_vtable_verify;
to that of the factory function:
  extern gimple_opt_pass *make_pass_vtable_verify (gcc::context *ctxt);
(the refactoring script will also do this, but currently in the branch
there are ~200 other passes that also get touched, so the specific
change would be lost in the noise).

Note that the vtable_verify pass appears to be single-instanced within
the pass tree.  For future reference, if someone wants to run the script
on a *multi-instanced* pass, they'll need to supply a clone method (the
refactor script only adds them for those that it "knows" are
multi-instanced, using a hardcoded list).  Writing a dummy clone method
is trivial, though.

> > Index: gcc/passes.def
> > ===================================================================
> > --- gcc/passes.def (revision 201377)
> > +++ gcc/passes.def (working copy)
> > @@ -292,6 +292,7 @@ along with GCC; see the file COPYING3.
> >        NEXT_PASS (pass_tm_memopt);
> >        NEXT_PASS (pass_tm_edges);
> >    POP_INSERT_PASSES ()
> > +  NEXT_PASS (pass_vtable_verify);
> 
> This will also need changes after David's changes.

If I reading this right, I don't think so - I think the above ought to
just work; r201505 updated the meaning of NEXT_PASS (actually for
pass-instances.def) so that it will use make_PASS_NAME i.e. this becomes
a call to "make_pass_vtable_verify" (wrapped in other stuff).

Hope this is helpful; sorry again for the inconvenience.
Dave

diff --git a/gcc/ChangeLog b/gcc/ChangeLog
index f357e85..f1d8aed 100644
--- a/gcc/ChangeLog
+++ b/gcc/ChangeLog
@@ -1,3 +1,14 @@
+2013-08-05  David Malcolm  <dmalcolm@redhat.com>
+
+	Patch autogenerated by refactor_passes.py from
+	https://github.com/davidmalcolm/gcc-refactoring-scripts
+	revision 03fe39476a4c4ea450b49e087cfa817b5f92021e
+
+	* vtable-verify.c (pass_vtable_verify): Convert from a global struct
+	to a subclass of gimple_opt_pass along with...
+	(pass_data_vtable_verify): ...new pass_data instance and...
+	(make_pass_vtable_verify): ...new function.
+
 2013-05-31  Yuri Rumyantsev  <yuri.s.rumyantsev@intel.com>
 	    Igor Zamyatin  <igor.zamyatin@intel.com>
 
diff --git a/gcc/vtable-verify.c b/gcc/vtable-verify.c
index e48f21f..e7b58fd 100644
--- a/gcc/vtable-verify.c
+++ b/gcc/vtable-verify.c
@@ -762,24 +762,42 @@ gate_tree_vtable_verify (void)
 
 /* Definition of this optimization pass.  */
 
-struct gimple_opt_pass pass_vtable_verify =
+namespace {
+
+const pass_data pass_data_vtable_verify =
 {
- {
-  GIMPLE_PASS,
-  "vtable-verify",                      /* name */
-  OPTGROUP_NONE,                        /* optinfo_flags */
-  gate_tree_vtable_verify,              /* gate */
-  vtable_verify_main,                   /* execute */
-  NULL,                                 /* sub */
-  NULL,                                 /* next */
-  0,                                    /* static_pass_number */
-  TV_VTABLE_VERIFICATION,               /* tv_id */
-  PROP_cfg | PROP_ssa,                  /* properties_required */
-  0,                                    /* properties_provided */
-  0,                                    /* properties_destroyed */
-  0,                                    /* todo_flags_start */
-  TODO_update_ssa                       /* todo_flags_finish */
- }
+  GIMPLE_PASS, /* type */
+  "vtable-verify", /* name */
+  OPTGROUP_NONE, /* optinfo_flags */
+  true, /* has_gate */
+  true, /* has_execute */
+  TV_VTABLE_VERIFICATION, /* tv_id */
+  ( PROP_cfg | PROP_ssa ), /* properties_required */
+  0, /* properties_provided */
+  0, /* properties_destroyed */
+  0, /* todo_flags_start */
+  TODO_update_ssa, /* todo_flags_finish */
 };
 
+class pass_vtable_verify : public gimple_opt_pass
+{
+public:
+  pass_vtable_verify(gcc::context *ctxt)
+    : gimple_opt_pass(pass_data_vtable_verify, ctxt)
+  {}
+
+  /* opt_pass methods: */
+  bool gate () { return gate_tree_vtable_verify (); }
+  unsigned int execute () { return vtable_verify_main (); }
+
+}; // class pass_vtable_verify
+
+} // anon namespace
+
+gimple_opt_pass *
+make_pass_vtable_verify (gcc::context *ctxt)
+{
+  return new pass_vtable_verify (ctxt);
+}
+
 #include "gt-vtable-verify.h"