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
> 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?
> >
> > 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.
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.
You didn't answer my question about "what about C++"?
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
> >>
>
