Closed Bug 416117 Opened 12 years ago Closed 10 years ago

allow building jemalloc with VC9 SP1

Categories

(Firefox Build System :: General, enhancement)

x86
Windows XP
enhancement
Not set

Tracking

(status1.9.2 beta1-fixed)

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

People

(Reporter: ted, Assigned: robarnold)

References

Details

Attachments

(12 files, 2 obsolete files)

3.43 KB, text/plain
Details
2.18 KB, patch
Details | Diff | Splinter Review
3.38 KB, text/plain
Details
1.59 KB, patch
Details | Diff | Splinter Review
1.18 KB, patch
Details | Diff | Splinter Review
847 bytes, patch
Details | Diff | Splinter Review
1.04 KB, patch
Details | Diff | Splinter Review
3.40 KB, text/plain
Details
6.34 KB, patch
ted
: review+
Details | Diff | Splinter Review
4.10 KB, patch
Details | Diff | Splinter Review
6.31 KB, patch
ted
: review+
beltzner
: approval1.9.2+
Details | Diff | Splinter Review
497 bytes, patch
Details | Diff | Splinter Review
Right now because of our awesome ed script patching, you can only build jemalloc on Win32 with exactly VC8 SP1.  I think if we generated individual diffs and named them by version, we could support VC8, VC8SP1, and VC9.  I don't know how much effort actually patching the different versions of the source would be, but I'm pretty sure stuart had it working on VC8 at one point.  No gain for our official builds here, but could be useful for the community at large.
Severity: normal → enhancement
I don't think we should spend time on this. Instead, the moz22 focus should be on not using the standard allocator (malloc) at all and replacing calls to it with moz_malloc or the new MMgc allocator.
And while that may be the right thing to do for Moz2, the 1.9 branch is going to be used for people for a while after we ship, so making it easier for them to work outside of our exact refplatform would be nice.  I agree that we shouldn't spend much time on it though.
It may be worth supporting different major versions, but I don't see the point in supporting VC8 SP0 since SP1 is a free update anyway.
I have tried building with the VC8SP1 CRT and VC9Exp Compiler and the resulting build seems to be hanging a lot for me. Maybe that has something to do with mixing different versions of the CRT and compiler.

PS: If you guys need the CRT sources from VC9 I think the 90 day trial version should be enough.
I am also getting random just in time debugging prompts/errors with the above configuration. I initially thought they were originating from another program that's why I didn't mention it.
This guy apparently has a patch to build on VC9:
http://marilab.hp.infoseek.co.jp/buildfx/index_en.html

I've emailed him about submitting it here, the rest of the build work should be trivial.
Attached patch Makefile.in diffSplinter Review
I check this patch, using VC9 Pro Jpn ver.
Thanks for the patch! I'll put together a build patch to select the proper CRT patch based on your compiler version.
Just tried the patch with VC9 Pro Eng. Working great over here
preed said he might be interested in this after he finishes bug 429745. I think the CRT patch here is ok (might need some parallel tweaking from bug 429745), but the Makefile patch needs to select the right patch at configure or build time given the compiler version.
I chekced patch with VC9 SP1 Jpn ver.
I remove "include __forceCRTManifestRTM" from crtexe.c and crtdll.c
(In reply to comment #11)
> preed said he might be interested in this after he finishes bug 429745. I think
> the CRT patch here is ok (might need some parallel tweaking from bug 429745),
> but the Makefile patch needs to select the right patch at configure or build
> time given the compiler version.

Most surely; let me know how I can help.
The windows mobile builds are using VS9 and might benefit from jemalloc
Someone should figure out if jemalloc works at all on wince, first.
Assignee: nobody → crowder
(In reply to comment #16)
> Someone should figure out if jemalloc works at all on wince, first.

errr.... we already have working WinCE builds being produced using VS9. Dont know if WinCE uses jemalloc, but that seems orthogonal to getting the Makefile change for VS9 landed. After all, if mobile are using jemalloc, it must be already working with VS9 in the working WinCE builds; if mobile are not using jemalloc, then they dont mind either way!?! 

cc-ing brad/stuart/aki in case I'm missing something.
WinCE is not using jemalloc currently. My comment was merely to point out to mfinkle that there might be other things to worry about first. Per recent IRC discussion, the patch here won't help WinCE one bit, since the WinCE compiler doesn't have the same kind of CRT as the desktop compiler anyway, so we will need to sort out a different way of hooking the allocator. That is not this bug.
Assignee: crowder → nobody
What Ted said -- This bug isn't related to Windows CE at all.
This patch for Fx3.6a
I threw together the crt patch from ayakawa.m@gmail.com and modified configure.in and Makefile.in to pick the right patch. We can support multiple compiler versions without too much trouble. We now support vc8 SP1 and vc9 SP1.

Unfortunately, the compiler version number for sp1 and non-sp1 is the same so we have no way to distinguish them (except that the vc9 sp1 patch doesn't apply). While we don't have a policy on service packs for compilers, I don't think it's unreasonable at this point to require the latest version of vc9 to compile with jemalloc.

This should apply cleanly on m-c, tested on win2k3 (vc8) and windows 7 (vc9).
Assignee: nobody → tellrob
Status: NEW → ASSIGNED
Attachment #394076 - Flags: review?
Attachment #394076 - Flags: review? → review?(pavlov)
Comment on attachment 394076 [details] [diff] [review]
patch to autodetect compiler version and select proper patch file

Needs my review, there's nothing here for stuart to review. Will get to it soon.
Attachment #394076 - Flags: review?(pavlov) → review?(ted.mielczarek)
Attached patch revised patchSplinter Review
Found a typo. Now should build properly on vc8.
Attachment #394076 - Attachment is obsolete: true
Attachment #394409 - Flags: review?(ted.mielczarek)
Attachment #394076 - Flags: review?(ted.mielczarek)
If I were to send you a unified diff of the updated CRT patch from bug 508861, do you think you could port those changes to the VC9 CRT and regenerate the CRT patch? (the goofy looking patch is just a diff -re) I don't have VC9 pro installed currently, and we're going to need to do that to land both of these patches. If you can't or don't want to do that, I think I have a copy I can install.
I don't have time to do it. I made this patch while concurrently trying out a fix for vc8 for bug 505504.

Note that we don't have a way to determine the service pack level (that I know of) so be sure you pick the right vc9 version. Mobile uses vc9 rtm, but this patch (and my machine) use vc9 sp1 and the diff does break between the two versions. Stuart has said he is not opposed to upgrading the mobile compiler to sp1.
Attachment #394409 - Flags: review?(ted.mielczarek) → review+
Comment on attachment 394409 [details] [diff] [review]
revised patch

+ifeq ($(CC_VERSION), 14.00.50727.762)
+CRTDIFF=crtvc8sp1.diff
+else
+CRTDIFF=crtvc9sp1.diff
+endif

Could you make this specifically check every version? I know you do that in configure already, but a little double-checking won't hurt (and might save someone's sanity down the line). Something like:
ifeq ($(CC_VERSION), 14.00.50727.762)
CRTDIFF=crtvc8sp1.diff
else
ifeq ($(CC_VERSION), ...)
CRTDIFF=crtvc9sp1.diff
else
$(error Compiler version $CC_VERSION) not supported!)
endif
endif

+# Binaries are available under separate licenses at 
+# http://www.microsoft.com/downloads/details.aspx?familyid=200b2fd9-ae1a-4a14-984d-389c36f85647&displaylang=en
+# convert for VC9 SP1 by Takeshi Ichimaru
+# Microsoft Visual C++ 2008 Redistributable Package (x86) Binaries are available under
+# separate licenses at 
+# http://www.microsoft.com/downloads/details.aspx?familyid=9B2DA534-3E03-4391-8A4D-074B9F2BC1BF&displaylang=en

I know you didn't write this, but please fix the comment here. Drop the first link, and just link to the VC9 redist package.
Blocks: 508861
No longer blocks: 508861
Just tried this patch, with --enable-optimize and --enable-jemalloc, and got:

*** Creating linker response file build\intel\dll_obj\linkp.rsp
*** Creating linker response file build\intel\dll_obj\implibp.rsp
        rc -l 409 -r -Fo build\intel\sample_p.res -D_MBCS -D_MB_MAP_DIRECT -D_CRTBLD -D_FN_WIDE -DWINHEAP -D_RTC -DWIN32_LEAN_AND_MEAN -D_WIN32_WINNT=0x0501 -D_WIN32_WINDOWS=0x501 -DNTDDI_VERSION=0x05010000 -D-D-DNOSERVICE -I"C:\Program Files\Microsoft SDKs\Windows\v7.0\include" sample_p.rc
Microsoft (R) Windows (R) Resource Compiler Version 6.1.7600.16385
Copyright (C) Microsoft Corporation.  All rights reserved.

        link -nologo -NXCOMPAT -DYNAMICBASE @build\intel\dll_obj\linkp.rsp
   Creating library build\intel\dll_obj\tmpp.lib and object build\intel\dll_obj\tmpp.exp
stdhndlr.obj : error LNK2019: unresolved external symbol "int (__cdecl*__cdecl _set_new_handler(int (__cdecl*)(unsigned int)))(unsigned int)" (?_set_new_handler@@YAP6AHI@ZP6AHI@Z@Z) referenced in function "void (__cdecl*__cdecl std::set_new_handler(void (__cdecl*)(void)))(void)" (?set_new_handler@std@@YAP6AXXZP6AXXZ@Z)
build\intel\sample_p.dll : fatal error LNK1120: 1 unresolved externals
Yeah, this patch needs updated. If it's blocking you on something, I can find a copy of VC9 and update it.
Comment on attachment 400175 [details] [diff] [review]
full patch for vc9sp1

+++ b/memory/jemalloc/crtvc9sp1.diff
@@ -0,0 +1,218 @@
+# The Microsoft C Runtime source code to which this document refers is available
+# directly from Microsoft Corporation, under a separate license.
+# Please ensure that if you are using that source code, you have appropriate
+# rights to use it.  By providing you access to this file, Mozilla Corporation
+# and its affiliates do not purport to grant any rights in that source code. 
+# Binaries are available under separate licenses at 
+# http://www.microsoft.com/downloads/details.aspx?familyid=200b2fd9-ae1a-4a14-984d-389c36f85647&displaylang=en

Just fix the link here to point to:
http://www.microsoft.com/downloads/details.aspx?familyid=9B2DA534-3E03-4391-8A4D-074B9F2BC1BF&displaylang=en

otherwise this looks fine.
Attachment #400175 - Flags: review+
http://hg.mozilla.org/mozilla-central/rev/cd6dde027905

with URL fix.
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Summary: allow building jemalloc with other versions of VC++ → allow building jemalloc with VC9 SP1
Comment on attachment 400175 [details] [diff] [review]
full patch for vc9sp1

Need this on 1.9.2 to cleanly land the about:memory patch. Ted says it should be fine for 192.
Attachment #400175 - Flags: approval1.9.2?
Comment on attachment 400175 [details] [diff] [review]
full patch for vc9sp1

a192=beltzner
Attachment #400175 - Flags: approval1.9.2? → approval1.9.2+
in crtvc9sp1.diff, I forgat to remove "-arch:SSE2" option on makefile.sub
we need to remove this....
(In reply to comment #40)

> in crtvc9sp1.diff, I forgat to remove "-arch:SSE2" option on makefile.sub
> we need to remove this....

Please file a separate bug for doing this, mark it as blocking this bug.
Depends on: 521182
(In reply to comment #42)
> Please file a separate bug for doing this, mark it as blocking this bug.

bug521182
Blocks: 531283
Please update https://developer.mozilla.org/En/Windows_Build_Prerequisites and its kin for the changes in this bug.
Flags: in-testsuite-
Target Milestone: --- → mozilla1.9.3a1
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.