[PATCH 1/3] [builtins] Generic support for __builtin_speculation_safe_load()

[Richard Earnshaw (lists)]

On 18/01/18 12:44, Richard Biener wrote:
> On Wed, Jan 17, 2018 at 3:55 PM, Richard Earnshaw
> <Richard.Earnshaw@arm.com> wrote:
>>
>> This patch adds generic support for the new builtin
>> __builtin_speculation_safe_load.  It provides the overloading of the
>> different access sizes and a default fall-back expansion for targets
>> that do not support a mechanism for inhibiting speculation.
> 
> +  if (ignore)
> +    {
> +      warning_at (input_location, 0,
> +                 "result of __builtin_speculation_safe_load must be used to "
> +                 "ensure correct operation");
> +      target = NULL;
> +    }
> 
> This warning cannot be disabled but these kind of cases could appear via
> path isolation or missed optimizations since memory optimizations do not
> recognize such speculative loads.  So - should it not be emitted way
> earlier instead, like from the FEs or during some early warning pass?

If you think so, and can recommend where to put it.  I'm not
particularly familiar with the front-end code, being mostly a back-end
maintainer.  Otherwise we could probably drop this entirely, but that
gives scope for users to do the Wrong Thing(tm)!

> 
> In which case it could be an error as well, no?
> 
> +  set_mem_align (mem, MAX (GET_MODE_ALIGNMENT (mode),
> +                          get_pointer_alignment (arg0)));
> 
> err...  so there's no way to do an unaligned speculation safe load?  Using
> just get_pointer_alignment would be safe here.   Other builtins/IFNs explicitely
> pass down the alignment and FEs generate such alignment from their
> language constraints.

No, last time I looked GCC didn't support taking the address of an
unaligned object and casting pointer to char to pointer to int implies
you know that it is correctly aligned.

> 
> +  set_mem_alias_set (mem, get_alias_set (TREE_TYPE (TREE_TYPE (arg0))));
> 
> sorry, but pointer types are arbitrary in GIMPLE so this is clearly
> wrong.  Without
> more information you have to use zero.  Thus,
> 
> __builtin_speculation_safe_load ((int *)p, ...);
> 
> will do the wrong thing if the type of p is not int *.

Ok, I'll fix that.

> 
> +  /* Mark the memory access as volatile.  We don't want the optimizers to
> +     move it or otherwise substitue an alternative value.  */
> +  MEM_VOLATILE_P (mem) = 1;
> 
> how is moving or substituting in any way related to inhibiting speculation?
> Why's this done for all targets rather than only those that need any such
> mitigation?

I can move that, but it seemed right to do it here.

> 
> I btw miss implementation for x86_64 (at least) where the agreed upon mitigation
> is to insert 'lfence' before the load.

/I'm/ not planning on doing implementations for other targets.  I don't
know those back-ends.  Sorry.  It really needs the attention of a port
expert.

> 
> Any reason the builtin expansion is not fully left to the targets?

Because that would be a significant impediment to deploying the builtin
in generic code.  The compiler would have to error out if it couldn't
handle the builtin at all.  By providing a generic implementation we can
support everything, even if you do get a warning.

> 
> +/* 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_LOAD_N, "speculation_safe_load",
> +                BT_FN_VOID_VAR, ATTR_NULL)
> +
> +DEF_GCC_BUILTIN (BUILT_IN_SPECULATION_SAFE_LOAD_1, "speculation_safe_load_1",
> +                BT_FN_I1_CONST_VPTR_CONST_VPTR_CONST_VPTR_VAR, ATTR_NULL)
> +DEF_GCC_BUILTIN (BUILT_IN_SPECULATION_SAFE_LOAD_2, "speculation_safe_load_2",
> +                BT_FN_I2_CONST_VPTR_CONST_VPTR_CONST_VPTR_VAR, ATTR_NULL)
> 
> any reason you are not simply using an internal function for the
> non-_N variants?
> Looks like you're closely following the atomic/sync builtins but those
> predate IFNs I think.
> They also have more constraints on alignment and use a restrictive alias set.

Wouldn't that make it more difficult to do backports to older versions
of GCC?  I know some folk are interested in going back as far as
gcc-4.8, and maybe even earlier.

> 
> +rtx
> +default_speculation_safe_load (machine_mode mode ATTRIBUTE_UNUSED,
> +                              rtx result, rtx mem, rtx lower_bound,
> +                              rtx upper_bound, rtx cmpptr, bool warn)
> +{
> +  rtx_code_label *done_label = gen_label_rtx ();
> +  rtx_code_label *inrange_label = gen_label_rtx ();
> +
> +  if (warn)
> +    warning_at
> +      (input_location, 0,
> +       "this target does not support anti-speculation operations.  "
> +       "Your program will still execute correctly, but speculation "
> +       "will not be inhibited");
> 
> so there's no way to inhibit this warning than changing all targets?

No.  It's trivial to bind this to a hook that just passes all the other
arguments through to the generic code with the warning suppressed - we
could even add an additional definition to do that.  But binding to it
directly as the default means that port maintainers may not notice that
they need to take action here; even if that is to confirm that they've
nothing to worry about.

> 
> Iff it is correct for a target to expand this to an unconditional load with a
> preceeding fence why not have the generic target hook implementation
> do exactly that - emit an unconditional move?  Maybe I misunderstand
> the clear words in the hook docs, but the builtin docs also say
> the behavior is undefined if the builtin is executed when the condition
> is false - which means traps are allowed.
> 

Certainly a permitted implementation of the builtin would conditionally
nullify the pointer if it were out-of-bounds (rather than nullifying the
result).  That would then fault if reached when not speculating.

As to why that isn't the default implementation, three reasons.  Firstly
the code here is mostly unchanged from the first implementation I did
which required the conditional behaviour.  Secondly, on some systems
with simple static branch predictors and speculation the default
expansion may be "good enough" without an additional barrier.  Finally,
not all architectures define a suitable barrier so there's nothing to
expand to in that case.

R.

> Richard.
> 
> 
>>         * builtin-types.def (BT_FN_I1_CONST_VPTR_CONST_VPTR_CONST_VPTR_VAR):
>>         New builtin type signature.
>>         (BT_FN_I2_CONST_VPTR_CONST_VPTR_CONST_VPTR_VAR): Likewise.
>>         (BT_FN_I4_CONST_VPTR_CONST_VPTR_CONST_VPTR_VAR): Likewise.
>>         (BT_FN_I8_CONST_VPTR_CONST_VPTR_CONST_VPTR_VAR): Likewise.
>>         (BT_FN_I16_CONST_VPTR_CONST_VPTR_CONST_VPTR_VAR): Likewise.
>>         * builtins.def (BUILT_IN_SPECULATION_SAFE_LOAD_N): New builtin.
>>         (BUILT_IN_SPECULATION_SAFE_LOAD_1): Likewise.
>>         (BUILT_IN_SPECULATION_SAFE_LOAD_2): Likewise.
>>         (BUILT_IN_SPECULATION_SAFE_LOAD_4): Likewise.
>>         (BUILT_IN_SPECULATION_SAFE_LOAD_8): Likewise.
>>         (BUILT_IN_SPECULATION_SAFE_LOAD_16): Likewise.
>>         * target.def (speculation_safe_load): New hook.
>>         * doc/tm.texi.in (TARGET_SPECULATION_SAFE_LOAD): Add to
>>         documentation.
>>         * doc/tm.texi: Regenerated.
>>         * doc/cpp.texi: Document __HAVE_SPECULATION_SAFE_LOAD.
>>         * doc/extend.texi: Document __builtin_speculation_safe_load.
>>         * c-family/c-common.c (speculation_safe_load_resolve_size): New
>>         function.
>>         (speculation_safe_load_resolve_params): New function.
>>         (speculation_safe_load_resolve_return): New function.
>>         (resolve_overloaded_builtin): Handle overloading
>>         __builtin_speculation_safe_load.
>>         * builtins.c (expand_speculation_safe_load): New function.
>>         (expand_builtin): Handle new speculation-safe builtins.
>>         * targhooks.h (default_speculation_safe_load): Declare.
>>         * targhooks.c (default_speculation_safe_load): New function.
>> ---
>>  gcc/builtin-types.def       |  16 +++++
>>  gcc/builtins.c              |  81 +++++++++++++++++++++++
>>  gcc/builtins.def            |  17 +++++
>>  gcc/c-family/c-common.c     | 152 ++++++++++++++++++++++++++++++++++++++++++++
>>  gcc/c-family/c-cppbuiltin.c |   5 +-
>>  gcc/doc/cpp.texi            |   4 ++
>>  gcc/doc/extend.texi         |  68 ++++++++++++++++++++
>>  gcc/doc/tm.texi             |   9 +++
>>  gcc/doc/tm.texi.in          |   2 +
>>  gcc/target.def              |  34 ++++++++++
>>  gcc/targhooks.c             |  59 +++++++++++++++++
>>  gcc/targhooks.h             |   3 +
>>  12 files changed, 449 insertions(+), 1 deletion(-)
>>