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 <l...@redhat.com>; gcc-patches@gcc.gnu.org > Cc: richard.guent...@gmail.com; Tsimbalist, Igor V > <igor.v.tsimbal...@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 <l...@redhat.com>; gcc-patches@gcc.gnu.org > > Cc: richard.guent...@gmail.com; Tsimbalist, Igor V > > <igor.v.tsimbal...@intel.com> > > Subject: RE: 0001-Part-1.-Add-generic-part-for-Intel-CET-enabling > > > > > -----Original Message----- > > > From: Jeff Law [mailto:l...@redhat.com] > > > Sent: Friday, September 29, 2017 12:44 AM > > > To: Tsimbalist, Igor V <igor.v.tsimbal...@intel.com>; gcc- > > > patc...@gcc.gnu.org > > > Cc: richard.guent...@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 > > > > > > > > >
