Page MenuHomePhabricator

Infrastructure changes to support DW_OP_piece in LLVM IR.

Authored by aprantl on Apr 14 2014, 4:55 PM.



Here are the infrastructure changes to support DW_OP_piece in LLVM IR split out from D2680.

Diff Detail

Event Timeline

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!

  • adrian

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.

aprantl updated this revision to Diff 8878.EditedApr 27 2014, 3:52 PM
aprantl edited edge metadata.

This is the remaining patch, with the DebugLocEntry and AsmPrinter refactorings split out.

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.

Hi Adrian,

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?


P1 && P1?


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?




weird formatting?


Comment please :)


I'm not sure I understand the AtFnBegin stuff, could you explain it a bit more?


Comment update.


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?

Ah. Makes sense. Thanks.


aprantl updated this revision to Diff 9019.May 1 2014, 12:37 PM
aprantl edited edge metadata.

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().
aprantl updated this revision to Diff 9024.May 1 2014, 4:58 PM

Rebased on Aleksei's collectVariableDebugInfo refactoring.

aprantl updated this revision to Diff 9025.May 1 2014, 5:01 PM

Rebased on Alexey's recent debug value history refactoring changes.

aprantl updated this revision to Diff 9510.May 17 2014, 3:21 PM
aprantl removed a subscriber: jholewinski.

Rebased patch on today's ToT.

aprantl updated this revision to Diff 9668.May 21 2014, 3:52 PM

Rebased on Aleksei's huge improvement to DbgValueHistoryCalculator.cpp

aprantl updated this revision to Diff 9926.May 29 2014, 12:06 PM

Rebased on 209832.

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.

echristo added a subscriber: Unknown Object (MLST).Jul 24 2014, 5:33 PM
aprantl updated this revision to Diff 11987.Jul 29 2014, 10:48 AM

Addressed comments from previous round; mostly clarifications.

aprantl added inline comments.Jul 29 2014, 10:49 AM
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).

As a note to all I just added llvm-commits to the thread last week,
it'd been an ongoing review and llvm-commits had accidentally been


aprantl updated this revision to Diff 11988.Jul 29 2014, 11:18 AM

Fix a merge error that slipped in while rebasing this patch.

aprantl updated this revision to Diff 12002.Jul 29 2014, 5:21 PM

Added the definition of DIBuilder::createVariablePiece().

echristo accepted this revision.Jul 31 2014, 4:42 PM
echristo edited edge metadata.

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.




Extra whitespace.


Weird formatting?


Weird formatting?

605 ↗(On Diff #12002)

Comment with the format here that you're adding? Also probably const for the SizeOfByte.

This revision is now accepted and ready to land.Jul 31 2014, 4:42 PM