Here are the infrastructure changes to support DW_OP_piece in LLVM IR split out from D2680.
And apparently I failed to attach the accompanying text:
sorry for the long delay, but I was out sick at the beginning of the week, and then when I was rebasing the patch, I saw an opportunity to clean it up a little. Anyhow, here is the promised infrastructure-only patch. It still touches a lot of files, but most of the changes are very straight-forward.
docs/SourceLevelDebugging.rst -- documentation of piece complex exprs
include/llvm/IR/DIBuilder.h -- add createVariablePiece()
lib/IR/DIBuilder.cpp -- add createVariablePiece()
include/llvm/IR/DebugInfo.h -- support for Variable pieces
lib/IR/DebugInfo.cpp -- misc. pieces support functions
include/llvm/CodeGen/AsmPrinter.h -- move EmitDwarfRegOpPiece into AsmPrinter
lib/CodeGen/AsmPrinter/AsmPrinter.cpp -- prettyprinting of pieces
lib/CodeGen/AsmPrinter/AsmPrinterDwarf.cpp -- emit DWARF for pieces
lib/CodeGen/AsmPrinter/DebugLocEntry.h -- Support for more than 1 Value/Entry.
lib/CodeGen/AsmPrinter/DwarfDebug.cpp -- Range handling of DW_op_pieces
lib/CodeGen/AsmPrinter/DwarfDebug.h -- move setMInsn to .cpp
lib/CodeGen/AsmPrinter/DwarfUnit.cpp -- emit DW_op_pieces inside of a DW_AT_location
test/DebugInfo/X86/pieces-1.ll -- testcase!
I'm thinking about some of the rest of it, but have you thought about having the complex expression required and just be another metadata node that would be null if there's no complex expression there?
Some inline comments and a couple of global comments:
Can you do the AP/TM refactorings separate?
it looks like the DebugLocEntry rewrite can largely be done independently of the DW_OP_piece infrastructure so it'd be great if you could do that separately - it'd make the parts that are just for OP_piece very obvious and should be a "no functional change" patch.
These two splits are preapproved for committing separate. I think, other than the inline comments and the request to split things up, the format of the IR is the only thing I've got a concern about here.
Thanks for all the work!
|436 ↗||(On Diff #8515)|
Cut and paste.
|443 ↗||(On Diff #8515)|
Can we use the complex address stuff to encompass register-indirect addressing there so we don't have to pass the bool?
|222 ↗||(On Diff #8515)|
Text here explaining the why of the assert.
|229 ↗||(On Diff #8515)|
Odd formatting plus some text, i.e. "DW_OP_piece wants byte sized offsets".
|230 ↗||(On Diff #8515)|
We do this in so many places throughout llvm that it's, unfortunately, probably not a useful comment.
|348 ↗||(On Diff #8515)|
EmitDwarfReg perhaps instead since we've got a 0 offset?
This needs a comment of how and why it's used.
This is the remaining patch, with the DebugLocEntry and AsmPrinter refactorings split out.
- http://reviews.llvm.org/D3373 , which makes DIVariable complex address expressions separate MDNodes.
- LLVM r207368, r207369, and r207372
|348 ↗||(On Diff #8515)|
No, we specifically want the sub/super-register magic here. I clarified the doxygen comment for EmitDwarfRegOpPiece() in 207372 to make this more obvious.
Some inline comments. On first start it's looking pretty good. Thanks!
Perhaps a different name here since it's not just doing the set. Perhaps updateMachineInstr?
Mmm... implicit precedence knowledge required. Can you fix this up?
Is it possible to break this function up a bit? Realize it's an unrelated refactor, but it would help a lot.
Removed a nullptr initialization?
Comment please :)
I'm not sure I understand the AtFnBegin stuff, could you explain it a bit more?
Probably just emitLocPieces.
Clever. How about some text? :)
Not llc -O0? Or do we need things to be optimized by llc?
Duplicate label adding?
If you're going to leave this in then my personal preference would be to use -mtriple above. Is it possible this works for just "any x86 with dwarf" though?
Besides addressing the comments from the previous revision there are tow changes:
- Emit "empty" pieces (DW_OP_piece without a location) for unavailable pieces. This is actually mandated by the DWARF spec, but only mentioned en passant in the examples.
- Simplify and improve the range computation/coalescing that used to be in DwarfDebug::collectVariableInfo() and split it out to a new DwarfDebug::buildLocationList(). The new code is shorter, easier to reason about, and will result in easier-to-digest location lists, because it won't emit overlapping ranges any more. See also the comment above buildLocationList().
Looks like a good start, some cleanups, comments and questions inline and then we can continue the review.
|407 ↗||(On Diff #9926)|
|255 ↗||(On Diff #9926)|
This seems to be the only function you've changed the capitalization on?
|26 ↗||(On Diff #9926)|
This appears unused.
|111 ↗||(On Diff #9926)|
Description of the logic here?
Having a hard time visualizing the && here, where will both of these be true?
I'm not sure what's overlapping here.
|255 ↗||(On Diff #9926)|
Yes, I promoted it from being a static function to a method and adopted what appears to be the spelling convention in AsmPrinter (although it's not terribly consistent).
One comment and then nothing but nits in the inline stuff. It looks very good in general, however, tests? :)
I assume they were dropped somewhere so go ahead and add them and LGTM.
Thanks for all of your hard work here!
Extra white space.
|605 ↗||(On Diff #12002)|
Comment with the format here that you're adding? Also probably const for the SizeOfByte.