Emit template type and value parameter DIEs for template variables.
ClosedPublic

Authored by ormris on Sep 13 2018, 1:24 PM.

Details

Summary

Ensure the TemplateParam attribute of the DIGlobalVariable node is translated into the proper DIEs.

Resolves https://bugs.llvm.org/show_bug.cgi?id=22119

Diff Detail

Repository
rL LLVM
ormris created this revision.Sep 13 2018, 1:24 PM

Since you're changing the IR please also add a round-trip test (llvm-as | llvm-dis | llvm-as | llvm-dis).

include/llvm-c/DebugInfo.h
977 ↗(On Diff #165366)

The C-API is considered stable. You could extend it if you have a use case for this?

ormris added inline comments.Sep 14 2018, 9:19 AM
include/llvm-c/DebugInfo.h
977 ↗(On Diff #165366)

Not currently. I'm happy removing this part of the change.

Since you're changing the IR please also add a round-trip test (llvm-as | llvm-dis | llvm-as | llvm-dis).

OK. Will do.

ormris updated this revision to Diff 165788.Sep 17 2018, 10:58 AM
  • Removed LLVM-C API changes
  • Added a round trip test for IR changes
  • Rebased
ormris marked an inline comment as done.Sep 17 2018, 11:01 AM
dblaikie added inline comments.Sep 18 2018, 11:19 AM
test/DebugInfo/X86/template.ll
6–9 ↗(On Diff #165788)

I'm guessing this source code is out of date, since it doesn't appear to include a global variable template named 'var'?

20 ↗(On Diff #165788)

Presumably this should actually be named 'var<int>'? Not a big deal, since this is just using whatever name is in the IR To begin with - but might make the test case less confusing.

ormris added inline comments.Sep 18 2018, 11:23 AM
test/DebugInfo/X86/template.ll
6–9 ↗(On Diff #165788)

Yep. That's out of date. Will fix.

ormris updated this revision to Diff 166022.Sep 18 2018, 1:03 PM

Update source code in template test

ormris marked an inline comment as done.Sep 18 2018, 1:09 PM
ormris added inline comments.
test/DebugInfo/X86/template.ll
20 ↗(On Diff #165788)

In this case, the template parameters are not added to the variable name.

dblaikie added inline comments.Sep 18 2018, 1:16 PM
test/DebugInfo/X86/template.ll
20 ↗(On Diff #165788)

Why's that?

The parameters are part of the variable name for class and function templates - seems they'd be necessary/useful to disambiguate for variable templates too.

(though I do see that GCC (8.1) also doesn't seem to put template parameters on variable templates... )

ormris added inline comments.Sep 18 2018, 1:25 PM
test/DebugInfo/X86/template.ll
20 ↗(On Diff #165788)

I did implement that in a previous patch (D44842), but it breaks LLDB's global symbol lookup.

ormris added inline comments.Sep 18 2018, 2:20 PM
test/DebugInfo/X86/template.ll
20 ↗(On Diff #165788)

Correction: It breaks global variable look up, not global symbol lookup.

whitequark resigned from this revision.Tue, Sep 25, 12:49 PM
dblaikie accepted this revision.Mon, Oct 1, 1:55 PM

Thanks!

test/DebugInfo/X86/template.ll
20 ↗(On Diff #165788)

Fair enough - not a bug to be fixed here anyway. Hopefully LLDB is tracking the fix for that & Clang can switch over after that. Assuming it works for GDB.

(though perhaps it'd be best to make this test case look like the expected name (rather than the bug-workaround name) - so as to be less surprising to folks working on LLVM/reading this in the future?)

This revision is now accepted and ready to land.Mon, Oct 1, 1:55 PM
ormris added a comment.Mon, Oct 1, 2:42 PM

Thanks for your comments! Does my Clang patch need further review?

ormris edited the summary of this revision. (Show Details)Wed, Oct 3, 11:42 AM
ormris edited the summary of this revision. (Show Details)Wed, Oct 3, 11:45 AM
This revision was automatically updated to reflect the committed changes.