487396 - fix some of the jemalloc windows build madness

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[(not currently active) Ted Mielczarek]

When I originally implemented the jemalloc windows CRT build, it was under time pressure and so I settled on a less-than-stellar implementation. preed's patches in bug 429745 fix a lot of the stupid, but they're intermingled with a lot of other stuff to make the debug CRT work. I'm going to separate out just the build hygiene bits and get that landed, since it will make life nicer regardless of whether we support the debug CRT. The primary motivation here is to fix this for pymake instead of using the patch on bug 485404.

Comment 1

[(not currently active) Ted Mielczarek]

Attachment: clean up some build hacks

This patch does a few things, mostly taken from the patches on bug 429745:
1) Stop setting LIB/PATH, and instead pass -NODEFAULTLIB:msvcrt -LIBPATH:... -DEFAULTLIB:mozcrt19 (+ slight tweak to the CRT patch to rename the import lib we build)
2) Some general cleanup, remove the option to use a pre-built CRT DLL (which I doubt anyone was using anyway)
3) Actually test for exactly VC8SP1 in configure
4) Remove most of the Win32 jemalloc stuff from js/src/configure.in, since it wouldn't really work standalone anyway. Just pass the LDFLAGS down from the top-level configure and use those.
5) sprinkle some more USE_STATIC_LIBS around

I haven't done a full build with this patch yet, but it builds jemalloc, nspr, js and xpcom ok, so that's a good start.

Comment 2

[(not currently active) Ted Mielczarek]

Comment on attachment 371656 [details] [diff] [review]
clean up some build hacks

Builds fine for me on Windows.

Comment 3

[Benjamin Smedberg]

Comment on attachment 371656 [details] [diff] [review]
clean up some build hacks

I'm a little concerned: how do the jemalloc cflags get passed down to NSPR?

Comment 4

[(not currently active) Ted Mielczarek]

In DLLFLAGS, just like before, which is why it gets exported in configure. (There are no CFLAGS, only LDFLAGS.)

Comment 5

[Nelson Bolyard (seldom reads bugmail)]

In reply to:
> 2) Some general cleanup, remove the option to use a pre-built CRT DLL 
> (which I doubt anyone was using anyway)
> 4) Remove most of the Win32 jemalloc stuff from js/src/configure.in, since 
> it wouldn't really work standalone anyway. Just pass the LDFLAGS down from 
> the top-level configure and use those.

I'm concerned about these changes.

The NSS and NSPR developers are trying to, and desire to, be able to build
mozcrt19 stand-alone, and use pre-built mozcrt19 in their developer builds
of NSS and NSPR on Windows.

Most of the developers of NSS and NSPR _NEVER_ build Firefox or Thunderbird,
but DO desire to test their own developer builds of NSS+NSPR with FF and TB 
nightlies built by MoCo.  We did that for years, until Mozilla started to use mozcrt19 on Windows.

Today, this is a huge problem on Windows because stand-alone developer builds
of NSS and NSPR do not use MOZCRT19, while Mozilla's builds of NSS and NSPR
(built as part of Firefox) DO use MOZCRT19.  

Also, the NSS + NSPR developers do NOT all (or even mostly) use Mozilla's 
package of build tools for Windows.  In particular, we use neither MSYS shell 
nor any of the MSYS utilities.  We use MKS.  This means that we use source 
files and shell scripts with Windows line endings, not Unix line endings.  
This has generally NOT been a problem, since the standard (not MSYS) version 
of CVS for Windows handles line ending conversions nicely.  But it has become 
a problem for building mozcrt19, because those files assume and require MSYS 
on Windows.  In particular, they assume the use of Unix line endings on 
Windows. (That problem is slowly pervading Firefox's own use of text files on Windows, I might add.)

Also, most of the NSS and NSPR team members don't use Hg at all, because of 
Hg's "all or nothing" mentality with the repository.  (Want a single file?
download the ENTIRE repository. SHeesh!) We continue to use cvs exclusively.

So, to deal with this problem of out stand-alone builds not working with Mozilla nightlies, the NSS developers (primarily me) have been been working 
on making it possible to 

A) CVS checkout mozilla/memory/jemalloc by itself,
B) build it "stand alone" and then 
C) build our stand-alone NSS and NSPR with the built mozcrt19. 

A was easy, using the upstream (not MSYS) cvs client for windows.  
B was a chore, due to dealing with the line endings problems, and a few 
other details.  
   - Use of python is COMPLETELY unnecessary, and I don't do it.
   - It's not clear which directory names should be put into several of 
     the required shell/make variables.  I figured some of them out by 
     trial and error.
C is the next step.  

I fear that the changes proposed in the text I quoted above is actually 
moving mozcrt19 in the direction of making it harder for the NSS and NSPR 
teams to achieve their goals with mozcrt19.  

So, the message is:
a) please do NOT make it more difficult to build or use mozcrt19 "stand alone"
and 
b) please do not make building mozcrt19 more dependent on MSYS, and 
c) please do not abandon the jemalloc sources in CVS.

Comment 6

[Benjamin Smedberg]

Nelson, it should be possible to mix NSS-without-jemalloc and mozilla-with-jemalloc. If it's not we have bugs with mismatched allocators that we should just fix.

In order to build NSS with jemalloc, you'd just have to pass appropriate linker flags, which would be:

-MANIFEST:NO -LIBPATH:"$WIN32_CUSTOM_CRT_DIR" -NODEFAULTLIB:msvcrt -NODEFAULTLIB:msvcrtd -DEFAULTLIB:mozcrt19

Obviously this patch is not going to affect the CVS version of jemalloc, because we don't use CVS any more.

Comment 7

[(not currently active) Ted Mielczarek]

Also, the bits you quoted only affect:
1) building Firefox with a pre-built jemalloc DLL
and
2) building spidermonkey, which it seems unlikely that you would care about

None of this should affect NSPR or NSS, they will continue to be controlled purely by DLLFLAGS from the Firefox build as they were before.

Comment 8

[Nelson Bolyard (seldom reads bugmail)]

Benjamin,  
> it should be possible to mix NSS-without-jemalloc and mozilla-with-jemalloc.

SHOULD is the operative word there.  
But it ISN'T possible, and hasn't been since the advent of mozcrt19.
When I attempt it, my browser stays up for rather few minutes at MOST.

> If it's not we have bugs with mismatched allocators that we should just fix.

Right, LOTS of them in Firefox.  It's not acceptable to the NSS developers to 
have to wait any longer for them to be fixed before we can test with Moz FF 
nightlies.

Comment 9

[(not currently active) Ted Mielczarek]

Pushed to m-c:
http://hg.mozilla.org/mozilla-central/rev/7e579ac2ae03

Comment 10

[Turuoka (Kyoya Sakaguchi)]

Attachment: correct version string comparison

*** to the full path to pkg-config.
*** Or see http://www.freedesktop.org/software/pkgconfig to get pkg-config.
checking for valid optimization flags... yes
/e/moz-src/pgo/mozilla-central/configure: test: 14.00.50727.762: integer express
ion expected
checking for stdint.h... (cached) no
checking for inttypes.h... (cached) no
checking for sys/int_types.h... (cached) no

Comment 11

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 373639 [details] [diff] [review]
correct version string comparison

Good catch!  
I'd give this r+ if I was a peer.

Comment 12

[J. Paul Reed [:preed]]

FWIW, you may want to consider making this emit mozcrt191 (and using that as the appropriate -DEFAULTLIB argument), to annotate the fact that there have been substantive changes to what is branded as the "Mozilla Win32 CRT" since 1.9.0.x shipped.

(Or, alternatively, do we set the version information appropriately anywhere?)

Comment 13

[(not currently active) Ted Mielczarek]

What do you mean "substantive changes"? Aside from some fixes in jemalloc (which don't look like much more than bug fixes), we're shipping the same VC8 SP1 CRT we have been. We use the version information that comes with the CRT, 8.0, since I don't think we've changed enough to bother changing that.

Comment 14

[(not currently active) Ted Mielczarek]

Comment on attachment 373639 [details] [diff] [review]
correct version string comparison

Sorry, my bad. I'll push this fix for you.

Comment 15

[(not currently active) Ted Mielczarek]

Pushed followup to m-c:
http://hg.mozilla.org/mozilla-central/rev/5ce2bb1c6357

Comment 16

[Nelson Bolyard (seldom reads bugmail)]

Please push this to CVS, too.

Comment 17

[(not currently active) Ted Mielczarek]

I haven't even pushed it to 1.9.1. As far as I'm concerned, it's just cleanup, so it's not really necessary on those branches.

Comment 18

[Ryan VanderMeulen [:RyanVM]]

Possible the following build error came from these patches?
LINK : warning LNK4098: defaultlib 'LIBCMT' conflicts with use of other libs; use /NODEFAULTLIB:library

Comment 19

[(not currently active) Ted Mielczarek]

That's pretty vague. Could you give me more context for that warning?

Comment 20

[Ryan VanderMeulen [:RyanVM]]

I get that during the libxul linking phase of the build. VC8SP1. jemalloc enabled.

Comment 21

[Hiroyuki Ikezoe (:hiro)]

I've opened Bug 489273 that is the same in comment #18.

Comment 22

[Ben Combee (:bcombee)]

I'm also seeing this problem; I updated my moz-central tree to latest and started getting the duplicate symbol and bad CRT errors when building fennec for Win32.  It's happening at libxul.dll link time.

Comment 23

[J. Paul Reed [:preed]]

(In reply to comment #13)
> What do you mean "substantive changes"? Aside from some fixes in jemalloc
> (which don't look like much more than bug fixes),

I think those bug fixes are probably relevant, but also the fact that we build it differently enough now to link specifically against it (and not a fake msvcrt). (Although, I had assumed you took all the .def file goo from the original patch to export the symbols for the .dll; so maybe it doesn't matter, since that part got left behind). 

This is mostly relevant for people who consume mozcrt (NSS, NSPR, other projects, etc.), and need to know more specific information about what did (or didn't) change between particular releases, especially as it may relate to certification or security issues.

We've traditionally been, for lack of a better word, sloppy about this. It would be nice to be cleaner about it because it's important to other people who are using the library.