This is an archive of the discontinued LLVM Phabricator instance.

Move DwarfCompileUnit::getOrCreateGlobalVariableDIE to DwarfUnit.
ClosedPublic

Authored by friss on Oct 24 2014, 8:23 AM.

Details

Summary

I'm not sure this doesn't break some intended layering, but the end
goal is to call it from constructInportedEntityDIE which is in
DwarfUnit (along with the others getOrCreate* helpers) so I suppose
it's OK to move it there. (Another option is to move everything down
into DwarfCompileUnit, but if you want to create some other types of
Units, you might need this functionality in the base class).

BTW, although I think I can imaging where you are going to with the
code shuffling you are doing around the Dwarf(Compile)Unit, could you
explain a bit what the expected end result hierarchy looks like, and
what the responsabilities of each level will be? And maybe update the
file header comments of these 2 classes to reflect the difference.

Thanks!

Diff Detail

Event Timeline

friss updated this revision to Diff 15414.Oct 24 2014, 8:23 AM
friss retitled this revision from to Move DwarfCompileUnit::getOrCreateGlobalVariableDIE to DwarfUnit..
friss added a reviewer: dblaikie.
friss updated this object.
friss added a subscriber: Unknown Object (MLST).
dblaikie edited edge metadata.Oct 24 2014, 10:13 AM

Looks like the right answer for your end goal is instead to sink constructImportedEntityDIE into DwarfCompileUnit - it's only needed for compile units, not type units.

(at some point, we should supported imported entities at class scope - but those are a special case, they can't refer to global variables, only to functions declared in base classes - so we still wouldn't need the ability to emit global variable definitions or declarations in a type unit)

Sorry for the rather terse commit messages on some of my recent refactorings. I'd mentioned in some detail the direction in some of the commit messages, but it seemed verbose to repeat it on every step. The basic idea is that we need to emit -gmlt-like data into the skeleton type unit to support in-process symbolication (including inlining) in tools like ASan even when using fission/-gsplit-dwarf. This means I need much of the subprogram emission functionality to be reusable for both the full and skeleton type unit - currently all that state exists in DwarfDebug, so it can't be reused to do both (eg: the AbstractSPDies map - from MDNode* to DIE*, well if we produce abstract subprograms in both the full and skeleton unit, tehre will be two such DIEs for every MDNode, so the map can't live here) - by sinking these variables down into DwarfFile, there will be one for each file and we'll be able to produce subprograms and inlining in both the full and skeleton DIEs.

Unfortunately, variables are rather inseparable from function emission, so this is sinking too, even though -gmlt doesn't need/have any variables. It seems like better layering anyway.

Sinking operations into DwarfCompileUnit (in theory I could've just sunk all the functions and their variables only as far as DwarfFile) just seems better for layering - they're compile unit-y kind of operations. It also makes for smaller files - DwarfDebug is pretty big, so it's nice to be able to pull out a bunch of its logic into the one place that should really be handling that.

Hence the suggestion to pull constructImportedEntityDIE down into DwarfCompileUnit - we would never emit a global variable into a type unit (type units can't reference addresses, so it wouldn't make sense to do that) so it's nice to have that enforced/supported by the API. Indeed when I made this layering change I found and fixed at least one bug, if I recall correctly... though it is a bit hazy now.

friss updated this revision to Diff 15436.Oct 24 2014, 1:51 PM
friss edited edge metadata.

Changing this into "Sink DwarfUnit::constructImportedEntityDIE into DwarfCompileUnit."

The revision title/description isn't accurate anymore, but the commit log will be.

dblaikie accepted this revision.Oct 24 2014, 1:56 PM
dblaikie edited edge metadata.

Looks good (would've been totally fine for you to commit this without an extra review round-trip, sorry - I could've been more clear about that when I suggested the idea)

This revision is now accepted and ready to land.Oct 24 2014, 1:56 PM
friss closed this revision.Oct 24 2014, 2:41 PM
friss updated this revision to Diff 15438.

Closed by commit rL220594 (authored by @friss).