PNaCl: Fix ExpandVarArgs to handle "byval" struct arguments properly

 * Ensure that the "byval" attribute is preserved for the fixed
   arguments.  Before, it was stripped off from function calls but
   left in for function definitions, which broke passing struct
   arguments (which PNaCl Clang generates as "byval").

 * Ensure that function and return attributes are preserved too.

 * Handle "byval" variable arguments in calls too by dereferencing the
   pointer.  These are not yet usable with PNaCl Clang, which does not
   allow va_arg on structs (see
   https://code.google.com/p/nativeclient/issues/detail?id=2381), but
   we might as well make this pass ready to handle this.

BUG=https://code.google.com/p/nativeclient/issues/detail?id=3338
TEST=test/Transforms/NaCl/expand-varargs*.ll

Review URL: https://codereview.chromium.org/13100002

3 files changed, 114 insertions, 8 deletions

diff --git a/lib/Transforms/NaCl/ExpandVarArgs.cpp b/lib/Transforms/NaCl/ExpandVarArgs.cpp
index e1bd8f93a8..6998001754 100644
--- a/lib/Transforms/NaCl/ExpandVarArgs.cpp
+++ b/lib/Transforms/NaCl/ExpandVarArgs.cpp
@@ -168,13 +168,13 @@ static void LifetimeDecl(Intrinsic::ID id, Value *Ptr, Value *Size,
 
 // CopyCall() uses argument overloading so that it can be used by the
 // template ExpandVarArgCall().
-static Instruction *CopyCall(CallInst *Original, Value *Callee,
-                             ArrayRef<Value*> Args) {
+static CallInst *CopyCall(CallInst *Original, Value *Callee,
+                          ArrayRef<Value*> Args) {
   return CallInst::Create(Callee, Args, "", Original);
 }
 
-static Instruction *CopyCall(InvokeInst *Original, Value *Callee,
-                             ArrayRef<Value*> Args) {
+static InvokeInst *CopyCall(InvokeInst *Original, Value *Callee,
+                            ArrayRef<Value*> Args) {
   return InvokeInst::Create(Callee, Original->getNormalDest(),
                             Original->getUnwindDest(), Args, "", Original);
 }
@@ -190,16 +190,30 @@ static bool ExpandVarArgCall(InstType *Call, DataLayout *DL) {
 
   LLVMContext *Context = &Call->getContext();
 
+  SmallVector<AttributeSet, 8> Attrs;
+  Attrs.push_back(Call->getAttributes().getFnAttributes());
+  Attrs.push_back(Call->getAttributes().getRetAttributes());
+
   // Split argument list into fixed and variable arguments.
   SmallVector<Value *, 8> FixedArgs;
   SmallVector<Value *, 8> VarArgs;
   SmallVector<Type *, 8> VarArgsTypes;
-  for (unsigned I = 0; I < FuncType->getNumParams(); ++I)
+  for (unsigned I = 0; I < FuncType->getNumParams(); ++I) {
     FixedArgs.push_back(Call->getArgOperand(I));
+    // AttributeSets use 1-based indexing.
+    Attrs.push_back(Call->getAttributes().getParamAttributes(I + 1));
+  }
   for (unsigned I = FuncType->getNumParams();
        I < Call->getNumArgOperands(); ++I) {
-    VarArgs.push_back(Call->getArgOperand(I));
-    VarArgsTypes.push_back(Call->getArgOperand(I)->getType());
+    Value *ArgVal = Call->getArgOperand(I);
+    if (Call->getAttributes().hasAttribute(I + 1, Attribute::ByVal)) {
+      // For "byval" arguments we must dereference the pointer and
+      // make a copy of the struct being passed by value.
+      ArgVal = CopyDebug(new LoadInst(ArgVal, "vararg_struct_copy", Call),
+                         Call);
+    }
+    VarArgs.push_back(ArgVal);
+    VarArgsTypes.push_back(ArgVal->getType());
   }
 
   StructType *VarArgsTy;
@@ -262,7 +276,9 @@ static bool ExpandVarArgCall(InstType *Call, DataLayout *DL) {
 
   // Create the converted function call.
   FixedArgs.push_back(ArgToAdd);
-  Value *NewCall = CopyDebug(CopyCall(Call, CastFunc, FixedArgs), Call);
+  InstType *NewCall = CopyCall(Call, CastFunc, FixedArgs);
+  CopyDebug(NewCall, Call);
+  NewCall->setAttributes(AttributeSet::get(Call->getContext(), Attrs));
   NewCall->takeName(Call);
 
   if (BufPtr) {
diff --git a/test/Transforms/NaCl/expand-varargs-attrs.ll b/test/Transforms/NaCl/expand-varargs-attrs.ll
new file mode 100644
index 0000000000..f53a8106f0
--- /dev/null
+++ b/test/Transforms/NaCl/expand-varargs-attrs.ll
@@ -0,0 +1,72 @@
+; RUN: opt < %s -expand-varargs -S | FileCheck %s
+
+declare i32 @varargs_func(i32 %arg, ...)
+
+
+; Check that attributes such as "byval" are preserved on fixed arguments.
+
+%MyStruct = type { i64, i64 }
+
+define void @func_with_arg_attrs(%MyStruct* byval, ...) {
+  ret void
+}
+; CHECK: define void @func_with_arg_attrs(%MyStruct* byval, i8* %varargs) {
+
+
+declare void @take_struct_arg(%MyStruct* byval %s, ...)
+
+define void @call_with_arg_attrs(%MyStruct* %s) {
+  call void (%MyStruct*, ...)* @take_struct_arg(%MyStruct* byval %s)
+  ret void
+}
+; CHECK: define void @call_with_arg_attrs(%MyStruct* %s) {
+; CHECK: call void %vararg_func(%MyStruct* byval %s, {}* undef)
+
+
+; The "byval" attribute here should be dropped.
+define i32 @pass_struct_via_vararg1(%MyStruct* %s) {
+  %result = call i32 (i32, ...)* @varargs_func(i32 111, %MyStruct* byval %s)
+  ret i32 %result
+}
+; CHECK: define i32 @pass_struct_via_vararg1(%MyStruct* %s) {
+; CHECK: %result = call i32 %vararg_func(i32 111, %{{.*}}* %vararg_buffer)
+
+
+; The "byval" attribute here should be dropped.
+define i32 @pass_struct_via_vararg2(%MyStruct* %s) {
+  %result = call i32 (i32, ...)* @varargs_func(i32 111, i32 2, %MyStruct* byval %s)
+  ret i32 %result
+}
+; CHECK: define i32 @pass_struct_via_vararg2(%MyStruct* %s) {
+; CHECK: %result = call i32 %vararg_func(i32 111, %{{.*}}* %vararg_buffer)
+
+
+; Check that return attributes such as "signext" are preserved.
+define i32 @call_with_return_attr() {
+  %result = call signext i32 (i32, ...)* @varargs_func(i32 111, i64 222)
+  ret i32 %result
+}
+; CHECK: define i32 @call_with_return_attr() {
+; CHECK: %result = call signext i32 %vararg_func(i32 111
+
+
+; Check that the "readonly" function attribute is preserved.
+define i32 @call_readonly() {
+  %result = call i32 (i32, ...)* @varargs_func(i32 111, i64 222) readonly
+  ret i32 %result
+}
+; CHECK: define i32 @call_readonly() {
+; CHECK: %result = call i32 %vararg_func(i32 111, {{.*}}) #1
+
+
+; Check that the "tail" attribute gets removed, because the callee
+; reads space alloca'd by the caller.
+define i32 @tail_call() {
+  %result = tail call i32 (i32, ...)* @varargs_func(i32 111, i64 222)
+  ret i32 %result
+}
+; CHECK: define i32 @tail_call() {
+; CHECK: %result = call i32 %vararg_func(i32 111
+
+
+; CHECK: attributes #1 = { readonly }
diff --git a/test/Transforms/NaCl/expand-varargs-struct.ll b/test/Transforms/NaCl/expand-varargs-struct.ll
new file mode 100644
index 0000000000..f147d7006f
--- /dev/null
+++ b/test/Transforms/NaCl/expand-varargs-struct.ll
@@ -0,0 +1,18 @@
+; RUN: opt < %s -expand-varargs -S | FileCheck %s
+
+declare i32 @varargs_func(i32 %arg, ...)
+
+
+%MyStruct = type { i64, i64 }
+
+; CHECK: %vararg_call = type <{ i64, %MyStruct }>
+
+; Test passing a struct by value.
+define i32 @varargs_call_struct(%MyStruct* %ptr) {
+  %result = call i32 (i32, ...)* @varargs_func(i32 111, i64 222, %MyStruct* byval %ptr)
+  ret i32 %result
+}
+; CHECK: define i32 @varargs_call_struct(%MyStruct* %ptr) {
+; CHECK: %vararg_struct_copy = load %MyStruct* %ptr
+; CHECK: %vararg_ptr1 = getelementptr %vararg_call* %vararg_buffer, i32 0, i32 1
+; CHECK: store %MyStruct %vararg_struct_copy, %MyStruct* %vararg_ptr1