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

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

Closed
wants to merge 7 commits into
base: master
from

Conversation

Projects
None yet
3 participants
@michaelwoerister
Contributor

michaelwoerister commented Aug 29, 2013

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

@jdm

This comment has been minimized.

Contributor

jdm commented Aug 29, 2013

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

@michaelwoerister

This comment has been minimized.

Contributor

michaelwoerister commented Aug 29, 2013

Thanks!

@michaelwoerister

This comment has been minimized.

Contributor

michaelwoerister commented Aug 30, 2013

@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

This comment has been minimized.

Contributor

michaelwoerister commented Aug 30, 2013

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

bors added a commit that referenced this pull request Aug 31, 2013

auto merge of #8855 : michaelwoerister/rust/captured_vars, r=jdm
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

This comment has been minimized.

Contributor

michaelwoerister commented Sep 4, 2013

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

This comment has been minimized.

Contributor

jdm commented Sep 4, 2013

Oh, I just noticed that this needs another rebase.

@michaelwoerister

This comment has been minimized.

Contributor

michaelwoerister commented Sep 4, 2013

@jdm rebased...

@jdm

This comment has been minimized.

jdm commented on 5b94ae9 Sep 4, 2013

r+

@bors

This comment has been minimized.

Contributor

bors commented on 5b94ae9 Sep 4, 2013

saw approval from jdm
at michaelwoerister@5b94ae9

This comment has been minimized.

Contributor

bors replied Sep 4, 2013

merging michaelwoerister/rust/captured_vars = 5b94ae9 into auto

This comment has been minimized.

Contributor

bors replied Sep 4, 2013

michaelwoerister/rust/captured_vars = 5b94ae9 merged ok, testing candidate = 45c3ca7

This comment has been minimized.

Contributor

bors replied Sep 4, 2013

fast-forwarding master to auto = 45c3ca7

bors added a commit that referenced this pull request Sep 4, 2013

auto merge of #8855 : michaelwoerister/rust/captured_vars, r=jdm
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

@bors bors closed this Sep 4, 2013

@michaelwoerister michaelwoerister deleted the michaelwoerister:captured_vars branch Jul 9, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment