Transparent_union attribute should be possible in front of union.

[erichkeane]

Summary

Clang compiles with error following test case

typedef union attribute((transparent_union)) {

int *i;
struct st *s;

} TU;

void bar(TU);

void foo(int *i) {

bar(i);

}

clang -c tu.c
tu.c:1:30: warning: transparent_union attribute can only be applied to a union definition; attribute ignored [-Wignored-attributes]
typedef union attribute((transparent_union)) {

^

tu.c:8:24: error: passing 'int *' to parameter of incompatible type 'TU'
void foo(int *i) { bar(i); }

^

tu.c:6:12: note: passing argument to parameter here
void bar(TU);

^

GCC compiles this test successfully.

The compilation is failed because the routine handleTransparentUnionAttr requires for the record decl to be completed. This fix provides handling of the attribute ‘transparent_union’ after parsing of union.

[aaron.ballman]

llvm/tools/clang/lib/Sema/SemaDeclAttr.cpp

3011

All of the handle*Attr() functions should have a uniform signature (there are plans for automating more attribute handling code), so you should find a way to implement this that does not require modifying the handler signature (or threading that information through ProcessDeclAttribute()).

[vbyakovlcl]

llvm/tools/clang/lib/Sema/SemaDeclAttr.cpp

3011

I did changes you asked two weeks ago. I'm waiting a review of the changes.

[aaron.ballman]

An interesting thing to note is that another implementation of this attribute (from IBM) is also explicit about requiring the type to be complete: http://www.ibm.com/support/knowledgecenter/SSGH2K_13.1.0/com.ibm.xlc131.aix.doc/language_ref/type_attr_transp_union.html

Out of curiosity, is this a common use of the feature that is required by some library headers? I kind of wonder if GCC gets around this by not checking the calling conventions for the union members at all. For instance, Clang compiles this, but GCC claims the union cannot be made transparent:

union TU2 {
  _Alignas(128) int *i;
  _Alignas(128) struct st *s;
} __attribute__((__transparent_union__));

void baz(union TU2);

void foo(int *i) {
  baz(i);
}

Adding Richard Smith, in case he has thoughts or ideas. I admit that I'm not overly familiar with this attribute, but the fact that two implementations agree that the union must be complete makes me wonder if this is an explicit feature of GCC or not.

llvm/tools/clang/include/clang/AST/DeclBase.h

284 ↗

(On Diff #76555)

This comment is a bit unclear. What does it mean for a declaration to be "parsing"?

llvm/tools/clang/lib/Sema/SemaDecl.cpp

13296

This does not seem like a particularly clean way to solve this problem. It's a bit strange for some part of the semantics engine to claim a declaration is currently being parsed, and it's fragile that the only way to set that it is no longer being parsed is when processing the attribute list.

13297

No need to pass true here as it defaults to true already.

llvm/tools/clang/lib/Sema/SemaDeclAttr.cpp

3011

Sorry about the delayed response; I've been on vacation for the past while and did not have Internet access.

[vbyakovlcl]

December 27 is my last day at Intel. I added a reviewer Erich Keane to finish the work if I will not able to do it.

llvm/tools/clang/include/clang/AST/DeclBase.h

284 ↗

(On Diff #76555)

Fixed

llvm/tools/clang/lib/Sema/SemaDecl.cpp

13296

To fix problem we need to handle the transparent_union attribute after the declaration parsing is finished. In the previous reduction I passed a boolean argument to handleTransparentUnionAttr to say that it needs to delay the attribute handling. Now I simple pass this flag via a Decl field. I don't see what problem can be here.

13297

Fxed

[rsmith]

llvm/tools/clang/include/clang/AST/DeclBase.h

287–288 ↗

(On Diff #82327)

It's not OK to add a bit here and make all declarations 8 bytes larger.

You also don't need this -- TagDecl::IsBeingDefined already carries the information you want here. It's possible that we don't set that flag quite early enough, but we can fix that if necessary, by slightly deferring when we process attributes in the parser.

Alternatively, we could defer processing attributes on a tag definition (in the parser) until after we've handled the tag definition.

[erichkeane]

It hasn't received any attention, but I've put the rework of this patch into the review here: https://reviews.llvm.org/D28266. The patch fixes all the compliants mentioned here.

I suspect that this one can be closed.

-Erich

[vbyakovlcl]

llvm/tools/clang/include/clang/AST/DeclBase.h

287–288 ↗

(On Diff #82327)

I also prepared changes using TagDecl::IsBeingDefined which quite similar to Erich did. Please change which is better.

[erichkeane]

I don't believe this will work correctly, the change I made in mine to SemaDecl.cpp is necessary, otherwise the ProcessDeclAttributeList will be called with out isBeingDefined properly set.

llvm/tools/clang/lib/Sema/SemaDeclAttr.cpp

3196

Why bother with the downcast? RD is a RecordDecl which inherits from TagDecl.

[vbyakovlcl]

llvm/tools/clang/lib/Sema/SemaDeclAttr.cpp

3196

You are right.

[erichkeane]

llvm/tools/clang/lib/Sema/SemaDeclAttr.cpp

3196

See my overall comment earlier, I think that the change to SemaDecl.cpp is necessary, otherwise 'isBeingDefined' will always be false here.

[erichkeane]

Did this in a separate review.

[erichkeane]

done separately

llvm/tools/clang/include/clang/Sema/Sema.h

Decl *ActOnTag(Scope *S, unsigned TagSpec, TagUseKind TUK,
                  SourceLocation KWLoc, CXXScopeSpec &SS,
                  IdentifierInfo *Name, SourceLocation NameLoc,
                  AttributeList *Attr, AccessSpecifier AS,
                  SourceLocation ModulePrivateLoc,
                  MultiTemplateParamsArg TemplateParameterLists,
                  bool &OwnedDecl, bool &IsDependent,
                  SourceLocation ScopedEnumKWLoc,
                  bool ScopedEnumUsesClassTag, TypeResult UnderlyingType,
-                 bool IsTypeSpecifier, SkipBodyInfo *SkipBody = nullptr);
+                 bool IsTypeSpecifier, SkipBodyInfo *SkipBody = nullptr,
+                 bool DeclIsParsed = true);
 
   Decl *ActOnTemplatedFriendTag(Scope *S, SourceLocation FriendLoc,
                                 unsigned TagSpec, SourceLocation TagLoc,
                                 CXXScopeSpec &SS,
                                 IdentifierInfo *Name, SourceLocation NameLoc,
                                 AttributeList *Attr,
                                 MultiTemplateParamsArg TempParamLists);
 
NamedDecl *ImplicitlyDefineFunction(SourceLocation Loc, IdentifierInfo &II,
                                       Scope *S);
   void AddKnownFunctionAttributes(FunctionDecl *FD);
 
   // More parsing and symbol table subroutines.
 
   void ProcessPragmaWeak(Scope *S, Decl *D);
   // Decl attributes - this routine is the top level dispatcher.
   void ProcessDeclAttributes(Scope *S, Decl *D, const Declarator &PD);
+  // Helper for delayed proccessing TransparentUnion attribute.
+  void ProcessDeclAttributeDelayed(Decl *D, const AttributeList *AttrList);
   void ProcessDeclAttributeList(Scope *S, Decl *D, const AttributeList *AL,
-                                bool IncludeCXX11Attributes = true);
+                                bool IncludeCXX11Attributes = true,
+                                bool DeclIsParsed = true);
   bool ProcessAccessDeclAttributeList(AccessSpecDecl *ASDecl,
                                       const AttributeList *AttrList);
 
   void checkUnusedDeclAttributes(Declarator &D);
 
   /// Determine if type T is a valid subject for a nonnull and similar
   /// attributes. By default, we look through references (the behavior used by
   /// nonnull), but if the second parameter is true, then we treat a reference

llvm/tools/clang/lib/Parse/ParseDeclCXX.cpp

if (TemplateId) {
     // Declaration or definition of a class type
     TagOrTempResult = Actions.ActOnTag(getCurScope(), TagType, TUK, StartLoc,
                                        SS, Name, NameLoc, attrs.getList(), AS,
                                        DS.getModulePrivateSpecLoc(),
                                        TParams, Owned, IsDependent,
                                        SourceLocation(), false,
                                        clang::TypeResult(),
                                        DSC == DSC_type_specifier,
-                                       &SkipBody);
+                                       &SkipBody,
+                                       false);
 
     // If ActOnTag said the type was dependent, try again with the
     // less common call.
     if (IsDependent) {
       assert(TUK == Sema::TUK_Reference || TUK == Sema::TUK_Friend);
       TypeResult = Actions.ActOnDependentTag(getCurScope(), TagType, TUK,
                                              SS, Name, StartLoc, NameLoc);
     }
   }
 
   // If there is a body, parse it and inform the actions module.
   if (TUK == Sema::TUK_Definition) {
     assert(Tok.is(tok::l_brace) ||
            (getLangOpts().CPlusPlus && Tok.is(tok::colon)) ||
            isCXX11FinalKeyword());
     if (SkipBody.ShouldSkip)
       SkipCXXMemberSpecification(StartLoc, AttrFixitLoc, TagType,
                                  TagOrTempResult.get());
     else if (getLangOpts().CPlusPlus)
       ParseCXXMemberSpecification(StartLoc, AttrFixitLoc, attrs, TagType,
                                   TagOrTempResult.get());
     else
       ParseStructUnionBody(StartLoc, TagType, TagOrTempResult.get());
   }
+  // Delayed proccessing TransparentUnion attribute.
+  Actions.ProcessDeclAttributeDelayed(TagOrTempResult.get(), attrs.getList());
 
   const char *PrevSpec = nullptr;
   unsigned DiagID;
   bool Result;
   if (!TypeResult.isInvalid()) {
     Result = DS.SetTypeSpecType(DeclSpec::TST_typename, StartLoc,
                                 NameLoc.isValid() ? NameLoc : StartLoc,
                                 PrevSpec, DiagID, TypeResult.get(), Policy);

llvm/tools/clang/lib/Sema/SemaDecl.cpp

Decl *Sema::ActOnTag(Scope *S, unsigned TagSpec, TagUseKind TUK,
                      IdentifierInfo *Name, SourceLocation NameLoc,
                      AttributeList *Attr, AccessSpecifier AS,
                      SourceLocation ModulePrivateLoc,
                      MultiTemplateParamsArg TemplateParameterLists,
                      bool &OwnedDecl, bool &IsDependent,
                      SourceLocation ScopedEnumKWLoc,
                      bool ScopedEnumUsesClassTag,
                      TypeResult UnderlyingType,
-                     bool IsTypeSpecifier, SkipBodyInfo *SkipBody) {
+                     bool IsTypeSpecifier, SkipBodyInfo *SkipBody,
+                     bool DeclIsParsed) {
   // If this is not a definition, it must have a name.
   IdentifierInfo *OrigName = Name;
   assert((Name != nullptr || TUK == TUK_Definition) &&
          "Nameless record must be a definition!");
   assert(TemplateParameterLists.size() == 0 || TUK != TUK_Reference);
 
   OwnedDecl = false;
   TagTypeKind Kind = TypeWithKeyword::getTagTypeKindForTypeSpec(TagSpec);
if (getLangOpts().CPlusPlus) {
       Diag(Loc, diag::warn_decl_in_param_list) << Context.getTagDeclType(New);
     }
     DeclsInPrototypeScope.push_back(New);
   }
 
   if (Invalid)
     New->setInvalidDecl();
 
   if (Attr)
-    ProcessDeclAttributeList(S, New, Attr);
+    ProcessDeclAttributeList(S, New, Attr, true, DeclIsParsed);
 
   // Set the lexical context. If the tag has a C++ scope specifier, the
   // lexical context will be different from the semantic context.
   New->setLexicalDeclContext(CurContext);
 
   // Mark this as a friend decl if applicable.
   // In Microsoft mode, a friend declaration also acts as a forward
   // declaration so we always pass true to setObjectOfFriendDecl to make

llvm/tools/clang/lib/Sema/SemaDeclAttr.cpp

static void handleFormatAttr(Sema &S, Decl *D, const AttributeList &Attr) {
   FormatAttr *NewAttr = S.mergeFormatAttr(D, Attr.getRange(), II,
                                           Idx, FirstArg,
                                           Attr.getAttributeSpellingListIndex());
   if (NewAttr)
     D->addAttr(NewAttr);
 }
 
 static void handleTransparentUnionAttr(Sema &S, Decl *D,
-                                       const AttributeList &Attr) {
+                                       const AttributeList &Attr,
+                                       bool DeclIsParsed) {
   // Try to find the underlying union declaration.
   RecordDecl *RD = nullptr;
   TypedefNameDecl *TD = dyn_cast<TypedefNameDecl>(D);
   if (TD && TD->getUnderlyingType()->isUnionType())
     RD = TD->getUnderlyingType()->getAsUnionType()->getDecl();
   else
     RD = dyn_cast<RecordDecl>(D);
 
   if (!RD || !RD->isUnion()) {
     S.Diag(Attr.getLoc(), diag::warn_attribute_wrong_decl_type)
       << Attr.getName() << ExpectedUnion;
     return;
   }
 
   if (!RD->isCompleteDefinition()) {
+    if (DeclIsParsed)
       S.Diag(Attr.getLoc(),
              diag::warn_transparent_union_attribute_not_definition);
     return;
   }
 
   RecordDecl::field_iterator Field = RD->field_begin(),
                           FieldEnd = RD->field_end();
   if (Field == FieldEnd) {
     S.Diag(Attr.getLoc(), diag::warn_transparent_union_attribute_zero_fields);
     return;
S.AddAlignedAttr(Attr.getRange(), D, E, Attr.getAttributeSpellingListIndex(),
                    Attr.isPackExpansion());
 }
 
 void Sema::AddAlignedAttr(SourceRange AttrRange, Decl *D, Expr *E,
                           unsigned SpellingListIndex, bool IsPackExpansion) {
   AlignedAttr TmpAttr(AttrRange, Context, true, E, SpellingListIndex);
   SourceLocation AttrLoc = AttrRange.getBegin();
 
   // C++11 alignas(...) and C11 _Alignas(...) have additional requirements.
   if (TmpAttr.isAlignas()) {
     // C++11 [dcl.align]p1:
     //   An alignment-specifier may be applied to a variable or to a class
     //   data member, but it shall not be applied to a bit-field, a function
     //   parameter, the formal parameter of a catch clause, or a variable
     //   declared with the register storage class specifier. An
     //   alignment-specifier may also be applied to the declaration of a class
     //   or enumeration type.

 // Top Level Sema Entry Points
 //===----------------------------------------------------------------------===//
 
 /// ProcessDeclAttribute - Apply the specific attribute to the specified decl if
 /// the attribute applies to decls.  If the attribute is a type attribute, just
 /// silently ignore it if a GNU attribute.
 static void ProcessDeclAttribute(Sema &S, Scope *scope, Decl *D,
                                  const AttributeList &Attr,
-                                 bool IncludeCXX11Attributes) {
+                                 bool IncludeCXX11Attributes,
+                                 bool DeclIsParsed = true) {
   if (Attr.isInvalid() || Attr.getKind() == AttributeList::IgnoredAttribute)
     return;
 
   // Ignore C++11 attributes on declarator chunks: they appertain to the type
   // instead.
   if (Attr.isCXX11Attribute() && !IncludeCXX11Attributes)
     return;
 
case AttributeList::AT_Weak:
     break;
   case AttributeList::AT_WeakRef:
     handleWeakRefAttr(S, D, Attr);
     break;
   case AttributeList::AT_WeakImport:
     handleWeakImportAttr(S, D, Attr);
     break;
   case AttributeList::AT_TransparentUnion:
-    handleTransparentUnionAttr(S, D, Attr);
+    handleTransparentUnionAttr(S, D, Attr, DeclIsParsed);
     break;
   case AttributeList::AT_ObjCException:
     handleSimpleAttribute<ObjCExceptionAttr>(S, D, Attr);
     break;
   case AttributeList::AT_ObjCMethodFamily:
     handleObjCMethodFamilyAttr(S, D, Attr);
     break;
   case AttributeList::AT_ObjCNSObject:
case AttributeList::AT_XRayInstrument:
     break;
   }
 }
 
 /// ProcessDeclAttributeList - Apply all the decl attributes in the specified
 /// attribute list to the specified decl, ignoring any type attributes.
 void Sema::ProcessDeclAttributeList(Scope *S, Decl *D,
                                     const AttributeList *AttrList,
-                                    bool IncludeCXX11Attributes) {
+                                    bool IncludeCXX11Attributes,
+                                    bool DeclIsParsed) {
   for (const AttributeList* l = AttrList; l; l = l->getNext())
-    ProcessDeclAttribute(*this, S, D, *l, IncludeCXX11Attributes);
+    ProcessDeclAttribute(*this, S, D, *l, IncludeCXX11Attributes,
+                         DeclIsParsed);
 
   // FIXME: We should be able to handle these cases in TableGen.
   // GCC accepts
   // static int a9 __attribute__((weakref));
   // but that looks really pointless. We reject it.
   if (D->hasAttr<WeakRefAttr>() && !D->hasAttr<AliasAttr>()) {
     Diag(AttrList->getLoc(), diag::err_attribute_weakref_without_alias)
       << cast<NamedDecl>(D);
if (!D->hasAttr<OpenCLKernelAttr>()) {
     } else if (Attr *A = D->getAttr<AMDGPUNumVGPRAttr>()) {
       Diag(D->getLocation(), diag::err_attribute_wrong_decl_type)
         << A << ExpectedKernelFunction;
       D->setInvalidDecl();
     }
   }
 }
 
+// Helper for delayed proccessing TransparentUnion attribute. 
+void Sema::ProcessDeclAttributeDelayed(Decl *D, const AttributeList * AttrList) {
+  for (const AttributeList* l = AttrList; l; l = l->getNext())
+    if (l->getKind() == AttributeList::AT_TransparentUnion) {
+      handleTransparentUnionAttr(*this, D, *l, true);
+      break;
+    }
+}
+
 // Annotation attributes are the only attributes allowed after an access
 // specifier.
 bool Sema::ProcessAccessDeclAttributeList(AccessSpecDecl *ASDecl,
                                           const AttributeList *AttrList) {
   for (const AttributeList* l = AttrList; l; l = l->getNext()) {
     if (l->getKind() == AttributeList::AT_Annotate) {
       ProcessDeclAttribute(*this, nullptr, ASDecl, *l, l->isCXX11Attribute());
     } else {

llvm/tools/clang/test/CodeGen/transparent-union.c

 // RUN: %clang_cc1 -Werror -triple x86_64-linux -emit-llvm -o - %s | FileCheck %s
 // RUN: %clang_cc1 -Werror -triple i386-linux -emit-llvm -o - %s | FileCheck %s
 // RUN: %clang_cc1 -Werror -triple armv7-linux -emit-llvm -o - %s | FileCheck %s --check-prefix=ARM
 // RUN: %clang_cc1 -Werror -triple powerpc64le-linux -emit-llvm -o - %s | FileCheck %s
 // RUN: %clang_cc1 -Werror -triple aarch64-linux -emit-llvm -o - %s | FileCheck %s
+// RUN: %clang_cc1 -DINFRONT -Werror -triple x86_64-linux -emit-llvm -o - %s | FileCheck %s
+// RUN: %clang_cc1 -DINFRONT -Werror -triple i386-linux -emit-llvm -o - %s | FileCheck %s
+// RUN: %clang_cc1 -DINFRONT -Werror -triple armv7-linux -emit-llvm -o - %s | FileCheck %s --check-prefix=ARM
+// RUN: %clang_cc1 -DINFRONT -Werror -triple powerpc64le-linux -emit-llvm -o - %s | FileCheck %s
+// RUN: %clang_cc1 -DINFRONT -Werror -triple aarch64-linux -emit-llvm -o - %s | FileCheck %s
 
+#ifdef INFRONT
+typedef union __attribute__((transparent_union)) {
+  void *f0;
+} transp_t0;
+#else
 typedef union {
   void *f0;
 } transp_t0 __attribute__((transparent_union));
+#endif
 
 void f0(transp_t0 obj);
 
 // CHECK-LABEL: define void @f1_0(i32* %a0)
 // CHECK:  call void @f0(i8* %{{.*}})
 // CHECK:  call void %{{.*}}(i8* %{{[a-z0-9]*}})
 // CHECK: }
 

llvm/tools/clang/test/Sema/transparent-union.c

 union pr30520v { void b; } __attribute__((transparent_union)); // expected-error {{field has incomplete type 'void'}}
 
 union pr30520a { int b[]; } __attribute__((transparent_union)); // expected-error {{field has incomplete type 'int []'}}
 
 // expected-note@+1 2 {{forward declaration of 'struct stb'}}
 union pr30520s { struct stb b; } __attribute__((transparent_union)); // expected-error {{field has incomplete type 'struct stb'}}
 
 union pr30520s2 { int *v; struct stb b; } __attribute__((transparent_union)); // expected-error {{field has incomplete type 'struct stb'}}
+
+typedef union __attribute__((__transparent_union__)) {
+  int *i;
+  struct st *s;
+} TU6;
+
+void bar(TU6);
+
+void foo11(int *i) {
+  bar(i);
+}
+void foo2(struct st *s) {
+  bar(s);
+}