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

[Akira Hatanaka via Phabricator via cfe-commits]

ahatanak added a comment.

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.
    u1.b = 0; // release the object.
    id t = getObj(3);
    u1.a = t; // assign to __weak field 'a'.
    u1.a = 0; // unregister as a __weak object.
  }
  
  struct S1 {
    union U1 f0;
    int f1;
  };
  
  void foo1() {
    struct S1 s1 = { .f0 = 0};
    s1.f0.a = getObj(4);
    struct S1 s2 = s1; // this is just a memcpy
    s2.f0.a = s1.f0.a;  // manually copy __weak field 'a'.
    s2.f0.a = 0;  // unregister as a __weak object.
  }

Storing to a `__strong` union member does release the old value. The old value can be a different union member and I think that should be fine as long as the other member is also `__strong`. If the other member has a different lifetime qualifier, I believe it has to be nulled out before assigning the new object as shown in the example above.

> Unions with non-trivial members are only permitted in C++11 onwards; should we allow the attribute in C++98 mode? But more than that, unions with non-trivial members require user-provided special members in C++11 onwards, meaning that a union using this attribute in C would have a different calling convention than the "natural" equivalent in C++. So I'm also wondering if we should allow this in all language modes.

I think we can allow this attribute in C++ mode including C++98. One thing to note is that the compiler creates temporaries users can't directly access when passing/returning unions with non-trivial members to/from functions or capturing them as a block capture. In those cases, users won't be able to patch up the code generated by the compiler, so it would be incorrect to pass a union whose active member is the one annotated with `non_trivial_union_member`.


Repository:
  rC Clang

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

https://reviews.llvm.org/D62988