-
Notifications
You must be signed in to change notification settings - Fork 390
Support weak definitions #4414
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
Support weak definitions #4414
Conversation
This has been extracted out of rust-lang/rust#134522. |
5a433c4
to
2402b40
Compare
src/shims/foreign_items.rs
Outdated
if let Some((instance, _, _)) = instance_and_crate { | ||
if !matches!(tcx.def_kind(instance.def_id()), DefKind::Fn | DefKind::AssocFn) { | ||
throw_ub_format!( | ||
"attempt to call an exported symbol that is not defined as a function" | ||
); | ||
} | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess this logic can live here as well, but is there a specific reason you moved it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not that I recall.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It will make a difference if there's a weak symbol that's not a function and a "strong" symbol that is. That's probably fine?
@rustbot author |
what's the status of this one @bjorn3? (I expect just little time which is very fair <3) |
Somehow I didn't notice there were outstanding review comments. |
59a946e
to
de4a356
Compare
@rustbot ready |
src/shims/foreign_items.rs
Outdated
if let Some(SymbolTarget { | ||
instance: original_instance, | ||
cnum: original_cnum, | ||
is_weak: original_is_weak, | ||
}) = symbol_target |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if let Some(SymbolTarget { | |
instance: original_instance, | |
cnum: original_cnum, | |
is_weak: original_is_weak, | |
}) = symbol_target | |
if let Some(original) = symbol_target |
seems more concise and more clear
src/shims/foreign_items.rs
Outdated
if let Some((instance, _, _)) = instance_and_crate { | ||
if !matches!(tcx.def_kind(instance.def_id()), DefKind::Fn | DefKind::AssocFn) { | ||
throw_ub_format!( | ||
"attempt to call an exported symbol that is not defined as a function" | ||
); | ||
} | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It will make a difference if there's a weak symbol that's not a function and a "strong" symbol that is. That's probably fine?
} | ||
interp_ok(()) | ||
})?; | ||
|
||
e.insert(instance_and_crate.map(|ic| ic.0)) | ||
if let Some(SymbolTarget { instance, .. }) = symbol_target { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if let Some(SymbolTarget { instance, .. }) = symbol_target { | |
// Once we identified the instance corresponding to the symbol, ensure | |
// it is a function. It is okay to encounter non-functions in the search above | |
// as long as the final instance we arrive at is a function. | |
if let Some(SymbolTarget { instance, .. }) = symbol_target { |
@rustbot author |
@rustbot ready |
Thanks! |
This PR was rebased onto a different master commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
When a symbol only has a weak definition, this definition will be picked. When a symbol has both a weak and a regular definition, the regular definition will be picked instead.
Rebased and squashed. @rustbot ready |
When a symbol only has a weak definition, this definition will be picked. When a symbol has both a weak and a regular definition, the regular definition will be picked instead.
In the future the allocator shim may be replaced by weak definitions. This already adds support to simplify things in the future.