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.
Details
Diff Detail
Event Timeline
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.
lib/Transforms/Instrumentation/MemorySanitizer.cpp | ||
---|---|---|
2865 | I'd prefer if this was done in CreateVarArgHelper() by selecting VarArgNoOpHelper for Win64. |
lib/Transforms/Instrumentation/MemorySanitizer.cpp | ||
---|---|---|
2865 | Done. (I didn't do this for Win64 targets, only for functions with the Win64 CC.) |
lib/Target/X86/X86ISelLowering.cpp | ||
---|---|---|
15114 | 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. |
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.)
lib/Target/X86/X86ISelLowering.cpp | ||
---|---|---|
15114 | 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. |
lib/Transforms/Instrumentation/MemorySanitizer.cpp | ||
---|---|---|
2882 | This looks very fragile. |
lib/Transforms/Instrumentation/MemorySanitizer.cpp | ||
---|---|---|
2882 | OK, va_list may be from a function with a different calling convention. |
lib/Transforms/Instrumentation/MemorySanitizer.cpp | ||
---|---|---|
2882 | 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.) |
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.)
Looks good to me. Owen also mentioned that the SDAG changes were fine. They are mostly code motion.
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.
lib/Target/X86/X86ISelLowering.cpp | ||
---|---|---|
15112–15114 | I'm assuming this extra condition was needed for some test case? Should we change isCallingConvWin64 instead maybe? |
lib/Target/X86/X86ISelLowering.cpp | ||
---|---|---|
15112–15114 | 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. |
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.)
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.