Firefox compilation failed with LTO (gcc 4.7.2)

RESOLVED FIXED in mozilla30

Status

()

Core
JavaScript Engine
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: Martin Liška, Unassigned)

Tracking

Trunk
mozilla30
x86_64
Linux
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments)

(Reporter)

Description

5 years ago
User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.11 (KHTML, like Gecko) Chrome/23.0.1271.97 Safari/537.11

Steps to reproduce:

Firefox nightly (Dec 23 - changeset 116995:dc2abccc2adb) compilation failed with gcc 4.7.2 having enabled switch -flto.

uname -a
Linux marxinbox 3.0.35-gentoo #4 SMP Fri Dec 28 17:30:28 CET 2012 x86_64 AMD FX(tm)-8350 Eight-Core Processor AuthenticAMD GNU/Linux

.mozconfig
mk_add_options MOZ_MAKE_FLAGS="-j10"
ac_add_options --enable-application=browser
ac_add_options --enable-optimize=-O2
export CXXFLAGS="-flto"
export LDFLAGS="-flto"



Actual results:

/usr/bin/python2.7 /home/marxin/Programming/firefox/js/src/config/pythonpath.py -I../config /home/marxin/Programming/firefox/js/src/config/expandlibs_exec.py --depend .deps/js.pp --target js --uselist --  c++ -o js  -pedantic -Wall -Wpointer-arith -Woverloaded-virtual -Werror=return-type -Wtype-limits -Wempty-body -Werror=conversion-null -Wno-ctor-dtor-privacy -Wno-overlength-strings -Wno-invalid-offsetof -Wno-variadic-macros -Wcast-align -Wno-long-long -flto -fno-rtti -ffunction-sections -fdata-sections -fno-exceptions -pthread -pipe  -DNDEBUG -DTRIMMED -g -O2 -fomit-frame-pointer js.o jsoptparse.o jsheaptools.o   -lpthread -flto -Wl,--build-id   -Wl,-rpath-link,../../../dist/bin -Wl,-rpath-link,/home/marxin/Programming/firefox/obj-x86_64-unknown-linux-gnu/dist/lib   -L../../../dist/bin -L../../../dist/lib -L/home/marxin/Programming/firefox/obj-x86_64-unknown-linux-gnu/dist/lib -lnspr4 -lplc4 -lplds4 ../editline/libeditline.a ../libjs_static.a /home/marxin/Programming/firefox/obj-x86_64-unknown-linux-gnu/modules/zlib/src/libmozz.a -Wl,--whole-archive ../../../dist/lib/libmozglue.a ../../../dist/lib/libmemory.a -Wl,--no-whole-archive -rdynamic -ldl    
`PushActiveVMFrame' referenced in section `.text' of /tmp/cc3CQA0q.ltrans0.ltrans.o: defined in discarded section `.text' of MethodJIT.o (symbol from plugin)
`PopActiveVMFrame' referenced in section `.text' of /tmp/cc3CQA0q.ltrans0.ltrans.o: defined in discarded section `.text' of MethodJIT.o (symbol from plugin)
`js_InternalThrow' referenced in section `.text' of /tmp/cc3CQA0q.ltrans0.ltrans.o: defined in discarded section `.text' of InvokeHelpers.o (symbol from plugin)
`PopActiveVMFrame' referenced in section `.text' of /tmp/cc3CQA0q.ltrans0.ltrans.o: defined in discarded section `.text' of MethodJIT.o (symbol from plugin)
`js_InternalInterpret' referenced in section `.text' of /tmp/cc3CQA0q.ltrans0.ltrans.o: defined in discarded section `.text' of InvokeHelpers.o (symbol from plugin)
`PopActiveVMFrame' referenced in section `.text' of /tmp/cc3CQA0q.ltrans0.ltrans.o: defined in discarded section `.text' of MethodJIT.o (symbol from plugin)
/usr/lib/gcc/x86_64-pc-linux-gnu/4.7.2/../../../../x86_64-pc-linux-gnu/bin/ld: js: hidden symbol `_ZN2js3ion18LiveRangeAllocatorINS0_27BacktrackingVirtualRegisterEE17buildLivenessInfoEv' isn't defined
/usr/lib/gcc/x86_64-pc-linux-gnu/4.7.2/../../../../x86_64-pc-linux-gnu/bin/ld: final link failed: Bad value
collect2: error: ld returned 1 exit status
Assignee: nobody → general
Component: Untriaged → JavaScript Engine
Product: Firefox → Core
Martin, at first glance that seems like a GCC bug, since those symbols are just fine in the source.  Have you reported the bug to them?
(Reporter)

Comment 2

5 years ago
It is really conected to gcc 4.7.2 -flto option.
(Reporter)

Comment 3

4 years ago
I created a simple patch adding '__attribute__ ((used))' to disable optimizing out these symbols by LTO.

Files in xpcom/reflect/xptcall/src/md/unix/xptcstubs_* are platform specific, I need to change just one (x86_64_linux), but I hope it make sense to modify all these platform specific files.

Thanks,
Martin
(Reporter)

Comment 4

4 years ago
Created attachment 777050 [details] [diff] [review]
LTO patch
(In reply to Boris Zbarsky [:bz] from comment #1)
> Martin, at first glance that seems like a GCC bug, since those symbols are
> just fine in the source.  Have you reported the bug to them?

yes, the jsapi.cpp change shouldn't be needed afaik, but PrepareAndDispatch() is only called from inline asm, so it seems fiarly reasonable that without being told otherwise the compiler would decide to get rid of those symbols.  Infact a few functions in xpcom/reflect/xptcall/src have already been marked as used.
Looks like those JS symbols are only referenced from assembly, too, so they have the same issues as the xptcall ones (link only valid for the next couple of weeks =/):

http://mxr.mozilla.org/mozilla-release/source/js/src/methodjit/TrampolineMingwX64.s#7

Patch looks good to me modulo the jsapi bits.  I don't know that it's worth adding attributes to every file, maybe only x86-64, x86, and arm ones...
(Reporter)

Comment 7

4 years ago
(In reply to Nathan Froyd (:froydnj) from comment #6)
> Looks like those JS symbols are only referenced from assembly, too, so they
> have the same issues as the xptcall ones (link only valid for the next
> couple of weeks =/):
> 
> http://mxr.mozilla.org/mozilla-release/source/js/src/methodjit/
> TrampolineMingwX64.s#7
> 
> Patch looks good to me modulo the jsapi bits.  I don't know that it's worth
> adding attributes to every file, maybe only x86-64, x86, and arm ones...

Please select architectures where these function should be marked by the attribute (for sure, I added the attribute to all of them).
(In reply to Nathan Froyd (:froydnj) from comment #6)
> Looks like those JS symbols are only referenced from assembly, too, so they
> have the same issues as the xptcall ones (link only valid for the next
> couple of weeks =/):

but the JS_API decorator makes them have default visibility as opposed to PrepareAndDispatch which afaik doesn't need to be externally visible.

> Patch looks good to me modulo the jsapi bits.  I don't know that it's worth
> adding attributes to every file, maybe only x86-64, x86, and arm ones...

well, if you're already there and it seems correct why not?
Created attachment 8361895 [details] [diff] [review]
bug 826481 - makr PrepareAndDispatch() as __attribute__((used)) when building with gcc
Attachment #8361895 - Flags: review?(nfroyd)
Comment on attachment 8361895 [details] [diff] [review]
bug 826481 - makr PrepareAndDispatch() as __attribute__((used)) when building with gcc

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

r=me assuming it all builds and with the changes below.

::: xpcom/reflect/xptcall/src/md/unix/xptcstubs_alpha_openbsd.cpp
@@ +13,2 @@
>  PrepareAndDispatch(nsXPTCStubBase* self, uint32_t methodIndex, uint64_t* args)
>  __asm__("PrepareAndDispatch") __attribute__((used));

Looks like we already specify the attribute here.  Please change it to ATTRIBUTE_USED and remove the other instance.

::: xpcom/reflect/xptcall/src/md/unix/xptcstubs_arm.cpp
@@ +40,5 @@
>  #else
>  #define CFI(str) str
>  #endif
>  
> +static nsresult ATTRIBUTE_USED

Instead of this bit, can you get rid of the DONT_DROP_OR_WARN bits above and s/DONT_DROP_OR_WARN/ATTRIBUTE_USED/ on the declaration?

::: xpcom/reflect/xptcall/src/md/unix/xptcstubs_linux_alpha.cpp
@@ +13,2 @@
>  PrepareAndDispatch(nsXPTCStubBase* self, uint32_t methodIndex, uint64_t* args)
>  __asm__("PrepareAndDispatch") __attribute__((used));

Looks like we already specify the attribute here.  Please change it to ATTRIBUTE_USED and remove the other instance.

::: xpcom/reflect/xptcall/src/xptcprivate.h
@@ +58,5 @@
>  
>  #undef STUB_ENTRY
>  #undef SENTINEL_ENTRY
>  
> +#if MOZ_IS_GCC

Probably most useful to say #if defined(__clang__) || defined(__GNUC__) and leave Compiler.h out of it.  Or sub MOZ_IS_GCC for defined(__GNUC__).  Either way, the __clang__ bit should be in there.
Attachment #8361895 - Flags: review?(nfroyd) → review+
Created attachment 8377761 [details] [diff] [review]
rebased patch

I forgot about landing this one because of a couple other lto things in my queue, but I guess this might as well land.  I think this fixed all the comments.
https://hg.mozilla.org/integration/mozilla-inbound/rev/f59bb340e3e6
Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/66f9f6671c10 for making Android angry like https://tbpl.mozilla.org/php/getParsedLog.php?id=34885564&tree=Mozilla-Inbound
Status: UNCONFIRMED → NEW
Ever confirmed: true
with __GNUC__ spelled correctly https://hg.mozilla.org/integration/mozilla-inbound/rev/cc5ce0bdff58
https://hg.mozilla.org/mozilla-central/rev/cc5ce0bdff58
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
You need to log in before you can comment on or make changes to this bug.