This is an archive of the discontinued LLVM Phabricator instance.

Update Clang for D20147 ("DebugInfo: New metadata representation for global variables.")
ClosedPublic

Authored by pcc on May 18 2016, 10:43 PM.

Diff Detail

Repository
rL LLVM

Event Timeline

pcc updated this revision to Diff 57738.May 18 2016, 10:43 PM
pcc retitled this revision from to Update Clang for D20147 ("DebugInfo: New metadata representation for global variables.").
pcc updated this object.
pcc added reviewers: dexonsmith, dblaikie, aprantl.
pcc added a subscriber: cfe-commits.
pcc planned changes to this revision.May 18 2016, 10:45 PM

One thing that I forgot to do here was to add a test covering my changes to CGDebugInfo::EmitGlobalVariable. I'll do that momentarily.

pcc updated this revision to Diff 57809.May 19 2016, 9:13 AM
  • Add a test for DW_OP_stack_value
aprantl added inline comments.May 20 2016, 9:30 AM
lib/CodeGen/CGDebugInfo.cpp
3393 ↗(On Diff #57809)

Is there a good reason for not changing the DIBuilder interface to drop the global field?

3477 ↗(On Diff #57809)

Are we regressing floating point constants here?

aprantl added inline comments.May 20 2016, 9:34 AM
lib/CodeGen/CGDebugInfo.cpp
3393 ↗(On Diff #57809)

Sorry about the confusion, I should have read the other patch first. This has morphed into the DIExpression. That's fine!

pcc added inline comments.May 20 2016, 1:09 PM
lib/CodeGen/CGDebugInfo.cpp
3477 ↗(On Diff #57809)

It looks like we never really supported floating point constants in the DWARF output. I can only see support for integer constants:
http://llvm-cs.pcc.me.uk/lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp#192

aprantl added inline comments.May 20 2016, 1:48 PM
lib/CodeGen/CGDebugInfo.cpp
3477 ↗(On Diff #57809)

Looking at http://llvm-cs.pcc.me.uk/test/DebugInfo/X86/float_const.ll we at least do to some degree :-)

pcc added inline comments.May 20 2016, 1:52 PM
lib/CodeGen/CGDebugInfo.cpp
3477 ↗(On Diff #57809)

That isn't a global variable test case though, is it?

aprantl added inline comments.May 20 2016, 1:56 PM
lib/CodeGen/CGDebugInfo.cpp
3477 ↗(On Diff #57809)

Ah that's right. We didn't support global float constants.

aprantl accepted this revision.May 31 2016, 11:17 AM
aprantl edited edge metadata.

LGTM with small changes.

lib/CodeGen/CGDebugInfo.cpp
3427 ↗(On Diff #57809)

I think it would be more readable to pass an empty DIExpression() here.

3474 ↗(On Diff #57809)

Shouldn't the default constructor of DIExpression() wrap an MDNode nullptr?

lib/CodeGen/CGDebugInfo.h
348 ↗(On Diff #57809)

While we're here: Emit *a* global variable's debug info?

This revision is now accepted and ready to land.May 31 2016, 11:17 AM
aprantl added inline comments.May 31 2016, 11:37 AM
lib/CodeGen/CGDebugInfo.cpp
3427 ↗(On Diff #57809)

/* expression=*/ is good enough.

3474 ↗(On Diff #57809)

Sorry, that was a couple of IR evolution steps ago. DIExpression now inserts from MDNode.

pcc updated this revision to Diff 59260.Jun 1 2016, 11:42 AM
pcc marked 2 inline comments as done.
pcc edited edge metadata.
  • Address review comments
This revision was automatically updated to reflect the committed changes.