Tweak type inference for `const` operands in inline asm #125558

Amanieu · 2024-05-25T23:15:23Z

Previously these would be treated like integer literals and default to i32 if a type could not be determined. To allow for forward-compatibility with str constants in the future, this PR changes type inference to use an unbound type variable instead.

The actual type checking is deferred until after typeck where we still ensure that the final type for the const operand is an integer type.

rustbot · 2024-05-25T23:15:30Z

r? @michaelwoerister

rustbot has assigned @michaelwoerister.
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

Amanieu · 2024-05-25T23:20:11Z

Unfortunately this fails in tests when an invalid type is passed to const operands.

error: invalid type for `const` operand
  --> /home/amanieu/code/rust/tests/ui/asm/type-check-1.rs:61:20
   |
LL |         asm!("{}", const &0);
   |                    ^^^^^^--
   |                          |
   |                          is a `&i32`
   |
   = help: `const` operands must be of an integer type

error: internal compiler error: compiler/rustc_borrowck/src/universal_regions.rs:880:36: cannot convert `'{erased}` to a region vid

thread 'rustc' panicked at compiler/rustc_borrowck/src/universal_regions.rs:880:36:

This happens because the invalid type now reaches a later compilation stage and is processed by the borrow checker. This seems similar to #96304. @compiler-errors had a PR to fix this in #116087 but that was closed. I'm not exactly sure what the correct approach is for solving this.

lcnr · 2024-06-13T10:24:46Z

compiler/rustc_hir_analysis/src/check/intrinsicck.rs

-                // - Typeck has checked that Const operands are integers.
-                // - AST lowering guarantees that SymStatic points to a static.
-                hir::InlineAsmOperand::Const { .. } | hir::InlineAsmOperand::SymStatic { .. } => {}
+                hir::InlineAsmOperand::Const { anon_const } => {


a suggestion: you can move this check into fn anon_const_type_of and return the returned type with ty::Error if it's not an int. THis should also fix the ICE in mir borrowck

I tried this here master...folkertdev:rust:const-asm-type-fix and that makes the test work! However, error messages are now emitted out of order. I'm not sure how big a problem that is, and how to fix it if needed.

as an extra note, this issue appears to be entirely unrelated to #96304, lifetimes work fine in these const expressions.

core::arch::global_asm!("/* {} */", const Foo::<'static>::X); struct Foo<'a>(&'a ()); impl<'a> Foo<'a> { const X: u64 = 42; }

👍 yeah, that's my preferred solution, though I'd maybe keep the check in intrinsicck as an assert in case type_of gets changed in the future. I don't think the message order is something we should worry about here.

Amanieu · 2024-07-23T22:03:09Z

@rustbot ready

lcnr · 2024-07-23T22:10:32Z

compiler/rustc_hir_analysis/src/collect/type_of.rs

+                    Ty::new_error_with_message(
+                        tcx,
+                        span,
+                        format!("invalid type for `sym` operand"),


use the ErrorGuaranteed from the above emit

lcnr · 2024-07-23T22:10:36Z

compiler/rustc_hir_analysis/src/collect/type_of.rs

+                    Ty::new_error_with_message(
+                        tcx,
+                        span,
+                        format!("invalid type for `const` operand"),


Previously these would be treated like integer literals and default to `i32` if a type could not be determined. To allow for forward-compatibility with `str` constants in the future, this PR changes type inference to use an unbound type variable instead. The actual type checking is deferred until after typeck where we still ensure that the final type for the `const` operand is an integer type.

Co-authored-by: Amanieu d'Antras <[email protected]>

folkertdev · 2024-08-02T20:26:53Z

time to try again? or should we try some specific things? messing with the asm tests is kind of annoying, maybe we could have some sort of lint for the asm-support annotation?

rustbot · 2024-08-03T09:41:55Z

Failed to set assignee to ready: cannot assign: response: {"message":"Not Found","documentation_url":"https://docs.github.com/rest/issues/assignees#add-assignees-to-an-issue","status":"404"}

Note: Only org members with at least the repository "read" role, users with write permissions, or people who have commented on the PR may be assigned.

folkertdev · 2024-08-03T09:42:56Z

ok lol I can't mark this as ready, @Amanieu can you do that?

Amanieu · 2024-08-03T14:06:47Z

@rustbot ready

lcnr · 2024-08-05T13:20:48Z

@folkertdev what did you change since the last merge attempt? Would like to avoid having unstable tests in the ui test suite 🤔 could you try and look through the backtrace for the error to check where a hashmap may be involved as suggested by @oli-obk. Alternatively, put the backtrace into a gist and I can take a quick look

folkertdev · 2024-08-05T14:12:12Z

since the last merge attempt, only eb726a5 was added because that broke a test.

The tests in question are, based on the CI that we've run so far, completely stable in their current form.

In any case, here is a reduced testcase that, based on my understanding of the problem, should have the error in CI

https://gist.github.com/folkertdev/1afae9ede304e8c14040458039bd70c3

it triggers the same error using an invalid sym with both asm! and global_asm!. Locally, the asm! error comes first, but on CI it would come second. It's unclear to me which of the error messages is "at fault" (is one too early, or the other too late) so I've added traces for both.

"hash" does not occur, so it might be something different. But again, to my knowledge, this is not a problem any more for the current PR.

lcnr · 2024-08-05T14:40:13Z

@bors r+

bors · 2024-08-05T14:40:17Z

📌 Commit eb726a5 has been approved by lcnr

It is now in the queue for this repository.

Tweak type inference for `const` operands in inline asm Previously these would be treated like integer literals and default to `i32` if a type could not be determined. To allow for forward-compatibility with `str` constants in the future, this PR changes type inference to use an unbound type variable instead. The actual type checking is deferred until after typeck where we still ensure that the final type for the `const` operand is an integer type.

Rollup of 8 pull requests Successful merges: - rust-lang#122049 (Promote riscv64gc-unknown-linux-musl to tier 2) - rust-lang#125558 (Tweak type inference for `const` operands in inline asm) - rust-lang#128638 (run-make: enable msvc for `link-dedup`) - rust-lang#128647 (Enable msvc for link-args-order) - rust-lang#128649 (run-make: Enable msvc for `no-duplicate-libs` and `zero-extend-abi-param-passing`) - rust-lang#128656 (Enable msvc for run-make/rust-lld) - rust-lang#128688 (custom MIR: add support for tail calls) - rust-lang#128691 (Update `compiler-builtins` to 0.1.115) r? `@ghost` `@rustbot` modify labels: rollup

bors · 2024-08-06T01:20:47Z

⌛ Testing commit eb726a5 with merge c9687a9...

bors · 2024-08-06T03:43:40Z

☀️ Test successful - checks-actions
Approved by: lcnr
Pushing c9687a9 to master...

rust-timer · 2024-08-06T06:15:00Z

Finished benchmarking commit (c9687a9): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results (primary -2.1%)

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.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-2.1%	[-2.1%, -2.1%]	1
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-2.1%	[-2.1%, -2.1%]	1

Cycles

Results (secondary -6.7%)

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.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-6.7%	[-6.7%, -6.7%]	1
All ❌✅ (primary)	-	-	0

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 761.437s -> 762.407s (0.13%)
Artifact size: 336.80 MiB -> 336.88 MiB (0.03%)

…m-const, r=lcnr fix ICE when `asm_const` and `const_refs_to_static` are combined fixes rust-lang#129462 fixes rust-lang#126896 fixes rust-lang#124164 I think this is a case that was missed in the fix for rust-lang#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`

…m-const, r=lcnr fix ICE when `asm_const` and `const_refs_to_static` are combined fixes rust-lang#129462 fixes rust-lang#126896 fixes rust-lang#124164 I think this is a case that was missed in the fix for rust-lang#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``

…m-const, r=lcnr fix ICE when `asm_const` and `const_refs_to_static` are combined fixes rust-lang#129462 fixes rust-lang#126896 fixes rust-lang#124164 I think this is a case that was missed in the fix for rust-lang#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```

…m-const, r=lcnr fix ICE when `asm_const` and `const_refs_to_static` are combined fixes rust-lang#129462 fixes rust-lang#126896 fixes rust-lang#124164 I think this is a case that was missed in the fix for rust-lang#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````

Rollup merge of rust-lang#129472 - folkertdev:const-refs-to-static-asm-const, r=lcnr fix ICE when `asm_const` and `const_refs_to_static` are combined fixes rust-lang#129462 fixes rust-lang#126896 fixes rust-lang#124164 I think this is a case that was missed in the fix for rust-lang#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 assigned michaelwoerister May 25, 2024

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 May 25, 2024

Amanieu mentioned this pull request May 25, 2024

Tracking Issue for asm_const #93332

Closed

This comment has been minimized.

Sign in to view

compiler-errors self-assigned this May 26, 2024

michaelwoerister removed their assignment May 27, 2024

compiler-errors added S-experimental Status: Ongoing experiment that does not require reviewing and won't be merged in its current state. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 4, 2024

lcnr self-assigned this Jun 6, 2024

Amanieu marked this pull request as ready for review June 11, 2024 16:13

This comment has been minimized.

Sign in to view

Amanieu force-pushed the const-asm-type branch from d61ff43 to fe07e37 Compare June 11, 2024 16:51

This comment has been minimized.

Sign in to view

Amanieu force-pushed the const-asm-type branch from fe07e37 to ff7418f Compare June 11, 2024 18:05

lcnr reviewed Jun 13, 2024

View reviewed changes

Amanieu force-pushed the const-asm-type branch from ff7418f to 2bb74fa Compare July 22, 2024 18:16

rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 23, 2024

lcnr reviewed Jul 23, 2024

View reviewed changes

This comment has been minimized.

Sign in to view

Amanieu and others added 4 commits July 25, 2024 20:12

Work around rust-lang#96304 by ignoring one test case

575fc72

apply fix suggested by lcnr

97738e1

Apply suggestions from code review

c77b569

Co-authored-by: Amanieu d'Antras <[email protected]>

bors added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Aug 2, 2024

add needs-asm-support to invalid-sym-operand

eb726a5

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Aug 3, 2024

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 Aug 5, 2024

tgross35 mentioned this pull request Aug 5, 2024

Rollup of 8 pull requests #128698

Closed

bors added the merged-by-bors This PR was explicitly merged by bors. label Aug 6, 2024

bors merged commit c9687a9 into rust-lang:master Aug 6, 2024
7 checks passed

rustbot added this to the 1.82.0 milestone Aug 6, 2024

This was referenced Aug 23, 2024

ICE when combining asm_const and const_refs_to_static #129462

Closed

fix ICE when asm_const and const_refs_to_static are combined #129472

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Tweak type inference for `const` operands in inline asm #125558

Tweak type inference for `const` operands in inline asm #125558

Amanieu commented May 25, 2024

rustbot commented May 25, 2024

Amanieu commented May 25, 2024

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

lcnr Jun 13, 2024

folkertdev Jul 21, 2024

folkertdev Jul 21, 2024

lcnr Jul 22, 2024 •

edited

Loading

Amanieu commented Jul 23, 2024

lcnr Jul 23, 2024

lcnr Jul 23, 2024

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

folkertdev commented Aug 2, 2024 •

edited

Loading

rustbot commented Aug 3, 2024

folkertdev commented Aug 3, 2024

Amanieu commented Aug 3, 2024

lcnr commented Aug 5, 2024

folkertdev commented Aug 5, 2024

lcnr commented Aug 5, 2024

bors commented Aug 5, 2024

bors commented Aug 6, 2024

bors commented Aug 6, 2024

rust-timer commented Aug 6, 2024

Tweak type inference for const operands in inline asm #125558

Tweak type inference for const operands in inline asm #125558

Conversation

Amanieu commented May 25, 2024

rustbot commented May 25, 2024

Amanieu commented May 25, 2024

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lcnr Jul 22, 2024 • edited Loading

Choose a reason for hiding this comment

Amanieu commented Jul 23, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

folkertdev commented Aug 2, 2024 • edited Loading

rustbot commented Aug 3, 2024

folkertdev commented Aug 3, 2024

Amanieu commented Aug 3, 2024

lcnr commented Aug 5, 2024

folkertdev commented Aug 5, 2024

lcnr commented Aug 5, 2024

bors commented Aug 5, 2024

bors commented Aug 6, 2024

bors commented Aug 6, 2024

rust-timer commented Aug 6, 2024

Overall result: no relevant changes - no action needed

Tweak type inference for `const` operands in inline asm #125558

Tweak type inference for `const` operands in inline asm #125558

lcnr Jul 22, 2024 •

edited

Loading

folkertdev commented Aug 2, 2024 •

edited

Loading