Bug 89039 – Update ANGLE in WebKit

• Status: RESOLVED
• Resolution: FIXED
• Component: WebGL

[Max Vujovic]

For CSS Shaders, we need some new features in ANGLE. ANGLE is currently r1009 in WebKit (last updated in bug 81717). Let's update it to r1147.

[Max Vujovic]

Created attachment 147620 [details]
Patch

[Gustavo Noronha (kov)]

Comment on attachment 147620 [details]
Patch

Attachment 147620 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/12961268

[Early Warning System Bot]

Comment on attachment 147620 [details]
Patch

Attachment 147620 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/12961278

[Early Warning System Bot]

Comment on attachment 147620 [details]
Patch

Attachment 147620 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/12957421

[Max Vujovic]

Created attachment 147843 [details]
Patch

Running EWS again. Trying to fix warnings on GTK EWS from new ANGLE source.

[Early Warning System Bot]

Comment on attachment 147843 [details]
Patch

Attachment 147843 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/12965531

[Early Warning System Bot]

Comment on attachment 147843 [details]
Patch

Attachment 147843 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/12952974

[Max Vujovic]

Created attachment 147858 [details]
Patch

[Early Warning System Bot]

Comment on attachment 147858 [details]
Patch

Attachment 147858 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/12959681

[Early Warning System Bot]

Comment on attachment 147858 [details]
Patch

Attachment 147858 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/12970014

[Early Warning System Bot]

Comment on attachment 147858 [details]
Patch

Attachment 147858 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/12959698

[Max Vujovic]

Created attachment 147931 [details]
Patch

[Early Warning System Bot]

Comment on attachment 147931 [details]
Patch

Attachment 147931 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/12965645

[Early Warning System Bot]

Comment on attachment 147931 [details]
Patch

Attachment 147931 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/12970097

[Max Vujovic]

Created attachment 147934 [details]
Patch

[Early Warning System Bot]

Comment on attachment 147934 [details]
Patch

Attachment 147934 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/12970102

[Early Warning System Bot]

Comment on attachment 147934 [details]
Patch

Attachment 147934 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/12959775

[Gustavo Noronha (kov)]

Comment on attachment 147934 [details]
Patch

Attachment 147934 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/12956868

[Max Vujovic]

Created attachment 147977 [details]
Patch

[Early Warning System Bot]

Comment on attachment 147977 [details]
Patch

Attachment 147977 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/12964827

[Early Warning System Bot]

Comment on attachment 147977 [details]
Patch

Attachment 147977 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/12970262

[Early Warning System Bot]

Comment on attachment 147977 [details]
Patch

Attachment 147977 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/12965785

[Max Vujovic]

Created attachment 148375 [details]
Patch

[Early Warning System Bot]

Comment on attachment 148375 [details]
Patch

Attachment 148375 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/12972874

[Early Warning System Bot]

Comment on attachment 148375 [details]
Patch

Attachment 148375 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/12988176

[Max Vujovic]

Created attachment 148426 [details]
Patch

[Max Vujovic]

Created attachment 148608 [details]
Patch

[Max Vujovic]

Created attachment 149888 [details]
Patch

[Early Warning System Bot]

Comment on attachment 149888 [details]
Patch

Attachment 149888 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/13107074

[Max Vujovic]

Created attachment 149894 [details]
Patch

[Early Warning System Bot]

Comment on attachment 149894 [details]
Patch

Attachment 149894 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/13100965

[Early Warning System Bot]

Comment on attachment 149894 [details]
Patch

Attachment 149894 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/13099946

[Early Warning System Bot]

Comment on attachment 149894 [details]
Patch

Attachment 149894 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/13101987

[Max Vujovic]

Created attachment 149975 [details]
Patch

[Early Warning System Bot]

Comment on attachment 149975 [details]
Patch

Attachment 149975 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/13110126

[Max Vujovic]

Created attachment 149996 [details]
Patch

[Gustavo Noronha (kov)]

Comment on attachment 149996 [details]
Patch

Attachment 149996 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/13107209

[Max Vujovic]

Created attachment 150006 [details]
Patch

[Max Vujovic]

Created attachment 150022 [details]
Patch

[Max Vujovic]

Comment on attachment 150022 [details]
Patch

This patch is ready for review.

I'm updating ANGLE to r1170.

When trying to update to ANGLE r1147, I was getting build warnings on the QT and GTK bots. I fixed the warnings and pushed the fixes to ANGLE in r1170.

However, between r1147 and r1170, ANGLE enabled its new preprocessor. This caused a new build issue on QT, which I fixed in this patch by renaming two files. 

To avoid chasing a moving target, I plan on fixing this issue in ANGLE after this patch. The issue and fix is described the ChangeLog, pasted below:

"""
Update ANGLE to r1170, with the following modifications:

Rename ANGLE/src/compiler/preprocessor/new/Diagnostic.cpp to DiagnosticBase.cpp.
Rename ANGLE/src/compiler/preprocessor/new/DirectiveHandler.cpp to DirectiveHandlerBase.cpp.

With the introduction of ANGLE's new preprocessor, there were two files named Diagnostic.cpp in ANGLE under different folders. This caused problems on the QT build when their object files, both named Diagnostic.o, tried to go in the same folder. Renaming one of them to DirectiveHandlerBase.cpp avoids this conflict. The same situation occurred with DirectiveHandler.cpp. I will try to get their names changed in ANGLE for future updates.
"""

[Dean Jackson]

Comment on attachment 150022 [details]
Patch

rs=me. I remember when I did this I had to be careful choosing the version of Bison that processed the OS X port, but since the mac EWS is ok I'm crossing my fingers here.

[WebKit Review Bot]

Comment on attachment 150022 [details]
Patch

Rejecting attachment 150022 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
t/git/webkit-commit-queue/Source/WebKit/chromium/v8 --revision 11947 --non-interactive --force --accept theirs-conflict --ignore-externals' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium'
47>At revision 11947.

________ running '/usr/bin/python tools/clang/scripts/update.py --mac-only' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium'

________ running '/usr/bin/python gyp_webkit' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium'
Updating webkit projects from gyp files...

Full output: http://queues.webkit.org/results/13130003

[Max Vujovic]

(In reply to comment #41)
> (From update of attachment 150022 [details])
> rs=me. I remember when I did this I had to be careful choosing the version of Bison that processed the OS X port, but since the mac EWS is ok I'm crossing my fingers here.

Thanks, Dean! You used Bison 2.3 last time. I'm using Bison 2.4.2 this time. I haven't had any build or test issues yet.

However, it looks like the review bot didn't like the tabs in two new Bison generated files. I'll fix that up next week, and watch the build bots then. (I'm traveling with limited internet access this week).

[Max Vujovic]

Created attachment 151493 [details]
Patch

In this new patch, I resolved a conflict from bug 90506 in WebCore/Target.pri. Let's run the bots again and make sure it works.

[Max Vujovic]

Created attachment 151520 [details]
Patch

To pass the commit-queue, created an svn patch with the allow-tabs property set on the new files in the preprocessor/new folder. This property is already set on the existing files in ANGLE.

[Max Vujovic]

Created attachment 151538 [details]
Patch

Updated the ChangeLog date and Reviewed By in the patch for the commit.

[Alexandru Chiculita]

Comment on attachment 151538 [details]
Patch

Setting the cq+ flag, as the patch was just rebased since it was reviewed last time.

[Mark Rowe (bdash)]

Comment on attachment 151538 [details]
Patch

This contains code generated by Bison that is licensed under GPL v3. Please generate it with a version of Bison that uses a license that the WebKit project is ok with (e.g., Bison v2.3).

[Max Vujovic]

(In reply to comment #48)
> (From update of attachment 151538 [details])
> This contains code generated by Bison that is licensed under GPL v3. Please generate it with a version of Bison that uses a license that the WebKit project is ok with (e.g., Bison v2.3).

Sorry about that- didn't know about the licensing issue. ANGLE uses Bison 2.4.2 nowadays. I'll try to change this patch to use Bison 2.3 like before.

[Max Vujovic]

(In reply to comment #49)
> (In reply to comment #48)
> > (From update of attachment 151538 [details] [details])
> > This contains code generated by Bison that is licensed under GPL v3. Please generate it with a version of Bison that uses a license that the WebKit project is ok with (e.g., Bison v2.3).
> 
> Sorry about that- didn't know about the licensing issue. ANGLE uses Bison 2.4.2 nowadays. I'll try to change this patch to use Bison 2.3 like before.

Hi Mark,

I found out that the license on the Bison 2.4.2 generated files isn't actually GPL v3. It's GPL v3 with the Bison exception. The exception means that you can use Bison under the license of your choice as long as you aren't using it to make a parser generator. This means the Bison generated code in this patch can be GPL v2. Ken Russell pointed this out to me.

Here's the Bison exception, lifted from the License in the ExpressionParser.cpp, which was generated with Bison 2.4.2:
"""
/* As a special exception, you may create a larger work that contains
   part or all of the Bison parser skeleton and distribute that work
   under terms of your choice, so long as that work isn't itself a
   parser generator using the skeleton or a modified version thereof
   as a parser skeleton.  Alternatively, if you modify or redistribute
   the parser skeleton itself, you may (at your option) remove this
   special exception, which will cause the skeleton and the resulting
   Bison output files to be licensed under the GNU General Public
   License without this special exception.
   
   This special exception was added by the Free Software Foundation in
   version 2.2 of Bison.  */
"""

If that's ok with you, could you revert the r-?

Thank you,
Max

[Mark Rowe (bdash)]

It's still not ok.

[Kenneth Russell]

(In reply to comment #51)
> It's still not ok.

Mark, could you please describe more clearly why not? The code in question is *not* being licensed under GPLv3 -- it is being licensed under the WebKit project's terms, per the Bison exception clause.

In parallel, we'll look into using the older Bison directive for making a reentrant parser -- but I think it would be a mistake to artificially limit the version of Bison that can be used within WebKit.

[Mark Rowe (bdash)]

Let me start off by pointing out that I'm not a lawyer, and the reason for my objecting so strongly comes from having no desire to discuss this with lawyers when it's so trivial to avoid having a need to do so.  That said, the file quite clearly states at the top of it:

   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.

The fact that there is an exception allowing distribution under terms of our choice doesn't change this fact. 

On an unrelated note, it's not obvious to me why these generated files are checked in rather than being generated during the build process.

[Kenneth Russell]

(In reply to comment #53)
> Let me start off by pointing out that I'm not a lawyer, and the reason for my objecting so strongly comes from having no desire to discuss this with lawyers when it's so trivial to avoid having a need to do so.  That said, the file quite clearly states at the top of it:
> 
>    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.
> 
> The fact that there is an exception allowing distribution under terms of our choice doesn't change this fact. 

I'm not a lawyer either, and hate to point this out, but the current sources say the same thing, only with GPL v2 instead of v3. To the best of my knowledge the WebKit project is currently relying on the exception clause.

> On an unrelated note, it's not obvious to me why these generated files are checked in rather than being generated during the build process.

It makes things easier for developers consuming the ANGLE sources.

[Mark Rowe (bdash)]

(In reply to comment #54)
> (In reply to comment #53)
> > Let me start off by pointing out that I'm not a lawyer, and the reason for my objecting so strongly comes from having no desire to discuss this with lawyers when it's so trivial to avoid having a need to do so.  That said, the file quite clearly states at the top of it:
> > 
> >    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.
> > 
> > The fact that there is an exception allowing distribution under terms of our choice doesn't change this fact. 
> 
> I'm not a lawyer either, and hate to point this out, but the current sources say the same thing, only with GPL v2 instead of v3. To the best of my knowledge the WebKit project is currently relying on the exception clause.

That's fine. It's version three of the GPL that we want nothing to do with.

> 
> > On an unrelated note, it's not obvious to me why these generated files are checked in rather than being generated during the build process.
> 
> It makes things easier for developers consuming the ANGLE sources.

[Dean Jackson]

(In reply to comment #54)

> > On an unrelated note, it's not obvious to me why these generated files are checked in rather than being generated during the build process.
> 
> It makes things easier for developers consuming the ANGLE sources.

Outside of the resolution here, I think we should look into this. We use bison to process other grammars in the build process. I'm not sure ANGLE is that much more difficult.

The most annoying part will be setting this up for all builds.

[Joshua Netterfield]

> Source/ThirdParty/ANGLE/src/compiler/osinclude.h:-20
> -      defined(__GLIBC__) || defined(__GNU__) || defined(__QNX__)

QNX systems are POSIX. I'll submit a bug to ANGLE, but in the meantime could you continue to recognize QNX as POSIX?

[Max Vujovic]

(In reply to comment #56)
> (In reply to comment #54)
> 
> > > On an unrelated note, it's not obvious to me why these generated files are checked in rather than being generated during the build process.
> > 
> > It makes things easier for developers consuming the ANGLE sources.
> 
> Outside of the resolution here, I think we should look into this. We use bison to process other grammars in the build process. I'm not sure ANGLE is that much more difficult.
> 
> The most annoying part will be setting this up for all builds.

It would be nice to use Bison and Flex in the WebKit ANGLE build.

I've made the following umbrella bug:
Bug 91105 - [ANGLE] Use Bison and Flex during ANGLE build

And child bugs for each platform using ANGLE:
Bug 91107 - [ANGLE] On Mac, use Bison and Flex during ANGLE build
Bug 91108 - [ANGLE] On QT, use Bison and Flex during ANGLE build
Bug 91109 - [ANGLE] On GTK, use Bison and Flex during ANGLE build

[Max Vujovic]

(In reply to comment #57)
> > Source/ThirdParty/ANGLE/src/compiler/osinclude.h:-20
> > -      defined(__GLIBC__) || defined(__GNU__) || defined(__QNX__)
> 
> QNX systems are POSIX. I'll submit a bug to ANGLE, but in the meantime could you continue to recognize QNX as POSIX?

Sure, I can do that. Thank you for filing the bug on ANGLE.

[Max Vujovic]

Created attachment 152576 [details]
Patch

I've uploaded a new patch with the following changes:

(1) Use Bison 2.3 instead of Bison 2.4.2 to generate ExpressionParser.cpp and glslang_tab.cpp. I had to modify ExpressionParser.y to make it compatible with Bison 2.3. The changes have been contributed back to ANGLE in r1224.

(2) Continue to recognize QNX as POSIX in ANGLE. This has been contributed back to ANGLE in r1223.

These changes are also noted in the ChangeLog entries.

Mark, does this work for you?

[Mark Rowe (bdash)]

Comment on attachment 152576 [details]
Patch

Thanks for making those changes.

[Max Vujovic]

(In reply to comment #61)
> (From update of attachment 152576 [details])
> Thanks for making those changes.

No problem. Thanks for the review :)

[Early Warning System Bot]

Comment on attachment 152576 [details]
Patch

Attachment 152576 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/13245973

[Early Warning System Bot]

Comment on attachment 152576 [details]
Patch

Attachment 152576 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/13262067

[Early Warning System Bot]

Comment on attachment 152576 [details]
Patch

Attachment 152576 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/13256224

[Early Warning System Bot]

Comment on attachment 152576 [details]
Patch

Attachment 152576 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/13255224

[Max Vujovic]

Created attachment 152623 [details]
Patch

Patch to fix the QT EWS failure.

QT EWS fails on the Bison 2.3 generated files because two macros aren't defined on QT: YYENABLE_NLS and YYLTYPE_IS_TRIVIAL.

In CSSGrammar.y, we fix this Bison 2.3 issue by adding the lines:
#define YYENABLE_NLS 0
#define YYLTYPE_IS_TRIVIAL 1

I've added the same lines to ANGLE's grammar files (glslang.y and ExpressionParser.y) and ran Bison 2.3 again.

I'll work on contributing this fix back to ANGLE.

As a side note, Bison 2.4.2 fixes this issue by first checking if these macros are defined and then checking their value. Here's the original patch to Bison: http://lists.gnu.org/archive/html/grub-devel/2010-01/msg00340.html

[Max Vujovic]

Created attachment 152784 [details]
Patch

Added Dean and Mark as the reviewers in the ChangeLog for landing.

[Alexandru Chiculita]

Comment on attachment 152784 [details]
Patch

Trying the CQ again.

[WebKit Review Bot]

Comment on attachment 152784 [details]
Patch

Rejecting attachment 152784 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
queue/Source/WebKit/chromium/third_party/skia/include --revision 4600 --non-interactive --force --accept theirs-conflict --ignore-externals' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium'
44>At revision 4600.

________ running '/usr/bin/python tools/clang/scripts/update.py --mac-only' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium'

________ running '/usr/bin/python gyp_webkit' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium'
Updating webkit projects from gyp files...

Full output: http://queues.webkit.org/results/13278214

[Adam Barth]

    The following files contain tab characters:

        trunk/Source/ThirdParty/ANGLE/src/compiler/preprocessor/new/ExpressionParser.cpp
        trunk/Source/ThirdParty/ANGLE/src/compiler/preprocessor/new/Tokenizer.cpp

    Please use spaces instead to indent.
    If you must commit a file with tabs, use svn propset to set the "allow-tabs" property.
 at /usr/lib/git-core/git-svn line 570

[Max Vujovic]

(In reply to comment #71)
>     The following files contain tab characters:
> 
>         trunk/Source/ThirdParty/ANGLE/src/compiler/preprocessor/new/ExpressionParser.cpp
>         trunk/Source/ThirdParty/ANGLE/src/compiler/preprocessor/new/Tokenizer.cpp
> 
>     Please use spaces instead to indent.
>     If you must commit a file with tabs, use svn propset to set the "allow-tabs" property.
>  at /usr/lib/git-core/git-svn line 570

That's strange because I already set "allow-tabs" on those files in the patch. I'll investigate further. Thanks, Adam.

[Alexandru Chiculita]

svn-apply was ignoring the propsets for allow-tabs inside the patch, so I landed it manually on behalf of Max.

Manually committed r122870: http://trac.webkit.org/changeset/122870

[Daniel Bates]

(In reply to comment #73)
> svn-apply was ignoring the propsets for allow-tabs inside the patch, so I landed it manually on behalf of Max.
> 
> Manually committed r122870: http://trac.webkit.org/changeset/122870

Filed bug #91556 to teach svn-{apply, unapply} about the allow-tabs property.

[Peter Gal]

This patch resulted a build fail on the Qt linux debug bots.

http://build.webkit.sed.hu/builders/x86-64%20Linux%20Qt%20Debug/builds/23917

error msg:

../Source/ThirdParty/ANGLE/src/compiler/preprocessor/new/MacroExpander.cpp: In member function 'void pp::MacroExpander::ungetToken(const pp::Token&)':
.../Source/ThirdParty/ANGLE/src/compiler/preprocessor/new/MacroExpander.cpp:129: error: comparison of unsigned expression >= 0 is always true


it seems the problem is the 'assert(context->index >= 0);' line, the index is of type size_t which is unsigned . Maybe we can remove this line or something else?

[Kristóf Kosztyó]

(In reply to comment #75)
> This patch resulted a build fail on the Qt linux debug bots.
> 
> http://build.webkit.sed.hu/builders/x86-64%20Linux%20Qt%20Debug/builds/23917
> 
> error msg:
> 
> ../Source/ThirdParty/ANGLE/src/compiler/preprocessor/new/MacroExpander.cpp: In member function 'void pp::MacroExpander::ungetToken(const pp::Token&)':
> .../Source/ThirdParty/ANGLE/src/compiler/preprocessor/new/MacroExpander.cpp:129: error: comparison of unsigned expression >= 0 is always true
> 
> 
> it seems the problem is the 'assert(context->index >= 0);' line, the index is of type size_t which is unsigned . Maybe we can remove this line or something else?

I made this fix in r122925 (http://trac.webkit.org/changeset/122925)

[Dongseong Hwang]

*** Bug 91586 has been marked as a duplicate of this bug. ***

[Max Vujovic]

(In reply to comment #76)
> (In reply to comment #75)
> > This patch resulted a build fail on the Qt linux debug bots.
> > 
> > http://build.webkit.sed.hu/builders/x86-64%20Linux%20Qt%20Debug/builds/23917
> > 
> > error msg:
> > 
> > ../Source/ThirdParty/ANGLE/src/compiler/preprocessor/new/MacroExpander.cpp: In member function 'void pp::MacroExpander::ungetToken(const pp::Token&)':
> > .../Source/ThirdParty/ANGLE/src/compiler/preprocessor/new/MacroExpander.cpp:129: error: comparison of unsigned expression >= 0 is always true
> > 
> > 
> > it seems the problem is the 'assert(context->index >= 0);' line, the index is of type size_t which is unsigned . Maybe we can remove this line or something else?
> 
> I made this fix in r122925 (http://trac.webkit.org/changeset/122925)

Thank you for the build fix. I'll get this fixed in ANGLE. I've created an ANGLE bug for the issue:
http://code.google.com/p/angleproject/issues/detail?id=349