This is an archive of the discontinued LLVM Phabricator instance.

[Sema][ObjC] Disallow non-trivial C struct fields in unions
ClosedPublic

Authored by ahatanak on Dec 13 2018, 9:32 AM.

Details

Summary

This patch fixes a bug where clang doesn’t reject union fields of non-trivial struct types:

struct S0 {
  id x;
};

struct S1 {
  id y;
};

union U0 {
  struct S0 s0;  // no diagnostics.
  struct S1 s1;  // no diagnostics.
};

union U1 {
  id x;  // clang rejects ObjC pointer fields in unions.
};

void test(union U0 a) {
  // Both ‘S0::x’ and ‘S1::y' are destructed in the IR.
}

rdar://problem/46677858

Diff Detail

Repository
rL LLVM

Event Timeline

ahatanak created this revision.Dec 13 2018, 9:32 AM

Just to clarify, this patch doesn't change clang's handling of C++ unions that have non-static data members with non-trivial special member functions.

ahatanak planned changes to this revision.Feb 5 2019, 4:08 PM

This patch is unnecessarily complicated and is not correct.

ahatanak updated this revision to Diff 185466.Feb 5 2019, 6:49 PM

Make sure that volatile trivial fields in a struct don't cause Sema to emit a diagnostic. Simplify the code in Sema::ActOnFields .

rjmccall accepted this revision.Feb 7 2019, 10:35 AM

Two minor tweaks, but otherwise LGTM.

lib/AST/Type.cpp
2273 ↗(On Diff #185466)

Might be good to spell this out: 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/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.

test/SemaObjC/arc-decls.m
24 ↗(On Diff #185466)

spacing

This revision is now accepted and ready to land.Feb 7 2019, 10:35 AM
This revision was automatically updated to reflect the committed changes.
ahatanak marked 2 inline comments as done.
Herald added a project: Restricted Project. · View Herald TranscriptFeb 7 2019, 12:21 PM