llvm dev - Feb 2018 - retpoline mitigation and 6.0

On Fri, Feb 2, 2018 at 4:03 PM David Woodhouse <dwmw2 at infradead.org>
wrote:
> On Thu, 2018-02-01 at 10:10 +0100, Hans Wennborg via llvm-dev wrote:
> >
> > I saw the retpoline mitigation landed in r323155. Are we ready to
> > merge this to 6.0, or are there any open issues that we're waiting
> > for? Also, were there any followups I should know about? Also,
> > release notes please :-)
>
> Eep, please can we keep the command line option for clang and the thunk
> ABI matching what GCC, the Linux kernel and Xen are all doing?
>
> To say that I am not stunningly keen on
> https://lkml.org/lkml/2018/2/2/975 would be a bit of an
> understatement...

Two aspects to this...

One, we're somewhat reluctant to guarantee an ABI here. At least I am.
While we don't *expect* rampant divergence here, I don't want this to
become something we cannot change if there are good reasons to do so. We've
already changed the thunks once based on feedback (putting LFENCE after the
PAUSE).

Given that we don't want this to be a guaranteed part of the ABI, I really
want the thunk names to be specific about which implementation is being
used. For example, if we come up with a new, better thunk interface, we
would want to give it a new name (even if it were just a suffix of
"2") so
that we don't get runtime failures. Since LLVM and GCC can't possible
release in sync, IMO they *should* use different names. I asked the GCC
developers to include 'gcc' in the name, but at least the person I asked
was not at all receptive.


Two, I actually agree with you about the command line flags. I asked for it
to be '-mretpoline'. I think a short, clear flag is really best, and
we've
very publicly documented this technique as 'retpoline'. But the GCC
community has a fairly different design AFAICT... The only embedded thunks
the offer are inline (ours are always out-of-line, even if they aren't
external). Also, they support thunking indirect branches that we don't:
ret. Our feature really is oriented around exposing the retpoline
mitigation, and I don't think there is any demand for the complexity that
would be entailed by generalizing it further. But I'm sure the GCC folks
have a very reasonably different perspective here that make them prefer a
quite different feature (they can thunk rets) and naming scheme.


So I don't really know how to fix this. I think there are reasonable
arguments in each direction. IMO, the easiest improvement would be for GCC
to add command line aliases spelled using 'retpoline' for the specific
use
case, but keep the more general commandline interface for which they have
reasonable use cases.

The naming thing I think is essentially by-design so that if we (or you)
need to split apart the implementation, there are distinct names available.
Until then, aliases seem very cheap to maintain?
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20180203/223d1983/attachment.html>

On Fri, Feb 2, 2018 at 4:23 PM Chandler Carruth <chandlerc at google.com>
wrote:
> On Fri, Feb 2, 2018 at 4:03 PM David Woodhouse <dwmw2 at
infradead.org>
> wrote:
>
>> On Thu, 2018-02-01 at 10:10 +0100, Hans Wennborg via llvm-dev wrote:
>> >
>> > I saw the retpoline mitigation landed in r323155. Are we ready to
>> > merge this to 6.0, or are there any open issues that we're
waiting
>> > for? Also, were there any followups I should know about? Also,
>> > release notes please :-)
>>
>> Eep, please can we keep the command line option for clang and the thunk
>> ABI matching what GCC, the Linux kernel and Xen are all doing?
>>
>> To say that I am not stunningly keen on
>> https://lkml.org/lkml/2018/2/2/975 would be a bit of an
>> understatement...
>
>
A minor note on this specific patch:

You don't need '-mretpoline -mretpoline-external-thunk'. The second
flag
implies the first. (Or should, if not, its a bug.) Our goal was that you
needed exactly one flag to enable this in whatever form.
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20180203/f0d1405f/attachment-0001.html>

On Sat, 2018-02-03 at 00:23 +0000, Chandler Carruth
wrote:
> 
> Two aspects to this...
> 
> One, we're somewhat reluctant to guarantee an ABI here. At least I
> am. While we don't *expect* rampant divergence here, I don't want
> this to become something we cannot change if there are good reasons
> to do so. We've already changed the thunks once based on feedback
> (putting LFENCE after the PAUSE).
Surely adding the lfence was changing your implementation, not the ABI?

And if we really are talking about the *ABI* not the implementation,
I'm not sure I understand your concern.

The ABI for each thunk is that it is identical in all respects, apart
from speculation, to 'jump *\reg'. There's not a lot of scope for a
'v2' of that, surely?

I could live with the command-line divergence, although it's
suboptimal. But *please* since these things also need to be symbols
exported to loadable modules, can't we keep the thunk names identical?

You can even use __x86_indirect_thunk_\reg for *now* and reserve the
right to use a different name if you ever do revise the ABI.

-------------- next part --------------
A non-text attachment was scrubbed...
Name: smime.p7s
Type: application/x-pkcs7-signature
Size: 5213 bytes
Desc: not available
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20180203/e847d501/attachment.bin>

On Fri, Feb 2, 2018 at 4:36 PM David Woodhouse <dwmw2 at infradead.org>
wrote:
> On Sat, 2018-02-03 at 00:23 +0000, Chandler Carruth wrote:
> >
> > Two aspects to this...
> >
> > One, we're somewhat reluctant to guarantee an ABI here. At least I
> > am. While we don't *expect* rampant divergence here, I don't
want
> > this to become something we cannot change if there are good reasons
> > to do so. We've already changed the thunks once based on feedback
> > (putting LFENCE after the PAUSE).
>
> Surely adding the lfence was changing your implementation, not the ABI?
>
> And if we really are talking about the *ABI* not the implementation,
> I'm not sure I understand your concern.
>
> The ABI for each thunk is that it is identical in all respects, apart
> from speculation, to 'jump *\reg'. There's not a lot of scope
for a
> 'v2' of that, surely?
>
While I hope we have converged and never need to chaneg them, when we
started, there was actually a substantially different ABI proposed, and
multiple different ones. So some changes already were needed.

We already have at least one change that has been proposed, but we haven't
decided to pursue yet: a thunk which includes the *load* of the address,
and specifically a collection to handle common repeated patterns of loads
that before retpoline would have folded into addressing modes. And these
would definitely have a different ABI.

>
> I could live with the command-line divergence, although it's
> suboptimal. But *please* since these things also need to be symbols
> exported to loadable modules, can't we keep the thunk names identical?
>
At least for LLVM when not using *external* thunks, we specifically *do not
export* the symbol. As a consequence, DSOs built with two versions of LLVM
(or LLVM and GCC) with different thunk behavior and each will find the
correct thunk. That was a specific part of the design.

While you *can* export your external thunk, that's a choice of the code
defining the thunk.

>
> You can even use __x86_indirect_thunk_\reg for *now* and reserve the
> right to use a different name if you ever do revise the ABI.
>
Maybe I don't understand, but I'm surprised that there is a significant
burden to having aliases for now instead?
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20180203/26baaa32/attachment.html>

On 02/02/2018 04:27 PM, Chandler Carruth wrote:
> On Fri, Feb 2, 2018 at 4:23 PM Chandler Carruth <chandlerc at google.com
<mailto:chandlerc at google.com>> wrote:
> 
>     On Fri, Feb 2, 2018 at 4:03 PM David Woodhouse <dwmw2 at
infradead.org <mailto:dwmw2 at infradead.org>> wrote:
> 
>         On Thu, 2018-02-01 at 10:10 +0100, Hans Wennborg via llvm-dev
wrote:
>          >
>          > I saw the retpoline mitigation landed in r323155. Are we
ready to
>          > merge this to 6.0, or are there any open issues that
we're waiting
>          > for? Also, were there any followups I should know about?
Also,
>          > release notes please :-)
> 
>         Eep, please can we keep the command line option for clang and the
thunk
>         ABI matching what GCC, the Linux kernel and Xen are all doing?
> 
>         To say that I am not stunningly keen on
>         https://lkml.org/lkml/2018/2/2/975 would be a bit of an
>         understatement...
> 
> 
> A minor note on this specific patch:
> 
> You don't need '-mretpoline -mretpoline-external-thunk'. The
second flag implies the first. (Or should, if not, its a bug.) Our goal was that
you needed exactly one flag to enable this in whatever form.
The llvm commit log says:

"... They can write this custom thunk and use `-mretpoline-external-thunk`
*in addition* to `-mretpoline`. In this case, on x86-64 thu thunk names
must be: ... "

which I understand to mean that _both_ are needed. I'll be happy to stand
corrected if that is guaranteed not to be the case.

Thanks,
Guenter