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 > >>>> > >> >