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? 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. > > + 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.