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

fix ICE when asm_const and const_refs_to_static are combined #129472

Merged
merged 2 commits into from
Sep 6, 2024

Conversation

folkertdev
Copy link
Contributor

@folkertdev folkertdev commented Aug 23, 2024

fixes #129462
fixes #126896
fixes #124164

I think this is a case that was missed in the fix for #125558, which inserts a type error in the case of an invalid (that is, non-integer) type being passed to an asm const operand.

I'm not 100% sure that span_mirbug_and_err is the right macro here, but it is used earlier with builtin_deref and seems to do the trick.

r? @lcnr

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Aug 23, 2024
@folkertdev folkertdev force-pushed the const-refs-to-static-asm-const branch from 3805bdc to 4f65da6 Compare September 2, 2024 15:22
@oli-obk
Copy link
Contributor

oli-obk commented Sep 4, 2024

I think this should be fixing a buch of known other ICEs, too. Try running the crashes test suite

@folkertdev
Copy link
Contributor Author

no luck there

> ./x test --keep-stage 1 tests/crashes/

running 232 tests
........................................................................................  88/232
........................................................................................ 176/232
........................................................

test result: ok. 232 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 5.63s

@folkertdev folkertdev force-pushed the const-refs-to-static-asm-const branch 2 times, most recently from 5e33c81 to 774fbc7 Compare September 4, 2024 11:04
@rust-log-analyzer

This comment has been minimized.

@folkertdev folkertdev force-pushed the const-refs-to-static-asm-const branch from 774fbc7 to 957502d Compare September 4, 2024 12:03
// FIXME(#129952): We probably want a more principled approach here.
if let Err(terr) = ty.error_reported() {
self.infcx.set_tainted_by_errors(terr);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I think that maybe we want to do it for all signatures, i.e. assign this match to a variable and then check whether that variable has any error_reported. I expect that to fix even more issues 😁

I am otherwise quite happy with this change

Copy link
Contributor Author

Choose a reason for hiding this comment

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

no further crashes are fixed by that change. I did rename #129952 to reflect that the tainting now occurs in a different place.

@folkertdev folkertdev force-pushed the const-refs-to-static-asm-const branch from 957502d to 7ec7724 Compare September 4, 2024 13:11
@folkertdev folkertdev force-pushed the const-refs-to-static-asm-const branch from 7ec7724 to 49e3b9a Compare September 4, 2024 18:07
@lcnr
Copy link
Contributor

lcnr commented Sep 5, 2024

thanks for your patience and good work ❤️

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Sep 5, 2024

📌 Commit 49e3b9a has been approved by lcnr

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 Sep 5, 2024
@bors bors merged commit 3daa015 into rust-lang:master Sep 6, 2024
6 checks passed
@rustbot rustbot added this to the 1.83.0 milestone Sep 6, 2024
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-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
7 participants