Closed Bug 508861 Opened 15 years ago Closed 15 years ago

[electrolysis] Build/ship the C++ runtime with MSVC/jemalloc

Categories

(Firefox Build System :: General, defect)

x86
Windows NT
defect
Not set
normal

Tracking

(status1.9.2 .4-fixed)

RESOLVED FIXED
mozilla1.9.3a1
Tracking Status
status1.9.2 --- .4-fixed

People

(Reporter: benjamin, Assigned: ted)

References

Details

(Whiteboard: [fixed-lorentz])

Attachments

(1 file, 1 obsolete file)

Electrolysis builds need the C++ runtime. For MSVC builds without jemalloc this means shipping msvcp80.dll which comes with MSVC (http://mxr.mozilla.org/mozilla-central/source/build/win32/Makefile.in#53)

For MSVC builds with jemalloc I'm not sure what we need to do. I think we'll have to build the C++ runtime with jemalloc. Ted, since you have MSVC8 can you take this?
Urk. This probably means actually touching the CRT build bits, doesn't it?
It might, I don't know. We probably need to name it mozcprt19.dll or something like that, and we probably don't currently.
Ok, tell you what, I'm willing to do the god-awful nmakefile hacking, but I'll probably need some help on the actual hook-jemalloc-into-the-c++-guts bits.
I don't *think* you'll need to hook jemalloc up at all: as long as ::operator new forwards to malloc() and not directly to some Windows function we should be good.
Attached patch build mozcpp19.dll, link to it (obsolete) — Splinter Review
Since the actual CRT diff is no use, here's what I changed there:
* Rename sample_p.dll -> mozcpp19.dll (and other related files)
* Stop trying to compile stdhndlr.cpp since it references _set_new_handler, which is in a file that apparently we removed from our libc in the previous round of patching
Attachment #393265 - Flags: review?(benjamin)
Status: NEW → ASSIGNED
I realized that I forgot to add the new DLL to the packaging manifest.
Attachment #393265 - Attachment is obsolete: true
Attachment #393497 - Flags: review?(benjamin)
Attachment #393265 - Flags: review?(benjamin)
Comment on attachment 393497 [details] [diff] [review]
build mozcpp19.dll, link to it, package it

Ted, can we go ahead and land this on m-c (after 1.9.2 branches) instead of keeping the forked copy in Electrolysis and worrying about regressions when that crash-lands?

Or, could we keep the _STATIC_CPPLIB bits (not actually using this new C++ stdlib on mozilla-central) but build it, and electrolysis will take the configure.in changes only?
Attachment #393497 - Flags: review?(benjamin) → review+
We can land it on m-c, it just seemed crummy to start shipping an extra 700KB of DLL that we didn't actually need yet.
Yeah... could we build it but not package it? Or at least land the fixes necessary to build it but not actually build it? I don't want Electrolysis to have a different patch than the upcoming ones for VC9, for example... that sounds like a maintenance/merge nightmare.
Going to hold this till bug 416117 gets fixed, since that has a CRT patch for VC9, and I'll need to fix that as well.
Depends on: 416117
Changed my mind, since this bug was breaking a compiled unittest that jorendorff landed that used C++ features. I'll just have to fix up that other patch before it lands.

Pushed to m-c:
http://hg.mozilla.org/mozilla-central/rev/d25e72de30f1
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
No longer depends on: 416117
Resolution: --- → FIXED
http://hg.mozilla.org/mozilla-central/rev/412b0a03c1cf
Disable using the static C++ runtime (on Windows-MSVC) because Chromium requires the full version and we're getting duplicate symbols.

Just to double-check, should
"dnl Statically link the C++ stdlib.  We only use this for Breakpad anyway."
be removed too?
Flags: in-testsuite-
Target Milestone: --- → mozilla1.9.3a1
Version: unspecified → Trunk
(In reply to comment #12)

Disregard that comment (about an e10s changeset.!.): I got confused trying to sort this story out for c-c...
Blanket approval for Lorentz merge to mozilla-1.9.2
a=beltzner for 1.9.2.4 - please make sure to mark status1.9.2:.4-fixed
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.