826481 - Firefox compilation failed with LTO (gcc 4.7.2)

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Martin Liška]

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

Comment 1

[Boris Zbarsky [:bzbarsky]]

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?

Comment 2

[Martin Liška]

It is really conected to gcc 4.7.2 -flto option.

Comment 3

[Martin Liška]

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

Comment 4

[Martin Liška]

Attachment: LTO patch

Comment 5

[Trevor Saunders (:tbsaunde)]

(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.

Comment 6

[Nathan Froyd [:froydnj]]

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...

Comment 7

[Martin Liška]

(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).

Comment 8

[Trevor Saunders (:tbsaunde)]

(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?

Comment 9

[Trevor Saunders (:tbsaunde)]

Attachment: bug 826481 - makr PrepareAndDispatch() as __attribute__((used)) when building with gcc

Comment 10

[Nathan Froyd [:froydnj]]

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.

Comment 11

[Trevor Saunders (:tbsaunde)]

Attachment: 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.

Comment 12

[Trevor Saunders (:tbsaunde)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f59bb340e3e6

Comment 13

[Phil Ringnalda (:philor)]

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

Comment 14

[Trevor Saunders (:tbsaunde)]

with __GNUC__ spelled correctly https://hg.mozilla.org/integration/mozilla-inbound/rev/cc5ce0bdff58

Comment 15

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/cc5ce0bdff58