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

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

# Details

Reviewers
 nadav eugenis rnk resistor asl
Summary

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
lib/Transforms/Instrumentation/MemorySanitizer.cpp
2861

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
lib/Transforms/Instrumentation/MemorySanitizer.cpp
2861

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
lib/Target/X86/X86ISelLowering.cpp
15116

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
lib/Target/X86/X86ISelLowering.cpp
15116

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
lib/Transforms/Instrumentation/MemorySanitizer.cpp
2878

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
lib/Transforms/Instrumentation/MemorySanitizer.cpp
2878

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
lib/Transforms/Instrumentation/MemorySanitizer.cpp
2878

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
lib/Target/X86/X86ISelLowering.cpp
15114–15116

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
lib/Target/X86/X86ISelLowering.cpp
15114–15116

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...