- User Since
- Mar 2 2013, 8:12 AM (216 w, 2 d)
Fri, Apr 21
Could you also add some unit tests for the new interface?
Thu, Apr 20
Besides my above comment, I'm very much looking forward to this commit. It will also help a lot with implementing dwarfdump -verify functionality.
LGTM with inline feedback addressed. Thanks!
Wed, Apr 19
Rather than scanning forward for DBG_VALUEs, is there a more systematic way to find them? E.g., by iterating over USEs? If not, doing it this way is probably fine.
Address review feedback. Thanks!
No, there's a typo :-)
Tue, Apr 18
okay — works for me.
Thanks! This LGTM from the debug info side of things. (One comment inline)
Mon, Apr 17
David, the review is still marked as accepted, but I did add a couple of changes since then, so I think I should be moving it back into "Request review".
Would you mind taking a second look?
Added test coverage for SelectionDAG in sret.ll and ensured it matches the FastISel behavior.
I think this is generally very nice. My question is, how do we want to deal with future changes to the API? Is it acceptable for future releases of LLVM to break the C-API? If the answer is yes, then I have absolutely no problem with adding this. Otherwise this may turn into a maintenance nightmare as we continue to evolve the debug info interface.
Fri, Apr 14
Addressed David's review feedback for the DW_OP_deref bitcode upgrade.
Tue, Apr 11
One last comment in line 664 otherwise looks fine to me.
Mon, Apr 10
Thu, Apr 6
Added a couple of superficial coding style comments.
Thanks a lot for reviewing this, David! While I was preparing the next updated of the corresponding clang changes in https://reviews.llvm.org/D31440 I realized that there is one use-case that is not supported by this variant. To encode locations for by-ref captures clang wants to describe a location that is at an offset inside of a larger alloca. To support this use-case I need to remove the implicit DW_OP_deref that we are adding to dbg.declares everywhere and make it explicit instead.
This will be a good change:
- it will make a future patch that replaces all instances of dbg.declare with dbg.value very simple
- we won't need to rewrite DIExpressions when we lower dbg.declare -> dbg.value
- it will make the DW_OP_xderef use-case that I complain about in DwarfCompileUnit.cpp:570 appear less out-of-place.
I'll be back here with an updated (albeit somewhat longer) patch soon. Meanwhile I'm also extending the debuginfo-tests a bit to make sure I'm not breaking the integration between llvm and clang.
Wed, Apr 5
Can you add a CU that only has a DIGlobalVariableExpression with a DIExpression(DW_OP_constu 0, DW_OP_stack_value) to your testcase and verify that it survives?
Otherwise this LGTM.
Tue, Apr 4
Mon, Apr 3
Add accidentally removed check for the debug info level back in.
Add DIEDwarfExpression::addAddress() and DIEDwarfExpression::addThreadLocalAddress().
Wed, Mar 29
Tue, Mar 28
Mar 24 2017
This needs a testcase.
Mar 23 2017
Mar 21 2017
I think it is off by default because it doesn't really help the debugger and is more useful as a informational tool (e.g., when investigating compiler bugs). It also might leak unexpected information into the build (paths, macros, etc, ...) that a user might want to have control over.
Sorry for being late to the party, but have you looked at CodeGenOptions::DwarfDebugFlags? It looks like it almost does what you want, but it is currently only enabled for MachO and when a specific environment variable is set. Would it make sense to generalize that mechanism instead?
Mar 20 2017
Mar 17 2017
Rebased on trunk and marked outstanding review comments as done.
Took me a while to find the functional change amidst the refactoring :-)
As discussed in the PR, this LGTM, thanks!
One inline comment about a comment.
Mar 16 2017
Landed together with a fix for dw_op_minus_direct.ll.
Mar 15 2017
... up until the point where I discover that I already created such a testcase a couple of months ago (llvm/test/DebugInfo/MIR/X86/bit-piece-dh.mir).
To review my own patch; I think we now have a coverage gap, since there is no testcase that tests a variable that is in a subregister at a nonzero offset.
Use the functionally equivalent but more appropriate APInt::getSExtValue() and rename some locals for better readability.
I think that should be all outstanding comments.
Mar 14 2017
New version attached.
Working on an updated version now...
Mar 13 2017
Mar 8 2017
Two tiny nitpicks, but apart from that I'm happy.
Mar 7 2017
Inline comments, otherwise looks ok.
Mar 6 2017
My LGTM still stands, there's nothing left here that couldn't be fixed in a post-commit review. I found one more opportunity to simplify the code a bit inline.
Seems generally plausible. LGTM with all inline comments addressed.
Mar 4 2017
Mar 3 2017
Seems plausible. (inline comments pending, ...)
Mar 2 2017
Few more inline comments, feel free to commit whenever ready.
Sorry for the delay. Few comments inline, LGTM with these addressed.
Mar 1 2017
Could you also add the missing languages (at least BLISS & Renderscript) to the switch while you are in there?
Feb 28 2017
Thanks, this looks good.
Feb 27 2017
A few stylistic issues inline.
It also wouldn't hurt to document what is being tested, either in the clang-import-test sources or in the input files. Otherwise this looks fine. More tests are always great!
Thanks for working on this!
Feb 24 2017
It would be good to split this into an NFC patch that just moves stuff around and then a small commit+testcase for the new functionality. This makes reviewing easier & faster.