1412988 - spidermonkey rust code compiles code with sized deallocation in c++14 mode

Status: RESOLVED WORKSFORME

(Firefox Build System :: General, defect)

Description

[Nathan Froyd [:froydnj]]

I have a set of patches that enable C++14 support.  C++14 enables support for "sized deallocation", where `operator delete` can be called with two parameters: the pointer to free and the size of the object being freed.  With many modern memory allocators, providing the size directly can enable significant speedups.  There are ABI issues with sized deallocation--a new runtime function that needs to be supported--and compatibility issues with existing code (e.g. if a class defines an `operator delete` method with the same signature as the sized deallocation `operator delete).

Accordingly, GCC and clang have required a flag to enable sized deallocation; MSVC does not require a flag, but we have disabled it for the reasons mentioned above.

I have a try push building everything with C++14 support and disabling sized allocation on clang:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=1edbabaff5edb1de1c6095b55546880d72858ce4

Everything builds fine, except for the spidermonkey rust bindings, where we get errors like:

[task 2017-10-30T18:40:54.235Z]   = note: /builds/worker/workspace/build/src/js/rust/target/debug/deps/libmozjs_sys-de8fd039e10b1f6c.rlib(Unified_cpp_js_src24.o): In function `js::XDRIncrementalEncoder::~XDRIncrementalEncoder()':
[task 2017-10-30T18:40:54.235Z]           /builds/worker/workspace/build/src/js/src/vm/Xdr.h:467: undefined reference to `operator delete(void*, unsigned long)'
[task 2017-10-30T18:40:54.235Z]           /builds/worker/workspace/build/src/js/rust/target/debug/deps/libmozjs_sys-de8fd039e10b1f6c.rlib(Unified_cpp_js_src24.o): In function `JS::WeakCache<js::InnerViewTable>::~WeakCache()':
[task 2017-10-30T18:40:54.235Z]           /builds/worker/workspace/build/src/js/rust/target/debug/build/mozjs_sys-59b00b80e3fc8445/out/dist/include/js/SweepingAPI.h:59: undefined reference to `operator delete(void*, unsigned long)'
[task 2017-10-30T18:40:54.235Z]           /builds/worker/workspace/build/src/js/rust/target/debug/deps/libmozjs_sys-de8fd039e10b1f6c.rlib(Unified_cpp_js_src24.o): In function `js::AutoSetNewObjectMetadata::~AutoSetNewObjectMetadata()':
[task 2017-10-30T18:40:54.235Z]           /builds/worker/workspace/build/src/js/src/jscompartment.cpp:1515: undefined reference to `operator delete(void*, unsigned long)'
[task 2017-10-30T18:40:54.235Z]           /builds/worker/workspace/build/src/js/rust/target/debug/deps/libmozjs_sys-de8fd039e10b1f6c.rlib(Unified_cpp_js_src24.o): In function `js::WeakMap<js::HeapPtr<JSObject*>, js::HeapPtr<JS::Value>, js::MovableCellHasher<js::HeapPtr<JSObject*> > >::~WeakMap()':
[task 2017-10-30T18:40:54.235Z]           /builds/worker/workspace/build/src/js/src/jsweakmap.h:122: undefined reference to `operator delete(void*, unsigned long)'
[task 2017-10-30T18:40:54.235Z]           /builds/worker/workspace/build/src/js/rust/target/debug/deps/libmozjs_sys-de8fd039e10b1f6c.rlib(Unified_cpp_js_src25.o): In function `_ZN11sweepaction15SweepActionCallIJPN2js6FreeOpERNS1_11SliceBudgetEPN2JS4ZoneEEED0Ev':
[task 2017-10-30T18:40:54.235Z]           /builds/worker/workspace/build/src/js/src/jsgc.cpp:6166: undefined reference to `operator delete(void*, unsigned long)'
[task 2017-10-30T18:40:54.235Z]           /builds/worker/workspace/build/src/js/rust/target/debug/deps/libmozjs_sys-de8fd039e10b1f6c.rlib(Unified_cpp_js_src25.o):/builds/worker/workspace/build/src/js/src/jsgc.cpp:6166: more undefined references to `operator delete(void*, unsigned long)' follow

So all of these places are calling the sized deallocation operator, which ought to have been disabled.  I do not understand this, and AFAICT, the spidermonkey-rust-bindings job builds spidermonkey as normal, which should not use sized deallocation...but something clearly is.

The build process is pretty opaque, and I was hoping somebody who knows the spidermonkey rust bindings better than I do might be able to see what is obviously wrong.  The bindings run bindgen with -std=c++14 even now (which just seems wrong), but disabling sized deallocation there as well does not have any effect.

Comment 1

[Nathan Froyd [:froydnj]]

OK, so the rust job was just the canary in the coal mine here, which I guess is a good thing?

Our normal builds use GCC 6, which defaults to -std=gnu++14.  This means that all of our logic to adjust language versions:

https://dxr.mozilla.org/mozilla-central/source/build/moz.configure/toolchain.configure#479

(when adjusted for the new C++14 default) doesn't actually trigger on GCC 6.  I was attempting to add -fno-sized-deallocation there, but since we're already at C++14, we don't have the opportunity to add any new flags!  And since GCC 5 and above enabled sized deallocation by default, *all* of our C++14-enabled builds were using sized deallocation functions.  It was only the Rust builds that ran into problems, because they apparently aren't linking in bits that have an appropriate sized deallocation function (?).

So there are two questions here:

1) Do we want to go ahead and use sized deallocation with GCC?  clang does not enable it by default, AFAICT, and we disable it for MSVC (bug 1160146).

2) If we do not want to use sized deallocation, what's the best place to put the flag to disable it?  We have two options:

2a) We put it in old-configure.in, as that's where the flag is for MSVC.  Maybe chmanchester's work on moving flags means we can put this somewhere in Python land?

2b) We put it in toolchain.configure, either where we already know the compiler is GCC, hereish:

https://dxr.mozilla.org/mozilla-central/source/build/moz.configure/toolchain.configure#918

or we finagle it in where we're setting things like -std=gnu++14:

https://dxr.mozilla.org/mozilla-central/source/build/moz.configure/toolchain.configure#485

For the latter option, we'd have to add a preprocessor check for __cpp_sized_deallocation (defined by GCC and clang when sized deallocation is active).

glandium, do you have a preference or alternate ideas here?

Comment 2

[Mike Hommey [:glandium]]

Add something like check_and_add_gcc_warning(), but for other flags, and use that to add it?

Comment 3

[Nathan Froyd [:froydnj]]

We fixed this in other ways for the C++14 work.