On Thu, Jul 26, 2018 at 12:03 PM Richard Earnshaw (lists)
<richard.earns...@arm.com> wrote:
>
> On 25/07/18 14:47, Richard Biener wrote:
> > On Wed, Jul 25, 2018 at 2:41 PM Richard Earnshaw (lists)
> > <richard.earns...@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.earns...@arm.com> wrote:
> >>>>
> >>>> On 24/07/18 18:26, Richard Biener wrote:
> >>>>> On Mon, Jul 9, 2018 at 6:40 PM Richard Earnshaw
> >>>>> <richard.earns...@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
> >>>>>>
> >>>>
> >>
>
> Here's an updated version of this patch, based on these discussions.
> Notable changes since last time:
> - __HAVE_SPECULATION_SAFE_VALUE is now only defined if the target has
> been updated for this feature.
> - Warnings are only issued if the builtin is used when
> __HAVE_SPECULATION_SAFE_VALUE is not defined (so the builtin will always
> generate a workable program, it just might not be protected in this case).
> - Some of the tests moved to c-c++-common to improve C++ testing.
> - The builtin is elided early on targets that do not need, or do not
> provide a specific means to restrict speculative execution.
>
> A full bootstrap has completed, but tests are still running.
Please make the builtins NOVOPS as well by adding
DEF_ATTR_TREE_LIST (ATTR_NOVOPS_NOTHROW_LEAF_LIST, ATTR_NOVOPS,
ATTR_NULL, ATTR_NOTHROW_LEAF_LIST)
to builtin-attrs.def and using that.
+ The default implementation returns true for the first case if the target
+ defines a pattern named @code{speculation_barrier}; for the second case
+ and if the pattern is enabled for the current compilation.
+@end deftypefn
I do not understand the last sentence. I suspect it shold be
"The default implementation returns false if the target does not define
a pattern named @code{speculation_barrier}. Else it returns true
for the first case and whether the pattern is enabled for the current
compilation for the second case."
Otherwise the middle-end changes look OK to me. The c-family changes
need review by a appropriate maintainer still.
Thanks,
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 and
> TARGET_HAVE_SPECULATION_SAFE_VALUE.
> * doc/tm.texi: Regenerated.
> * target.def (have_speculation_safe_value, speculation_safe_value):
> New
> hooks.
> * targhooks.c (default_have_speculation_safe_value): New function.
> (default_speculation_safe_value): New function.
> * targhooks.h (default_have_speculation_safe_value): Add prototype.
> (default_speculation_safe_value): Add prototype.
>
> c-family:
> * c-common.c (speculation_safe_resolve_call): 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:
> * c-c++-common/spec-barrier-1.c: New test.
> * c-c++-common/spec-barrier-2.c: New test.
> * gcc.dg/spec-barrier-3.c: New test.
