> -----Original Message-----
> From: Jeff Law [mailto:l...@redhat.com]
> Sent: Friday, August 25, 2017 10:50 PM
> To: Tsimbalist, Igor V <igor.v.tsimbal...@intel.com>; 'gcc-
> patc...@gcc.gnu.org' <gcc-patches@gcc.gnu.org>
> Subject: Re: 0001-Part-1.-Add-generic-part-for-Intel-CET-enabling
>
> On 08/01/2017 02:56 AM, Tsimbalist, Igor V wrote:
> > Part#1. Add generic part for Intel CET enabling.
> >
> > The spec is available at
> >
> > https://software.intel.com/sites/default/files/managed/4d/2a/control-f
> > low-enforcement-technology-preview.pdf
> >
> > High-level design.
> > ------------------
> >
> > A proposal is to introduce a target independent flag
> > -finstrument-control-flow with a semantic to instrument a code to
> > control validness or integrity of control-flow transfers using jump
> > and call instructions. The main goal is to detect and block a possible
> > malware execution through transfer the execution to unknown target
> > address. Implementation could be either software or target based. Any
> > target platforms can provide their implementation for instrumentation
> > under this option.
> >
> > When the -finstrument-control-flow flag is set each implementation has
> > to check if a support exists for a target platform and report an error
> > if no support is found.
> >
> > The compiler should instrument any control-flow transfer points in a
> > program (ex. call/jmp/ret) as well as any landing pads, which are
> > targets of for control-flow transfers.
> >
> > A new 'notrack' attribute is introduced to provide hand tuning support.
> > The attribute directs the compiler to skip a call to a function and a
> > function's landing pad from instrumentation (tracking). The attribute
> > can be used for function and pointer to function types, otherwise it
> > will be ignored. The attribute is saved in a type and propagated to a
> > GIMPLE call statement and later to a call instruction.
> >
> > Currently all platforms except i386 will report the error and do no
> > instrumentation. i386 will provide the implementation based on a
> > specification published by Intel for a new technology called
> > Control-flow Enforcement Technology (CET).
> >
> >
> > 0001-Part-1.-Add-generic-part-for-Intel-CET-enabling.patch
> >
> >
> > From 403fc8239fb1f690cc378287b4def57dcc9d25bf Mon Sep 17 00:00:00
> 2001
> > From: Igor Tsimbalist <igor.v.tsimbal...@intel.com>
> > Date: Mon, 3 Jul 2017 17:11:58 +0300
> > Subject: [PATCH 1/9] Part#1. Add generic part for Intel CET enabling.
> >
> > The spec is available at
> >
> > https://software.intel.com/sites/default/files/managed/4d/2a/control-f
> > low-enforcement-technology-preview.pdf
> >
> > High-level design.
> > ------------------
> >
> > A proposal is to introduce a target independent flag
> > -finstrument-control-flow with a semantic to instrument a code to
> > control validness or integrity of control-flow transfers using jump
> > and call instructions. The main goal is to detect and block a possible
> > malware execution through transfer the execution to unknown target
> > address. Implementation could be either software or target based. Any
> > target platforms can provide their implementation for instrumentation
> > under this option.
> >
> > When the -finstrument-control-flow flag is set each implementation has
> > to check if a support exists for a target platform and report an error
> > if no support is found.
> >
> > The compiler should instrument any control-flow transfer points in a
> > program (ex. call/jmp/ret) as well as any landing pads, which are
> > targets of for control-flow transfers.
> >
> > A new 'notrack' attribute is introduced to provide hand tuning support.
> > The attribute directs the compiler to skip a call to a function and a
> > function's landing pad from instrumentation (tracking). The attribute
> > can be used for function and pointer to function types, otherwise it
> > will be ignored. The attribute is saved in a type and propagated to a
> > GIMPLE call statement and later to a call instruction.
> >
> > Currently all platforms except i386 will report the error and do no
> > instrumentation. i386 will provide the implementation based on a
> > specification published by Intel for a new technology called
> > Control-flow Enforcement Technology (CET).
> >
> > gcc/c-family/
> >
> > * c-attribs.c (handle_notrack_attribute): New function.
> > (c_common_attribute_table): Add 'notrack' handling.
> >
> > gcc/
> >
> > * cfgexpand.c (expand_call_stmt): Set REG_CALL_NOTRACK.
> > * combine.c (distribute_notes): Add REG_CALL_NOTRACK handling.
> > * common.opt: Add finstrument-control-flow flag.
> > * emit-rtl.c (try_split): Add REG_CALL_NOTRACK handling.
> > * gimple.c (gimple_build_call_from_tree): Add 'notrack'
> > propogation.
> > * gimple.h (gf_mask): Add GF_CALL_WITH_NOTRACK.
> > (gimple_call_with_notrack_p): function.
> > (gimple_call_set_with_notrack): Likewise.
> > * recog.c (peep2_attempt): Add REG_CALL_NOTRACK handling.
> > * reg-notes.def: Add REG_NOTE (CALL_NOTRACK).
> > * toplev.c (process_options): Add flag_instrument_control_flow
> > handling.
> > ---
> > gcc/c-family/c-attribs.c | 23 +++++++++++++++++++++++
> > gcc/cfgexpand.c | 11 +++++++++++
> > gcc/combine.c | 1 +
> > gcc/common.opt | 5 +++++
> > gcc/emit-rtl.c | 1 +
> > gcc/gimple.c | 17 +++++++++++++++++
> > gcc/gimple.h | 34 ++++++++++++++++++++++++++++++++++
> > gcc/recog.c | 1 +
> > gcc/reg-notes.def | 7 +++++++
> > gcc/toplev.c | 11 +++++++++++
> > 10 files changed, 111 insertions(+)
> > ---
> > gcc/c-family/c-attribs.c | 23 +++++++++++++++++++++++
> > gcc/cfgexpand.c | 11 +++++++++++
> > gcc/combine.c | 1 +
> > gcc/common.opt | 5 +++++
> > gcc/emit-rtl.c | 1 +
> > gcc/gimple.c | 17 +++++++++++++++++
> > gcc/gimple.h | 34 ++++++++++++++++++++++++++++++++++
> > gcc/recog.c | 1 +
> > gcc/reg-notes.def | 7 +++++++
> > gcc/toplev.c | 11 +++++++++++
> > 10 files changed, 111 insertions(+)
> >
> > diff --git a/gcc/c-family/c-attribs.c b/gcc/c-family/c-attribs.c index
> > 0d9ab2d..4d62f7c 100644
> > --- a/gcc/c-family/c-attribs.c
> > +++ b/gcc/c-family/c-attribs.c
> > diff --git a/gcc/cfgexpand.c b/gcc/cfgexpand.c index c9d8118..ed344c5
> > 100644
> > --- a/gcc/cfgexpand.c
> > +++ b/gcc/cfgexpand.c
> > @@ -2657,12 +2657,23 @@ expand_call_stmt (gcall *stmt)
> > }
> > }
> >
> > + rtx_insn *before_call = get_last_insn ();
> > lhs = gimple_call_lhs (stmt);
> > if (lhs)
> > expand_assignment (lhs, exp, false);
> > else
> > expand_expr (exp, const0_rtx, VOIDmode, EXPAND_NORMAL);
> >
> > + /* Find a generated CALL insn to propagate a 'notrack' attribute.
> > +*/
> > + rtx_insn *last = get_last_insn ();
> > + while (!CALL_P (last)
> > + && (last != before_call))
> > + last = PREV_INSN (last);
> > +
> > + if (last != before_call
> > + && gimple_call_with_notrack_p (stmt))
> > + add_reg_note (last, REG_CALL_NOTRACK, const0_rtx);
> > +
> > mark_transaction_restart_calls (stmt); }
> Just to be sure -- this is only searching through the currently open sequence
> to find the RTL call emitted into the sequence to implement the gimple call
> we're expanding, right?
>
> ISTM that you don't need to do the search if !gimple_call_with_notrack_p,
> right? Which would imply this code should look something like
>
> if (gimple_call_with_notrack_p (stmt))
> {
> rtx insn *last = get_last_insn ();
> while (!CALL_P (last)
> && last != before_call)
> last = PREV_INSN (last);
> add_reg_note (last, REG_CALL_NOTRACK, const0_rtx)
> }
>
> Or something close to that?
>
>
> I'd be slightly concerned if expansion of an argument resulted in a call. I'm
> not sure if that can happen anymore, but if it does, then you could end up
> marking the wrong CALL_INSN.
>
> THe only cases where I think that might be possible would be if we needed
> to emit a libcall or memcpy. So perhaps you want to verify that last is a
> call
> *and* that it's an indirect call.
I've rewrote the code as you suggested to check if gimple call is an indirect
call and is 'noverify_cf' call.
>
> > diff --git a/gcc/gimple.c b/gcc/gimple.c index 479f90c..2e4ab2d 100644
> > --- a/gcc/gimple.c
> > +++ b/gcc/gimple.c
> > @@ -378,6 +378,23 @@ gimple_build_call_from_tree (tree t)
> > gimple_set_no_warning (call, TREE_NO_WARNING (t));
> > gimple_call_set_with_bounds (call, CALL_WITH_BOUNDS_P (t));
> >
> > + if (fndecl == NULL_TREE)
> > + {
> > + /* Find the type of an indirect call. */
> > + tree addr = CALL_EXPR_FN (t);
> > + if (TREE_CODE (addr) != FUNCTION_DECL)
> > + {
> > + tree fntype = TREE_TYPE (addr);
> > + gcc_assert (POINTER_TYPE_P (fntype));
> > + fntype = TREE_TYPE (fntype);
> > +
> > + /* Check if its type has the no-track attribute and propagate
> > + it to the CALL insn. */
> > + if (lookup_attribute ("notrack", TYPE_ATTRIBUTES (fntype)))
> > + gimple_call_set_with_notrack (call, TRUE);
> > + }
> > + }
> > +
> > return call;
> As Richi noted, this deserves a better comment.
>
> It may even be advisable to build a predicate to test if a given gimple
> statement is an indirect call and use that rather than fndecl == NULL_TREE.
> That would help make this code clearer as well.
I've rewrote the code above according to Richard's suggestion: second parameter
was introduced
and a proper type is passed to the function to look if it has the required
attribute.
> Q. Do we need to do anything with ICF (identical code folding) and CFE?
> Given two functions which have the same implementation in gimple, except
> that one has a notrack indirect call and the other has a tracked indirect
> call,
> what is proper behavior? I think we'd keep them separate which implies we
> need to make sure the notrack attribute is part of the ICF hashing
> implementation. It'd probably even be worth building a test for this :-)
Are you talking about a case when such two functions are inlined? Or there is a
possibility to merge
function bodies if they are identical?
I agree with you that the functions should be kept separate. I haven't looked
into such optimization
in gcc so I need to learn it.
> > }
> >
> >
> > +/* Return true if call GS is marked as no-track. */
> > +
> > +static inline bool
> > +gimple_call_with_notrack_p (const gcall *gs) {
> > + return (gs->subcode & GF_CALL_WITH_NOTRACK) != 0; }
> > +
> > +static inline bool
> > +gimple_call_with_notrack_p (const gimple *gs) {
> > + const gcall *gc = GIMPLE_CHECK2<const gcall *> (gs);
> > + return gimple_call_with_notrack_p (gc); }
> Agree with Richi WRT avoiding gimple * overloads.
Fixed.
Thanks,
Igor
>
>
> Jeff
