Transparent_union attribute should be possible in front of union (rework)

[erichkeane]

Summary

This is an update of this patch: https://reviews.llvm.org/D25824

I cannot seem to get that review to update with my diff. This first patch removes the dependency on isParsing and switches to isBeingDefined. In order to get that to work, changing where attributes where changed was necessary.

Original description below:

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.

[erichkeane]

lib/Sema/SemaDecl.cpp

13499 ↗

(On Diff #82976)

I wasn't sure if this was the correct change in here. My other option was to move the above 2 lines (13495/13496) above these line's original spot, but moving it here seems to have correctly passed all tests. Anyone with more knowledge here want to chime in?

test/Sema/transparent-union.c

57 ↗

(On Diff #82976)

2 Additional sema tests to ensure that the same warnings occur both in prefix and postfix attribute definition.

[erichkeane]

Hi guys-
Since this is a rework of Vlad's changes, I'm not sure you noticed that this was a different review! Anyone have opinions? Additionally, I had 1 question in SemaDecl.cpp that I think was right, otherwise I hope this is a pretty light review.

[aaron.ballman]

Some minor nits, but I think this is generally correct.

include/clang/Sema/Sema.h

3059 ↗

(On Diff #82976)

I would remove the mention of TransparentUnion -- this is generalized for other uses already.

lib/Parse/ParseDeclCXX.cpp

1889 ↗

(On Diff #82976)

I'd remove mention of TransparentUnion here as well.

lib/Sema/SemaDecl.cpp

13499 ↗

(On Diff #82976)

I think this change is reasonable.

lib/Sema/SemaDeclAttr.cpp

6201 ↗

(On Diff #82976)

Nit: the * should bind to l and l should be L by our usual naming conventions (but a name better than L wouldn't be amiss).

[erichkeane]

Thanks! Changes incoming.

[erichkeane]

Response to Aaron's comments

[aaron.ballman]

LGTM, with one formatting nit.

lib/Sema/SemaDeclAttr.cpp

6201 ↗

(On Diff #82976)

Nit: missing a space before the first =. You should run clang-format on the patch to catch this sort of thing.

cfe/trunk/include/clang/Sema/Sema.h

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 of attributes.
+  void ProcessDeclAttributeDelayed(Decl *D, const AttributeList *AttrList);
   void ProcessDeclAttributeList(Scope *S, Decl *D, const AttributeList *AL,
                                 bool IncludeCXX11Attributes = 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

cfe/trunk/lib/Parse/ParseDeclCXX.cpp

if (SkipBody.ShouldSkip)
                                  TagOrTempResult.get());
     else if (getLangOpts().CPlusPlus)
       ParseCXXMemberSpecification(StartLoc, AttrFixitLoc, attrs, TagType,
                                   TagOrTempResult.get());
     else
       ParseStructUnionBody(StartLoc, TagType, TagOrTempResult.get());
   }
 
+  if (!TagOrTempResult.isInvalid())
+    // Delayed proccessing of attributes.
+    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);
   } else if (!TagOrTempResult.isInvalid()) {

cfe/trunk/lib/Sema/SemaDecl.cpp

if ((Name || Kind == TTK_Enum) &&
     } else if (!PrevDecl) {
       Diag(Loc, diag::warn_decl_in_param_list) << Context.getTagDeclType(New);
     }
   }
 
   if (Invalid)
     New->setInvalidDecl();
 
-  if (Attr)
-    ProcessDeclAttributeList(S, New, Attr);
-
   // 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
   // the tag name visible.
   if (TUK == TUK_Friend)
     New->setObjectOfFriendDecl(getLangOpts().MSVCCompat);
 
   // Set the access specifier.
   if (!Invalid && SearchDC->isRecord())
     SetMemberAccessSpecifier(New, PrevDecl, AS);
 
   if (TUK == TUK_Definition)
     New->startDefinition();
 
+  if (Attr)
+    ProcessDeclAttributeList(S, New, Attr);
+
   // If this has an identifier, add it to the scope stack.
   if (TUK == TUK_Friend) {
     // We might be replacing an existing declaration in the lookup tables;
     // if so, borrow its access specifier.
     if (PrevDecl)
       New->setAccess(PrevDecl->getAccess());
 
     DeclContext *DC = New->getDeclContext()->getRedeclContext();

cfe/trunk/lib/Sema/SemaDeclAttr.cpp

static void handleTransparentUnionAttr(Sema &S, Decl *D,
 
   if (!RD || !RD->isUnion()) {
     S.Diag(Attr.getLoc(), diag::warn_attribute_wrong_decl_type)
       << Attr.getName() << ExpectedUnion;
     return;
   }
 
   if (!RD->isCompleteDefinition()) {
+    if (!RD->isBeingDefined())
       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;
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 *Attr = AttrList; Attr; Attr = Attr->getNext())
+    if (Attr->getKind() == AttributeList::AT_TransparentUnion) {
+      handleTransparentUnionAttr(*this, D, *Attr);
+      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 {

cfe/trunk/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: }
 

cfe/trunk/test/Sema/transparent-union.c

 typedef struct { int x, y; } __attribute__((aligned(8))) aligned_struct8;
 
 typedef struct { int x, y; } __attribute__((aligned(4))) aligned_struct4;
 typedef union {
   aligned_struct4 s4; // expected-note{{alignment of first field}}
   aligned_struct8 s8; // expected-warning{{alignment of field}}
 } TU1 __attribute__((transparent_union));
 
+typedef union __attribute__((transparent_union)) {
+  aligned_struct4 s4; // expected-note{{alignment of first field}}
+  aligned_struct8 s8; // expected-warning{{alignment of field}}
+} TU1b ;
+
 typedef union {
   char c; // expected-note{{size of first field is 8 bits}}
   int i; // expected-warning{{size of field}}
 } TU2 __attribute__((transparent_union));
 
+typedef union __attribute__((transparent_union)){
+  char c; // expected-note{{size of first field is 8 bits}}
+  int i; // expected-warning{{size of field}}
+} TU2b;
+
 typedef union {
   float f; // expected-warning{{floating}}
 } TU3 __attribute__((transparent_union));
 
 typedef union { } TU4 __attribute__((transparent_union)); // expected-warning{{field}} 
 
 typedef int int4 __attribute__((ext_vector_type(4)));
 typedef union {

 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);
+}