introduce a non-null-checking placement operator new

RESOLVED FIXED in Firefox 51

Status

()

defect
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: froydnj, Assigned: froydnj)

Tracking

unspecified
mozilla51
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox51 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

Assignee

Description

3 years ago
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.
Assignee

Comment 1

3 years ago
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)
Assignee

Comment 2

3 years ago
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.
Assignee

Comment 5

3 years ago
(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.
Assignee

Comment 6

3 years ago
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+
Assignee

Updated

3 years ago
Attachment #8780285 - Attachment is obsolete: true

Comment 7

3 years ago
Pushed by nfroyd@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e3608df9fe09
introduce a non-null-checking placement operator new; r=sunfish,nbp

Comment 8

3 years ago
Pushed by jwalden@mit.edu:
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

Comment 9

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/e3608df9fe09
https://hg.mozilla.org/mozilla-central/rev/522135225936
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
You need to log in before you can comment on or make changes to this bug.