Page MenuHomePhabricator

[Coroutines] Use dbg.declare for frame variables
ClosedPublic

Authored by modocache on Feb 28 2020, 4:04 AM.

Details

Summary

https://gist.github.com/modocache/ed7c62f6e570766c0f39b35dad675c2f
is an example of a small C++ program that uses C++20 coroutines that
is difficult to debug, due to the loss of debug info for variables that
"spill" across coroutine suspension boundaries. This patch addresses
that issue by inserting 'llvm.dbg.declare' intrinsics that point the
debugger to the variables' location at an offset to the coroutine frame.

With this patch, I confirmed that running the 'frame variable' commands in
https://gist.github.com/modocache/ed7c62f6e570766c0f39b35dad675c2f at
the specified breakpoints results in the correct values being printed
for coroutine frame variables 'i' and 'j' when using an lldb built from
trunk, as well as with gdb 8.3 (lldb 9.0.1, however, could not print the
values). The added test case also verifies this improved behavior.

The existing coro-debug.ll test case is also modified to reflect the
locations at which Clang actually places calls to 'dbg.declare', and
additional checks are added to ensure this patch works as intended in that
example as well.

Diff Detail

Event Timeline

modocache created this revision.Feb 28 2020, 4:04 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 28 2020, 4:04 AM
modocache marked an inline comment as done.Feb 28 2020, 4:07 AM
modocache added inline comments.
llvm/test/Transforms/Coroutines/coro-debug-frame-variable.ll
878

Beginner question of mine: is there a good way to reduce this debugging metadata to just the nodes that are necessary for my test? The 654 (!!) nodes here are the result of having run llvm-reduce on this test case, but I think many of these remaining nodes come from my having #include <experimental/coroutine> in my test program, and not actually necessary for the test case.

dstenb added a subscriber: dstenb.Feb 28 2020, 4:30 AM
dstenb added inline comments.
llvm/test/Transforms/Coroutines/coro-debug-frame-variable.ll
878

Bugpoint can reduce metadata, but I have personally found that it is not the best at that. It might just be a PEBKAC problem though. I personally just try to minimize the number of metadata nodes that Clang emits by first creducing the source file, omitting information that is not needed (e.g. passing -gno-column-info and -fno-strict-aliasing depending on situation), and then hand reducing the IR file.

In this case it seems that we can remove all the import node references in !108 (I assume they are not needed for this test case), and then run opt -S on the file, to remove all unused nodes. That leaves around 120 metadata nodes. I don't know much about C++ metadata (especially coroutines), so not sure how much more can be removed, but perhaps that is a start?

modocache updated this revision to Diff 247262.Feb 28 2020, 7:17 AM

Wow, great suggestion, @dstenb! That brought the number of nodes down to 119 -- a reduction of 82%! (Also, I ran clang-format on the include files to appease the pre-merge checkers -- although to be honest, I'm not sure I agree with the rationale here, because this diff didn't modify the include that clang-format is complaining about. But oh well.)

modocache updated this revision to Diff 247264.Feb 28 2020, 7:27 AM

I played around a bit more and got it down to just 13 nodes. I think this is the bare minimum.

modocache marked an inline comment as done.Feb 28 2020, 7:27 AM
modocache updated this revision to Diff 247266.Feb 28 2020, 7:31 AM

Remove Git commit hashes from the debug info metadata, just to slim it down a tiny bit more.

Harbormaster completed remote builds in B47591: Diff 247264.
Harbormaster completed remote builds in B47592: Diff 247266.
aprantl added inline comments.Feb 28 2020, 9:17 AM
llvm/test/Transforms/Coroutines/coro-debug-frame-variable.ll
878

Delta http://delta.tigris.org is always your friend. My favorite trick is taking all nodes that are lists of metadata (!1 = {!2, !3}) and making sure the elements are on a separate line followed by a run of delta. Other methods include removing (most) !dbg attachments on instructions and of course removing all unnecessary dbg.value intrinsics and their dependencies. You can also replace most "parent:"-kind fields with a link to the DIFile or CU.

As a side note: Are you using LLDB as your debugger? We could probably use a couple of end-to-end tests in the LLDB testsuite for coroutines as well if you are interested in contributing one.

aprantl added inline comments.Feb 28 2020, 9:25 AM
llvm/test/Transforms/Coroutines/coro-debug-frame-variable.ll
29

we should also check that the !dbg location attachment is correct.

As a side note: Are you using LLDB as your debugger? We could probably use a couple of end-to-end tests in the LLDB testsuite for coroutines as well if you are interested in contributing one.

I would absolutely love to add some! I was hoping to supplement the tests here with something that would act as an end-to-end regression test of my specific C++ source file. I'll grep around for examples of how to do that, if there's a specific doc on how to add my first test please point it out to me :)

As a side note: Are you using LLDB as your debugger? We could probably use a couple of end-to-end tests in the LLDB testsuite for coroutines as well if you are interested in contributing one.

I would absolutely love to add some! I was hoping to supplement the tests here with something that would act as an end-to-end regression test of my specific C++ source file. I'll grep around for examples of how to do that, if there's a specific doc on how to add my first test please point it out to me :)

A good starting point is the LLDB example test: https://github.com/llvm/llvm-project/tree/master/lldb/test/API/sample_test (This example shows *both* how to write an inline test and one with a separate python driver).

I tried adding an lldb test for this, but I encountered 39 failing tests when I ran ninja cxx check-lldb, and it made me wonder whether tests that use libc++ are currently broken?

Testing Time: 343.25s
********************
Failing Tests (39):
    lldb-api :: commands/expression/import-std-module/basic/TestImportStdModule.py
    lldb-api :: commands/expression/import-std-module/conflicts/TestStdModuleWithConflicts.py
    lldb-api :: commands/expression/import-std-module/deque-basic/TestDequeFromStdModule.py
    lldb-api :: commands/expression/import-std-module/deque-dbg-info-content/TestDbgInfoContentDequeFromStdModule.py
    lldb-api :: commands/expression/import-std-module/forward_list-dbg-info-content/TestDbgInfoContentForwardListFromStdModule.py
    lldb-api :: commands/expression/import-std-module/forward_list/TestForwardListFromStdModule.py
    lldb-api :: commands/expression/import-std-module/list-dbg-info-content/TestDbgInfoContentListFromStdModule.py
    lldb-api :: commands/expression/import-std-module/list/TestListFromStdModule.py
    lldb-api :: commands/expression/import-std-module/no-std-module/TestMissingStdModule.py
    lldb-api :: commands/expression/import-std-module/queue/TestQueueFromStdModule.py
    lldb-api :: commands/expression/import-std-module/shared_ptr-dbg-info-content/TestSharedPtrDbgInfoContentFromStdModule.py
    lldb-api :: commands/expression/import-std-module/shared_ptr/TestSharedPtrFromStdModule.py
    lldb-api :: commands/expression/import-std-module/stack/TestStackFromStdModule.py
    lldb-api :: commands/expression/import-std-module/unique_ptr-dbg-info-content/TestUniquePtrDbgInfoContent.py
    lldb-api :: commands/expression/import-std-module/unique_ptr/TestUniquePtrFromStdModule.py
    lldb-api :: commands/expression/import-std-module/vector-bool/TestVectorBoolFromStdModule.py
    lldb-api :: commands/expression/import-std-module/vector-dbg-info-content/TestDbgInfoContentVectorFromStdModule.py
    lldb-api :: commands/expression/import-std-module/vector-of-vectors/TestVectorOfVectorsFromStdModule.py
    lldb-api :: commands/expression/import-std-module/vector/TestVectorFromStdModule.py
    lldb-api :: commands/expression/import-std-module/weak_ptr-dbg-info-content/TestDbgInfoContentWeakPtrFromStdModule.py
    lldb-api :: commands/expression/import-std-module/weak_ptr/TestWeakPtrFromStdModule.py
    lldb-api :: functionalities/data-formatter/data-formatter-stl/libcxx/bitset/TestDataFormatterLibcxxBitset.py
    lldb-api :: functionalities/data-formatter/data-formatter-stl/libcxx/forward_list/TestDataFormatterLibcxxForwardList.py
    lldb-api :: functionalities/data-formatter/data-formatter-stl/libcxx/function/TestLibCxxFunction.py
    lldb-api :: functionalities/data-formatter/data-formatter-stl/libcxx/iterator/TestDataFormatterLibccIterator.py
    lldb-api :: functionalities/data-formatter/data-formatter-stl/libcxx/list/loop/TestDataFormatterLibcxxListLoop.py
    lldb-api :: functionalities/data-formatter/data-formatter-stl/libcxx/map/TestDataFormatterLibccMap.py
    lldb-api :: functionalities/data-formatter/data-formatter-stl/libcxx/multimap/TestDataFormatterLibccMultiMap.py
    lldb-api :: functionalities/data-formatter/data-formatter-stl/libcxx/multiset/TestDataFormatterLibcxxMultiSet.py
    lldb-api :: functionalities/data-formatter/data-formatter-stl/libcxx/optional/TestDataFormatterLibcxxOptional.py
    lldb-api :: functionalities/data-formatter/data-formatter-stl/libcxx/queue/TestDataFormatterLibcxxQueue.py
    lldb-api :: functionalities/data-formatter/data-formatter-stl/libcxx/set/TestDataFormatterLibcxxSet.py
    lldb-api :: functionalities/data-formatter/data-formatter-stl/libcxx/string/TestDataFormatterLibcxxString.py
    lldb-api :: functionalities/data-formatter/data-formatter-stl/libcxx/tuple/TestDataFormatterLibcxxTuple.py
    lldb-api :: functionalities/data-formatter/data-formatter-stl/libcxx/unordered/TestDataFormatterUnordered.py
    lldb-api :: functionalities/data-formatter/data-formatter-stl/libcxx/variant/TestDataFormatterLibcxxVariant.py
    lldb-api :: functionalities/data-formatter/data-formatter-stl/libcxx/vbool/TestDataFormatterLibcxxVBool.py
    lldb-api :: functionalities/data-formatter/data-formatter-stl/libcxx/vector/TestDataFormatterLibcxxVector.py
    lldb-api :: lang/cpp/std-function-step-into-callable/TestStdFunctionStepIntoCallable.py

  Expected Passes    : 1604
  Expected Failures  : 5
  Unsupported Tests  : 294
  Unexpected Failures: 39

This list of 39 tests lines up fairly well with ones that specify USE_LIBCPP := 1 in their Makefile, or @add_test_categories(["libc++"]) in their Python test file. They all fail with the same assertion that indicates no breakpoints were hit: AssertionError: 0 != 1 : Expected 1 thread to stop at breakpoint, 0 did. Unfortunately, I couldn't find a working example of a test that uses libc++.

Initially, using a freshly rm -rf and mkdir'ed build directory, I ran ninja check-lldb, and all builds and tests succeeded. Then, I tried to add a simple test case that included <cstdio> and <experimental/coroutine>, but this failed with fatal error: 'cstdio' file not found. So I'm currently in a bind: build the cxx target that my test would depend on, and my check-lldb test suite begins failing. Don't build it, and I can't run my test. Also, for what it's worth, I tried a configuration that used cmake -DLLVM_ENABLE_PROJECTS="clang;clang-tools-extra;libcxx;libcxxabi;lld;lldb" and ninja check-all, and these same 39 tests also failed.

I checked the LLVM buildbots to see if any exercised a libc++ & lldb build. I found 3 active lldb buildbots:

Let me know if this is PEBKAC, I'd love to add a coroutines regression test to the suite, but at the moment I can't figure out how to successfully run the tests.

modocache updated this revision to Diff 247550.Mar 1 2020, 9:17 PM

Add checks for !dbg locations.

modocache marked 2 inline comments as done.Mar 1 2020, 9:17 PM
jmorse added a comment.Mar 2 2020, 7:37 AM

Thanks for pushing through and finding out how to improve co-routine variable locations Brian; I have a few comments inline. Alas, I can't help with the lldb tests.

We do have Dexter too [0] for running integration tests, although I've no idea whether it'll play nice with coroutines.

[0] https://github.com/llvm/llvm-project/tree/master/debuginfo-tests/dexter

llvm/lib/Transforms/Coroutines/CoroFrame.cpp
748

This might be splitting hairs, but FindDbgAddrUses will also return dbg.addr instructions, and promoting those to be dbg.declares would mess with the validity of other variable locations. Could you std::remove_if DbgAddrIntrinsic's first?

(I don't think this would happen with C++, but there are other IR producers).

769

"We cannot replace"?

777–780

This will also erase dbg.value intrinsics, which I think will correspond to pointer variables that point(ed) at an alloca losing a location. Those dbg.values should be set to "undef" instead of being deleted, so that they terminate any earlier variable locations. I'd suggest using FindDbgAddrUses to get dbg.declares and delete those; then calling replaceDbgUsesWithUndef on the alloca. Or alternately, delete the dbg.declare's when they're migrated, and just call replaceDbgUsesWithUndef on the alloca.

llvm/test/Transforms/Coroutines/coro-debug-frame-variable.ll
34

These explicitly numbered metadata nodes (!dbg !13) should be replaced with pattern matching, as you've done a few lines down.

219

NB, we usually delete any un-necessary attributes.

llvm/test/Transforms/Coroutines/coro-debug.ll
29–30

Why are these dbg.declares moving down -- is it to check that they're moved into the entry block later on?

133

This might need to be CHECK-NEXT if you want to ensure the dbg.declare is in the entry.resume block, not just after the label.

I tried adding an lldb test for this, but I encountered 39 failing tests when I ran ninja cxx check-lldb, and it made me wonder whether tests that use libc++ are currently broken?

They are not broken [1], but you may need to add libcxx and friends to your list of projects in your LLVM cmake invocation.

http://green.lab.llvm.org/green/view/LLDB/job/lldb-cmake/

They are not broken [1], but you may need to add libcxx and friends to your list of projects in your LLVM cmake invocation.

Thanks for the link to the buildbot! In my comment above, I mention:

Also, for what it's worth, I tried a configuration that used cmake -DLLVM_ENABLE_PROJECTS="clang;clang-tools-extra;libcxx;libcxxabi;lld;lldb" and ninja check-all, and these same 39 tests also failed.

When you say "add libcxx and friends to your list of projects in your LLVM cmake invocation", do you mean -DLLVM_ENABLE_PROJECTS=, or something else? The primary difference I see between my own build invocation and the buildbot is that it passes -DLLDB_TEST_USER_ARGS=.... I'll try mimicking those to see if that works. But, if it's alright, I think I'd like to respond to @jmorse's review comments and commit this patch, and add an lldb regression test in a separate follow-up patch.

modocache updated this revision to Diff 247780.Mar 2 2020, 7:22 PM
modocache marked 6 inline comments as done.

Thanks for the review, @jmorse! I responded to your comments: I delete only dbg.declare, replace other uses with undef, and I removed unnecessary attributes.

llvm/test/Transforms/Coroutines/coro-debug.ll
29–30

Right, sorry for the confusion. I mention in the commit message:

The existing coro-debug.ll test case is also modified to reflect the locations at which Clang actually places calls to 'dbg.declare', and additional checks are added to ensure this patch works as intended in that example as well.

This patch adds the CHECK below that the llvm.dbg.declare will correctly appear in the coroutine funclet f.resume, which makes it possible for debuggers to "find" the variable x when stopped in a resumed coroutine function.

But, the logic in the patch works on the assumption that the original llvm.dbg.declare describing the variable x appears not in the function's entry block (where Clang places it in a non-coroutine function), but instead in the basic block that represents the coroutine's resumption after its implicit "initial suspend" point (where Clang places it in a coroutine function, since Clang first codegen's the br instructions for the implicit suspend point, and then codegen's the function's body).

These dbg.declare don't need to be moved like this, but if they aren't, then the added CHECK below won't succeed -- the patch ensures that dbg.declare are migrated to coroutine funclets, but specifically only when dealing with IR in the form that Clang generates it.

jmorse accepted this revision.Mar 3 2020, 5:57 AM

LGTM with the remaining inline comments addressed, cheers.

llvm/lib/Transforms/Coroutines/CoroFrame.cpp
748

I'd recommend putting findDbgDeclareUses in Local.h/Local.cpp, we already have utilities there for finding {all-dbg-users, debug-values, debug-addresses}, we may as well make this available to other developers.

781–784

This should now be redundant and removed, as replaceDbgUsesWithUndef should have undef'd all debug users.

llvm/test/Transforms/Coroutines/coro-debug.ll
29–30

Thanks for the explanation!

This revision is now accepted and ready to land.Mar 3 2020, 5:57 AM
modocache added inline comments.Mar 3 2020, 12:20 PM
llvm/lib/Transforms/Coroutines/CoroFrame.cpp
748

Awesome! I'm very glad to do so, I just wasn't sure if having one user of the interface meant it shouldn't be in a shared header. But this sounds great to me, I'll try to add a unittest for it as well.

modocache updated this revision to Diff 248029.Mar 3 2020, 2:13 PM
modocache marked 7 inline comments as done.

Thanks again for the review. I moved findDbgDeclareUses to Local.h (no unittest for now but I'll add one in a follow-up diff for both it and FindDbgAddrUses, which doesn't have one either) and I removed the redundant loop. I'll commit this now and add an lldb regression test in a follow-up patch.

llvm/lib/Transforms/Coroutines/CoroFrame.cpp
781–784

Oops! I'd missed this and accidentally left it in. Thanks for catching! :)

This revision was automatically updated to reflect the committed changes.