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

Fix debug information for function arguments of type &str or slice. #81898

Merged

Conversation

nanguye2496
Copy link

Issue details:
When lowering MIR to LLVM IR, the compiler decomposes every &str and slice argument into a data pointer and a usize. Then, the original argument is reconstructed from the pointer and the usize arguments in the body of the function that owns it. Since the original argument is declared in the body of a function, it should be marked as a LocalVariable instead of an ArgumentVairable. This confusion causes MSVC debuggers unable to visualize &str and slice arguments correctly. (See #81894 for more details).

Fix details:
Making sure that the debug variable for every &str and slice argument is marked as LocalVariable instead of ArgumentVariable in computing_per_local_var_debug_info. This change has been verified on VS Code debugger, VS debugger, WinDbg and LLDB.

@rust-highfive
Copy link
Collaborator

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @varkor (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Feb 8, 2021
@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@bjorn3
Copy link
Member

bjorn3 commented Feb 9, 2021

When using DWARF debuginfo, using DW_TAG_formal_parameter instead of DW_TAG_variable makes much more sense. Otherwise debuggers won't show the arguments as arguments. For example for

fn foo(a: &str) {}
fn main() {
    foo("a");
}

Lldb will currently show:

$ lldb ./rust_out
(lldb) target create "./rust_out"
Current executable set to './rust_out' (x86_64).
(lldb) rbreak rust_out
Breakpoint 1: 2 locations.
(lldb) run
Process 4075 launched: './rust_out' (x86_64)
Process 4075 stopped
* thread #1, name = 'rust_out', stop reason = breakpoint 1.1
    frame #0: 0x000055555555932b rust_out`rust_out::main::h405f65324f7b53be at <anon>:1
(lldb) cont
Process 4075 resuming
Process 4075 stopped
* thread #1, name = 'rust_out', stop reason = breakpoint 1.2
    frame #0: 0x000055555555931a rust_out`rust_out::foo::ha632284b5a2f1ab8(a=(data_ptr = "a", length = 1)) at <anon>:1
(lldb)

This PR would remove the a=(data_ptr = "a", length = 1) part.

@nanguye2496
Copy link
Author

nanguye2496 commented Feb 9, 2021

@bjorn3: Thank you for your feedback. My fix aims to target CodeView symbols only. I wonder what would be the ideal way to fix this issue for both CodeView and DWARF symbols?

@bjorn3
Copy link
Member

bjorn3 commented Feb 9, 2021

Maybe you could add a check that the target is windows. (msvc only. I think mingw uses dwarf, but I am not sure)

@nanguye2496
Copy link
Author

nanguye2496 commented Feb 9, 2021

Maybe you could add a check that the target is windows. (msvc only. I think mingw uses dwarf, but I am not sure)

@bjorn3: I believe the is_like_msvc check should be sufficient? mingw would meet the is_like_windows check but not is_like_msvc?

@bjorn3
Copy link
Member

bjorn3 commented Feb 9, 2021

I think so.

@rust-log-analyzer

This comment has been minimized.

@nanguye2496 nanguye2496 force-pushed the nanguye2496/fix_str_and_slice_visualization branch from 731b9e6 to 615fd14 Compare February 10, 2021 00:00
@nanguye2496
Copy link
Author

I think so.

Alright, I just updated my code accordingly.

@varkor
Copy link
Member

varkor commented Feb 12, 2021

@bjorn3 seems to already have taken a look, so it seems convenient to reassign. (Feel free to assign me again if not.)

r? @bjorn3

@rust-highfive rust-highfive assigned bjorn3 and unassigned varkor Feb 12, 2021
@bjorn3
Copy link
Member

bjorn3 commented Feb 12, 2021

I have absolutely zero knowledge about the format of pdb debuginfo, so I have no idea if this is correct for msvc targets.

r? @varkor

@rust-highfive rust-highfive assigned varkor and unassigned bjorn3 Feb 12, 2021
@nanguye2496
Copy link
Author

@varkor : Hi, I wonder if there is any further work that should be done for this PR?

@sivadeilra
Copy link

sivadeilra commented Feb 16, 2021

I have absolutely zero knowledge about the format of pdb debuginfo, so I have no idea if this is correct for msvc targets.

Hi, @varkor . I'm @nanguye2496 's manager. We are members of the Windows Engineering System, at Microsoft. We have worked with our peers in the Windows Debugger team, and confirmed that this change is correct. Before this commit, the PDB / CodeView records are malformed. This commit fixes that, and allows the debug-spill variable that is created for &str and &[T] arguments to be interpreted correctly by WinDbg and Visual Studio.

We believe there is more work to do in this area, but that this change is a good first step toward improving the experience of debugging Rust code on Windows.

@varkor
Copy link
Member

varkor commented Feb 17, 2021

Sorry for the delay; I've been busy with non-Rust priorities for the last few days. The change looks fine to me, thanks!

@bors r+

@bors
Copy link
Contributor

bors commented Feb 17, 2021

📌 Commit 615fd14 has been approved by varkor

@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 Feb 17, 2021
@sivadeilra
Copy link

No worries. Thanks!

bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 18, 2021
Rollup of 8 pull requests

Successful merges:

 - rust-lang#77728 (Expose force_quotes on Windows.)
 - rust-lang#80572 (Add a `Result::into_ok_or_err` method to extract a `T` from `Result<T, T>`)
 - rust-lang#81860 (Fix SourceMap::start_point)
 - rust-lang#81869 (Simplify pattern grammar, improve or-pattern diagnostics)
 - rust-lang#81898 (Fix debug information for function arguments of type &str or slice.)
 - rust-lang#81972 (Placeholder lifetime error cleanup)
 - rust-lang#82007 (Implement reborrow for closure captures)
 - rust-lang#82021 (Spell out nested Self type in lint message)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit f79be2c into rust-lang:master Feb 18, 2021
@rustbot rustbot added this to the 1.52.0 milestone Feb 18, 2021
JohnTitor added a commit to JohnTitor/rust that referenced this pull request Jun 3, 2021
…ests, r=Mark-Simulacrum

Add debug info tests for range, fix-sized array, and cell types

This PR add several debug info tests to guarantee that the displays of fixed sized arrays, range types, cell types, threads, locks, and mutexes in CDB are correct.

It also updates CDB tests for slices in pretty-std.rs after string visualization in WinDbg is fixed by this PR: rust-lang#81898.
JohnTitor added a commit to JohnTitor/rust that referenced this pull request Jun 8, 2021
…ests, r=Mark-Simulacrum

Add debug info tests for range, fix-sized array, and cell types

This PR add several debug info tests to guarantee that the displays of fixed sized arrays, range types, cell types, threads, locks, and mutexes in CDB are correct.

It also updates CDB tests for slices in pretty-std.rs after string visualization in WinDbg is fixed by this PR: rust-lang#81898.
bors added a commit to rust-lang-ci/rust that referenced this pull request Jun 27, 2021
…ts, r=Mark-Simulacrum

Add debug info tests for range, fix-sized array, and cell types

This PR add several debug info tests to guarantee that the displays of fixed sized arrays, range types, cell types, threads, locks, and mutexes in CDB are correct.

It also updates CDB tests for slices in pretty-std.rs after string visualization in WinDbg is fixed by this PR: rust-lang#81898.
wesleywiser added a commit to wesleywiser/rust that referenced this pull request Sep 7, 2021
Mark all of these as locals so the debugger does not try to interpret
them as being a pointer to the value. This extends the approach used in
PR rust-lang#81898.
Manishearth added a commit to Manishearth/rust that referenced this pull request Sep 12, 2021
…rpair_params, r=oli-obk

Fix debuginfo for parameters passed via the ScalarPair abi on Windows

Mark all of these as locals so the debugger does not try to interpret
them as being a pointer to the value. This extends the approach used
in rust-lang#81898.

Fixes rust-lang#88625
wesleywiser added a commit to wesleywiser/rust that referenced this pull request Sep 13, 2021
Mark all of these as locals so the debugger does not try to interpret
them as being a pointer to the value. This extends the approach used in
PR rust-lang#81898.
bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 22, 2021
…air_params, r=oli-obk

Fix debuginfo for parameters passed via the ScalarPair abi on Windows

Mark all of these as locals so the debugger does not try to interpret
them as being a pointer to the value. This extends the approach used
in rust-lang#81898.

Fixes rust-lang#88625
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants