GCC enum bit field compile error

[GoogleCodeExporter]

Compiling Types.h fails with any GCC older than 2013-02-16.  This is because of 
a bug in GCC that causes an error when using bit fields with enums.  
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=51242

A workaround now included in WebKit to compile the GTK port successfully is the 
following diff, although it might be worth removing the bit fields entirely and 
replacing them with unsigned char types which aren't much bigger than the 
current 6-bit and 7-bit bit fields.

Index: src/compiler/Types.h
===================================================================
--- src/compiler/Types.h    (revision 2426)
+++ src/compiler/Types.h    (working copy)
@@ -198,9 +198,15 @@
 private:
     TString buildMangledName() const;

+#ifdef __GNUC__
+    TBasicType type;
+    TPrecision precision;
+    TQualifier qualifier;
+#else
     TBasicType type      : 6;
     TPrecision precision;
     TQualifier qualifier : 7;
+#endif
     int size             : 8; // size of vector or matrix, not size of array
     unsigned int matrix  : 1;
     unsigned int array   : 1;

Original issue reported on code.google.com by achriste...@gmail.com on 19 Jul 2013 at 7:23

[GoogleCodeExporter]

I prefer to keep using enum types to facilitate debugging. There are other ways 
to keep the size of TType fairly small though. I'll see what I can do...

Original comment by nico...@transgaming.com on 30 Jul 2013 at 8:40

• Changed state: Accepted

[GoogleCodeExporter]

Original comment by nico...@transgaming.com on 2 Aug 2013 at 8:14

• Changed state: Started

[GoogleCodeExporter]

Fixed in revision d206c85e27e10636060f09b7feaba04f6d17665d.

Original comment by nico...@transgaming.com on 12 Aug 2013 at 3:44

• Changed state: Fixed

[GoogleCodeExporter]

Reopening-- this commit appears to break certain Chrome builds and has been 
reverted.

Original comment by shannonw...@chromium.org on 14 Aug 2013 at 2:47

• Changed state: Accepted

[GoogleCodeExporter]

Maybe keep the bit fields and make TBasicType and TQualifier a non-enum type 
for GCC and keep their enum type for Visual Studio to help with debugging. (I 
assume most of the development and debugging is done with Visual Studio.)

Original comment by achriste...@gmail.com on 15 Aug 2013 at 12:45

[GoogleCodeExporter]

Shannon, any idea what the build issue was exactly? I notice the GYP files 
disable fewer warnings than the Visual Studio projects, so I'll see if that 
turns up something...

Alex, I'm trying to avoid using compiler-specific code. If all else fails I'll 
use it, but my patch was probably missing some trivial conversion that didn't 
pass the warning level on the Chrome build system.

Original comment by nico...@transgaming.com on 15 Aug 2013 at 1:46

• Changed state: Started

[GoogleCodeExporter]

My understanding is that some builds were rendering all-black with this 
commit-- so not a build issue, but a rendering issue. Not sure if it's an issue 
with particular build (at least one person seeing the problem was running an 
Aura build), or with particular systems.

Original comment by shannonw...@google.com on 15 Aug 2013 at 3:06

[GoogleCodeExporter]

The build error was from WebKit's Qt, GTK, and EFL ports:
https://webkit-queues.appspot.com/results/1066395
https://webkit-queues.appspot.com/results/1069137

It was from patch #4 of my many tries to update ANGLE:
https://bugs.webkit.org/show_bug.cgi?id=118550

The EFL output is not terribly informative, but the fix I posted fixed all the 
ports.  These ports do not use ANGLE for rendering, but just for validation.

Original comment by achriste...@gmail.com on 15 Aug 2013 at 3:49

[GoogleCodeExporter]

There were no issues with building ANGLE. The shaders would either not compile 
or the translated shaders had junk characters. I think there was a char 
overflow somewhere.

Original comment by alokp@chromium.org on 15 Aug 2013 at 4:20

[GoogleCodeExporter]

That's quite bizarre because the only variable that got reduced in size and 
could therefore theoretically become prone to overflow is TType::size, but the 
only valid values for it are 1 to 4. Even when used to represent the matrix 
size in 'decimal' form, by computing size * 10 + size (e.g. 44 for a 4x4 
matrix), there's no char range overflow.

Either I overlooked some other potential use, or some external code didn't 
expect this interface change? Could you tell me how to reproduce the issue? I 
found no WebGL 1.0.2 regressions (although that doesn't test the use of ANGLE 
as a validator).

Original comment by nico...@transgaming.com on 15 Aug 2013 at 9:34

[GoogleCodeExporter]

The patch looked fine to me as well. I did not spend too much time debugging 
the problem. zmo@ reported seeing invalid characters in the output of 
translator sample (essl_to_glsl).

Original comment by alokp@chromium.org on 19 Aug 2013 at 4:41

[GoogleCodeExporter]

On linux every shader out of the shader translator failed because vec4 is 
translated to vec*, where * is a non-ascII character.

Original comment by z...@google.com on 19 Aug 2013 at 6:07

[GoogleCodeExporter]

Can this problem be repeated?  It doesn't seem like this problem would be 
caused by this fix.  It seems like the non-ascii character is caused by using 
the wrong version of a header somewhere and linking to get bad data.  That's 
just my gut instinct, though.  Could someone find out exactly which build 
system exhibited the translation failure and investigate it further?

Original comment by achriste...@gmail.com on 27 Aug 2013 at 4:08

[GoogleCodeExporter]

Jamie noticed that in TType::getCompleteString, the size would be printed out 
as a literal character, instead of being converted to an integer string 
representation. This explains the vec* with a non-ascii character.

I don't have the right setup to reproduce this and make sure the next patch is 
robust, but we're looking into it.

Original comment by nico...@transgaming.com on 27 Aug 2013 at 2:53

[GoogleCodeExporter]

New patch: https://codereview.appspot.com/13239046

It's basically the same as the reverted one except that getNominalSize() 
returns an int instead of a char. This should avoid the issue with 
getCompleteString(). Can someone who was able to reproduce the issue give it a 
try?

Original comment by nico...@transgaming.com on 27 Aug 2013 at 9:24

• Changed state: Review

[GoogleCodeExporter]

I'll give it a try tomorrow and let you know.

Original comment by z...@google.com on 28 Aug 2013 at 12:57

[GoogleCodeExporter]

Why was this only seen on linux?

Original comment by achriste...@gmail.com on 7 Sep 2013 at 1:16

[GoogleCodeExporter]

On Linux ANGLE is used to translate from GLSL ES to desktop GLSL, while on 
Windows things are translated to HLSL. So this uses a different code path and 
only the one on Linux was affected badly enough to cause actual issues.

Original comment by nico...@transgaming.com on 9 Sep 2013 at 3:20

[GoogleCodeExporter]

Is this stalled?

Original comment by shannonw...@google.com on 23 Sep 2013 at 10:17

[GoogleCodeExporter]

Yes.  Nobody has done anything on this for a few weeks.  I'd like to see this 
resolved.

Original comment by achriste...@gmail.com on 24 Sep 2013 at 10:49

[GoogleCodeExporter]

Sorry for the delay. New patch is in review.

Original comment by nico...@transgaming.com on 25 Sep 2013 at 6:54

[GoogleCodeExporter]

Committed as r7bf02174b003.

Original comment by nico...@transgaming.com on 26 Sep 2013 at 2:35

• Changed state: Fixed