Add an attribute to allow fields of non-trivial types in C unions

[ahatanak]

Summary

clang currently disallows fields of non-trivial types (e.g., __strong) in unions in C mode since it's not possible for the compiler to determine how the unions should be initialized, destructed, or copied.

This patch adds support for a new attribute non_trivial_union_member, which causes fields annotated with the attribute to be trivial when computing the trivialness of the containing union. This means the users are responsible for patching up the code so that the union is initialized, destructed, and copied in a functionally correct way.

rdar://problem/50591731

[rjmccall]

Does this lead to C/C++ ABI mismatches? Should we just honor this in C++ as well by ignoring it when deciding to delete special members? Is taking such a general name a good idea if it's language-specific? Richard, thoughts?

[ahatanak]

In D62988#1537082, @rjmccall wrote:

Does this lead to C/C++ ABI mismatches? Should we just honor this in C++ as well by ignoring it when deciding to delete special members? Is taking such a general name a good idea if it's language-specific? Richard, thoughts?

This is a C-only attribute, so clang will emit a diagnostic (warning 'attribute ignored') if the attribute is used to annotate a member of a C++ union. I think that would be sufficient to prevent possible C/C++ ABI mismatches?

[rjmccall]

In D62988#1537401, @ahatanak wrote:

In D62988#1537082, @rjmccall wrote:

Does this lead to C/C++ ABI mismatches? Should we just honor this in C++ as well by ignoring it when deciding to delete special members? Is taking such a general name a good idea if it's language-specific? Richard, thoughts?

This is a C-only attribute, so clang will emit a diagnostic (warning 'attribute ignored') if the attribute is used to annotate a member of a C++ union. I think that would be sufficient to prevent possible C/C++ ABI mismatches?

Is there a way to write a C++ union that would be ABI-compatible with a C union with this attribute?

[rsmith]

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?

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.

[ahatanak]

In 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.

[rsmith]

In D62988#1539230, @ahatanak wrote:

In 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.

[rjmccall]

For ARC, you could bzero the union member; this is what how we tell people to initialize malloc'ed memory as well, since there's no default-constructor concept that they can invoke for such cases.

Our immediate need for this attribute is that we have some code that wants to adopt non-trivial annotations in unions, with no intention of ever copying or destroying them as aggregates. Of course a better solution would be to make any C code that would copy or destroy the aggregate type illegal, but that seems like a big project. But maybe there'd be no need for this attribute in the long term if we eventually do have that support.

[rsmith]

In D62988#1540359, @rjmccall wrote:

For ARC, you could bzero the union member; this is what how we tell people to initialize malloc'ed memory as well, since there's no default-constructor concept that they can invoke for such cases.
Our immediate need for this attribute is that we have some code that wants to adopt non-trivial annotations in unions, with no intention of ever copying or destroying them as aggregates.

OK, so we'd be looking at something like this when using the attribute:

typedef struct S {
  int is_b;
  union {
    float f;
    __attribute__((non_trivial_union_member)) id b;
  };
} S;
S make() {
  S s = {0, 0.0f};
  return s;
}
void set_f(S *s, float f) {
  if (s->is_b) s->b = 0;
  s->is_b = 0;
  s->f = f;
}
void set_b(S *s, id b) {
  if (!s->is_b) bzero(s, sizeof(S));
  s->is_b = 1;
  s->b = b;
}
void destroy_s(S *s) {
  if (s->is_b) s->b = 0;
}

versus the same code written without the attribute, which might be:

typedef struct S {
  int is_b;
  union {
    float f;
    void *b;
  };
} S;
S make() {
  S s = {0, 0.0f};
  return s;
}
void set_f(S *s, float f) {
  if (s->is_b) (__bridge_transfer id)s->b;
  s->is_b = 0;
  s->f = f;
}
void set_b(S *s, id b) {
  if (s->is_b) (__bridge_transfer id)s->b;
  s->is_b = 1;
  s->b = (__bridge_retained void*)b;
}
void destroy_s(S *s) {
  if (s->is_b) (__bridge_transfer id)s->b;
}

To me, using the attribute here doesn't seem worthwhile; the first form of this code seems scary since important reference counting actions are hidden behind what appear to be dead stores, and the second form (while verbose) is at least explicit about what's going on. If you still think this is a useful feature, so be it, but we should at least be explicit in Clang's ARC documentation about how to correctly make use of it.

I'm not sure whether we should support this as a way of disabling the union triviality features in general, or only the restriction on lifetime types, but either way, I think the support of this attribute should not be dependent on the language mode; I think adding a C-only extension for this does a disservice to our users.

Of course a better solution would be to make any C code that would copy or destroy the aggregate type illegal, but that seems like a big project. But maybe there'd be no need for this attribute in the long term if we eventually do have that support.

Yeah, maybe so; adopting the C++11 "unrestricted unions" behavior for non-trivial types in unions in C does seem to make a certain amount of sense.

[ahatanak]

John and I had a discussion offline and decided that we should not pursue the approach taken in this patch. I'll try to work on a patch that follows the C++11 approach when I have time later.

include/clang/AST/Decl.h

public:
   bool isUnnamedBitfield() const { return isBitField() && !getDeclName(); }
 
   /// Determines whether this field is a
   /// representative for an anonymous struct or union. Such fields are
   /// unnamed and are implicitly generated by the implementation to
   /// store the data for the anonymous union or struct.
   bool isAnonymousStructOrUnion() const;
 
+  /// Indicates whether this field has a non-trivial C type.
+  bool hasNonTrivialPrimitiveCType(const ASTContext &Ctx) const;
+
   Expr *getBitWidth() const {
     if (!BitField)
       return nullptr;
     void *Ptr = InitStorage.getPointer();
     if (getInClassInitStyle())
       return static_cast<InitAndBitWidth*>(Ptr)->BitWidth;
     return static_cast<Expr*>(Ptr);
   }

include/clang/Basic/Attr.td

def NonParmVar : SubsetSubject<Var,
                                "variables">;
 def NonLocalVar : SubsetSubject<Var,
                                 [{!S->hasLocalStorage()}],
                                 "variables with non-local storage">;
 def NonBitField : SubsetSubject<Field,
                                 [{!S->isBitField()}],
                                 "non-bit-field non-static data members">;
 
+def FieldInUnion : SubsetSubject<Field,
+                                 [{S->getParent()->isUnion()}],
+                                 "field in union">;
+
 def NonStaticCXXMethod : SubsetSubject<CXXMethod,
                                        [{!S->isStatic()}],
                                        "non-static member functions">;
 
 def NonStaticNonConstCXXMethod
     : SubsetSubject<CXXMethod,
                     [{!S->isStatic() && !S->isConst()}],
                     "non-static non-const member functions">;

 
 def TransparentUnion : InheritableAttr {
   let Spellings = [GCC<"transparent_union">];
 //  let Subjects = SubjectList<[Record, TypedefName]>;
   let Documentation = [TransparentUnionDocs];
   let LangOpts = [COnly];
 }
 
+def NonTrivialUnionMember : InheritableAttr {
+  let Spellings = [Clang<"non_trivial_union_member">];
+  let Subjects = SubjectList<[FieldInUnion]>;
+  let Documentation = [NonTrivialUnionMemberDocs];
+  let LangOpts = [COnly];
+}
+
 def Unavailable : InheritableAttr {
   let Spellings = [Clang<"unavailable">];
   let Args = [StringArgument<"Message", 1>,
               EnumArgument<"ImplicitReason", "ImplicitReason",
                 ["", "", "", ""],
                 ["IR_None",
                  "IR_ARCForbiddenType",
                  "IR_ForbiddenWeak",

include/clang/Basic/AttrDocs.td

 - The argument is passed to the function using the calling convention of the
   first member of the transparent union. Consequently, all the members of the
   transparent union should have the same calling convention as its first member.
 
 Transparent unions are not supported in C++.
   }];
 }
 
+def NonTrivialUnionMemberDocs : Documentation {
+  let Category = DocCatDecl;
+  let Content = [{
+This attribute causes fields of non-trivial types (e.g., `__strong`) in C unions
+to be treated as trivial fields when computing the trivialness of the
+containing union. Users are responsible for patching up the code so that the
+union is initialized, destructed, and copied in a functionally correct way.
+  }];
+}
+
 def ObjCSubclassingRestrictedDocs : Documentation {
   let Category = DocCatDecl;
   let Content = [{
 This attribute can be added to an Objective-C ``@interface`` declaration to
 ensure that this class cannot be subclassed.
   }];
 }
 

lib/AST/Decl.cpp

if (!isImplicit() || getDeclName())
     return false;
 
   if (const auto *Record = getType()->getAs<RecordType>())
     return Record->getDecl()->isAnonymousStructOrUnion();
 
   return false;
 }
 
+bool FieldDecl::hasNonTrivialPrimitiveCType(const ASTContext &Ctx) const {
+  return !hasAttr<NonTrivialUnionMemberAttr>() &&
+         getType().isNonTrivialPrimitiveCType(Ctx);
+}
+
 unsigned FieldDecl::getBitWidthValue(const ASTContext &Ctx) const {
   assert(isBitField() && "not a bitfield");
   return getBitWidth()->EvaluateKnownConstInt(Ctx).getZExtValue();
 }
 
 bool FieldDecl::isZeroLengthBitField(const ASTContext &Ctx) const {
   return isUnnamedBitfield() && !getBitWidth()->isValueDependent() &&
          getBitWidthValue(Ctx) == 0;

lib/AST/Type.cpp

bool visitStruct(QualType QT) {
     // We don't want to apply the C restriction in C++ because C++
     // (1) can apply the restriction at a finer grain by banning copying or
     //     destroying the union, and
     // (2) allows users to override these restrictions by declaring explicit
     //     constructors/etc, which we're not proposing to add to C.
     if (isa<CXXRecordDecl>(RD))
       return false;
     for (const FieldDecl *FD : RD->fields())
-      if (this->asDerived().visit(FD->getType()))
+      if (!FD->hasAttr<NonTrivialUnionMemberAttr>() &&
+          this->asDerived().visit(FD->getType()))
         return true;
     return false;
   }
 
   const ASTContext &Ctx;
 };
 
 } // namespace

lib/Sema/SemaDecl.cpp

if (FDTy->isFunctionType()) {
         // Ivars can not have abstract class types
         FD->setInvalidDecl();
       }
       if (Record && FDTTy->getDecl()->hasObjectMember())
         Record->setHasObjectMember(true);
       if (Record && FDTTy->getDecl()->hasVolatileMember())
         Record->setHasVolatileMember(true);
       if (Record && Record->isUnion() &&
-          FD->getType().isNonTrivialPrimitiveCType(Context))
+          FD->hasNonTrivialPrimitiveCType(Context))
         Diag(FD->getLocation(),
              diag::err_nontrivial_primitive_type_in_union);
     } else if (FDTy->isObjCObjectType()) {
       /// A field cannot be an Objective-c object
       Diag(FD->getLocation(), diag::err_statically_allocated_object)
         << FixItHint::CreateInsertion(FD->getLocation(), "*");
       QualType T = Context.getObjCObjectPointerType(FD->getType());
       FD->setType(T);
     } else if (getLangOpts().allowsNonTrivialObjCLifetimeQualifiers() &&
                Record && !ObjCFieldLifetimeErrReported && Record->isUnion() &&
-               !getLangOpts().CPlusPlus) {
+               !getLangOpts().CPlusPlus &&
+               !FD->hasAttr<NonTrivialUnionMemberAttr>()) {
       // It's an error in ARC or Weak if a field has lifetime.
       // We don't want to report this in a system header, though,
       // so we just make the field unavailable.
       // FIXME: that's really not sufficient; we need to make the type
       // itself invalid to, say, initialize or copy.
       QualType T = FD->getType();
       if (T.hasNonTrivialObjCLifetime()) {
         SourceLocation loc = FD->getLocation();
if (FDTy->isFunctionType()) {
         else if (BaseType->isObjCObjectPointerType() ||
                  BaseType.isObjCGCStrong())
                Record->setHasObjectMember(true);
       }
     }
 
     if (Record && !getLangOpts().CPlusPlus && !FD->hasAttr<UnavailableAttr>()) {
       QualType FT = FD->getType();
+      if (!FD->hasAttr<NonTrivialUnionMemberAttr>()) {
         if (FT.isNonTrivialToPrimitiveDefaultInitialize())
           Record->setNonTrivialToPrimitiveDefaultInitialize(true);
         QualType::PrimitiveCopyKind PCK = FT.isNonTrivialToPrimitiveCopy();
-      if (PCK != QualType::PCK_Trivial && PCK != QualType::PCK_VolatileTrivial)
+        if (PCK != QualType::PCK_Trivial &&
+            PCK != QualType::PCK_VolatileTrivial)
           Record->setNonTrivialToPrimitiveCopy(true);
         if (FT.isDestructedType()) {
           Record->setNonTrivialToPrimitiveDestroy(true);
           Record->setParamDestroyedInCallee(true);
         }
+      }
 
       if (const auto *RT = FT->getAs<RecordType>()) {
         if (RT->getDecl()->getArgPassingRestrictions() ==
             RecordDecl::APK_CanNeverPassInRegs)
           Record->setArgPassingRestrictions(RecordDecl::APK_CanNeverPassInRegs);
       } else if (FT.getQualifiers().getObjCLifetime() == Qualifiers::OCL_Weak)
         Record->setArgPassingRestrictions(RecordDecl::APK_CanNeverPassInRegs);
     }

lib/Sema/SemaDeclAttr.cpp

case ParsedAttr::AT_WeakRef:
     handleWeakRefAttr(S, D, AL);
     break;
   case ParsedAttr::AT_WeakImport:
     handleWeakImportAttr(S, D, AL);
     break;
   case ParsedAttr::AT_TransparentUnion:
     handleTransparentUnionAttr(S, D, AL);
     break;
+  case ParsedAttr::AT_NonTrivialUnionMember:
+    handleSimpleAttribute<NonTrivialUnionMemberAttr>(S, D, AL);
+    break;
   case ParsedAttr::AT_ObjCException:
     handleSimpleAttribute<ObjCExceptionAttr>(S, D, AL);
     break;
   case ParsedAttr::AT_ObjCMethodFamily:
     handleObjCMethodFamilyAttr(S, D, AL);
     break;
   case ParsedAttr::AT_ObjCNSObject:
     handleObjCNSObject(S, D, AL);

test/CodeGenObjC/strong-in-c-struct.m

typedef struct {
   volatile char i6;
 } Bitfield1;
 
 typedef struct {
   id x;
   volatile int a[16];
 } VolatileArray ;
 
+typedef union {
+  id __attribute__((non_trivial_union_member)) f0;
+} NonTrivialAttrUnion;
+
 #endif
 
 #ifdef USESTRUCT
 
 StrongSmall getStrongSmall(void);
 StrongOuter getStrongOuter(void);
 StrongOuter2 getStrongOuter2(void);
 void calleeStrongSmall(StrongSmall);

 // CHECK: %[[V13:.*]] = bitcast i8** %[[ADDR_CUR1]] to i32*
 // CHECK: %[[V14:.*]] = load volatile i32, i32* %[[V13]], align 4
 // CHECK: store volatile i32 %[[V14]], i32* %[[V12]], align 4
 
 void test_copy_constructor_VolatileArray(VolatileArray *a) {
   VolatileArray t = *a;
 }
 
+// CHECK-LABEL: define void @test_non_trivial_union_member(i64 %
+// CHECK: %[[A:.*]] = alloca %[[UNION_NONTRIVIALATTRUNION:.*]], align 8
+// CHECK: %[[T:.*]] = alloca %[[UNION_NONTRIVIALATTRUNION]], align 8
+// CHECK: %[[COERCE_DIVE:.*]] = getelementptr inbounds %[[UNION_NONTRIVIALATTRUNION]], %[[UNION_NONTRIVIALATTRUNION]]* %[[A]], i32 0, i32 0
+// CHECK: %[[COERCE_VAL_IP:.*]] = inttoptr i64 %[[A_COERCE]] to i8*
+// CHECK: store i8* %[[COERCE_VAL_IP]], i8** %[[COERCE_DIVE]], align 8
+// CHECK: %[[V0:.*]] = bitcast %[[UNION_NONTRIVIALATTRUNION]]* %[[T]] to i8*
+// CHECK: %[[V1:.*]] = bitcast %[[UNION_NONTRIVIALATTRUNION]]* %[[A]] to i8*
+// CHECK: call void @llvm.memcpy.p0i8.p0i8.i64(i8* align 8 %[[V0]], i8* align 8 %[[V1]], i64 8, i1 false)
+// CHECK: ret void
+
+void test_non_trivial_union_member(NonTrivialAttrUnion a) {
+  NonTrivialAttrUnion t = a;
+}
+
 #endif /* USESTRUCT */

test/SemaObjC/attr-non-trivial-union-member.m

+// RUN: %clang_cc1 -fsyntax-only -fblocks -fobjc-arc -verify -Wno-objc-root-class %s
+
+int x __attribute__((non_trivial_union_member)); // expected-warning {{'non_trivial_union_member' attribute only applies to field in union}}
+
+struct S0 {
+  int __attribute__((non_trivial_union_member)) f0; // expected-warning {{'non_trivial_union_member' attribute only applies to field in union}}
+};
+
+struct S1 {
+  id f0;
+};
+
+union U0 {
+  id f0; // expected-error {{ARC forbids Objective-C objects in union}}
+};
+
+union U1 {
+  struct S1 f0; // expected-error {{non-trivial C types are disallowed in union}}
+};
+
+union U2 {
+  id __attribute__((non_trivial_union_member)) f0;
+};
+
+union U3 {
+  union U2 f0;
+  struct S1 __attribute__((non_trivial_union_member)) f1;
+};