[PATCH] D62988: Add an attribute to allow fields of non-trivial types in C unions

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added a comment.

In D62988#1539230 <https://reviews.llvm.org/D62988#1539230>, @ahatanak wrote:

> In D62988#1538577 <https://reviews.llvm.org/D62988#1538577>, @rsmith wrote:
>
> > How do you write correct (non-leaking, non-double-freeing, non-releasing-invalid-pointers) code with this attribute? For example, suppose I have a `__strong` union member: does storing to it release the old value (which might be a different union member)? If so, how do you work around that? https://clang.llvm.org/docs/AutomaticReferenceCounting.html#ownership-qualified-fields-of-structs-and-unions should be updated to say what happens. If manual reference counting code is required to make any use of this feature correct (which seems superficially likely), is this a better programming model than `__unsafe_unretained`?
>
>
> I should first make it clear that this attribute is a dangerous feature that makes it easier for users to write incorrect code if they aren't careful.
>
> To write correct code, users have to manually (I don't mean manual-retain-release, this is compiled with ARC) do assignments to the fields that are annotated with the attribute to patch up the code generated by the compiler since the compiler isn't generating the kind of special functions it generates for non-trivial structs. For example:
>
>   union U1 {
>     id __weak __attribute__((non_trivial_union_member)) a;
>     id __attribute__((non_trivial_union_member)) b;
>   };
>    
>   id getObj(int);
>  
>   void foo() {
>     union U1 u1 = { .b = 0}; // zero-initialize 'b'.
>     u1.b = getObj(1); // assign to __strong field 'b'.
>     u1.b = getObj(2); // retain and assign another object to 'b' and release the previously referenced object.


This is what I expected and what I was worried about. I could be missing something, but this approach appears to me to cause this feature to not be useful in many cases. If a store to a union member now first reads from (and releases) that union member, then you cannot change the active union member to a member with lifetime type, because that would trigger a load of an inactive union member with undefined behavior. For example, consider:

  union U {
    float f;
    __attribute__((non_trivial_union_member)) id b;
  };
  void f() {
    union U u = {.f = 1.0f};
    u.b = getObj(1); // undefined behavior: tries to release existing value of u.b, which is not active union member; the implied `[u.b release]` is going to do Very Bad Things.

In C, storing to a union member is the way you change the active member, and I can't off-hand think of any sensible workaround for this problem. But maybe I overlooked it: can you demonstrate how you would correctly write the last line of the above example? If that can't be done reasonably, I do not think we should add this attribute.


Repository:
  rC Clang

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

https://reviews.llvm.org/D62988