undefined behaviour of nested anonymous unions with bitfields doesn't work as intended

[Quuxplusone]

Bugzilla Link	PR24608
Status	NEW
Importance	P normal
Reported by	Stefan Teleman (stefan.teleman@oracle.com)
Reported on	2015-08-27 21:24:23 -0700
Last modified on	2017-07-26 18:55:53 -0700
Version	3.6
Hardware	Sun Solaris
CC	fedor.v.sergeev@gmail.com, hfinkel@anl.gov, llvm-bugs@lists.llvm.org, shawn.walker@oracle.com, stefan.teleman@gmail.com, stefan.teleman@oracle.com
Fixed by commit(s)
Attachments	007-TemplateBase.patch (8544 bytes, text/plain) 008-DeclTemplate.patch (1098 bytes, text/plain)
Blocks
Blocked by
See also

This is a placeholder bug for a patch I will be submitting very shortly.

in ${top_srcdir}/tools/clang/include/clang/lib/AST/TemplateBase.h:

class TemplateArgument makes uses of an anonymous union of several structs.
One of these structs, namely struct I, contains an union of bitfields. Also,
class TemplateArgument does not explicitly define a copy constructor and an
assignment operator, it relies on the compiler-generated defaults.

C++ bitfields inside an union are undefined behaviour according to the
Standard. Relying on compiler default-generated copy-constructor and
assignment operator for copying unions with bitfields of different type,
or size, is undefined behaviour as well.

This happens to work correctly on Intel (i386/x86_64) but does not work at
all on SPARC.

I have a patch for this defect, which corrects it. I will submit this patch
very shortly.

We have a number of clang and llvm patches for Solaris, and we have every
intention of contributing them to the project. This is just one of several
bugs with follow-up patches that I will be filing.

[Quuxplusone]

Attached 007-TemplateBase.patch (8544 bytes, text/plain): proposed patch

[Quuxplusone]

Attached 008-DeclTemplate.patch (1098 bytes, text/plain): proposed patch 2 - 008-DeclTemplate.patch

[Quuxplusone]

> C++ bitfields inside an union are undefined behaviour according to the
Standard.

Sorry, I dont see any standardeze that prohibits bitfields inside a union.
While it is indeed tricky to use union members, undefined behavior in that part
is related only accessing a "non-active" member of a union, and it is not
related to bitfields.

> Relying on compiler default-generated copy-constructor and
assignment operator for copying unions with bitfields of different type,
or size, is undefined behaviour as well.

Sorry, again this is not what standard specifies.
See 15.8.1[class.copy.ctor]p15 (latest draft numbering):
   The implicitly-defined copy/move constructor for a union X copies the object representation of X.

(same for assignment operator in 15.8.2).

I dont see a point in adding zeroing memsets to constructors where all the
necessary members are being initialized.

Please, show a reproducer, otherwise I'm inclined to close this.

[Quuxplusone]

Try removing this patch, re-compile clang, and then try compiling any templates
with the new clang. It will reliably crash on SPARC.

Also:

http://en.cppreference.com/w/cpp/language/union

Also: C99 Standard Appendix J Section 1: Unspecified behavior:

— The value of padding bytes when storing values in structures or unions
(6.2.6.1).
— The value of a union member other than the last one stored into (6.2.6.1).

[Quuxplusone]

> Try removing this patch

I dont understand what patch are you talking about.
As far as I know the patch(es) attached here are not in LLVM upstream.

> try compiling any templates with the new clang. It will reliably crash on
SPARC.

Current LLVM upstream is capable of bootstrapping itself on SPARC Solaris.
I'm not seeing any crashes during bootstrap, and clang/LLVM is surely full of
templates.

> http://en.cppreference.com/w/cpp/language/union

there is nothing here that requires writing explicit copy constructors for
unions.

> C99 Standard Appendix J Section 1: Unspecified behavior:

C99 standard is somewhat irrelevant, as we are in C++ land here

[Quuxplusone]

You claim LLVM 6.0 on SPARC now bootstraps itself with no problems and no crashes? Great. What does this have to do with LLVM 3.8.1 or 3.6.2?

Also: you are wrong in saying that unions and/or union bitfields in C99 have nothing to do with C++.

If the C++ Standard is unclear about the behavior of union bitfields, or if it explicitly states that union bitfields have normative behavior, you should raise a defect with the C++ Standard. Because (a) I don't remember the C++ Standard ever saying anything about nested unions or union bitfields having normative behavior and (b) that would be an incompatibility with C.

If the compiler you are using now to bootstrap LLVM 6.0 handles nested union bitfields predictably and correctly on SPARC, great. I wouldn't count on it being normative behavior.

However, stop saying that these patches weren't necessary for the LLVM version that they were written for - namely 3.8.1. They were necessary.