Closed Bug 1272647 Opened 3 years ago Closed 3 years ago

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

Categories

(Core :: Graphics, defect)

45 Branch
All
Unspecified
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla49
Tracking Status
firefox49 --- fixed

People

(Reporter: u403884, Assigned: jfkthame)

Details

(Whiteboard: gfx-noted)

Attachments

(2 files)

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
Hardware: Unspecified → All
Component: Untriaged → Graphics
Product: Firefox → Core
Attachment #8752179 - Flags: review?(bas)
Whiteboard: gfx-noted
Attachment #8752179 - Flags: review?(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/
Yes, please commit the resolution in Comment 1 and close this bug as FIXED.
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?
Flags: needinfo?(g.trentalancia)
Patch locally. Issue does not show up when building upstream source.
Flags: needinfo?(g.trentalancia)
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.
Attachment #8752179 - Flags: review?(bas) → review+
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).
Attachment #8752179 - Flags: review?(jfkthame) → review-
Speculative build-system patch to avoid gcc6 compilation error in graphite.
Assignee: nobody → jfkthame
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
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.
Flags: needinfo?(g.trentalancia)
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.
Flags: needinfo?(g.trentalancia)
Attachment #8753749 - Flags: review?(bas)
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.
Attachment #8753749 - Flags: review?(bas) → review+
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
https://hg.mozilla.org/mozilla-central/rev/9c57b7cacffc
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
You need to log in before you can comment on or make changes to this bug.