This is an archive of the discontinued LLVM Phabricator instance.

[musttail] Don't forward incoming registers over call site registers
ClosedPublic

Authored by rnk on Nov 18 2019, 3:40 PM.

Details

Summary

AL is only used for varargs on SysV platforms. Don't forward it on
Windows. This allows control flow guard to set up an extra hidden
parameter in RAX, as described in PR44049.

This also has the effect of freeing up RAX for use in virtual member
pointer thunks, which may also be a nice little code size improvement on
Win64.

Fixes PR44049

Diff Detail

Event Timeline

rnk created this revision.Nov 18 2019, 3:40 PM
Herald added a project: Restricted Project. · View Herald TranscriptNov 18 2019, 3:40 PM
Herald added a subscriber: hiraditya. · View Herald Transcript
efriedma added inline comments.Nov 18 2019, 3:51 PM
llvm/lib/Target/X86/X86ISelLowering.cpp
3912

This doesn't seem right... either we need to forward the value, or we don't.

We shouldn't be forwarding AL for Windows callling conventions; that convention for specifying the number of xmm arguments only applies to the "Linux" varargs ABI.

rnk marked an inline comment as done.Nov 18 2019, 4:04 PM
rnk added inline comments.
llvm/lib/Target/X86/X86ISelLowering.cpp
3912

So, I actually feel like musttail thunks really shouldn't assume a calling convention at all, they should simply preserve all non-CSR registers, including things like RAX, R11, R10, which are often scratch, and used for nest parameters, this CFG side parameter, and retpoline thunks. This makes it a much more general device for doing perfect forwarding. This change here is a step in that direction.

efriedma added inline comments.Nov 18 2019, 4:29 PM
llvm/lib/Target/X86/X86ISelLowering.cpp
3912

I guess my concern here, really, is that the "CCInfo.isAllocated" check seems sort of fragile? But I guess it's a fundamental part of the way the forwarding works; we have argument registers that may or may not be used by a known argument.

It still seems confusing if getForwardedMustTailRegParms() contains a register we know doesn't actually need to forward. And having a comment here describing that unnecessary functionality doesn't help.

Thanks for finding and fixing this @rnk! I've double-checked that Windows doesn't use AL for varargs (https://docs.microsoft.com/en-us/cpp/build/x64-calling-convention?view=vs-2019#varargs), but having a general mechanism for forwarding that doesn't assume a particular calling convention sounds like a good approach. This looks good from the control flow guard perspective.

rnk edited the summary of this revision. (Show Details)Nov 19 2019, 4:53 PM
rnk updated this revision to Diff 230176.Nov 19 2019, 4:54 PM
  • Don't forward AL on Windows instead.
This revision is now accepted and ready to land.Nov 19 2019, 5:05 PM
rnk added a comment.Nov 19 2019, 5:23 PM

Thanks for the quick review, sorry for creating the extra back & forth.

This revision was automatically updated to reflect the committed changes.
dmajor added a subscriber: dmajor.Nov 19 2019, 6:00 PM