This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [RFC] Making fold-const sane WRT symbol visibilities


> > The problem is that the patch fails testcases that assume we do such folding at parsing
> > time.
> > 
> > ./testsuite/gcc/gcc.sum:FAIL: gcc.dg/pr36901-1.c (test for excess errors)
> > ./testsuite/gcc/gcc.sum:FAIL: gcc.dg/pr36901-2.c (test for excess errors)
> > ./testsuite/gcc/gcc.sum:FAIL: gcc.dg/pr36901-3.c (test for excess errors)
> > ./testsuite/gcc/gcc.sum:FAIL: gcc.dg/pr36901-4.c (test for excess errors)
> > 
> > Here we accept the source as compile time constant while I think it is not:
> > int sc = (&sc > 0);
> > 
> > Does it seem resonable to turn those testcases into one testing for error and 
> > live with delaying the folding oppurtunities to early opts?  They are now caught
> > by ccp1 pass usually.
> 
> IMHO all symbol visibility related foldings are very premature if done
> in the frontends (well, most of fold-const.c is ...).  Of course
> everything depends on whether there exists a frontend that requires
> these foldings for correctness ...

Yeah, in my testing it seems that C frontend do care in the testcase above - we
used to accept code that does test nonzeroness of address as a constant, while we
don't.
Clang (IMO correctly) reject it.  If we are OK with changing the bhaviour, I will
just commit the patch and remove the testcase above (or turn it into error check)
> 
> Personally I find "nonzero" ambiguous as it doesn't clearly state
> it is about the symbols address rather than its value.

I can change it to nonzero_address.  It did not appear one might think about
value stored in the symbol.

Honza
> 
> Richard.
> 
> 
> > Honza
> > 
> > 	* cgraph.h (symtab_node): Add method nonzero.
> > 	(decl_in_symtab_p): Break out from ...
> > 	(symtab_get_node): ... here.
> > 	* symtab.c (symtab_node::nonzero): New method.
> > 	* fold-const.c: Include cgraph.h
> > 	(tree_single_nonzero_warnv_p): Use symtab for symbols.
> > 
> > 	* testsuite/g++.dg/tree-ssa/nonzero-2.C: New testcase.
> > 	* testsuite/g++.dg/tree-ssa/nonzero-1.C: New testcase.
> > 	* testsuite/gcc.dg/tree-ssa/nonzero-1.c: New testcase.
> > Index: cgraph.h
> > ===================================================================
> > --- cgraph.h	(revision 211915)
> > +++ cgraph.h	(working copy)
> > @@ -214,6 +214,9 @@ public:
> >  
> >    void set_init_priority (priority_type priority);
> >    priority_type get_init_priority ();
> > +
> > +  /* Return true if symbol is known to be nonzero.  */
> > +  bool nonzero ();
> >  };
> >  
> >  enum availability
> > @@ -1068,6 +1077,17 @@ void varpool_remove_initializer (varpool
> >  /* In cgraph.c */
> >  extern void change_decl_assembler_name (tree, tree);
> >  
> > +/* Return true if DECL should have entry in symbol table if used.
> > +   Those are functions and static & external veriables*/
> > +
> > +static bool
> > +decl_in_symtab_p (const_tree decl)
> > +{
> > +  return (TREE_CODE (decl) == FUNCTION_DECL
> > +          || (TREE_CODE (decl) == VAR_DECL
> > +	      && (TREE_STATIC (decl) || DECL_EXTERNAL (decl))));
> > +}
> > +
> >  /* Return symbol table node associated with DECL, if any,
> >     and NULL otherwise.  */
> >  
> > @@ -1075,12 +1095,7 @@ static inline symtab_node *
> >  symtab_get_node (const_tree decl)
> >  {
> >  #ifdef ENABLE_CHECKING
> > -  /* Check that we are called for sane type of object - functions
> > -     and static or external variables.  */
> > -  gcc_checking_assert (TREE_CODE (decl) == FUNCTION_DECL
> > -		       || (TREE_CODE (decl) == VAR_DECL
> > -			   && (TREE_STATIC (decl) || DECL_EXTERNAL (decl)
> > -			       || in_lto_p)));
> > +  gcc_checking_assert (decl_in_symtab_p (decl));
> >    /* Check that the mapping is sane - perhaps this check can go away,
> >       but at the moment frontends tends to corrupt the mapping by calling
> >       memcpy/memset on the tree nodes.  */
> > Index: symtab.c
> > ===================================================================
> > --- symtab.c	(revision 211915)
> > +++ symtab.c	(working copy)
> > @@ -1574,4 +1574,67 @@ symtab_get_symbol_partitioning_class (sy
> >  
> >    return SYMBOL_PARTITION;
> >  }
> > +
> > +/* Return true when symbol is known to be non-zero.  */
> > +
> > +bool
> > +symtab_node::nonzero ()
> > +{
> > +  /* Weakrefs may be NULL when their target is not defined.  */
> > +  if (this->alias && this->weakref)
> > +    {
> > +      if (this->analyzed)
> > +	{
> > +	  symtab_node *target = symtab_alias_ultimate_target (this);
> > +
> > +	  if (target->alias && target->weakref)
> > +	    return false;
> > +	  /* We can not recurse to target::nonzero.  It is possible that the
> > +	     target is used only via the alias.
> > +	     We may walk references and look for strong use, but we do not know
> > +	     if this strong use will survive to final binary, so be
> > +	     conservative here.  
> > +	     ??? Maybe we could do the lookup during late optimization that
> > +	     could be useful to eliminate the NULL pointer checks in LTO
> > +	     programs.  */
> > +	  if (target->definition && !DECL_EXTERNAL (target->decl))
> > +	    return true;
> > +	  if (target->resolution != LDPR_UNKNOWN
> > +	      && target->resolution != LDPR_UNDEF
> > +	      && flag_delete_null_pointer_checks)
> > +	    return true;
> > +	  return false;
> > +	}
> > +      else
> > +        return false;
> > +    }
> > +
> > +  /* With !flag_delete_null_pointer_checks we assume that symbols may
> > +     bind to NULL. This is on by default on embedded targets only.
> > +
> > +     Otherwise all non-WEAK symbols must be defined and thus non-NULL or
> > +     linking fails.  Important case of WEAK we want to do well are comdats.
> > +     Those are handled by later check for definition.
> > +
> > +     When parsing, beware the cases when WEAK attribute is added later.  */
> > +  if (!DECL_WEAK (this->decl)
> > +      && flag_delete_null_pointer_checks
> > +      && cgraph_state > CGRAPH_STATE_PARSING)
> > +    return true;
> > +
> > +  /* If target is defined and not extern, we know it will be output and thus
> > +     it will bind to non-NULL.
> > +     Play safe for flag_delete_null_pointer_checks where weak definition maye
> > +     be re-defined by NULL.  */
> > +  if (this->definition && !DECL_EXTERNAL (this->decl)
> > +      && (flag_delete_null_pointer_checks || !DECL_WEAK (this->decl)))
> > +    return true;
> > +
> > +  /* As the last resort, check the resolution info.  */
> > +  if (this->resolution != LDPR_UNKNOWN
> > +      && this->resolution != LDPR_UNDEF
> > +      && flag_delete_null_pointer_checks)
> > +    return true;
> > +  return false;
> > +}
> >  #include "gt-symtab.h"
> > Index: fold-const.c
> > ===================================================================
> > --- fold-const.c	(revision 211915)
> > +++ fold-const.c	(working copy)
> > @@ -69,6 +69,7 @@ along with GCC; see the file COPYING3.
> >  #include "tree-dfa.h"
> >  #include "hash-table.h"  /* Required for ENABLE_FOLD_CHECKING.  */
> >  #include "builtins.h"
> > +#include "cgraph.h"
> >  
> >  /* Nonzero if we are folding constants inside an initializer; zero
> >     otherwise.  */
> > @@ -16032,21 +16033,33 @@ tree_single_nonzero_warnv_p (tree t, boo
> >      case ADDR_EXPR:
> >        {
> >  	tree base = TREE_OPERAND (t, 0);
> > +
> >  	if (!DECL_P (base))
> >  	  base = get_base_address (base);
> >  
> >  	if (!base)
> >  	  return false;
> >  
> > -	/* Weak declarations may link to NULL.  Other things may also be NULL
> > -	   so protect with -fdelete-null-pointer-checks; but not variables
> > -	   allocated on the stack.  */
> > +	/* For objects in symbol table check if we know they are non-zero.
> > +	   Don't do anything for variables and functions before symtab is built;
> > +	   it is quite possible that they will be declared weak later.  */
> > +	if (DECL_P (base) && decl_in_symtab_p (base))
> > +	  {
> > +	    struct symtab_node *symbol;
> > +
> > +	    symbol = symtab_get_node (base);
> > +	    if (symbol)
> > +	      return symbol->nonzero ();
> > +	    else
> > +	      return false;
> > +	  }
> > +
> > +	/* Function local objects are never NULL.  */
> >  	if (DECL_P (base)
> > -	    && (flag_delete_null_pointer_checks
> > -		|| (DECL_CONTEXT (base)
> > -		    && TREE_CODE (DECL_CONTEXT (base)) == FUNCTION_DECL
> > -		    && auto_var_in_fn_p (base, DECL_CONTEXT (base)))))
> > -	  return !VAR_OR_FUNCTION_DECL_P (base) || !DECL_WEAK (base);
> > +	    && (DECL_CONTEXT (base)
> > +		&& TREE_CODE (DECL_CONTEXT (base)) == FUNCTION_DECL
> > +		&& auto_var_in_fn_p (base, DECL_CONTEXT (base))))
> > +	  return true;
> >  
> >  	/* Constants are never weak.  */
> >  	if (CONSTANT_CLASS_P (base))
> > Index: testsuite/gcc.dg/tree-ssa/nonzero-1.c
> > ===================================================================
> > --- testsuite/gcc.dg/tree-ssa/nonzero-1.c	(revision 0)
> > +++ testsuite/gcc.dg/tree-ssa/nonzero-1.c	(revision 0)
> > @@ -0,0 +1,11 @@
> > +/* { dg-do compile } */
> > +/* { dg-options "-O2 -fdump-tree-optimized" } */
> > +extern int a;
> > +t()
> > +{
> > +  return &a!=0;
> > +}
> > +extern int a __attribute__ ((weak));
> > +
> > +/* { dg-final { scan-tree-dump-not "return 1" "optimized"} } */
> > +/* { dg-final { cleanup-tree-dump "optimized" } } */
> > Index: testsuite/g++.dg/tree-ssa/nonzero-1.C
> > ===================================================================
> > --- testsuite/g++.dg/tree-ssa/nonzero-1.C	(revision 0)
> > +++ testsuite/g++.dg/tree-ssa/nonzero-1.C	(revision 0)
> > @@ -0,0 +1,12 @@
> > +/* { dg-do compile } */
> > +/* { dg-options "-O2 -fdump-tree-ccp1" } */
> > +inline void t()
> > +{
> > +}
> > +int m()
> > +{
> > +  void *q = (void *)&t;
> > +  return q != 0;
> > +}
> > +/* { dg-final { scan-tree-dump "return 1" "ccp1"} } */
> > +/* { dg-final { cleanup-tree-dump "ccp1" } } */
> > Index: testsuite/g++.dg/tree-ssa/nonzero-2.C
> > ===================================================================
> > --- testsuite/g++.dg/tree-ssa/nonzero-2.C	(revision 0)
> > +++ testsuite/g++.dg/tree-ssa/nonzero-2.C	(revision 0)
> > @@ -0,0 +1,16 @@
> > +/* { dg-do compile } */
> > +/* { dg-options "-O2 -fdump-tree-ccp1 -fdelete-null-pointer-checks" } */
> > +struct t
> > +{
> > +  static inline void tt()
> > +  {
> > +  }
> > +  virtual void q();
> > +};
> > +int m()
> > +{
> > +  void *q = (void *)&t::tt;
> > +  return q != 0;
> > +}
> > +/* { dg-final { scan-tree-dump "return 1" "ccp1"} } */
> > +/* { dg-final { cleanup-tree-dump "ccp1" } } */
> > 
> > 
> 
> -- 
> Richard Biener <rguenther@suse.de>
> SUSE / SUSE Labs
> SUSE LINUX Products GmbH - Nuernberg - AG Nuernberg - HRB 16746
> GF: Jeff Hawn, Jennifer Guild, Felix Imend"orffer


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]