Closed Bug 1435407 Opened 6 years ago Closed 6 years ago

newer clang complains about mozmemory_wrap definitions of operator delete/delete[]

Categories

(Core :: Memory Allocator, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla61
Tracking Status
firefox61 --- fixed

People

(Reporter: froydnj, Assigned: froydnj)

References

Details

Attachments

(2 files)

Trying to build with a newer version of clang than the one in the NDK errors out:

INFO -  /builds/worker/workspace/build/src/sccache2/sccache /builds/worker/workspace/build/src/clang/bin/clang++ -std=gnu++14 --target=arm-linux-androideabi -o Unified_cpp_memory_build0.o -c -I/builds/worker/workspace/build/src/obj-firefox/dist/system_wrappers -include /builds/worker/workspace/build/src/config/gcc_hidden.h -DNDEBUG=1 -DTRIMMED=1 -DMOZ_MEMORY_IMPL -DMOZ_REPLACE_MALLOC_STATIC -DMOZ_HAS_MOZGLUE -I/builds/worker/workspace/build/src/memory/build -I/builds/worker/workspace/build/src/obj-firefox/memory/build -I/builds/worker/workspace/build/src/obj-firefox/dist/include -I/builds/worker/workspace/build/src/obj-firefox/dist/include/nspr -I/builds/worker/workspace/build/src/obj-firefox/dist/include/nss -fPIC -DMOZILLA_CLIENT -include /builds/worker/workspace/build/src/obj-firefox/mozilla-config.h -Qunused-arguments -isystem /builds/worker/workspace/build/src/android-ndk/platforms/android-9/arch-arm/usr/include -isystem /builds/worker/workspace/build/src/android-ndk/platforms/android-9/arch-arm/usr/include -gcc-toolchain /builds/worker/workspace/build/src/android-ndk/toolchains/arm-linux-androideabi-4.9/prebuilt/linux-x86_64 -D__ANDROID_API__=9 -Qunused-arguments -Wall -Wempty-body -Wignored-qualifiers -Woverloaded-virtual -Wpointer-arith -Wsign-compare -Wtype-limits -Wunreachable-code -Wunreachable-code-return -Wwrite-strings -Wno-invalid-offsetof -Wclass-varargs -Wloop-analysis -Wc++1z-compat -Wcomma -Wimplicit-fallthrough -Werror=non-literal-null-conversion -Wstring-conversion -Wno-inline-new-delete -Wno-error=deprecated-declarations -Wno-error=array-bounds -Wformat -Wformat-security -Wno-gnu-zero-variadic-macro-arguments -Wno-unknown-warning-option -Wno-return-type-c-linkage -fno-sized-deallocation -fno-short-enums -fno-exceptions -I/builds/worker/workspace/build/src/android-ndk/sources/cxx-stl/llvm-libc++/include -I/builds/worker/workspace/build/src/android-ndk/sources/android/support/include -I/builds/worker/workspace/build/src/android-ndk/sources/cxx-stl/llvm-libc++abi/include -march=armv7-a -mthumb -mfpu=vfpv3-d16 -mfloat-abi=softfp -mno-unaligned-access -fno-exceptions -fno-strict-aliasing -fno-rtti -ffunction-sections -fdata-sections -fno-exceptions -fno-math-errno -pipe -g -Oz -funwind-tables -Werror -Wno-tautological-pointer-compare  -MD -MP -MF .deps/Unified_cpp_memory_build0.o.pp   /builds/worker/workspace/build/src/obj-firefox/memory/build/Unified_cpp_memory_build0.cpp
INFO -  In file included from /builds/worker/workspace/build/src/obj-firefox/memory/build/Unified_cpp_memory_build0.cpp:11:
INFO -  /builds/worker/workspace/build/src/memory/build/mozmemory_wrap.cpp:34:1: error: function previously declared with an explicit exception specification redeclared with an implicit exception specification [-Werror,-Wimplicit-exception-spec-mismatch]
INFO -  operator delete(void* ptr)
INFO -  ^
INFO -  /builds/worker/workspace/build/src/android-ndk/sources/cxx-stl/llvm-libc++/include/new:137:30: note: previous declaration is here
INFO -  _LIBCPP_NEW_DELETE_VIS void  operator delete(void* __p) _NOEXCEPT;
INFO -                               ^
INFO -  In file included from /builds/worker/workspace/build/src/obj-firefox/memory/build/Unified_cpp_memory_build0.cpp:11:
INFO -  /builds/worker/workspace/build/src/memory/build/mozmemory_wrap.cpp:40:1: error: function previously declared with an explicit exception specification redeclared with an implicit exception specification [-Werror,-Wimplicit-exception-spec-mismatch]
INFO -  operator delete[](void* ptr)
INFO -  ^
INFO -  /builds/worker/workspace/build/src/android-ndk/sources/cxx-stl/llvm-libc++/include/new:150:30: note: previous declaration is here
INFO -  _LIBCPP_NEW_DELETE_VIS void  operator delete[](void* __p) _NOEXCEPT;
INFO -                               ^
INFO -  In file included from /builds/worker/workspace/build/src/obj-firefox/memory/build/Unified_cpp_memory_build0.cpp:11:
INFO -  /builds/worker/workspace/build/src/memory/build/mozmemory_wrap.cpp:58:1: error: function previously declared with an explicit exception specification redeclared with an implicit exception specification [-Werror,-Wimplicit-exception-spec-mismatch]
INFO -  operator delete(void* ptr, std::nothrow_t const&)
INFO -  ^
INFO -  /builds/worker/workspace/build/src/android-ndk/sources/cxx-stl/llvm-libc++/include/new:138:30: note: previous declaration is here
INFO -  _LIBCPP_NEW_DELETE_VIS void  operator delete(void* __p, const std::nothrow_t&) _NOEXCEPT;
INFO -                               ^
INFO -  In file included from /builds/worker/workspace/build/src/obj-firefox/memory/build/Unified_cpp_memory_build0.cpp:11:
INFO -  /builds/worker/workspace/build/src/memory/build/mozmemory_wrap.cpp:64:1: error: function previously declared with an explicit exception specification redeclared with an implicit exception specification [-Werror,-Wimplicit-exception-spec-mismatch]
INFO -  operator delete[](void* ptr, std::nothrow_t const&)
INFO -  ^
INFO -  /builds/worker/workspace/build/src/android-ndk/sources/cxx-stl/llvm-libc++/include/new:151:30: note: previous declaration is here
INFO -  _LIBCPP_NEW_DELETE_VIS void  operator delete[](void* __p, const std::nothrow_t&) _NOEXCEPT;
INFO -                               ^
INFO -  4 errors generated.

So our exception spec does not match the one in libc++.  We have appropriately declared exception specs in mozalloc.h:

http://dxr.mozilla.org/mozilla-central/source/memory/mozalloc/mozalloc.h#127

But I assume that we can't actually use mozalloc.h in mozmemory_wrap.cpp.

I think we do want to define these wrapper functions with exception specifications...is that correct?  If so, what's the best way to do that?  Separate out the MOZALLOC_THROW logic into a standalone header that can be included from both places?
Flags: needinfo?(mh+mozilla)
Blocks: 1435409
It would be even better to move the full definitions of those functions in a header that is shared between mozmemory_wrap and mozalloc, with something that triggers them being inlined or not inlined depending where they're included from.
Flags: needinfo?(mh+mozilla)
declare our wrapped delete definitions with noexcept(true)

This patch is sufficient to make things work, even if it's not the all-singing,
all-dancing common header for mozalloc.h/mozmemory_wrap.cpp.  I'm attempting to
write said header up, but I'm not 100% it will be all that nice.
Attachment #8957179 - Flags: review?(mh+mozilla)
mozmemory_wrap.cpp was defining `operator delete` in various flavors
without marking it as a `nothrow(true)` function.  Rather than just
adding the appropriate bits, it seemed better to write a common header
included by mozmemory_wrap.cpp and mozalloc.h.  That way the inline and
out-of-line definitions of `operator new`/`operator delete` can be kept
in sync.

This is the more complete patch that comment 1 suggested doing.  It compiles
for me locally; I haven't tried running it through the wringer of try yet.
Note that it *is* on top of the previous patch, just because I don't want to
lose the other one while we decide which way to go.
Attachment #8957221 - Flags: review?(mh+mozilla)
Attachment #8957179 - Attachment description: bo# Attachment to Bug 1435407 - newer clang complains about mozmemory_wrap definitions of operator delete/delete[] → declare our wrapped delete definitions with noexcept(true)
Comment on attachment 8957221 [details] [diff] [review]
unify mozalloc.h and mozmemory_wrap.cpp definitions of new/delete

Review of attachment 8957221 [details] [diff] [review]:
-----------------------------------------------------------------

::: memory/build/mozmemory_wrap.cpp
@@ +18,5 @@
>  #ifdef MOZ_WRAP_NEW_DELETE
>  #include <new>
>  
> +#define MOZ_MEMORY_EXPORT MFBT_API
> +#define MOZ_MEMORY_INFALLIBLE_ALLOC malloc_impl

Back when the wrappers in this file were created, mozalloc was a separate library, and mozglue, which contains this file, couldn't use moz_xmalloc because of that. That's not true anymore, so in fact, this file could switch to use moz_xmalloc, which would be better.

Practically speaking, that also means the only difference between mozmemory_wrap.cpp and mozalloc.h would be inlining vs. exporting as MFBT_API. I don't think it's necessary to keep more boilerplate than to choose that before including malloc_defs.h.

@@ +21,5 @@
> +#define MOZ_MEMORY_EXPORT MFBT_API
> +#define MOZ_MEMORY_INFALLIBLE_ALLOC malloc_impl
> +#define MOZ_MEMORY_FALLIBLE_ALLOC malloc_impl
> +#define MOZ_MEMORY_FREE free_impl
> +#define MOZ_MEMORY_THROW_BADD_ALLOC

typo
Attachment #8957221 - Flags: review?(mh+mozilla)
Attachment #8957179 - Flags: review?(mh+mozilla) → review+
Pushed by nfroyd@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/fc923e676ed2
declare our wrapped delete definitions with noexcept(true); r=glandium
https://hg.mozilla.org/mozilla-central/rev/fc923e676ed2
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Assignee: nobody → nfroyd
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: