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

13295 ↗

(On Diff #76555)

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.

13298 ↗

(On Diff #76555)

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

13295 ↗

(On Diff #76555)

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.

13298 ↗

(On Diff #76555)

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–3198

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

[vbyakovlcl]

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

3196–3198

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

   void ProcessPragmaWeak(Scope *S, Decl *D);
   // Decl attributes - this routine is the top level dispatcher.
   void ProcessDeclAttributes(Scope *S, Decl *D, const Declarator &PD);
+  void ProcessDeclAttributeDelayed(Decl *D, const AttributeList *AttrList);
   void ProcessDeclAttributeList(Scope *S, Decl *D, const AttributeList *AL,
                                 bool IncludeCXX11Attributes = true);
   bool ProcessAccessDeclAttributeList(AccessSpecDecl *ASDecl,

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

       ParseStructUnionBody(StartLoc, TagType, TagOrTempResult.get());
   }
 
+  if (!TagOrTempResult.isInvalid())
+    Actions.ProcessDeclAttributeDelayed(TagOrTempResult.get(), attrs.getList());
+
   const char *PrevSpec = nullptr;
   unsigned DiagID;
   bool Result;

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

   }
 
   if (!RD->isCompleteDefinition()) {
-    S.Diag(Attr.getLoc(),
-        diag::warn_transparent_union_attribute_not_definition);
+    if (RD->isBeingDefined())
+      S.Diag(Attr.getLoc(),
+             diag::warn_transparent_union_attribute_not_definition);
     return;
   }
 

   }
 }
 
+// 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);
+      break;
+    }
+}
+
 // Annotation attributes are the only attributes allowed after an access
 // specifier.
 bool Sema::ProcessAccessDeclAttributeList(AccessSpecDecl *ASDecl,

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

 // 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);
 

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

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