[PATCH] D70935: [CodeGen][ObjC] Emit a primitive store to store a __strong field in ExpandTypeFromArgs

[Akira Hatanaka via Phabricator via cfe-commits]

ahatanak added inline comments.


================
Comment at: clang/lib/CodeGen/CGCall.cpp:1054
+             "pointer to __strong expected");
+      EmitStoreOfScalar(*AI++, LV);
+    } else
----------------
rjmccall wrote:
> This definitely deserves a comment.
> 
> I don't think the assertion is right; the condition is that the type is legal for a field in a struct that can be passed directly, and while that does exclude `__weak` (because the struct will have to be passed indirectly) and `__autoreleasing` (because that's not legal in a struct), it doesn't exclude `__unsafe_unretained`.
> 
> This function is implementing an operation that's broadly meaningful (it's a store-init of an owned value, in contrast to a store-init with an unowned value which is what `isInit` is implementing) but not extensively used in the C frontend.  On some level, I feel like we should probably teach `EmitStoreThroughLValue` to handle that properly, but that's a more significant refactor.
> 
> It does look like this change isn't enough to handle `__ptrauth`, which will assume that the source value is signed appropriately for the unqualified type when probably it should be signed appropriately for the qualifier (which, like `__weak`, cannot be address-discriminated because it would stop being passed directly).  Probably the default case should be to use `EmitStoreOfScalar`, and `EmitStoreThroughLValue` should only be used if the l-value is a bit-field (the only non-simple case that can actually happen from drilling down to a field).
> 
> The same logic applies on the load side in the abstract, except that it is only causing problems for `__ptrauth` (well, if we change the behavior above) because loads from the ARC qualifiers don't implicitly retain.  Regardless, analogously to this, `EmitRValueForField` should only be calling `EmitLoadOfLValue` for bit-fields and should otherwise call `EmitLoadOfScalar`.  Please add a comment on both sides making it clear that the logic is expected to be parallel.
Ah right, the assertion isn't correct. My intention was to catch `__weak` pointers.

Let me know if the comment I added is OK.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70935