Page MenuHomePhabricator

Infrastructure changes to support DW_OP_piece in LLVM IR.
ClosedPublic

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

Details

Reviewers
echristo
Summary

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!

include/llvm/CodeGen/AsmPrinter.h
436

Cut and paste.

443

Can we use the complex address stuff to encompass register-indirect addressing there so we don't have to pass the bool?

lib/CodeGen/AsmPrinter/AsmPrinterDwarf.cpp
222

Text here explaining the why of the assert.

229

Odd formatting plus some text, i.e. "DW_OP_piece wants byte sized offsets".

230

We do this in so many places throughout llvm that it's, unfortunately, probably not a useful comment.

348

EmitDwarfReg perhaps instead since we've got a 0 offset?

lib/CodeGen/AsmPrinter/DwarfDebug.cpp
1375

Formatting.

lib/IR/DebugInfo.cpp
945

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.
Related:

lib/CodeGen/AsmPrinter/AsmPrinterDwarf.cpp
348

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!

-eric

lib/CodeGen/AsmPrinter/DwarfDebug.cpp
110

Perhaps a different name here since it's not just doing the set. Perhaps updateMachineInstr?

1174

P1 && P1?

1175

Mmm... implicit precedence knowledge required. Can you fix this up?

1210

Is it possible to break this function up a bit? Realize it's an unrelated refactor, but it would help a lot.

1286

Removed a nullptr initialization?

1304

nullptr?

1311

weird formatting?

1326

Comment please :)

1514

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

1594

Comment update.

2144

Probably just emitLocPieces.

2151

Clever. How about some text? :)

test/DebugInfo/X86/pieces-1.ll
1 ↗(On Diff #8878)

Not llc -O0? Or do we need things to be optimized by llc?

30 ↗(On Diff #8878)

Duplicate label adding?

34 ↗(On Diff #8878)

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.

-eric

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.

Thanks!

-eric

docs/SourceLevelDebugging.rst
592

s/addresses/locations

594

s/address/location

include/llvm/CodeGen/AsmPrinter.h
407

s/small//

lib/CodeGen/AsmPrinter/AsmPrinterDwarf.cpp
255

This seems to be the only function you've changed the capitalization on?

lib/CodeGen/AsmPrinter/DebugLocEntry.h
26

This appears unused.

111

Description of the logic here?

lib/CodeGen/AsmPrinter/DwarfDebug.cpp
1155

Having a hard time visualizing the && here, where will both of these be true?

1180

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
lib/CodeGen/AsmPrinter/AsmPrinterDwarf.cpp
255

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
forgotten.

-eric

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!

-eric

lib/CodeGen/AsmPrinter/DwarfDebug.cpp
1240

Extra white space.

1242

"inaccessible"

1256

Extra whitespace.

2086

Weird formatting?

2092

Weird formatting?

lib/CodeGen/AsmPrinter/DwarfUnit.cpp
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