Comment on attachment 8903396 [details] Bug 1395776 - Uniformize posix_memalign implementations in replace-malloc and mozjemalloc. https://reviewboard.mozilla.org/r/175236/#review180318

Comment on attachment 8903397 [details] Bug 1395776 - Uniformize valloc implementations in replace-malloc and mozjemalloc. https://reviewboard.mozilla.org/r/175238/#review180324

Comment on attachment 8903398 [details] Bug 1395776 - Move usable_ptr_t definition to mozjemalloc_types.h. https://reviewboard.mozilla.org/r/175240/#review180326

Comment on attachment 8903399 [details] Bug 1395776 - Add a level of indirection on top of mozjemalloc. https://reviewboard.mozilla.org/r/175242/#review180328 ::: memory/mozjemalloc/mozjemalloc.cpp:4814 (Diff revision 1) > > - if (!ret) { > + if (!ret) { > - errno = ENOMEM; > + errno = ENOMEM; > - } > + } > - } else { > + } else { > - if (malloc_init()) > + if (malloc_init()) Add curly braces? Alternatively, you could use :? instead. ::: memory/mozjemalloc/mozjemalloc.cpp:5253 (Diff revision 1) > +#define ARGS1(t1) arg1 > +#define ARGS2(t1, t2) ARGS1(t1), arg2 > +#define ARGS3(t1, t2, t3) ARGS2(t1, t2), arg3 > + > +#define GENERIC_MALLOC_DECL_HELPER(name, return, return_type, ...) \ > + return_type name ## _impl(ARGS_HELPER(TYPED_ARGS, ##__VA_ARGS__)) \ I would remove the spaces around `##`.

Comment on attachment 8903400 [details] Bug 1395776 - Remove useless goto. https://reviewboard.mozilla.org/r/175244/#review180330 ::: memory/mozjemalloc/mozjemalloc.cpp (Diff revision 1) > { > void *ret; > > if (malloc_init()) { > - ret = nullptr; > + return nullptr; > - goto RETURN; This changes the behaviour -- it now doesn't set `errno` on this return path. ::: memory/mozjemalloc/mozjemalloc.cpp:4704 (Diff revision 1) > - void *ret; > - > MOZ_ASSERT(((alignment - 1) & alignment) == 0); > > if (malloc_init()) { > - ret = nullptr; > + return nullptr; This change is legit, though I wonder why memalign doesn't set `errno` like malloc and other functions do... ::: memory/mozjemalloc/mozjemalloc.cpp:4759 (Diff revision 1) > void *ret; > size_t num_size; > > if (malloc_init()) { > num_size = 0; > - ret = nullptr; > + return nullptr; Ths changes behaviour here again. ::: memory/mozjemalloc/mozjemalloc.cpp:4773 (Diff revision 1) > * most significant half of the bits in a size_t. > */ > } else if (((num | size) & (SIZE_T_MAX << (sizeof(size_t) << 2))) > && (num_size / size != num)) { > /* size_t overflow. */ > - ret = nullptr; > + return nullptr; And here.

Comment on attachment 8903401 [details] Bug 1395776 - Fold replace-malloc into mozjemalloc. https://reviewboard.mozilla.org/r/175246/#review180332 I admit I didn't follow every line of this, but it seems reasonable as much as I can tell.

Comment on attachment 8903402 [details] Bug 1395776 - Share the posix_memalign, aligned_alloc and valloc implementations between mozjemalloc and replace-malloc. https://reviewboard.mozilla.org/r/175248/#review180334 ::: memory/mozjemalloc/mozjemalloc.cpp:4728 (Diff revision 1) > > -template<> inline int > -MozJemalloc::posix_memalign(void** memptr, size_t alignment, size_t size) > +template<void* (*memalign)(size_t, size_t)> > +struct AlignedAllocator > +{ > + static inline int > + posix_memalign(void**memptr, size_t alignment, size_t size) Space before `memptr`. ::: memory/mozjemalloc/mozjemalloc.cpp:4730 (Diff revision 1) > -MozJemalloc::posix_memalign(void** memptr, size_t alignment, size_t size) > +struct AlignedAllocator > +{ > + static inline int > + posix_memalign(void**memptr, size_t alignment, size_t size) > -{ > + { > - void *result; > + void *result; `*` on left. ::: memory/mozjemalloc/mozjemalloc.cpp:4742 (Diff revision 1) > - /* The 0-->1 size promotion is done in the memalign() call below */ > + /* The 0-->1 size promotion is done in the memalign() call below */ > > - result = memalign(alignment, size); > + result = memalign(alignment, size); > > - if (!result) > + if (!result) > - return ENOMEM; > + return ENOMEM; curly braces

Comment on attachment 8903403 [details] Bug 1395776 - Make _recalloc, _expand and _msize go through replace-malloc when enabled. https://reviewboard.mozilla.org/r/175250/#review180336 ::: memory/mozjemalloc/mozjemalloc.cpp:5477 (Diff revision 1) > #ifdef XP_WIN > +void* > +_recalloc(void* ptr, size_t count, size_t size) > +{ > + size_t oldsize = ptr ? isalloc(ptr) : 0; > + size_t newsize = count * size; Should this multiplication have overflow checking? ::: memory/mozjemalloc/mozjemalloc.cpp:5503 (Diff revision 1) > + */ > +void* > +_expand(void* ptr, size_t newsize) > +{ > + if (isalloc(ptr) >= newsize) > + return ptr; curly braces

Bonus points if you go back through all the functions that you reformatted and change the arguments to `aFoo` form :)

(In reply to Nicholas Nethercote [:njn] from comment #13) > Comment on attachment 8903400 [details] > Bug 1395776 - Remove useless gotos. > > https://reviewboard.mozilla.org/r/175244/#review180330 > > ::: memory/mozjemalloc/mozjemalloc.cpp > (Diff revision 1) > > { > > void *ret; > > > > if (malloc_init()) { > > - ret = nullptr; > > + return nullptr; > > - goto RETURN; > > This changes the behaviour -- it now doesn't set `errno` on this return path. facepalm, I misread the condition that was setting errno. > ::: memory/mozjemalloc/mozjemalloc.cpp:4704 > (Diff revision 1) > > - void *ret; > > - > > MOZ_ASSERT(((alignment - 1) & alignment) == 0); > > > > if (malloc_init()) { > > - ret = nullptr; > > + return nullptr; > > This change is legit, though I wonder why memalign doesn't set `errno` like > malloc and other functions do... Mmmmm it's actually tricky... posix_memalign is specified as not setting errno and is implemented on top of memalign, while memalign, technicall can set errno per https://www.mkssoftware.com/docs/man3/memalign.3.asp. Let's... punt and keep it as it is now :) (In reply to Nicholas Nethercote [:njn] from comment #16) > Comment on attachment 8903403 [details] > Bug 1395776 - Make _recalloc, _expand and _msize go through replace-malloc > when enabled. > > https://reviewboard.mozilla.org/r/175250/#review180336 > > ::: memory/mozjemalloc/mozjemalloc.cpp:5477 > (Diff revision 1) > > #ifdef XP_WIN > > +void* > > +_recalloc(void* ptr, size_t count, size_t size) > > +{ > > + size_t oldsize = ptr ? isalloc(ptr) : 0; > > + size_t newsize = count * size; > > Should this multiplication have overflow checking? Yes, it should. OTOH, nothing actually uses this function, so... followup?

(In reply to Nicholas Nethercote [:njn] from comment #14) > Comment on attachment 8903401 [details] > Bug 1395776 - Fold replace-malloc into mozjemalloc. > > https://reviewboard.mozilla.org/r/175246/#review180332 > > I admit I didn't follow every line of this, but it seems reasonable as much > as I can tell. Note that mozreview likes to not show removed files, but if you look at the actual patch, you'll see things are straightforwardly copies from replace-malloc.c, with a few adaptations where needed.

Comment on attachment 8903400 [details] Bug 1395776 - Remove useless goto. https://reviewboard.mozilla.org/r/175244/#review180674

Pushed by mh@glandium.org: https://hg.mozilla.org/integration/autoland/rev/6c647268fcad Uniformize posix_memalign implementations in replace-malloc and mozjemalloc. r=njn https://hg.mozilla.org/integration/autoland/rev/12b9309d675c Uniformize valloc implementations in replace-malloc and mozjemalloc. r=njn https://hg.mozilla.org/integration/autoland/rev/49cec367d3ca Move usable_ptr_t definition to mozjemalloc_types.h. r=njn https://hg.mozilla.org/integration/autoland/rev/20c694d0dba9 Add a level of indirection on top of mozjemalloc. r=njn https://hg.mozilla.org/integration/autoland/rev/91571e2385a4 Remove useless goto. r=njn https://hg.mozilla.org/integration/autoland/rev/cce866682b2c Fold replace-malloc into mozjemalloc. r=njn https://hg.mozilla.org/integration/autoland/rev/59ebc7093c39 Share the posix_memalign, aligned_alloc and valloc implementations between mozjemalloc and replace-malloc. r=njn https://hg.mozilla.org/integration/autoland/rev/e88120f2f0dc Make _recalloc, _expand and _msize go through replace-malloc when enabled. r=njn

https://hg.mozilla.org/mozilla-central/rev/6c647268fcad https://hg.mozilla.org/mozilla-central/rev/12b9309d675c https://hg.mozilla.org/mozilla-central/rev/49cec367d3ca https://hg.mozilla.org/mozilla-central/rev/20c694d0dba9 https://hg.mozilla.org/mozilla-central/rev/91571e2385a4 https://hg.mozilla.org/mozilla-central/rev/cce866682b2c https://hg.mozilla.org/mozilla-central/rev/59ebc7093c39 https://hg.mozilla.org/mozilla-central/rev/e88120f2f0dc

Pushed by mh@glandium.org: https://hg.mozilla.org/integration/mozilla-inbound/rev/72b1e768c334 No bug - Remove a comment that has no significance after bug 1395776. r=me