search

rust-lang / rust

Empowering everyone to build reliable and efficient software.
https://www.rust-lang.org
Other
99.13k stars 12.8k forks source link

Add `known-bug` tests for all(?) I-unsound issues #105107

Closed jackh726 closed 2 months ago

jackh726 commented 2 years ago

I'll come back at some point and create an actual list. However, we should aim to add known-bug tests (or work to finding MCVEs) for all the I-unsound issues.

The list of issues remaining: https://github.com/rust-lang/rust/issues?q=is%3Aopen+is%3Aissue+label%3AI-unsound+-label%3AS-bug-has-test+

The list of completed issues: https://github.com/rust-lang/rust/issues?q=is%3Aopen+is%3Aissue+label%3AI-unsound+label%3AS-bug-has-test+

sladyn98 commented 1 year ago

@jackh726 I would like to give this a shot, any hints on where I could begin

jackh726 commented 1 year ago

@sladyn98 sure!

Okay, so we have a concept of known-bug tests. Here's an example: https://github.com/rust-lang/rust/blob/18a6d911caba59605eb03db1452848a85d2e5879/tests/ui/coherence/coherence-overlap-negative-impls.rs

Note the // known-bug: #101350 at the top. There is also // check-pass, which means that this test currently passes under during check (though that test shouldn't pass, hence the known-bug)

The goal for this issue is to go through the open I-unsound issues (https://github.com/rust-lang/rust/issues?page=1&q=is%3Aopen+is%3Aissue+label%3AI-unsound), identify a minimal reproduction that shouldn't pass, but does, and add a known-bug test for each one.

Nearly all of the issues should have a minimal repro in the comments somewhere, so I would start with those. Tests should go in src/test/ui in an appropriate directory. Test names should likely be meaningful compared to the open issue. Any comments at the top of the file briefly explaining the problem would also be helpful.

Let me know if that helps!

sladyn98 commented 1 year ago

https://github.com/rust-lang/rust/issues/105084 I found this issue, and planning to a PR with a known test bug

KenCox94 commented 1 year ago

is there anything a new contributor can do for this? any help still needed @sladyn98 @jackh726

jackh726 commented 1 year ago

My previous comment still applies; there haven't been any PRs opened for this.

If anything is confusing, please let me know.

gburgessiv commented 1 year ago

If it helps, the listing I was able to scrape is:

rging for the list of I-Unsound issues makes it seem that there're no tests for any of these ATM, so I left them unpopulated. I added a few tests while I was in the area, which I'll build a PR for now. :)

KisaragiEffective commented 1 year ago

NOTE: #105084 is already added

whtahy commented 1 year ago

Big thanks to @gburgessiv for the big checklist! Very useful.

@jackh726 Thanks for adding the test labels and updating the issue. Looking through the remaining issues, I'm a bit lost for most of these.

I'm comfortable adding tests where the mcve compiles but shouldn't, but I'm wondering what to do for mcves that don't quite follow this pattern or have other variations: multiple crates, llvm/asm/etc, panic/warning/error, etc.

Below, I've organized the remaining issues as I see them, hopefully useful for other contributors as well. (Currently only 44 issues in my list vs 55 actual, so missing 11 issues as of 2023.04.28)

Let me know if there's anything else sort of bookkeeping that would be useful here.

1. Not able to find an mcve for these 7 issues:

issue | name :--- | :--- 27970 | set_var/remove_var are unsound in combination with C code that reads the environment 43241 | Extend stack probe support to non-tier-1 platforms, and clarify policy for mitigating LLVM-dependent unsafety 52652 | Abort instead of unwinding past FFI functions 64718 | std::process::Command doesn't follow Unix signal safety in forked child 67497 | Switching to opt-level=z on i686-windows-msvc triggers STATUS_ACCESS_VIOLATION 68015 | Pin is unsound due to transitive effects of CoerceUnsized 74551 | error: could not compile gkrust since Rust 1.43 on SPARC Solaris

2. Tracking issue needs test?

Wondering if the specialization tracking issue (issue 31844) needs a test, since it's sort of multiple issues in 1 issue.

3. These 4 issues already include tests on master:

issue | name | test | test status :-- | :-- | :-- | :-- 50781 | Trait objects can call nonexistent concrete methods | `tests/ui/issues/issue-50781.rs` | test errors and includes .stderr 97156 | TypeId exposes equality-by-subtyping vs normal-form-syntactic-equality unsoundness. | `tests/ui/const-generics/generic_const_exprs/typeid-equality-by-subtyping.rs` | test includes `known-bug` and `check-pass` comments 102048 | relating projection substs is unsound during coherence | `tests/ui/traits/new-solver/coherence/issue-102048.rs` | test compiles but includes .stderr 105084 | Box syntax and generator_clone can lead to double free | `tests/ui/generator/issue-105084.rs` | test includes `known-bug` and `check-pass` comments

4. These 2 issues have an mcve, but don't work as is on playground:

issue | name | mcve | error | needs? :-- | :-- | :-- | :-- | :-- 74743 | AVR: Miscompilation with Trait Object in Option | https://github.com/rust-lang/rust/issues/74743#issue-665586119 | error: unknown feature `llvm_asm` | `llvm_asm!` macro 76507 | #[link_section] is unsound on Harvard architectures | https://github.com/rust-lang/rust/issues/76507#issue-696552104 | error: expected expression, found `;` | serial output

5. These 3 issues may need regression tests instead?

issue | name | mcve | status :-- | :-- | :-- | :-- 79457 | With min_specialization enabled, an incomplete impl for a non-static type will delegate method calls to a less-specific impl with a 'static bound | https://github.com/rust-lang/rust/issues/79457#issue-751911849 | fixed w/ error? https://github.com/rust-lang/rust/issues/79457#issuecomment-1492052359 79865 | Miscompilation of AVX2 code under --release | https://github.com/rust-lang/rust/issues/79865#issuecomment-748537875 | fixed by llvm 14 upgrade: https://github.com/rust-lang/rust/issues/79865#issuecomment-1064272929 80127 | rustc has wrong signature for C function with 16-byte aligned stack argument in x86_64 Linux | https://github.com/rust-lang/rust/issues/80127#issue-770277496 | wip PR 103830 includes regression test: https://github.com/rust-lang/rust/pull/103830#issuecomment-1301157562

6. Not sure how to test: These 11 issues involve llvm, asm, C, etc

issue | name | mcve | needs? :-- | :-- | :-- | :-- 18072 | Investigate how LLVM deals with huge stack frames | https://github.com/rust-lang/rust/issues/18072#issuecomment-345268545 | llvm 46188 | ill-typed unused FFI declarations can cause UB | https://github.com/rust-lang/rust/issues/46188#issuecomment-1288098840 | llvm 53346 | repr(simd) is unsound in C FFI | https://github.com/rust-lang/rust/issues/53346#issue-350418164 | UB caused by C vs Rust ABI mismatch 63818 | Resolve unsound interaction between noalias and self-referential data (incl. generators, async fn) | https://github.com/rust-lang/rust/issues/63818#issuecomment-751973452 | miri 64609 | -C target-feature/-C target-cpu are unsound | https://github.com/rust-lang/rust/issues/64609#issue-495840716 | multiple crates, rustflags, ABI 70022 | Statics don't support alignments larger than the page size | https://github.com/rust-lang/rust/issues/70022#issue-581682673 | platform specific, llvm 70143 | Locals aligned to greater than page size can cause unsound behavior | https://github.com/rust-lang/rust/issues/70143#issuecomment-657209328 | platform specific, llvm 72327 | Comparing to infinity is buggy on x87 | https://github.com/rust-lang/rust/issues/72327#issue-620328373 | platform specific, asm 81996 | repr(C) is unsound on MSVC targets | https://github.com/rust-lang/rust/issues/81996#issue-806529263 | https://github.com/rust-lang/rust/issues/81996#issuecomment-1040477935 101346 | Incorrect handling of lateout pairs in inline asm | https://github.com/rust-lang/rust/issues/101346#issue-1360647823 | asm 102174 | extern "C" functions don't generate the same IR definitions as clang on x86, causing problems with cross-language LTO | https://github.com/rust-lang/rust/issues/102174#issue-1383407584 | C, asm

7. Not sure how to test: These 10 issues error, segfault, or panic:

issue | name | mcve | test outcome :-- | :-- | :-- | :-- 10389 | Collisions in type_id | https://github.com/rust-lang/rust/issues/10389#issuecomment-274780814 | panic: `Option::unwrap` on `None` 47949 | Panics in destructors can cause the return value to be leaked | https://github.com/rust-lang/rust/issues/47949#issue-293710135 | explicit panic 71416 | unsized_locals fails to uphold alignment requirements | https://github.com/rust-lang/rust/issues/71416#issue-604484607 | fails `left == right` test 80176 | Unsoundness in type checking of trait impls. Differences in implied lifetime bounds are not considered. | https://github.com/rust-lang/rust/issues/80176#issuecomment-748466836 | error: `#[deny(implied_bounds_entailment)]` on by default 83060 | Regressions with large (2-4GB) stack arrays on large stacks | https://github.com/rust-lang/rust/issues/83060#issue-830428341 | panics in playground: `Result::unwrap() on Err` or stack overflow 84857 | auto trait candidate selection is unsound | https://github.com/rust-lang/rust/issues/84857#issue-874470444 | segfault and warning: cross-crate traits with a default impl, like `UnwindSafe`, should not be specialized 89948 | SpecExtend for TrustedLen is unsound | https://github.com/rust-lang/rust/issues/89948#issue-1028027263 | segfault, but maybe not bug? https://github.com/rust-lang/rust/issues/89948#issuecomment-1007361167 100914 | Double free on Linux with stable toolchain | https://github.com/rust-lang/rust/issues/100914#issuecomment-1224435113 | panics in playground: `Result::unwrap() on Err` or stack overflow. (related to 83060) 102678 | prevent negative impl cycles | https://github.com/rust-lang/rust/issues/102678#issue-1396897505 | should compile but errors 105295 | Seg fault in Rust 1.65.0 if I don't create temporary variable | https://github.com/rust-lang/rust/issues/105295#issuecomment-1343710419 | error: `#[deny(implied_bounds_entailment)]` on by default

8. Not sure how to test: These 2 mcves split code across multiple crates

issue | name | mcve :-- | :-- | :-- 28179 | #[no_mangle] is unsafe | https://github.com/rust-lang/rust/issues/28179#issue-104592603 99554 | orphan check incorrectly handles projections | https://github.com/rust-lang/rust/issues/99554#issue-1313070635

9. This mcve compiles, but not sure if it's supposed to error?

issue | name | mcve :-- | :-- | :-- 80925 | Accessing DST field of packed struct calculates the wrong field address | https://github.com/rust-lang/rust/issues/80925#issue-783672486

10. ~I think I'm good to PR these 4 issues:~ Merged PR 110878

issue | name | mcve -- | -- | -- ~49682~ | Keeping references to #[thread_local] statics is allowed across yields. | https://github.com/rust-lang/rust/issues/49682#issuecomment-581702674 ~40582~ | Specialization and lifetime dispatch | https://github.com/rust-lang/rust/issues/40582#issue-214833291 ~74629~ | negative_impls and optin_builtin_traits nightly-features allow trait impls to overlap | https://github.com/rust-lang/rust/issues/74629#issue-663734265 ~105782~ | specialization: default items completely drop candidates instead of ambiguity | https://github.com/rust-lang/rust/issues/105782#issue-1500394507

11. These 3 issues are part of @gburgessiv's PR 108445:

issue | name :-- | :-- 105787 | occurs check with projections results in error, not ambiguity 107975 | Miscompilation: Equal pointers comparing as unequal 108425 | TAIT shouldn't constrain impl generics
J-Kappes commented 1 year ago

Okay, so we have a concept of known-bug tests. Here's an example:

That link is dead.

I'm also not sure I understand what to do with the MCVEs. Are they to be used for writing the tests?

jackh726 commented 1 year ago

That link is dead.

Fixed

I'm also not sure I understand what to do with the MCVEs. Are they to be used for writing the tests?

The test is essentially the MCVE, with some comments describing the issue.

YpeKingma commented 8 months ago

For a somewhat different approach, there is a pull request for a clippy lint for three of these issues #25860 #84591 and #100051 :

https://github.com/rust-lang/rust-clippy/pull/12471

The lint looks for nested references and suggests to add a lifetimes bound that is implied by the nested reference. After adding this bound, the compiler currently gives an error message. Since the added bound is implied, the error message should have been given before adding it.

jackh726 commented 2 months ago

I'm going to go ahead and close this. While there are still many issues (75 as of writing this!) that don't have a known-bug test, I don't expect keeping this issue open to meaningful make progress on reducing that number.

The remaining relevant issues are mixed in the reasoning for why they don't have a known-bug test, but it's most often because either 1) there is no MCVE because the unsoundness is either "theoretical" or just hard to test, or 2) the unsoundness is only for some select targets.

Thanks all who've helped make progress on this though!