mir: prohibit projection into scalable vec - #160642
Conversation
|
Some changes occurred to MIR optimizations cc @rust-lang/wg-mir-opt
cc @Amanieu, @folkertdev, @sayantn |
|
r? @TaKO8Ki rustbot has assigned @TaKO8Ki. Use Why was this reviewer chosen?The reviewer was selected based on:
|
|
r? @folkertdev |
| //@ compile-flags: --target aarch64-unknown-linux-gnu -Copt-level=0 | ||
| //@ check-fail | ||
| //@ needs-llvm-components: aarch64 | ||
| //@ only-aarch64 |
There was a problem hiding this comment.
is the --target and needs-llvm-components necessary when it's already restricted to aarch64?
There was a problem hiding this comment.
going in the other direction, I think this can be a minicore test?
There was a problem hiding this comment.
is the
--targetandneeds-llvm-componentsnecessary when it's already restricted to aarch64?
are you suggesting to remove both? (if you have a test with a --target, tidy will require needs-llvm-components)
There was a problem hiding this comment.
But combining //@ only-aarch64 with compile-flags: --target aarch64-unknown-linux-gnu doesn't really make sense I think?
Anyhow, this should be a minicore test so it run everywhere.
There was a problem hiding this comment.
Removed --target and needs-llvm-components, added minicore
There was a problem hiding this comment.
Oh, sorry, I meant to keep --target and needs-llvm-components in combination with minicore, so that this test runs even on my x86_64 machine. The logic here isn't os-specific, so defaulting to aarch64-unknown-linux-gnu should be fine. It's the only-aarch64 that should go.
|
cc @scottmcm Since you worked on prohibiting similar projections in the past. |
303387d to
168f22f
Compare
I looked at #143833 while doing this - the |
|
|
||
| #[derive(Diagnostic)] | ||
| #[diag("cannot project into scalable vector type `{$ty}`")] | ||
| pub(crate) struct ScalableVectorFieldAccess<'tcx> { |
There was a problem hiding this comment.
Are you expecting scalable vectors to be something that people can mess up like this outside of the library? We intentionally just ICE under rust-lang/compiler-team#620 for the misuses of repr(simd).
(The tricky cases were things that didn't have a great place in the code to point at.)
There was a problem hiding this comment.
Ah, that makes sense, I hadn't considered that the visibility errors would prevent field access normally, so this was really only for direct uses of rustc_scalable_vector, so the MIR validator error is sufficient - removed the typeck error.
| } | ||
|
|
||
| if adt_def.repr().simd() { | ||
| if adt_def.repr().simd() || adt_def.repr().scalable() { |
There was a problem hiding this comment.
Would it worth having a negative test that mir-opt SRoA doesn't try to explode one of these, or similar?
168f22f to
c1f9520
Compare
c1f9520 to
ae4a53f
Compare
This comment was marked as resolved.
This comment was marked as resolved.
ae4a53f to
8574222
Compare
This comment has been minimized.
This comment has been minimized.
|
Hmm i've been running into this too. Tidy already lints on tests having |
8574222 to
0e878d6
Compare
|
@bors r+ |
…folkertdev mir: prohibit projection into scalable vec Fixes rust-lang#160580. Preventing projections into scalable vectors is an oversight from the initial implementation and something we should fix. I'm surprised it caused a stdarch CI failure as reported by rust-lang#160580, as nothing in rustc or stdarch seems to have changed that would have caused that to start happening as far as I can tell. This likely won't fix that stdarch CI failure if it keeps happening, because if there is a projection coming from somewhere then that needs to be fixed - nevertheless, preventing them as in this patch is the right thing to do. I've tested this against the stdarch CI locally.
Rollup of 5 pull requests Successful merges: - #160634 (miri subtree update) - #160642 (mir: prohibit projection into scalable vec) - #160303 (rustc_parse: A few cleanups to expression parsing next to attributes) - #160429 (tidy: Update Python version requirements to 3.10) - #160543 (Default `RawOsError` to `i16` for 16-bit targets)
| extern crate minicore; | ||
|
|
||
| #[rustc_scalable_vector(4)] | ||
| pub struct Sv(f32); |
There was a problem hiding this comment.
This needs to silence a lint.
---- [ui] tests/ui/scalable-vectors/project-into-field-extern.rs stdout ----
error: auxiliary build of /Users/runner/work/rust/rust/tests/ui/scalable-vectors/auxiliary/simple.rs failed to compile:
status: exit status: 1
command: env -u RUSTC_LOG_COLOR RUSTC_ICE="0" RUST_BACKTRACE="short" "/Users/runner/work/rust/rust/build/aarch64-apple-darwin/stage2/bin/rustc" "/Users/runner/work/rust/rust/tests/ui/scalable-vectors/auxiliary/simple.rs" "-Zsimulate-remapped-rust-src-base=/rustc/FAKE_PREFIX" "-Ztranslate-remapped-path-to-local-path=no" "-Z" "ignore-directory-in-diagnostics-source-blocks=/Users/runner/.cargo" "-Z" "ignore-directory-in-diagnostics-source-blocks=/Users/runner/work/rust/rust/vendor" "--sysroot" "/Users/runner/work/rust/rust/build/aarch64-apple-darwin/stage2" "--target=aarch64-apple-darwin" "--check-cfg" "cfg(test,FALSE)" "--error-format" "json" "--json" "future-incompat" "-Ccodegen-units=1" "-Zui-testing" "-Zdeduplicate-diagnostics=no" "-Zwrite-long-types-to-disk=no" "-Cstrip=debuginfo" "-C" "prefer-dynamic" "--out-dir" "/Users/runner/work/rust/rust/build/aarch64-apple-darwin/test/ui/scalable-vectors/project-into-field-extern/auxiliary" "-A" "internal_features" "-A" "incomplete_features" "-A" "unused_parens" "-A" "unused_braces" "-Crpath" "-Cdebuginfo=0" "-Lnative=/Users/runner/work/rust/rust/build/aarch64-apple-darwin/native/rust-test-helpers" "-Cpanic=abort" "-Cforce-unwind-tables=yes" "-Copt-level=0" "--crate-type" "dylib" "-L" "/Users/runner/work/rust/rust/build/aarch64-apple-darwin/test/ui/scalable-vectors/project-into-field-extern/auxiliary" "--extern" "minicore=/Users/runner/work/rust/rust/build/aarch64-apple-darwin/test/ui/scalable-vectors/project-into-field-extern/libminicore.rlib"
stdout: none
--- stderr -------------------------------
warning: field `0` is never read
##[warning] --> /Users/runner/work/rust/rust/tests/ui/scalable-vectors/auxiliary/simple.rs:13:15
|
LL | pub struct Sv(f32);
| -- ^^^
| |
| field in this struct
|
= help: consider removing this field
= note: `#[warn(dead_code)]` (part of `#[warn(unused)]`) on by default
error: linking with `cc` failed: exit status: 1
|
= note: "cc" "-Wl,-exported_symbols_list" "-Wl,/Users/runner/work/rust/rust/build/aarch64-apple-darwin/test/ui/scalable-vectors/project-into-field-extern/auxiliary/rustcnDOBCi/list" "/Users/runner/work/rust/rust/build/aarch64-apple-darwin/test/ui/scalable-vectors/project-into-field-extern/auxiliary/rustcnDOBCi/symbols.o" "<1 object files omitted>" "/Users/runner/work/rust/rust/build/aarch64-apple-darwin/test/ui/scalable-vectors/project-into-field-extern/auxiliary/rustcnDOBCi/rmeta.o" "/Users/runner/work/rust/rust/build/aarch64-apple-darwin/test/ui/scalable-vectors/project-into-field-extern/libminicore.rlib" "-arch" "arm64" "-mmacosx-version-min=11.0.0" "-L" "/Users/runner/work/rust/rust/build/aarch64-apple-darwin/native/rust-test-helpers" "-L" "/Users/runner/work/rust/rust/build/aarch64-apple-darwin/test/ui/scalable-vectors/project-into-field-extern/auxiliary" "-o" "/Users/runner/work/rust/rust/build/aarch64-apple-darwin/test/ui/scalable-vectors/project-into-field-extern/auxiliary/libsimple.dylib" "-Wl,-dead_strip" "-dynamiclib" "-Wl,-install_name" "-Wl,@rpath/libsimple.dylib" "-nodefaultlibs"
= note: some arguments are omitted. use `--verbose` to show all linker arguments
= note: Undefined symbols for architecture arm64:
"dyld_stub_binder", referenced from:
<initial-undefines>
ld: symbol(s) not found for architecture arm64
clang: error: linker command failed with exit code 1 (use -v to see invocation)
error: aborting due to 1 previous error; 1 warning emitted
------------------------------------------
|
This pull request was unapproved. This PR was contained in a rollup (#160735), which was unapproved. |
View all comments
Fixes #160580. Preventing projections into scalable vectors is an oversight from the initial implementation and something we should fix.
I'm surprised it caused a stdarch CI failure as reported by #160580, as nothing in rustc or stdarch seems to have changed that would have caused that to start happening as far as I can tell. This likely won't fix that stdarch CI failure if it keeps happening, because if there is a projection coming from somewhere then that needs to be fixed - nevertheless, preventing them as in this patch is the right thing to do.
I've tested this against the stdarch CI locally.