Don't infer attributes of virtual calls based on the function body #137669
+123
−38
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
rustbot
added
S-waiting-on-review
|
r? codegen |
bjorn3
reviewed
Feb 26, 2025
bjorn3
reviewed
Feb 26, 2025
DianQK
force-pushed
the
fn-atts-virtual
branch
from
a3711e5 to
9c90415
Compare
saethlin
reviewed
Feb 26, 2025
compiler/rustc_ty_utils/src/abi.rs
Outdated
| @@ -648,7 +635,15 @@ fn fn_abi_new_uncached<'tcx>( | |||
| conv, | |||
| can_unwind: fn_can_unwind(cx.tcx(), fn_def_id, sig.abi), | |||
There was a problem hiding this comment.
Do we have the same bug in fn_can_unwind where we are consulting the DefId even if the call is virtual? It sure looks like the same bug to me.
There was a problem hiding this comment.
I double-checked, and you're right. #[rustc_nounwind] can modify the unwinding behavior. But I think that should be a separate pull request: https://rust.godbolt.org/z/aTWsEMxWq.
There was a problem hiding this comment.
But I think that should be a separate pull request: https://rust.godbolt.org/z/aTWsEMxWq.
Or we need an explicit semantic on the trait?
There was a problem hiding this comment.
Well this is a rustc_ attribute, which in general means it is a hack and not well-designed. So while there is definitely some good design space for declaring non-unwinding, I think just patching up the compiler to make #[rustc_nounwind] sound (which is the idea) is enough for now.
|
Can you squash the commits a little? At least combine the middle two, I think the first and last commits can stand on their own if you want but the middle two are kind of the same thing. r? saethlin |
saethlin
added
the
beta-nominated
DianQK
force-pushed
the
fn-atts-virtual
branch
from
d564c88 to
fbe0075
Compare
|
I have squashed. @bors r=saethlin rollup=never p=1 |
bors
added
S-waiting-on-bors
bors
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Feb 27, 2025
Don't infer attributes of virtual calls based on the function body Fixes (after backport) rust-lang#137646. Since we don't know the exact implementation of the virtual call, it might write to parameters, we can't infer the readonly attribute.
|
The job Click to see the possible cause of the failure (guessed by this bot) |
|
💔 Test failed - checks-actions |
bors
added
S-waiting-on-review
bjorn3
reviewed
Feb 27, 2025
| can_unwind: fn_can_unwind(cx.tcx(), fn_def_id, sig.abi), | ||
| can_unwind: fn_can_unwind( | ||
| tcx, | ||
| // Since `#[rustc_nounwind]` can change unwinding, we cannot infer unwinding by `fn_def_id` for a virtual call. |
There was a problem hiding this comment.
If you apply #[rustc_nounwind] on a trait method, it should be fine to assume that all calls of this trait method don't unwind, right? If any of the implementations doesn't use #[rustc_nounwind], that should probably be reported as error.
There was a problem hiding this comment.
As it stands, the semantics are somewhat ambiguous; rustc currently only applies this to the annotated function. But I also think the constraint is fine. I will create an issue for it.
|
@bors retry |
bors
added
S-waiting-on-bors
|
@bors p=5 (rollup scheduling) |
|
☀️ Test successful - checks-actions |
|
Finished benchmarking commit (e6059f5): comparison URL. Overall result: no relevant changes - no action needed@rustbot label: -perf-regression Instruction countThis benchmark run did not return any relevant results for this metric. Max RSS (memory usage)Results (secondary -2.4%)This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
CyclesResults (primary -2.6%, secondary 2.5%)This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
Binary sizeResults (primary -0.0%)This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
Bootstrap: 770.19s -> 771.509s (0.17%) |
rustbot
added
the
beta-accepted
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes (after backport) #137646.
Since we don't know the exact implementation of the virtual call, it might write to parameters, we can't infer the readonly attribute.