|Submitter||Diff||Changes||Open Issues||Last Updated|
|Error loading review requests:|
58 bytes, text/x-review-board-request
|Details | Review|
Currently xul.dll exports a bunch of symbols like this: struct mozilla::Module const * const Apprunner_NSModule struct mozilla::Module const * const Browser_Embedding_Module_NSModule I'm pretty sure that we don't need to export these modules. I'd like to try not doing it and see what happens.
See bug 938437 comment 5 why the symbols are exported. The good news is it's not a problem for Windows. And things may have changed on other platforms.
Assignee: nobody → benjamin
Status: NEW → ASSIGNED
Comment on attachment 8786838 [details] Bug 1299159 - Stop exporting *_NSModule from xul.dll, https://reviewboard.mozilla.org/r/75726/#review73756 If you're not going to do it in this bug, please file a followup to at least try to do the same on other platforms. ::: toolkit/library/libxul.mk:56 (Diff revision 1) > endif > endif > endif > > ifdef _MSC_VER > -get_first_and_last = dumpbin -exports $1 | grep _NSModule@@ | sort -k 3 | sed -n 's/^.*?\([^@]*\)@@.*$$/\1/;1p;$$p' > +get_first_and_last = dumpbin -section:rodata -map $1 | grep _NSModule@@ | sort -k 2 | sed -n 's/^.*?\([^@]*\)@@.*$$/\1/;1p;$$p' It's funny how -map is not documented on MSDN. Interestingly, -section:rodata doesn't make much difference. Also interestingly, this requires the .pdb file to work, and when the .pdb file is not there, the output format is different (and doesn't include symbols). I hope nobody is building without debugging info.
Attachment #8786838 - Flags: review?(mh+mozilla) → review+
Pushed by email@example.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/58b07057fe25 Stop exporting *_NSModule from xul.dll, r=glandium
First guess on what happens is that it breaks Windows PGO builds in startup cache precompilation (https://treeherder.mozilla.org/logviewer.html#?job_id=35164615&repo=mozilla-inbound, not that there's ever anything in the log from that bustage). Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/9b827786ca68a923bacef503c9a52369381b3170 to see whether I'm right that it was this one.
From a local PGO build: $ dumpbin -map obj-i686-pc-mingw32/toolkit/library/xul.dll |grep _NSModule | sort -k2 Dump of file obj-i686-pc-mingw32/toolkit/library/xul.dll 0002:00002AB4 05847AB4 40302040 4 ?start_kPStaticModules_NSModule@@3QBUModule@mozilla@@B (struct mozilla::Module const * const start_kPStaticModules_NSModule) 0002:00002AB8 05847AB8 40302040 4 ?end_kPStaticModules_NSModule@@3QBUModule@mozilla@@B (struct mozilla::Module const * const end_kPStaticModules_NSModule) So the LOCAL_CHECK still passes because the first and last are still start and end, but... they are adjacent and there are no other NSModules in there, so no component registration, so no component initialization, and kaboom. Thanks PGO.
Ugh. Are these symbols being excluded by opt:ref (removing unreferenced symbols)? Other than explicitly referencing symbols in toolkit/library (which takes us almost back to nsDllDeps.cpp) do you have other tips how I might achieve this?
(In reply to Benjamin Smedberg [:bsmedberg] from comment #7) > Ugh. Are these symbols being excluded by opt:ref (removing unreferenced > symbols)? We're passing -OPT:REF on both normal and PGO builds. I'd expect it to have the same outcome in both cases if it was responsible for it. But yeah, it looks a bit like it. > Other than explicitly referencing symbols in toolkit/library (which takes us > almost back to nsDllDeps.cpp) do you have other tips how I might achieve > this? Unfortunately, it doesn't seem there is an easy way out: https://connect.microsoft.com/VisualStudio/Feedback/Details/1587892 https://connect.microsoft.com/VisualStudio/feedback/details/1314631/c-lost-global-variables-with-wpo-ltcg It would be sad to go back to the old days of nsStaticXULComponents.cpp. Does it hurt to leave them exported? Not all XPCOM components are equal, and I suspect that if we need to hide some, we can probably get away with not hiding most. In that case, maybe we can look into deCOMtaminating them? Or to manually register them first in nsComponentManagerImpl::InitializeStaticModules? Better to have a small list of hardcoded central components that are always there than to have a list of abount a hundred components with plenty of #ifdefs like we used to have.
Assignee: benjamin → nobody
Status: ASSIGNED → NEW
You need to log in before you can comment on or make changes to this bug.