Closed
Bug 818922
Opened 11 years ago
Closed 10 years ago
Add a way to do bidirectional function calls between libxul and a replace-malloc lib
Categories
(Core :: Memory Allocator, defect)
Core
Memory Allocator
Tracking
()
RESOLVED
FIXED
mozilla36
People
(Reporter: glandium, Assigned: glandium)
References
Details
Attachments
(1 file, 1 obsolete file)
11.58 KB,
patch
|
n.nethercote
:
review+
|
Details | Diff | Splinter Review |
There can be needs to call libxul functions from a replace-malloc library, and vice-versa. A reasonable generic way to address this need is to add a function that takes a magic number and a function table and returns a function table. The magic number would be used for both ends to agree on compatibility, and the function tables would contain functions pointers from both ends.
Assignee | ||
Comment 1•10 years ago
|
||
For convenience, the function tables could be replaced by pure virtual class instances in C++. Really, any type libxul and the replace malloc lib can agree on, so API-wise, we'd give and return void *.
Assignee | ||
Comment 2•10 years ago
|
||
See patch from bug 1097507 for how replace_malloc_end_point.h ends up being filled. I'm not very attached to the "end point" terminology. I just had no better idea. Please feel free to bikeshed.
Attachment #8521192 -
Flags: review?(n.nethercote)
Comment 3•10 years ago
|
||
Comment on attachment 8521192 [details] [diff] [review] Add bidirectional method calls with replace-malloc library Review of attachment 8521192 [details] [diff] [review]: ----------------------------------------------------------------- (Still working through this. Here are some comments.) As discussed on IRC, "bridge" might be a better name than "end point". Is the ReplaceMallocEndPointImpl/ReplaceMallocEndPoint split necessary? (Likewise with DMDImpl/DMD.) ::: memory/build/replace_malloc.c @@ +15,5 @@ > #endif > > #include "mozmemory_wrap.h" > > +#define ReplaceMallocEndPointImpl ReplaceMallocEndPoint XXX: Why is this necessary? ::: memory/build/replace_malloc_end_point.h @@ +20,5 @@ > + * can decide to implement those methods or not. > + * > + * Replace-malloc libraries can provide such an end point by implementing > + * a ReplaceMallocEndPointImpl-derived class, and a replace_get_end_point > + * function returning an instance of that class. The defaults methods in s/defaults/default/ @@ +26,5 @@ > + * would be understand as "end point doesn't implement this method", so that > + * a replace-malloc library doesn't have to implement all methods. > + * > + * All methods of ReplaceMallocEndPointImpl must be virtual. Similarly, > + * anything passed as argument to those methods must be plain data, or "as an argument" @@ +27,5 @@ > + * a replace-malloc library doesn't have to implement all methods. > + * > + * All methods of ReplaceMallocEndPointImpl must be virtual. Similarly, > + * anything passed as argument to those methods must be plain data, or > + * instances of classes with only virtual methods. "an instance of a class" @@ +31,5 @@ > + * instances of classes with only virtual methods. > + * > + * Binary compatibility is expected to be maintained, such that a newer > + * Firefox can be used with an old replace-malloc library, or an old > + * Firefox can be used with a newere replace-malloc library. As such, only s/newere/newer/ @@ +57,5 @@ > + * Helper around ReplaceMallocEndPointImpl for use by the program. > + * Each virtual method in ReplaceMallocEndPointImpl should have a > + * corresponding static method doing version checking. > + */ > +struct ReplaceMallocEndPoint: public ReplaceMallocEndPointImpl Nit: space before ':'.
Assignee | ||
Comment 4•10 years ago
|
||
(In reply to Nicholas Nethercote [:njn] from comment #3) > Comment on attachment 8521192 [details] [diff] [review] > Add bidirectional method calls with replace-malloc library > > Review of attachment 8521192 [details] [diff] [review]: > ----------------------------------------------------------------- > > (Still working through this. Here are some comments.) > > As discussed on IRC, "bridge" might be a better name than "end point". > > Is the ReplaceMallocEndPointImpl/ReplaceMallocEndPoint split necessary? > (Likewise with DMDImpl/DMD.) I wanted to clearly separate methods for use from the program side (gecko).
Comment 5•10 years ago
|
||
Comment on attachment 8521192 [details] [diff] [review] Add bidirectional method calls with replace-malloc library Review of attachment 8521192 [details] [diff] [review]: ----------------------------------------------------------------- I'd like to see new patches here and in bug 1097507 using "bridge" naming before I give r+.
Attachment #8521192 -
Flags: review?(n.nethercote)
Assignee | ||
Comment 6•10 years ago
|
||
This addresses your first comments, and changes some more, hopefully helping clarify things.
Attachment #8522555 -
Flags: review?(n.nethercote)
Assignee | ||
Updated•10 years ago
|
Attachment #8521192 -
Attachment is obsolete: true
Comment 7•10 years ago
|
||
Comment on attachment 8522555 [details] [diff] [review] Add bidirectional method calls with replace-malloc library Review of attachment 8522555 [details] [diff] [review]: ----------------------------------------------------------------- ::: memory/build/malloc_decls.h @@ +25,5 @@ > # define MALLOC_FUNCS_JEMALLOC 2 > # define MALLOC_FUNCS_INIT 4 > +# define MALLOC_FUNCS_END_POINT 8 > +# define MALLOC_FUNCS_ALL (MALLOC_FUNCS_INIT | MALLOC_FUNCS_END_POINT | \ > + MALLOC_FUNCS_MALLOC | MALLOC_FUNCS_JEMALLOC) Change END_POINT to BRIDGE, please :) ::: memory/build/replace_malloc_bridge.h @@ +64,5 @@ > + > +#ifndef REPLACE_MALLOC_IMPL > + /* Returns the replace-malloc bridge if its version is at least the > + * requested one. */ > + static ReplaceMallocBridge* get(int aWantedVersion) { Nit: to match Mozilla style, this should be Get(). Would aMinimumVersion be a better name than aWantedVersion?
Attachment #8522555 -
Flags: review?(n.nethercote) → review+
Assignee | ||
Comment 8•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/bd1cbced0ccb
Comment 9•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/bd1cbced0ccb
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
You need to log in
before you can comment on or make changes to this bug.
Description
•