Page MenuHomePhabricator

[clang][Sema] Don't try to initialize implicit variable of invalid anonymous union/struct
ClosedPublic

Authored by TaWeiTu on Mar 12 2021, 12:38 PM.

Details

Summary

This fixes https://bugs.llvm.org/show_bug.cgi?id=49534, where the call to the constructor
of the anonymous union is checked and triggers assertion failure when trying to retrieve
the alignment of the this argument (which is a union with virtual function).

The extra check for alignment was introduced in D97187.

Diff Detail

Event Timeline

TaWeiTu requested review of this revision.Mar 12 2021, 12:38 PM
TaWeiTu created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptMar 12 2021, 12:38 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript

I'm not familiar with this code and clang/sema in general, so not sure if the fix makes sense.

This seems quite an early point to bail out, so it will prevent some errors/warnings associated with initialization from being emitted. For example, this warning is currently emitted but would be suppressed by this patch:

union {
  virtual int a();
  int b = 'c'; // expected-warning {{in-class initialization of non-static data member is a C++11 extension [-Wc++11-extensions]}}
}

I don't know how much that matters seeing as the union is invalid by this point.

dyung added a subscriber: dyung.Mar 17 2021, 12:43 AM

This seems quite an early point to bail out, so it will prevent some errors/warnings associated with initialization from being emitted. For example, this warning is currently emitted but would be suppressed by this patch:

union {
  virtual int a();
  int b = 'c'; // expected-warning {{in-class initialization of non-static data member is a C++11 extension [-Wc++11-extensions]}}
}

I don't know how much that matters seeing as the union is invalid by this point.

Hi,

Thanks for the comment!
I understand the problem, but the warning you mentioned is not suppressed in this patch from what I've seen.
After poking around, I saw that diagnostics related to anonymous unions are all emitted in another call to ActOnUninitializedDecl in the parser, which takes place before this check.
If I completely remove this call to ActOnUninitializedDecl (on line 5215 to be specific), all the failed testcases seem to be related to CodeGen, so I suspect that removing this call for invalid unions will not suppress further warnings/errors.

What do you think?

tmatheson accepted this revision.Tue, Mar 30, 10:08 AM

That makes sense, you are correct that that warning is not prevented. I'm not sure what I did differently when I checked. LGTM.

This revision is now accepted and ready to land.Tue, Mar 30, 10:08 AM