Closed Bug 507082 Opened 12 years ago Closed 12 years ago

Modify Gecko code to use the "infallible malloc" library

Categories

(Core :: XPCOM, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED DUPLICATE of bug 441324

People

(Reporter: cjones, Assigned: cjones)

Details

Attachments

(1 file, 3 obsolete files)

The library (libmozalloc) exports a header that defines moz_* variants of malloc, realloc, et al., along with ::operator new's.  The library will also |#define malloc moz_xmalloc| by default.  See bug 441324.

The work for this bug is to ensure that this header is being included where necessary and libmozalloc is being linked into all Gecko libraries.  No Gecko-originating libraries should directly use the malloc/realloc/etc. symbols; they should instead explicitly use the moz_[sym] or moz_x[sym] variants.  Whether our imported libraries (e.g. libpng) use these allocators should be up to the module owners.
Attached patch v1, mozilla-central part (obsolete) — Splinter Review
This patch touches some C++ headers in XPConnect.  JSContexts use the identifiers "malloc", "calloc", and "free" as field names, but since we're #define'ing these tokens, "jscnxt.h" can't be included after "mozalloc.h"  This seems trivial enough, but please advise if XPConnect review is necessary.
Attachment #391812 - Flags: superreview?(benjamin)
Attached patch v1, mobile-browser part (obsolete) — Splinter Review
Need to link against libmozalloc when building libmozphone.  r? to bsmedberg b/c it's a build change.

NB: this needs to be committed "atomically" with the above patch or there's a window when builds might fail.  I think that this patch should go first, b/c I /think/ that make will expand the |MOZALLOC_LIB| to empty if it's undefined.
Attachment #391812 - Attachment is obsolete: true
Attachment #391814 - Flags: review?(benjamin)
Attachment #391812 - Flags: superreview?(benjamin)
Comment on attachment 391812 [details] [diff] [review]
v1, mozilla-central part

Must have obsoleted this without thinking, oops.
Attachment #391812 - Attachment is obsolete: false
Attachment #391812 - Flags: superreview?(benjamin)
Attached patch v1, mozilla-central part (obsolete) — Splinter Review
In all the "excitement" of fixing linker errors, I forgot to change the implementation of NS_alloc/_realloc!  Included here.  (Note that on the advice of bsmedberg I did not attempt to modify nsCRT.)
Attachment #391812 - Attachment is obsolete: true
Attachment #391904 - Flags: review?(benjamin)
Attachment #391812 - Flags: superreview?(benjamin)
Comment on attachment 391904 [details] [diff] [review]
v1, mozilla-central part

If you add MOZALLOC_LIB to XPCOM_GLUE_LDOPTS/XPCOM_FROZEN_LDOPTS/XPCOM_LIBS you'll avoid a lot of the custom makefile patchery here (and make me happy).

>diff --git a/toolkit/mozapps/update/src/updater/Makefile.in b/toolkit/mozapps/update/src/updater/Makefile.in

> LIBS += \
> 	$(DEPTH)/modules/libmar/src/$(LIB_PREFIX)mar.$(LIB_SUFFIX) \
> 	$(BZ2_LIBS) \
>+	$(MOZALLOC_LIB) \
> 	$(NULL)

Updater must not use mozalloc (because it is responsible for updating mozalloc, and it can't have the file open while updating it).


>--- a/widget/src/xremoteclient/Makefile.in
>+++ b/widget/src/xremoteclient/Makefile.in
>@@ -72,16 +72,17 @@ endif
> PROGRAM         = mozilla-xremote-client$(BIN_SUFFIX)
> 
> PROGOBJS        = mozilla-xremote-client.$(OBJ_SUFFIX) \
> 		XRemoteClient_standalone.$(OBJ_SUFFIX) \
> 		$(NULL)
> 
> LIBS            = \
> 		$(NSPR_LIBS) \
>+		$(MOZALLOC_LIB) \
> 		$(XLDFLAGS) $(XLIBS)

I *think* mozilla-xremote-client is installed into /usr/bin in the linux distros which still build it, and we shouldn't make it depend on mozalloc.

>diff --git a/xpcom/sample/program/Makefile.in b/xpcom/sample/program/Makefile.in

> LIBS            = \
> 		$(XPCOM_STANDALONE_GLUE_LDOPTS) \
>+		$(MOZALLOC_LIB) \
> 		$(NULL)

Anything which uses the standalone glue must not use mozalloc (it can't dynamically link against the mozilla libraries at all, because it doesn't know where they are located yet). That also means that in nscore.h if XPCOM_GLUE is defined we shouldn't include mozalloc.h, and we may not be allowed to use the infallible functions in xpcom/glue.

>diff --git a/xulrunner/stub/Makefile.in b/xulrunner/stub/Makefile.in

More standalone glue here.
Attachment #391904 - Flags: review?(benjamin) → review-
Attachment #391814 - Flags: review?(benjamin)
Attached patch v2Splinter Review
Attachment #391814 - Attachment is obsolete: true
Attachment #391904 - Attachment is obsolete: true
Attachment #395211 - Flags: review?(benjamin)
Attachment #395211 - Flags: review?(benjamin)
Comment on attachment 395211 [details] [diff] [review]
v2

Something broke badly in between v1 and v2.  Hunting down.
Attachment #395211 - Flags: review?(benjamin)
Comment on attachment 395211 [details] [diff] [review]
v2

The problem was elsewhere
Attachment #395211 - Flags: review?(benjamin) → review+
Comment on attachment 395211 [details] [diff] [review]
v2

>diff --git a/configure.in b/configure.in

>-DYNAMIC_XPCOM_LIBS='-L$(LIBXUL_DIST)/bin -lxpcom -lxpcom_core'
>+DYNAMIC_XPCOM_LIBS='-L$(LIBXUL_DIST)/bin -lxpcom -lxpcom_core $(MOZALLOC_LIB)'

I believe there are other locations in configure.in which will also need to be updated, such as the MSVC-specific section (because MSVC uses import libs and doesn't recognize the -Ldir -llib syntax).

>diff --git a/xpcom/base/nscore.h b/xpcom/base/nscore.h

>+/* Definitions of functions and operators that allocate memory. */
>+#if defined(MOZILLA_INTERNAL_API) && !defined(MOZ_NO_MOZALLOC)
>+#  include "mozilla/mozalloc.h"
>+#endif

I may have misled you at some point on IRC: I think it's ok for xpcom/glue to link against mozalloc and use the functions, just not xpcom/glue/standalone. So I think this should read:

#if !defined(XPCOM_GLUE) && !defined(MOZ_NO_MOZALLOC)

>--- a/xpcom/stub/Makefile.in

> ifeq ($(OS_TARGET),OS2)
> EXTRA_DSO_LIBS = xpcomcor
> DEPENDENT_LIBS_LIST += xpcomcor.dll
> else
> EXTRA_DSO_LIBS = xpcom_core
>-DEPENDENT_LIBS_LIST += $(LIB_PREFIX)xpcom_core$(DLL_SUFFIX)
>+DEPENDENT_LIBS_LIST += \
>+	$(LIB_PREFIX)xpcom_core$(DLL_SUFFIX) \
>+	$(LIB_PREFIX)mozalloc$(DLL_SUFFIX) \
>+	$(NULL)
> endif

mozalloc needs to go before xpcom_core here, and did you miss an OS2 block?

r=me with those changes
Bug 511568 landing makes this rather more complicated.  I'll r? vlad for the forthcoming wince hacks.
Not much point in doing this in two stages anymore.
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 441324
No longer depends on: 507083
No longer depends on: 441324
Version: unspecified → Trunk
You need to log in before you can comment on or make changes to this bug.