| Message ID | 1531154299-28349-2-git-send-email-Richard.Earnshaw@arm.com |
|---|---|
| State | New |
| Headers | show |
| Series | Mitigation against unsafe data speculation (CVE-2017-5753) | expand |
Ping. This patch needs reviewing an appropriate maintainer. I'm aware that the documentation needs extending a bit to provide some example use cases as discussed in the follow-up to the generic discussion, but the rest of the patch still stands. R. On 09/07/18 17:38, Richard Earnshaw 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. > > 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 > > > 0001-Add-__builtin_speculation_safe_value.patch > > > diff --git a/gcc/builtin-types.def b/gcc/builtin-types.def > index b01095c..70fae35 100644 > --- a/gcc/builtin-types.def > +++ b/gcc/builtin-types.def > @@ -763,6 +763,12 @@ DEF_FUNCTION_TYPE_VAR_1 (BT_FN_VOID_LONG_VAR, > BT_VOID, BT_LONG) > DEF_FUNCTION_TYPE_VAR_1 (BT_FN_VOID_ULL_VAR, > BT_VOID, BT_ULONGLONG) > +DEF_FUNCTION_TYPE_VAR_1 (BT_FN_PTR_PTR_VAR, BT_PTR, BT_PTR) > +DEF_FUNCTION_TYPE_VAR_1 (BT_FN_I1_I1_VAR, BT_I1, BT_I1) > +DEF_FUNCTION_TYPE_VAR_1 (BT_FN_I2_I2_VAR, BT_I2, BT_I2) > +DEF_FUNCTION_TYPE_VAR_1 (BT_FN_I4_I4_VAR, BT_I4, BT_I4) > +DEF_FUNCTION_TYPE_VAR_1 (BT_FN_I8_I8_VAR, BT_I8, BT_I8) > +DEF_FUNCTION_TYPE_VAR_1 (BT_FN_I16_I16_VAR, BT_I16, BT_I16) > > DEF_FUNCTION_TYPE_VAR_2 (BT_FN_INT_FILEPTR_CONST_STRING_VAR, > BT_INT, BT_FILEPTR, BT_CONST_STRING) > diff --git a/gcc/builtins.c b/gcc/builtins.c > index 91658e8..9f97ecf 100644 > --- a/gcc/builtins.c > +++ b/gcc/builtins.c > @@ -6716,6 +6716,52 @@ expand_builtin_goacc_parlevel_id_size (tree exp, rtx target, int ignore) > return target; > } > > +/* Expand a call to __builtin_speculation_safe_value_<N>. MODE > + represents the size of the first argument to that call, or VOIDmode > + if the argument is a pointer. IGNORE will be true if the result > + isn't used. */ > +static rtx > +expand_speculation_safe_value (machine_mode mode, tree exp, rtx target, > + bool ignore) > +{ > + rtx val, failsafe; > + unsigned nargs = call_expr_nargs (exp); > + > + tree arg0 = CALL_EXPR_ARG (exp, 0); > + > + if (mode == VOIDmode) > + { > + mode = TYPE_MODE (TREE_TYPE (arg0)); > + gcc_assert (GET_MODE_CLASS (mode) == MODE_INT); > + } > + > + val = expand_expr (arg0, NULL_RTX, mode, EXPAND_NORMAL); > + > + /* An optional second argument can be used as a failsafe value on > + some machines. If it isn't present, then the failsafe value is > + assumed to be 0. */ > + if (nargs > 1) > + { > + tree arg1 = CALL_EXPR_ARG (exp, 1); > + failsafe = expand_expr (arg1, NULL_RTX, mode, EXPAND_NORMAL); > + } > + else > + failsafe = const0_rtx; > + > + /* If the result isn't used, the behavior is undefined. It would be > + nice to emit a warning here, but path splitting means this might > + happen with legitimate code. So simply drop the builtin > + expansion in that case; we've handled any side-effects above. */ > + if (ignore) > + return const0_rtx; > + > + /* If we don't have a suitable target, create one to hold the result. */ > + if (target == NULL) > + target = gen_reg_rtx (mode); > + > + return targetm.speculation_safe_value (mode, target, val, failsafe); > +} > + > /* Expand an expression EXP that calls a built-in function, > with result going to TARGET if that's convenient > (and in mode MODE if that's convenient). > @@ -7827,6 +7873,17 @@ expand_builtin (tree exp, rtx target, rtx subtarget, machine_mode mode, > case BUILT_IN_GOACC_PARLEVEL_SIZE: > return expand_builtin_goacc_parlevel_id_size (exp, target, ignore); > > + case BUILT_IN_SPECULATION_SAFE_VALUE_PTR: > + return expand_speculation_safe_value (VOIDmode, exp, target, ignore); > + > + case BUILT_IN_SPECULATION_SAFE_VALUE_1: > + case BUILT_IN_SPECULATION_SAFE_VALUE_2: > + case BUILT_IN_SPECULATION_SAFE_VALUE_4: > + case BUILT_IN_SPECULATION_SAFE_VALUE_8: > + case BUILT_IN_SPECULATION_SAFE_VALUE_16: > + mode = get_builtin_sync_mode (fcode - BUILT_IN_SPECULATION_SAFE_VALUE_1); > + return expand_speculation_safe_value (mode, exp, target, ignore); > + > default: /* just do library call, if unknown builtin */ > break; > } > diff --git a/gcc/builtins.def b/gcc/builtins.def > index aacbd51..b71d89c 100644 > --- a/gcc/builtins.def > +++ b/gcc/builtins.def > @@ -1003,6 +1003,26 @@ DEF_BUILTIN (BUILT_IN_EMUTLS_REGISTER_COMMON, > true, true, true, ATTR_NOTHROW_LEAF_LIST, false, > !targetm.have_tls) > > +/* Suppressing speculation. Users are expected to use the first (N) > + variant, which will be translated internally into one of the other > + types. */ > + > +DEF_GCC_BUILTIN (BUILT_IN_SPECULATION_SAFE_VALUE_N, "speculation_safe_value", > + BT_FN_VOID_VAR, ATTR_NULL) > + > +DEF_GCC_BUILTIN (BUILT_IN_SPECULATION_SAFE_VALUE_PTR, > + "speculation_safe_value_ptr", BT_FN_PTR_PTR_VAR, ATTR_NULL) > +DEF_GCC_BUILTIN (BUILT_IN_SPECULATION_SAFE_VALUE_1, "speculation_safe_value_1", > + BT_FN_I1_I1_VAR, ATTR_NULL) > +DEF_GCC_BUILTIN (BUILT_IN_SPECULATION_SAFE_VALUE_2, "speculation_safe_value_2", > + BT_FN_I2_I2_VAR, ATTR_NULL) > +DEF_GCC_BUILTIN (BUILT_IN_SPECULATION_SAFE_VALUE_4, "speculation_safe_value_4", > + BT_FN_I4_I4_VAR, ATTR_NULL) > +DEF_GCC_BUILTIN (BUILT_IN_SPECULATION_SAFE_VALUE_8, "speculation_safe_value_8", > + BT_FN_I8_I8_VAR, ATTR_NULL) > +DEF_GCC_BUILTIN (BUILT_IN_SPECULATION_SAFE_VALUE_16, > + "speculation_safe_value_16", BT_FN_I16_I16_VAR, ATTR_NULL) > + > /* Exception support. */ > DEF_BUILTIN_STUB (BUILT_IN_UNWIND_RESUME, "__builtin_unwind_resume") > DEF_BUILTIN_STUB (BUILT_IN_CXA_END_CLEANUP, "__builtin_cxa_end_cleanup") > diff --git a/gcc/c-family/c-common.c b/gcc/c-family/c-common.c > index f5e1111..32a2de2 100644 > --- a/gcc/c-family/c-common.c > +++ b/gcc/c-family/c-common.c > @@ -6457,6 +6457,121 @@ builtin_type_for_size (int size, bool unsignedp) > return type ? type : error_mark_node; > } > > +/* Work out the size of the first argument of a call to > + __builtin_speculation_safe_value. Only pointers and integral types > + are permitted. Return -1 if the argument type is not supported or > + the size is too large; 0 if the argument type is a pointer or the > + size if it is integral. */ > +static int > +speculation_safe_value_resolve_size (tree function, vec<tree, va_gc> *params) > +{ > + /* Type of the argument. */ > + tree type; > + int size; > + > + if (vec_safe_is_empty (params)) > + { > + error ("too few arguments to function %qE", function); > + return -1; > + } > + > + type = TREE_TYPE ((*params)[0]); > + if (TREE_CODE (type) == ARRAY_TYPE && c_dialect_cxx ()) > + { > + /* Force array-to-pointer decay for C++. */ > + (*params)[0] = default_conversion ((*params)[0]); > + type = TREE_TYPE ((*params)[0]); > + } > + > + if (POINTER_TYPE_P (type)) > + return 0; > + > + if (!INTEGRAL_TYPE_P (type)) > + goto incompatible; > + > + if (!COMPLETE_TYPE_P (type)) > + goto incompatible; > + > + size = tree_to_uhwi (TYPE_SIZE_UNIT (type)); > + if (size == 1 || size == 2 || size == 4 || size == 8 || size == 16) > + return size; > + > + incompatible: > + /* Issue the diagnostic only if the argument is valid, otherwise > + it would be redundant at best and could be misleading. */ > + if (type != error_mark_node) > + error ("operand type %qT is incompatible with argument %d of %qE", > + type, 1, function); > + > + return -1; > +} > + > +/* Validate and coerce PARAMS, the arguments to ORIG_FUNCTION to fit > + the prototype for FUNCTION. The first argument is mandatory, a second > + argument, if present, must be type compatible with the first. */ > +static bool > +speculation_safe_value_resolve_params (location_t loc, tree orig_function, > + vec<tree, va_gc> *params) > +{ > + tree val; > + > + if (params->length () == 0) > + { > + error_at (loc, "too few arguments to function %qE", orig_function); > + return false; > + } > + > + else if (params->length () > 2) > + { > + error_at (loc, "too many arguments to function %qE", orig_function); > + return false; > + } > + > + val = (*params)[0]; > + if (TREE_CODE (TREE_TYPE (val)) == ARRAY_TYPE) > + val = default_conversion (val); > + if (!(TREE_CODE (TREE_TYPE (val)) == POINTER_TYPE > + || TREE_CODE (TREE_TYPE (val)) == INTEGER_TYPE)) > + { > + error_at (loc, > + "expecting argument of type pointer or of type integer " > + "for argument 1"); > + return false; > + } > + (*params)[0] = val; > + > + if (params->length () == 2) > + { > + tree val2 = (*params)[1]; > + if (TREE_CODE (TREE_TYPE (val2)) == ARRAY_TYPE) > + val2 = default_conversion (val2); > + if (!(TREE_TYPE (val) == TREE_TYPE (val2) > + || useless_type_conversion_p (TREE_TYPE (val), TREE_TYPE (val2)))) > + { > + error_at (loc, "both arguments must be compatible"); > + return false; > + } > + (*params)[1] = val2; > + } > + > + return true; > +} > + > +/* Cast the result of the builtin back to the type of the first argument, > + preserving any qualifiers that it might have. */ > +static tree > +speculation_safe_value_resolve_return (tree first_param, tree result) > +{ > + tree ptype = TREE_TYPE (first_param); > + tree rtype = TREE_TYPE (result); > + ptype = TYPE_MAIN_VARIANT (ptype); > + > + if (tree_int_cst_equal (TYPE_SIZE (ptype), TYPE_SIZE (rtype))) > + return convert (ptype, result); > + > + return result; > +} > + > /* A helper function for resolve_overloaded_builtin in resolving the > overloaded __sync_ builtins. Returns a positive power of 2 if the > first operand of PARAMS is a pointer to a supported data type. > @@ -7111,6 +7226,34 @@ resolve_overloaded_builtin (location_t loc, tree function, > /* Handle BUILT_IN_NORMAL here. */ > switch (orig_code) > { > + 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); > + > + new_function = builtin_decl_explicit (fncode); > + first_param = (*params)[0]; > + if (!speculation_safe_value_resolve_params (loc, function, params)) > + return error_mark_node; > + > + result = build_function_call_vec (loc, vNULL, new_function, params, > + NULL); > + > + if (result == error_mark_node) > + return result; > + > + return speculation_safe_value_resolve_return (first_param, result); > + } > + > case BUILT_IN_ATOMIC_EXCHANGE: > case BUILT_IN_ATOMIC_COMPARE_EXCHANGE: > case BUILT_IN_ATOMIC_LOAD: > diff --git a/gcc/c-family/c-cppbuiltin.c b/gcc/c-family/c-cppbuiltin.c > index bdb5691..0b10e65 100644 > --- a/gcc/c-family/c-cppbuiltin.c > +++ b/gcc/c-family/c-cppbuiltin.c > @@ -1361,7 +1361,10 @@ c_cpp_builtins (cpp_reader *pfile) > cpp_define (pfile, "__WCHAR_UNSIGNED__"); > > cpp_atomic_builtins (pfile); > - > + > + /* Show support for __builtin_speculation_safe_value (). */ > + cpp_define (pfile, "__HAVE_SPECULATION_SAFE_VALUE"); > + > #ifdef DWARF2_UNWIND_INFO > if (dwarf2out_do_cfi_asm ()) > cpp_define (pfile, "__GCC_HAVE_DWARF2_CFI_ASM"); > diff --git a/gcc/doc/cpp.texi b/gcc/doc/cpp.texi > index 3f7a8fc..efad2c8 100644 > --- a/gcc/doc/cpp.texi > +++ b/gcc/doc/cpp.texi > @@ -2381,6 +2381,10 @@ If GCC cannot determine the current date, it will emit a warning message > These macros are defined when the target processor supports atomic compare > and swap operations on operands 1, 2, 4, 8 or 16 bytes in length, respectively. > > +@item __HAVE_SPECULATION_SAFE_VALUE > +This macro is defined with the value 1 to show that this version of GCC > +supports @code{__builtin_speculation_safe_value}. > + > @item __GCC_HAVE_DWARF2_CFI_ASM > This macro is defined when the compiler is emitting DWARF CFI directives > to the assembler. When this is defined, it is possible to emit those same > diff --git a/gcc/doc/extend.texi b/gcc/doc/extend.texi > index c7745c4..6eb0c6b 100644 > --- a/gcc/doc/extend.texi > +++ b/gcc/doc/extend.texi > @@ -10935,6 +10935,7 @@ is called and the @var{flag} argument passed to it. > @findex __builtin_powi > @findex __builtin_powif > @findex __builtin_powil > +@findex __builtin_speculation_safe_value > @findex _Exit > @findex _exit > @findex abort > @@ -11579,6 +11580,34 @@ check its compatibility with @var{size}. > > @end deftypefn > > +@deftypefn {Built-in Function} @var{type} __builtin_speculation_safe_value (@var{type} val, @var{type} failval) > + > +This builtin can be used to help mitigate against unsafe speculative > +execution. @var{type} may be any integral type or any pointer type. > + > +@enumerate > +@item > +If the CPU is not speculatively executing the code, then @var{val} > +is returned. > +@item > +If the CPU is executing speculatively then either: > +@itemize > +@item > +The function may cause execution to pause until it is known that the > +code is no-longer being executed speculatively (in which case > +@var{val} can be returned, as above); or > +@item > +The function may use target-dependent speculation tracking state to cause > +@var{failval} to be returned when it is known that speculative > +execution has incorrectly predicted a conditional branch operation. > +@end itemize > +@end enumerate > + > +The second argument, @var{failval}, is optional and defaults to zero > +if omitted. > + > +@end deftypefn > + > @deftypefn {Built-in Function} int __builtin_types_compatible_p (@var{type1}, @var{type2}) > > You can use the built-in function @code{__builtin_types_compatible_p} to > diff --git a/gcc/doc/md.texi b/gcc/doc/md.texi > index 6d15d99..5de27f6 100644 > --- a/gcc/doc/md.texi > +++ b/gcc/doc/md.texi > @@ -7026,6 +7026,21 @@ should be defined to an instruction that orders both loads and stores > before the instruction with respect to loads and stores after the instruction. > This pattern has no operands. > > +@cindex @code{speculation_barrier} instruction pattern > +@item @samp{speculation_barrier} > +If the target can support speculative execution, then this pattern should > +be defined to an instruction that will block subsequent execution until > +any prior speculation conditions has been resolved. The pattern must also > +ensure that the compiler cannot move memory operations past the barrier, > +so it needs to be an UNSPEC_VOLATILE pattern. The pattern has no > +operands. > + > +If this pattern is not defined then the default expansion of > +@code{__builtin_speculation_safe_value} will emit a warning. You can > +suppress this warning by defining this pattern with a final condition > +of @code{0} (zero), which tells the compiler that a speculation > +barrier is not needed for this target. > + > @cindex @code{sync_compare_and_swap@var{mode}} instruction pattern > @item @samp{sync_compare_and_swap@var{mode}} > This pattern, if defined, emits code for an atomic compare-and-swap > diff --git a/gcc/doc/tm.texi b/gcc/doc/tm.texi > index 7e2cdc2..681e53b 100644 > --- a/gcc/doc/tm.texi > +++ b/gcc/doc/tm.texi > @@ -11932,6 +11932,26 @@ maintainer is familiar with. > > @end defmac > > +@deftypefn {Target Hook} rtx TARGET_SPECULATION_SAFE_VALUE (machine_mode @var{mode}, rtx @var{result}, rtx @var{val}, rtx @var{failval}) > +This target hook can be used to generate a target-specific code > + sequence that implements the @code{__builtin_speculation_safe_value} > + built-in function. The function must always return @var{val} in > + @var{result} in mode @var{mode} when the cpu is not executing > + speculatively, but must never return that when speculating until it > + is known that the speculation will not be unwound. The hook supports > + two primary mechanisms for implementing the requirements. The first > + is to emit a speculation barrier which forces the processor to wait > + until all prior speculative operations have been resolved; the second > + is to use a target-specific mechanism that can track the speculation > + state and to return @var{failval} if it can determine that > + speculation must be unwound at a later time. > + > + The default implementation simply copies @var{val} to @var{result} and > + emits a @code{speculation_barrier} instruction if that is defined. If > + @code{speculation_barrier} is not defined for the target a warning will > + be generated. > +@end deftypefn > + > @deftypefn {Target Hook} void TARGET_RUN_TARGET_SELFTESTS (void) > If selftests are enabled, run any selftests for this target. > @end deftypefn > diff --git a/gcc/doc/tm.texi.in b/gcc/doc/tm.texi.in > index b7b0e8a..6e20afb 100644 > --- a/gcc/doc/tm.texi.in > +++ b/gcc/doc/tm.texi.in > @@ -8107,4 +8107,6 @@ maintainer is familiar with. > > @end defmac > > +@hook TARGET_SPECULATION_SAFE_VALUE > + > @hook TARGET_RUN_TARGET_SELFTESTS > diff --git a/gcc/target.def b/gcc/target.def > index 112c772..c8bd7f8 100644 > --- a/gcc/target.def > +++ b/gcc/target.def > @@ -4177,6 +4177,29 @@ DEFHOOK > hook_bool_void_true) > > DEFHOOK > +(speculation_safe_value, > +"This target hook can be used to generate a target-specific code\n\ > + sequence that implements the @code{__builtin_speculation_safe_value}\n\ > + built-in function. The function must always return @var{val} in\n\ > + @var{result} in mode @var{mode} when the cpu is not executing\n\ > + speculatively, but must never return that when speculating until it\n\ > + is known that the speculation will not be unwound. The hook supports\n\ > + two primary mechanisms for implementing the requirements. The first\n\ > + is to emit a speculation barrier which forces the processor to wait\n\ > + until all prior speculative operations have been resolved; the second\n\ > + is to use a target-specific mechanism that can track the speculation\n\ > + state and to return @var{failval} if it can determine that\n\ > + speculation must be unwound at a later time.\n\ > + \n\ > + The default implementation simply copies @var{val} to @var{result} and\n\ > + emits a @code{speculation_barrier} instruction if that is defined. If\n\ > + @code{speculation_barrier} is not defined for the target a warning will\n\ > + be generated.", > +rtx, (machine_mode mode, rtx result, rtx val, rtx failval), > + default_speculation_safe_value) > + > + > +DEFHOOK > (can_use_doloop_p, > "Return true if it is possible to use low-overhead loops (@code{doloop_end}\n\ > and @code{doloop_begin}) for a particular loop. @var{iterations} gives the\n\ > diff --git a/gcc/targhooks.c b/gcc/targhooks.c > index 7315f1a..2061f07 100644 > --- a/gcc/targhooks.c > +++ b/gcc/targhooks.c > @@ -2306,4 +2306,31 @@ default_select_early_remat_modes (sbitmap) > { > } > > +/* Default implementation of the speculation-safe-load builtin. This > + implementation simply copies val to result and generates a > + speculation_barrier insn, if such a pattern is defined. If > + speculation_barrier is not defined at all, a warning is generated. */ > + > +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; > +} > + > #include "gt-targhooks.h" > diff --git a/gcc/targhooks.h b/gcc/targhooks.h > index 4107e22..80ac283 100644 > --- a/gcc/targhooks.h > +++ b/gcc/targhooks.h > @@ -284,4 +284,6 @@ default_excess_precision (enum excess_precision_type ATTRIBUTE_UNUSED); > extern bool default_stack_clash_protection_final_dynamic_probe (rtx); > extern void default_select_early_remat_modes (sbitmap); > > +extern rtx default_speculation_safe_value (machine_mode, rtx, rtx, rtx); > + > #endif /* GCC_TARGHOOKS_H */ > diff --git a/gcc/testsuite/gcc.dg/spec-barrier-1.c b/gcc/testsuite/gcc.dg/spec-barrier-1.c > new file mode 100644 > index 0000000..106f89a > --- /dev/null > +++ b/gcc/testsuite/gcc.dg/spec-barrier-1.c > @@ -0,0 +1,40 @@ > +/* { dg-do run } */ > +/* { dg-options "-O" } */ > + > +/* Test that __builtin_speculation_safe_value returns the correct value. */ > +/* This test will cause an unfiltered warning to be emitted on targets > + that have not implemented support for speculative execution > + barriers. They should fix that rather than disabling this > + test. */ > +char a = 1; > +short b = 2; > +int c = 3; > +long d = 4; > +long long e = 5; > +int *f = (int*) &c; > +#ifdef __SIZEOF_INT128__ > +__int128 g = 9; > +#endif > + > +extern void abort (void); > + > +int main () > +{ > + if (__builtin_speculation_safe_value (a) != 1) > + abort (); > + if (__builtin_speculation_safe_value (b) != 2) > + abort (); > + if (__builtin_speculation_safe_value (c) != 3) > + abort (); > + if (__builtin_speculation_safe_value (d) != 4) > + abort (); > + if (__builtin_speculation_safe_value (e) != 5) > + abort (); > + if (__builtin_speculation_safe_value (f) != &c) > + abort (); > +#ifdef __SIZEOF_INT128__ > + if (__builtin_speculation_safe_value (g) != 9) > + abort (); > +#endif > + return 0; > +} > diff --git a/gcc/testsuite/gcc.dg/spec-barrier-2.c b/gcc/testsuite/gcc.dg/spec-barrier-2.c > new file mode 100644 > index 0000000..7e9c497 > --- /dev/null > +++ b/gcc/testsuite/gcc.dg/spec-barrier-2.c > @@ -0,0 +1,19 @@ > +/* { dg-do run } */ > + > +/* Even on targets that don't need the optional failval parameter, > + side-effects on the operand should still be calculated. */ > + > +int x = 3; > +volatile int y = 9; > + > +extern void abort (void); > + > +int main () > +{ > + int z = __builtin_speculation_safe_value (x, y++); > + if (z != 3 || y != 10) > + abort (); > + return 0; > +} > + > +/* { dg-prune-output "this target does not define a speculation barrier;" } */ > diff --git a/gcc/testsuite/gcc.dg/spec-barrier-3.c b/gcc/testsuite/gcc.dg/spec-barrier-3.c > new file mode 100644 > index 0000000..3ed4d39 > --- /dev/null > +++ b/gcc/testsuite/gcc.dg/spec-barrier-3.c > @@ -0,0 +1,13 @@ > +/* { dg-do compile } */ > +/* { dg-options "-Wpedantic" } */ > + > +/* __builtin_speculation_safe_value returns a value with the same type > + as its first argument. There should be a warning if that isn't > + type-compatible with the use. */ > +int * > +f (int x) > +{ > + return __builtin_speculation_safe_value (x); /* { dg-warning "returning 'int' from a function with return type 'int \\*' makes pointer from integer without a cast" } */ > +} > + > +/* { dg-prune-output "this target does not define a speculation barrier;" } */ >
On Mon, Jul 9, 2018 at 6:40 PM Richard Earnshaw <Richard.Earnshaw@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? 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. 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 >
On 24/07/18 18:26, Richard Biener wrote: > On Mon, Jul 9, 2018 at 6:40 PM Richard Earnshaw > <Richard.Earnshaw@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. 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. 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. > > 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, ... 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. > 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 >>
On Wed, Jul 25, 2018 at 11:49 AM Richard Earnshaw (lists) <Richard.Earnshaw@arm.com> wrote: > > On 24/07/18 18:26, Richard Biener wrote: > > On Mon, Jul 9, 2018 at 6:40 PM Richard Earnshaw > > <Richard.Earnshaw@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 > >> >
On 25/07/18 11:36, Richard Biener wrote: > On Wed, Jul 25, 2018 at 11:49 AM Richard Earnshaw (lists) > <Richard.Earnshaw@arm.com> wrote: >> >> On 24/07/18 18:26, Richard Biener wrote: >>> On Mon, Jul 9, 2018 at 6:40 PM Richard Earnshaw >>> <Richard.Earnshaw@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? >> 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 :-) >>> >>> 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. > > 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. 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 >>>> >>
On Wed, Jul 25, 2018 at 2:41 PM Richard Earnshaw (lists) <Richard.Earnshaw@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.Earnshaw@arm.com> wrote: > >> > >> On 24/07/18 18:26, Richard Biener wrote: > >>> On Mon, Jul 9, 2018 at 6:40 PM Richard Earnshaw > >>> <Richard.Earnshaw@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 > >>>> > >> >
On 24/07/18 18:26, Richard Biener wrote: > 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? So I tried to make resolve_overloaded_builtin collapse the builtin entirely if it's not needed by the machine, transforming x = __b_s_s_v (y); into x = y; but I can't see how to make any side-effects on the optional second argument hang around. It's somewhat obscure, but if the user does write x = __b_s_s_v (y, z++); then z++ does still need to be performed. The problem seems to be that the callers of resolve_overloaded_builtin expect just a simple value result - they can't, for example, deal with a statement list and just calling save_expr on the argument isn't enough; so I can't see an obvious way to force the z++ expression back into the token stream at this point. Any ideas? The alternative seems to be that we must keep the call until such time as the builtins are lowered during expansion, which pretty much loses all the benefits you were looking for. R.
On Wed, Jul 25, 2018 at 8:03 PM Richard Earnshaw (lists) <Richard.Earnshaw@arm.com> wrote: > > On 24/07/18 18:26, Richard Biener wrote: > > 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? > > So I tried to make resolve_overloaded_builtin collapse the builtin > entirely if it's not needed by the machine, transforming > > x = __b_s_s_v (y); > > into > > x = y; > > > but I can't see how to make any side-effects on the optional second > argument hang around. It's somewhat obscure, but if the user does write > > x = __b_s_s_v (y, z++); > > then z++ does still need to be performed. > > The problem seems to be that the callers of resolve_overloaded_builtin > expect just a simple value result - they can't, for example, deal with a > statement list and just calling save_expr on the argument isn't enough; > so I can't see an obvious way to force the z++ expression back into the > token stream at this point. > > Any ideas? The alternative seems to be that we must keep the call until > such time as the builtins are lowered during expansion, which pretty > much loses all the benefits you were looking for. Use a COMPOUND_EXPR: (z++, y). So, if (TREE_SIDE_EFFECTS (2ndarg)) res = build2_loc (loc, COMPOUND_EXPR, type, 2ndarg, 1starg); else res = 2starg; Richard. > > R.
On 25/07/18 14:47, Richard Biener wrote: > On Wed, Jul 25, 2018 at 2:41 PM Richard Earnshaw (lists) > <Richard.Earnshaw@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.Earnshaw@arm.com> wrote: >>>> >>>> On 24/07/18 18:26, Richard Biener wrote: >>>>> On Mon, Jul 9, 2018 at 6:40 PM Richard Earnshaw >>>>> <Richard.Earnshaw@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. 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. diff --git a/gcc/builtin-types.def b/gcc/builtin-types.def index b01095c..70fae35 100644 --- a/gcc/builtin-types.def +++ b/gcc/builtin-types.def @@ -763,6 +763,12 @@ DEF_FUNCTION_TYPE_VAR_1 (BT_FN_VOID_LONG_VAR, BT_VOID, BT_LONG) DEF_FUNCTION_TYPE_VAR_1 (BT_FN_VOID_ULL_VAR, BT_VOID, BT_ULONGLONG) +DEF_FUNCTION_TYPE_VAR_1 (BT_FN_PTR_PTR_VAR, BT_PTR, BT_PTR) +DEF_FUNCTION_TYPE_VAR_1 (BT_FN_I1_I1_VAR, BT_I1, BT_I1) +DEF_FUNCTION_TYPE_VAR_1 (BT_FN_I2_I2_VAR, BT_I2, BT_I2) +DEF_FUNCTION_TYPE_VAR_1 (BT_FN_I4_I4_VAR, BT_I4, BT_I4) +DEF_FUNCTION_TYPE_VAR_1 (BT_FN_I8_I8_VAR, BT_I8, BT_I8) +DEF_FUNCTION_TYPE_VAR_1 (BT_FN_I16_I16_VAR, BT_I16, BT_I16) DEF_FUNCTION_TYPE_VAR_2 (BT_FN_INT_FILEPTR_CONST_STRING_VAR, BT_INT, BT_FILEPTR, BT_CONST_STRING) diff --git a/gcc/builtins.c b/gcc/builtins.c index 839a818..40183fb 100644 --- a/gcc/builtins.c +++ b/gcc/builtins.c @@ -6881,6 +6881,52 @@ inline_expand_builtin_string_cmp (tree exp, rtx target, bool is_memcmp) const_str_n, mode, is_memcmp); } +/* Expand a call to __builtin_speculation_safe_value_<N>. MODE + represents the size of the first argument to that call, or VOIDmode + if the argument is a pointer. IGNORE will be true if the result + isn't used. */ +static rtx +expand_speculation_safe_value (machine_mode mode, tree exp, rtx target, + bool ignore) +{ + rtx val, failsafe; + unsigned nargs = call_expr_nargs (exp); + + tree arg0 = CALL_EXPR_ARG (exp, 0); + + if (mode == VOIDmode) + { + mode = TYPE_MODE (TREE_TYPE (arg0)); + gcc_assert (GET_MODE_CLASS (mode) == MODE_INT); + } + + val = expand_expr (arg0, NULL_RTX, mode, EXPAND_NORMAL); + + /* An optional second argument can be used as a failsafe value on + some machines. If it isn't present, then the failsafe value is + assumed to be 0. */ + if (nargs > 1) + { + tree arg1 = CALL_EXPR_ARG (exp, 1); + failsafe = expand_expr (arg1, NULL_RTX, mode, EXPAND_NORMAL); + } + else + failsafe = const0_rtx; + + /* If the result isn't used, the behavior is undefined. It would be + nice to emit a warning here, but path splitting means this might + happen with legitimate code. So simply drop the builtin + expansion in that case; we've handled any side-effects above. */ + if (ignore) + return const0_rtx; + + /* If we don't have a suitable target, create one to hold the result. */ + if (target == NULL) + target = gen_reg_rtx (mode); + + return targetm.speculation_safe_value (mode, target, val, failsafe); +} + /* Expand an expression EXP that calls a built-in function, with result going to TARGET if that's convenient (and in mode MODE if that's convenient). @@ -7992,6 +8038,17 @@ expand_builtin (tree exp, rtx target, rtx subtarget, machine_mode mode, case BUILT_IN_GOACC_PARLEVEL_SIZE: return expand_builtin_goacc_parlevel_id_size (exp, target, ignore); + case BUILT_IN_SPECULATION_SAFE_VALUE_PTR: + return expand_speculation_safe_value (VOIDmode, exp, target, ignore); + + case BUILT_IN_SPECULATION_SAFE_VALUE_1: + case BUILT_IN_SPECULATION_SAFE_VALUE_2: + case BUILT_IN_SPECULATION_SAFE_VALUE_4: + case BUILT_IN_SPECULATION_SAFE_VALUE_8: + case BUILT_IN_SPECULATION_SAFE_VALUE_16: + mode = get_builtin_sync_mode (fcode - BUILT_IN_SPECULATION_SAFE_VALUE_1); + return expand_speculation_safe_value (mode, exp, target, ignore); + default: /* just do library call, if unknown builtin */ break; } diff --git a/gcc/builtins.def b/gcc/builtins.def index aacbd51..6e8af2a 100644 --- a/gcc/builtins.def +++ b/gcc/builtins.def @@ -1003,6 +1003,28 @@ DEF_BUILTIN (BUILT_IN_EMUTLS_REGISTER_COMMON, true, true, true, ATTR_NOTHROW_LEAF_LIST, false, !targetm.have_tls) +/* Suppressing speculation. Users are expected to use the first (N) + variant, which will be translated internally into one of the other + types. */ + +DEF_GCC_BUILTIN (BUILT_IN_SPECULATION_SAFE_VALUE_N, "speculation_safe_value", + BT_FN_VOID_VAR, ATTR_NOTHROW_LEAF_LIST) + +DEF_GCC_BUILTIN (BUILT_IN_SPECULATION_SAFE_VALUE_PTR, + "speculation_safe_value_ptr", BT_FN_PTR_PTR_VAR, + ATTR_NOTHROW_LEAF_LIST) +DEF_GCC_BUILTIN (BUILT_IN_SPECULATION_SAFE_VALUE_1, "speculation_safe_value_1", + BT_FN_I1_I1_VAR, ATTR_NOTHROW_LEAF_LIST) +DEF_GCC_BUILTIN (BUILT_IN_SPECULATION_SAFE_VALUE_2, "speculation_safe_value_2", + BT_FN_I2_I2_VAR, ATTR_NOTHROW_LEAF_LIST) +DEF_GCC_BUILTIN (BUILT_IN_SPECULATION_SAFE_VALUE_4, "speculation_safe_value_4", + BT_FN_I4_I4_VAR, ATTR_NOTHROW_LEAF_LIST) +DEF_GCC_BUILTIN (BUILT_IN_SPECULATION_SAFE_VALUE_8, "speculation_safe_value_8", + BT_FN_I8_I8_VAR, ATTR_NOTHROW_LEAF_LIST) +DEF_GCC_BUILTIN (BUILT_IN_SPECULATION_SAFE_VALUE_16, + "speculation_safe_value_16", BT_FN_I16_I16_VAR, + ATTR_NOTHROW_LEAF_LIST) + /* Exception support. */ DEF_BUILTIN_STUB (BUILT_IN_UNWIND_RESUME, "__builtin_unwind_resume") DEF_BUILTIN_STUB (BUILT_IN_CXA_END_CLEANUP, "__builtin_cxa_end_cleanup") diff --git a/gcc/c-family/c-common.c b/gcc/c-family/c-common.c index f5e1111..e368fd2 100644 --- a/gcc/c-family/c-common.c +++ b/gcc/c-family/c-common.c @@ -6457,6 +6457,122 @@ builtin_type_for_size (int size, bool unsignedp) return type ? type : error_mark_node; } +/* Work out the size of the first argument of a call to + __builtin_speculation_safe_value. Only pointers and integral types + are permitted. Return -1 if the argument type is not supported or + the size is too large; 0 if the argument type is a pointer or the + size if it is integral. */ +static enum built_in_function +speculation_safe_value_resolve_call (tree function, vec<tree, va_gc> *params) +{ + /* Type of the argument. */ + tree type; + int size; + + if (vec_safe_is_empty (params)) + { + error ("too few arguments to function %qE", function); + return BUILT_IN_NONE; + } + + type = TREE_TYPE ((*params)[0]); + if (TREE_CODE (type) == ARRAY_TYPE && c_dialect_cxx ()) + { + /* Force array-to-pointer decay for C++. */ + (*params)[0] = default_conversion ((*params)[0]); + type = TREE_TYPE ((*params)[0]); + } + + if (POINTER_TYPE_P (type)) + return BUILT_IN_SPECULATION_SAFE_VALUE_PTR; + + if (!INTEGRAL_TYPE_P (type)) + goto incompatible; + + if (!COMPLETE_TYPE_P (type)) + goto incompatible; + + size = tree_to_uhwi (TYPE_SIZE_UNIT (type)); + if (size == 1 || size == 2 || size == 4 || size == 8 || size == 16) + return ((enum built_in_function) + ((int) BUILT_IN_SPECULATION_SAFE_VALUE_1 + exact_log2 (size))); + + incompatible: + /* Issue the diagnostic only if the argument is valid, otherwise + it would be redundant at best and could be misleading. */ + if (type != error_mark_node) + error ("operand type %qT is incompatible with argument %d of %qE", + type, 1, function); + + return BUILT_IN_NONE; +} + +/* Validate and coerce PARAMS, the arguments to ORIG_FUNCTION to fit + the prototype for FUNCTION. The first argument is mandatory, a second + argument, if present, must be type compatible with the first. */ +static bool +speculation_safe_value_resolve_params (location_t loc, tree orig_function, + vec<tree, va_gc> *params) +{ + tree val; + + if (params->length () == 0) + { + error_at (loc, "too few arguments to function %qE", orig_function); + return false; + } + + else if (params->length () > 2) + { + error_at (loc, "too many arguments to function %qE", orig_function); + return false; + } + + val = (*params)[0]; + if (TREE_CODE (TREE_TYPE (val)) == ARRAY_TYPE) + val = default_conversion (val); + if (!(TREE_CODE (TREE_TYPE (val)) == POINTER_TYPE + || TREE_CODE (TREE_TYPE (val)) == INTEGER_TYPE)) + { + error_at (loc, + "expecting argument of type pointer or of type integer " + "for argument 1"); + return false; + } + (*params)[0] = val; + + if (params->length () == 2) + { + tree val2 = (*params)[1]; + if (TREE_CODE (TREE_TYPE (val2)) == ARRAY_TYPE) + val2 = default_conversion (val2); + if (!(TREE_TYPE (val) == TREE_TYPE (val2) + || useless_type_conversion_p (TREE_TYPE (val), TREE_TYPE (val2)))) + { + error_at (loc, "both arguments must be compatible"); + return false; + } + (*params)[1] = val2; + } + + return true; +} + +/* Cast the result of the builtin back to the type of the first argument, + preserving any qualifiers that it might have. */ +static tree +speculation_safe_value_resolve_return (tree first_param, tree result) +{ + tree ptype = TREE_TYPE (first_param); + tree rtype = TREE_TYPE (result); + ptype = TYPE_MAIN_VARIANT (ptype); + + if (tree_int_cst_equal (TYPE_SIZE (ptype), TYPE_SIZE (rtype))) + return convert (ptype, result); + + return result; +} + /* A helper function for resolve_overloaded_builtin in resolving the overloaded __sync_ builtins. Returns a positive power of 2 if the first operand of PARAMS is a pointer to a supported data type. @@ -7111,6 +7227,54 @@ resolve_overloaded_builtin (location_t loc, tree function, /* Handle BUILT_IN_NORMAL here. */ switch (orig_code) { + case BUILT_IN_SPECULATION_SAFE_VALUE_N: + { + tree new_function, first_param, result; + enum built_in_function fncode + = speculation_safe_value_resolve_call (function, params);; + + first_param = (*params)[0]; + if (fncode == BUILT_IN_NONE + || !speculation_safe_value_resolve_params (loc, function, params)) + return error_mark_node; + + if (targetm.have_speculation_safe_value (true)) + { + new_function = builtin_decl_explicit (fncode); + result = build_function_call_vec (loc, vNULL, new_function, params, + NULL); + + if (result == error_mark_node) + return result; + + return speculation_safe_value_resolve_return (first_param, result); + } + else + { + /* This target doesn't have, or doesn't need, active mitigation + against incorrect speculative execution. Simply return the + first parameter to the builtin. */ + if (!targetm.have_speculation_safe_value (false)) + /* The user has invoked __builtin_speculation_safe_value + even though __HAVE_SPECULATION_SAFE_VALUE is not + defined: emit a warning. */ + warning_at (input_location, 0, + "this target does not define a speculation barrier; " + "your program will still execute correctly, " + "but incorrect speculation may not be be " + "restricted"); + + /* If the optional second argument is present, handle any side + effects now. */ + if (params->length () == 2 + && TREE_SIDE_EFFECTS ((*params)[1])) + return build2 (COMPOUND_EXPR, TREE_TYPE (first_param), + (*params)[1], first_param); + + return first_param; + } + } + case BUILT_IN_ATOMIC_EXCHANGE: case BUILT_IN_ATOMIC_COMPARE_EXCHANGE: case BUILT_IN_ATOMIC_LOAD: diff --git a/gcc/c-family/c-cppbuiltin.c b/gcc/c-family/c-cppbuiltin.c index bdb5691..4fcf3a6 100644 --- a/gcc/c-family/c-cppbuiltin.c +++ b/gcc/c-family/c-cppbuiltin.c @@ -1361,7 +1361,12 @@ c_cpp_builtins (cpp_reader *pfile) cpp_define (pfile, "__WCHAR_UNSIGNED__"); cpp_atomic_builtins (pfile); - + + /* Show support for __builtin_speculation_safe_value () if the target + has been updated to fully support it. */ + if (targetm.have_speculation_safe_value (false)) + cpp_define (pfile, "__HAVE_SPECULATION_SAFE_VALUE"); + #ifdef DWARF2_UNWIND_INFO if (dwarf2out_do_cfi_asm ()) cpp_define (pfile, "__GCC_HAVE_DWARF2_CFI_ASM"); diff --git a/gcc/doc/cpp.texi b/gcc/doc/cpp.texi index 3f7a8fc..efad2c8 100644 --- a/gcc/doc/cpp.texi +++ b/gcc/doc/cpp.texi @@ -2381,6 +2381,10 @@ If GCC cannot determine the current date, it will emit a warning message These macros are defined when the target processor supports atomic compare and swap operations on operands 1, 2, 4, 8 or 16 bytes in length, respectively. +@item __HAVE_SPECULATION_SAFE_VALUE +This macro is defined with the value 1 to show that this version of GCC +supports @code{__builtin_speculation_safe_value}. + @item __GCC_HAVE_DWARF2_CFI_ASM This macro is defined when the compiler is emitting DWARF CFI directives to the assembler. When this is defined, it is possible to emit those same diff --git a/gcc/doc/extend.texi b/gcc/doc/extend.texi index 954e8a1..0ba1931 100644 --- a/gcc/doc/extend.texi +++ b/gcc/doc/extend.texi @@ -10948,6 +10948,7 @@ is called and the @var{flag} argument passed to it. @findex __builtin_powi @findex __builtin_powif @findex __builtin_powil +@findex __builtin_speculation_safe_value @findex _Exit @findex _exit @findex abort @@ -11592,6 +11593,96 @@ check its compatibility with @var{size}. @end deftypefn +@deftypefn {Built-in Function} @var{type} __builtin_speculation_safe_value (@var{type} val, @var{type} failval) + +This built-in function can be used to help mitigate against unsafe +speculative execution. @var{type} may be any integral type or any +pointer type. + +@enumerate +@item +If the CPU is not speculatively executing the code, then @var{val} +is returned. +@item +If the CPU is executing speculatively then either: +@itemize +@item +The function may cause execution to pause until it is known that the +code is no-longer being executed speculatively (in which case +@var{val} can be returned, as above); or +@item +The function may use target-dependent speculation tracking state to cause +@var{failval} to be returned when it is known that speculative +execution has incorrectly predicted a conditional branch operation. +@end itemize +@end enumerate + +The second argument, @var{failval}, is optional and defaults to zero +if omitted. + +GCC defines the preprocessor macro +@code{__HAVE_BUILTIN_SPECULATION_SAFE_VALUE} for targets that have been +updated to support this builtin. + +The built-in function can be used where a variable appears to be used in a +safe way, but the CPU, due to speculative execution may temporarily ignore +the bounds checks. Consider, for example, the following function: + +@smallexample +int array[500]; +int f (unsigned untrusted_index) +@{ + if (untrusted_index < 500) + return array[untrusted_index]; + return 0; +@} +@end smallexample + +If the function is called repeatedly with @code{untrusted_index} less +than the limit of 500, then a branch predictor will learn that the +block of code that returns a value stored in @code{array} will be +executed. If the function is subsequently called with an +out-of-range value it will still try to execute that block of code +first until the CPU determines that the prediction was incorrect +(the CPU will unwind any incorrect operations at that point). +However, depending on how the result of the function is used, it might be +possible to leave traces in the cache that can reveal what was stored +at the out-of-bounds location. The built-in function can be used to +provide some protection against leaking data in this way by changing +the code to: + +@smallexample +int array[500]; +int f (unsigned untrusted_index) +@{ + if (untrusted_index < 500) + return array[__builtin_speculation_safe_value (untrusted_index)]; + return 0; +@} +@end smallexample + +The built-in function will either cause execution to stall until the +conditional branch has been fully resolved, or it may permit +speculative execution to continue, but using 0 instead of +@code{untrusted_value} if that exceeds the limit. + +If accessing any memory location is potentially unsafe when speculative +execution is incorrect, then the code can be rewritten as + +@smallexample +int array[500]; +int f (unsigned untrusted_index) +@{ + if (untrusted_index < 500) + return *__builtin_speculation_safe_value (&array[untrusted_index], NULL); + return 0; +@} +@end smallexample + +which will cause a @code{NULL} pointer to be used for the unsafe case. + +@end deftypefn + @deftypefn {Built-in Function} int __builtin_types_compatible_p (@var{type1}, @var{type2}) You can use the built-in function @code{__builtin_types_compatible_p} to diff --git a/gcc/doc/md.texi b/gcc/doc/md.texi index 734bc76..00c1239 100644 --- a/gcc/doc/md.texi +++ b/gcc/doc/md.texi @@ -7032,6 +7032,21 @@ should be defined to an instruction that orders both loads and stores before the instruction with respect to loads and stores after the instruction. This pattern has no operands. +@cindex @code{speculation_barrier} instruction pattern +@item @samp{speculation_barrier} +If the target can support speculative execution, then this pattern should +be defined to an instruction that will block subsequent execution until +any prior speculation conditions has been resolved. The pattern must also +ensure that the compiler cannot move memory operations past the barrier, +so it needs to be an UNSPEC_VOLATILE pattern. The pattern has no +operands. + +If this pattern is not defined then the default expansion of +@code{__builtin_speculation_safe_value} will emit a warning. You can +suppress this warning by defining this pattern with a final condition +of @code{0} (zero), which tells the compiler that a speculation +barrier is not needed for this target. + @cindex @code{sync_compare_and_swap@var{mode}} instruction pattern @item @samp{sync_compare_and_swap@var{mode}} This pattern, if defined, emits code for an atomic compare-and-swap diff --git a/gcc/doc/tm.texi b/gcc/doc/tm.texi index ff6d514..ee3f55c 100644 --- a/gcc/doc/tm.texi +++ b/gcc/doc/tm.texi @@ -11948,6 +11948,36 @@ maintainer is familiar with. @end defmac +@deftypefn {Target Hook} bool TARGET_HAVE_SPECULATION_SAFE_VALUE (bool @var{active}) +This hook is used to determine the level of target support for + @code{__builtin_speculation_safe_value}. If called with an argument + of false, it returns true if the target has been modified to support + this builtin. If called with an argument of true, it returns true + if the target requires active mitigation execution might be speculative. + + 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 + +@deftypefn {Target Hook} rtx TARGET_SPECULATION_SAFE_VALUE (machine_mode @var{mode}, rtx @var{result}, rtx @var{val}, rtx @var{failval}) +This target hook can be used to generate a target-specific code + sequence that implements the @code{__builtin_speculation_safe_value} + built-in function. The function must always return @var{val} in + @var{result} in mode @var{mode} when the cpu is not executing + speculatively, but must never return that when speculating until it + is known that the speculation will not be unwound. The hook supports + two primary mechanisms for implementing the requirements. The first + is to emit a speculation barrier which forces the processor to wait + until all prior speculative operations have been resolved; the second + is to use a target-specific mechanism that can track the speculation + state and to return @var{failval} if it can determine that + speculation must be unwound at a later time. + + The default implementation simply copies @var{val} to @var{result} and + emits a @code{speculation_barrier} instruction if that is defined. +@end deftypefn + @deftypefn {Target Hook} void TARGET_RUN_TARGET_SELFTESTS (void) If selftests are enabled, run any selftests for this target. @end deftypefn diff --git a/gcc/doc/tm.texi.in b/gcc/doc/tm.texi.in index 2f97151..94ad868 100644 --- a/gcc/doc/tm.texi.in +++ b/gcc/doc/tm.texi.in @@ -8109,4 +8109,8 @@ maintainer is familiar with. @end defmac +@hook TARGET_HAVE_SPECULATION_SAFE_VALUE + +@hook TARGET_SPECULATION_SAFE_VALUE + @hook TARGET_RUN_TARGET_SELFTESTS diff --git a/gcc/target.def b/gcc/target.def index ff89e72..3a9de78 100644 --- a/gcc/target.def +++ b/gcc/target.def @@ -4196,6 +4196,40 @@ DEFHOOK hook_bool_void_true) DEFHOOK +(have_speculation_safe_value, +"This hook is used to determine the level of target support for\n\ + @code{__builtin_speculation_safe_value}. If called with an argument\n\ + of false, it returns true if the target has been modified to support\n\ + this builtin. If called with an argument of true, it returns true\n\ + if the target requires active mitigation execution might be speculative.\n\ + \n\ + The default implementation returns true for the first case if the target\n\ + defines a pattern named @code{speculation_barrier}; for the second case\n\ + and if the pattern is enabled for the current compilation.", +bool, (bool active), default_have_speculation_safe_value) + +DEFHOOK +(speculation_safe_value, +"This target hook can be used to generate a target-specific code\n\ + sequence that implements the @code{__builtin_speculation_safe_value}\n\ + built-in function. The function must always return @var{val} in\n\ + @var{result} in mode @var{mode} when the cpu is not executing\n\ + speculatively, but must never return that when speculating until it\n\ + is known that the speculation will not be unwound. The hook supports\n\ + two primary mechanisms for implementing the requirements. The first\n\ + is to emit a speculation barrier which forces the processor to wait\n\ + until all prior speculative operations have been resolved; the second\n\ + is to use a target-specific mechanism that can track the speculation\n\ + state and to return @var{failval} if it can determine that\n\ + speculation must be unwound at a later time.\n\ + \n\ + The default implementation simply copies @var{val} to @var{result} and\n\ + emits a @code{speculation_barrier} instruction if that is defined.", +rtx, (machine_mode mode, rtx result, rtx val, rtx failval), + default_speculation_safe_value) + + +DEFHOOK (can_use_doloop_p, "Return true if it is possible to use low-overhead loops (@code{doloop_end}\n\ and @code{doloop_begin}) for a particular loop. @var{iterations} gives the\n\ diff --git a/gcc/targhooks.c b/gcc/targhooks.c index 9b06d7a..06de1e3 100644 --- a/gcc/targhooks.c +++ b/gcc/targhooks.c @@ -2314,4 +2314,36 @@ default_preferred_else_value (unsigned, tree type, unsigned, tree *) return build_zero_cst (type); } +/* Default implementation of TARGET_HAVE_SPECULATION_SAFE_VALUE. */ +bool +default_have_speculation_safe_value (bool active) +{ +#ifdef HAVE_speculation_barrier + return active ? HAVE_speculation_barrier : true; +#else + return false; +#endif +} + +/* Default implementation of the speculation-safe-load builtin. This + implementation simply copies val to result and generates a + speculation_barrier insn, if such a pattern is defined. */ +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 ()); +#endif + + return result; +} + #include "gt-targhooks.h" diff --git a/gcc/targhooks.h b/gcc/targhooks.h index 8d234cf..74ffe5f 100644 --- a/gcc/targhooks.h +++ b/gcc/targhooks.h @@ -285,4 +285,7 @@ extern bool default_stack_clash_protection_final_dynamic_probe (rtx); extern void default_select_early_remat_modes (sbitmap); extern tree default_preferred_else_value (unsigned, tree, unsigned, tree *); +extern bool default_have_speculation_safe_value (bool); +extern rtx default_speculation_safe_value (machine_mode, rtx, rtx, rtx); + #endif /* GCC_TARGHOOKS_H */ diff --git a/gcc/testsuite/c-c++-common/spec-barrier-1.c b/gcc/testsuite/c-c++-common/spec-barrier-1.c new file mode 100644 index 0000000..e4b44f2 --- /dev/null +++ b/gcc/testsuite/c-c++-common/spec-barrier-1.c @@ -0,0 +1,38 @@ +/* { dg-do run } */ +/* { dg-options "-O" } */ + +/* Test that __builtin_speculation_safe_value returns the correct value. */ +/* This test will cause an unfiltered warning to be emitted on targets + that have not implemented support for speculative execution + barriers. They should fix that rather than disabling this + test. */ +char a = 1; +short b = 2; +int c = 3; +long d = 4; +long long e = 5; +int *f = (int*) &c; +#ifdef __SIZEOF_INT128__ +__int128 g = 9; +#endif + +int main () +{ + if (__builtin_speculation_safe_value (a) != 1) + __builtin_abort (); + if (__builtin_speculation_safe_value (b) != 2) + __builtin_abort (); + if (__builtin_speculation_safe_value (c) != 3) + __builtin_abort (); + if (__builtin_speculation_safe_value (d) != 4) + __builtin_abort (); + if (__builtin_speculation_safe_value (e) != 5) + __builtin_abort (); + if (__builtin_speculation_safe_value (f) != &c) + __builtin_abort (); +#ifdef __SIZEOF_INT128__ + if (__builtin_speculation_safe_value (g) != 9) + __builtin_abort (); +#endif + return 0; +} diff --git a/gcc/testsuite/c-c++-common/spec-barrier-2.c b/gcc/testsuite/c-c++-common/spec-barrier-2.c new file mode 100644 index 0000000..b09567e --- /dev/null +++ b/gcc/testsuite/c-c++-common/spec-barrier-2.c @@ -0,0 +1,17 @@ +/* { dg-do run } */ + +/* Even on targets that don't need the optional failval parameter, + side-effects on the operand should still be calculated. */ + +int x = 3; +volatile int y = 9; + +int main () +{ + int z = __builtin_speculation_safe_value (x, y++); + if (z != 3 || y != 10) + __builtin_abort (); + return 0; +} + +/* { dg-prune-output "this target does not define a speculation barrier;" } */ diff --git a/gcc/testsuite/gcc.dg/spec-barrier-3.c b/gcc/testsuite/gcc.dg/spec-barrier-3.c new file mode 100644 index 0000000..3ed4d39 --- /dev/null +++ b/gcc/testsuite/gcc.dg/spec-barrier-3.c @@ -0,0 +1,13 @@ +/* { dg-do compile } */ +/* { dg-options "-Wpedantic" } */ + +/* __builtin_speculation_safe_value returns a value with the same type + as its first argument. There should be a warning if that isn't + type-compatible with the use. */ +int * +f (int x) +{ + return __builtin_speculation_safe_value (x); /* { dg-warning "returning 'int' from a function with return type 'int \\*' makes pointer from integer without a cast" } */ +} + +/* { dg-prune-output "this target does not define a speculation barrier;" } */
On Thu, Jul 26, 2018 at 12:03 PM Richard Earnshaw (lists) <Richard.Earnshaw@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.Earnshaw@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.Earnshaw@arm.com> wrote: > >>>> > >>>> On 24/07/18 18:26, Richard Biener wrote: > >>>>> On Mon, Jul 9, 2018 at 6:40 PM Richard Earnshaw > >>>>> <Richard.Earnshaw@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.
On 26/07/18 13:41, Richard Biener wrote: > On Thu, Jul 26, 2018 at 12:03 PM Richard Earnshaw (lists) > <Richard.Earnshaw@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.Earnshaw@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.Earnshaw@arm.com> wrote: >>>>>> >>>>>> On 24/07/18 18:26, Richard Biener wrote: >>>>>>> On Mon, Jul 9, 2018 at 6:40 PM Richard Earnshaw >>>>>>> <Richard.Earnshaw@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.
On Thu, Jul 26, 2018 at 3:06 PM Richard Earnshaw (lists) <Richard.Earnshaw@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.Earnshaw@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.Earnshaw@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.Earnshaw@arm.com> wrote: > >>>>>> > >>>>>> On 24/07/18 18:26, Richard Biener wrote: > >>>>>>> On Mon, Jul 9, 2018 at 6:40 PM Richard Earnshaw > >>>>>>> <Richard.Earnshaw@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. >
On Wed, 25 Jul 2018, Richard Earnshaw (lists) wrote: > >> 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 suggest the following as an appropriate process for anything needing attention from architecture maintainers: * Send a message to the gcc list, starting its own thread, CC:ed to all target architecture maintainers, stating explicitly in its first sentence that it is about something needing action from all such maintainers. The message needs to give all the information required for those maintainers to work out what is appropriate for their ports, or point to a wiki page with that information. For example, the message must not assume maintainer familiarity with Spectre - it must give sufficient information for maintainers unfamiliar with Spectre to work out what to do with their ports. * If the messages to any maintainers bounce, actively seek out alternative contact details for them or alternative people who might be interested in maintaining those ports. Likewise, it's necessary to check for any ports that do not have a listed maintainer in the MAINTAINERS file and find appropriate contacts for those. * Over the next few months, send occasional reminders, each including a list of the ports that have not been updated. * No backport should be considered for anything that might involve breakage, e.g. warnings, for unchanged ports, until either all ports have been updated or at least several months have passed. (Normally, a change needing architecture maintainer attention would be something for which backports are obviously inappropriate, e.g. a new C / C++ language feature requiring ABI decisions to be made for which there isn't an obvious default that can safely be applied to all architectures.) -- Joseph S. Myers joseph@codesourcery.com
> On Jul 26, 2018, at 7:34 PM, Joseph Myers <joseph@codesourcery.com> wrote: > > On Wed, 25 Jul 2018, Richard Earnshaw (lists) wrote: > >>>> 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 suggest the following as an appropriate process for anything needing > attention from architecture maintainers: > > * Send a message to the gcc list, starting its own thread, CC:ed to all > target architecture maintainers, stating explicitly in its first sentence > that it is about something needing action from all such maintainers. Yes, because it was not clear to me that a patch discussion about a speculation builtin was something that every target maintainer was supposed to look at. "Speculation" is not a term that shows up in my target... > ... > * Over the next few months, send occasional reminders, each including a > list of the ports that have not been updated. Would the GCC Wiki be a good place to collect all the responses and track what is still open? If not, what is a good way to do the tracking? paul
On 27/07/18 01:46, Paul Koning wrote: > > >> On Jul 26, 2018, at 7:34 PM, Joseph Myers <joseph@codesourcery.com> wrote: >> >> On Wed, 25 Jul 2018, Richard Earnshaw (lists) wrote: >> >>>>> 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 suggest the following as an appropriate process for anything needing >> attention from architecture maintainers: >> >> * Send a message to the gcc list, starting its own thread, CC:ed to all >> target architecture maintainers, stating explicitly in its first sentence >> that it is about something needing action from all such maintainers. > > Yes, because it was not clear to me that a patch discussion about a speculation builtin was something that every target maintainer was supposed to look at. "Speculation" is not a term that shows up in my target... > >> ... >> * Over the next few months, send occasional reminders, each including a >> list of the ports that have not been updated. > > Would the GCC Wiki be a good place to collect all the responses and track what is still open? If not, what is a good way to do the tracking? > > paul > The c-c++-common/spec-barrier-1.c test will fail on any target that has not been updated (it deliberately doesn't check for __HAVE_SPECULATION_BARRIER before trying to use the new intrinsic). The test contains a comment to that effect. That should be enough to alert maintainers if they are tracking testsuite errors. I'll be posting some examples for how to handle various types of target shortly. Target maintainers should then be able to decide which action they need to take. R.
On Fri, 27 Jul 2018, Richard Earnshaw (lists) wrote: > The c-c++-common/spec-barrier-1.c test will fail on any target that has > not been updated (it deliberately doesn't check for > __HAVE_SPECULATION_BARRIER before trying to use the new intrinsic). The > test contains a comment to that effect. That should be enough to alert > maintainers if they are tracking testsuite errors. Introducing a test failure is not enough. You need to *explicitly* alert target maintainers to the need for action, with a self-contained explanation not assuming any prior understanding of Spectre and its mitigations, and if any backport is to be considered you'll then need to track the status of different targets and remind maintainers as needed (so that you know when all targets have been updated, or not updated despite maintainers having been informed a few months previously without the emails bouncing and reminded a few times since, or not updated and the maintainers have explicitly acknowledged this and given their OK to a backport without updates for their architectures). A mail to the gcc list (that draws attention in the subject line and the first sentence to the need for architecture maintainer action) is the bare minimum (you should not assume target maintainers read gcc-patches discussions not mentioning their architecture in the subject), but CC:ing the architecture maintainers (immediately, or for those not fixed within a month or two) provides better assurance that the issue has been properly drawn to their attention. -- Joseph S. Myers joseph@codesourcery.com
diff --git a/gcc/builtin-types.def b/gcc/builtin-types.def index b01095c..70fae35 100644 --- a/gcc/builtin-types.def +++ b/gcc/builtin-types.def @@ -763,6 +763,12 @@ DEF_FUNCTION_TYPE_VAR_1 (BT_FN_VOID_LONG_VAR, BT_VOID, BT_LONG) DEF_FUNCTION_TYPE_VAR_1 (BT_FN_VOID_ULL_VAR, BT_VOID, BT_ULONGLONG) +DEF_FUNCTION_TYPE_VAR_1 (BT_FN_PTR_PTR_VAR, BT_PTR, BT_PTR) +DEF_FUNCTION_TYPE_VAR_1 (BT_FN_I1_I1_VAR, BT_I1, BT_I1) +DEF_FUNCTION_TYPE_VAR_1 (BT_FN_I2_I2_VAR, BT_I2, BT_I2) +DEF_FUNCTION_TYPE_VAR_1 (BT_FN_I4_I4_VAR, BT_I4, BT_I4) +DEF_FUNCTION_TYPE_VAR_1 (BT_FN_I8_I8_VAR, BT_I8, BT_I8) +DEF_FUNCTION_TYPE_VAR_1 (BT_FN_I16_I16_VAR, BT_I16, BT_I16) DEF_FUNCTION_TYPE_VAR_2 (BT_FN_INT_FILEPTR_CONST_STRING_VAR, BT_INT, BT_FILEPTR, BT_CONST_STRING) diff --git a/gcc/builtins.c b/gcc/builtins.c index 91658e8..9f97ecf 100644 --- a/gcc/builtins.c +++ b/gcc/builtins.c @@ -6716,6 +6716,52 @@ expand_builtin_goacc_parlevel_id_size (tree exp, rtx target, int ignore) return target; } +/* Expand a call to __builtin_speculation_safe_value_<N>. MODE + represents the size of the first argument to that call, or VOIDmode + if the argument is a pointer. IGNORE will be true if the result + isn't used. */ +static rtx +expand_speculation_safe_value (machine_mode mode, tree exp, rtx target, + bool ignore) +{ + rtx val, failsafe; + unsigned nargs = call_expr_nargs (exp); + + tree arg0 = CALL_EXPR_ARG (exp, 0); + + if (mode == VOIDmode) + { + mode = TYPE_MODE (TREE_TYPE (arg0)); + gcc_assert (GET_MODE_CLASS (mode) == MODE_INT); + } + + val = expand_expr (arg0, NULL_RTX, mode, EXPAND_NORMAL); + + /* An optional second argument can be used as a failsafe value on + some machines. If it isn't present, then the failsafe value is + assumed to be 0. */ + if (nargs > 1) + { + tree arg1 = CALL_EXPR_ARG (exp, 1); + failsafe = expand_expr (arg1, NULL_RTX, mode, EXPAND_NORMAL); + } + else + failsafe = const0_rtx; + + /* If the result isn't used, the behavior is undefined. It would be + nice to emit a warning here, but path splitting means this might + happen with legitimate code. So simply drop the builtin + expansion in that case; we've handled any side-effects above. */ + if (ignore) + return const0_rtx; + + /* If we don't have a suitable target, create one to hold the result. */ + if (target == NULL) + target = gen_reg_rtx (mode); + + return targetm.speculation_safe_value (mode, target, val, failsafe); +} + /* Expand an expression EXP that calls a built-in function, with result going to TARGET if that's convenient (and in mode MODE if that's convenient). @@ -7827,6 +7873,17 @@ expand_builtin (tree exp, rtx target, rtx subtarget, machine_mode mode, case BUILT_IN_GOACC_PARLEVEL_SIZE: return expand_builtin_goacc_parlevel_id_size (exp, target, ignore); + case BUILT_IN_SPECULATION_SAFE_VALUE_PTR: + return expand_speculation_safe_value (VOIDmode, exp, target, ignore); + + case BUILT_IN_SPECULATION_SAFE_VALUE_1: + case BUILT_IN_SPECULATION_SAFE_VALUE_2: + case BUILT_IN_SPECULATION_SAFE_VALUE_4: + case BUILT_IN_SPECULATION_SAFE_VALUE_8: + case BUILT_IN_SPECULATION_SAFE_VALUE_16: + mode = get_builtin_sync_mode (fcode - BUILT_IN_SPECULATION_SAFE_VALUE_1); + return expand_speculation_safe_value (mode, exp, target, ignore); + default: /* just do library call, if unknown builtin */ break; } diff --git a/gcc/builtins.def b/gcc/builtins.def index aacbd51..b71d89c 100644 --- a/gcc/builtins.def +++ b/gcc/builtins.def @@ -1003,6 +1003,26 @@ DEF_BUILTIN (BUILT_IN_EMUTLS_REGISTER_COMMON, true, true, true, ATTR_NOTHROW_LEAF_LIST, false, !targetm.have_tls) +/* Suppressing speculation. Users are expected to use the first (N) + variant, which will be translated internally into one of the other + types. */ + +DEF_GCC_BUILTIN (BUILT_IN_SPECULATION_SAFE_VALUE_N, "speculation_safe_value", + BT_FN_VOID_VAR, ATTR_NULL) + +DEF_GCC_BUILTIN (BUILT_IN_SPECULATION_SAFE_VALUE_PTR, + "speculation_safe_value_ptr", BT_FN_PTR_PTR_VAR, ATTR_NULL) +DEF_GCC_BUILTIN (BUILT_IN_SPECULATION_SAFE_VALUE_1, "speculation_safe_value_1", + BT_FN_I1_I1_VAR, ATTR_NULL) +DEF_GCC_BUILTIN (BUILT_IN_SPECULATION_SAFE_VALUE_2, "speculation_safe_value_2", + BT_FN_I2_I2_VAR, ATTR_NULL) +DEF_GCC_BUILTIN (BUILT_IN_SPECULATION_SAFE_VALUE_4, "speculation_safe_value_4", + BT_FN_I4_I4_VAR, ATTR_NULL) +DEF_GCC_BUILTIN (BUILT_IN_SPECULATION_SAFE_VALUE_8, "speculation_safe_value_8", + BT_FN_I8_I8_VAR, ATTR_NULL) +DEF_GCC_BUILTIN (BUILT_IN_SPECULATION_SAFE_VALUE_16, + "speculation_safe_value_16", BT_FN_I16_I16_VAR, ATTR_NULL) + /* Exception support. */ DEF_BUILTIN_STUB (BUILT_IN_UNWIND_RESUME, "__builtin_unwind_resume") DEF_BUILTIN_STUB (BUILT_IN_CXA_END_CLEANUP, "__builtin_cxa_end_cleanup") diff --git a/gcc/c-family/c-common.c b/gcc/c-family/c-common.c index f5e1111..32a2de2 100644 --- a/gcc/c-family/c-common.c +++ b/gcc/c-family/c-common.c @@ -6457,6 +6457,121 @@ builtin_type_for_size (int size, bool unsignedp) return type ? type : error_mark_node; } +/* Work out the size of the first argument of a call to + __builtin_speculation_safe_value. Only pointers and integral types + are permitted. Return -1 if the argument type is not supported or + the size is too large; 0 if the argument type is a pointer or the + size if it is integral. */ +static int +speculation_safe_value_resolve_size (tree function, vec<tree, va_gc> *params) +{ + /* Type of the argument. */ + tree type; + int size; + + if (vec_safe_is_empty (params)) + { + error ("too few arguments to function %qE", function); + return -1; + } + + type = TREE_TYPE ((*params)[0]); + if (TREE_CODE (type) == ARRAY_TYPE && c_dialect_cxx ()) + { + /* Force array-to-pointer decay for C++. */ + (*params)[0] = default_conversion ((*params)[0]); + type = TREE_TYPE ((*params)[0]); + } + + if (POINTER_TYPE_P (type)) + return 0; + + if (!INTEGRAL_TYPE_P (type)) + goto incompatible; + + if (!COMPLETE_TYPE_P (type)) + goto incompatible; + + size = tree_to_uhwi (TYPE_SIZE_UNIT (type)); + if (size == 1 || size == 2 || size == 4 || size == 8 || size == 16) + return size; + + incompatible: + /* Issue the diagnostic only if the argument is valid, otherwise + it would be redundant at best and could be misleading. */ + if (type != error_mark_node) + error ("operand type %qT is incompatible with argument %d of %qE", + type, 1, function); + + return -1; +} + +/* Validate and coerce PARAMS, the arguments to ORIG_FUNCTION to fit + the prototype for FUNCTION. The first argument is mandatory, a second + argument, if present, must be type compatible with the first. */ +static bool +speculation_safe_value_resolve_params (location_t loc, tree orig_function, + vec<tree, va_gc> *params) +{ + tree val; + + if (params->length () == 0) + { + error_at (loc, "too few arguments to function %qE", orig_function); + return false; + } + + else if (params->length () > 2) + { + error_at (loc, "too many arguments to function %qE", orig_function); + return false; + } + + val = (*params)[0]; + if (TREE_CODE (TREE_TYPE (val)) == ARRAY_TYPE) + val = default_conversion (val); + if (!(TREE_CODE (TREE_TYPE (val)) == POINTER_TYPE + || TREE_CODE (TREE_TYPE (val)) == INTEGER_TYPE)) + { + error_at (loc, + "expecting argument of type pointer or of type integer " + "for argument 1"); + return false; + } + (*params)[0] = val; + + if (params->length () == 2) + { + tree val2 = (*params)[1]; + if (TREE_CODE (TREE_TYPE (val2)) == ARRAY_TYPE) + val2 = default_conversion (val2); + if (!(TREE_TYPE (val) == TREE_TYPE (val2) + || useless_type_conversion_p (TREE_TYPE (val), TREE_TYPE (val2)))) + { + error_at (loc, "both arguments must be compatible"); + return false; + } + (*params)[1] = val2; + } + + return true; +} + +/* Cast the result of the builtin back to the type of the first argument, + preserving any qualifiers that it might have. */ +static tree +speculation_safe_value_resolve_return (tree first_param, tree result) +{ + tree ptype = TREE_TYPE (first_param); + tree rtype = TREE_TYPE (result); + ptype = TYPE_MAIN_VARIANT (ptype); + + if (tree_int_cst_equal (TYPE_SIZE (ptype), TYPE_SIZE (rtype))) + return convert (ptype, result); + + return result; +} + /* A helper function for resolve_overloaded_builtin in resolving the overloaded __sync_ builtins. Returns a positive power of 2 if the first operand of PARAMS is a pointer to a supported data type. @@ -7111,6 +7226,34 @@ resolve_overloaded_builtin (location_t loc, tree function, /* Handle BUILT_IN_NORMAL here. */ switch (orig_code) { + 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); + + new_function = builtin_decl_explicit (fncode); + first_param = (*params)[0]; + if (!speculation_safe_value_resolve_params (loc, function, params)) + return error_mark_node; + + result = build_function_call_vec (loc, vNULL, new_function, params, + NULL); + + if (result == error_mark_node) + return result; + + return speculation_safe_value_resolve_return (first_param, result); + } + case BUILT_IN_ATOMIC_EXCHANGE: case BUILT_IN_ATOMIC_COMPARE_EXCHANGE: case BUILT_IN_ATOMIC_LOAD: diff --git a/gcc/c-family/c-cppbuiltin.c b/gcc/c-family/c-cppbuiltin.c index bdb5691..0b10e65 100644 --- a/gcc/c-family/c-cppbuiltin.c +++ b/gcc/c-family/c-cppbuiltin.c @@ -1361,7 +1361,10 @@ c_cpp_builtins (cpp_reader *pfile) cpp_define (pfile, "__WCHAR_UNSIGNED__"); cpp_atomic_builtins (pfile); - + + /* Show support for __builtin_speculation_safe_value (). */ + cpp_define (pfile, "__HAVE_SPECULATION_SAFE_VALUE"); + #ifdef DWARF2_UNWIND_INFO if (dwarf2out_do_cfi_asm ()) cpp_define (pfile, "__GCC_HAVE_DWARF2_CFI_ASM"); diff --git a/gcc/doc/cpp.texi b/gcc/doc/cpp.texi index 3f7a8fc..efad2c8 100644 --- a/gcc/doc/cpp.texi +++ b/gcc/doc/cpp.texi @@ -2381,6 +2381,10 @@ If GCC cannot determine the current date, it will emit a warning message These macros are defined when the target processor supports atomic compare and swap operations on operands 1, 2, 4, 8 or 16 bytes in length, respectively. +@item __HAVE_SPECULATION_SAFE_VALUE +This macro is defined with the value 1 to show that this version of GCC +supports @code{__builtin_speculation_safe_value}. + @item __GCC_HAVE_DWARF2_CFI_ASM This macro is defined when the compiler is emitting DWARF CFI directives to the assembler. When this is defined, it is possible to emit those same diff --git a/gcc/doc/extend.texi b/gcc/doc/extend.texi index c7745c4..6eb0c6b 100644 --- a/gcc/doc/extend.texi +++ b/gcc/doc/extend.texi @@ -10935,6 +10935,7 @@ is called and the @var{flag} argument passed to it. @findex __builtin_powi @findex __builtin_powif @findex __builtin_powil +@findex __builtin_speculation_safe_value @findex _Exit @findex _exit @findex abort @@ -11579,6 +11580,34 @@ check its compatibility with @var{size}. @end deftypefn +@deftypefn {Built-in Function} @var{type} __builtin_speculation_safe_value (@var{type} val, @var{type} failval) + +This builtin can be used to help mitigate against unsafe speculative +execution. @var{type} may be any integral type or any pointer type. + +@enumerate +@item +If the CPU is not speculatively executing the code, then @var{val} +is returned. +@item +If the CPU is executing speculatively then either: +@itemize +@item +The function may cause execution to pause until it is known that the +code is no-longer being executed speculatively (in which case +@var{val} can be returned, as above); or +@item +The function may use target-dependent speculation tracking state to cause +@var{failval} to be returned when it is known that speculative +execution has incorrectly predicted a conditional branch operation. +@end itemize +@end enumerate + +The second argument, @var{failval}, is optional and defaults to zero +if omitted. + +@end deftypefn + @deftypefn {Built-in Function} int __builtin_types_compatible_p (@var{type1}, @var{type2}) You can use the built-in function @code{__builtin_types_compatible_p} to diff --git a/gcc/doc/md.texi b/gcc/doc/md.texi index 6d15d99..5de27f6 100644 --- a/gcc/doc/md.texi +++ b/gcc/doc/md.texi @@ -7026,6 +7026,21 @@ should be defined to an instruction that orders both loads and stores before the instruction with respect to loads and stores after the instruction. This pattern has no operands. +@cindex @code{speculation_barrier} instruction pattern +@item @samp{speculation_barrier} +If the target can support speculative execution, then this pattern should +be defined to an instruction that will block subsequent execution until +any prior speculation conditions has been resolved. The pattern must also +ensure that the compiler cannot move memory operations past the barrier, +so it needs to be an UNSPEC_VOLATILE pattern. The pattern has no +operands. + +If this pattern is not defined then the default expansion of +@code{__builtin_speculation_safe_value} will emit a warning. You can +suppress this warning by defining this pattern with a final condition +of @code{0} (zero), which tells the compiler that a speculation +barrier is not needed for this target. + @cindex @code{sync_compare_and_swap@var{mode}} instruction pattern @item @samp{sync_compare_and_swap@var{mode}} This pattern, if defined, emits code for an atomic compare-and-swap diff --git a/gcc/doc/tm.texi b/gcc/doc/tm.texi index 7e2cdc2..681e53b 100644 --- a/gcc/doc/tm.texi +++ b/gcc/doc/tm.texi @@ -11932,6 +11932,26 @@ maintainer is familiar with. @end defmac +@deftypefn {Target Hook} rtx TARGET_SPECULATION_SAFE_VALUE (machine_mode @var{mode}, rtx @var{result}, rtx @var{val}, rtx @var{failval}) +This target hook can be used to generate a target-specific code + sequence that implements the @code{__builtin_speculation_safe_value} + built-in function. The function must always return @var{val} in + @var{result} in mode @var{mode} when the cpu is not executing + speculatively, but must never return that when speculating until it + is known that the speculation will not be unwound. The hook supports + two primary mechanisms for implementing the requirements. The first + is to emit a speculation barrier which forces the processor to wait + until all prior speculative operations have been resolved; the second + is to use a target-specific mechanism that can track the speculation + state and to return @var{failval} if it can determine that + speculation must be unwound at a later time. + + The default implementation simply copies @var{val} to @var{result} and + emits a @code{speculation_barrier} instruction if that is defined. If + @code{speculation_barrier} is not defined for the target a warning will + be generated. +@end deftypefn + @deftypefn {Target Hook} void TARGET_RUN_TARGET_SELFTESTS (void) If selftests are enabled, run any selftests for this target. @end deftypefn diff --git a/gcc/doc/tm.texi.in b/gcc/doc/tm.texi.in index b7b0e8a..6e20afb 100644 --- a/gcc/doc/tm.texi.in +++ b/gcc/doc/tm.texi.in @@ -8107,4 +8107,6 @@ maintainer is familiar with. @end defmac +@hook TARGET_SPECULATION_SAFE_VALUE + @hook TARGET_RUN_TARGET_SELFTESTS diff --git a/gcc/target.def b/gcc/target.def index 112c772..c8bd7f8 100644 --- a/gcc/target.def +++ b/gcc/target.def @@ -4177,6 +4177,29 @@ DEFHOOK hook_bool_void_true) DEFHOOK +(speculation_safe_value, +"This target hook can be used to generate a target-specific code\n\ + sequence that implements the @code{__builtin_speculation_safe_value}\n\ + built-in function. The function must always return @var{val} in\n\ + @var{result} in mode @var{mode} when the cpu is not executing\n\ + speculatively, but must never return that when speculating until it\n\ + is known that the speculation will not be unwound. The hook supports\n\ + two primary mechanisms for implementing the requirements. The first\n\ + is to emit a speculation barrier which forces the processor to wait\n\ + until all prior speculative operations have been resolved; the second\n\ + is to use a target-specific mechanism that can track the speculation\n\ + state and to return @var{failval} if it can determine that\n\ + speculation must be unwound at a later time.\n\ + \n\ + The default implementation simply copies @var{val} to @var{result} and\n\ + emits a @code{speculation_barrier} instruction if that is defined. If\n\ + @code{speculation_barrier} is not defined for the target a warning will\n\ + be generated.", +rtx, (machine_mode mode, rtx result, rtx val, rtx failval), + default_speculation_safe_value) + + +DEFHOOK (can_use_doloop_p, "Return true if it is possible to use low-overhead loops (@code{doloop_end}\n\ and @code{doloop_begin}) for a particular loop. @var{iterations} gives the\n\ diff --git a/gcc/targhooks.c b/gcc/targhooks.c index 7315f1a..2061f07 100644 --- a/gcc/targhooks.c +++ b/gcc/targhooks.c @@ -2306,4 +2306,31 @@ default_select_early_remat_modes (sbitmap) { } +/* Default implementation of the speculation-safe-load builtin. This + implementation simply copies val to result and generates a + speculation_barrier insn, if such a pattern is defined. If + speculation_barrier is not defined at all, a warning is generated. */ + +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; +} + #include "gt-targhooks.h" diff --git a/gcc/targhooks.h b/gcc/targhooks.h index 4107e22..80ac283 100644 --- a/gcc/targhooks.h +++ b/gcc/targhooks.h @@ -284,4 +284,6 @@ default_excess_precision (enum excess_precision_type ATTRIBUTE_UNUSED); extern bool default_stack_clash_protection_final_dynamic_probe (rtx); extern void default_select_early_remat_modes (sbitmap); +extern rtx default_speculation_safe_value (machine_mode, rtx, rtx, rtx); + #endif /* GCC_TARGHOOKS_H */ diff --git a/gcc/testsuite/gcc.dg/spec-barrier-1.c b/gcc/testsuite/gcc.dg/spec-barrier-1.c new file mode 100644 index 0000000..106f89a --- /dev/null +++ b/gcc/testsuite/gcc.dg/spec-barrier-1.c @@ -0,0 +1,40 @@ +/* { dg-do run } */ +/* { dg-options "-O" } */ + +/* Test that __builtin_speculation_safe_value returns the correct value. */ +/* This test will cause an unfiltered warning to be emitted on targets + that have not implemented support for speculative execution + barriers. They should fix that rather than disabling this + test. */ +char a = 1; +short b = 2; +int c = 3; +long d = 4; +long long e = 5; +int *f = (int*) &c; +#ifdef __SIZEOF_INT128__ +__int128 g = 9; +#endif + +extern void abort (void); + +int main () +{ + if (__builtin_speculation_safe_value (a) != 1) + abort (); + if (__builtin_speculation_safe_value (b) != 2) + abort (); + if (__builtin_speculation_safe_value (c) != 3) + abort (); + if (__builtin_speculation_safe_value (d) != 4) + abort (); + if (__builtin_speculation_safe_value (e) != 5) + abort (); + if (__builtin_speculation_safe_value (f) != &c) + abort (); +#ifdef __SIZEOF_INT128__ + if (__builtin_speculation_safe_value (g) != 9) + abort (); +#endif + return 0; +} diff --git a/gcc/testsuite/gcc.dg/spec-barrier-2.c b/gcc/testsuite/gcc.dg/spec-barrier-2.c new file mode 100644 index 0000000..7e9c497 --- /dev/null +++ b/gcc/testsuite/gcc.dg/spec-barrier-2.c @@ -0,0 +1,19 @@ +/* { dg-do run } */ + +/* Even on targets that don't need the optional failval parameter, + side-effects on the operand should still be calculated. */ + +int x = 3; +volatile int y = 9; + +extern void abort (void); + +int main () +{ + int z = __builtin_speculation_safe_value (x, y++); + if (z != 3 || y != 10) + abort (); + return 0; +} + +/* { dg-prune-output "this target does not define a speculation barrier;" } */ diff --git a/gcc/testsuite/gcc.dg/spec-barrier-3.c b/gcc/testsuite/gcc.dg/spec-barrier-3.c new file mode 100644 index 0000000..3ed4d39 --- /dev/null +++ b/gcc/testsuite/gcc.dg/spec-barrier-3.c @@ -0,0 +1,13 @@ +/* { dg-do compile } */ +/* { dg-options "-Wpedantic" } */ + +/* __builtin_speculation_safe_value returns a value with the same type + as its first argument. There should be a warning if that isn't + type-compatible with the use. */ +int * +f (int x) +{ + return __builtin_speculation_safe_value (x); /* { dg-warning "returning 'int' from a function with return type 'int \\*' makes pointer from integer without a cast" } */ +} + +/* { dg-prune-output "this target does not define a speculation barrier;" } */