IIUC, mozalloc_macro_wrappers.h doesn't serve any useful purpose now, if I'm understanding bug 507249's resolution correctly. (See also karl's comments in bug 581478 comment 1 bug 581478 comment 11). Filing this bug on removing mozalloc_macro_wrappers.h, mozalloc_undef_macro_wrappers.h and a few associated #defines / comments.

This removes everything related to this that I could find by searching for "macro_wrappers": http://mxr.mozilla.org/mozilla-central/search?string=_macro_wrappers I'll give this a Try run and request review if it succeeds.

Try run: https://tbpl.mozilla.org/?tree=Try&rev=3d0eac4f2569 ALSO: Note that all of the #defines in this header end up evaluating back just to the original function-name -- e.g. free is #defined to moz_free, which simply calls "return free()", so the #define is useless. Sample function definition: http://mxr.mozilla.org/mozilla-central/source/memory/mozalloc/mozalloc.cpp#45 (The only exception is moz_posix_memalign() -- that one's not just a one-line call -- it has some special-case Mac OS X code that was added back in bug 414946. But I don't think it's super important that we use that code for raw posix_memalign implementations -- I believe that was just part of getting jemalloc enabled. And for all I know, the bug we were working around there may have been fixed in the past few years.)

We can get rid of some "#undef free" lines after this is fixed, too. (per bug 924768 comment 8) http://mxr.mozilla.org/mozilla-central/search?string=undef%20free

(In reply to Daniel Holbert [:dholbert] from comment #2) > ALSO: Note that all of the #defines in this header end up evaluating back > just to the original function-name -- e.g. free is #defined to moz_free, > which simply calls "return free()", so the #define is useless. Why those functions are present at all? Maybe it deserves a followup bug to remove useless functions.

Per the a header-comment in mozalloc.h, it looks like these moz_ wrapper-methods exist to allow us to be explicit about whether we want fallible or infallible allocation. Specifically, the header-comment says: > 67 [...] The |moz_x| versions will never return a NULL pointer; > 68 * if memory is exhausted, they abort. The |moz_| versions may return > 69 * NULL pointers if memory is exhausted http://mxr.mozilla.org/mozilla-central/source/memory/mozalloc/mozalloc.h#64 So we have e.g. moz_malloc and moz_xmalloc, where the _malloc one is fallible (may return NULL) but the _xmalloc is infallible. Perhaps their usefulness is debatable (makes sense to me, from a being-explicit point of view); but in any case, removing them is outside the scope of this bug & can be dealt with a followup if appropriate.

But mozalloc_macro_wrappers.h is making it implicit :) One more reason to remove this useless header.

(In reply to Masatoshi Kimura [:emk] from comment #6) > But mozalloc_macro_wrappers.h is making it implicit Not exactly. moz_malloc and moz_xmalloc are still explicit about what they do, regardless of the wrappers header. The wrappers header just lets us customize the underlying behavior that implicitly gets invoked, when people call e.g. "malloc". And we've decided against taking advantage of that, per the resolution of bug 507249; and that's why the wrappers header no longer serves any purpose. Anyway. bsmedberg, do you have cycles for this review? :D [I totally thought I'd already r?'d you, but it looks like I forgot to toggle that after the Try run succeeded. Doing that now.]

Comment on attachment 816422 [details] [diff] [review] fix v1 Boy, the history of this is confusing. It seems that we have the following questions: * Do we want to make plain malloc() infallible by default? We certainly did originally, and I don't remember changing that decision. The comment in the other bug seems to be about a different issue, which is how we figure out which allocation sites need to continue using the fallible version. I suspect that we would like to make this change eventually, although perhaps only for gecko code and not 3rd-party code * Is this re#defineing the way to do it? I don't know the answer to this. I believe that we have or want to have the ability to use malloc replacements in general, and so all this ugly #defining isn't necessary. But I don't know that for certain. And I don't know if we'd end up with the granularity for "infallible malloc by default but not in webrtc code or skia" which is what we'd probably need.

Here's a third question, which is possibly easier to answer & probably more immediately-relevant to this bug: * Does having mozalloc_macro_wrappers.h in the tree buy us anything currently, or in the near term? I'd posit that the answer is "No", and that isn't likely to change soon, per the "eventually" (& uncertainty) expressed in comment 8. :) Given that this header's #defines are ugly & cause problems, I'd suggest that we remove them for now, until we're closer to a point where someone's actively working on making this set of functions infallible-by-default. Until someone's actively doing that (or likely to be doing that soon), I don't see any value in having these tautalogical "#define malloc [another function that calls malloc]" lines in the tree.

(At the point where someone *is* actively working on making these infallible-by-default, we can add this header back (in a way that doesn't affect 3rd-party libraries like Skia (which it currently does affect, which is part of the problem)), or we can implement whatever other non-#define-dependent malloc-replacement strategy that we settle on.)

Comment on attachment 816422 [details] [diff] [review] fix v1 Review of attachment 816422 [details] [diff] [review]: ----------------------------------------------------------------- There is actually kind of a use for this, which is that it makes the malloc calls from libxul all go through the mozalloc library (well at least it did until we added more third party code), as well as for components. For components, the goal was to allow components to work on builds with or without jemalloc. With the current state of affairs wrt jemalloc, libmozalloc is more or less useless, and I eventually want to fold it into libmozglue. This is a first step in that direction, so I'm all for it.

Comment on attachment 816422 [details] [diff] [review] fix v1 Thanks, glandium. bsmedberg: given comment 11 (particularly the last 3 lines), are you OK with this?

Try run: https://tbpl.mozilla.org/?tree=Try&rev=abb7329ebc02 Landed: https://hg.mozilla.org/integration/mozilla-inbound/rev/018fd283fa7e