Closed Bug 938437 Opened 11 years ago Closed 10 years ago

Replace nsStaticXULComponents.cpp with smart use or sections

Categories

(Firefox Build System :: General, defect)

Product:
Firefox Build System
Component:
General
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED
Milestone:
mozilla28

People

(Reporter: glandium, Assigned: glandium)

References

Details

Attachments

(2 files, 3 obsolete files)

Work around BFD ld suckage, win PGO bustage, and add some checks
11.05 KB, patch
froydnj
: review+
Details | Diff | Splinter Review
Replace nsStaticXULComponents.cpp with smart use of sections
19.17 KB, patch
gps
: review+
Details | Diff | Splinter Review 
It's 2013. There's no reason we should inflict ourselves the need to update nsStaticXULComponents.cpp and add the right DEFINES for the collection of static modules we build into libxul.
This also removes a level of indirection in the list of static modules.
Attachment #831928 - Flags: review?(benjamin)
I'm torn. Is this going to make porting to other targets impossibly hard? It's a cool hack, but maybe we could just use equivalent global knowledge from mozbuild files to do the same thing?
(In reply to Benjamin Smedberg  [:bsmedberg] from comment #2)
> I'm torn. Is this going to make porting to other targets impossibly hard?
> It's a cool hack, but maybe we could just use equivalent global knowledge
> from mozbuild files to do the same thing?

mozbuild files don't have knowledge about what is in the code. It would seem awkward to add it. That being said, we could collect that info and generate nsStaticXULComponents.cpp. But I'd rather do that on those other targets only. Which i'm not even sure actually exist. What compiler supports the C++11 subset we require and doesn't have a way to put things in separate sections?
The only non-gcc non-clang non-MSVC compiler I know people have been building Firefox with in recent history (for some large value of recent), is Sun compiler (which, afaik, doesn't have an equivalent to __attribute__((section)) ), but considering http://wiki.apache.org/stdcxx/C++0xCompilerSupport I'm doubtful it can compile Firefox at all anymore.

Someone asked recently on irc about trying ibm xlc, but I don't know how far they've got. I also don't know if it supports an equivalent to __attribute__((section)).

So how about we ignore it for the moment, and come back when someone actually reports a bug? I don't think coming up with a fix then will be extremely hard, but I also don't think it's worth doing until it's necessary. We have plenty of untested things bitrotting in the build system, let's not add more (and i really don't think we should use such a fallback solution by default for our builds).
Comment on attachment 831928 [details] [diff] [review]
Replace nsStaticXULComponents.cpp with smart use or sections

So here's an interesting status:
- It works perfectly fine on windows
- On OSX, -dead_strip kills it. That can be worked around with __attribute__((used)), but somehow, clang decides to warn it's ignored (might be related to the "extern"), and combined with -Werror, that fails. I'm sure this can be made to work.
- On Linux, --gc-sections kills it. A small linker script can make that work. That works fine with BFD ld. Gold crashes. We're using BFD ld on build slaves, and local builds don't --gc-sections. Still something we should check at configure time (I hope we can create a small testcase)
- On Android, same as Linux... except we use gold on the build slaves. Yay.

So it looks like the fallback is going to be necessary :(
Attachment #831928 - Flags: review?(benjamin)
Or... we can make those symbols visible. Which is kind of horrible, but it's only about 50 of them.
Let's go with this, which works on all platforms on tbpl, and refine in followups.
Attachment #832209 - Flags: review?(benjamin)
Attachment #831928 - Attachment is obsolete: true
(In reply to Mike Hommey [:glandium] from comment #7)
> Created attachment 832209 [details] [diff] [review]
> Replace nsStaticXULComponents.cpp with smart use or sections

Nit: you probably want to say "smart use *of* sections".
Comment on attachment 832209 [details] [diff] [review]
Replace nsStaticXULComponents.cpp with smart use or sections

This is going to break mingw builds, isn't it? Do they also support the __declspec(allocate) syntax?

We used to have MODULE in the build system specifically so that we could generate this in the build system. Is this really better than resurrecting that?

begrudging r+, as long as there is a way for mingw to be fixed somehow.
Attachment #832209 - Flags: review?(benjamin) → review+
mingw does support __attribute__((section)) iirc, but I think it needs the pragma too. This will need some testing, and I don't want to block on this. I'll Cc Jacek Caban on that bug.
Depends on: 940208
had to backout this change in https://hg.mozilla.org/integration/mozilla-inbound/rev/3b1d81c4c118 because of build bustages on PGO Builds
A new version with a fix for the test on mac, an additional check on linux/android, and the use of TOOLCHAIN_PREFIX for nm.
Attachment #8335875 - Flags: review?(nfroyd)
Attachment #8335776 - Attachment is obsolete: true
Attachment #8335776 - Flags: review?(nfroyd)
Comment on attachment 8335875 [details] [diff] [review]
Work around BFD ld suckage, win PGO bustage, and add some checks

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

::: toolkit/library/Makefile.in
@@ +404,5 @@
> +# script however makes gold misbehave, first because it doesn't like that
> +# the linker script is given after crtbegin.o, and even past that, replaces
> +# the default section rules with those from the script instead of
> +# supplementing them. Which leads to a lib with a huge load of sections.
> +ifdef LD_IS_BFD

Does this need to be guarded for the mingw32 case?  (I realize it this whole scheme doesn't work on mingw32 right now, but we can try a little bit to not make it deliberately difficult.)  Or does the mingw32 case need an entirely different linker script?

::: toolkit/library/StaticXULComponentsEnd/StaticXULComponentsEnd.cpp
@@ +1,5 @@
>  #include "mozilla/Module.h"
>  #include "mozilla/NullPtr.h"
>  
> +/* Ensure end_kPStaticModules is at the end of the .kPStaticModules section
> + * on Windows. Somehow, placing the object last is not enough with PGO/LTCG. */

Can you clarify this comment by saying something about the linker sorting section names alphabetically?
Attachment #8335875 - Flags: review?(nfroyd) → review+
https://hg.mozilla.org/mozilla-central/rev/1eb6ceed2cda
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
Confirmed green post-backout.
Depends on: 942184
In a local build (VS2012, Windows 8, opt, not debug) before this was backed out, I got a "NSModules are not ordered appropriately" error in toolkit/library/libs.  Is there anything I can do to help debug that if it happens again?
(In reply to Matt Brubeck (:mbrubeck) from comment #20)
> In a local build (VS2012, Windows 8, opt, not debug) before this was backed
> out, I got a "NSModules are not ordered appropriately" error in
> toolkit/library/libs.  Is there anything I can do to help debug that if it
> happens again?

What does:

dumpbin -exports xul.dll | grep _NSModule@@ | sort -k 3

produce on a build that shows that error?  I'm sure glandium will be interested.
Flags: needinfo?(mbrubeck)
(In reply to Nathan Froyd (:froydnj) from comment #21)
> What does:
> 
> dumpbin -exports xul.dll | grep _NSModule@@ | sort -k 3
> 
> produce on a build that shows that error?

Unfortunately I don't have the "bad" xul.dll around anymore (make automatically deletes the file when the recipe fails, and I've since started a new build from inbound tip).  Over the weekend I can try building with this patch applied and capture the dumpbin output.
(In reply to Matt Brubeck (:mbrubeck) from comment #22)
> (In reply to Nathan Froyd (:froydnj) from comment #21)
> > What does:
> > 
> > dumpbin -exports xul.dll | grep _NSModule@@ | sort -k 3
> > 
> > produce on a build that shows that error?
> 
> Unfortunately I don't have the "bad" xul.dll around anymore (make
> automatically deletes the file when the recipe fails, and I've since started
> a new build from inbound tip).  Over the weekend I can try building with
> this patch applied and capture the dumpbin output.

When you do, add .PRECIOUS: $(SHARED_LIBRARY) at the end of toolkit/library/Makefile.in.
(In reply to Matt Brubeck (:mbrubeck) from comment #20)
> In a local build (VS2012, Windows 8, opt, not debug) before this was backed
> out, I got a "NSModules are not ordered appropriately" error in
> toolkit/library/libs.

I couldn't reproduce the error with this patch applied on the current m-c tip.  Perhaps it was caused by something in my previous objdir state.
Flags: needinfo?(mbrubeck)
(In reply to Matt Brubeck (:mbrubeck) from comment #24)

> I couldn't reproduce the error with this patch applied on the current m-c
> tip.  Perhaps it was caused by something in my previous objdir state.

clobber problem, maybe? Were you doing a full build back then, or a mach build objdir/subdir build?
Yes, could have been a clobber problem.  I did a full build (just "mach build") but I didn't clobber.
For posterity, the reason this was backed out is the same as why the following patch kills jetpack tests:

diff --git a/toolkit/library/nsStaticXULComponents.cpp b/toolkit/library/nsStaticXULComponents.cpp
--- a/toolkit/library/nsStaticXULComponents.cpp
+++ b/toolkit/library/nsStaticXULComponents.cpp
@@ -189,19 +189,19 @@
     AUTH_MODULE                              \
     MODULE(nsJarModule)                      \
     ZIPWRITER_MODULE                         \
     MODULE(StartupCacheModule)               \
     MODULE(nsPrefModule)                     \
     MODULE(nsRDFModule)                      \
     MODULE(nsWindowDataSourceModule)         \
     MODULE(nsParserModule)                   \
-    MODULE(nsImageLib2Module)                \
     MODULE(nsMediaSnifferModule)             \
     MODULE(nsGfxModule)                      \
+    MODULE(nsImageLib2Module)                \
     PROFILER_MODULE                          \
     WIDGET_MODULES                           \
     ICON_MODULE                              \
     MODULE(nsPluginModule)                   \
     MODULE(nsLayoutModule)                   \
     MODULE(docshell_provider)                \
     MODULE(embedcomponents)                  \
     MODULE(Browser_Embedding_Module)         \


Now, if someone could figure out why that's happening...
(In reply to Mike Hommey [:glandium] from comment #27)
> For posterity, the reason this was backed out is the same as why the
> following patch kills jetpack tests:
> 
> diff --git a/toolkit/library/nsStaticXULComponents.cpp
> b/toolkit/library/nsStaticXULComponents.cpp
> --- a/toolkit/library/nsStaticXULComponents.cpp
> +++ b/toolkit/library/nsStaticXULComponents.cpp
> @@ -189,19 +189,19 @@
>      AUTH_MODULE                              \
>      MODULE(nsJarModule)                      \
>      ZIPWRITER_MODULE                         \
>      MODULE(StartupCacheModule)               \
>      MODULE(nsPrefModule)                     \
>      MODULE(nsRDFModule)                      \
>      MODULE(nsWindowDataSourceModule)         \
>      MODULE(nsParserModule)                   \
> -    MODULE(nsImageLib2Module)                \
>      MODULE(nsMediaSnifferModule)             \
>      MODULE(nsGfxModule)                      \
> +    MODULE(nsImageLib2Module)                \
>      PROFILER_MODULE                          \
>      WIDGET_MODULES                           \
>      ICON_MODULE                              \
>      MODULE(nsPluginModule)                   \
>      MODULE(nsLayoutModule)                   \
>      MODULE(docshell_provider)                \
>      MODULE(embedcomponents)                  \
>      MODULE(Browser_Embedding_Module)         \

Possibly related to bug 651498 or similar?  ISTR Zack griping in some other bug about gfx <-> imagelib module dependencies, but I cannot find the bug with the awesomebar.
(In reply to Nathan Froyd (:froydnj) from comment #28)
> Possibly related to bug 651498 or similar?  ISTR Zack griping in some other
> bug about gfx <-> imagelib module dependencies, but I cannot find the bug
> with the awesomebar.

Well, whatever it was, it looks like it's gone.
https://tbpl.mozilla.org/?tree=Try&rev=ced0bfac03ca
Rebased to current trunk, as we had many build system changes since last landing. Logic is still the same, just adjusted to current moz.builds (so you don't really have to care about the gory details).
Attachment #8445544 - Flags: review?(gps)
Attachment #832209 - Attachment is obsolete: true
Comment on attachment 8445544 [details] [diff] [review]
Replace nsStaticXULComponents.cpp with smart use of sections

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

307 Temporary Redirect
Attachment #8445544 - Flags: review?(gps) → review?(mshal)
Comment on attachment 8445544 [details] [diff] [review]
Replace nsStaticXULComponents.cpp with smart use of sections

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

I didn't realize mshal was on vacation.

Anyway, I'm granting review on the basis the build foo looks sane. However, I don't really grok everything that's going on in libxul.mk w.r.t. PT_LOAD and symbol tables and linker behavior. I see earlier reviews from people who I think grok this. I'm assuming they know what's going on.
Attachment #8445544 - Flags: review?(mshal) → review+
https://hg.mozilla.org/mozilla-central/rev/f4f90287d00a
Status: REOPENED → RESOLVED
Closed: 11 years ago → 10 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
On Linux with I get with -flto:
...
test "$(nm -g libxul.so | grep _NSModule$ | sort | sed -n 's/^.* _*\([^ ]*\)$/\1/;1p;$p' | xargs echo)" != "start_kPStaticModules_NSModule end_kPStaticModules_NSModule" && echo "NSModules are not ordered appropriately" && exit 1 || exit 0 ; test "$(readelf -l libxul.so | awk 'libxul.so == "LOAD" { t += 1 } END { print t }')" -le 1 && echo "Only one PT_LOAD segment" && exit 1 || exit 0
NSModules are not ordered appropriately
markus@x4 build % nm -g libxul.so | grep _NSModule
00000000034e9720 D application_NSModule
00000000034e9748 D Apprunner_NSModule
00000000034e9700 D appshell_NSModule
00000000034e9730 D BOOT_NSModule
00000000034e96f8 D Browser_Embedding_Module_NSModule
00000000034e9760 D CommandLineModule_NSModule
00000000034e9768 D DiskSpaceWatcherModule_NSModule
00000000034e96e8 D docshell_provider_NSModule
00000000034e96f0 D embedcomponents_NSModule
00000000034e9628 D end_kPStaticModules_NSModule
00000000034e97c0 D identity_NSModule
00000000034e97a0 D jsctypes_NSModule
00000000034e97e8 D jsdebugger_NSModule
00000000034e97b8 D jsinspector_NSModule
00000000034e9780 D jsperf_NSModule
00000000034e9790 D jsreflect_NSModule
00000000034e9728 D mozSpellCheckerModule_NSModule
00000000034e9670 D mozStorageModule_NSModule
00000000034e9648 D necko_NSModule
00000000034e9650 D nsAuthModule_NSModule
00000000034e97d0 D nsAutoConfigModule_NSModule
00000000034e9658 D nsChardetModule_NSModule
00000000034e96d8 D nsComposerModule_NSModule
00000000034e96c8 D nsContentProcessWidgetModule_NSModule
00000000034e9678 D nsCookieModule_NSModule
00000000034e9770 D nsFileViewModule_NSModule
00000000034e9698 D nsGfxModule_NSModule
00000000034e97f0 D nsGIOModule_NSModule
00000000034e9640 D nsI18nModule_NSModule
00000000034e96a8 D nsIconDecoderModule_NSModule
00000000034e96a0 D nsImageLib2Module_NSModule
00000000034e9660 D nsJarModule_NSModule
00000000034e96e0 D nsLayoutModule_NSModule
00000000034e9778 D nsMediaSnifferModule_NSModule
00000000034e9690 D nsParserModule_NSModule
00000000034e9680 D nsPermissionsModule_NSModule
00000000034e9788 D nsPlacesModule_NSModule
00000000034e96b8 D nsPluginModule_NSModule
00000000034e9630 D nsPrefModule_NSModule
00000000034e9710 D nsProfilerModule_NSModule
00000000034e9688 D nsRDFModule_NSModule
00000000034e97d8 D nsServicesCryptoModule_NSModule
00000000034e9738 D NSS_NSModule
00000000034e9798 D nsTelemetryModule_NSModule
00000000034e9750 D nsToolkitCompsModule_NSModule
00000000034e96d0 D nsTransactionManagerModule_NSModule
00000000034e9638 D nsUConvModule_NSModule
00000000034e9708 D nsUniversalCharDetModule_NSModule
00000000034e97c8 D nsUnixProxyModule_NSModule
00000000034e96c0 D nsWidgetGtk2Module_NSModule
00000000034e9718 D nsWindowDataSourceModule_NSModule
00000000034e96b0 D peerconnection_NSModule
00000000034e9740 D PKI_NSModule
00000000034e9758 D RemoteServiceModule_NSModule
00000000034e97b0 D satchel_NSModule
00000000034e9620 D start_kPStaticModules_NSModule
00000000034e97e0 D StartupCacheModule_NSModule
00000000034e97a8 D tkAutoCompleteModule_NSModule
00000000034e9668 D ZipWriterModule_NSModule
markus@x4 build % nm -g libxul.so | grep _NSModule$ | sort | sed -n 's/^.* _*\([^ ]*\)$/\1/;1p;$p'
start_kPStaticModules_NSModule
nsGIOModule_NSModule

gcc-4.9, gold linker
File a bug.
Blocks: 1032055
No longer blocks: 1032055
Depends on: 1032055
Blocks: 1036832
JFYI: The other platform that would suffer from this trick is OS/2. Fortunately, there is a bunch of special .stabs types (so-called set elements) in the EMX variant of ld that allows to achieve the same goal. Even more nicely I'd say — these set elements are there specifically for creating tables like the one from nsStaticXULComponents.cpp (if these set elements cross-platform we'd not need section magic at all). However, to keep the amount of platform dependent code at minimum I had to use double indirection again (pretty much as it was before but done automatically by the linker, as with the section trick). The details are here (for ones who's interested): https://github.com/bitwiseworks/mozilla-os2/commit/695a89977dc63daa7d0c5fa5e3d1550573397595.
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: