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)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla36

People

(Reporter: glandium, Assigned: glandium)

References

Details

Attachments

(1 file, 1 obsolete file)

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.
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 *.
Blocks: 999869
Blocks: 1097507
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)
Blocks: 1087245
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 ':'.
(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 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)
This addresses your first comments, and changes some more, hopefully helping clarify things.
Attachment #8522555 - Flags: review?(n.nethercote)
Attachment #8521192 - Attachment is obsolete: true
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+
https://hg.mozilla.org/mozilla-central/rev/bd1cbced0ccb
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
Depends on: 1101070
Depends on: 1104173
You need to log in before you can comment on or make changes to this bug.