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

Mon May 10 22:50:02 PDT 2021

rjmccall added a comment.

In D102015#2748441 <https://reviews.llvm.org/D102015#2748441>, @efriedma wrote:

> In D102015#2743634 <https://reviews.llvm.org/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 <https://reviews.llvm.org/D102015#2748595>, @efriedma wrote:

> In D102015#2748441 <https://reviews.llvm.org/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 <https://reviews.llvm.org/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?

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D102015/new/

https://reviews.llvm.org/D102015