[clang CodeGen] Don't crash on large atomic function parameter.

[efriedma]

Summary

I wouldn't recommend writing code like the testcase; a function parameter isn't atomic, so using an atomic type doesn't really make sense. But it's valid, so clang shouldn't crash on it.

The code was assuming hasAggregateEvaluationKind(Ty) implies Ty is a RecordType, which isn't true. Looking at the code, I think the only other possibility for a parameter is an atomic type, though. Atomic types are always trivially destructible, I think.

[rjmccall]

Objective-C object types also have aggregate evaluation kind. Those can only be used as value types in an old, fragile ObjC ABI, but I believe we haven't yet formally decided to remove support for that. Fortunately, however, there's a much better simplification available: this hasAggregateEvaluationKind(Ty) call is literally doing nothing that couldn't just be a getAs<RecordType>() call, and then we don't need to worry about it.

...I'm confused about why this code is doing what it's doing with cleanups, though. Why does it only apply when the parameter is indirect? I believe isParamDestroyedInCallee() can apply to types that are passed in other ways, so where we pushing the destructor for those, and why isn't it good enough to handle this case as well?

[efriedma]

In D102015#2743634, @rjmccall wrote:

Objective-C object types also have aggregate evaluation kind. Those can only be used as value types in an old, fragile ObjC ABI, but I believe we haven't yet formally decided to remove support for that. Fortunately, however, there's a much better simplification available: this hasAggregateEvaluationKind(Ty) call is literally doing nothing that couldn't just be a getAs<RecordType>() call, and then we don't need to worry about it.

If you're confident that only RecordTypes need to be destroyed, sure.

Did some grepping, and there's some other places using isParamDestroyedInCallee(); I'll make sure they're also okay.

...I'm confused about why this code is doing what it's doing with cleanups, though. Why does it only apply when the parameter is indirect? I believe isParamDestroyedInCallee() can apply to types that are passed in other ways, so where we pushing the destructor for those, and why isn't it good enough to handle this case as well?

Objects with a non-trivial destructor end up indirect anyway under the normal ABI rules. Not sure how it interacts with trivial_abi; I'll look into it.

[efriedma]

In D102015#2748441, @efriedma wrote:

...I'm confused about why this code is doing what it's doing with cleanups, though. Why does it only apply when the parameter is indirect? I believe isParamDestroyedInCallee() can apply to types that are passed in other ways, so where we pushing the destructor for those, and why isn't it good enough to handle this case as well?

Objects with a non-trivial destructor end up indirect anyway under the normal ABI rules. Not sure how it interacts with trivial_abi; I'll look into it.

Figured it out. "isIndirect()" here doesn't mean the same thing that it means in ABIArgInfo. If hasScalarEvaluationKind(Ty) is false, the caller ensures the value is "isIndirect()", i.e. on the stack. It doesn't matter if the value was actually passed in registers.

It's not immediately obvious to me why the responsibility for creating stack temporaries was split this way, but it's not really relevant to this patch.

[efriedma]

Use isRecordType() instead of checking for an atomic type. Fix the caller side as well.

[ahatanak]

C structs with ObjC pointer fields under ARC are non-trivial to destruct too.

struct foo {
  int big[128];
  id a;
};

void test_atomic_array_param(_Atomic(struct foo) a) {
}

[ahatanak]

Using _Atomic for C structs with non-trivially destructible fields currently doesn't work, so maybe that should just be disallowed.

[efriedma]

Using _Atomic for C structs with non-trivially destructible fields currently doesn't work, so maybe that should just be disallowed.

I'd prefer to disallow _Atomic on all types that aren't trivially destructible, yes. Not impossible to support, of course, but I don't really see the value.

[rjmccall]

In D102015#2748441, @efriedma wrote:

In D102015#2743634, @rjmccall wrote:

Objective-C object types also have aggregate evaluation kind. Those can only be used as value types in an old, fragile ObjC ABI, but I believe we haven't yet formally decided to remove support for that. Fortunately, however, there's a much better simplification available: this hasAggregateEvaluationKind(Ty) call is literally doing nothing that couldn't just be a getAs<RecordType>() call, and then we don't need to worry about it.

If you're confident that only RecordTypes need to be destroyed, sure.

Well, they're the only thing for which isParamDestroyedInCallee() is defined. If we generalize that in the future, I think the code in your patch will be obvious enough how to modify.

In D102015#2748595, @efriedma wrote:

In D102015#2748441, @efriedma wrote:

...I'm confused about why this code is doing what it's doing with cleanups, though. Why does it only apply when the parameter is indirect? I believe isParamDestroyedInCallee() can apply to types that are passed in other ways, so where we pushing the destructor for those, and why isn't it good enough to handle this case as well?

Objects with a non-trivial destructor end up indirect anyway under the normal ABI rules. Not sure how it interacts with trivial_abi; I'll look into it.

Figured it out. "isIndirect()" here doesn't mean the same thing that it means in ABIArgInfo. If hasScalarEvaluationKind(Ty) is false, the caller ensures the value is "isIndirect()", i.e. on the stack. It doesn't matter if the value was actually passed in registers.

Ugh. Well, that's not great, but yeah, I agree we don't need to mess with that right now. Thanks for checking it out.

In D102015#2749212, @efriedma wrote:

Using _Atomic for C structs with non-trivially destructible fields currently doesn't work, so maybe that should just be disallowed.

I'd prefer to disallow _Atomic on all types that aren't trivially destructible, yes. Not impossible to support, of course, but I don't really see the value.

I agree that disallowing this is the right way to go. There are non-trivial C structs that can support atomic operations easily enough, but the ones with ARC strong references specifically aren't among them.

I guess your test case is testing both the parameter and argument code. The delegate-arg code should be testable with an override thunk with a big atomic parameter, right?

[efriedma]

Figured out a way to test the EmitDelegateCallArg codepath. (A thunk doesn't work because the code is guarded by !CurFuncIsThunk.)

[rjmccall]

Thanks, LGTM

clang/lib/CodeGen/CGCall.cpp

void CodeGenFunction::EmitDelegateCallArg(CallArgList &args,
 
   // For the most part, we just need to load the alloca, except that
   // aggregate r-values are actually pointers to temporaries.
   } else {
     args.add(convertTempToRValue(local, type, loc), type);
   }
 
   // Deactivate the cleanup for the callee-destructed param that was pushed.
-  if (hasAggregateEvaluationKind(type) && !CurFuncIsThunk &&
+  if (type->isRecordType() && !CurFuncIsThunk &&
       type->castAs<RecordType>()->getDecl()->isParamDestroyedInCallee() &&
       param->needsDestruction(getContext())) {
     EHScopeStack::stable_iterator cleanup =
         CalleeDestructedParamCleanups.lookup(cast<ParmVarDecl>(param));
     assert(cleanup.isValid() &&
            "cleanup for callee-destructed param not recorded");
     // This unreachable is a temporary marker which will be removed later.
     llvm::Instruction *isActive = Builder.CreateUnreachable();
if (E->isGLValue()) {
     return args.add(EmitReferenceBindingToExpr(E), type);
   }
 
   bool HasAggregateEvalKind = hasAggregateEvaluationKind(type);
 
   // In the Microsoft C++ ABI, aggregate arguments are destructed by the callee.
   // However, we still have to push an EH-only cleanup in case we unwind before
   // we make it to the call.
-  if (HasAggregateEvalKind &&
+  if (type->isRecordType() &&
       type->castAs<RecordType>()->getDecl()->isParamDestroyedInCallee()) {
     // If we're using inalloca, use the argument memory.  Otherwise, use a
     // temporary.
     AggValueSlot Slot;
     if (args.isUsingInAlloca())
       Slot = createPlaceholderSlot(*this, type);
     else
       Slot = CreateAggTemp(type, "agg.tmp");

clang/lib/CodeGen/CGDecl.cpp

if (SrcLangAS != DestLangAS) {
       DeclPtr = Address(getTargetHooks().performAddrSpaceCast(
                             *this, V, SrcLangAS, DestLangAS, T, true),
                         DeclPtr.getAlignment());
     }
 
     // Push a destructor cleanup for this parameter if the ABI requires it.
     // Don't push a cleanup in a thunk for a method that will also emit a
     // cleanup.
-    if (hasAggregateEvaluationKind(Ty) && !CurFuncIsThunk &&
+    if (Ty->isRecordType() && !CurFuncIsThunk &&
         Ty->castAs<RecordType>()->getDecl()->isParamDestroyedInCallee()) {
       if (QualType::DestructionKind DtorKind =
               D.needsDestruction(getContext())) {
         assert((DtorKind == QualType::DK_cxx_destructor ||
                 DtorKind == QualType::DK_nontrivial_c_struct) &&
                "unexpected destructor type");
         pushDestroy(DtorKind, DeclPtr, Ty);
         CalleeDestructedParamCleanups[cast<ParmVarDecl>(&D)] =

clang/test/CodeGen/big-atomic-ops.c

void atomic_init_foo()
   // CHECK-NOT: atomic
   // CHECK: store
   __c11_atomic_init(&j, 42);
 
   // CHECK-NOT: atomic
   // CHECK: }
 }
 
+// Check this doesn't crash
+// CHECK: @test_atomic_array_param(
+void test_atomic_array_param(_Atomic(struct foo) a) {
+  test_atomic_array_param(a);
+}
+
 #endif

clang/test/CodeGenCXX/atomic.cpp

namespace PR11411 {
   template<typename _Tp> inline void Ptr<_Tp>::f() {
     int* _refcount;
     // CHECK: atomicrmw add i32* {{.*}} seq_cst, align 4
     __sync_fetch_and_add(_refcount, 1);
     // CHECK-NEXT: ret void
   }
   void f(Ptr<int> *a) { a->f(); }
 }
+
+namespace DelegatingParameter {
+  // Check that we're delegating the complete ctor to the base
+  // ctor, and that doesn't crash.
+  // CHECK-LABEL: define void @_ZN19DelegatingParameter1SC1EU7_AtomicNS_1ZE
+  // CHECK: call void @_ZN19DelegatingParameter1SC2EU7_AtomicNS_1ZE
+  struct Z { int z[100]; };
+  struct S { S(_Atomic Z); };
+  S::S(_Atomic Z) {}
+}