883478 - Update ANGLE to pull from 13-08-02.

Status: RESOLVED FIXED

(Core :: Graphics: CanvasWebGL, defect)

Description

[Kelsey Gilbert [:jgilbert]]

r2426 was tip as of Jun13.

Comment 1

[Kelsey Gilbert [:jgilbert]]

https://tbpl.mozilla.org/?tree=Try&rev=4248fb10e7f1

Comment 2

[Kelsey Gilbert [:jgilbert]]

Try to fix the GCC4.7 issue:
https://tbpl.mozilla.org/?tree=Try&rev=0d902187f521

Comment 3

[Kelsey Gilbert [:jgilbert]]

https://tbpl.mozilla.org/?tree=Try&showall=1&rev=227608824f78

Comment 4

[Kelsey Gilbert [:jgilbert]]

Attachment: patch: Use ANGLE's built-in MurmurHash3 instead of Spooky Hash for long ident hashing.

Comment 5

[Kelsey Gilbert [:jgilbert]]

(In reply to Jeff Gilbert [:jgilbert] from comment #4)
> Created attachment 766199 [details] [diff] [review]
> patch: Use ANGLE's built-in MurmurHash3 instead of Spooky Hash for long
> ident hashing.

ANGLE includes MurmurHash3 now for something or other. We should just use it instead of including our own hash, unless there's a good reason not to.

Comment 6

[Kelsey Gilbert [:jgilbert]]

Attachment: patch: Rename compiler/Uniform.(cpp|h) to compiler/CompilerUniform.(cpp|h)

There's another name collision in the new pull, so this fixes it. I used CamelCase instead of underscores, since the original file was capitalized.

Comment 7

[Kelsey Gilbert [:jgilbert]]

Attachment: patch: Work around a GCC4.7 issue where its implicit copy constructor generates type errors.

Comment 8

[Kelsey Gilbert [:jgilbert]]

Those are all the new patches I have so far.

Comment 9

[Kelsey Gilbert [:jgilbert]]

With the exception of things which don't look like my fault, the Try run is clean save for a crash in libGLESv2.dll!glBufferData during m1. This doesn't sound like anything I touched, but hopefully it's simple enough to fix.

Comment 10

[Kelsey Gilbert [:jgilbert]]

New try:
https://tbpl.mozilla.org/?tree=Try&showall=1&rev=3ae9274c1206

Includes fixes to work around ANGLE issue 438:
http://code.google.com/p/angleproject/issues/detail?id=438

Comment 11

[Benoit Jacob [:bjacob] (mostly away)]

Comment on attachment 766199 [details] [diff] [review]
patch: Use ANGLE's built-in MurmurHash3 instead of Spooky Hash for long ident hashing.

Review of attachment 766199 [details] [diff] [review]:
-----------------------------------------------------------------

r=me but IMPORTANT: remove the now-unused spooky.cpp !

::: gfx/angle/src/compiler/MapLongVariableNames.cpp
@@ +13,5 @@
>  TString mapLongName(size_t id, const TString& name, bool isGlobal)
>  {
>      ASSERT(name.size() > MAX_SHORTENED_IDENTIFIER_SIZE);
>      TStringStream stream;
> +    

Trailing \w

@@ +15,5 @@
>      ASSERT(name.size() > MAX_SHORTENED_IDENTIFIER_SIZE);
>      TStringStream stream;
> +    
> +    uint64_t hash[2] = {0, 0};
> +    MurmurHash3_x64_128(name.data(), name.length(), 0, (void*)hash);

You don't need to explicitly cast to void*, do you? If you had to, that would be a static_cast. C-style casts for pointers are a bit dangerous.

Comment 12

[Benoit Jacob [:bjacob] (mostly away)]

Comment on attachment 766200 [details] [diff] [review]
patch: Rename compiler/Uniform.(cpp|h) to compiler/CompilerUniform.(cpp|h)

Review of attachment 766200 [details] [diff] [review]:
-----------------------------------------------------------------

Sigh, I hope we get a build system that allows for multiple files with same leaf name, during my lifetime.

Comment 13

[Benoit Jacob [:bjacob] (mostly away)]

Comment on attachment 766201 [details] [diff] [review]
patch: Work around a GCC4.7 issue where its implicit copy constructor generates type errors.

I can't r+ this without a more specific explanation, sorry. What is that "type error"?

This is tricksy because this is a nontrivial class with many member, and although it does look like all of them are currently POD, that could change under our feet at anytime without us knowing. So it really is worth pursuing a more c++ orthodox approach.

Comment 14

[Kelsey Gilbert [:jgilbert]]

Alright. This Try run should exhibit the error:
https://tbpl.mozilla.org/?tree=Try&rev=e4914e78e1e4

Comment 15

[Kelsey Gilbert [:jgilbert]]

Huh, might be gone.
Let's try more completely:
https://tbpl.mozilla.org/?tree=Try&rev=a9a79a22c2ad

Comment 16

[Kelsey Gilbert [:jgilbert]]

Nope, maybe a later patch removes the error? Weird. Let's just remove that patch for now.
New Try run:
https://tbpl.mozilla.org/?tree=Try&rev=fb112545dedc

Comment 17

[Kelsey Gilbert [:jgilbert]]

Here's the error:
/usr/bin/ccache /tools/gcc-4.7.2-0moz1/bin/g++ -m32 -march=pentiumpro -o Tokenizer.o -c -I../../dist/stl_wrappers -I../../dist/system_wrappers -include /builds/slave/try-lx-d-000000000000000000000/build/config/gcc_hidden.h -DANGLE_DISABLE_TRACE -DANGLE_COMPILE_OPTIMIZATION_LEVEL=D3DCOMPILE_OPTIMIZATION_LEVEL1 -DCOMPILER_IMPLEMENTATION -DMOZ_GLUE_IN_PROGRAM -DMOZILLA_INTERNAL_API -DIMPL_LIBXUL  -DSTATIC_EXPORTABLE_JS_API -DNO_NSPR_10_SUPPORT -I/builds/slave/try-lx-d-000000000000000000000/build/gfx/angle/include -I/builds/slave/try-lx-d-000000000000000000000/build/gfx/angle/include/KHR -I/builds/slave/try-lx-d-000000000000000000000/build/gfx/angle/src -I/builds/slave/try-lx-d-000000000000000000000/build/gfx/angle -I. -I../../dist/include  -I/builds/slave/try-lx-d-000000000000000000000/build/obj-firefox/dist/include/nspr -I/builds/slave/try-lx-d-000000000000000000000/build/obj-firefox/dist/include/nss      -fPIC -Wall -Wpointer-arith -Woverloaded-virtual -Werror=return-type -Wtype-limits -Wempty-body -Wsign-compare -Wno-invalid-offsetof -Wcast-align -fno-exceptions -fno-strict-aliasing -fno-rtti -ffunction-sections -fdata-sections -fno-exceptions -std=gnu++0x -pthread -pipe -DDEBUG -D_DEBUG -DTRACING -g -Os -freorder-blocks -fno-omit-frame-pointer   -DMOZILLA_CLIENT -include ../../mozilla-config.h -MD -MP -MF .deps/Tokenizer.o.pp  /builds/slave/try-lx-d-000000000000000000000/build/gfx/angle/src/compiler/preprocessor/Tokenizer.cpp
BuiltInFunctionEmulator.cpp
/usr/bin/ccache /tools/gcc-4.7.2-0moz1/bin/g++ -m32 -march=pentiumpro -o BuiltInFunctionEmulator.o -c -I../../dist/stl_wrappers -I../../dist/system_wrappers -include /builds/slave/try-lx-d-000000000000000000000/build/config/gcc_hidden.h -DANGLE_DISABLE_TRACE -DANGLE_COMPILE_OPTIMIZATION_LEVEL=D3DCOMPILE_OPTIMIZATION_LEVEL1 -DCOMPILER_IMPLEMENTATION -DMOZ_GLUE_IN_PROGRAM -DMOZILLA_INTERNAL_API -DIMPL_LIBXUL  -DSTATIC_EXPORTABLE_JS_API -DNO_NSPR_10_SUPPORT -I/builds/slave/try-lx-d-000000000000000000000/build/gfx/angle/include -I/builds/slave/try-lx-d-000000000000000000000/build/gfx/angle/include/KHR -I/builds/slave/try-lx-d-000000000000000000000/build/gfx/angle/src -I/builds/slave/try-lx-d-000000000000000000000/build/gfx/angle -I. -I../../dist/include  -I/builds/slave/try-lx-d-000000000000000000000/build/obj-firefox/dist/include/nspr -I/builds/slave/try-lx-d-000000000000000000000/build/obj-firefox/dist/include/nss      -fPIC -Wall -Wpointer-arith -Woverloaded-virtual -Werror=return-type -Wtype-limits -Wempty-body -Wsign-compare -Wno-invalid-offsetof -Wcast-align -fno-exceptions -fno-strict-aliasing -fno-rtti -ffunction-sections -fdata-sections -fno-exceptions -std=gnu++0x -pthread -pipe -DDEBUG -D_DEBUG -DTRACING -g -Os -freorder-blocks -fno-omit-frame-pointer   -DMOZILLA_CLIENT -include ../../mozilla-config.h -MD -MP -MF .deps/BuiltInFunctionEmulator.o.pp  /builds/slave/try-lx-d-000000000000000000000/build/gfx/angle/src/compiler/BuiltInFunctionEmulator.cpp
Compiler.cpp
/usr/bin/ccache /tools/gcc-4.7.2-0moz1/bin/g++ -m32 -march=pentiumpro -o Compiler.o -c -I../../dist/stl_wrappers -I../../dist/system_wrappers -include /builds/slave/try-lx-d-000000000000000000000/build/config/gcc_hidden.h -DANGLE_DISABLE_TRACE -DANGLE_COMPILE_OPTIMIZATION_LEVEL=D3DCOMPILE_OPTIMIZATION_LEVEL1 -DCOMPILER_IMPLEMENTATION -DMOZ_GLUE_IN_PROGRAM -DMOZILLA_INTERNAL_API -DIMPL_LIBXUL  -DSTATIC_EXPORTABLE_JS_API -DNO_NSPR_10_SUPPORT -I/builds/slave/try-lx-d-000000000000000000000/build/gfx/angle/include -I/builds/slave/try-lx-d-000000000000000000000/build/gfx/angle/include/KHR -I/builds/slave/try-lx-d-000000000000000000000/build/gfx/angle/src -I/builds/slave/try-lx-d-000000000000000000000/build/gfx/angle -I. -I../../dist/include  -I/builds/slave/try-lx-d-000000000000000000000/build/obj-firefox/dist/include/nspr -I/builds/slave/try-lx-d-000000000000000000000/build/obj-firefox/dist/include/nss      -fPIC -Wall -Wpointer-arith -Woverloaded-virtual -Werror=return-type -Wtype-limits -Wempty-body -Wsign-compare -Wno-invalid-offsetof -Wcast-align -fno-exceptions -fno-strict-aliasing -fno-rtti -ffunction-sections -fdata-sections -fno-exceptions -std=gnu++0x -pthread -pipe -DDEBUG -D_DEBUG -DTRACING -g -Os -freorder-blocks -fno-omit-frame-pointer   -DMOZILLA_CLIENT -include ../../mozilla-config.h -MD -MP -MF .deps/Compiler.o.pp  /builds/slave/try-lx-d-000000000000000000000/build/gfx/angle/src/compiler/Compiler.cpp
compiler_debug.cpp
/usr/bin/ccache /tools/gcc-4.7.2-0moz1/bin/g++ -m32 -march=pentiumpro -o compiler_debug.o -c -I../../dist/stl_wrappers -I../../dist/system_wrappers -include /builds/slave/try-lx-d-000000000000000000000/build/config/gcc_hidden.h -DANGLE_DISABLE_TRACE -DANGLE_COMPILE_OPTIMIZATION_LEVEL=D3DCOMPILE_OPTIMIZATION_LEVEL1 -DCOMPILER_IMPLEMENTATION -DMOZ_GLUE_IN_PROGRAM -DMOZILLA_INTERNAL_API -DIMPL_LIBXUL  -DSTATIC_EXPORTABLE_JS_API -DNO_NSPR_10_SUPPORT -I/builds/slave/try-lx-d-000000000000000000000/build/gfx/angle/include -I/builds/slave/try-lx-d-000000000000000000000/build/gfx/angle/include/KHR -I/builds/slave/try-lx-d-000000000000000000000/build/gfx/angle/src -I/builds/slave/try-lx-d-000000000000000000000/build/gfx/angle -I. -I../../dist/include  -I/builds/slave/try-lx-d-000000000000000000000/build/obj-firefox/dist/include/nspr -I/builds/slave/try-lx-d-000000000000000000000/build/obj-firefox/dist/include/nss      -fPIC -Wall -Wpointer-arith -Woverloaded-virtual -Werror=return-type -Wtype-limits -Wempty-body -Wsign-compare -Wno-invalid-offsetof -Wcast-align -fno-exceptions -fno-strict-aliasing -fno-rtti -ffunction-sections -fdata-sections -fno-exceptions -std=gnu++0x -pthread -pipe -DDEBUG -D_DEBUG -DTRACING -g -Os -freorder-blocks -fno-omit-frame-pointer   -DMOZILLA_CLIENT -include ../../mozilla-config.h -MD -MP -MF .deps/compiler_debug.o.pp  /builds/slave/try-lx-d-000000000000000000000/build/gfx/angle/src/compiler/compiler_debug.cpp
DetectCallDepth.cpp
In file included from ../../../gfx/angle/src/compiler/intermediate.h:23:0,
                 from ../../../gfx/angle/src/compiler/BuiltInFunctionEmulator.h:13,
                 from ../../../gfx/angle/src/compiler/BuiltInFunctionEmulator.cpp:7:
../../../gfx/angle/src/compiler/Types.h: In constructor 'TType::TType(TType&&)':
../../../gfx/angle/src/compiler/Types.h:93:7: error: invalid conversion from 'unsigned char:6' to 'TBasicType' [-fpermissive]
../../../gfx/angle/src/compiler/Types.h:93:7: error: invalid conversion from 'unsigned char:7' to 'TQualifier' [-fpermissive]
In file included from ../../../gfx/angle/src/compiler/BuiltInFunctionEmulator.cpp:9:0:
../../../gfx/angle/src/compiler/SymbolTable.h: In constructor 'TFunction::TFunction(TOperator)':
../../../gfx/angle/src/compiler/SymbolTable.h:135:22: note: synthesized method 'TType::TType(TType&&)' first required here 
make[6]: *** [BuiltInFunctionEmulator.o] Error 1
make[6]: *** Waiting for unfinished jobs....

From:
https://tbpl.mozilla.org/php/getParsedLog.php?id=26189041&tree=Try#error0

Comment 18

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 766201 [details] [diff] [review]
patch: Work around a GCC4.7 issue where its implicit copy constructor generates type errors.

I posted the error that this patch fixes in the previous comment.
I also see that this uses tabs, so I need to fix that.

Comment 19

[Kelsey Gilbert [:jgilbert]]

Attachment: patch: Use D3DPOOL_DEFAULT instead of _MANAGED

This was the XP bug. (really D3D9 non-Ex)
Pulling in review from Bas since this is D3D stuff.
Basically, on D3D9, ANGLE was trying to use POOL_MANAGED where it could get away with it, but uses POOL_DEFAULT everywhere on D3D9Ex and above.

Comment 20

[Kelsey Gilbert [:jgilbert]]

Here's the most recent Try build, where I forgot to reinclude the TType GCC compiler workaround:
https://tbpl.mozilla.org/?tree=Try&rev=2ff687141649

Looking at this patch again, I don't love it, but do you have any suggestions?

Comment 21

[Bas Schouten (:bas.schouten)]

Comment on attachment 789174 [details] [diff] [review]
patch: Use D3DPOOL_DEFAULT instead of _MANAGED

Review of attachment 789174 [details] [diff] [review]:
-----------------------------------------------------------------

Will all the surrounding code deal with this? We have to realize D3DPOOL_DEFAULT textures do -not- automatically come back when you have a system reset and can't be mapped directly. Does ANGLE deal with this correctly when -not- using a D3D9Ex device?

Comment 22

[Kelsey Gilbert [:jgilbert]]

(In reply to Bas Schouten (:bas.schouten) from comment #21)
> Comment on attachment 789174 [details] [diff] [review]
> patch: Use D3DPOOL_DEFAULT instead of _MANAGED
> 
> Review of attachment 789174 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Will all the surrounding code deal with this? We have to realize
> D3DPOOL_DEFAULT textures do -not- automatically come back when you have a
> system reset and can't be mapped directly. Does ANGLE deal with this
> correctly when -not- using a D3D9Ex device?

We shouldn't be concerned about system reset, as in GL land, everything is considered lost after such an event. I don't think that we map them directly, since that should be a d3d9-non-ex perf improvement (not a goal), given that in d3d9ex+, we use POOL_DEFAULT.

Comment 23

[Benoit Jacob [:bjacob] (mostly away)]

Comment on attachment 766201 [details] [diff] [review]
patch: Work around a GCC4.7 issue where its implicit copy constructor generates type errors.

Review of attachment 766201 [details] [diff] [review]:
-----------------------------------------------------------------

Ouch... I'm afraid that my concerns expressed in comment 13 still stand. Even if this happens to be OK in the current state of the code (if none of the members of this class have nontrivial ctors) this is very prone to silently breaking at the next ANGLE update. What were the compiler errors, again, and what else could be tried?

Comment 24

[Benoit Jacob [:bjacob] (mostly away)]

Comment on attachment 789174 [details] [diff] [review]
patch: Use D3DPOOL_DEFAULT instead of _MANAGED

Review of attachment 789174 [details] [diff] [review]:
-----------------------------------------------------------------

This is solely for Bas to review, and he already r+'d. I don't have the D3D knowledge.

Comment 25

[Kelsey Gilbert [:jgilbert]]

(In reply to Benoit Jacob [:bjacob] from comment #23)
> Comment on attachment 766201 [details] [diff] [review]
> patch: Work around a GCC4.7 issue where its implicit copy constructor
> generates type errors.
> 
> Review of attachment 766201 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Ouch... I'm afraid that my concerns expressed in comment 13 still stand.
> Even if this happens to be OK in the current state of the code (if none of
> the members of this class have nontrivial ctors) this is very prone to
> silently breaking at the next ANGLE update. What were the compiler errors,
> again, and what else could be tried?

The error is in Comment 17.

Comment 26

[Kelsey Gilbert [:jgilbert]]

Attachment: patch: Build system changes

Comment 27

[Kelsey Gilbert [:jgilbert]]

Attachment: patch: Remove bitfield decls from class TType to work around GCC4.7 bug

Comment 28

[Benoit Jacob [:bjacob] (mostly away)]

Comment on attachment 800953 [details] [diff] [review]
patch: Remove bitfield decls from class TType to work around GCC4.7 bug

Review of attachment 800953 [details] [diff] [review]:
-----------------------------------------------------------------

I thought that we had agreed that trying to remove the bitfields was dangerous as we didn't know whether ANGLE was making unsafe sizeof assumptions, and we wouldn't necessarily know if things went wrong. Also, if we do that, we have to worry about memory usage increases (there could be many 'TType's in a compiler...)

I thought that we'd just go the simpler route of doing your earlier fix but #ifdef GCC4.7 ?

::: gfx/angle/src/compiler/Types.h
@@ +160,3 @@
>      void setMatrix(bool m) { matrix = m; }
>  
> +    bool isArray() const  { return array; }

No cosmetic changes here please.

Comment 29

[Kelsey Gilbert [:jgilbert]]

(In reply to Benoit Jacob [:bjacob] from comment #28)
> Comment on attachment 800953 [details] [diff] [review]
> patch: Remove bitfield decls from class TType to work around GCC4.7 bug
> 
> Review of attachment 800953 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I thought that we had agreed that trying to remove the bitfields was
> dangerous as we didn't know whether ANGLE was making unsafe sizeof
> assumptions, and we wouldn't necessarily know if things went wrong. Also, if
> we do that, we have to worry about memory usage increases (there could be
> many 'TType's in a compiler...)
> 
> I thought that we'd just go the simpler route of doing your earlier fix but
> #ifdef GCC4.7 ?
This is the path that's being taken in the ANGLE issue:
http://code.google.com/p/angleproject/issues/detail?id=448&q=bit%20field
> 
> ::: gfx/angle/src/compiler/Types.h
> @@ +160,3 @@
> >      void setMatrix(bool m) { matrix = m; }
> >  
> > +    bool isArray() const  { return array; }
> 
> No cosmetic changes here please.
Alright.

Comment 30

[Benoit Jacob [:bjacob] (mostly away)]

Comment on attachment 800953 [details] [diff] [review]
patch: Remove bitfield decls from class TType to work around GCC4.7 bug

Review of attachment 800953 [details] [diff] [review]:
-----------------------------------------------------------------

Alright, didn't realize that this came from an upstream conversation. R+ to anything that does.

Comment 31

[Kelsey Gilbert [:jgilbert]]

Attachment: patch: Remove bitfield decls from class TType to work around GCC4.7 bug

Comment 32

[Kelsey Gilbert [:jgilbert]]

https://tbpl.mozilla.org/?tree=Try&rev=b8ac24a22ea3

Comment 33

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 800995 [details] [diff] [review]
patch: Remove bitfield decls from class TType to work around GCC4.7 bug

Carrying forward r+ from bjacob.

Comment 34

[Kelsey Gilbert [:jgilbert]]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/356866ae2f68

Comment 35

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/356866ae2f68

Comment 36

[Bob Clary [:bc] (inactive)]

I started getting build failures on Windows 7 32bit for Nightly (but not Beta or Aurora) with:

c:\work\mozilla\builds\nightly\mozilla\gfx\angle\src\libGLESv2\precompiled.h(38) : fatal error C1083: Cannot open include file: 'd3dcompiler.h': No such file or directory. 

I have  Mozilla Build 1.8.0,  June 2010 DirectX SDK installed and have D3Dcompiler.h in 
C:\Program Files\Microsoft DirectX SDK (June 2010)\Include\D3Dcompiler.h

I tried with the make.py client.mk build and with mach ./build with the same results.

Comment 37

[Kelsey Gilbert [:jgilbert]]

(In reply to Bob Clary [:bc:] from comment #36)
> I started getting build failures on Windows 7 32bit for Nightly (but not
> Beta or Aurora) with:
> 
> c:\work\mozilla\builds\nightly\mozilla\gfx\angle\src\libGLESv2\precompiled.
> h(38) : fatal error C1083: Cannot open include file: 'd3dcompiler.h': No
> such file or directory. 
> 
> I have  Mozilla Build 1.8.0,  June 2010 DirectX SDK installed and have
> D3Dcompiler.h in 
> C:\Program Files\Microsoft DirectX SDK (June 2010)\Include\D3Dcompiler.h
> 
> I tried with the make.py client.mk build and with mach ./build with the same
> results.

Does replacing 'd3dcompiler.h' with 'D3Dcompiler.h' do anything? We changed this a while back, but it looks like June 2010 is actually D3Dcompiler.h, not d3dcompiler.h that we switched to. (though it looks like the Include folder is full of random cases) It should work anyways, since windows should be case-insensitive, but maybe it's worth a try?

Comment 38

[Kelsey Gilbert [:jgilbert]]

Actually, let's continue this discussion in bug 914547.