diff mbox

[CHKP] Fix static const bounds creation in LTO

Message ID 20150403161131.GB61994@msticlxl57.ims.intel.com
State New
Headers show

Commit Message

Ilya Enkovich April 3, 2015, 4:11 p.m. UTC 
On 02 Apr 22:45, Jan Hubicka wrote:
> > Hi,
> > 
> > Current in LTO static const bounds are created as external symbol.  It doesn't work in case original symbols were removed and privatized.  This patch introduces a separate comdat symbol to be used in LTO.   Bootstrapped and tested on x86_64-unknown-linux-gnu.  Does this approach look OK?
> 
> As I understand it, you want to create a static variables, like __chkp_zero_bounds
> which should be shared across translation units.  Currently the function does:
>   static tree
>   chkp_make_static_const_bounds (HOST_WIDE_INT lb,
> 				 HOST_WIDE_INT ub,
> 				 const char *name)
>   {
>     tree var;
> 
>     /* With LTO we may have constant bounds already in varpool.
>        Try to find it.  */
>     var = chkp_find_const_bounds_var (lb, ub);
> 
>     if (var)
>       return var;
> 
>     var  = build_decl (UNKNOWN_LOCATION, VAR_DECL,
> 		       get_identifier (name), pointer_bounds_type_node);
> 
>     TREE_PUBLIC (var) = 1;
>     TREE_USED (var) = 1;
>     TREE_READONLY (var) = 1;
>     TREE_STATIC (var) = 1;
>     TREE_ADDRESSABLE (var) = 0;
>     DECL_ARTIFICIAL (var) = 1;
>     DECL_READ_P (var) = 1;
>     /* We may use this symbol during ctors generation in chkp_finish_file
>        when all symbols are emitted.  Force output to avoid undefined
>        symbols in ctors.  */
>     if (!in_lto_p)
>       {
> 	DECL_INITIAL (var) = targetm.chkp_make_bounds_constant (lb, ub);
> 	DECL_COMDAT (var) = 1;
> 	varpool_node::get_create (var)->set_comdat_group (DECL_ASSEMBLER_NAME (var));
> 	varpool_node::get_create (var)->force_output = 1;
>       }
>     else
>       DECL_EXTERNAL (var) = 1;
>     varpool_node::finalize_decl (var);
>   }
> 
> You should not set comdat group by hand, or we get into troubles on non-ELF
> targets.  Just call make_decl_one_only (var, DECL_ASSEMBLER_NAME (var));
> 
> Now in LTO I guess you want to  check if the symbol is provided by the source
> compilation unit, i.e. call symtab_node::get_for_asmname (DECL_ASSEMBLER_NAME
> (var)) and if it is there, check it have proper type and fatal_error otherwise
> and if it does, discar var and use existing variable
> 
> This avoid a duplicate declaration of a symbol that is invalid in symtab.
> (once we solve the transparent alias thing, I will add verification for that)
> Because you already set force_output, the symbol should not be privatized and
> renamed in any way.
> 
> If there are cases the symbol can get legally optimized out, I guess you
> can also avoid setting force_output and we can stream references to the
> decls into optimization summary in a case we want ot bind for it in WPA,
> but that can wait for next stage1.
> 
> Honza

Thanks a lot for looking into it!  Seems originally I mostly got problems due to no DECL_WEAK for generated var, make_decl_one_only seems to work for my tests.  Will run more testing to make sure it's OK.  Here is a new patch version.  Is it OK?

Thanks,
Ilya
--
gcc/

2015-04-03  Ilya Enkovich  <ilya.enkovich@intel.com>

	* tree-chkp.c (chkp_find_const_bounds_var): Search
	by assembler name.
	(chkp_make_static_const_bounds): Use make_decl_one_only.

gcc/testsuite/

2015-04-03  Ilya Enkovich  <ilya.enkovich@intel.com>

	* gcc.dg/lto/chkp-static-bounds_0.c: New.

Comments

Jan Hubicka April 3, 2015, 5:01 p.m. UTC | #1 
> 
> Thanks a lot for looking into it!  Seems originally I mostly got problems due to no DECL_WEAK for generated var, make_decl_one_only seems to work for my tests.  Will run more testing to make sure it's OK.  Here is a new patch version.  Is it OK?

Good ;)
> diff --git a/gcc/tree-chkp.c b/gcc/tree-chkp.c
> index 03f75b3..4daeab6 100644
> --- a/gcc/tree-chkp.c
> +++ b/gcc/tree-chkp.c
> @@ -1876,26 +1876,17 @@ chkp_add_bounds_to_call_stmt (gimple_stmt_iterator *gsi)
>  /* Return constant static bounds var with specified LB and UB
>     if such var exists in varpool.  Return NULL otherwise.  */
>  static tree
> -chkp_find_const_bounds_var (HOST_WIDE_INT lb,
> -			    HOST_WIDE_INT ub)
> +chkp_find_const_bounds_var (tree id)
>  {
> -  tree val = targetm.chkp_make_bounds_constant (lb, ub);
> -  struct varpool_node *node;
> -
> -  /* We expect bounds constant is represented as a complex value
> -     of two pointer sized integers.  */
> -  gcc_assert (TREE_CODE (val) == COMPLEX_CST);
> +  struct symtab_node *node = symtab_node::get_for_asmname (id);

Sadly I think this won't work on targets that adds prefix to assembler name,
like djgpp/mingw.
To obtain mangled assembler name you need to call
id = targetm.mangle_decl_assembler_name (decl, DECL_NAME (decl));
that in turn requires decl to already exist.  That is why I guess you may want
to just build the new var and see if already exists....

Honza
>  
> -  FOR_EACH_VARIABLE (node)
> -    if (POINTER_BOUNDS_P (node->decl)
> -	&& TREE_READONLY (node->decl)
> -	&& DECL_INITIAL (node->decl)
> -	&& TREE_CODE (DECL_INITIAL (node->decl)) == COMPLEX_CST
> -	&& tree_int_cst_equal (TREE_REALPART (DECL_INITIAL (node->decl)),
> -			       TREE_REALPART (val))
> -	&& tree_int_cst_equal (TREE_IMAGPART (DECL_INITIAL (node->decl)),
> -			       TREE_IMAGPART (val)))
> +  if (node)
> +    {
> +      /* We don't allow this symbol usage for non bounds.  */
> +      gcc_assert (node->type == SYMTAB_VARIABLE);
> +      gcc_assert (POINTER_BOUNDS_P (node->decl));
>        return node->decl;
> +    }
>  
>    return NULL;
>  }
> @@ -1907,37 +1898,34 @@ chkp_make_static_const_bounds (HOST_WIDE_INT lb,
>  			       HOST_WIDE_INT ub,
>  			       const char *name)
>  {
> +  tree id = get_identifier (name);
>    tree var;
> +  varpool_node *node;
>  
>    /* With LTO we may have constant bounds already in varpool.
>       Try to find it.  */
> -  var = chkp_find_const_bounds_var (lb, ub);
> +  var = chkp_find_const_bounds_var (id);
>  
>    if (var)
>      return var;
>  
> -  var  = build_decl (UNKNOWN_LOCATION, VAR_DECL,
> -		     get_identifier (name), pointer_bounds_type_node);
> +  var  = build_decl (UNKNOWN_LOCATION, VAR_DECL, id,
> +		     pointer_bounds_type_node);
>  
> -  TREE_PUBLIC (var) = 1;
>    TREE_USED (var) = 1;
>    TREE_READONLY (var) = 1;
>    TREE_STATIC (var) = 1;
>    TREE_ADDRESSABLE (var) = 0;
>    DECL_ARTIFICIAL (var) = 1;
>    DECL_READ_P (var) = 1;
> +  DECL_INITIAL (var) = targetm.chkp_make_bounds_constant (lb, ub);
> +  make_decl_one_only (var, DECL_ASSEMBLER_NAME (var));
>    /* We may use this symbol during ctors generation in chkp_finish_file
>       when all symbols are emitted.  Force output to avoid undefined
>       symbols in ctors.  */
> -  if (!in_lto_p)
> -    {
> -      DECL_INITIAL (var) = targetm.chkp_make_bounds_constant (lb, ub);
> -      DECL_COMDAT (var) = 1;
> -      varpool_node::get_create (var)->set_comdat_group (DECL_ASSEMBLER_NAME (var));
> -      varpool_node::get_create (var)->force_output = 1;
> -    }
> -  else
> -    DECL_EXTERNAL (var) = 1;
> +  node = varpool_node::get_create (var);
> +  node->force_output = 1;
> +
>    varpool_node::finalize_decl (var);
>  
>    return var;
diff mbox

Patch

diff --git a/gcc/testsuite/gcc.dg/lto/chkp-static-bounds_0.c b/gcc/testsuite/gcc.dg/lto/chkp-static-bounds_0.c
new file mode 100644
index 0000000..596e551
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/lto/chkp-static-bounds_0.c
@@ -0,0 +1,26 @@ 
+/* { dg-lto-do link } */
+/* { dg-require-effective-target mpx } */
+/* { dg-lto-options { { -flto -flto-partition=max -fcheck-pointer-bounds -mmpx } } } */
+
+const char *cc;
+
+int test1 (const char *c)
+{
+  c = __builtin___bnd_init_ptr_bounds (c);
+  cc = c;
+  return c[0] * 2;
+}
+
+struct S
+{
+  int (*fnptr) (const char *);
+} S;
+
+struct S s1 = {test1};
+struct S s2 = {test1};
+struct S s3 = {test1};
+
+int main (int argc, const char **argv)
+{
+  return s1.fnptr (argv[0]) + s2.fnptr (argv[1]);
+}
diff --git a/gcc/tree-chkp.c b/gcc/tree-chkp.c
index 03f75b3..4daeab6 100644
--- a/gcc/tree-chkp.c
+++ b/gcc/tree-chkp.c
@@ -1876,26 +1876,17 @@  chkp_add_bounds_to_call_stmt (gimple_stmt_iterator *gsi)
 /* Return constant static bounds var with specified LB and UB
    if such var exists in varpool.  Return NULL otherwise.  */
 static tree
-chkp_find_const_bounds_var (HOST_WIDE_INT lb,
-			    HOST_WIDE_INT ub)
+chkp_find_const_bounds_var (tree id)
 {
-  tree val = targetm.chkp_make_bounds_constant (lb, ub);
-  struct varpool_node *node;
-
-  /* We expect bounds constant is represented as a complex value
-     of two pointer sized integers.  */
-  gcc_assert (TREE_CODE (val) == COMPLEX_CST);
+  struct symtab_node *node = symtab_node::get_for_asmname (id);
 
-  FOR_EACH_VARIABLE (node)
-    if (POINTER_BOUNDS_P (node->decl)
-	&& TREE_READONLY (node->decl)
-	&& DECL_INITIAL (node->decl)
-	&& TREE_CODE (DECL_INITIAL (node->decl)) == COMPLEX_CST
-	&& tree_int_cst_equal (TREE_REALPART (DECL_INITIAL (node->decl)),
-			       TREE_REALPART (val))
-	&& tree_int_cst_equal (TREE_IMAGPART (DECL_INITIAL (node->decl)),
-			       TREE_IMAGPART (val)))
+  if (node)
+    {
+      /* We don't allow this symbol usage for non bounds.  */
+      gcc_assert (node->type == SYMTAB_VARIABLE);
+      gcc_assert (POINTER_BOUNDS_P (node->decl));
       return node->decl;
+    }
 
   return NULL;
 }
@@ -1907,37 +1898,34 @@  chkp_make_static_const_bounds (HOST_WIDE_INT lb,
 			       HOST_WIDE_INT ub,
 			       const char *name)
 {
+  tree id = get_identifier (name);
   tree var;
+  varpool_node *node;
 
   /* With LTO we may have constant bounds already in varpool.
      Try to find it.  */
-  var = chkp_find_const_bounds_var (lb, ub);
+  var = chkp_find_const_bounds_var (id);
 
   if (var)
     return var;
 
-  var  = build_decl (UNKNOWN_LOCATION, VAR_DECL,
-		     get_identifier (name), pointer_bounds_type_node);
+  var  = build_decl (UNKNOWN_LOCATION, VAR_DECL, id,
+		     pointer_bounds_type_node);
 
-  TREE_PUBLIC (var) = 1;
   TREE_USED (var) = 1;
   TREE_READONLY (var) = 1;
   TREE_STATIC (var) = 1;
   TREE_ADDRESSABLE (var) = 0;
   DECL_ARTIFICIAL (var) = 1;
   DECL_READ_P (var) = 1;
+  DECL_INITIAL (var) = targetm.chkp_make_bounds_constant (lb, ub);
+  make_decl_one_only (var, DECL_ASSEMBLER_NAME (var));
   /* We may use this symbol during ctors generation in chkp_finish_file
      when all symbols are emitted.  Force output to avoid undefined
      symbols in ctors.  */
-  if (!in_lto_p)
-    {
-      DECL_INITIAL (var) = targetm.chkp_make_bounds_constant (lb, ub);
-      DECL_COMDAT (var) = 1;
-      varpool_node::get_create (var)->set_comdat_group (DECL_ASSEMBLER_NAME (var));
-      varpool_node::get_create (var)->force_output = 1;
-    }
-  else
-    DECL_EXTERNAL (var) = 1;
+  node = varpool_node::get_create (var);
+  node->force_output = 1;
+
   varpool_node::finalize_decl (var);
 
   return var;