Closed Bug 1294537 Opened 4 years ago Closed 4 years ago
introduce a non-null-checking placement operator new
The default placement operator new is defined to always require that its result be null-checked. A sufficiently smart compiler can remove this check, but not all compilers are sufficiently smart. Better to have a custom placement operator new that will remove null checks in a way defined by the standard.
Asking Dan for review, as he tried to do something similar to this in bug 1265892. I can still see null checks in Vector functions with my local GCC 4.9; I suspect that our default builds aren't eliminating null checks with the MOZ_NONNULL attribute in Vector.h, and MSVC doesn't support MOZ_NONNULL in any event. The comment in OperatorNewExtensions.h will ideally explain the reasoning behind the function, and also provides a (partial) explanation for why Dan didn't see null checks go away in bug 1265892 after trying to implement Waldo's suggestion of using references. Bikeshedding welcome on the name of the new header; WebKit puts this function in a StdLibExtras.h file, for reference. Inspection of assembly seems to confirm that null checks go away in Vector functions with the code provided in this patch with GCC 4.9 on Linux.
Attachment #8780285 - Flags: review?(sunfish)
Comment on attachment 8780285 [details] [diff] [review] introduce a non-null-checking placement operator new Review of attachment 8780285 [details] [diff] [review]: ----------------------------------------------------------------- ::: mfbt/OperatorNewExtensions.h @@ +34,5 @@ > + * > + * You might think that MOZ_NONNULL might perform the same function, but > + * MOZ_NONNULL isn't supported on all of our compilers, and even when it is > + * supported, doesn't work on all the versions we support. And even keeping > + * those limitations, we can't put MOZ_NONNULL on the global, standardized This should have been "...even keeping those limitations in mind, ..."
Comment on attachment 8780285 [details] [diff] [review] introduce a non-null-checking placement operator new Review of attachment 8780285 [details] [diff] [review]: ----------------------------------------------------------------- This looks good. The changes I made previously only helped elements for which IsPod is true, but this patch will help all types. (If you're using Vector on user-defined types that are POD, you may wish to specialize IsPod for them, even with this change.) ::: mfbt/OperatorNewExtensions.h @@ +38,5 @@ > + * those limitations, we can't put MOZ_NONNULL on the global, standardized > + * placement new function in any event. > + */ > +inline void* > +operator new(size_t, mozilla::NotNullTag, void* p) You could add a MOZ_NONNULL for |p| here too. It's not super important, but it may help some static analysis tools detect paths where null pointers could be passed in. ::: mfbt/Vector.h @@ -58,5 @@ > /* > * Constructs an object in the uninitialized memory at *aDst with aArgs. > */ > template<typename... Args> > - MOZ_NONNULL(1) I'd suggest keeping this MOZ_NONNULL here too, though I don't feel strongly about it.
Attachment #8780285 - Flags: review?(sunfish) → review+
I wonder, would it make sense to have a mozilla::NotNull<Ptr> wrapper type instead of a KnownNotNull tag? Thus not-null can be exposed as part of the signature of the functions instead of being part of a comment or being an implicit assumption. Thus, the new operator would become: void* operator new(size_t, mozilla::NotNull<void*> p); and we could later add things like: void bar(mozilla::NotNull<JS::Handle<JSObject*>>); Thus, we can move the MOZ_ASSERT to the NotNull constructor.
(In reply to Nicolas B. Pierron [:nbp] from comment #4) > I wonder, would it make sense to have a mozilla::NotNull<Ptr> wrapper type > instead of a KnownNotNull tag? We have a mozilla::NotNull in mfbt already: http://dxr.mozilla.org/mozilla-central/source/mfbt/NotNull.h I think it would be ideal to get to the point where we could say: > void* operator new(size_t, mozilla::NotNull<void*> p); and things would Just Work, but doing that efficiently with the release-mode asserts that NotNull currently has is a bit difficult.
I added the MOZ_NONNULL suggestions that Dan made. I also needed a bit of adjustment to the JIT to accommodate this new style of placement new; it's possible that qualifying the |new| call in Vector would actually be better...in any event, nbp irc-r+'d this, so carrying over r+.
Attachment #8780593 - Flags: review+
Attachment #8780285 - Attachment is obsolete: true
Pushed by firstname.lastname@example.org: https://hg.mozilla.org/integration/mozilla-inbound/rev/e3608df9fe09 introduce a non-null-checking placement operator new; r=sunfish,nbp
Pushed by email@example.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/522135225936 Don't tag |void* p| in the don't-nullcheck placement operator new overload as MOZ_NONNULL, because that makes a double-checking assertion of non-nullness into a compiler warning. r=froydnj
You need to log in before you can comment on or make changes to this bug.