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

default_alloc_error_hook: explain difference to default __rdl_oom in alloc #124059

Merged
merged 1 commit into from
May 3, 2024

Conversation

RalfJung
Copy link
Member

@RalfJung RalfJung commented Apr 17, 2024

Though I'm not sure if that is really the reason that this code is duplicated. On no_std it may already be possible to call user-defined code on allocation failure.

@rustbot
Copy link
Collaborator

rustbot commented Apr 17, 2024

r? @m-ou-se

rustbot has assigned @m-ou-se.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Apr 17, 2024
@rust-log-analyzer

This comment has been minimized.

@RalfJung
Copy link
Member Author

Hm, actually there is a subtle difference in behavior... currently the default path taken does not invoke any user-defined code. That seems worth preserving for now. I'll change the PR to just add a comment instead.

@RalfJung RalfJung force-pushed the default_alloc_error_hook branch from 3efa247 to b5ec268 Compare April 17, 2024 08:06
@RalfJung RalfJung changed the title default_alloc_error_hook: call the default hook for alloc instead of duplicating it default_alloc_error_hook: explain difference to default __rdl_oom in alloc Apr 17, 2024
@RalfJung RalfJung force-pushed the default_alloc_error_hook branch from b5ec268 to cb13c4d Compare May 3, 2024 13:10
@RalfJung
Copy link
Member Author

RalfJung commented May 3, 2024

r? libs

@rustbot rustbot assigned workingjubilee and unassigned m-ou-se May 3, 2024
Comment on lines 357 to 358
// Crucially, it does *not* call any user-defined code, and therefore there are no potential
// reentrancy issues. That makes it different from the default `__rdl_oom` defined in alloc.
Copy link
Member

Choose a reason for hiding this comment

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

no potential reentrancy issues.

innit more like "no potential reentrancy issues aside from the ones we foist on ourselves"?

__rdl_oom is an awfully descriptive name, congratulations on us for coming up with that one.

Copy link
Member

Choose a reason for hiding this comment

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

You describe two possible differences, and either of those can independently vary, technically. You should note that __rdl_oom calls like handle_alloc_error() or whatever it actually does.

Copy link
Member Author

Choose a reason for hiding this comment

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

I have tried to clarify both of these points, does that help?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, thank you!

@RalfJung RalfJung force-pushed the default_alloc_error_hook branch from cb13c4d to 3f6703b Compare May 3, 2024 17:12
@workingjubilee
Copy link
Member

@bors r+ rollup

@bors
Copy link
Contributor

bors commented May 3, 2024

📌 Commit 3f6703b has been approved by workingjubilee

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 3, 2024
workingjubilee added a commit to workingjubilee/rustc that referenced this pull request May 3, 2024
… r=workingjubilee

default_alloc_error_hook: explain difference to default __rdl_oom in alloc

Though I'm not sure if that is really the reason that this code is duplicated. On no_std it may already be possible to call user-defined code on allocation failure.
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request May 3, 2024
… r=workingjubilee

default_alloc_error_hook: explain difference to default __rdl_oom in alloc

Though I'm not sure if that is really the reason that this code is duplicated. On no_std it may already be possible to call user-defined code on allocation failure.
bors added a commit to rust-lang-ci/rust that referenced this pull request May 3, 2024
…iaskrgr

Rollup of 7 pull requests

Successful merges:

 - rust-lang#122492 (Implement ptr_as_ref_unchecked)
 - rust-lang#123815 (Fix cannot usage in time.rs)
 - rust-lang#124059 (default_alloc_error_hook: explain difference to default __rdl_oom in alloc)
 - rust-lang#124510 (Add raw identifier in a typo suggestion)
 - rust-lang#124555 (coverage: Clean up creation of MC/DC condition bitmaps)
 - rust-lang#124593 (Describe and use CStr literals in CStr and CString docs)
 - rust-lang#124630 (CI: remove `env-x86_64-apple-tests` YAML anchor)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 8e3f61b into rust-lang:master May 3, 2024
6 checks passed
@rustbot rustbot added this to the 1.80.0 milestone May 3, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request May 3, 2024
Rollup merge of rust-lang#124059 - RalfJung:default_alloc_error_hook, r=workingjubilee

default_alloc_error_hook: explain difference to default __rdl_oom in alloc

Though I'm not sure if that is really the reason that this code is duplicated. On no_std it may already be possible to call user-defined code on allocation failure.
@RalfJung RalfJung deleted the default_alloc_error_hook branch May 4, 2024 06:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants