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

(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?

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?

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.

(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 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.

Comment on attachment 8872902 [details] Bug 1368932 - Remove void argument from declarations in malloc_decls.h. https://reviewboard.mozilla.org/r/144430/#review148574

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.

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

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...)

Comment on attachment 8872907 [details] Bug 1368932 - Generate all the _impl functions with macros in replace-malloc. https://reviewboard.mozilla.org/r/144440/#review148580

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

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).

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

Comment on attachment 8872911 [details] Bug 1368932 - Move the replace_malloc_init_funcs function around. https://reviewboard.mozilla.org/r/144448/#review148588

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.

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

(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.

(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 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?

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.

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

https://hg.mozilla.org/mozilla-central/rev/a92caa51ce41 https://hg.mozilla.org/mozilla-central/rev/aa1bdb69c9e8 https://hg.mozilla.org/mozilla-central/rev/80496d55346d https://hg.mozilla.org/mozilla-central/rev/2265602a8955 https://hg.mozilla.org/mozilla-central/rev/899d3cbc450c https://hg.mozilla.org/mozilla-central/rev/3d78fdcff608 https://hg.mozilla.org/mozilla-central/rev/3f71138313da https://hg.mozilla.org/mozilla-central/rev/ff2a02c2733b https://hg.mozilla.org/mozilla-central/rev/13a1349fb675 https://hg.mozilla.org/mozilla-central/rev/4753d3677cbe https://hg.mozilla.org/mozilla-central/rev/4bd3eb1b5f37 https://hg.mozilla.org/mozilla-central/rev/bc5283bd5f85