This is an archive of the discontinued LLVM Phabricator instance.

[CodeGen] Split CGDebugInfo into CGDebugInfo and CGDebugDwarf
Needs RevisionPublic

Authored by rnk on Apr 13 2016, 1:44 PM.

Details

Reviewers
echristo
aprantl
Summary

The DWARF part contains all the logic for dealing with DITypes.

The split isn't very clean, there are a large number of places where
DISubprogram generation calls into type info generation. All of these
places currently have protected virtual methods to allow calling into
the dwarf specific code without changing it too heavily. Later these
call sites will be cleaned up to behave appropriately for CV and DWARF.

Diff Detail

Event Timeline

rnk updated this revision to Diff 53614.Apr 13 2016, 1:44 PM
rnk retitled this revision from to [CodeGen] Split CGDebugInfo into CGDebugInfo and CGDebugDwarf.
rnk added reviewers: echristo, aprantl.
rnk updated this object.
rnk added a subscriber: cfe-commits.
rnk updated this revision to Diff 53615.Apr 13 2016, 1:49 PM
  • Further minimize diff
aprantl added inline comments.Apr 13 2016, 3:23 PM
lib/CodeGen/CodeGenModule.h
292

This could be a separate useful NFC commit.

echristo added inline comments.Apr 13 2016, 3:24 PM
lib/CodeGen/CodeGenModule.h
292

And can you fix the formatting of the stuff above it while you're at "useful NFC commits"?

rnk marked 2 inline comments as done.Apr 13 2016, 4:44 PM
rnk added inline comments.
lib/CodeGen/CodeGenModule.h
292

r266263

rnk updated this revision to Diff 53641.Apr 13 2016, 4:44 PM
rnk marked an inline comment as done.
  • Rebase over unique_ptr change
aprantl added inline comments.Apr 18 2016, 12:53 PM
lib/CodeGen/CGDebugInfo.h
97

Why does this need to be virtual?

298

As you said, this isn't very clean. Can you outline how this can be improved later? Will CodeView still need some form of DISubroutineTypes?

348

Should this be part of CGDebugInfoDwarf?

350
  1. Why does retainType need to be virtual?
  2. I assume that the list of retained types is meaningless for CodeView? Could this be moved to CGDebugInfoDwarf instead?
354–356

redundant private.

rnk marked an inline comment as done.Apr 18 2016, 2:06 PM
rnk added inline comments.
lib/CodeGen/CGDebugInfo.h
97

I guess it adjusts the CU, which is shared. OK, I'll move it back to CGDebugInfo. It just felt fairly dwarf-specific.

298

I think the parameters are right for CodeView and DWARF, but the return type will have to be DITypeRef so that I can get a DITypeIndex out of here. My next patch changes this and a few other return types.

348

The implementation directly uses DIBuilder to make a DICompositeType. If we wanted to make this logic common, we would have to first build a synthetic clang AST RecordDecl and then send that through getOrCreateType.

350
  1. Yes, I intend to keep retainType as a no-op for CodeView. If a type makes it to the stream, it's up to the linker to decide whether it keeps it or not.
  2. Are you asking if I can sink all the callers of this method down into CGDebugDwarf? Possibly, but EmitGlobalVariable directly references it, and I would expect that to be part of CGDebugInfo. We could come up with a different level of indirection, such as getOrCreateGlobalVariableType, similar to getOrCreateFunctionType and sink that logic to DWARF.
rnk updated this revision to Diff 54114.Apr 18 2016, 2:06 PM
  • Rebase, move setDwoId back to CGDebugInfo
echristo requested changes to this revision.Apr 18 2016, 2:07 PM
echristo edited edge metadata.

As a note I'm going to want to review this before it goes in.

This revision now requires changes to proceed.Apr 18 2016, 2:07 PM

This appears to be a different direction from what we'd agreed upon. Please
update the original thread with discussion and your intentions and a
breakdown of costs, long-term maintainability, and your plans here.

Thanks.