On 24/07/18 18:26, Richard Biener wrote:
> On Mon, Jul 9, 2018 at 6:40 PM Richard Earnshaw
> <richard.earns...@arm.com> wrote:
>>
>>
>> This patch defines a new intrinsic function
>> __builtin_speculation_safe_value. A generic default implementation is
>> defined which will attempt to use the backend pattern
>> "speculation_safe_barrier". If this pattern is not defined, or if it
>> is not available, then the compiler will emit a warning, but
>> compilation will continue.
>>
>> Note that the test spec-barrier-1.c will currently fail on all
>> targets. This is deliberate, the failure will go away when
>> appropriate action is taken for each target backend.
>
> So given this series is supposed to be backported I question
>
> +rtx
> +default_speculation_safe_value (machine_mode mode ATTRIBUTE_UNUSED,
> + rtx result, rtx val,
> + rtx failval ATTRIBUTE_UNUSED)
> +{
> + emit_move_insn (result, val);
> +#ifdef HAVE_speculation_barrier
> + /* Assume the target knows what it is doing: if it defines a
> + speculation barrier, but it is not enabled, then assume that one
> + isn't needed. */
> + if (HAVE_speculation_barrier)
> + emit_insn (gen_speculation_barrier ());
> +
> +#else
> + warning_at (input_location, 0,
> + "this target does not define a speculation barrier; "
> + "your program will still execute correctly, but speculation "
> + "will not be inhibited");
> +#endif
> + return result;
>
> which makes all but aarch64 archs warn on __bultin_speculation_safe_value
> uses, even those that do not suffer from Spectre like all those embedded
> targets
> where implementations usually do not speculate at all.
>
> In fact for those targets the builtin stays in the way of optimization on
> GIMPLE
> as well so we should fold it away early if neither the target hook is
> implemented
> nor there is a speculation_barrier insn.
>
> So, please make resolve_overloaded_builtin return a no-op on such targets
> which means you can remove the above warning. Maybe such targets
> shouldn't advertise / initialize the builtins at all?
I disagree with your approach here. Why would users not want to know
when the compiler is failing to implement a security feature when it
should? As for targets that don't need something, they can easily
define the hook as described to suppress the warning.
Or are you just suggesting moving the warning to resolve overloaded builtin.
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
>>
