Enable jemalloc on windows debug builds

RESOLVED FIXED in mozilla32

Status

()

defect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: glandium, Assigned: glandium)

Tracking

unspecified
mozilla32
All
Windows 7
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(8 attachments, 5 obsolete attachments)

4.47 KB, patch
ted
: review+
ted
: checkin+
Details | Diff | Splinter Review
7.02 KB, patch
mshal
: review+
Details | Diff | Splinter Review
874 bytes, patch
mshal
: review+
Details | Diff | Splinter Review
1.49 KB, patch
benjamin
: review+
Details | Diff | Splinter Review
1.04 KB, patch
benjamin
: review+
Details | Diff | Splinter Review
3.73 KB, patch
RyanVM
: review+
Details | Diff | Splinter Review
9.35 KB, patch
benjamin
: review+
Details | Diff | Splinter Review
2.34 KB, patch
benjamin
: review+
Details | Diff | Splinter Review
(Assignee)

Description

5 years ago
See bug 1013014 comment 25 and subsequent.

Considering those problems, I'm seriously considering not using the msvc debug crt for our debug builds. This is what was said on irc, related to that:

<glandium> now, i wonder, is the debug crt actually buying us anything?
<glandium> iow, is it really worth bothering? can't we just not use it?
<khuey> it does heap corruption checking
<khuey> of course, if you don't use its heap ...
<khuey> the debug CRT is also a PITA because it means you can't run our windows debug builds without the matching compiler installed
<khuey> because the debug CRT is non-redistributable
<njn> glandium: not using the debug crt sounds like a good idea
(Assignee)

Updated

5 years ago
Assignee: nobody → mh+mozilla
FWIW, we do occasionally hit the CRT heap corruption detector on TBPL.  On the other hand, I don't know that it has ever resulted in any useful action.
(Assignee)

Comment 2

5 years ago
(In reply to Andrew McCreight [:mccr8] from comment #1)
> FWIW, we do occasionally hit the CRT heap corruption detector on TBPL.  On
> the other hand, I don't know that it has ever resulted in any useful action.

But as khuey mentioned, if we don't use the CRT heap (which we wouldn't with jemalloc), the heap corruption detector won't help much. Note that jemalloc does have some additional things enabled in debug builds too, but i don't know how they compare to the msvc crt heap corruption detector.
(Assignee)

Comment 3

5 years ago
Something else worth mentioning: we can't enable DMD with the debug CRT.
> we can't enable DMD with the debug CRT.

Get rid of it! :)
(Assignee)

Updated

5 years ago
Attachment #8427528 - Attachment is obsolete: true
Attachment #8427528 - Flags: review?(ryanvm)
(Assignee)

Comment 9

5 years ago
This thing has been useless has soon as it was added to the tree, because it used to be "-MOZ_NO_DEBUG_RTL", not "-DMOZ_NO_DEBUG_RTL":

http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&root=/cvsroot&subdir=mozilla/netwerk/streamconv/test&command=DIFF_FRAMESET&file=Makefile.in&rev2=1.12&rev1=1.11

(commit from december 2001)

It only recently was promoted as a define in bug 874266 (and I'm guilty of not having seen then that it was useless)
Attachment #8427531 - Flags: review?(mshal)
(Assignee)

Comment 10

5 years ago
Turns out we have had MOZ_NO_DEBUG_RTL almost for ever. It's just not hooked to everything.

The --disable-debug-rtl in configure.in is for nspr and requires the nspr patch from this bug to work properly.

ICU fails to build because of -D_DEBUG triggering the use of a debug CRT symbol in udbgutil.c. I didn't feel like working that around in ICU. I don't think we have much need for --enable-debug in ICU anyways. Even if we do, it will still be possible to do a build *with* the debug rtl anyways.
Attachment #8427533 - Flags: review?(benjamin)
(Assignee)

Comment 15

5 years ago
(In reply to Mike Hommey [:glandium] from comment #14)
> https://tbpl.mozilla.org/?tree=Try&rev=490636f3eeb6

That push failed because I tried to work around the current win64 bustage due to libstagefright. New attempt, win32 only:

https://tbpl.mozilla.org/?tree=Try&rev=fc2aec9905bb
(Assignee)

Comment 16

5 years ago
Comment on attachment 8427530 [details] [diff] [review]
Don't make --enable-debug imply using the debug CRT in FFI

This part will need a different implementation. That one overrides too much of the CFLAGS.
Attachment #8427530 - Flags: review?(ryanvm)
(Assignee)

Comment 17

5 years ago
Fixup to properly ship the CRT on debug builds not using the debug CRT.
Attachment #8427676 - Flags: review?(benjamin)
(Assignee)

Updated

5 years ago
Attachment #8427533 - Attachment is obsolete: true
Attachment #8427533 - Flags: review?(benjamin)
(Assignee)

Comment 19

5 years ago
Comment on attachment 8427676 [details] [diff] [review]
Make MOZ_NO_DEBUG_RTL builds actually disable the MSVC debug CRT everywhere

Sigh, there are remaining issues with WIN32_REDIST_DIR because it's mk_add_options'd.
Attachment #8427676 - Flags: review?(benjamin)
Comment on attachment 8427531 [details] [diff] [review]
Remove MOZ_NO_DEBUG_RTL define in netwerk/streamconv/test

Speaking of useless defines, can NGPREFS go away as well? The only use of it I see in the tree is this moz.build file.
Attachment #8427531 - Flags: review?(mshal) → review+
Attachment #8427526 - Flags: review?(mshal) → review+
(Assignee)

Updated

5 years ago
Attachment #8427530 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Attachment #8427676 - Attachment is obsolete: true
(Assignee)

Comment 23

5 years ago
Same patch as before, but with context updated.
Attachment #8429948 - Flags: review?(benjamin)
(Assignee)

Updated

5 years ago
Attachment #8427534 - Attachment is obsolete: true
Attachment #8427534 - Flags: review?(benjamin)

Updated

5 years ago
Attachment #8427537 - Flags: review?(benjamin) → review+

Updated

5 years ago
Attachment #8427538 - Flags: review?(benjamin) → review+

Updated

5 years ago
Attachment #8429946 - Flags: review?(benjamin) → review+

Updated

5 years ago
Attachment #8429948 - Flags: review?(benjamin) → review+
Attachment #8427523 - Flags: review?(ted) → review+
Comment on attachment 8429801 [details] [diff] [review]
Don't make --enable-debug imply using the debug CRT in FFI

Review of attachment 8429801 [details] [diff] [review]:
-----------------------------------------------------------------

Upstream's cool with it, so fire away :)

::: js/src/ctypes/libffi-patches/01-rtl.patch
@@ +1,1 @@
> +Don't make --enable-debug imply using the debug CRT in FFI

Include the bug number please? :)
Attachment #8429801 - Flags: review?(ryanvm) → review+
(Assignee)

Updated

5 years ago
Depends on: 1017926
As of 185771, my builds (and clobber-builds) are failing, starting with this error:
3:23.22 e:\dev\mozilla\central\memory\jemalloc\src\include\msvc_compat\stdbool.h(8) : error C2628: 'BOOL' followed by 'bool' is illegal (did you forget a ';'?)

This is with mozbuild 1.9.0 and msvc2013.
(Assignee)

Comment 30

5 years ago
(In reply to Jeff Gilbert [:jgilbert] from comment #29)
> As of 185771, my builds (and clobber-builds) are failing, starting with this
> error:
> 3:23.22
> e:\dev\mozilla\central\memory\jemalloc\src\include\msvc_compat\stdbool.h(8)
> : error C2628: 'BOOL' followed by 'bool' is illegal (did you forget a ';'?)
> 
> This is with mozbuild 1.9.0 and msvc2013.

That's bug 945582.
Depends on: 1023703
You need to log in before you can comment on or make changes to this bug.