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: [PATCH 1/7] Add __builtin_speculation_safe_value


On Wed, Jul 25, 2018 at 2:41 PM Richard Earnshaw (lists)
<Richard.Earnshaw@arm.com> wrote:
>
> On 25/07/18 11:36, Richard Biener wrote:
> > On Wed, Jul 25, 2018 at 11:49 AM Richard Earnshaw (lists)
> > <Richard.Earnshaw@arm.com> wrote:
> >>
> >> On 24/07/18 18:26, Richard Biener wrote:
> >>> On Mon, Jul 9, 2018 at 6:40 PM Richard Earnshaw
> >>> <Richard.Earnshaw@arm.com> wrote:
> >>>>
> >>>>
> >>>> This patch defines a new intrinsic function
> >>>> __builtin_speculation_safe_value.  A generic default implementation is
> >>>> defined which will attempt to use the backend pattern
> >>>> "speculation_safe_barrier".  If this pattern is not defined, or if it
> >>>> is not available, then the compiler will emit a warning, but
> >>>> compilation will continue.
> >>>>
> >>>> Note that the test spec-barrier-1.c will currently fail on all
> >>>> targets.  This is deliberate, the failure will go away when
> >>>> appropriate action is taken for each target backend.
> >>>
> >>> So given this series is supposed to be backported I question
> >>>
> >>> +rtx
> >>> +default_speculation_safe_value (machine_mode mode ATTRIBUTE_UNUSED,
> >>> +                               rtx result, rtx val,
> >>> +                               rtx failval ATTRIBUTE_UNUSED)
> >>> +{
> >>> +  emit_move_insn (result, val);
> >>> +#ifdef HAVE_speculation_barrier
> >>> +  /* Assume the target knows what it is doing: if it defines a
> >>> +     speculation barrier, but it is not enabled, then assume that one
> >>> +     isn't needed.  */
> >>> +  if (HAVE_speculation_barrier)
> >>> +    emit_insn (gen_speculation_barrier ());
> >>> +
> >>> +#else
> >>> +  warning_at (input_location, 0,
> >>> +             "this target does not define a speculation barrier; "
> >>> +             "your program will still execute correctly, but speculation "
> >>> +             "will not be inhibited");
> >>> +#endif
> >>> +  return result;
> >>>
> >>> which makes all but aarch64 archs warn on __bultin_speculation_safe_value
> >>> uses, even those that do not suffer from Spectre like all those embedded targets
> >>> where implementations usually do not speculate at all.
> >>>
> >>> In fact for those targets the builtin stays in the way of optimization on GIMPLE
> >>> as well so we should fold it away early if neither the target hook is
> >>> implemented
> >>> nor there is a speculation_barrier insn.
> >>>
> >>> So, please make resolve_overloaded_builtin return a no-op on such targets
> >>> which means you can remove the above warning.  Maybe such targets
> >>> shouldn't advertise / initialize the builtins at all?
> >>
> >> I disagree with your approach here.  Why would users not want to know
> >> when the compiler is failing to implement a security feature when it
> >> should?  As for targets that don't need something, they can easily
> >> define the hook as described to suppress the warning.
> >>
> >> Or are you just suggesting moving the warning to resolve overloaded builtin.
> >
> > Well.  You could argue I say we shouldn't even support
> > __builtin_sepeculation_safe_value
> > for archs that do not need it or have it not implemented.  That way users can
> > decide:
> >
> > #if __HAVE_SPECULATION_SAFE_VALUE
> >  ....
> > #else
> > #warning oops // or nothing
> > #endif
> >
>
> So how about removing the predefine of __HAVE_S_S_V when the builtin is
> a nop, but then leaving the warning in if people try to use it anyway?

Little bit inconsistent but I guess I could live with that.  It still leaves
the question open for how to declare you do not need speculation
barriers at all then.

> >> Other ports will need to take action, but in general, it can be as
> >> simple as, eg patch 2 or 3 do for the Arm and AArch64 backends - or
> >> simpler still if nothing is needed for that architecture.
> >
> > Then that should be the default.  You might argue we'll only see
> > __builtin_speculation_safe_value uses for things like Firefox which
> > is unlikely built for AVR (just to make an example).  But people
> > are going to test build just on x86 and if they build with -Werror
> > this will break builds on all targets that didn't even get the chance
> > to implement this feature.
> >
> >> There is a test which is intended to fail to targets that have not yet
> >> been patched - I thought that was better than hard-failing the build,
> >> especially given that we want to back-port.
> >>
> >> Port maintainers DO need to decide what to do about speculation, even if
> >> it is explicitly that no mitigation is needed.
> >
> > Agreed.  But I didn't yet see a request for maintainers to decide that?
> >
>
> consider it made, then :-)

I suspect that drew their attention ;)

So a different idea would be to produce patches implementing the hook for
each target "empty", CC the target maintainers and hope they quickly
ack if the target doesn't have a speculation problem.  Others then would
get no patch (from you) and thus raise a warning?

Maybe at least do that for all primary and secondary targets given we do
not want to regress diagnostic-wise (not get new _false_-positives) on
the branch.

> >>>
> >>> The builtins also have no attributes which mean they are assumed to be
> >>> 1) calling back into the CU via exported functions, 2) possibly throwing
> >>> exceptions, 3) affecting memory state.  I think you at least want
> >>> to use ATTR_NOTHROW_LEAF_LIST.
> >>>
> >>> The builtins are not designed to be optimization or memory barriers as
> >>> far as I can see and should thus be CONST as well.
> >>>
> >>
> >> I think they should be barriers.  They do need to ensure that they can't
> >> be moved past other operations that might depend on the speculation
> >> state.  Consider, for example,
> >
> > That makes eliding them for targets that do not need mitigation even
> > more important.
> >
> >>  ...
> >>  t = untrusted_value;
> >>  ...
> >>  if (t + 5 < limit)
> >>  {
> >>    v = mem[__builtin_speculation_safe_value (untrusted_value)];
> >>    ...
> >>
> >> The compiler must never lift the builtin outside the bounds check as
> >> that is part of the speculation state.
> >
> > OK, so you are relying on the fact that with the current setup GCC has
> > to assume the builtin has side-effects (GCC may not move it to a place that
> > the original location is not post-dominated on).  It doesn't explain
> > why you cannot set ECF_LEAF or why the builtin needs to be
> > considered affecting the memory state.  That is, ECF_NOVOPS
> > or ECF_LOOPING_CONST_OR_PURE (I don't think you can
> > set that manually) would work here, both keep the builtin as
> > having side-effects.
> >
>
> I wish some of this builtin gloop were better documented; at present you
> have to reverse engineer significant amounts of code just to decide
> whether or not you even have to think about whether or not it's relevant...
>
>
> > Btw, if you have an inline function with a pattern like above and
> > you use it multiple times in a row GCC should be able to
> > optimize this?  That is, optimizations like jump-threading also
> > affect the speculation state by modifying the controling
> > conditions.
>
> Ideally, if there's no control flow change, yes.  As soon as you insert
> another branch (in or out) then you might have another speculation path
> to consider.  Not sure how that can easily merging could be done, though.

The usual case would be

  if (cond)
   ... _b_s_s_v (x);
<code>
  if (cond)
    ... _b_s_s_v (x);

where jump-threading might decide to make that to

  if (cond)
   {
     ... _b_s_s_v (x);
     <copy of code>
     ... _b_s_s_v (x);
   }

now we might even be able to CSE the 2nd _b_s_s_v (x)
to the first?  That would mean using ECF_CONST|ECF_LOOPING_PURE_OR_CONST
is the best (but we currently have no attribute for the latter).

> >
> > You didn't answer my question about "what about C++"?
> >
>
> It didn't need a response at this point.  It's a reasonable one, as are
> some of your others...  I was focusing on the the comments that were
> potentially contentious.
>
> BTW, this bit:
>
> +    case BUILT_IN_SPECULATION_SAFE_VALUE_N:
> +      {
> +       int n = speculation_safe_value_resolve_size (function, params);
> +       tree new_function, first_param, result;
> +       enum built_in_function fncode;
> +
> +       if (n == -1)
> +         return error_mark_node;
> +       else if (n == 0)
> +         fncode = (enum built_in_function)((int)orig_code + 1);
> +       else
> +         fncode
> +           = (enum built_in_function)((int)orig_code + exact_log2 (n) + 2);
>
> > resolve_size does that?  Why can that not return the built_in_function
> > itself or BUILT_IN_NONE on error to make that clearer?
>
> is essentially a clone of some existing code that already does it this
> way.  See BUILT_IN_SYNC_LOCK_RELEASE_N etc.  Admittedly, that hunk
> handles multiple origins so would be harder to rewrite as you suggest;
> it just seemed more appropriate to handle the cases similarly.

Yes, I realized you copied handling from that so I didn't look too closely...

These days we'd probably use an internal-function and spare us all
the resolving completely (besides a test for validity) ;)

Richard.

> R.
>
> > Richard.
> >
> >>
> >>
> >>> BUILT_IN_SPECULATION_SAFE_VALUE_PTR is declared but
> >>> nowhere generated?  Maybe
> >>>
> >>> +    case BUILT_IN_SPECULATION_SAFE_VALUE_N:
> >>> +      {
> >>> +       int n = speculation_safe_value_resolve_size (function, params);
> >>> +       tree new_function, first_param, result;
> >>> +       enum built_in_function fncode;
> >>> +
> >>> +       if (n == -1)
> >>> +         return error_mark_node;
> >>> +       else if (n == 0)
> >>> +         fncode = (enum built_in_function)((int)orig_code + 1);
> >>> +       else
> >>> +         fncode
> >>> +           = (enum built_in_function)((int)orig_code + exact_log2 (n) + 2);
> >>>
> >>> resolve_size does that?  Why can that not return the built_in_function
> >>> itself or BUILT_IN_NONE on error to make that clearer?
> >>>
> >>> Otherwise it looks reasonable but C FE maintainers should comment.
> >>> I miss C++ testcases (or rather testcases should be in c-c++-common).
> >>>
> >>> Richard.
> >>>
> >>>> gcc:
> >>>>         * builtin-types.def (BT_FN_PTR_PTR_VAR): New function type.
> >>>>         (BT_FN_I1_I1_VAR, BT_FN_I2_I2_VAR, BT_FN_I4_I4_VAR): Likewise.
> >>>>         (BT_FN_I8_I8_VAR, BT_FN_I16_I16_VAR): Likewise.
> >>>>         * builtins.def (BUILT_IN_SPECULATION_SAFE_VALUE_N): New builtin.
> >>>>         (BUILT_IN_SPECULATION_SAFE_VALUE_PTR): New internal builtin.
> >>>>         (BUILT_IN_SPECULATION_SAFE_VALUE_1): Likewise.
> >>>>         (BUILT_IN_SPECULATION_SAFE_VALUE_2): Likewise.
> >>>>         (BUILT_IN_SPECULATION_SAFE_VALUE_4): Likewise.
> >>>>         (BUILT_IN_SPECULATION_SAFE_VALUE_8): Likewise.
> >>>>         (BUILT_IN_SPECULATION_SAFE_VALUE_16): Likewise.
> >>>>         * builtins.c (expand_speculation_safe_value): New function.
> >>>>         (expand_builtin): Call it.
> >>>>         * doc/cpp.texi: Document predefine __HAVE_SPECULATION_SAFE_VALUE.
> >>>>         * doc/extend.texi: Document __builtin_speculation_safe_value.
> >>>>         * doc/md.texi: Document "speculation_barrier" pattern.
> >>>>         * doc/tm.texi.in: Pull in TARGET_SPECULATION_SAFE_VALUE.
> >>>>         * doc/tm.texi: Regenerated.
> >>>>         * target.def (speculation_safe_value): New hook.
> >>>>         * targhooks.c (default_speculation_safe_value): New function.
> >>>>         * targhooks.h (default_speculation_safe_value): Add prototype.
> >>>>
> >>>> c-family:
> >>>>         * c-common.c (speculation_safe_resolve_size): New function.
> >>>>         (speculation_safe_resolve_params): New function.
> >>>>         (speculation_safe_resolve_return): New function.
> >>>>         (resolve_overloaded_builtin): Handle __builtin_speculation_safe_value.
> >>>>         * c-cppbuiltin.c (c_cpp_builtins): Add pre-define for
> >>>>         __HAVE_SPECULATION_SAFE_VALUE.
> >>>>
> >>>> testsuite:
> >>>>         * gcc.dg/spec-barrier-1.c: New test.
> >>>>         * gcc.dg/spec-barrier-2.c: New test.
> >>>>         * gcc.dg/spec-barrier-3.c: New test.
> >>>> ---
> >>>>  gcc/builtin-types.def                 |   6 ++
> >>>>  gcc/builtins.c                        |  57 ++++++++++++++
> >>>>  gcc/builtins.def                      |  20 +++++
> >>>>  gcc/c-family/c-common.c               | 143 ++++++++++++++++++++++++++++++++++
> >>>>  gcc/c-family/c-cppbuiltin.c           |   5 +-
> >>>>  gcc/doc/cpp.texi                      |   4 +
> >>>>  gcc/doc/extend.texi                   |  29 +++++++
> >>>>  gcc/doc/md.texi                       |  15 ++++
> >>>>  gcc/doc/tm.texi                       |  20 +++++
> >>>>  gcc/doc/tm.texi.in                    |   2 +
> >>>>  gcc/target.def                        |  23 ++++++
> >>>>  gcc/targhooks.c                       |  27 +++++++
> >>>>  gcc/targhooks.h                       |   2 +
> >>>>  gcc/testsuite/gcc.dg/spec-barrier-1.c |  40 ++++++++++
> >>>>  gcc/testsuite/gcc.dg/spec-barrier-2.c |  19 +++++
> >>>>  gcc/testsuite/gcc.dg/spec-barrier-3.c |  13 ++++
> >>>>  16 files changed, 424 insertions(+), 1 deletion(-)
> >>>>  create mode 100644 gcc/testsuite/gcc.dg/spec-barrier-1.c
> >>>>  create mode 100644 gcc/testsuite/gcc.dg/spec-barrier-2.c
> >>>>  create mode 100644 gcc/testsuite/gcc.dg/spec-barrier-3.c
> >>>>
> >>
>


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