1272647 - Invalid operands error in gfx/graphite2/src/inc/UtfCodec.h

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[u403884]

Attachment: firefox-45.0.1-UtfCodec_h-plusplus-operator.patch

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Firefox/45.0
Build ID: 20160317220330

Steps to reproduce:

Try to rebuild firefox 45.0.1 using gcc6 (6.1.0) instead of gcc5


Actual results:

Build failure while trying to compile gfx/graphite2/src/Unified_cpp_gfx_graphite2_src0.cpp:

make[3]: ingresso nella directory "/home/guido/new/build-firefox-45.0.1/gfx/graphite2/src"
mkdir -p '.deps/'
Unified_cpp_gfx_graphite2_src0.o
c++ -o Unified_cpp_gfx_graphite2_src0.o -c -I/home/guido/new/build-firefox-45.0.1/dist/stl_wrappers -I/home/guido/new/build-firefox-45.0.1/dist/system_wrappers -include /home/guido/new/firefox-45.0.1/config/gcc_hidden.h -DGRAPHITE2_STATIC -DPACKAGE_VERSION='"moz"' -DPACKAGE_BUGREPORT='"http://bugzilla.mozilla.org/"' -DGRAPHITE2_NFILEFACE -DGRAPHITE2_NTRACING -DGRAPHITE2_NSEGCACHE -DGRAPHITE2_CUSTOM_HEADER='"MozGrMalloc.h"' -I/home/guido/new/firefox-45.0.1/gfx/graphite2/src -I.  -I../../../dist/include  -I/usr/include/nspr -I/usr/include/nss3    -I/usr/include/pixman-1   -fPIC  -DMOZILLA_CLIENT -include ../../../mozilla-config.h -MD -MP -MF .deps/Unified_cpp_gfx_graphite2_src0.o.pp  -Wall -Wempty-body -Woverloaded-virtual -Wsign-compare -Wwrite-strings -Wno-invalid-offsetof -Wcast-align -O2 -march=core2 -mtune=core2 -fno-exceptions -fno-strict-aliasing -fno-rtti -ffunction-sections -fdata-sections -fno-exceptions -fno-math-errno -std=gnu++0x -pthread -pipe  -DDEBUG -DTRACING -g -freorder-blocks -Os -fno-omit-frame-pointer     /home/guido/new/build-firefox-45.0.1/gfx/graphite2/src/Unified_cpp_gfx_graphite2_src0.cpp
In file included from /home/guido/new/build-firefox-45.0.1/gfx/graphite2/src/Unified_cpp_gfx_graphite2_src0.cpp:29:0:
/home/guido/new/firefox-45.0.1/gfx/graphite2/src/Code.cpp:85:12: warning: ‘graphite2::vm::Machine::Code::decoder::analysis’ has a field ‘graphite2::vm::Machine::Code::decoder::analysis::contexts’ whose type uses the anonymous namespace [-Wsubobject-linkage]
     struct analysis
            ^~~~~~~~
In file included from /home/guido/new/firefox-45.0.1/gfx/graphite2/src/NameTable.cpp:31:0,
                 from /home/guido/new/build-firefox-45.0.1/gfx/graphite2/src/Unified_cpp_gfx_graphite2_src0.cpp:128:
/home/guido/new/firefox-45.0.1/gfx/graphite2/src/inc/UtfCodec.h: In instantiation of ‘graphite2::_utf_iterator<C>& graphite2::_utf_iterator<C>::operator++() [with C = const short unsigned int]’:
/home/guido/new/firefox-45.0.1/gfx/graphite2/src/NameTable.cpp:172:90:   required from here
/home/guido/new/firefox-45.0.1/gfx/graphite2/src/inc/UtfCodec.h:182:46: error: invalid operands of types ‘const short unsigned int*’ and ‘__gnu_cxx::__enable_if<true, double>::__type {aka double}’ to binary ‘operator+’
     _utf_iterator   & operator ++ ()    { cp += abs(sl); return *this; }
                                           ~~~^~~~~~~~~~
/home/guido/new/firefox-45.0.1/gfx/graphite2/src/inc/UtfCodec.h:182:46: error:   in evaluation of ‘operator+=(const short unsigned int*, __gnu_cxx::__enable_if<true, double>::__type {aka double})’
/home/guido/new/firefox-45.0.1/gfx/graphite2/src/inc/UtfCodec.h: In instantiation of ‘graphite2::_utf_iterator<C>& graphite2::_utf_iterator<C>::operator++() [with C = unsigned char]’:
/home/guido/new/firefox-45.0.1/gfx/graphite2/src/NameTable.cpp:172:95:   required from here
/home/guido/new/firefox-45.0.1/gfx/graphite2/src/inc/UtfCodec.h:182:46: error: invalid operands of types ‘unsigned char*’ and ‘__gnu_cxx::__enable_if<true, double>::__type {aka double}’ to binary ‘operator+’
/home/guido/new/firefox-45.0.1/gfx/graphite2/src/inc/UtfCodec.h:182:46: error:   in evaluation of ‘operator+=(unsigned char*, __gnu_cxx::__enable_if<true, double>::__type {aka double})’
/home/guido/new/firefox-45.0.1/gfx/graphite2/src/inc/UtfCodec.h: In instantiation of ‘graphite2::_utf_iterator<C>& graphite2::_utf_iterator<C>::operator++() [with C = unsigned int]’:
/home/guido/new/firefox-45.0.1/gfx/graphite2/src/NameTable.cpp:193:95:   required from here
/home/guido/new/firefox-45.0.1/gfx/graphite2/src/inc/UtfCodec.h:182:46: error: invalid operands of types ‘unsigned int*’ and ‘__gnu_cxx::__enable_if<true, double>::__type {aka double}’ to binary ‘operator+’
/home/guido/new/firefox-45.0.1/gfx/graphite2/src/inc/UtfCodec.h:182:46: error:   in evaluation of ‘operator+=(unsigned int*, __gnu_cxx::__enable_if<true, double>::__type {aka double})’
/home/guido/new/firefox-45.0.1/config/rules.mk:956: set di istruzioni per l'obiettivo "Unified_cpp_gfx_graphite2_src0.o" non riuscito
make[3]: *** [Unified_cpp_gfx_graphite2_src0.o] Errore 1
make[3]: uscita dalla directory "/home/guido/new/build-firefox-45.0.1/gfx/graphite2/src"
/home/guido/new/firefox-45.0.1/config/recurse.mk:71: set di istruzioni per l'obiettivo "gfx/graphite2/src/target" non riuscito
make[2]: *** [gfx/graphite2/src/target] Errore 2


Expected results:

The build should not have failed while trying to compile gfx/graphite2/src/Unified_cpp_gfx_graphite2_src0.cpp

Comment 1

[Jonathan Kew [:jfkthame]]

The problem seems to be that abs(int8) is resolving to a function that returns double. So if a code patch is needed here, it should be a simple cast or similar to ensure we get an integer function rather than a floating-point one here. I'd guess the more appropriate patch is something like

-    _utf_iterator   & operator ++ ()    { cp += abs(sl); return *this; }
+    _utf_iterator   & operator ++ ()    { cp += abs(int(sl)); return *this; }


Does the same problem occur if you compile the graphite2 library by itself, directly from the upstream,[1] or is it somehow related to how it's built within gecko?

If the problem occurs with a standalone graphite build, please report it upstream.


[1] https://github.com/silnrsi/graphite/

Comment 2

[u403884]

Yes, please commit the resolution in Comment 1 and close this bug as FIXED.

Comment 3

[Jonathan Kew [:jfkthame]]

We prefer not to patch graphite locally; it's better to fix issues in the upstream source, unless they're specific to the mozilla build and not appropriate for upstreaming. Hence my question:

> Does the same problem occur if you compile the graphite2 library by itself, directly from the
> upstream,[1] or is it somehow related to how it's built within gecko?

Comment 4

[u403884]

Patch locally. Issue does not show up when building upstream source.

Comment 5

[Bas Schouten (:bas.schouten)]

Comment on attachment 8752179 [details] [diff] [review]
firefox-45.0.1-UtfCodec_h-plusplus-operator.patch

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

Please wait for jfkthame before landing this. I don't feel entirely qualified to review this code.

Comment 6

[Jonathan Kew [:jfkthame]]

Comment on attachment 8752179 [details] [diff] [review]
firefox-45.0.1-UtfCodec_h-plusplus-operator.patch

Definitely not this form of the patch. Comment 1 shows a better alternative. However, given comment 4, this seems to be specific to our build, and as such I'd prefer to avoid a code patch and fix our build setup instead.

I'm guessing that the problem arises because of unified compilation: earlier files in graphite have done #include <cmath>, which pulls in the abs() overload that's being chosen here and resulting in the error. NameTable.cpp by itself doesn't have that #include, and so in a standalone graphite build it compiles fine.

If that's correct, then we can solve this by taking NameTable.cpp out of UNIFIED_SOURCES, and avoid patching the code (and having to maintain that change every time we update).

Comment 7

[Jonathan Kew [:jfkthame]]

Attachment: Exclude NameTable.cpp from unified compilation because #include <cmath> in other source files causes gcc6 compilation failure

Speculative build-system patch to avoid gcc6 compilation error in graphite.

Comment 8

[Jonathan Kew [:jfkthame]]

g.trentalancia, could you please try the patch in attachment 8753749 [details] [diff] [review], and confirm whether this solves the problem you're seeing? Thanks.

Comment 9

[u403884]

Yes, it fixes the problem in a partial build.

Unfortunately, full builds are blocked most of the time by Bug 1068209.

Anyway, it's better to avoid the initial temporary patch I submitted and use this latest one you created as it is optimal for all the reasons you mentioned.

Comment 10

[Bas Schouten (:bas.schouten)]

Comment on attachment 8753749 [details] [diff] [review]
Exclude NameTable.cpp from unified compilation because #include <cmath> in other source files causes gcc6 compilation failure

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

Thanks for looking at this! This seems like a fine solution to me as well. Less ugly in any case.

Comment 11

[Jonathan Kew [:jfkthame]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9c57b7cacffc57ec3919a2cf3b5ce861e0cf3842
Bug 1272647 - Exclude NameTable.cpp from unified compilation because #include <cmath> in other source files causes gcc6 compilation failure. r=bas

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/9c57b7cacffc