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

[Diego Novillo <dnovillo at google dot com>]

This looks almost ready to commit.  Some comments below:

Once this is committed, you should write a blurb in GCC's home page
describing the contribution.

> ===================================================================
> --- libgcc/vtv_start_preinit.c (revision 0)
> +++ libgcc/vtv_start_preinit.c (revision 0)
> @@ -0,0 +1,68 @@
> +/*  Copyright (C) 2012, 2013
> +    Free Software Foundation, Inc.
> +
> +This file is part of GCC.
> +
> +GCC is free software; you can redistribute it and/or modify it under
> +the terms of the GNU General Public License as published by the Free
> +Software Foundation; either version 3, or (at your option) any later
> +version.
> +
> +GCC is distributed in the hope that it will be useful, but WITHOUT ANY
> +WARRANTY; without even the implied warranty of MERCHANTABILITY or
> +FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License
> +for more details.
> +
> +You should have received a copy of the GNU General Public License
> +along with GCC; see the file COPYING3.  If not see
> +<http://www.gnu.org/licenses/>.  */

Shouldn't this file have the GPL+exception license?

> +
> +/* This file is part of the vtable verification feature (for a
> +   detailed description of the feature, see comments in
> +   tree-vtable-verify.c).  The vtable verification feature creates

s/tree-vtable-verify.c/vtable-verify.c/

> +GCC is distributed in the hope that it will be useful, but WITHOUT ANY
> +WARRANTY; without even the implied warranty of MERCHANTABILITY or
> +FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License
> +for more details.

Ditto this one and all the other new files in libgcc/

> +
> +You should have received a copy of the GNU General Public License
> +along with GCC; see the file COPYING3.  If not see
> +<http://www.gnu.org/licenses/>.  */
> +
> +/* This file is part of the vtable verification feature (for a
> +   detailed description of the feature, see comments in
> +   tree-vtable-verify.c).  The vtable verification feature creates

s/tree-vtable-verify.c/vtable-verify.c/

> +You should have received a copy of the GNU General Public License
> +along with GCC; see the file COPYING3.  If not see
> +<http://www.gnu.org/licenses/>.  */
> +
> +/* This file is part of the vtable verification feature (for a
> +   detailed description of the feature, see comments in
> +   tree-vtable-verify.c).  The vtable verification feature creates

s/tree-vtable-verify.c/vtable-verify.c/

> +You should have received a copy of the GNU General Public License
> +along with GCC; see the file COPYING3.  If not see
> +<http://www.gnu.org/licenses/>.  */
> +
> +/* This file is part of the vtable verification feature (for a
> +   detailed description of the feature, see comments in
> +   tree-vtable-verify.c).  The vtable verification feature creates

s/tree-vtable-verify.c/vtable-verify.c/

> +  The vtable verification feature is controlled by the flag
> +  '-fvtable-verify='.  There are three flavors of this:
> +  '-fvtable-verify=std', '-fvtable-verify=preinit', and
> +  '-fvtable-verify=none'.  If the option '-fvtable-verfy=preinit' is
> +  used, then our constructor initialization function gets put into the
> +  preinit array.  This is necessary if there are data sets that need
> +  to be built very early in execution.  If the constructor
> +  initialization function gets put into the preinit array, the we also
> +  add calls to __VLTChangePermission at the beginning and end of the
> +  function.  The call at the beginning sets the permissions on the
> +  data sets and vtable map variables to read/write, and the one at the
> +  end makes them read-only.  If the '-fvtable-verify=std' option is
> +  used, the constructor initialization functions are executed at their
> +  normal time, and the __VLTChangePermission calls are handled
> +  differently (see the comments in libstdc++-v3/libsupc++/vtv_rts.cc).
> +  The option '-fvtable-verify=none' turns off vtable verification.

This part of the documentation, dealing with compiler flags and
user-visible features should also be in doc/invoke.texi.

> +#include "config.h"
> +#include "system.h"
> +#include "coretypes.h"
> +#include "tm.h"
> +#include "tree.h"
> +#include "tm_p.h"
> +#include "basic-block.h"
> +#include "output.h"
> +#include "tree-flow.h"
> +#include "tree-dump.h"
> +#include "tree-pass.h"
> +#include "timevar.h"
> +#include "cfgloop.h"
> +#include "flags.h"
> +#include "tree-inline.h"
> +#include "tree-scalar-evolution.h"
> +#include "diagnostic-core.h"
> +#include "gimple-pretty-print.h"
> +#include "toplev.h"
> +#include "langhooks.h"
> +#include "tree-ssa-propagate.h"

Have you made sure you actually need all these headers?  There are
some that look superfluous (like tree-ssa-propagate.h).

> +
> +#include "vtable-verify.h"
> +
> +unsigned num_vtable_map_nodes = 0;
> +bool any_verification_calls_generated = false;
> +int total_num_virtual_calls = 0;
> +int total_num_verified_vcalls = 0;

I suppose all these are needed in the global namespace?  I tested a
couple and it seems to be case.  If not, please make them static.
Ideally, these would go in the new gcc::context class, but we are not
there yet.

> +/* 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
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).

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.

> +#include "gt-vtable-verify.h"
> Index: gcc/vtable-verify.h
> ===================================================================
> --- gcc/vtable-verify.h (revision 0)
> +++ gcc/vtable-verify.h (revision 0)
> @@ -0,0 +1,154 @@
> +/* Interprocedural constant propagation

Hmm?

> +#ifndef VTABLE_VERIFY_H
> +#define VTABLE_VERIFY_H
> +
> +#include "config.h"
> +#include "system.h"
> +#include "coretypes.h"
> +#include "tm.h"
> +#include "timevar.h"
> +#include "cpplib.h"
> +#include "tree.h"
> +#include "hashtab.h"
> +#include "sbitmap.h"
> +#include "hash-table.h"

There are several seemingly redundant #includes here.

> 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.

> +static void
> +write_out_current_set_data (tree base_class, int set_size)
> +{
> +  static int class_data_log_fd = -1;
> +  char buffer[1024];
> +  int bytes_written __attribute__ ((unused));
> +
> +  if (class_data_log_fd == -1)
> +    class_data_log_fd = open ("/tmp/vtv_class_set_sizes.log",

Better use choose_tmpdir() (from libiberty) instead of "/tmp/".

> +                              O_WRONLY | O_APPEND | O_CREAT, S_IRWXU);
> +
> +  if (class_data_log_fd == -1)
> +    return;

Show an error message here?

> +static tree
> +build_key_buffer_arg (tree base_ptr_var_decl)
> +{
> +#define KEY_TYPE_FIXED_SIZE 8

const int, please.

> +static void
> +output_set_info (tree record_type, tree *vtbl_ptr_array, int array_size)
> +{
> +  static int vtv_debug_log_fd = -1;
> +  char buffer[1024];
> +  int bytes_written __attribute__ ((unused));
> +  const char *class_name =
> +              IDENTIFIER_POINTER (DECL_ASSEMBLER_NAME (TYPE_NAME (record_type)));
> +
> +  if (vtv_debug_log_fd == -1)
> +    vtv_debug_log_fd = open ("/tmp/vtv_set_ptr_data.log",

choose_dir() instead of "/tmp".

> +      snprintf (buffer, sizeof (buffer), "%s %s %s + %d\n",
> + main_input_filename, class_name, vptr_name, vptr_offset);

you could just use std::string here, and in other places where you are
doing a bunch of string manipulation.  But this is fine for now.  You
may want to refactor it in subsequent patches.

> +  if (vtv_count_log_fd == -1)
> +    vtv_count_log_fd = open ("/tmp/vtv_count_data.log",
> +                             O_WRONLY | O_APPEND | O_CREAT, S_IRWXU);

choose_tmpdir() instead of "/tmp".

> +# Filter out unsupported systems.
> +case "${target}" in
> +  x86_64-*-linux* | i?86-*-linux*)
> + VTV_SUPPORTED=yes
> + ;;
> +  powerpc*-*-linux*)
> + ;;
> +  sparc*-*-linux*)
> + ;;
> +  arm*-*-linux*)
> + ;;

What about powerpc, sparc and arm?  Why are they mentioned here if no
actual decision is made about support?


Diego.