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)
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)
198.12 KB,
patch
|
luke
:
review+
|
Details | Diff | Splinter Review |
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?
Comment 1•13 years ago
|
||
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!
Assignee | ||
Comment 3•13 years ago
|
||
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
Assignee | ||
Comment 4•13 years ago
|
||
> 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.
Assignee | ||
Comment 5•13 years ago
|
||
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)
Assignee | ||
Comment 6•13 years ago
|
||
(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)
Assignee | ||
Comment 8•13 years ago
|
||
Attachment #520783 -
Attachment is obsolete: true
Attachment #521004 -
Flags: review?
Assignee | ||
Updated•13 years ago
|
Attachment #521004 -
Flags: review? → review?(luke)
Comment 9•13 years ago
|
||
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 10•13 years ago
|
||
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)
Comment 11•13 years ago
|
||
Paul: can you land this? I need this patch to avoid mozalloc_macro_wrappers insanity in at least 2 of my patches.
Assignee | ||
Comment 12•13 years ago
|
||
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
Comment 13•13 years ago
|
||
(In reply to comment #12) > I can land it in two parts if that would make it easier for you. That would be nice.
Assignee | ||
Comment 14•13 years ago
|
||
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.
Assignee | ||
Updated•13 years ago
|
Whiteboard: [fixed-in-tracemonkey]
Comment 15•13 years ago
|
||
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)
Comment 16•13 years ago
|
||
(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.
Assignee | ||
Comment 17•13 years ago
|
||
(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?)
Comment 18•13 years ago
|
||
cdleary-bot mozilla-central merge info: http://hg.mozilla.org/mozilla-central/rev/d796fb18f555 http://hg.mozilla.org/mozilla-central/rev/ebb7eb076e89
Comment 19•13 years ago
|
||
cdleary-bot mozilla-central merge info: http://hg.mozilla.org/mozilla-central/rev/d796fb18f555 http://hg.mozilla.org/mozilla-central/rev/ebb7eb076e89
Updated•13 years ago
|
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•