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)
Core
Memory Allocator
Tracking
()
RESOLVED
FIXED
mozilla61
Tracking | Status | |
---|---|---|
firefox61 | --- | fixed |
People
(Reporter: froydnj, Assigned: froydnj)
References
Details
Attachments
(2 files)
1.37 KB,
patch
|
glandium
:
review+
|
Details | Diff | Splinter Review |
9.97 KB,
patch
|
Details | Diff | Splinter Review |
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)
Comment 1•6 years ago
|
||
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)
Assignee | ||
Comment 2•6 years ago
|
||
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)
Assignee | ||
Comment 3•6 years ago
|
||
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)
Assignee | ||
Updated•6 years ago
|
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 4•6 years ago
|
||
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)
Updated•6 years ago
|
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
Comment 6•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/fc923e676ed2
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox61:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Updated•6 years ago
|
status-firefox60:
affected → ---
Updated•6 years ago
|
Assignee: nobody → nfroyd
You need to log in
before you can comment on or make changes to this bug.
Description
•