All users were logged out of Bugzilla on October 13th, 2018

--enable-jemalloc + --enable-debug fails (unresolved posix_memalign symbol in jsgc.c)

RESOLVED WORKSFORME

Status

()

--
major
RESOLVED WORKSFORME
11 years ago
3 years ago

People

(Reporter: preed, Unassigned)

Tracking

Trunk
x86
Windows XP
Points:
---
Dependency tree / graph
Bug Flags:
wanted1.9.2 +
wanted1.9.0.x -

Firefox Tracking Flags

(status1.9.1 wontfix)

Details

Attachments

(6 attachments, 6 obsolete attachments)

(Reporter)

Description

11 years ago
When building xulrunner with --enable-debug and --enable-jemalloc, the build fails in jsgc.c with:

link -NOLOGO -DLL -OUT:js3250.dll -PDB:js3250.pdb -SUBSYSTEM:WINDOWS  jsapi.obj jsarena.obj jsarray.obj jsatom.obj jsbool.obj jscntxt.obj jsdate.obj jsdbgapi.obj jsdhash.obj jsdtoa.obj jsemit.obj jsexn.obj jsfun.obj jsgc.obj jshash.obj jsinterp.obj jsinvoke.obj jsiter.obj jslock.obj jslog2.obj jslong.obj jsmath.obj jsnum.obj jsobj.obj jsopcode.obj jsparse.obj jsprf.obj jsregexp.obj jsscan.obj jsscope.obj jsscript.obj jsstr.obj jsutil.obj jsxdrapi.obj jsxml.obj prmjtime.obj
 js3240.res -NXCOMPAT -SAFESEH -DYNAMICBASE -MANIFEST:NO  -DEBUG -DEBUGTYPE:CV
       ../../dist/lib/nspr4.lib ../../dist/lib/plc4.lib ../../dist/lib/plds4.lib
  kernel32.lib user32.lib gdi32.lib winmm.lib wsock32.lib advapi32.lib
   Creating library js3250.lib and object js3250.exp
jsgc.obj : error LNK2019: unresolved external symbol _posix_memalign referenced in function _NewGCChunk
js3250.dll : fatal error LNK1120: 1 unresolved externals
make[4]: *** [js3250.dll] Error 96
make[4]: Leaving directory `/c/builds/jemalloc/pristine/mozilla/compiled/xulrunner-debug/js/src'
make[3]: *** [libs_tier_js] Error 2
make[3]: Leaving directory `/c/builds/jemalloc/pristine/mozilla/compiled/xulrunner-debug'
make[2]: *** [tier_js] Error 2
make[2]: Leaving directory `/c/builds/jemalloc/pristine/mozilla/compiled/xulrunner-debug'
make[1]: *** [default] Error 2
make[1]: Leaving directory `/c/builds/jemalloc/pristine/mozilla/compiled/xulrunner-debug'
make: *** [build] Error 2

After spending *way* too much time learning about Win32 DLL linking semantics ;-), it looks like the problem is that when we patch the Win32 CRT for jemalloc, we keep the RETAIL_LIB_NAME as msvcrt.

In debug builds, the linker is looking for msvcrtd (and we're not passing the right -DEFAULTLIB/-NODEFAULTLIB combination(s)), so the link fails, even though the symbol is (mostly) properly exported.

I've got a patch that does get it building in debug mode with jemalloc, but I don't like it for a number of reasons. I'll go into all of them when I post the patch.

Comment 1

11 years ago
I am seeing this building the debug browser with jemalloc as well 

Updated

11 years ago
Blocks: 432729
(Reporter)

Comment 2

11 years ago
I have a patch that fixes this.

Unfortunately, it's pretty lengthy, and I need to clean it up to remove some copy/paste code from the CRT itself.

I'll try to do that by the end of the week.
(Reporter)

Comment 3

11 years ago
Created attachment 320202 [details] [diff] [review]
The CRT Patch, take 1

Here's the CRT patch (direct replacement for memory/jemalloc/crtsp1.diff) that we're "sorta" shipping with; I had to redo some of the preprocessor magic (pronounced: "crap") we're doing here, and I didn't do a full rebuild CRT + XULRunner + Songbird run; I *think* I got it mostly right (I diffed the .i files; whee! They looked Ok.)

This can probably be cleaned up a bit, since after I got it working, I was like "don't touch it!" :-)

I've got a patch for the build system guts-part itself... I'll post that in a sec.
Assignee: nobody → mozpreed
Status: NEW → ASSIGNED
(Reporter)

Comment 4

11 years ago
Created attachment 320206 [details] [diff] [review]
Build system gut-ectomy, Take 1

Goes with the above CRT patch.
(Reporter)

Comment 5

11 years ago
Created attachment 320209 [details] [diff] [review]
More tool munging, take 1

I need to poke at this more, as, again, what I had was working.

This (supposedly) disables jemalloc for certain executables that probably should be linked against the standard runtimes, no matter what (i.e. crashreporter, etc.)

But, after some limited testing, I don't think it's 100% correct; comments from ted or bsmedberg appreciated.
(Reporter)

Comment 6

11 years ago
Changing summary, since this looks like it's not XR-only.
Summary: XULRunner with --enable-jemalloc and --enable-debug fails (unresolved posix_memalign symbol in jsgc.c) → --enable-jemalloc + --enable-debug fails (unresolved posix_memalign symbol in jsgc.c)

Comment 7

11 years ago
$ patch -p0 --dry-run < 429745.p2.v1.patch 
patching file configure.in
Hunk #2 FAILED at 6196.
1 out of 2 hunks FAILED -- saving rejects to file configure.in.rej
patching file config/autoconf.mk.in
Hunk #1 succeeded at 583 (offset 4 lines).
patching file config/config.mk
patch: **** malformed patch at line 166: Index: memory/jemalloc/Makefile.in
(Reporter)

Comment 8

11 years ago
(In reply to comment #7)
> $ patch -p0 --dry-run < 429745.p2.v1.patch 
> patching file configure.in
> Hunk #2 FAILED at 6196.
> 1 out of 2 hunks FAILED -- saving rejects to file configure.in.rej
> patching file config/autoconf.mk.in
> Hunk #1 succeeded at 583 (offset 4 lines).
> patching file config/config.mk
> patch: **** malformed patch at line 166: Index: memory/jemalloc/Makefile.in

Hey bc,

This patch was against 1.9b5, not head of trunk; I can look at updating the patch, but can you pull 1.9b5, and try to apply it there real quick?

Comment 9

11 years ago
against FIREFOX_3_0b5_RELEASE

$ patch -p0 --dry-run < 429745.p2.v1.patch 
patching file configure.in
patching file config/autoconf.mk.in
patching file config/config.mk
patch: **** malformed patch at line 166: Index: memory/jemalloc/Makefile.in

it's not obvious to me what the malformation is... looking.

$ patch -p0 --dry-run < 429745.p3.v1.patch 
patching file config/mkdepend/Makefile.in
patching file modules/libmar/tool/Makefile.in
patching file toolkit/crashreporter/client/Makefile.in
patching file xpcom/typelib/xpidl/Makefile.in
patching file xpcom/typelib/xpt/tools/Makefile.in

Comment 10

11 years ago
Stuart, can you comment here about preed's approach and also document your "by-hand" method for getting debug builds to work with je-malloc on windows?

(Reporter)

Comment 11

11 years ago
(In reply to comment #9)
> against FIREFOX_3_0b5_RELEASE
> 
> $ patch -p0 --dry-run < 429745.p2.v1.patch 
> patching file configure.in
> patching file config/autoconf.mk.in
> patching file config/config.mk
> patch: **** malformed patch at line 166: Index: memory/jemalloc/Makefile.in
> 
> it's not obvious to me what the malformation is... looking.

I caught the problem; I made a stupid edit to the patch before posting it (further up in the file).

I'll attach a new patch against b5 as soon as the "cvs -q di" finishes running over the tree. :-)

A couple of other points of clarification:

1. I'm happy to update this patch to head; I just didn't do that because I didn't have the free time, and don't want to do it unless it was asked for.

2. I understand there's some concern about the scope of change in this patch, and that's completely understandable; I'm happy to recode this patch as:

ifdef MOZ_DEBUG || MOZ_NEW_CRT_MODE
   ... my patch ...
else
   ... old style ...

The nice thing about this is that it gets us debug builds, and makes it easier to make a switch to a mozcrt19.dll that identifies itself as such (which, AIUI, is a large part of the issue) for 1.9.next, but eliminates risk to Fx 3.0.x.y.
(Reporter)

Comment 12

11 years ago
Created attachment 320595 [details] [diff] [review]
Build system gut-ectomy, Take 2

bc: this should fix the patch errors you were seeing; the patch was invalid; it was my bad:

preed@preed-desktop:~/checkouts/test/mozilla$ patch -p0 --dry-run < ~/patches/mozilla/429745.p2.v2.patch 
patching file configure.in
patching file config/autoconf.mk.in
patching file config/config.mk
patching file memory/jemalloc/Makefile.in
patching file memory/jemalloc/build-crt.py
patching file memory/jemalloc/jemalloc.c
patching file toolkit/xre/nsAppRunner.cpp

You *might* see some fuz on xre/nsAppRunner.cpp; it should apply, though.
Attachment #320206 - Attachment is obsolete: true

Comment 13

11 years ago
yes, that applies cleanly. thanks.
(Reporter)

Updated

11 years ago
Attachment #320202 - Flags: review?(ted.mielczarek)
(Reporter)

Updated

11 years ago
Attachment #320209 - Flags: review?(ted.mielczarek)
(Reporter)

Updated

11 years ago
Attachment #320595 - Flags: review?(ted.mielczarek)

Comment 14

11 years ago
fyi, I get build failures on opt and debug with

make[5]: Entering directory `/c/work/mozilla/builds/1.9.0-jemalloc-test/mozilla/firefox-debug/config/mkdepend'
make[5]: echo: Command not found
(In reply to comment #14)
> make[5]: echo: Command not found

I've seen things like this before, usually means your path is messed up somehow in make. Try reconfiguring?

yeah, the jemalloc build explicitly sets PATH in autoconf.mk.

Comment 17

11 years ago
Created attachment 320973 [details]
build errors

I've tried and tried to get this to build, but keep failing. This is with a fresh tree checked out for Firefox 3.0b5 release with the patches applied and configure rebuilt with autoconf-2.13.
(Reporter)

Comment 18

11 years ago
(In reply to comment #17)
> Created an attachment (id=320973) [details]
> build errors
> 
> I've tried and tried to get this to build, but keep failing. This is with a
> fresh tree checked out for Firefox 3.0b5 release with the patches applied and
> configure rebuilt with autoconf-2.13.

bc: I ran into this error while developing the patch, and I'm trying to remember exactly what caused ed.exe to barf; as I remember, it was something relatively stupid (the diff done in the wrong directory, etc.)

Did you drop the crt patch right into the same place directly, or?

I'm updating this patch to rc1 as we speak, since we're picking up new XulRunners right now.

I can post the new patch, although the only thing that changed that was interesting was the configure.in stuff.

Comment 19

11 years ago
(In reply to comment #18)
> 
> Did you drop the crt patch right into the same place directly, or?

I overwrote crtsp1.diff

> I can post the new patch, although the only thing that changed that was
> interesting was the configure.in stuff.

rc1==trunk right? Yeah, I would like to try that out since I don't care about b5 any more ;-)
(Reporter)

Comment 20

11 years ago
(In reply to comment #17)
> Created an attachment (id=320973) [details]
> build errors
> 
> I've tried and tried to get this to build, but keep failing. This is with a
> fresh tree checked out for Firefox 3.0b5 release with the patches applied and
> configure rebuilt with autoconf-2.13.

After working through the problem with bc, I think we figured out that his version of the CRT isn't the VS8SP1, or at least doesn't have the right combo of SDKs and service packs:

http://pastebin.mozilla.org/432345

I'm working on an rc1-compliant patch right now; will post when it's ready.

Comment 21

11 years ago
preed, can you tell me what updates you've applied to visual studio 2005?
(Reporter)

Comment 22

11 years ago
(In reply to comment #21)
> preed, can you tell me what updates you've applied to visual studio 2005?

For visual studio, I've applied:

* Security Update for Microsoft Visual Studio 2005 (KB937060)
* Visual Studio 2005 Service Pack 1
* Security Update for Microsoft Visual Studio 2005 (KB925674)

All of these were applied via Windows update.
(Reporter)

Comment 23

11 years ago
Created attachment 321336 [details] [diff] [review]
The CRT patch, take 2

Changes some instances of MOZ_DEBUG to MOZ_MEMORY_DEBUG because not all parts of the code (js, notably) define MOZ_DEBUG when in debug mode. MOZ_MEMORY_DEBUG, however, does get defined.

(We also propagate MOZ_MEMORY_DEBUG down into the CRT build environment now.)
Attachment #320202 - Attachment is obsolete: true
Attachment #321336 - Flags: review?(ted.mielczarek)
Attachment #320202 - Flags: review?(ted.mielczarek)
(Reporter)

Comment 24

11 years ago
Created attachment 321337 [details] [diff] [review]
Build system gut-ectomy, Take 3

Updated for 3.0rc1.
Attachment #320595 - Attachment is obsolete: true
Attachment #321337 - Flags: review?(ted.mielczarek)
Attachment #320595 - Flags: review?(ted.mielczarek)
Comment on attachment 320209 [details] [diff] [review]
More tool munging, take 1

So I know I've been putting off reviewing this, I apologize for that. What does this particular patch do that USE_STATIC_LIBS=1 doesn't do? I notice that one of the Makefiles you're changing here already has that present. Does it not work as expected with jemalloc enabled?
(Reporter)

Comment 26

11 years ago
(In reply to comment #25)
> (From update of attachment 320209 [details] [diff] [review])
> So I know I've been putting off reviewing this, I apologize for that. What does
> this particular patch do that USE_STATIC_LIBS=1 doesn't do? I notice that one
> of the Makefiles you're changing here already has that present. Does it not
> work as expected with jemalloc enabled?

We chatted on IRC about this, but beyond the "getting it building in debug"-part, I (mostly, it turns out; not entirely) wanted to separate the concept of building statically and building against jemalloc.

That's why there's now "DISABLE_MOZ_MEMORY" in addition to USE_STATIC_LIBS. (Right now, it's somewhat confusing that USE_STATIC_LIBS automatically implies linking against the standard CRT, and the fact that this sorta "just happens" because the static .libs get deleted in the directory that precedes the standard CRT directory in the PATH is... kinda scary; so I wanted to make that more explicit, by resetting the environment to its standard state and creating [yet another] a flag for it.)

In the Songbird 0.6 cycle, we found a couple of issues with the debug version of this patch, so I'll post that patch; for now, you can probably ignore these patches, Ted.

I'll get them updated against 3.0 final (which should be easy) and include the debug fixes. 
Comment on attachment 320209 [details] [diff] [review]
More tool munging, take 1

Clearing review requests, waiting for updated patches.
Attachment #320209 - Flags: review?(ted.mielczarek)
Attachment #321336 - Flags: review?(ted.mielczarek)
Attachment #321337 - Flags: review?(ted.mielczarek)
(Reporter)

Comment 28

10 years ago
Created attachment 333783 [details] [diff] [review]
crtpatch, v4

This is pretty much the same as take 2, except it exports a few more debug symbols that the standard CRT exports (_sample_.def lies!!)

Ted: after you've looked through the other stuff, I'd be happy to figure out a way to make verifying this particular patch easier in various configurations; I think running this through the cl equiv of -i and comparing the output would be useful, but we can chat about it on IRC.
Attachment #333783 - Flags: review?(ted.mielczarek)
(Reporter)

Comment 29

10 years ago
Created attachment 333784 [details] [diff] [review]
Build system gut-ectomy, Take 4

Fundamental change here:

-- the LIB, PATH, and INCLUDE environment variables are no longer munged; we munge -I and -LIBPATH as necessary; this makes it clearer when compiling what's actually happening, and also allows config.mk and rules.mk to change the behavior of the compile, instead of putting logic in autoconf.mk.in, which is the more standard pattern.
-- USE_STATIC_LIBS implies DISABLE_MOZ_MEMORY now; there's a comment as to why
Attachment #321337 - Attachment is obsolete: true
Attachment #333784 - Flags: review?(ted.mielczarek)
(Reporter)

Comment 30

10 years ago
Created attachment 333785 [details] [diff] [review]
Make more stuff static, take 1

Makes various supporting tools (most notably, updater.exe and crashreporter.exe) tools not use jemalloc.
Attachment #320209 - Attachment is obsolete: true
Attachment #333785 - Flags: review?(ted.mielczarek)
(Reporter)

Updated

10 years ago
Attachment #321336 - Attachment is obsolete: true
This all sounds great! I promise I'll take a look at it by next week, please ping me repeatedly if I fail to honor that promise.
Comment on attachment 333785 [details] [diff] [review]
Make more stuff static, take 1

Shoulda looked at this patch first. Srsly.
Attachment #333785 - Flags: review?(ted.mielczarek) → review+
Comment on attachment 333784 [details] [diff] [review]
Build system gut-ectomy, Take 4

 # These are for custom CRT building
 ifdef MOZ_MEMORY

Guess this should just read "this is for custom CRT building" now.

+
+#
+# jemalloc crap; the INCLUDE and LIB paths are munged on Win32
+#

You're not actually munging INCLUDE and LIB, could you phrase this differently?

+MOZ_MEMORY_LDFLAGS += -LIBPATH:$(WIN32_CUSTOM_CRT_DIR)
+MOZ_MEMORY_LDFLAGS += -NODEFAULTLIB:msvcrt -NODEFAULTLIB:msvcrtd
+MOZ_MEMORY_LDFLAGS += -NODEFAULTLIB:libcmt -NODEFAULTLIB:libcmtd

Just make this one statement, with backslash line continuation. You can end the first line after the += to make everything line up nicely.

         WIN32_CRT_SRC_DIR="$VCINSTALLDIR\crt\src"
+

Hey, no introducing stray empty lines into my nice clean configure.in!

+libs:: mozcrtlibs mozcrtruntimedll

You could just replace both of these targets with lib::, I think, for the same effect without extra targets.

+mozcrtlibs: $(MOZ_CRT_DLL) $(MOZ_CRT_LIB) $(MOZ_CRT_PDB)
+	$(INSTALL) $^ $(DIST)/lib

Do you need to install the DLL into dist/lib? We don't tend to stick DLLs there.

+	# Ooh, let's be clever...

Why do you need to do all this file copying and sed? Couldn't this be part of the CRT patch?

+	# debug mode
+	# In debug mode, we need to copy the debug headers that we've patched,
+	# so the mozilla build uses the same headers that we used for the CRT
+	# build; incidentally, this is why INCLUDE now gets munged

I don't see you munging INCLUDE anywhere.

diff -r 889e2e6df7c8 memory/jemalloc/jemalloc.c

Please r?jasone on this bit.

r- to answer or address the points above, but overall looks good!
Attachment #333784 - Flags: review?(ted.mielczarek) → review-

Comment 34

10 years ago
Comment on attachment 333784 [details] [diff] [review]
Build system gut-ectomy, Take 4

i suggest adding those windows functions to some separate file and possibly #including them, rather than adding them to jemalloc.c or something else clever.
(Reporter)

Comment 35

10 years ago
Quick update:

I haven't forgotten about this; I'm currently working through issues that are most likely the same described by Nelson here:

http://groups.google.com/group/mozilla.dev.builds/browse_thread/thread/e9779909bb9f71bb#

pk12util.exe now crashes after bug 428009 landed.
Attachment #333783 - Flags: review?(ted.mielczarek)
Comment on attachment 333783 [details] [diff] [review]
crtpatch, v4

Clearing this review request until we get some new patches. I don't really want to review a CRT patch in this format anyway. As we discussed at some point, hopefully you could mail me a unified diff of the CRT source with the current patch applied vs. the CRT source with your new patch applied.
(Reporter)

Comment 37

10 years ago
I'm going to kick this to nobody@; I'm not actively working on this anymore and don't foresee having the time in the (near) future to do so.

Hopefully someone can pick it up if it's decided there's value in being able to get a debug build in the configuration that Win32 Fx ships in (although, with PGO, who knows if that's possible anymore).

FWIW, Songbird has been using this patch for 3-4 releases, so it has had some testing in that regard; the release-mode stuff appears to be stable/usable.

Some answers to Ted's last review questions:

(In reply to comment #33)
> +libs:: mozcrtlibs mozcrtruntimedll
> 
> You could just replace both of these targets with lib::, I think, for the same
> effect without extra targets.

Yeah, the reason I did it this way is to make a distinction between the runtime dll that's necessary, and the rest of the libs that are only useful in an SDK/XR context. As evidenced by below, I wasn't 100% sure where all of this stuff should go, since NSS, NSPR, and the Firefox build itself all do slightly different things; it's possible (probable) that I just didn't discern the proper pattern.

> +mozcrtlibs: $(MOZ_CRT_DLL) $(MOZ_CRT_LIB) $(MOZ_CRT_PDB)
> +    $(INSTALL) $^ $(DIST)/lib
> 
> Do you need to install the DLL into dist/lib? We don't tend to stick DLLs
> there.

Yeah; I did this because I wasn't 100% sure where they should go, and this was a holdover from getting the xulrunner build to do the right thing. I couldn't find any consistent guidance from the makefiles on where to put .dlls vs. the .pdbs vs. the .libs (the latter two of which are possibly-more important in the XR case, but not the Fx case).

> +    # Ooh, let's be clever...
> 
> Why do you need to do all this file copying and sed? Couldn't this be part of
> the CRT patch?

I did it this way because we take the sample def (and friends) files, copy them to new names required for the CRT's build system, and make minor modifications to those; if we included them in the CRT patch, since there new files, there were be a bunch of line additions which would basically consist of a dump of Microsoft-copyrighted source code.

Maybe they don't care, because it's sample code, but I didn't want to take the chance... so we copy the files to the new names in the makefile, and patch them with the CRT patch.
Assignee: mozpreed → nobody
Status: ASSIGNED → NEW

Updated

9 years ago
Duplicate of this bug: 471820

Comment 39

9 years ago
It would really be *nice* to be able to test debug windows builds with jemalloc. Nominating for wanted1.9.*
Flags: wanted1.9.2?
Flags: wanted1.9.1.x?
Flags: wanted1.9.0.x?
I think these changes are probably too invasive to take on branch.

Comment 41

9 years ago
I agree, we should not be taking jemalloc+debug on the stable branches.
Flags: wanted1.9.2? → wanted1.9.2+
Deferring to Ted and Benjamin. Not wanted on either branch.
status1.9.1: --- → wontfix
Flags: wanted1.9.1.x?
Flags: wanted1.9.0.x?
Flags: wanted1.9.0.x-
Created attachment 443148 [details] [diff] [review]
error out if the build would fail

Let's at least error out at configure time if the build wouldn't succeed, so that people aren't confused.
Attachment #443148 - Flags: review?(ted.mielczarek)
Comment on attachment 443148 [details] [diff] [review]
error out if the build would fail

I think you want test -n for maximum consistency with the rest of configure. (But I could be wrong.)

Sorry, we should have done this ages ago.
Attachment #443148 - Flags: review?(ted.mielczarek) → review+

Updated

8 years ago
Blocks: 586962
I just got this trying to build a Debug SeaMonkey on MoMe Try:
http://build.mozillamessaging.com/buildbot/try/builders/WINNT%205.2%20tryserver%20build/builds/120/steps/compile/logs/stdio
{
   Creating library mozalloc.lib and object mozalloc.exp
mozalloc.obj : error LNK2019: unresolved external symbol _posix_memalign referenced in function _moz_xposix_memalign
mozalloc.obj : error LNK2019: unresolved external symbol _memalign referenced in function _moz_xmemalign
mozalloc.obj : error LNK2019: unresolved external symbol _malloc_usable_size referenced in function _moz_malloc_usable_size
mozalloc.dll : fatal error LNK1120: 3 unresolved externals
make[6]: *** [mozalloc.dll] Error 96
make[6]: Leaving directory `/e/buildbot/tryserver-win32/build/objdir/mozilla/memory/mozalloc'
}
Component: XULRunner → jemalloc
Product: Toolkit → Core
QA Contact: xulrunner → jemalloc
I've been building --enable-jemalloc --enable-debug successfully on Linux for a month or so.  I never realized this wasn't supposed to work.  :)  This may be WORKSFORME.

Comment 47

7 years ago
Sorry, Justin, you missed the point. This issue is all about jemalloc and debug under WINDOWS platfrom. The other platforms may NOT be affected because they have nothing to do with mentioned MSVCRT (ms visual C runtime library).
Indeed; thanks for clearing that up for me.
This bug tripped me up today, leading to the following exchange on IRC:

<glandium> khuey: we should probably make --enable-jemalloc and --enable-debug
  mutually exclusive in configure.in on windows
<khuey> yeah
• khuey would be ok with that
<khuey> or fixing our python script not to replace __imp__free when it's part of
  __imp__free_dbg
[...]
<jorendorff> khuey, glandium, bsmedberg:
  https://bug429745.bugzilla.mozilla.org/attachment.cgi?id=443148
<jorendorff> already has review from ted, never landed
<bsmedberg> jorendorff: feel free ;-)

But the patch doesn't apply cleanly anymore. I'll post a new one in a minute.
Created attachment 586515 [details] [diff] [review]
error out if the build would fail, v2
Attachment #586515 - Flags: review?(benjamin)
(In reply to Jason Orendorff from comment #49)
> <glandium> khuey: we should probably make --enable-jemalloc and
>            --enable-debug mutually exclusive in configure.in on windows
That's overkill. The 32-bit debug CRT doesn't import free, so there's no downside to using jemalloc.

> <khuey> or fixing our python script not to replace __imp__free when it's
>         part of __imp__free_dbg
That's unhelpful, because that means the CRT will try to free stuff...

My best idea is to export _frex_dbg from mozutils.
I tried this:
>--- a/memory/mozutils/mozutils.def.in
>+++ b/memory/mozutils/mozutils.def.in
>@@ -56,3 +56,7 @@ EXPORTS
>   ; A hack to work around the CRT (see giant comment in Makefile.in)
>+#ifdef MOZ_MEMORY_DEBUG
>+  frex_dbg=je_dumb_free_thunk
>+#else
>   frex=je_dumb_free_thunk
>+#endif
> #endif

but although this does change the output in $OBJDIR/memory/mozutils/mozutils.def, it didn't cause the build to succeed. Same error:

   Creating library mozalloc.lib and object mozalloc.exp
mozcrt.lib(crtdll_fixed.obj) : error LNK2019: unresolved external symbol __imp__
frex_dbg referenced in function _CRT_INIT
mozalloc.dll : fatal error LNK1120: 1 unresolved externals
Er-- I don't mean to say comment 51 is wrong or anything like that, I just could use a bit of help figuring out how to implement the change it proposes.
(In reply to neil@parkwaycc.co.uk from comment #51)
> (In reply to Jason Orendorff from comment #49)
> > <glandium> khuey: we should probably make --enable-jemalloc and
> >            --enable-debug mutually exclusive in configure.in on windows
> That's overkill. The 32-bit debug CRT doesn't import free, so there's no
> downside to using jemalloc.
Actually that's not quite true.
The 32-bit release CRT references free. 32-bit calling convention prefixes an underscore, and then exporting adds __imp_ resulting in __imp__free which we change to __imp__frex.
The 64-bit release CRT references free. 64-bit calling convention has no prefix, but we still get the __imp_ resulting in __imp_free which we change to __imp__frex.
The 32-bit debug CRT references _free_dbg. 32-bit calling convention prefixes an extra underscore, plus the __imp_ resulting in __imp___free_dbg which we ignore because it has too many underscores!
The 64-bit debug CRT references _free_dbg. This only gets the __imp_ treatment resulting in __imp__free_dbg which confuses us resulting in __imp__frex_dbg.
What I don't know yet is whether we actually try to free something we shouldn't; in my hacked 64-bit debug jemalloc build I do hit je_dumb_free_thunk.

Updated

7 years ago
Attachment #586515 - Flags: review?(benjamin) → review+
We enabled jemalloc on Windows debug builds in bug 1014976, so this is obviously not a problem anymore.
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → WORKSFORME
You need to log in before you can comment on or make changes to this bug.