Resolve instance for SymFn in global/naked asm #140374
Conversation
|
r? @davidtwco rustbot has assigned @davidtwco. Use |
rustbot
added
S-waiting-on-review
|
Some changes occurred in compiler/rustc_codegen_cranelift cc @bjorn3 Some changes occurred in compiler/rustc_codegen_ssa |
|
r=me after comment/rename on |
compiler-errors
force-pushed
the
global_asm-bug
branch
from
3d7402b to
9cb3727
Compare
compiler-errors
force-pushed
the
global_asm-bug
branch
from
9cb3727 to
844b1e2
Compare
|
@bors r=lcnr rollup |
bors
added
S-waiting-on-bors
| pub fn new(def_id: DefId, args: GenericArgsRef<'tcx>) -> Instance<'tcx> { | ||
| /// Creates a new [`InstanceKind::Item`] from the `def_id` and `args`. Note that this | ||
| /// does not resolve trait items to their corresponding impl items -- to do that, use | ||
| /// [`Instance::expect_resolve`] instead. |
There was a problem hiding this comment.
As a compiler dev I would be rather clueless about whether I want to "resolve trait items to their corresponding impl items" or not -- I'm not even sure what exactly this means. So I'm afraid this comment isn't very helpful in terms of giving guidance for when I should use which of these functions.
There was a problem hiding this comment.
Do you have a suggestion for a new comment? Because just saying it is not very helpful as a comment is... well, also not very actionable 😆
There was a problem hiding this comment.
See the comment on https://doc.rust-lang.org/nightly/nightly-rustc/rustc_middle/ty/struct.Instance.html#method.try_resolve to understand what it means to resolve a trait item to its corresponding impl item.
There was a problem hiding this comment.
I guess what matters is: "Are you certain that the body of def_id is the actual body you want to use. This is not the case for trait methods or functions we may stub out with a separate implementation"
There was a problem hiding this comment.
Do you have a suggestion for a new comment? Because just saying it is not very helpful as a comment is... well, also not very actionable 😆
I'm not sure how to suggest a better comment without having any clue about when this function should or should not be used.^^ You made const-eval use new_raw so I guess it is the right thing to use for anything I encountered in the compiler so far...
There was a problem hiding this comment.
You made const-eval use new_raw so I guess it is the right thing to use for anything I encountered in the compiler so far...
CTFE uses Instance::expect_resolve all throughout, so that's definitely not true. The only usage sites I've touched are the ones that attempt to directly evaluate const bodies.
There was a problem hiding this comment.
I guess what matters is: "Are you certain that the body of
def_idis the actual body you want to use. This is not the case for trait methods or functions we may stub out with a separate implementation"
That sounds helpful. Are there docs for the concept of "resolve" that we can link to? I guess it refers to going from a trait-method pair to the actual impl? Also, is there ever a downside to using resolve when we don't have to?
new_raw is scary as a name so now I feel like every use of this function needs a comment justifying it, but I'm not sure what the justification looks like in the query wrappers.
There was a problem hiding this comment.
Also, is there ever a downside to using resolve when we don't have to?
Query calls, so perf. I'll open a follow-up to reduce the number of new_raw calls b/c it'll need a perf run and I see no reason to block this PR unnecessarily, but yes, the name was intentionally chosen to discourage usage of this function and to prompt compiler devs to question whether it's the appropriate function to call -- it's the root cause of this bug after all.
compiler-errors
force-pushed
the
global_asm-bug
branch
3 times, most recently
from
d30ad80 to
9ac697f
Compare
|
@bors r=lcnr |
| /// implementation, and which may not even have a body themselves. Usages of | ||
| /// this function should probably use [`Instance::expect_resolve`], or if run | ||
| /// in a polymorphic environment or within a lint (that may encounter ambiguity) | ||
| /// [`Instance::expect_resolve`] instead. |
There was a problem hiding this comment.
| /// implementation, and which may not even have a body themselves. Usages of | |
| /// this function should probably use [`Instance::expect_resolve`], or if run | |
| /// in a polymorphic environment or within a lint (that may encounter ambiguity) | |
| /// [`Instance::expect_resolve`] instead. | |
| /// implementation, and which may not even have a body themselves. Instead of | |
| /// this function, you probably should use use [`Instance::expect_resolve`], or if run | |
| /// in a polymorphic environment or within a lint (that may encounter ambiguity) | |
| /// [`Instance::expect_resolve`] instead. |
Also, this recommends expect_resolve twice now...
compiler-errors
force-pushed
the
global_asm-bug
branch
from
9ac697f to
8f55844
Compare
|
@bors r=lcnr rollup |
…lcnr Resolve instance for SymFn in global/naked asm `Instance::expect_resolve` ensures that we're actually going from trait item -> impl item. Fixes rust-lang#140373
…enton Rollup of 9 pull requests Successful merges: - rust-lang#139308 (add autodiff inline) - rust-lang#140276 (Do not compute type_of for impl item if impl where clauses are unsatisfied) - rust-lang#140302 (Move inline asm check to typeck, properly handle aliases) - rust-lang#140323 (Implement the internal feature `cfg_target_has_reliable_f16_f128`) - rust-lang#140374 (Resolve instance for SymFn in global/naked asm) - rust-lang#140391 (Rename sub_ptr to offset_from_unsigned in docs) - rust-lang#140394 (Make bootstrap git tests more self-contained) - rust-lang#140396 (Workaround for windows-gnu rust-lld test failure) - rust-lang#140402 (only return nested goals for `Certainty::Yes`) r? `@ghost` `@rustbot` modify labels: rollup
|
failed in rollup #140411 (comment) @bors r- |
bors
added
S-waiting-on-author
Instance::expect_resolveensures that we're actually going from trait item -> impl item.Fixes #140373