Closed Bug 1472569 Opened 6 years ago Closed 6 years ago

Recent change in js/ fails on gcc 8

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

VERIFIED FIXED
mozilla63
Tracking Status
firefox63 --- fixed

People

(Reporter: Sylvestre, Assigned: Waldo)

References

Details

Attachments

(1 file)

/usr/bin/g++-8 -o RegExp.o -c -I/root/firefox-gcc-last/obj-x86_64-pc-linux-gnu/dist/system_wrappers -include /root/firefox-gcc-last/config/gcc_hidden.h -DDEBUG=1 -DENABLE_BINARYDATA -DENABLE_SIMD -DENABLE_WASM_BULKMEM_OPS -DENABLE_WASM_SATURATING_TRUNC_OPS -DENABLE_WASM_THREAD_OPS -DENABLE_WASM_GC -DWASM_HUGE_MEMORY -DJS_CACHEIR_SPEW -DENABLE_SHARED_ARRAY_BUFFER -DEXPORT_JS_API -DJS_HAS_CTYPES '-DDLL_PREFIX="lib"' '-DDLL_SUFFIX=".so"' -DFFI_BUILDING -DMOZ_HAS_MOZGLUE -I/root/firefox-gcc-last/js/src -I/root/firefox-gcc-last/obj-x86_64-pc-linux-gnu/js/src -I/root/firefox-gcc-last/obj-x86_64-pc-linux-gnu/js/src/ctypes/libffi/include -I/root/firefox-gcc-last/js/src/ctypes/libffi/src/x86 -I/root/firefox-gcc-last/obj-x86_64-pc-linux-gnu/dist/include -I/root/firefox-gcc-last/obj-x86_64-pc-linux-gnu/dist/include/nspr -fPIC -DMOZILLA_CLIENT -include /root/firefox-gcc-last/obj-x86_64-pc-linux-gnu/js/src/js-confdefs.h -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2 -Wall -Wempty-body -Wignored-qualifiers -Woverloaded-virtual -Wpointer-arith -Wsign-compare -Wtype-limits -Wunreachable-code -Wwrite-strings -Wno-invalid-offsetof -Wc++1z-compat -Wduplicated-cond -Wimplicit-fallthrough -Wno-error=maybe-uninitialized -Wno-error=deprecated-declarations -Wno-error=array-bounds -Wno-error=free-nonheap-object -Wno-error=multistatement-macros -Wno-error=class-memaccess -Wformat -Wformat-overflow=2 -Wno-noexcept-type -fno-sized-deallocation -Wno-error=class-memaccess -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2 -fno-rtti -fno-exceptions -fno-math-errno -pthread -pipe -g -freorder-blocks -O3 -fno-omit-frame-pointer -Werror -Wno-shadow -Werror=format -fno-strict-aliasing  -MD -MP -MF .deps/RegExp.o.pp   /root/firefox-gcc-last/js/src/builtin/RegExp.cpp


In file included from /root/firefox-gcc-last/obj-x86_64-pc-linux-gnu/dist/include/mozilla/Atomics.h:19,
                 from /root/firefox-gcc-last/js/src/jsfriendapi.h:10,
                 from /root/firefox-gcc-last/js/src/gc/Tracer.h:10,
                 from /root/firefox-gcc-last/js/src/vm/TaggedProto.h:10,
                 from /root/firefox-gcc-last/js/src/gc/Marking.h:16,
                 from /root/firefox-gcc-last/js/src/vm/RegExpObject.h:16,
                 from /root/firefox-gcc-last/js/src/builtin/RegExp.h:10,
                 from /root/firefox-gcc-last/js/src/builtin/RegExp.cpp:7:
/root/firefox-gcc-last/js/src/frontend/TokenStream.h: In member function 'bool js::frontend::TokenStreamChars<char16_t, AnyCharsAccess>::getFullAsciiCodePoint(int32_t, int32_t*)':
/root/firefox-gcc-last/js/src/frontend/TokenStream.h:1467:40: error: 'using CharsBase = class js::frontend::TokenStreamCharsBase<char16_t>' {aka 'class js::frontend::TokenStreamCharsBase<char16_t>'} has no member named 'previousCodeUnit'; did you mean 'peekCodeUnit'?
         MOZ_ASSERT(lead == sourceUnits.previousCodeUnit(),
                                        ^~~~~~~~~~~~~~~~
/root/firefox-gcc-last/obj-x86_64-pc-linux-gnu/dist/include/mozilla/Assertions.h:417:69: note: in definition of macro 'MOZ_VALIDATE_ASSERT_CONDITION_TYPE'
      static_assert(mozilla::detail::AssertionConditionType<decltype(x)>::isValid, \
                                                                     ^
/root/firefox-gcc-last/obj-x86_64-pc-linux-gnu/dist/include/mozilla/Assertions.h:450:39: note: in expansion of macro 'MOZ_ASSERT_HELPER2'
 #define MOZ_RELEASE_ASSERT_GLUE(a, b) a b
                                       ^
/root/firefox-gcc-last/obj-x86_64-pc-linux-gnu/dist/include/mozilla/Assertions.h:452:3: note: in expansion of macro 'MOZ_RELEASE_ASSERT_GLUE'
   MOZ_RELEASE_ASSERT_GLUE( \
   ^~~~~~~~~~~~~~~~~~~~~~~
/root/firefox-gcc-last/obj-x86_64-pc-linux-gnu/dist/include/mozilla/Assertions.h:457:27: note: in expansion of macro 'MOZ_RELEASE_ASSERT'
 #  define MOZ_ASSERT(...) MOZ_RELEASE_ASSERT(__VA_ARGS__)
                           ^~~~~~~~~~~~~~~~~~
/root/firefox-gcc-last/js/src/frontend/TokenStream.h:1467:9: note: in expansion of macro 'MOZ_ASSERT'
         MOZ_ASSERT(lead == sourceUnits.previousCodeUnit(),
         ^~~~~~~~~~
/root/firefox-gcc-last/obj-x86_64-pc-linux-gnu/dist/include/mozilla/Assertions.h:417:71: error: template argument 1 is invalid
      static_assert(mozilla::detail::AssertionConditionType<decltype(x)>::isValid, \
                                                                       ^
/root/firefox-gcc-last/obj-x86_64-pc-linux-gnu/dist/include/mozilla/Assertions.h:442:5: note: in expansion of macro 'MOZ_VALIDATE_ASSERT_CONDITION_TYPE'
     MOZ_VALIDATE_ASSERT_CONDITION_TYPE(expr); \
     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/root/firefox-gcc-last/obj-x86_64-pc-linux-gnu/dist/include/mozilla/Assertions.h:450:39: note: in expansion of macro 'MOZ_ASSERT_HELPER2'
 #define MOZ_RELEASE_ASSERT_GLUE(a, b) a b
                                       ^
/root/firefox-gcc-last/obj-x86_64-pc-linux-gnu/dist/include/mozilla/Assertions.h:452:3: note: in expansion of macro 'MOZ_RELEASE_ASSERT_GLUE'
   MOZ_RELEASE_ASSERT_GLUE( \
   ^~~~~~~~~~~~~~~~~~~~~~~
/root/firefox-gcc-last/obj-x86_64-pc-linux-gnu/dist/include/mozilla/MacroArgs.h:16:26: note: in expansion of macro 'MOZ_CONCAT2'
 #define MOZ_CONCAT(x, y) MOZ_CONCAT2(x, y)
                          ^~~~~~~~~~~
/root/firefox-gcc-last/obj-x86_64-pc-linux-gnu/dist/include/mozilla/MacroArgs.h:73:51: note: in expansion of macro 'MOZ_CONCAT'
 #define MOZ_PASTE_PREFIX_AND_ARG_COUNT_GLUE(a, b) a b
                                                   ^
/root/firefox-gcc-last/obj-x86_64-pc-linux-gnu/dist/include/mozilla/MacroArgs.h:75:3: note: in expansion of macro 'MOZ_PASTE_PREFIX_AND_ARG_COUNT_GLUE'
   MOZ_PASTE_PREFIX_AND_ARG_COUNT_GLUE( \
   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/root/firefox-gcc-last/obj-x86_64-pc-linux-gnu/dist/include/mozilla/Assertions.h:453:5: note: in expansion of macro 'MOZ_PASTE_PREFIX_AND_ARG_COUNT'
     MOZ_PASTE_PREFIX_AND_ARG_COUNT(MOZ_ASSERT_HELPER, __VA_ARGS__), \
     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/root/firefox-gcc-last/obj-x86_64-pc-linux-gnu/dist/include/mozilla/Assertions.h:457:27: note: in expansion of macro 'MOZ_RELEASE_ASSERT'
 #  define MOZ_ASSERT(...) MOZ_RELEASE_ASSERT(__VA_ARGS__)
                           ^~~~~~~~~~~~~~~~~~
/root/firefox-gcc-last/js/src/frontend/TokenStream.h:1467:9: note: in expansion of macro 'MOZ_ASSERT'
         MOZ_ASSERT(lead == sourceUnits.previousCodeUnit(),
         ^~~~~~~~~~
Jeff, Rings a bell ?
Flags: needinfo?(jwalden+bmo)
This is a very unsubtle gcc bug.  It's complaining about a (.*)TokenStream(.*) class not having a previousCodeUnit member function, but the call very clearly is on |sourceUnits| which quite obviously is not a token stream class at all.

Fortunately this looks like some interaction of |using| inside template classes that's trivially avoided by just throwing some |this-|-qualification on things.  Will land that change shortly.
Flags: needinfo?(jwalden+bmo)
...okay, so comment 2 is technically correct, but the fix involves qualifying a massive number of spots in code.  Enough that you basically should just remove a few |using|s for non-obvious reasons.

I'm going to reduce a testcase and report a gcc bug.  Maybe the gcc folks will have a suggestion for an elegant workaround, at least temporarily until gcc fixes this (IMO, potentially maybe even in existing releases).  Barring that, I will probably be removing some |using|s.
I filed https://gcc.gnu.org/bugzilla/show_bug.cgi?id=86379 with a mostly-reduced testcase that fails to compile with gcc 8.0+ but *does* compile with clang and with gcc<8.0.

I don't know when they'll get back to us on what the best workaround is, if there is one.  In the meantime, the |this|-qualifying approach works, so I'll clean up that patch and post it for review.
I'm so sorry.  :-(

Not all of these qualifications are necessary, but it isn't immediately clear to me *which* ones are necessary and *why*.  *Because* it isn't clear, I think the best approach is to require that all accesses to |sourceUnits| and |charBuffer| be qualified, by removing the |using|s for each of them.  Better to have one clear rule -- always |this->|-qualify -- than to leave a permanent minefield in place for future changes.  (I know there's a distinction between the necessary/unnecessary ones because I first wrote a patch changing only the ones that caused compile errors, then removed the |using|s and added qualifiers for the rest.)
Attachment #8989301 - Flags: review?(arai.unmht)
Assignee: nobody → jwalden+bmo
Status: NEW → ASSIGNED
Comment on attachment 8989301 [details] [diff] [review]
|this->|-qualify a bunch of function calls on |using Base::foo| members that gcc 8 completely chokes on and grossly misunderstands

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

Looks good :)
Attachment #8989301 - Flags: review?(arai.unmht) → review+
Pushed by jwalden@mit.edu:
https://hg.mozilla.org/integration/mozilla-inbound/rev/25d29385236c
|this->|-qualify a bunch of function calls on |using Base::foo| members that gcc 8 completely chokes on and grossly misunderstands.  r=arai
https://hg.mozilla.org/mozilla-central/rev/25d29385236c
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: