When deduplicating unreachable blocks, erase the source information. #128628
Conversation
After deduplication the block conceptually belongs to multiple locations in the source. Although these blocks are unreachable, in rust-lang#123341 we did come across a real side effect, an unreachable block that survives into the compiled code can cause a debugger to set a breakpoint on the wrong instruction. Erasing the source information ensures that a debugger will never be misled into thinking that the unreachable block is worth setting a breakpoint on, especially after rust-lang#128627. Technically we don't need to erase the source information if all the deduplicated blocks have identical source information, but tracking that seems like more effort than it's worth.
rustbot
added
S-waiting-on-review
|
Some changes occurred to MIR optimizations cc @rust-lang/wg-mir-opt |
|
Seems fine. Is it possible to write a test? |
|
I will cc @jieyouxu too, in case they have opinions. |
Yeah, this looks like having a regression test would be very useful. This probably wants to be a run-make test? I can help with the run-make test infra, but what exactly the test logic is I'm not too sure about, since does this want to check differences in line table info? |
|
I don't know if it's the right fix. We need tests in codegen and/or mir-opt. Maybe we also need a debugger test? (This should not require run-make.) |
|
cc @michaelwoerister : since I think you know quite a bit about debuginfo tests, could this be checked with some kind of debuginfo test? |
|
The debuginfo tests can invoke arbitrary GDB/LLDB/CDB commands and check their output. I don't know if it's possible to write a non-flaky test for this. Like other tests, we can specify the compiler flags the test is compiled with. That might help in making a test more reliable. |
|
I don't think there's a reliable way to test this because really LLVM should remove every unreachable block. I could write a test that's very specific to the situation in #123341 but I think it's likely to break on some future LLVM upgrade. |
bors
added
S-waiting-on-bors
Rollup of 7 pull requests Successful merges: - rust-lang#127679 (Stabilize `raw_ref_op` (RFC 2582)) - rust-lang#128084 (Suggest adding Result return type for associated method in E0277.) - rust-lang#128628 (When deduplicating unreachable blocks, erase the source information.) - rust-lang#128902 (doc: std::env::var: Returns None for names with '=' or NUL byte) - rust-lang#129048 (Update `crosstool-ng` for loongarch64) - rust-lang#129116 (Include a copy of `compiler-rt` source in the `download-ci-llvm` tarball) - rust-lang#129208 (Fix order of normalization and recursion in const folding.) r? `@ghost` `@rustbot` modify labels: rollup
Rollup merge of rust-lang#128628 - khuey:simply-cfg-erase-source-info, r=nnethercote When deduplicating unreachable blocks, erase the source information. After deduplication the block conceptually belongs to multiple locations in the source. Although these blocks are unreachable, in rust-lang#123341 we did come across a real side effect, an unreachable block that survives into the compiled code can cause a debugger to set a breakpoint on the wrong instruction. Erasing the source information ensures that a debugger will never be misled into thinking that the unreachable block is worth setting a breakpoint on, especially after rust-lang#128627. Technically we don't need to erase the source information if all the deduplicated blocks have identical source information, but tracking that seems like more effort than it's worth. I'll let njn redirect this one too. r? `@nnethercote`
After deduplication the block conceptually belongs to multiple locations in the source. Although these blocks are unreachable, in #123341 we did come across a real side effect, an unreachable block that survives into the compiled code can cause a debugger to set a breakpoint on the wrong instruction. Erasing the source information ensures that a debugger will never be misled into thinking that the unreachable block is worth setting a breakpoint on, especially after #128627.
Technically we don't need to erase the source information if all the deduplicated blocks have identical source information, but tracking that seems like more effort than it's worth.
I'll let njn redirect this one too. r? @nnethercote