Page MenuHomePhabricator

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

Authored by efriedma on May 6 2021, 12:38 PM.

Details

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.

Diff Detail

Event Timeline

efriedma created this revision.May 6 2021, 12:38 PM
efriedma requested review of this revision.May 6 2021, 12:38 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 6 2021, 12:38 PM

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?

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.

...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 updated this revision to Diff 344193.May 10 2021, 1:58 PM

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

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) {
}

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

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.

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.

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

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 updated this revision to Diff 345543.May 14 2021, 1:33 PM

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

rjmccall accepted this revision.May 14 2021, 2:59 PM

Thanks, LGTM

This revision is now accepted and ready to land.May 14 2021, 2:59 PM