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

new w/ OOM now aborts by defaults, or throw an exception #7536

Merged
merged 11 commits into from
Aug 31, 2020

Conversation

d-a-v
Copy link
Collaborator

@d-a-v d-a-v commented Aug 17, 2020

Changes:

  • remove legacy new behavior, c++ operator new is supposed to never return nullptr
    • when exceptions are enabled: a catchable exception is raised
    • when exceptions are disabled (defaults): abort is called: caller address is showed for debuggers
  • unused arduino_new has exactly the same behavior as new (std::nothrow), removing the first.
  • all source code with new which are checking the returned value are now calling new (std::nothrow) instead
    (the others will abort on oom, then reboot)
  • malloc is untouched: it returns nullptr on oom like new (std::nothrow)

@d-a-v d-a-v changed the title (WIP) replace new by new (std::nothrow), remove arduino_new replace new by new (std::nothrow), remove arduino_new Aug 23, 2020
@d-a-v d-a-v changed the title replace new by new (std::nothrow), remove arduino_new new w/ OOM now aborts by defaults, or throw an exception Aug 24, 2020
@TD-er
Copy link
Contributor

TD-er commented Aug 30, 2020

Can we also have a build option to deliberately keep the old situation?
Or else my code will cause crashes where it is checking the returned pointer as soon as this gets merged.
And right now I cannot change the code to call new(std::nothrow) as it is not yet merged.
I may need to make a macro for it or else my code will become quite messy while I still want to support old and new core libs.

@d-a-v
Copy link
Collaborator Author

d-a-v commented Aug 30, 2020

new (std::nothrow) is currently supposed to work. Did you try it ?

@devyte
Copy link
Collaborator

devyte commented Aug 30, 2020

@TD-er the whole point of this PR is to clean up the current situation and better conform to the C++ language standard. That means removing the legacy behavior.
Although this is expected to be merged soon, you have ample time to migrate, assuming you really need it.
@d-a-v 's question is valid: new(std::nothrow) is supposed to work now. If it doesn't, We Need To Know.

@TD-er
Copy link
Contributor

TD-er commented Aug 30, 2020

I have not tried it yet.
I just assumed it wouldn't because this PR implemented it...

I will try to see if it compiles using the current core code and will start making a macro for it.

@TD-er
Copy link
Contributor

TD-er commented Aug 30, 2020

OK, next time I should try it first before panic.
It does compile just fine, and the changes do appear quite easy to do in my code (not that many new calls)

Copy link
Collaborator

@earlephilhower earlephilhower left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks very nice! Smart move to overload the new(std::nothrow&) operator, I was wondering how you'd fix things.

@devyte
Copy link
Collaborator

devyte commented Aug 31, 2020

Thanks for this, finally this is cleaned up!

@devyte devyte merged commit 5b767a3 into esp8266:master Aug 31, 2020
@devyte devyte added this to the 3.0.0 milestone Aug 31, 2020
if (ctx)
delete ctx;
if (sha256)
delete sha256;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is if (...) redundant here? afaik, delete will be no-op with nullptr

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't know which one is nullptr

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

meaning, we can delete both. one nullptr, another created (or both nullptr)
i.e. https://godbolt.org/z/63PKa3

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

Successfully merging this pull request may close these issues.

5 participants