416117 - allow building jemalloc with VC9 SP1

Status: RESOLVED FIXED

(Firefox Build System :: General, enhancement)

Description

[(not currently active) Ted Mielczarek]

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.

Comment 1

[Benjamin Smedberg]

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.

Comment 2

[(not currently active) Ted Mielczarek]

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.

Comment 3

[Ryan VanderMeulen [:RyanVM]]

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.

Comment 4

[atzaus]

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.

Comment 5

[atzaus]

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.

Comment 6

[(not currently active) Ted Mielczarek]

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.

Comment 7

[ICHIMARU Takeshi(a.k.a. Ayakawa)]

Attachment: patchfile for vc9

Comment 8

[ICHIMARU Takeshi(a.k.a. Ayakawa)]

Attachment: Makefile.in diff

I check this patch, using VC9 Pro Jpn ver.

Comment 9

[(not currently active) Ted Mielczarek]

Thanks for the patch! I'll put together a build patch to select the proper CRT patch based on your compiler version.

Comment 10

[atzaus]

Just tried the patch with VC9 Pro Eng. Working great over here

Comment 11

[(not currently active) Ted Mielczarek]

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.

Comment 12

[ICHIMARU Takeshi(a.k.a. Ayakawa)]

Attachment: patchfile for vc9 SP1

Comment 13

[ICHIMARU Takeshi(a.k.a. Ayakawa)]

Attachment: Makefile.in diff for VC9 SP1

I chekced patch with VC9 SP1 Jpn ver.
I remove "include __forceCRTManifestRTM" from crtexe.c and crtdll.c

Comment 14

[J. Paul Reed [:preed]]

(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.

Comment 15

[Mark Finkle (:mfinkle) (use needinfo?)]

The windows mobile builds are using VS9 and might benefit from jemalloc

Comment 16

[(not currently active) Ted Mielczarek]

Someone should figure out if jemalloc works at all on wince, first.

Comment 17

[John O'Duinn [:joduinn] (please use "needinfo?" flag)]

(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.

Comment 18

[(not currently active) Ted Mielczarek]

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.

Comment 19

[Stuart Parmenter]

What Ted said -- This bug isn't related to Windows CE at all.

Comment 20

[ICHIMARU Takeshi(a.k.a. Ayakawa)]

Attachment: Makefile.in diff for VC9 SP1, for Shiretoko

Comment 21

[ICHIMARU Takeshi(a.k.a. Ayakawa)]

Attachment: Makefile.in diff for VC9 SP1, for trunk

This patch for Fx3.6a

Comment 22

[ICHIMARU Takeshi(a.k.a. Ayakawa)]

Attachment: Makefile.in diff for VC9 SP1, for trunk

Comment 23

[ICHIMARU Takeshi(a.k.a. Ayakawa)]

Attachment: configure.in diff for VC9 SP1, for trunk

Comment 24

[ICHIMARU Takeshi(a.k.a. Ayakawa)]

Attachment: patchfile for VC9 SP1, for trunk

Comment 25

[Rob Arnold [:robarnold]]

Attachment: patch to autodetect compiler version and select proper patch file

I threw together the crt patch from [e-mail] 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).

Comment 26

[(not currently active) Ted Mielczarek]

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.

Comment 27

[Rob Arnold [:robarnold]]

Attachment: revised patch

Found a typo. Now should build properly on vc8.

Comment 28

[(not currently active) 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.

Comment 29

[Rob Arnold [:robarnold]]

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.

Comment 30

[(not currently active) Ted Mielczarek]

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.

Comment 31

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

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

Comment 32

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Seems to be caused by bug 508861.

Comment 33

[(not currently active) Ted Mielczarek]

Yeah, this patch needs updated. If it's blocking you on something, I can find a copy of VC9 and update it.

Comment 34

[ICHIMARU Takeshi(a.k.a. Ayakawa)]

Attachment: patchfile for VC9 SP1 w/ bug508861

Comment 35

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Attachment: full patch for vc9sp1

Includes all the configure goop.

Comment 36

[(not currently active) Ted Mielczarek]

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.

Comment 37

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

http://hg.mozilla.org/mozilla-central/rev/cd6dde027905

with URL fix.

Comment 38

[Justin Dolske [:Dolske]]

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.

Comment 39

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 400175 [details] [diff] [review]
full patch for vc9sp1

a192=beltzner

Comment 40

[ICHIMARU Takeshi(a.k.a. Ayakawa)]

Attachment: remove -arch:SSE2 from crtvc9sp1.diff

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

Comment 41

[Justin Dolske [:Dolske]]

Pushed http://hg.mozilla.org/releases/mozilla-1.9.2/rev/04a6dc001571

Comment 42

[Justin Dolske [:Dolske]]

(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.

Comment 43

[ICHIMARU Takeshi(a.k.a. Ayakawa)]

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

bug521182

Comment 44

[Nick Thomas [:nthomas] (UTC+12)]

Please update https://developer.mozilla.org/En/Windows_Build_Prerequisites and its kin for the changes in this bug.