Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Operator new improvements #287

Closed
matthijskooijman opened this issue Sep 19, 2019 · 2 comments · Fixed by #361
Closed

Operator new improvements #287

matthijskooijman opened this issue Sep 19, 2019 · 2 comments · Fixed by #361

Comments

@matthijskooijman
Copy link
Collaborator

In arduino/Arduino#108, some improvements to the operator new provided by Arduino were suggested (since the current forms are not standards-compliant wrt error handling IIRC). The PR itself originally suggested to remove them and include uclibc++ instead, but after some discussion, we concluded that it would be better to:

  • Copy the new and delete implementations from uclibc++ (including array, placement and nothrow versions).
  • Change it to remove throw(std::bad_alloc).
  • Define a weak std::__throw_bad_alloc that calls std::terminate()
  • Define a weak std::terminate that calls abort()

I think the PR itself is not really useful directly, but the dicussion on it is. I do not recall the details right now, but wanted to at least keep record of this here.

@matthijskooijman
Copy link
Collaborator Author

matthijskooijman commented Sep 17, 2020

This issue became relevant recently, when #340 was merged. With that, the Arduino core exposes a <new> header (in addition to the <new.h> that was already present), but it is incomplete. In particular, it does not define a nothrow_t type, making code that relies on it (in particular libraries like boost and uclibc++/ArduinoSTL) fail (see e.g. mike-matera/ArduinoSTL#56). Before this change, the <new> header from e.g. ArduinoSTL would be used and things would work, but now the Arduino version takes precedence. So we had better make sure it properly exposes everything the C++ spec mandates (at least for as far as possible without exceptions)

I had another look at this and planning to submit a fixing PR. I think the above steps are still good, except that we should probably not copy the uclibc++ implementation completely. Changes I think we need compared to uclibc++:

  • The uclibc++ implementation defines different new and delete operators independently, when the specification (based on cppreference.com) requires the library/default versions to call each other so overriding just one version is enough.
  • The spec also requires that some of these operators can be overridden by user code. In practice, I think they should be weak.
  • The C++11 version (which Arduino uses) has noexcept rather than throw() (on the non-throwing new and delete operators). Newer compilers even warn about throw(), so better use noexcept.

As for calling std::terminate (through std::__throw_bad_alloc) on failed allocations, I'm not so sure if we should really do this right away. It is certainly a good idea, but any code that is properly handling failed allocations now will instantly be broken by this change. The proper way to do this is to use the std::nothrow_t versions, but since those are not currently exposed, code cannot do things properly right now. I'd suggest we add the nothrow versions first, then after some time, change the implementation to terminate rather then return NULL (probably add the modified code now already, but guarded by a define).

With the above, instead of using the uclibc++ coe, I think we might end up just using the existing implementation and modifying it to correctly handle 0-byte allocations and the above things, and maybe also call __throw_bad_alloc, since that also simplifies the licensing situation.

One open question is what should we implement exactly? Ideally, everything mandated by the standard for C++11, with the following differences:

  • We can probably even add the few C++14 additions as well, guarded by the __cplusplus macro, to simplify upgrading to C++14 later. C++17 adds quite some things, so I'd rather not do that now.
  • The exception classes must be left out (they are simple enough to declare anyway, except they derive form the exception class from the <exception> header that we do not (want to) offer, so better to just omit them entirely.
  • The new_handler stuff seems tricky (that registers a global function that is called whenever memory allocation fails to free up memory, or decide whether to terminate or throw). We could support it, but that would significantly complicate the allocation functions, so I'd rather not. I would propose a compromise: Declare the functions but not implement them. This allows a library (such as uclibc++, though that currently allows setting a new handler but not actually uses it) to provide an implementation for this function and override operator new to call the handler. This does require declaring the function in our <new> file, since libraries cannot add anything to that file. This also means that if code tries to use this new handler, then it will compile, but fail at link time, which I think is acceptable.

If we implement the __throw_bad_alloc / std::terminate route above, there is one open question of where to declare and define these. I think the standard puts them in <exception>, but we do not want to offer that (since we must then again make it complete, since e.g. uclibc++ cannot override it). I think a good compromise would be to just use forward declarations in new.cpp of these functions and define them (weakly) in maybe abi.cpp or so. Any library that wants to replace them can just declare and define them and (provided their version has the same typing), they will then just be used.

matthijskooijman added a commit to matthijskooijman/ArduinoCore-avr that referenced this issue Sep 17, 2020
This makes this header complete up to including C++14, except two
exception classes that cannot be defined without `<exception>`.

The functions related to the "new_handler" are declared but not actually
defined, to prevent overhead and complexity. They are still declared to
allow implementing them in user code if needed.

This makes the implementation of all operator new and delete functions
comply with the C++11/C++14 specification in terms of which should be
actually implemented and which should be delegate to other functions.

There are still some areas where these implementations are not entirely
standards-compliant, which will be fixed in subsequent commits.

This fixes part of arduino#287 and fixes arduino#47.
matthijskooijman added a commit to matthijskooijman/ArduinoCore-avr that referenced this issue Sep 17, 2020
matthijskooijman added a commit to matthijskooijman/ArduinoCore-avr that referenced this issue Sep 17, 2020
This is currently disabled, keeping the old behavior of returning
NULL on failure, but should probably be enabled in the future as code
that does want to do a null check has had a chance to switch to the
more portable nothrow versions.

When enabled, allocation failure calls the weak `std::terminate()`,
which calls `abort()` by default, but can be replaced by user code to do
more specific handling.

To enable this, a macro must be defined (in new.cpp or on the compiler
commandline).

This fixes part of arduino#287.
matthijskooijman added a commit to matthijskooijman/ArduinoCore-avr that referenced this issue Sep 17, 2020
This makes this header complete up to including C++14, except two
exception classes that cannot be defined without `<exception>`.

The functions related to the "new_handler" are declared but not actually
defined, to prevent overhead and complexity. They are still declared to
allow implementing them in user code if needed.

This makes the implementation of all operator new and delete functions
comply with the C++11/C++14 specification in terms of which should be
actually implemented and which should be delegate to other functions.

There are still some areas where these implementations are not entirely
standards-compliant, which will be fixed in subsequent commits.

This fixes part of arduino#287 and fixes arduino#47.
matthijskooijman added a commit to matthijskooijman/ArduinoCore-avr that referenced this issue Sep 17, 2020
matthijskooijman added a commit to matthijskooijman/ArduinoCore-avr that referenced this issue Sep 17, 2020
This is currently disabled, keeping the old behavior of returning
NULL on failure, but should probably be enabled in the future as code
that does want to do a null check has had a chance to switch to the
more portable nothrow versions.

When enabled, allocation failure calls the weak `std::terminate()`,
which calls `abort()` by default, but can be replaced by user code to do
more specific handling.

To enable this, a macro must be defined (in new.cpp or on the compiler
commandline).

This fixes part of arduino#287.
@matthijskooijman
Copy link
Collaborator Author

I submitted a PR, implementing pretty much what I proposed above. I did leave out the __throw_bad_alloc() function and opted to just call std::terminate() directly, since the __throw_bad_alloc() seemed like a uclibc++-specific concept that is not defined in standard C++ and it seemed rather meaningless in an exceptionless environment, so I left it out.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant