This is an archive of the discontinued LLVM Phabricator instance.

[Sema] Ignore transparent_union attributes in C++
ClosedPublic

Authored by arphaman on Oct 6 2016, 3:31 AM.

Details

Summary

While working on fixing PR30520 yesterday I noticed that clang also crashes on code like this:

template<typename T>
union u { T b; } __attribute__((transparent_union));

I mentioned in https://reviews.llvm.org/D25273 that I plan on fixing it properly by instantiating the attribute, but, after looking more into it, I discovered that we don't really support 'transparent_union' in C++ at all. This is why I decided to go with the approach that's presented in this patch: the attribute is completely ignored when clang is in C++ mode, and thus the crash is avoided.

I wasn't sure if we should we emit an error or a warning when ignoring this attribute and if we need any diagnostics in Gnu mode at all. That's why in this initial patch doesn't emit a diagnostic when ignoring this attribute. Please let me know whether we need to show one or not.

Thanks

Diff Detail

Repository
rL LLVM

Event Timeline

arphaman updated this revision to Diff 73750.Oct 6 2016, 3:31 AM
arphaman retitled this revision from to [Sema] Ignore transparent_union attributes in C++.
arphaman updated this object.
arphaman added reviewers: rnk, aaron.ballman.
arphaman set the repository for this revision to rL LLVM.
arphaman added a subscriber: cfe-commits.
aaron.ballman requested changes to this revision.Oct 10 2016, 6:42 AM
aaron.ballman edited edge metadata.
aaron.ballman added inline comments.
lib/Sema/SemaDeclAttr.cpp
3011 ↗(On Diff #73750)

The correct way to do this is to mark the attribute as C-only on its declaration in Attr.td. See the definition of FlagEnum for an example.

This revision now requires changes to proceed.Oct 10 2016, 6:42 AM
arphaman updated this revision to Diff 74127.Oct 10 2016, 8:02 AM
arphaman edited edge metadata.
arphaman marked an inline comment as done.

The updated patch uses a proper way to mark the attribute as C only.

The updated patch now makes clang warn every time it encounters this attribute in C++ mode. Would that be the desired behaviour?

The updated patch now makes clang warn every time it encounters this attribute in C++ mode. Would that be the desired behaviour?

As I understand it, transparent_union was designed for use in system headers, and these might certainly be included into C++ source files. Does that not work correctly, or does the feature just not work correctly when combined with C++-specific features (templates, function overloading, etc.)?

aaron.ballman accepted this revision.Oct 10 2016, 1:29 PM
aaron.ballman edited edge metadata.

The updated patch now makes clang warn every time it encounters this attribute in C++ mode. Would that be the desired behaviour?

Yes, I believe that is the correct behavior in this instance.

The patch LGTM now, thank you!

include/clang/Basic/Attr.td
1545

If you, or anyone else, is bored, it would be nice to document this attribute now that we're touching it. (I am not signing you up for work as part of this patch, just prodding in case you wanted to do it.)

This revision is now accepted and ready to land.Oct 10 2016, 1:29 PM

Thanks!

include/clang/Basic/Attr.td
1545

That's a good idea, the lack of documentation stood out to me as well. I'll try to find some time this week for it.

This revision was automatically updated to reflect the committed changes.