Review of transparent_union patch

Anders Johnsen submitted a patch to implement semantic analysis for transparent_union in

  http://llvm.org/bugs/show_bug.cgi?id=2015

Here's a review of that patch, which is almost ready to go in.

Could you include a test-case as part of your patch?

+ // FIXME: Corredt behaviour for no-definition unions?
+ if (!RD->isDefinition())
+ return;

Typo "corredt".

Also, I don't think this is the behavior we want. If the union is
forward-declared with the transparent_union attribute, we should keep
that attribute, as in this example:

   union wait { float *fp; double *dp; };

   union wait_status_ptr_t __attribute__ ((__transparent_union__));

   union wait_status_ptr_t
   {
     int *__ip;
     union wait *__up;
   };

   int wait (union wait_status_ptr_t);

   int w1 () { int w; return wait (&w); }
   int w2 () { union wait w; return wait (&w); }

GCC fails on this example. We also fail, because when there is no
definition of the union, we silently ignore the attribute. We either
need to (a) produce a warning saying that this attribute is ignored if
it isn't on the definition, or (b) add the attribute, then make sure
it gets processed properly when the union is defined, later. While I
like (b) better, GCC seems to be doing (a) without warning about it,
so I suggest we do (a) with the warning.

+ // Ignore anonymous unions.
+ if (RD->isAnonymousStructOrUnion())
+ return;

Here, again, we should produce a warning saying that we're ignoring
this attribute, since it can't apply to anonymous structs/unions.

+ if (ArgType->isUnionType()) {
+ const RecordType *UT = ArgType->getAsUnionType();

Please replace this with:

   if (const RecordType *UT = ArgType->getAsUnionType())

That way, we only walk through typedefs once.

GCC completely ignores the transparent_union attribute in C++ mode. I
suggest that we do the same (and produce a warning to say we did it!).

With those (few) tweaks, this is ready to go in. Thanks!

   - Doug

Hi,

Thank you for the review. I've attached a corrected patch.

Anders

transparent_union.patch (8.26 KB)

A few notes:

1. This breaks CodeGen for transparent unions, which was working
before; it seems like this will be a large regression for CodeGen for
many programs.

2. The way the attribute is getting attached looks wrong; this is a
union attribute, so it's required to appear immediately after the
definition of the union. I suppose that's mostly orthogonal to this
patch, though.

3.
+ // Ignore anonymous unions.
+ if (RD->isAnonymousStructOrUnion()) {
+ S.Diag(Attr.getLoc(),
+ diag::warn_transparent_union_attribute_anonymous);
+ return;
+ }
Why exactly are we ignoring anonymous structs/unions? gcc seems to
support them; here's a testcase:
void a(union {int a; int* b;} __attribute((transparent_union)) x);
void b() {a(1);}

4. It looks like this allows floating-point types where gcc doesn't; testcase:

union x {float a;int* b;} __attribute((transparent_union));
void a(union x a);
void b() {a(1.0f);}

-Eli

Hi,

Hi,

Thank you for the review. I've attached a corrected patch.

A few notes:

1. This breaks CodeGen for transparent unions, which was working
before; it seems like this will be a large regression for CodeGen for
many programs.

Yes. I'm not sure where in CodeGen I should add support for this. But
the other way was limited to pointer-types only and was using the
wrong CC(should be as first type *i8, but was handled as [x * i8]
afaik.

2. The way the attribute is getting attached looks wrong; this is a
union attribute, so it's required to appear immediately after the
definition of the union. I suppose that's mostly orthogonal to this
patch, though.

What exactly do you mean by this?

3.
+ // Ignore anonymous unions.
+ if (RD->isAnonymousStructOrUnion()) {
+ S.Diag(Attr.getLoc(),
+ diag::warn_transparent_union_attribute_anonymous);
+ return;
+ }
Why exactly are we ignoring anonymous structs/unions? gcc seems to
support them; here's a testcase:
void a(union {int a; int* b;} __attribute((transparent_union)) x);
void b() {a(1);}

You are right. Fixed

4. It looks like this allows floating-point types where gcc doesn't; testcase:

union x {float a;int* b;} __attribute((transparent_union));
void a(union x a);
void b() {a(1.0f);}

-Eli

I've not handled the case where the first element is a floating point
or vector. Is fixed in patch.

I've also added a check for a empty union.

Thanks you for the comments!

Anders

transparent_union.patch (8.62 KB)

Something like the following:
struct x {int x; int* y;};
typedef struct x a __attribute((transparent_union));

This isn't valid, but it looks like your patch accepts it.

-Eli

Something like the following:
struct x {int x; int* y;};
typedef struct x a __attribute((transparent_union));

This isn't valid, but it looks like your patch accepts it.

I just tested this, and it looks like it worked:

test/Sema/transparent-union-pointer.c:3:33: warning:
'transparent_union' attribute only applies to union types
typedef struct x a __attribute((transparent_union));
                                ^

-Eli

Anders

Oh, oops, make that "union x".

-Eli

Something like the following:
struct x {int x; int* y;};
typedef struct x a __attribute((transparent_union));

This isn't valid, but it looks like your patch accepts it.

I just tested this, and it looks like it worked:

Oh, oops, make that "union x".

gcc does't complain. I'm not sure if i should do anything? Where have
you read that it shouldn't allow such things?

-Eli

Anders

Hmm, I don't remember the details... if gcc accepts it, I suppose I
must be mistaken.

-Eli

GCC's attributes aren't handle consistently in the type system. Try this example:

   union x {int x; int* y;};
   typedef union x a __attribute((transparent_union));

   void foo(union x);
   void bar(a);

   void test(int i) {
     foo(i); // fails
     bar(i); // okay
   }

So, a typedef of "union x" has different semantics from "union x", despite the fact that those types should be identical. The GCC guys know about these problems; see the discussion at Mark Mitchell - Proposed semantics for attributes in C++ (and in C?)

I think that Clang should warn about the addition of "semantic" attributes (to steal Mark M.'s terminology) via typedefs, and say something about the typedef behaving differently from the original type due to the attribute. In C++ this is particularly important, since template instantiation needs to be based on the original type, even when one uses the typedef to name the instantiation (this is a particularly messy area in GCC).

However, it seems to me that this patch is ready to go in. Any other issues we need to address first?

  - Doug

Hi Anders,

I have a few comments...