On Thu, Jul 26, 2018 at 3:06 PM Richard Earnshaw (lists)
<richard.earns...@arm.com> wrote:
>
> On 26/07/18 13:41, Richard Biener wrote:
> > 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.
>
> This is an optimization right?
So was adding LEAF and NOTHROW.
> If so, can I defer that to a follow-up
> patch? Although the builtin doesn't touch memory, I need think very
> carefully about whether or not it is safe to move other memory ops
> across it.
Well. We cannot even DCE the thing if there is no use left of the result...
NOVOPS doesn't change that - we're still telling GCC that the builtin
has arbitrary non-memory side-effects. But that's necessary to prevent
moving it to places where it wasn't executed before.
Throwing additional wrenches into the machiner in terms of making
the compiler think it has changes on escaped(!) memory state doesn't help,
it will just hide bugs. Being not NOVOPS doesn't mean it is a
barrer for every memory access.
Richard.
> >
> > + 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."
>
> Thanks, I like that wording better than mine.
>
> R.
>
> >
> > 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.
>
