0001-Part-1.-Add-generic-part-for-Intel-CET-enabling

Tsimbalist, Igor V igor.v.tsimbalist@intel.com
Thu Oct 5 10:20:00 GMT 2017


I would like to implement the patch in a bit different way depending on answers I will get for
my following proposals:

- I propose to make a type with 'nocf_check' attribute to be different from type w/o the attribute.
   The reason is that the type with 'nocf_check' attribute implies different code generation. It will be
   done by setting affects_type_identity field to true for the attribute. That in turn will trigger
   needed or expected type checking;

- I propose to ignore the 'nocf_check' attribute if 'fcf-protection' option is not specified and output
   the warning about this. If there is no instrumentation the type with attribute should not be treated
   differently from type w/o the attribute (see previous item) and should not be recorded into the
   type.

If it's ok, please ignore my previous patch (version#3) and I will post the updated patch shortly.

Igor


> -----Original Message-----
> From: Tsimbalist, Igor V
> Sent: Friday, September 29, 2017 6:04 PM
> To: Jeff Law <law@redhat.com>; gcc-patches@gcc.gnu.org
> Cc: richard.guenther@gmail.com; Tsimbalist, Igor V
> <igor.v.tsimbalist@intel.com>
> Subject: RE: 0001-Part-1.-Add-generic-part-for-Intel-CET-enabling
> 
> Updated patch, version #3.
> 
> Igor
> 
> 
> > -----Original Message-----
> > From: Tsimbalist, Igor V
> > Sent: Friday, September 29, 2017 4:32 PM
> > To: Jeff Law <law@redhat.com>; gcc-patches@gcc.gnu.org
> > Cc: richard.guenther@gmail.com; Tsimbalist, Igor V
> > <igor.v.tsimbalist@intel.com>
> > Subject: RE: 0001-Part-1.-Add-generic-part-for-Intel-CET-enabling
> >
> > > -----Original Message-----
> > > From: Jeff Law [mailto:law@redhat.com]
> > > Sent: Friday, September 29, 2017 12:44 AM
> > > To: Tsimbalist, Igor V <igor.v.tsimbalist@intel.com>; gcc-
> > > patches@gcc.gnu.org
> > > Cc: richard.guenther@gmail.com
> > > Subject: Re: 0001-Part-1.-Add-generic-part-for-Intel-CET-enabling
> > >
> > > On 09/19/2017 07:39 AM, Tsimbalist, Igor V wrote:
> > > > Here is an updated patch (version #2). The main differences are:
> > > >
> > > > - Change attribute and option names;
> > > > - Add additional parameter to gimple_build_call_from_tree by
> > > > adding a
> > > type parameter and
> > > >   use it 'nocf_check' attribute propagation;
> > > > - Reimplement fixes in expand_call_stmt to propagate 'nocf_check'
> > > > attribute;
> > > > - Consider 'nocf_check' attribute in Identical Code Folding (ICF)
> > > > optimization;
> > > > - Add warning for type inconsistency regarding 'nocf_check'
> > > > attribute;
> > > > - Many small fixes;
> > > >
> > > > gcc/c-family/
> > > > 	* c-attribs.c (handle_nocf_check_attribute): New function.
> > > > 	(c_common_attribute_table): Add 'nocf_check' handling.
> > > > 	* c-common.c (check_missing_format_attribute): New function.
> > > > 	* c-common.h: Likewise.
> > > >
> > > > gcc/c/
> > > > 	* c-typeck.c (convert_for_assignment): Add check for nocf_check
> > > > 	attribute.
> > > > 	* gimple-parser.c: Add second argument NULL to
> > > > 	gimple_build_call_from_tree.
> > > >
> > > > gcc/cp/
> > > > 	* typeck.c (convert_for_assignment): Add check for nocf_check
> > > > 	attribute.
> > > >
> > > > gcc/
> > > > 	* cfgexpand.c (expand_call_stmt): Set REG_CALL_NOCF_CHECK for
> > > > 	call insn.
> > > > 	* combine.c (distribute_notes): Add REG_CALL_NOCF_CHECK
> > > handling.
> > > > 	* common.opt: Add fcf-protection flag.
> > > > 	* emit-rtl.c (try_split): Add REG_CALL_NOCF_CHECK handling.
> > > > 	* flag-types.h: Add enum cf_protection_level.
> > > > 	* gimple.c (gimple_build_call_from_tree): Add second parameter.
> > > > 	Add 'nocf_check' attribute propagation to gimple call.
> > > > 	* gimple.h (gf_mask): Add GF_CALL_NOCF_CHECK.
> > > > 	(gimple_call_nocf_check_p): New function.
> > > > 	(gimple_call_set_nocf_check): Likewise.
> > > > 	* gimplify.c: Add second argument to gimple_build_call_from_tree.
> > > > 	* ipa-icf.c: Add nocf_check attribute in statement hash.
> > > > 	* recog.c (peep2_attempt): Add REG_CALL_NOCF_CHECK handling.
> > > > 	* reg-notes.def: Add REG_NOTE (CALL_NOCF_CHECK).
> > > > 	* toplev.c (process_options): Add flag_cf_protection handling.
> > > >
> > > > Is it ok for trunk?
> > > >
> > > > Thanks,
> > > > Igor
> > > >
> > > >
> > >
> > >
> > > >
> > > > diff --git a/gcc/c-family/c-attribs.c b/gcc/c-family/c-attribs.c
> > > > index
> > > > 0337537..77d1909 100644
> > > > --- a/gcc/c-family/c-attribs.c
> > > > +++ b/gcc/c-family/c-attribs.c
> > > > @@ -65,6 +65,7 @@ static tree handle_asan_odr_indicator_attribute
> > > > (tree *, tree, tree, int,  static tree
> > > > handle_stack_protect_attribute (tree *, tree, tree, int, bool *);
> > > > static tree handle_noinline_attribute (tree *, tree, tree, int,
> > > > bool *);  static tree handle_noclone_attribute (tree *, tree,
> > > > tree, int, bool *);
> > > > +static tree handle_nocf_check_attribute (tree *, tree, tree, int,
> > > > +bool *);
> > > >  static tree handle_noicf_attribute (tree *, tree, tree, int, bool
> > > > *); static tree handle_noipa_attribute (tree *, tree, tree, int,
> > > > bool *); static tree handle_leaf_attribute (tree *, tree, tree,
> > > > int, bool *); @@ -367,6 +368,8 @@ const struct attribute_spec
> > > c_common_attribute_table[] =
> > > >    { "patchable_function_entry",	1, 2, true, false, false,
> > > >  			      handle_patchable_function_entry_attribute,
> > > >  			      false },
> > > > +  { "nocf_check",		      0, 0, false, true, true,
> > > > +			      handle_nocf_check_attribute, false },
> > > >    { NULL,                     0, 0, false, false, false, NULL, false }
> > > >  };
> > > >
> > > > @@ -783,6 +786,26 @@ handle_noclone_attribute (tree *node, tree
> > > name,
> > > >    return NULL_TREE;
> > > >  }
> > > >
> > > > +/* Handle a "nocf_check" attribute; arguments as in
> > > > +   struct attribute_spec.handler.  */
> > > > +
> > > > +static tree
> > > > +handle_nocf_check_attribute (tree *node, tree name,
> > > > +			  tree ARG_UNUSED (args),
> > > > +			  int ARG_UNUSED (flags), bool *no_add_attrs) {
> > > > +  if (TREE_CODE (*node) != FUNCTION_TYPE
> > > > +      && TREE_CODE (*node) != METHOD_TYPE
> > > > +      && TREE_CODE (*node) != FIELD_DECL
> > > > +      && TREE_CODE (*node) != TYPE_DECL)
> > > So curious, is this needed for FIELD_DECL and TYPE_DECL?  ISTM the
> > > attribute is applied to function/method types.
> > >
> > > If we do need to handle FIELD_DECL and TYPE_DECL here, can you add a
> > > quick comment why?
> >
> > You are right. Probably it was left from the attribute transition from
> > decl to type.
> > I removed these two lines. All CET tests passed.
> >
> > > > diff --git a/gcc/c-family/c-common.c b/gcc/c-family/c-common.c
> > > > index b3ec3a0..78a730e 100644
> > > > --- a/gcc/c-family/c-common.c
> > > > +++ b/gcc/c-family/c-common.c
> > > > @@ -7253,6 +7253,26 @@ check_missing_format_attribute (tree ltype,
> > > tree rtype)
> > > >      return false;
> > > >  }
> > > >
> > > > +/* Check for missing nocf_check attributes on function pointers.
> > > > +LTYPE
> > is
> > > > +   the new type or left-hand side type.  RTYPE is the old type or
> > > > +   right-hand side type.  Returns TRUE if LTYPE is missing the desired
> > > > +   attribute.  */
> > > > +
> > > > +bool
> > > > +check_missing_nocf_check_attribute (tree ltype, tree rtype) {
> > > > +  tree const ttr = TREE_TYPE (rtype), ttl = TREE_TYPE (ltype);
> > > > +  tree ra, la;
> > > > +
> > > > +  for (ra = TYPE_ATTRIBUTES (ttr); ra; ra = TREE_CHAIN (ra))
> > > > +    if (is_attribute_p ("nocf_check", TREE_PURPOSE (ra)))
> > > > +      break;
> > > > +  for (la = TYPE_ATTRIBUTES (ttl); la; la = TREE_CHAIN (la))
> > > > +    if (is_attribute_p ("nocf_check", TREE_PURPOSE (la)))
> > > > +      break;
> > > > +  return la != ra;
> > > ?  ISTM the only time la == ra here is when they're both NULL.
> > > Aren't the TYPE_ATTRIBUTE chain members unique and thus pointer
> > > equality isn't the right check?
> > >
> > > Shouldn't you be looking at the TREE_PURPOSE of ra and la and
> > > comparing those?
> >
> > It looks I was lucky :). I see the point and re-wrote the return
> > statement as
> >
> >    if ((la && ra)		/* Both types have the attribute.  */
> >        || (la == ra))	/* Both types do not have the attribute.  */
> >      return false;
> >    else
> >      return true;		/* One of the types has the attribute.  */
> >
> > Igor
> >
> > > Not accepting or rejecting at this point as I could mis-understand
> > > how how this is supposed to work in my two comments above.
> > >
> > > jeff
> > >
> > >
> > >



More information about the Gcc-patches mailing list