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

Special case DUMMY_SP to emit line 0/column 0 locations on DWARF platforms. #128627

Merged
merged 5 commits into from
Aug 21, 2024

Conversation

khuey
Copy link
Contributor

@khuey khuey commented Aug 4, 2024

Line 0 has a special meaning in DWARF. From the version 5 spec:

The compiler may emit the value 0 in cases
where an instruction cannot be attributed to any
source line.

DUMMY_SP spans cannot be attributed to any line. However, because rustc internally stores line numbers starting at zero, lookup_debug_loc() adjusts every line number by one. Special casing DUMMY_SP to actually emit line 0 ensures rustc communicates to the debugger that there's no meaningful source code for this instruction, rather than telling the debugger to jump to line 1 randomly.

…forms.

Line 0 has a special meaning in DWARF. From the version 5 spec:

    The compiler may emit the value 0 in cases
    where an instruction cannot be attributed to any
    source line.

DUMMY_SP spans cannot be attributed to any line. However, because rustc
internally stores line numbers starting at zero, lookup_debug_loc() adjusts
every line number by one. Special casing DUMMY_SP to actually emit line 0
ensures rustc communicates to the debugger that there's no meaningful source
code for this instruction, rather than telling the debugger to jump to line 1
randomly.
@rustbot
Copy link
Collaborator

rustbot commented Aug 4, 2024

r? @nnethercote

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

@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 4, 2024
khuey added a commit to khuey/rust that referenced this pull request Aug 4, 2024
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.
Copy link
Member

@jieyouxu jieyouxu left a comment

Choose a reason for hiding this comment

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

Is it possible to add a test for this?

@klensy
Copy link
Contributor

klensy commented Aug 4, 2024

And this probably need comment in code too?

@rust-log-analyzer

This comment has been minimized.

@nnethercote
Copy link
Contributor

I agree a comment would be useful, particularly for explaining how/why MSVC is treated differently.

tgross35 added a commit to tgross35/rust that referenced this pull request Aug 19, 2024
…, 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`
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Aug 19, 2024
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`
@khuey
Copy link
Contributor Author

khuey commented Aug 20, 2024

I added a comment and a test. The test is potentially fragile, if the code sample I used to emit a dummy span ever stops getting dummy spans the test will no longer test anything meaningful. But here's an example of the debug experience.

Before:

Breakpoint 1, tmp::main () at src/main.rs:5
5	    let args = env::args();
(gdb) n
6	    let number_str = args.skip(1).next().unwrap();
(gdb)
6	    let number_str = args.skip(1).next().unwrap();
(gdb)
7	    let number = number_str.parse::<i32>()?;
(gdb)
8	    if number % 7 == 0 {
(gdb)
9	        return Ok(());
(gdb)
1	use std::env;
(gdb)
13	}
(gdb)

After

Breakpoint 1, tmp::main () at src/main.rs:5
5	    let args = env::args();
(gdb) n
6	    let number_str = args.skip(1).next().unwrap();
(gdb) 
6	    let number_str = args.skip(1).next().unwrap();
(gdb) 
7	    let number = number_str.parse::<i32>()?;
(gdb) 
8	    if number % 7 == 0 {
(gdb) 
9	        return Ok(());
(gdb) 
13	}
(gdb) 

The inexplicable (to the debugger user) jump to line 1 goes away.

// attributed to any line in the source. That's also exactly what dummy
// spans are. Make that equivalence here, rather than passing dummy spans
// to lookup_debug_loc, which will return line 1 for them.
let (line, col) = if span.is_dummy() && !self.sess().target.is_like_msvc {
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be nice to have an is_dwarf() method rather than relying on the current-but-not-guaranteed-to-forever-be-true equivalence with is_like_msvc. But that's beyond the scope of this PR.

@nnethercote
Copy link
Contributor

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Aug 20, 2024

📌 Commit 4e9725c has been approved by nnethercote

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 Aug 20, 2024
jieyouxu added a commit to jieyouxu/rust that referenced this pull request Aug 20, 2024
Special case DUMMY_SP to emit line 0/column 0 locations on DWARF platforms.

Line 0 has a special meaning in DWARF. From the version 5 spec:

    The compiler may emit the value 0 in cases
    where an instruction cannot be attributed to any
    source line.

DUMMY_SP spans cannot be attributed to any line. However, because rustc internally stores line numbers starting at zero, lookup_debug_loc() adjusts every line number by one. Special casing DUMMY_SP to actually emit line 0 ensures rustc communicates to the debugger that there's no meaningful source code for this instruction, rather than telling the debugger to jump to line 1 randomly.
bors added a commit to rust-lang-ci/rust that referenced this pull request Aug 20, 2024
Rollup of 6 pull requests

Successful merges:

 - rust-lang#127623 (fix: fs::remove_dir_all: treat internal ENOENT as success)
 - rust-lang#128627 (Special case DUMMY_SP to emit line 0/column 0 locations on DWARF platforms.)
 - rust-lang#129187 (bootstrap: fix clean's remove_dir_all implementation)
 - rust-lang#129190 (Added f16 and f128 to tests/ui/consts/const-float-bits-conv.rs)
 - rust-lang#129231 (improve submodule updates)
 - rust-lang#129257 (Allow rust staticlib to work with MSVC's /WHOLEARCHIVE)

r? `@ghost`
`@rustbot` modify labels: rollup
jieyouxu added a commit to jieyouxu/rust that referenced this pull request Aug 20, 2024
Special case DUMMY_SP to emit line 0/column 0 locations on DWARF platforms.

Line 0 has a special meaning in DWARF. From the version 5 spec:

    The compiler may emit the value 0 in cases
    where an instruction cannot be attributed to any
    source line.

DUMMY_SP spans cannot be attributed to any line. However, because rustc internally stores line numbers starting at zero, lookup_debug_loc() adjusts every line number by one. Special casing DUMMY_SP to actually emit line 0 ensures rustc communicates to the debugger that there's no meaningful source code for this instruction, rather than telling the debugger to jump to line 1 randomly.
bors added a commit to rust-lang-ci/rust that referenced this pull request Aug 20, 2024
Rollup of 6 pull requests

Successful merges:

 - rust-lang#127623 (fix: fs::remove_dir_all: treat internal ENOENT as success)
 - rust-lang#128627 (Special case DUMMY_SP to emit line 0/column 0 locations on DWARF platforms.)
 - rust-lang#129187 (bootstrap: fix clean's remove_dir_all implementation)
 - rust-lang#129190 (Added f16 and f128 to tests/ui/consts/const-float-bits-conv.rs)
 - rust-lang#129231 (improve submodule updates)
 - rust-lang#129284 (rustdoc: animate the `:target` highlight)

r? `@ghost`
`@rustbot` modify labels: rollup
bors added a commit to rust-lang-ci/rust that referenced this pull request Aug 20, 2024
Rollup of 6 pull requests

Successful merges:

 - rust-lang#127623 (fix: fs::remove_dir_all: treat internal ENOENT as success)
 - rust-lang#128627 (Special case DUMMY_SP to emit line 0/column 0 locations on DWARF platforms.)
 - rust-lang#129187 (bootstrap: fix clean's remove_dir_all implementation)
 - rust-lang#129190 (Added f16 and f128 to tests/ui/consts/const-float-bits-conv.rs)
 - rust-lang#129231 (improve submodule updates)
 - rust-lang#129284 (rustdoc: animate the `:target` highlight)

r? `@ghost`
`@rustbot` modify labels: rollup
tgross35 added a commit to tgross35/rust that referenced this pull request Aug 21, 2024
Special case DUMMY_SP to emit line 0/column 0 locations on DWARF platforms.

Line 0 has a special meaning in DWARF. From the version 5 spec:

    The compiler may emit the value 0 in cases
    where an instruction cannot be attributed to any
    source line.

DUMMY_SP spans cannot be attributed to any line. However, because rustc internally stores line numbers starting at zero, lookup_debug_loc() adjusts every line number by one. Special casing DUMMY_SP to actually emit line 0 ensures rustc communicates to the debugger that there's no meaningful source code for this instruction, rather than telling the debugger to jump to line 1 randomly.
tgross35 added a commit to tgross35/rust that referenced this pull request Aug 21, 2024
Special case DUMMY_SP to emit line 0/column 0 locations on DWARF platforms.

Line 0 has a special meaning in DWARF. From the version 5 spec:

    The compiler may emit the value 0 in cases
    where an instruction cannot be attributed to any
    source line.

DUMMY_SP spans cannot be attributed to any line. However, because rustc internally stores line numbers starting at zero, lookup_debug_loc() adjusts every line number by one. Special casing DUMMY_SP to actually emit line 0 ensures rustc communicates to the debugger that there's no meaningful source code for this instruction, rather than telling the debugger to jump to line 1 randomly.
tgross35 added a commit to tgross35/rust that referenced this pull request Aug 21, 2024
Special case DUMMY_SP to emit line 0/column 0 locations on DWARF platforms.

Line 0 has a special meaning in DWARF. From the version 5 spec:

    The compiler may emit the value 0 in cases
    where an instruction cannot be attributed to any
    source line.

DUMMY_SP spans cannot be attributed to any line. However, because rustc internally stores line numbers starting at zero, lookup_debug_loc() adjusts every line number by one. Special casing DUMMY_SP to actually emit line 0 ensures rustc communicates to the debugger that there's no meaningful source code for this instruction, rather than telling the debugger to jump to line 1 randomly.
bors added a commit to rust-lang-ci/rust that referenced this pull request Aug 21, 2024
Rollup of 7 pull requests

Successful merges:

 - rust-lang#128432 (WASI: forbid `unsafe_op_in_unsafe_fn` for `std::{os, sys}`)
 - rust-lang#128627 (Special case DUMMY_SP to emit line 0/column 0 locations on DWARF platforms.)
 - rust-lang#129187 (bootstrap: fix clean's remove_dir_all implementation)
 - rust-lang#129257 (Allow rust staticlib to work with MSVC's /WHOLEARCHIVE)
 - rust-lang#129264 (Update `library/Cargo.toml` in weekly job)
 - rust-lang#129284 (rustdoc: animate the `:target` highlight)
 - rust-lang#129332 (Avoid extra `cast()`s after `CStr::as_ptr()`)

r? `@ghost`
`@rustbot` modify labels: rollup
bors added a commit to rust-lang-ci/rust that referenced this pull request Aug 21, 2024
…iaskrgr

Rollup of 10 pull requests

Successful merges:

 - rust-lang#128627 (Special case DUMMY_SP to emit line 0/column 0 locations on DWARF platforms.)
 - rust-lang#128843 (Minor Refactor: Remove a Redundant Conditional Check)
 - rust-lang#129179 (CFI: Erase regions when projecting ADT to its transparent non-1zst field)
 - rust-lang#129281 (Tweak unreachable lint wording)
 - rust-lang#129312 (Fix stability attribute of `impl !Error for &str`)
 - rust-lang#129332 (Avoid extra `cast()`s after `CStr::as_ptr()`)
 - rust-lang#129339 (Make `ArgAbi::make_indirect_force` more specific)
 - rust-lang#129344 (Use `bool` in favor of `Option<()>` for diagnostics)
 - rust-lang#129345 (Use shorthand field initialization syntax more aggressively in the compiler)
 - rust-lang#129355 (fix comment on PlaceMention semantics)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit dea325e into rust-lang:master Aug 21, 2024
6 checks passed
@rustbot rustbot added this to the 1.82.0 milestone Aug 21, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Aug 21, 2024
Rollup merge of rust-lang#128627 - khuey:DUMMY_SP-line-no, r=nnethercote

Special case DUMMY_SP to emit line 0/column 0 locations on DWARF platforms.

Line 0 has a special meaning in DWARF. From the version 5 spec:

    The compiler may emit the value 0 in cases
    where an instruction cannot be attributed to any
    source line.

DUMMY_SP spans cannot be attributed to any line. However, because rustc internally stores line numbers starting at zero, lookup_debug_loc() adjusts every line number by one. Special casing DUMMY_SP to actually emit line 0 ensures rustc communicates to the debugger that there's no meaningful source code for this instruction, rather than telling the debugger to jump to line 1 randomly.
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
Development

Successfully merging this pull request may close these issues.

8 participants