Stop exporting _NSModule modules from xul.dll

NEW
Unassigned

Status

Firefox Build System
General
2 years ago
2 months ago

People

(Reporter: Benjamin Smedberg, Unassigned)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

MozReview Requests

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Reporter)

Description

2 years ago
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.
(Reporter)

Updated

2 years ago
Blocks: 1299187
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.
Comment hidden (mozreview-request)
(Reporter)

Updated

2 years ago
Assignee: nobody → benjamin
Status: NEW → ASSIGNED

Comment 3

2 years ago
mozreview-review
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+

Comment 4

2 years ago
Pushed by bsmedberg@mozilla.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.
(Reporter)

Comment 7

2 years ago
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?
Flags: needinfo?(mh+mozilla)
(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.
Flags: needinfo?(mh+mozilla)
(Reporter)

Updated

2 years ago
Assignee: benjamin → nobody
Status: ASSIGNED → NEW

Updated

2 months ago
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.