Add a way to do bidirectional function calls between libxul and a replace-malloc lib

RESOLVED FIXED in mozilla36

Status

()

defect
RESOLVED FIXED
7 years ago
5 years ago

People

(Reporter: glandium, Assigned: glandium)

Tracking

Trunk
mozilla36
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

Assignee

Description

7 years ago
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

5 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

Updated

5 years ago
Blocks: 999869
Assignee

Updated

5 years ago
Blocks: 1097507
Assignee

Comment 2

5 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)
Assignee

Updated

5 years ago
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 ':'.
Assignee

Comment 4

5 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 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

5 years ago
This addresses your first comments, and changes some more, hopefully helping clarify things.
Attachment #8522555 - Flags: review?(n.nethercote)
Assignee

Updated

5 years ago
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: 5 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.