Closed Bug 643548 Opened 13 years ago Closed 13 years ago

Remove mozalloc_undef_macro_wrappers hack from JS engine

Categories

(Core :: JavaScript Engine, defect)

Other Branch
x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla2.0

People

(Reporter: paul.biggar, Assigned: paul.biggar)

References

(Blocks 1 open bug)

Details

(Whiteboard: [fixed-in-tracemonkey])

Attachments

(1 file, 1 obsolete file)

The mozalloc_macro_wrappers hack is really unpleasant. I think it should be removed from the browser entirely, but I'll settle on just the JS engine for now.

Background, as I understand it:

memory/mozalloc/ provides moz_malloc, moz_free, etc. We want to use these in all places instead of malloc(), free() etc. So we have a macro which replaces malloc() with moz_malloc(). The mechanism for this is in mozalloc_macro_wrappers.h and looks like this:

  #define free(_) moz_free(_)

Unfortunately, since this is the preprocessor, it mangles far more thgan it should. Any use of the text "free" followed by an opening parens get's converted. So declarations of methods get converted unless they are written like this:

  void (free)(void*) { ... }

They are rarely written as such in practice, so many classes which should have a "free" method have a "moz_free" one instead. Similarly, callsites of a free() method also get changed to moz_free().

For a reason I don't fully understand, we sometime do want to call free(), and sometimes we don't. (Perhaps we don't want to accidentally rename the free() methods, I'm not clear on this). As a result, we have a mechanism to turn off the mozalloc macros, and then turn them back on afterwards. This are mozalloc_undef_macro_wrappers.h.

Fixing compilation errors related to mozalloc_macro_wrappers generally means sprinkling mozalloc_undef_macro_wrappers.h around, with a corresponding mozalloc_macro_wrappers.h to resume it.

The indended usage is like this:

def
undef
redef



The problem:

Unfortunately, sometimes the undef is used multiple times, like so:

def
undef
undef
redef
// in here is code which doesn't want it redefed, but can't avoid it
redef

The result of this madness is classes who's definitions and declarations have not been consitently mozalloced. Since there is no tool for tracing this through the preprocessor, it's a pretty gnarly one to fix.


Solution:

I suspect the correct solution in the browser might be to remove the wrappers altogether (I don't have to context to know if I'm right here). For JS though, we can just rename all of our malloc() and free() calls to malloc_() and free_(), in the same way that we have with new and delete. I have a patch for that, but it's bitrotted.


cjones, is my reasoning above correct?
The purpose of the wrappers is to replace "malloc" with a version which is infallible. If we are truly out of memory, it will abort instead of returning NULL, and this allows us to remove a lot of error handling code which has been the subject of security issues and general poor maintenance in the past.

We want to do this without necessarily affecting other code loaded into our process, so simply using ELF binding to say "ours is the true malloc" isn't useful (and doesn't work on other platforms anyway).
(In reply to comment #0)
> cjones, is my reasoning above correct?

Yes, that's about right.  Some notes
 - we're trying to move away from using non-standard interfaces in Gecko as much as possible.  One instance of that was a move to being able to use malloc/free/et al.  Sadly, there's no clean way to map those to moz_* on all Tier I platforms.  Macros are the lowest common denominator, and the result is ugly.  Either we have to give up malloc/free/et al. or we have to live with the macros.

 - there's no reason a priori why Gecko would be the only SM embedder with gross macro hacks

For those reasons, as we've discussed elsewhere, I favor the SM malloc->malloc_ rename.  Sorry!
Thanks for your replies. Unfortunately, I misstated my case above. I should have said that I'd like to remove the uses of the "undef" portion of wrappers. Apologies for the confusion.
Summary: Remove mozalloc_macro_wrappers hack from JS engine → Remove mozalloc_undef_macro_wrappers hack from JS engine
> I suspect the correct solution in the browser might be to remove the wrappers
altogether

So this should have said:

I suspect the correct solution in the browser might be to remove the undef part of wrappers altogether.
This patch won't apply, since it's high up in the tree, but I hope it's clear that it does what it says.

This renames all declarations, definitions, and uses from {malloc,free,calloc,realloc} to have an underscore at the end of them.
Assignee: general → pbiggar
Status: NEW → ASSIGNED
Attachment #520783 - Flags: review?(jones.chris.g)
(In reply to comment #5)
> This patch won't apply, since it's high up in the tree, but I hope it's clear
> that it does what it says.

I'm having one of those days. I mean to say that it's based on stuff in my queue, including an unpublished sequel to the patch from bug 634155.
Comment on attachment 520783 [details] [diff] [review]
Add underscores to the end of allocation functions

This seems OK to me, but shouldn't an SM person review this?
Attachment #520783 - Flags: review?(jones.chris.g)
Blocks: 632137
Attachment #520783 - Attachment is obsolete: true
Attachment #521004 - Flags: review?
Attachment #521004 - Flags: review? → review?(luke)
Comment on attachment 521004 [details] [diff] [review]
Add underscores to all allocation keywords

Seems reasonable to me.  I actually kinda like it because it makes malloc_ et al consistent with new_ et al.  However, it seems like the widespread renaming deserves a Brendan-blessing.
Attachment #521004 - Flags: superreview?(brendan)
Attachment #521004 - Flags: review?(luke)
Attachment #521004 - Flags: review+
Comment on attachment 521004 [details] [diff] [review]
Add underscores to all allocation keywords

Brendan said 'ok by me' on IRC; don't need the sr+.
Attachment #521004 - Flags: superreview?(brendan)
Blocks: 641048
Paul: can you land this? I need this patch to avoid mozalloc_macro_wrappers insanity in at least 2 of my patches.
I'm trying to, but there are problems on tryserver with the checking script. I can land it in two parts if that would make it easier for you.
Target Milestone: --- → mozilla2.0
Version: Trunk → Other Branch
(In reply to comment #12)
> I can land it in two parts if that would make it easier for you.

That would be nice.
http://hg.mozilla.org/tracemonkey/rev/d796fb18f555

I landed this in one part because I just turned off the windows checks. Not ideal, but the same checks are run on all platforms, so we don't lose a great deal.
Whiteboard: [fixed-in-tracemonkey]
We must to fix the comment in jsutil.h. They suggest that cx->malloc_ is a preferable allocation method. This is wrong. cx->malloc_ must be used *only* for memory that will be released during the GC. For temporary stuff or the stuff that the GC would never release we should use Foreground::*.

I also think that an explicit js_new is better than the class-declared new operators. Without it is not obvious from the new operator invocation which new should be used. This is especially so if we fix broken usage of cx->malloc_ where the method is used for temporary non-GC stuff only because cx->malloc also reports an OOM on failure. For that we would need a set of allocation methods that take cx as parameter but do not account for the GC pressure and just report OOM on failure. But with such methods it would be impossible to judge from new (cx) T which method is used.

So instead of rather lengthy and ugly due to the need for last _ Foreground::somethig_ I would prefer to see a set of methods like:

sysMalloc(size)
sysNew<T>(...)
sysFree(p)
sysDelete(p)

reportingSysMalloc(cx, size) - call sysMalloc and report OOM 
reportingSysNew<T>(cx, ...)

gcAlloc(cx, size) - allocating memory that normally will be released during the GC
gcFree(cx, p) - releasing the memory from the finalizer via the background thread
gcForegroundFree(ptr) - releasing the memory that was allocated via gc_alloc on a failure path outside the GC. This is necessary so it would be possble to release, for example, the char array for the JSString if we failed to allocate the string.

This way from the call site it would be very obvious how the instance of the class is allocated. I.e:

sysNew<T>()
reportingSysNew<T>(cx)
gcNew<T>(cx)
(In reply to comment #15)
> I also think that an explicit js_new is better than the class-declared new
> operators. 

I was wrong in that sentence - the patch keeps the current explicit mentioning of the allocation at the call site. I.e. OffTheBooks::new_ od cx->new_ makes things very explicit. 

But what is wrong is the penalization of non cx->new_ users with long names as if cx->something_ is the best way to go. cx->malloc_ and friend should be only used for the heap-allocate memory that will be released by the mean of a finalizer.

My current observation is that in SM the heap memory, that is not released during the GC, is mostly used temporarily under some native method. Given that we should have three allocation classes like:

SysMem::something_ - calls something directly without any OOM reporting or background memory release.

GCMem::something_ - allocate memory that would be stored in GC things and that should be released using a finalizer.

TempMem::something_ - allocate temporary memory that will be released when some native method returns. This should also report OOM.
(In reply to comment #16)
> SysMem::something_ - calls something directly without any OOM reporting or
> background memory release.
> 
> GCMem::something_ - allocate memory that would be stored in GC things and that
> should be released using a finalizer.
> 
> TempMem::something_ - allocate temporary memory that will be released when some
> native method returns. This should also report OOM.

(Classes are good - they make private friendship much easier.)


Can you elaborate how this stuff gels with the existing OffTheBooks, Foreground, JSRuntime/JSContext stuff? What would be split, replaced, etc?

(Also, we need a new bug. The allocation changes was in bug 634155. I saw another bug about splitting OOM reporting from GC stuff, where was that?)
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Blocks: 647103
You need to log in before you can comment on or make changes to this bug.