Various replace-malloc refactors

RESOLVED FIXED in Firefox 55

Status

()

enhancement
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: glandium, Assigned: glandium)

Tracking

unspecified
mozilla55
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed)

Details

Attachments

(12 attachments, 1 obsolete attachment)

59 bytes, text/x-review-board-request
njn
: review+
Details
59 bytes, text/x-review-board-request
njn
: review+
Details
59 bytes, text/x-review-board-request
njn
: review+
Details
59 bytes, text/x-review-board-request
froydnj
: review+
Details
59 bytes, text/x-review-board-request
njn
: review+
Details
59 bytes, text/x-review-board-request
njn
: review+
Details
59 bytes, text/x-review-board-request
njn
: review+
Details
59 bytes, text/x-review-board-request
njn
: review+
Details
59 bytes, text/x-review-board-request
njn
: review+
Details
59 bytes, text/x-review-board-request
njn
: review+
Details
59 bytes, text/x-review-board-request
njn
: review+
Details
59 bytes, text/x-review-board-request
njn
: review+
Details
Assignee

Description

2 years ago
There are two main motivations here:
- Make adding new allocator API functions (a little) easier.
- More flexible handling of missing replace-malloc functions (e.g. allow to only define replace_memalign without replace_valloc, replace_posix_memalign, replace_aligned_alloc), which also participates in the first motivation.

My local patch queue is split in really small pieces hopefully making the macro changes easier to follow.

Further down the line, I'm considering merging replace-malloc into mozjemalloc, which should simplify the whole thing even further.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 14

2 years ago
mozreview-review
Comment on attachment 8872905 [details]
Bug 1368932 - Allow MOZ_PASTE_PREFIX_AND_ARG_COUNT to work with 0 arguments.

https://reviewboard.mozilla.org/r/144436/#review148508

I think it might be useful to keep the `MOZ_STATIC_ASSERT_VALID_ARG_COUNT` behavior around; interested in your thoughts on it.

::: commit-message-74f38:7
(Diff revision 1)
> +(*) the build fails, but not directly thanks to the static_assert it
> +expands to.

For clarity, this is supposed to indicate that the build fails for other reasons than the `static_assert`?

::: xpcom/base/nsCycleCollectionParticipant.h:444
(Diff revision 1)
>  #define NS_IMPL_CYCLE_COLLECTION_UNLINK(...)                                   \
> -  MOZ_STATIC_ASSERT_VALID_ARG_COUNT(__VA_ARGS__);                              \

What do macros like this do for 0 arguments now?  Do they give reasonable errors?  You say it's not reasonable to have `MOZ_STATIC_ASSERT_VALID_ARG_COUNT` now, but maybe it is still useful for the macro write to assert that args were given (perhaps with a rewrite of sorts to accomodate the new `MOZ_PASTE_PREFIX_AND_ARG_COUNT` behavior), rather than have potentially weird errors somewhere else?
Attachment #8872905 - Flags: review?(nfroyd) → review-
Assignee

Comment 15

2 years ago
(In reply to Nathan Froyd [:froydnj] from comment #14)
> Comment on attachment 8872905 [details]
> Bug 1368932 - Allow MOZ_PASTE_PREFIX_AND_ARG_COUNT to work with 0 arguments.
> 
> https://reviewboard.mozilla.org/r/144436/#review148508
> 
> I think it might be useful to keep the `MOZ_STATIC_ASSERT_VALID_ARG_COUNT`
> behavior around; interested in your thoughts on it.
> 
> ::: commit-message-74f38:7
> (Diff revision 1)
> > +(*) the build fails, but not directly thanks to the static_assert it
> > +expands to.
> 
> For clarity, this is supposed to indicate that the build fails for other
> reasons than the `static_assert`?

Yes. And the kind of error depends on the kind of arguments given, as well as what surrounded things are.

> ::: xpcom/base/nsCycleCollectionParticipant.h:444
> (Diff revision 1)
> >  #define NS_IMPL_CYCLE_COLLECTION_UNLINK(...)                                   \
> > -  MOZ_STATIC_ASSERT_VALID_ARG_COUNT(__VA_ARGS__);                              \
> 
> What do macros like this do for 0 arguments now?  Do they give reasonable
> errors?  You say it's not reasonable to have
> `MOZ_STATIC_ASSERT_VALID_ARG_COUNT` now, but maybe it is still useful for
> the macro write to assert that args were given (perhaps with a rewrite of
> sorts to accomodate the new `MOZ_PASTE_PREFIX_AND_ARG_COUNT` behavior),
> rather than have potentially weird errors somewhere else?

They all expand without an error. Some expansions will fail to further compile with no arguments. But, really, that has nothing to do with MOZ_PASTE_PREFIX_AND_ARG_COUNT. The only thing that MOZ_STATIC_ASSERT_VALID_ARG_COUNT had for it (supposedly) is the check for > 50 arguments, but that doesn't work. If individual macros do need a check for > 0 arguments, there should be an entirely new test for that. Or, in fact, better than that, we should just add a mandatory argument (i.e. #define foo(arg, ...) instead of #define foo(...)). Thoughts?
Flags: needinfo?(nfroyd)
I don't know what these CC macros have to do with replace malloc, but it is okay for them to take zero arguments. Please just file a followup in XPCOM to remove the NS_IMPL_CYCLE_COLLECTION_UNLINK_0 macro.
(In reply to Mike Hommey [:glandium] from comment #15)
> They all expand without an error. Some expansions will fail to further
> compile with no arguments. But, really, that has nothing to do with
> MOZ_PASTE_PREFIX_AND_ARG_COUNT. The only thing that
> MOZ_STATIC_ASSERT_VALID_ARG_COUNT had for it (supposedly) is the check for >
> 50 arguments, but that doesn't work. If individual macros do need a check
> for > 0 arguments, there should be an entirely new test for that. Or, in
> fact, better than that, we should just add a mandatory argument (i.e.
> #define foo(arg, ...) instead of #define foo(...)). Thoughts?

OK, let's go with the patch as-is.  File followups as appropriate to ensure the macros in question have a single argument and then __VA_ARGS__, please?
Flags: needinfo?(nfroyd)

Comment 18

2 years ago
mozreview-review
Comment on attachment 8872905 [details]
Bug 1368932 - Allow MOZ_PASTE_PREFIX_AND_ARG_COUNT to work with 0 arguments.

https://reviewboard.mozilla.org/r/144436/#review148536

Make mozreview happy.
Attachment #8872905 - Flags: review- → review+
Assignee

Comment 19

2 years ago
(In reply to Andrew McCreight [:mccr8] from comment #16)
> I don't know what these CC macros have to do with replace malloc, but it is
> okay for them to take zero arguments. Please just file a followup in XPCOM
> to remove the NS_IMPL_CYCLE_COLLECTION_UNLINK_0 macro.

NS_IMPL_CYCLE_COLLECTION_UNLINK_0 actually doesn't do the same as NS_IMPL_CYCLE_COLLECTION_UNLINK

Comment 20

2 years ago
mozreview-review
Comment on attachment 8872913 [details]
Bug 1368932 - Add a testcase for a replace-malloc library that doesn't implement all functions.

https://reviewboard.mozilla.org/r/144452/#review148566

::: memory/build/replace_malloc.c:263
(Diff revision 1)
>    return replace_malloc_table.memalign(page_size, size);
>  }
>  
> +/* For convenience, we use wrappers around malloc/free for calloc and realloc
> + * when there isn't one provided. */
> +static void *

'*' on left.

::: memory/build/replace_malloc.c:264
(Diff revision 1)
>  }
>  
> +/* For convenience, we use wrappers around malloc/free for calloc and realloc
> + * when there isn't one provided. */
> +static void *
> +dummy_calloc(size_t nmemb, size_t size)

Again, is dummy_ the best prefix?

::: memory/build/replace_malloc.c:271
(Diff revision 1)
> +  size_t total_size = nmemb * size;
> +  /* Size checks copied from calloc_impl in mozjemalloc.cpp */
> +  if (total_size == 0) {
> +    total_size = 1;
> +  } else if (((nmemb | size) & (SIZE_MAX << (sizeof(size_t) << 2)))
> +    && (total_size / size != nmemb)) {

'&&' on previous line, and align the '(' before 'total_size' with the second '(' on the previous line.

::: memory/build/replace_malloc.c:281
(Diff revision 1)
> +    memset(ptr, 0, total_size);
> +  }
> +  return ptr;
> +}
> +
> +static void *

'*' on left.

::: memory/build/replace_malloc.c:282
(Diff revision 1)
> +  }
> +  return ptr;
> +}
> +
> +static void *
> +dummy_realloc(void *ptr, size_t size)

'*' on left.

::: memory/build/replace_malloc.c:284
(Diff revision 1)
> +}
> +
> +static void *
> +dummy_realloc(void *ptr, size_t size)
> +{
> +  void *new_ptr = replace_malloc_table.malloc(size);

'*' on left.

::: memory/build/replace_malloc.c:286
(Diff revision 1)
> +static void *
> +dummy_realloc(void *ptr, size_t size)
> +{
> +  void *new_ptr = replace_malloc_table.malloc(size);
> +  if (new_ptr) {
> +    memcpy(new_ptr, ptr, size);

No no no! |size| is the new size. You need to use the old size here. Otherwise you might read past the end of |ptr|'s block.
Attachment #8872913 - Flags: review?(n.nethercote) → review-

Comment 21

2 years ago
mozreview-review
Comment on attachment 8872902 [details]
Bug 1368932 - Remove void argument from declarations in malloc_decls.h.

https://reviewboard.mozilla.org/r/144430/#review148574
Attachment #8872902 - Flags: review?(n.nethercote) → review+

Comment 22

2 years ago
mozreview-review
Comment on attachment 8872903 [details]
Bug 1368932 - Factor out function declarations for malloc implementation.

https://reviewboard.mozilla.org/r/144432/#review148552

It's surprising that malloc_decls.h #undefs MALLOC_DECL and MALLOC_FUNCS. Took me a minute to work out what's going on.
Attachment #8872903 - Flags: review?(n.nethercote) → review+

Comment 23

2 years ago
mozreview-review
Comment on attachment 8872904 [details]
Bug 1368932 - Don't rely on the default MALLOC_DECL_VOID for malloc function declarations in replace-malloc.

https://reviewboard.mozilla.org/r/144434/#review148576
Attachment #8872904 - Flags: review?(n.nethercote) → review+

Comment 24

2 years ago
mozreview-review
Comment on attachment 8872906 [details]
Bug 1368932 - Add argument names to malloc implementation declarations in replace-malloc.

https://reviewboard.mozilla.org/r/144438/#review148554

::: memory/build/replace_malloc.c:111
(Diff revision 1)
>   * specific functions MOZ_JEMALLOC_API; see mozmemory_wrap.h
>   */
>  #define MACRO_CALL(a, b) a b
> +#define MACRO_CALL2(a, b) a b
> +
> +#define TYPED_ARGS(...) MACRO_CALL2( \

I fully admit I don't understand why you have to use MACRO_CALL2 here instead of MACRO_CALL. Maybe add a brief comment? (Assuming it is necessary...)
Attachment #8872906 - Flags: review?(n.nethercote) → review+

Comment 25

2 years ago
mozreview-review
Comment on attachment 8872907 [details]
Bug 1368932 - Generate all the _impl functions with macros in replace-malloc.

https://reviewboard.mozilla.org/r/144440/#review148580
Attachment #8872907 - Flags: review?(n.nethercote) → review+

Comment 26

2 years ago
mozreview-review
Comment on attachment 8872908 [details]
Bug 1368932 - Refactor such that there is only one definition of replace_malloc_init_funcs.

https://reviewboard.mozilla.org/r/144442/#review148558

The indentation of all the # lines in this part of the code is really screwy :/

::: memory/build/replace_malloc.c:74
(Diff revision 1)
>    }
> +  return NULL;
>  }
> +
> +#define REPLACE_MALLOC_GET_FUNC(handle, name) \
> +  (replace_ ## name ## _impl_t *) GetProcAddress(handle, "replace_" # name)

'*' to the left.

::: memory/build/replace_malloc.c:80
(Diff revision 1)
> +
>  #  elif defined(MOZ_WIDGET_ANDROID)
>  #    include <dlfcn.h>
>  #    include <stdlib.h>
> -static void
> -replace_malloc_init_funcs()
> +
> +typedef void *replace_malloc_handle_t;

'*' on the left.

::: memory/build/replace_malloc.c:93
(Diff revision 1)
> +  }
> +  return NULL;
> +}
> +
> +#define REPLACE_MALLOC_GET_FUNC(handle, name) \
> +  (replace_ ## name ## _impl_t *) dlsym(handle, "replace_" # name)

'*' to the left.

Comment 27

2 years ago
mozreview-review
Comment on attachment 8872908 [details]
Bug 1368932 - Refactor such that there is only one definition of replace_malloc_init_funcs.

https://reviewboard.mozilla.org/r/144442/#review148582
Attachment #8872908 - Flags: review?(n.nethercote) → review+

Comment 28

2 years ago
mozreview-review
Comment on attachment 8872909 [details]
Bug 1368932 - Use a malloc_table_t for most replace-malloc function pointers, on all platforms.

https://reviewboard.mozilla.org/r/144444/#review148564

::: memory/build/replace_malloc.c:55
(Diff revision 1)
> +static malloc_table_t replace_malloc_table;
> +
>  #ifdef MOZ_NO_REPLACE_FUNC_DECL
>  #  define MALLOC_DECL(name, return_type, ...) \
> -    typedef return_type (replace_ ## name ## _impl_t)(__VA_ARGS__); \
> -    replace_ ## name ## _impl_t *replace_ ## name = NULL;
> +    typedef return_type (name ## _impl_t)(__VA_ARGS__); \
> +    name ## _impl_t *replace_ ## name = NULL;

'*' to the left (I think).
Attachment #8872909 - Flags: review?(n.nethercote) → review+

Comment 29

2 years ago
mozreview-review
Comment on attachment 8872910 [details]
Bug 1368932 - Fill the replace-malloc malloc_table_t with the real allocator as a fallback.

https://reviewboard.mozilla.org/r/144446/#review148586
Attachment #8872910 - Flags: review?(n.nethercote) → review+

Comment 30

2 years ago
mozreview-review
Comment on attachment 8872911 [details]
Bug 1368932 - Move the replace_malloc_init_funcs function around.

https://reviewboard.mozilla.org/r/144448/#review148588
Attachment #8872911 - Flags: review?(n.nethercote) → review+

Comment 31

2 years ago
mozreview-review
Comment on attachment 8872912 [details]
Bug 1368932 - Handle missing replace_posix_memalign at the replace-malloc level.

https://reviewboard.mozilla.org/r/144450/#review148562

::: memory/build/replace_malloc.c:220
(Diff revision 1)
>  
> +/*
> + * posix_memalign, aligned_alloc, memalign and valloc all implement some
> + * kind of aligned memory allocation. For convenience, replace_posix_memalign,
> + * replace_aligned_alloc and replace_valloc can be automatically derived from
> + * replace_memalign.

This comment still refers to the replace\_\* names. Is that right?

::: memory/build/replace_malloc.c:223
(Diff revision 1)
> + * kind of aligned memory allocation. For convenience, replace_posix_memalign,
> + * replace_aligned_alloc and replace_valloc can be automatically derived from
> + * replace_memalign.
> + */
> +static int
> +dummy_posix_memalign(void **ptr, size_t alignment, size_t size)

'**' to the left.

::: memory/build/replace_malloc.c:223
(Diff revision 1)
> + * kind of aligned memory allocation. For convenience, replace_posix_memalign,
> + * replace_aligned_alloc and replace_valloc can be automatically derived from
> + * replace_memalign.
> + */
> +static int
> +dummy_posix_memalign(void **ptr, size_t alignment, size_t size)

I would expect dummy_* functions to do nothing. Would replace_* or default_* names be better?

::: memory/build/replace_malloc.c:236
(Diff revision 1)
> +    return EINVAL;
> +  *ptr = replace_malloc_table.memalign(alignment, size);
> +  return *ptr ? 0 : ENOMEM;
> +}
> +
> +static void *

'*' to the left.

::: memory/build/replace_malloc.c:246
(Diff revision 1)
> +    return NULL;
> +  return replace_malloc_table.memalign(alignment, size);
> +}
> +
> +// Nb: sysconf() is expensive, but valloc is obsolete and rarely used.
> +static void *

'*' to the left.
Attachment #8872912 - Flags: review?(n.nethercote) → review+

Comment 32

2 years ago
mozreview-review
Comment on attachment 8872913 [details]
Bug 1368932 - Add a testcase for a replace-malloc library that doesn't implement all functions.

https://reviewboard.mozilla.org/r/144452/#review148572

r- because of the dummy_realloc() bug.

Also, is this functionality really useful? For DMD at least, the default calloc is useful, but the default realloc is not.

Comment 33

2 years ago
mozreview-review
Comment on attachment 8872914 [details]
Bug 1368932 - Add a testcase for a replace-malloc library that doesn't implement all functions.

https://reviewboard.mozilla.org/r/144454/#review148568

r+ as is, but I'm still uncertain if this functionality is useful.

::: memory/replace/logalloc/replay/replay.log:10
(Diff revision 1)
>  1 1 free(#1)
>  1 1 posix_memalign(4096,1024)=#1
>  1 1 calloc(4,42)=#3
>  1 1 free(#2)
>  1 1 realloc(#3,84)=#2
> -1 1 aligned_alloc(512,1024)=#3
> +1 1 aligned_alloc(256,1024)=#3

I don't understand why this changed.
Attachment #8872914 - Flags: review?(n.nethercote) → review+
Assignee

Comment 34

2 years ago
(In reply to Nicholas Nethercote [:njn] from comment #20)
> > +    memcpy(new_ptr, ptr, size);
> 
> No no no! |size| is the new size. You need to use the old size here.
> Otherwise you might read past the end of |ptr|'s block.

Facepalm.
Assignee

Comment 35

2 years ago
(In reply to Nicholas Nethercote [:njn] from comment #33)
> Comment on attachment 8872914 [details]
> Bug 1368932 - Add a testcase for a replace-malloc library that doesn't
> implement all functions.
> 
> https://reviewboard.mozilla.org/r/144454/#review148568
> 
> r+ as is, but I'm still uncertain if this functionality is useful.

Considering the memcpy thing, realloc is not really useful (it needs replace_malloc_usable_size). I'll just remove realloc, and leave calloc.
 
> ::: memory/replace/logalloc/replay/replay.log:10
> (Diff revision 1)
> >  1 1 free(#1)
> >  1 1 posix_memalign(4096,1024)=#1
> >  1 1 calloc(4,42)=#3
> >  1 1 free(#2)
> >  1 1 realloc(#3,84)=#2
> > -1 1 aligned_alloc(512,1024)=#3
> > +1 1 aligned_alloc(256,1024)=#3
> 
> I don't understand why this changed.

I wanted the expected output to have differentiable lines for the various corresponding memalign calls.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Assignee

Updated

2 years ago
Attachment #8872914 - Attachment is obsolete: true

Comment 46

2 years ago
mozreview-review
Comment on attachment 8872913 [details]
Bug 1368932 - Add a testcase for a replace-malloc library that doesn't implement all functions.

https://reviewboard.mozilla.org/r/144452/#review148608

::: memory/build/replace_malloc.c:312
(Diff revision 2)
>      replace_malloc_table.aligned_alloc = default_aligned_alloc;
>  
>    if (!replace_malloc_table.valloc && replace_malloc_table.memalign)
>      replace_malloc_table.valloc = default_valloc;
>  
> +  if (!replace_malloc_table.calloc && replace_malloc_table.malloc)

Does it make sense to put these lines higher up, given that calloc is usually ordered before posix_memalign et al?
Attachment #8872913 - Flags: review?(n.nethercote) → review+
Assignee

Comment 47

2 years ago
mozreview-review-reply
Comment on attachment 8872913 [details]
Bug 1368932 - Add a testcase for a replace-malloc library that doesn't implement all functions.

https://reviewboard.mozilla.org/r/144452/#review148608

> Does it make sense to put these lines higher up, given that calloc is usually ordered before posix_memalign et al?

These lines were removed.

Comment 48

2 years ago
Pushed by mh@glandium.org:
https://hg.mozilla.org/integration/autoland/rev/a92caa51ce41
Remove void argument from declarations in malloc_decls.h. r=njn
https://hg.mozilla.org/integration/autoland/rev/aa1bdb69c9e8
Factor out function declarations for malloc implementation. r=njn
https://hg.mozilla.org/integration/autoland/rev/80496d55346d
Don't rely on the default MALLOC_DECL_VOID for malloc function declarations in replace-malloc. r=njn
https://hg.mozilla.org/integration/autoland/rev/2265602a8955
Allow MOZ_PASTE_PREFIX_AND_ARG_COUNT to work with 0 arguments. r=froydnj
https://hg.mozilla.org/integration/autoland/rev/899d3cbc450c
Add argument names to malloc implementation declarations in replace-malloc. r=njn
https://hg.mozilla.org/integration/autoland/rev/3d78fdcff608
Generate all the _impl functions with macros in replace-malloc. r=njn
https://hg.mozilla.org/integration/autoland/rev/3f71138313da
Refactor such that there is only one definition of replace_malloc_init_funcs. r=njn
https://hg.mozilla.org/integration/autoland/rev/ff2a02c2733b
Use a malloc_table_t for most replace-malloc function pointers, on all platforms. r=njn
https://hg.mozilla.org/integration/autoland/rev/13a1349fb675
Fill the replace-malloc malloc_table_t with the real allocator as a fallback. r=njn
https://hg.mozilla.org/integration/autoland/rev/4753d3677cbe
Move the replace_malloc_init_funcs function around. r=njn
https://hg.mozilla.org/integration/autoland/rev/4bd3eb1b5f37
Handle missing replace_posix_memalign at the replace-malloc level. r=njn
https://hg.mozilla.org/integration/autoland/rev/bc5283bd5f85
Add a testcase for a replace-malloc library that doesn't implement all functions. r=njn
Assignee

Updated

2 years ago
Depends on: 1369622
Assignee

Updated

2 years ago
Blocks: 1402174
You need to log in before you can comment on or make changes to this bug.