Page MenuHomePhabricator

Make variable argument intrinsics behave correctly in a Win64 CC function.

Authored by cdavis5x on Sep 6 2013, 1:43 PM.



This change makes the variable argument intrinsics, llvm.va_start and
llvm.va_copy, and the va_arg instruction behave as they do on Windows
inside a CallingConv::X86_64_Win64 function. It's needed for a Clang patch
I have to add support for GCC's __builtin_ms_va_list constructs.

Diff Detail

Event Timeline

Adding Owen to review the changes to SelectionDAG.

The generic SelectionDAG changes look fine to me in principle. I'm not familiar with the X86 or MS-specific lowering.

@resistor wrote:

The generic SelectionDAG changes look fine to me in principle. I'm not familiar with the X86 or MS-specific lowering.

I know. I just wanted to make sure the SelectionDAG changes were fine.

eugenis added inline comments.Sep 8 2013, 11:55 PM

I'd prefer if this was done in CreateVarArgHelper() by selecting VarArgNoOpHelper for Win64.

cdavis5x updated this revision to Unknown Object (????).Sep 9 2013, 6:37 PM

Create a no-op varargs handler in msan for a Win64 ABI function.

cdavis5x added inline comments.Sep 9 2013, 6:39 PM

Done. (I didn't do this for Win64 targets, only for functions with the Win64 CC.)

eli.friedman added inline comments.Sep 9 2013, 7:36 PM

Checking DAG.getMachineFunction().getFunction()->getCallingConv() is fine for va_start, but it isn't correct for va_arg or va_copy: a function using the Win64 calling convention can pass its va_list to a function using the AMD calling convention, or vice versa. And even if nobody writes code like that, the optimizer could potentially introduce such constructs.

eugenis accepted this revision.Sep 9 2013, 11:56 PM

MSan change looks good.

cdavis5x updated this revision to Unknown Object (????).Sep 12 2013, 5:12 PM

Attempt to address Eli's concerns.

  • Make Win64 ABI variable argument intrinsics work even in a SysV ABI function.
  • Sink the Win64 ABI checks back into the visitVA*Inst functions in msan. (Sorry, but I had to do it that way because of the first change.)
cdavis5x added inline comments.Sep 12 2013, 5:16 PM

I've attempted to address this now by having the lowering code for va_arg and va_copy try to do Win64-style lowering based on the types of the arguments, but I'm not sure if this is the right way to go.

eugenis added inline comments.Sep 13 2013, 1:48 AM

This looks very fragile.
Why can't you decide this based on the calling convention of the instrumented function?

eugenis added inline comments.Sep 13 2013, 2:14 AM

OK, va_list may be from a function with a different calling convention.
I don't see a better way to detect this, but... how does va_copy work in this case? It gets plain i8* as an argument.

cdavis5x added inline comments.Sep 13 2013, 11:38 AM

Remember how LLVM IR works--it's single-static assignment. All instructions are values. The IR for a Win64 ABI va_copy typically looks something like this:

%ap = alloca i8*, align 8
%cp = alloca i8*, align 8
%ap1 = bitcast i8** %ap to i8*
%cp1 = bitcast i8** %cp to i8*
call void @llvm.va_copy(i8* %cp1, i8* %ap1)

So, I dyn_cast the argument to an llvm::BitCastInst (and hope that the optimizer didn't make the bitcast go away--yes, this is very fragile, but I can't think of any better way to do this), and check if the source type is a i8** (the argument to the bitcast is a pointer to the va_list itself). If it is, we assume it's a Win64 ABI va_copy, else, a System V ABI va_copy.

Now that I think about it, it might be better if I just add a new intrinsic (llvm.ms_va_copy, maybe?) to handle Win64 ABI va_copy calls, but when I tried to do it this way, I couldn't work out how to use the generic lowering code (that I pulled out of LegalizeDAG) to lower it. (In retrospect, I should probably have asked about it earlier on the list.)

cdavis5x updated this revision to Diff 30720.Jul 27 2015, 12:40 PM
cdavis5x edited edge metadata.

Big belated update:

  • Rebase onto rL243284.
  • Go back to checking if the containing function uses the Win64 ABI. Opaque pointer types make checking the va_list type difficult.
  • Don't do the "don't save XMM registers" dance in a variadic Win64 ABI function on a SysV platform. (This was something I missed last time.)
This revision now requires review to proceed.Jul 27 2015, 12:40 PM
rnk accepted this revision.Jul 27 2015, 1:55 PM
rnk added a reviewer: rnk.
rnk added a subscriber: rnk.

Looks good to me. Owen also mentioned that the SDAG changes were fine. They are mostly code motion.

This revision is now accepted and ready to land.Jul 27 2015, 1:55 PM
asl accepted this revision.Aug 4 2015, 3:16 AM
asl edited edge metadata.
cdavis5x updated this revision to Diff 32780.Aug 20 2015, 5:59 PM
cdavis5x edited edge metadata.

Gut target OS assertion from LowerVAARG.

I tried very hard not to change its current behavior other than inside
a Win64 ABI function.

I know you said it was OK to commit, but I want to run this one last
change by you guys before I go ahead.

rnk added inline comments.Aug 24 2015, 11:49 AM

I'm assuming this extra condition was needed for some test case? Should we change isCallingConvWin64 instead maybe?

cdavis5x added inline comments.Aug 24 2015, 12:12 PM

This was me trying really hard--too hard?--not to change the existing behavior. Basically, prior to this change on Windows, we always had SelectionDAG expand va_arg instructions assuming a char * pointer. Now that I've reread the isCallingConvWin64 function source, I realize this isn't really necessary.

cdavis5x updated this revision to Diff 33051.Aug 25 2015, 12:37 AM

Just call X86Subtarget::isCallingConvWin64 in LowerVAARG.

Don't bother checking if the function has a System V calling
convention on Windows; just use X86Subtarget::isCallingConvWin64.
That should be good enough for our purposes, since we're just going
to manually lower __builtin_va_arg on a __builtin_ms_va_list
anyhow. (I only added lowering of the va_arg instruction for
completeness here.)

cdavis5x closed this revision.Aug 25 2015, 5:19 PM

Closing manually because Phabricator for some reason didn't close it for me...