remove %code from glslang.y

[GoogleCodeExporter]

Bison 2.3 or 2.5 (which are required by the WebKit project) do not support the 
"%code requires" syntax currently used in glslang.y, and it fails to generate 
glslang_lex.cpp, glslang_tab.h, and glslang_tab.cpp.  The following workaround 
is now included in WebKit, and we'd like it if it were included upstream.  It's 
a bit unconventional to put #defines in union declarations, but it works.  
https://bugs.webkit.org/show_bug.cgi?id=118815

Index: src/compiler/glslang.y
===================================================================
--- src/compiler/glslang.y  (revision 2426)
+++ src/compiler/glslang.y  (working copy)
@@ -48,12 +48,9 @@
 %parse-param {TParseContext* context}
 %locations

-%code requires {
+%union {
 #define YYLTYPE TSourceLoc
 #define YYLTYPE_IS_DECLARED 1
-}
-
-%union {
     struct {
         union {
             TString *string;

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

[GoogleCodeExporter]

It is not required to compile glslang.y from source. There are generated 
glslang_tab.cpp and glslang_tab.h files in the repository that are always in 
sync with it. You only have to regenerate them if you plan on changing the 
grammar. If there are bugs in the grammar, please report them and we'll fix 
them as soon as possible and regenerate the parser files.

Is there a reason you can't use Bison 2.7?

I don't think there's a good guarantee that defining YYLTYPE inside the %union 
will always work. If this is the only option for you, we should at least add a 
comment to explain the hack.

That said, we may be able to eliminate using a custom YYLTYPE altogether...

Original comment by nico...@transgaming.com on 30 Jul 2013 at 9:28

[GoogleCodeExporter]

Bison 2.7 generates code with GPLv3 license which we are legally not allowed to 
include in WebKit:

   This program is free software: you can redistribute it and/or modify
   it under the terms of the GNU General Public License as published by
   the Free Software Foundation, either version 3 of the License, or
   (at your option) any later version.

Bison 2.3 (included in OSX) generates code with GPLv2 license which we legally 
are allowed to include in WebKit:

   This program is free software; you can redistribute it and/or modify
   it under the terms of the GNU General Public License as published by
   the Free Software Foundation; either version 2, or (at your option)
   any later version.

We are almost done making all build systems (Qt, Mac, EFL, GTK, and Win) run 
bison and flex as part of the build process so we don't have to include the 
generated files at all (glslang_tab.cpp, glslang_lex.cpp, glslang_tab.h, 
ExpressionParser.cpp, and Tokenizer.cpp).  This will make the process of 
merging ANGLE changes into WebKit painless, but we need this hack and only this 
hack to make glslang.y work.  It compiles successfully in all the 
aforementioned systems.

Original comment by achriste...@gmail.com on 30 Jul 2013 at 11:46

[GoogleCodeExporter]

[deleted comment]

[GoogleCodeExporter]

Alok should be consulted regarding this change.

The license in Bison 2.7's generated code is *not* the GPL v3 -- it is the GPL 
v3 with the Bison exception, and that exception applies to the usage of the 
code in WebKit. This difference is significant.

Original comment by kbr@chromium.org on 30 Jul 2013 at 11:49

[GoogleCodeExporter]

That may be true, but I'm still not allowed to put the generated files from 
ANGLE into WebKit.  The legal header in the generated files is enough to make 
all of us add an ANGLEGenerated build step to avoid it.  We'd really appreciate 
this change upstream so we don't have to maintain another change while updating.

Original comment by achriste...@gmail.com on 31 Jul 2013 at 12:07

[GoogleCodeExporter]

I am not too keen on having to maintain this hack. It is also difficult to 
guarantee that the grammar file will always be compatible with older versions 
of bison. I would understand if it were temporary situation, but it is not.

Is there a Bison 2.5 equivalent of "%code requires"?

Original comment by alokp@chromium.org on 31 Jul 2013 at 4:52

[GoogleCodeExporter]

I tried putting the definitions in the prologue just above them (which is 
similar to %code requires), but they were missing in one location. Maybe 
something similar to that could be done. I'm not a bison expert.

Original comment by achriste...@gmail.com on 31 Jul 2013 at 5:04

[GoogleCodeExporter]

(They were missing in one location in the generated files)

Original comment by achriste...@gmail.com on 31 Jul 2013 at 5:05

[GoogleCodeExporter]

Alok, do you see any reason why we even need a custom YYLTYPE at all? It's 
currently defined as TSourceLoc (in Common.h), and holds first/last file and 
first/last line locations. However, in YY_USER_ACTION (in glslang.l), the file 
variables gets initialized to the first/last column! This actually corresponds 
to the default YYLTYPE definition (in glslang_tab.h).

Original comment by nico...@transgaming.com on 31 Jul 2013 at 1:39

[GoogleCodeExporter]

Chiming into this, I've been having some trouble building ANGLE with Bison 3.0:

/b/webkit/efl/Debug/DerivedSources/WebCore/glslang_tab.cpp: In function 'int 
yyparse(TParseContext*)':
/b/webkit/efl/Debug/DerivedSources/WebCore/glslang_tab.cpp:1916:39: error: too 
few arguments to function 'int yylex(YYSTYPE*, TSourceLoc*, void*)'
/b/webkit/efl/Debug/DerivedSources/WebCore/glslang_tab.cpp:291:12: note: 
declared here
/b/webkit/efl/Debug/DerivedSources/WebCore/glslang_tab.cpp:1991:22: error: 
'YYID' was not declared in this scope
/b/webkit/efl/Debug/DerivedSources/WebCore/glslang_tab.cpp:4754:18: error: 
'YYID' was not declared in this scope

Let me know if you'd like to track this one separately.

Original comment by raphael.kubo.da.costa@intel.com on 13 Aug 2013 at 5:26

[GoogleCodeExporter]

@9: Yes we can do the same trick here by considering first/last column as 
first/last file. I will be happy to review a patch that does this cleanly.

@10: This seems like a different issue. Please file another bug.

Original comment by alokp@chromium.org on 13 Aug 2013 at 6:06

[GoogleCodeExporter]

> @10: This seems like a different issue. Please file another bug.

Looks like it's already covered in bug 462.

Original comment by raphael.kubo.da.costa@intel.com on 15 Aug 2013 at 11:12

[GoogleCodeExporter]

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

• Changed state: Accepted

[GoogleCodeExporter]

What about something like this?  This is a diff from git checkout 
047373aa3eb408be62be52ade840fa5f11e72337.  I added a typedef right after the 
TSourceLoc definition.  Since then someone has put another definition in the 
%code section, but that is a different problem.

diff --git a/src/compiler/Common.h b/src/compiler/Common.h
index 532486a..5fcea88 100644
--- a/src/compiler/Common.h
+++ b/src/compiler/Common.h
@@ -20,6 +20,8 @@ struct TSourceLoc {
     int last_file;
     int last_line;
 };
+typedef TSourceLoc YYLTYPE;
+#define YYLTYPE_IS_DECLARED 1

 //
 // Put POOL_ALLOCATOR_NEW_DELETE in base classes to make them use this scheme.
diff --git a/src/compiler/glslang.y b/src/compiler/glslang.y
index 0e8c3c3..1401aff 100644
--- a/src/compiler/glslang.y
+++ b/src/compiler/glslang.y
@@ -48,11 +48,6 @@ WHICH GENERATES THE GLSL ES PARSER (glslang_tab.cpp AND 
glslang_tab.h).
 %parse-param {TParseContext* context}
 %locations

-%code requires {
-#define YYLTYPE TSourceLoc
-#define YYLTYPE_IS_DECLARED 1
-}
-
 %union {
     struct {
         union {

Original comment by achriste...@gmail.com on 19 Aug 2013 at 8:53

[GoogleCodeExporter]

For review: https://codereview.appspot.com/13339044

I've read through the old and new Bison documentation and I think this might be 
the intended approach when '%code requires' is not available. Note that the 
includes of Common.h are not strictly necessary since it's already included by 
the headers above it, but this should be clearer.

Indeed defining YYLTYPE in Common.h would have worked too, but that would bring 
Bison constructs into the back end of the compiler.

Original comment by nico...@transgaming.com on 29 Aug 2013 at 3:46

• Changed state: Review

[GoogleCodeExporter]

LGTM.  Thanks

Original comment by achriste...@gmail.com on 30 Aug 2013 at 6:02

[GoogleCodeExporter]

Original comment by c...@chromium.org on 16 Nov 2013 at 12:35

[GoogleCodeExporter]

Would it be useful if I made a patch with the changes requested by the 
reviewers?  I'm not sure I have access to upload a patch.

Original comment by achriste...@gmail.com on 20 Nov 2013 at 4:34

[GoogleCodeExporter]

Our new Gerrit review system makes it much easier to accept and commit external 
patches, I've been told.

Anyway, is WebKit considering upgrading Bison, which would make this patch 
unnecessary, or did I pick that up incorrectly? I can go ahead with this patch 
quite quickly now if it's still required.

Original comment by c...@chromium.org on 20 Nov 2013 at 6:32

[GoogleCodeExporter]

I'm told that as of yesterday WebKit can accept code generated with newer 
versions of Bison, so this isn't an issue for WebKit now.  It might be an issue 
for someone else, but I doubt it.

Original comment by achriste...@gmail.com on 20 Nov 2013 at 8:40

[GoogleCodeExporter]

Abandoning this, as it's no longer an issue.

Original comment by c...@chromium.org on 3 Jan 2014 at 9:13

• Changed state: WontFix