1400096 - Build mozmemory_wrap as C++

Status: RESOLVED FIXED

(Core :: Memory Allocator, enhancement)

Comment 1

[Mike Hommey [:glandium]]

Attachment: Bug 1400096 - Remove mozmem_malloc_impl wrapping for operator methods exposed in mozmemory_wrap.c.

This used to be necessary because those functions might be prefixed with
__wrap_, and linked against with -Wl,--wrap, but that's not been the case
since bug 1077366. Furthermore, mozmem_malloc_impl nowadays only does
something on Windows, and those operators are only exposed on Android.

Review commit: https://reviewboard.mozilla.org/r/180112/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/180112/

Comment 2

[Mike Hommey [:glandium]]

Attachment: Bug 1400096 - Annotate mozmemory_wrap.c windows functions as MOZ_MEMORY_API.

It happens to work because of mozglue.def, but we might as well have the
right annotations (which will also make things correct when building this
file to C++)

Review commit: https://reviewboard.mozilla.org/r/180114/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/180114/

Comment 3

[Mike Hommey [:glandium]]

Attachment: Bug 1400096 - Build mozmemory_wrap as C++.

And since the build system doesn't handle transitions to foo.c to
foo.cpp properly, we work around the issue by switching to unified
sources.

Review commit: https://reviewboard.mozilla.org/r/180116/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/180116/

Comment 4

[Mike Hommey [:glandium]]

Attachment: Bug 1400096 - Don't define the operator new/delete functions as mangled in mozmem_wrap.cpp.

Now that this is a C++ file, and that the function names are not
mangled, we can just use the actual C++ names.

We do however need to replace MOZ_MEMORY_API, which implies extern "C",
with MFBT_API.

Also use the correct type for the size given to operator new. It
happened to work before because the generated code would just jump to
malloc without touching any register, but on aarch64, unsigned int was
the wrong type.

Review commit: https://reviewboard.mozilla.org/r/180118/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/180118/

Comment 5

[Nicholas Nethercote [inactive]]

Comment on attachment 8908458 [details]
Bug 1400096 - Remove mozmem_malloc_impl wrapping for operator methods exposed in mozmemory_wrap.c.

https://reviewboard.mozilla.org/r/180112/#review185272

Comment 6

[Nicholas Nethercote [inactive]]

Comment on attachment 8908459 [details]
Bug 1400096 - Annotate mozmemory_wrap.c windows functions as MOZ_MEMORY_API.

https://reviewboard.mozilla.org/r/180114/#review185274

Comment 7

[Nicholas Nethercote [inactive]]

Comment on attachment 8908460 [details]
Bug 1400096 - Build mozmemory_wrap as C++.

https://reviewboard.mozilla.org/r/180116/#review185276

::: commit-message-52c6c:3
(Diff revision 1)
> +Bug 1400096 - Build mozmemory_wrap as C++. r?njn
> +
> +And since the build system doesn't handle transitions to foo.c to

"from foo.c"

Comment 8

[Nicholas Nethercote [inactive]]

Comment on attachment 8908461 [details]
Bug 1400096 - Don't define the operator new/delete functions as mangled in mozmem_wrap.cpp.

https://reviewboard.mozilla.org/r/180118/#review185278

::: memory/build/mozmemory_wrap.cpp:56
(Diff revision 1)
>  {
>    return malloc_impl(size);
>  }
> -/* operator delete(void*, std::nothrow_t const&) */
> -MOZ_MEMORY_API void
> -_ZdlPvRKSt9nothrow_t(void* ptr)
> +
> +MFBT_API void
> +operator delete(void* ptr, std::nothrow_t const&)

The nothrow_t argument disappears and just effectively becomes part of the function name? Interesting.

Comment 9

[Mike Hommey [:glandium]]

(In reply to Nicholas Nethercote [:njn] from comment #8)
> Comment on attachment 8908461 [details]
> Bug 1400096 - Don't define the operator new/delete functions as mangled in
> mozmem_wrap.cpp.
> 
> https://reviewboard.mozilla.org/r/180118/#review185278
> 
> ::: memory/build/mozmemory_wrap.cpp:56
> (Diff revision 1)
> >  {
> >    return malloc_impl(size);
> >  }
> > -/* operator delete(void*, std::nothrow_t const&) */
> > -MOZ_MEMORY_API void
> > -_ZdlPvRKSt9nothrow_t(void* ptr)
> > +
> > +MFBT_API void
> > +operator delete(void* ptr, std::nothrow_t const&)
> 
> The nothrow_t argument disappears and just effectively becomes part of the
> function name? Interesting.

Strictly speaking, it should have been there already, but it's not passed down, the C function signature doesn't change without it, and the file was a C source anyways, so it was not really possible to put it there.

Comment 10

[Mike Hommey [:glandium]]

Comment on attachment 8908460 [details]
Bug 1400096 - Build mozmemory_wrap as C++.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/180116/diff/1-2/

Comment 11

[Mike Hommey [:glandium]]

Comment on attachment 8908461 [details]
Bug 1400096 - Don't define the operator new/delete functions as mangled in mozmem_wrap.cpp.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/180118/diff/1-2/

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6cf651d919d1
Remove mozmem_malloc_impl wrapping for operator methods exposed in mozmemory_wrap.c. r=njn
https://hg.mozilla.org/integration/autoland/rev/092857175e36
Annotate mozmemory_wrap.c windows functions as MOZ_MEMORY_API. r=njn
https://hg.mozilla.org/integration/autoland/rev/09aa9fae6280
Build mozmemory_wrap as C++. r=njn
https://hg.mozilla.org/integration/autoland/rev/738de4c495b9
Don't define the operator new/delete functions as mangled in mozmem_wrap.cpp. r=njn

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/6cf651d919d1
https://hg.mozilla.org/mozilla-central/rev/092857175e36
https://hg.mozilla.org/mozilla-central/rev/09aa9fae6280
https://hg.mozilla.org/mozilla-central/rev/738de4c495b9