debuginfo: Captured variables and large pass-by-value arguments #8855

michaelwoerister · 2013-08-29T12:53:38Z

This pull request includes

support for variables captured in closures*,
a fix for issue debuginfo: by-value function parameters are not described correctly. #8512: arguments of non-immediate type (structs, tuples, etc) passed by value can now be accessed correctly in GDB. (I managed to fix this by using llvm::DIBuilder::createComplexVariable(). ~~However, I am not sure if this relies on unstable implementation details of LLVM's debug info handling. I'll try to clarify this on the LLVM mailing list~~).
simplification of the debuginfo module's public interface: the caller of functions like create_local_var_metadata() doesn't have to know and catch all cases when it mustn't call the function,
a cleanup refactoring with unified handling for locals, [self] arguments, captured variables, and match bindings,
and proper span information for self arguments.

* However, see comment at https://github.com/michaelwoerister/rust/blob/1d916ace136a27e354d73d65f488603c65f65bd2/src/test/debug-info/var-captured-in-nested-closure.rs#L62 . This is the same problem as with the fix for issue #8512 above: We are probably using llvm.dbg.declare in an unsupported way that works today but might not work after the next LLVM update.

Cheers,
Michael

Fixes #8512
Fixes #1341

jdm · 2013-08-29T14:35:41Z

This is really excellent work. 🌵 The refactoring is particularly nice!

michaelwoerister · 2013-08-29T17:33:21Z

Thanks!

michaelwoerister · 2013-08-30T07:28:42Z

@cmr This was a legitimate error: The Mac builds need LLVM wrapper functions to be listed in rustllvm.def.in and I forgot to add the new ones there (on Linux the linker also finds the functions without them being listed). This is fixed now with michaelwoerister@dde633e, but is in need of another r+ ...

michaelwoerister · 2013-08-30T17:17:48Z

@jdm This failure doesn't seem to be related to my code. Please retry...

This pull request includes * support for variables captured in closures*, * a fix for issue #8512: arguments of non-immediate type (structs, tuples, etc) passed by value can now be accessed correctly in GDB. (I managed to fix this by using `llvm::DIBuilder::createComplexVariable()`. However, I am not sure if this relies on unstable implementation details of LLVM's debug info handling. I'll try to clarify this on the LLVM mailing list). * simplification of the `debuginfo` module's public interface: the caller of functions like `create_local_var_metadata()` doesn't have to know and catch all cases when it mustn't call the function, * a cleanup refactoring with unified handling for locals, [self] arguments, captured variables, and match bindings, * and proper span information for self arguments. \* However, see comment at https://github.com/michaelwoerister/rust/blob/1d916ace136a27e354d73d65f488603c65f65bd2/src/test/debug-info/var-captured-in-nested-closure.rs#L62 . This is the same problem as with the fix for issue #8512 above: We are probably using `llvm.dbg.declare` in an unsupported way that works today but might not work after the next LLVM update. Cheers, Michael Fixes #8512 Fixes #1341

michaelwoerister · 2013-09-04T09:55:32Z

OK, I've updated this. Captured variables and complex by-value arguments are now handled "the right way". That is, the code now always provides an alloca to llvm.dbg.declare which allows LLVM to correctly deal with various transformations (such as mem2reg). This should now really be stable and work the same on all platforms.

Note, however, that this sometimes introduces new allocas just for use by debug information. The specific cases are:

Every closure has an additional alloca holding a pointer to the its environment data
The 'optimized path' for immutable non-immediate-type by-value arguments is disabled, so the value is always actually copied into the function's activation record.
These two things are only done when extra-debug-info is enabled, so this should not lead to performance regressions for non-debug builds.

The only thing that does not yet use this more stable method is self argument descriptions. They have rather specialized handling in the code and I haven't gotten around to analyse this.

jdm · 2013-09-04T14:10:24Z

Oh, I just noticed that this needs another rebase.

…e descriptions.

… of self arguments)

… value. Also updated documentation comments in debuginfo and renamed DebugContext to CrateDebugContext.

…arguments.

michaelwoerister · 2013-09-04T17:32:53Z

@jdm rebased...

This pull request includes * support for variables captured in closures*, * a fix for issue #8512: arguments of non-immediate type (structs, tuples, etc) passed by value can now be accessed correctly in GDB. (I managed to fix this by using `llvm::DIBuilder::createComplexVariable()`. ~~However, I am not sure if this relies on unstable implementation details of LLVM's debug info handling. I'll try to clarify this on the LLVM mailing list~~). * simplification of the `debuginfo` module's public interface: the caller of functions like `create_local_var_metadata()` doesn't have to know and catch all cases when it mustn't call the function, * a cleanup refactoring with unified handling for locals, [self] arguments, captured variables, and match bindings, * and proper span information for self arguments. \* However, see comment at https://github.com/michaelwoerister/rust/blob/1d916ace136a27e354d73d65f488603c65f65bd2/src/test/debug-info/var-captured-in-nested-closure.rs#L62 . This is the same problem as with the fix for issue #8512 above: We are probably using `llvm.dbg.declare` in an unsupported way that works today but might not work after the next LLVM update. Cheers, Michael Fixes #8512 Fixes #1341

eddyb · 2020-02-03T23:05:08Z

src/librustc/middle/trans/debuginfo.rs

+                        file_metadata,
+                        loc.line as c_uint,
+                        type_metadata,
+                        cx.sess.opts.optimize != session::No,


I know I'm a bit late, but was this intended?

For other LLVM functions, the cx.sess.opts.optimize != session::No argument corresponds to isOptimized: bool, but for LLVMDIBuilderCreateLocalVariable, it's actually AlwaysPreserve: bool.

And AFAICT, ever setting AlwaysPreserve to false is asking for trouble, at least when it comes to real user variables, and we don't really use LLVM debuginfo for anything else.

cc @nikomatsakis @rkruppe (I came across this while trying to get llvm.dbg.value to work)

@michaelwoerister

rustc_codegen_llvm: always set AlwaysPreserve on all debuginfo variables Making this depend on the optimization level appears to have been a copy-paste mistake (other LLVM functions called in this module also take a `bool` argument, but there it means something unrelated). Also see #8855 (comment). I don't believe we have any reason to let LLVM omit user variables from DWARF, and we were already setting this to `true` when LLVM *could* optimize them away, so this PR should have no effect anyway. r? @michaelwoerister or @nagisa cc @rkruppe @nikomatsakis

@michaelwoerister

…agisa rustc_codegen_llvm: always set AlwaysPreserve on all debuginfo variables Making this depend on the optimization level appears to have been a copy-paste mistake (other LLVM functions called in this module also take a `bool` argument, but there it means something unrelated). Also see rust-lang#8855 (comment). I don't believe we have any reason to let LLVM omit user variables from DWARF, and we were already setting this to `true` when LLVM *could* optimize them away, so this PR should have no effect anyway. r? @michaelwoerister or @nagisa cc @rkruppe @nikomatsakis

@michaelwoerister

…agisa rustc_codegen_llvm: always set AlwaysPreserve on all debuginfo variables Making this depend on the optimization level appears to have been a copy-paste mistake (other LLVM functions called in this module also take a `bool` argument, but there it means something unrelated). Also see rust-lang#8855 (comment). I don't believe we have any reason to let LLVM omit user variables from DWARF, and we were already setting this to `true` when LLVM *could* optimize them away, so this PR should have no effect anyway. r? @michaelwoerister or @nagisa cc @rkruppe @nikomatsakis

@michaelwoerister

…agisa rustc_codegen_llvm: always set AlwaysPreserve on all debuginfo variables Making this depend on the optimization level appears to have been a copy-paste mistake (other LLVM functions called in this module also take a `bool` argument, but there it means something unrelated). Also see rust-lang#8855 (comment). I don't believe we have any reason to let LLVM omit user variables from DWARF, and we were already setting this to `true` when LLVM *could* optimize them away, so this PR should have no effect anyway. r? @michaelwoerister or @nagisa cc @rkruppe @nikomatsakis

Add test for rust-lang#8855 Fix rust-lang#8855 Here is what I think is going on. First, the expression `format!("{:>6} {:>6}", a, b.to_string())` expands to: ```rust { let res = ::alloc::fmt::format(::core::fmt::Arguments::new_v1_formatted(&["", " "], &[::core::fmt::ArgumentV1::new_display(&a), ::core::fmt::ArgumentV1::new_display(&b.to_string())], &[::core::fmt::rt::v1::Argument { position: 0usize, format: ::core::fmt::rt::v1::FormatSpec { fill: ' ', align: ::core::fmt::rt::v1::Alignment::Right, flags: 0u32, precision: ::core::fmt::rt::v1::Count::Implied, width: ::core::fmt::rt::v1::Count::Is(6usize), }, }, ::core::fmt::rt::v1::Argument { position: 1usize, format: ::core::fmt::rt::v1::FormatSpec { fill: ' ', align: ::core::fmt::rt::v1::Alignment::Right, flags: 0u32, precision: ::core::fmt::rt::v1::Count::Implied, width: ::core::fmt::rt::v1::Count::Is(6usize), }, }], unsafe { ::core::fmt::UnsafeArg::new() })); res } ``` When I dump the expressions that get past the call to `has_string_formatting` [here](https://github.com/rust-lang/rust-clippy/blob/b312ad7d0cf0f30be2bd4658b71a3520a2e76709/clippy_lints/src/format_args.rs#L83), I see more than I would expect. In particular, I see this subexpression of the above: ``` &[::core::fmt::ArgumentV1::new_display(&a), ::core::fmt::ArgumentV1::new_display(&b.to_string())], ``` This suggests to me that more expressions are getting past [this call](https://github.com/rust-lang/rust-clippy/blob/b312ad7d0cf0f30be2bd4658b71a3520a2e76709/clippy_lints/src/format_args.rs#L71) to `FormatArgsExpn::parse` than should. Those expressions are then visited, but no `::core::fmt::rt::v1::Argument`s are found and pushed [here](https://github.com/rust-lang/rust-clippy/blob/b312ad7d0cf0f30be2bd4658b71a3520a2e76709/clippy_utils/src/macros.rs#L407). As a result, the expressions appear unformatted, hence, the false positive. My proposed fix is to restrict `FormatArgsExpn::parse` so that it only matches `Call` expressions. cc: `@akanalytics` changelog: none

michaelwoerister added 7 commits September 4, 2013 18:38

debuginfo: Support for variables captured in closures and closure typ…

b81ea86

…e descriptions.

debuginfo: Support for by-value arguments (still excluding some cases…

30375cc

… of self arguments)

debuginfo: Added test cases for structs, tuples, enums, etc passed by…

c19f493

… value. Also updated documentation comments in debuginfo and renamed DebugContext to CrateDebugContext.

debuginfo: Always copy args to allocas if debuginfo is enabled

6b2df76

debuginfo: Fixed some merge fallout

e0b63b0

debuginfo: Much improved handling of captured variables and by-value …

c49eb07

…arguments.

debuginfo: Fixed some merge fallout

5b94ae9

bors closed this Sep 4, 2013

michaelwoerister deleted the captured_vars branch July 9, 2014 07:51

eddyb reviewed Feb 3, 2020

View reviewed changes

This was referenced Feb 4, 2020

llvm.dbg.value should be used instead of llvm.dbg.declare wherever possible. #68817

Open

rustc_codegen_llvm: always set AlwaysPreserve on all debuginfo variables #68881

Merged

Jarcho pushed a commit to Jarcho/rust that referenced this pull request Aug 29, 2022

Add test for rust-lang#8855

6f3d398

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

debuginfo: Captured variables and large pass-by-value arguments #8855

debuginfo: Captured variables and large pass-by-value arguments #8855

michaelwoerister commented Aug 29, 2013

jdm commented Aug 29, 2013

michaelwoerister commented Aug 29, 2013

michaelwoerister commented Aug 30, 2013

michaelwoerister commented Aug 30, 2013

michaelwoerister commented Sep 4, 2013

jdm commented Sep 4, 2013

michaelwoerister commented Sep 4, 2013

eddyb Feb 3, 2020 •

edited

debuginfo: Captured variables and large pass-by-value arguments #8855

debuginfo: Captured variables and large pass-by-value arguments #8855

Conversation

michaelwoerister commented Aug 29, 2013

jdm commented Aug 29, 2013

michaelwoerister commented Aug 29, 2013

michaelwoerister commented Aug 30, 2013

michaelwoerister commented Aug 30, 2013

michaelwoerister commented Sep 4, 2013

jdm commented Sep 4, 2013

michaelwoerister commented Sep 4, 2013

eddyb Feb 3, 2020 • edited

Choose a reason for hiding this comment

eddyb Feb 3, 2020 •

edited