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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
