abort() on malloc() failure in new with exceptions disabled

[kripken]

When libc++/libc++abi are built with exceptions disabled, the
new implementation there does not throw an exception for
an error, but it also does nothing else. So new ends up returning
a zero if malloc did, which can break programs.

Technically libc++/libc++abi are doing a reasonable thing here,
just removing all exceptions-related code when exceptions are
disabled. The assumption is likely that a user program would
set a new_handler if an error is desired. For us, we have to change
this as our default mode is to have exceptions disabled, and we
don't want users to need to know they need to do anything.

This makes it abort instead. (Note that without growth this
happened to always work, since we abort on any failing allocation.
With growth enabled, though, malloc returns 0, and we end up
in this situation.)

Fixes #11042 As discussed there I also looked at the option of
installing a set_new_handler that does an abort. That ends up
increasing code size by a little (bit less than 1%) because if adds
a global constructor, a function to the table, and some memory
operations. It seems better to just modify new itself which avoids
all that.

[sbc100]

Does doesn't seem related to memory growth. Why not just not set any settings and leave the default 16Mb memory. The result should be the same.

Then we can just call this test something like: test_aborting_new? (The name "new error" makes it sounds to me like new is an adjective).

[kripken]

I'll add a comment - this is actually related to memory growth. When growth is disabled, we abort() in another code path (the code to grow memory looks very different in the two cases).

[sbc100]

Nice! I like it.

It does make me a little sad the we have to case much about such a small number of bytes that we can't use choose not to use standard mechanisms. But I accept those are the compromises we are making.

[kripken]

Renamed test, added comments, and adjusted the error messages, as suggested.

It does make me a little sad the we have to case much about such a small number of bytes that we can't use choose not to use standard mechanisms.

One possibility to make this more standard is to optimize out that overhead. If the ctor-evaller worked then at least the constructor wouldn't be an issue, which might be good enough.

[sbc100]

Renamed test, added comments, and adjusted the error messages, as suggested.

It does make me a little sad the we have to case much about such a small number of bytes that we can't use choose not to use standard mechanisms.

One possibility to make this more standard is to optimize out that overhead. If the ctor-evaller worked then at least the constructor wouldn't be an issue, which might be good enough.

Hmm.. that makes me see this change differently then. From looking at the change I can't see why memory growth should effect this. Does malloc(X) return NULL in the non-growable mode too?

[sbc100]

The title and description of the change don't talk about growth....

[kripken]

There are two factors here.

First, that there are multiple code paths. The code for growth looks very different if growth is enabled or not. If growth is not enabled, then the grow memory function is trivial, and always worked! But if growth is allowed then we have a bunch of code there to try pick how much to grow memory, that tries to grow, and that handles things, etc. And that code path returns "failure" if growth fails. (Note that as I mentioned in the issue, perhaps it is slightly inconsistent that growth traps if growth is not enabled at all, but returns failure if growth is allowed but it failed in practice. I don't want to change that behavior now, though.)

Second, this is specific to C++. With plain C, when growth is enabled but it failed and it returned 0, it would return 0 from malloc. That's correct for C. With C++, it's not ok for new to return 0, it should never do that. But that's the default behavior in libc++/libc++abi, so this PR changes that.

[sbc100]

Sorry, I still don't understand why this test doesn't pass when growth is disabled.

Why doesn't the malloc on line 67 return 0 in both cases? its malloc trapping in the non-growth case? Should we fix that first?

[kripken]

its malloc trapping in the non-growth case?

Yes.

Should we fix that first?

While, as I said, it's arguable that there is an inconsistency, I don't think so. First, it's always wrong for new to return 0. That it happens to only happen in this path is irrelevant - we should ensure new never returns 0, period. That's all this PR does.

Second, as I said it's arguable whether we should change that behavior. It may be a little inconsistent, but it is quite convenient in practice and matches what most users want. We've not gotten complaints that I can remember. The current model is:

• Without growth, trap on running out of memory.
• With growth, you are opting into a more standard memory management model. Memory can grow and an attempt to grow can succeed or fail, and when it fails, malloc returns 0.

[sbc100]

OK, can you at least add a comment about why this test only passed with growth enabled. Something along the lines of: "Only makes since with growth enabled, since when growth is disabled, malloc already traps".

But wait.. now that I write the sentence it seems that the trapping behaviour should happen both with and without growth. In one case malloc itself traps and in the other case we are adding this abort so new will trap. If this is true, can we run the test both with and without growth to make sure it traps in both cases? .. then we can can call the test test_trapping_new since its checking for trapping under all circumstances? :)

[kripken]

Sure, added a comment, renamed, and made it run on non-growth too.

[sbc100]

Nice! Thanks for being patient with all comments.