This is an archive of the discontinued LLVM Phabricator instance.

Add a unit field to DIGlobalVariable, mirroring the design of DISubprogram.
Needs ReviewPublic

Authored by aprantl on Dec 20 2016, 4:16 PM.

Details

Summary

This has been suggested as an incremental follow-up to https://reviews.llvm.org/D26769.

Adding a unit field to DIGlobalVariable makes the design consistent with the design of DISubprogram
(DISubprogram definitions that are attached to Functions also have a unit field).
It also allows us to more efficiently process global variables in the DWARF backend.

After this patch, global variables occur in three forms in the debug info metadata:

  1. Variable definitions are reachable via a !dbg attachment on a global that points to a DIGlobalVariableExpression which points to a distinct DIGlobalVariable (and an optional DIExpression) with a unit field pointing to the variable's DICompileUnit.
  1. Constant definitions are reachable via the DICompileUnit's list of globals which points to a DIGlobalVariableExpression with a DIGlobalVariable without a unit field and a DIExpression describing a constant value.
  1. Declarations are DIGlobalVariables without a unit field that are only reachable via a DIImportedEntity and have isDefinition set to false.

Diff Detail

Event Timeline

aprantl updated this revision to Diff 82172.Dec 20 2016, 4:16 PM
aprantl retitled this revision from to Add a unit field to DIGlobalVariable, mirroring the design of DISubprogram..
aprantl updated this object.
aprantl added reviewers: dexonsmith, dblaikie.
aprantl added a subscriber: llvm-commits.
aprantl added inline comments.Dec 20 2016, 4:32 PM
lib/Bitcode/Writer/BitcodeWriter.cpp
1681

Note that I accidentally overwrote my correct bitocde upgrade code with an older revision while separating this patch out of my WIP D26769 patch. Will post the correct code and a bitcode upgrade testcase here soon.

aprantl updated this revision to Diff 82385.Dec 22 2016, 4:53 PM

Upload latest WIP.

aprantl updated this revision to Diff 82942.Jan 3 2017, 1:37 PM

Rebased patch on trunk and fixed the remaining test cases. This is now good to review :-)

dblaikie added inline comments.Jan 3 2017, 2:01 PM
docs/SourceLevelDebugging.rst
452

Probably could omit the reference to linkonce_odr and let the rest of the definition stand.

That way if anything were to duplicate other globals (eg: thinlto might import globals, etc) they could/would use this functionality as well, potentially.

456–458

Is this true? Do we go and change the DIGlobalVariable if we collapse an llvm::GlobalValue into a constant? So that it doesn't have a unit field anymore? I assume we don't do that, so some constants might point to DIGlobalVariables with a non-null unit field.

460–461

Is this correct? If all DIGlobalVariables referenced by DIImportedEntities can have no unit field (& thus be emitted into any unit that references them) - should any DIGlobalVariables have unit fields?

aprantl added inline comments.Jan 3 2017, 4:18 PM
docs/SourceLevelDebugging.rst
452

If the compile unit the global variable gets emitted into doesn't matter (such as a linkonce_odr symbol), ...

456–458

Good catch, I should implement this.

460–461

We added the unit field to DIGlobalVariables definitions (hanging off a global's !dbg attachment) so we can efficiently determine where to emit it. It's not strictly necessary, but it mirrors the DISubprogram design.

A pure declaration doesn't need one (and having one would prevent uniquing the DIImportedEntity) because it will be emitted as a locationless declaration in the CU it is being imported into (which should always be a correct one).

aprantl updated this revision to Diff 92213.Mar 17 2017, 3:26 PM
aprantl marked 6 inline comments as done.

Rebased on trunk and marked outstanding review comments as done.

dblaikie added inline comments.Mar 18 2017, 12:16 PM
docs/SourceLevelDebugging.rst
456–458

Does this happen now? Where should I look for the code and test case?

lib/CodeGen/AsmPrinter/DwarfDebug.cpp
499–504

This is a rather deeply nested data structure and I'm not sure the type aliases do enough to help improve readability. I wonder if it'd be worth writing a small class with named functions, some structs (rather than several std::pairs), etc. At least some more detailed commentary about the structure and invariants of this data structure. (eg: I'm a bit confused by how the contents of the GVMapType and WorklistType relate, or don't)

For example the GVMapType and WorklistType probably deserve a type rather than only std::pair - since insertIntoWorklist uses both seems they're sort of connected-ish.

502

Not sure if this is the best terminology here. At least my thought when I see "worklist" is an algorithm that starts with a list of things to process and the act of processing those things can add new things to the worklist. That doesn't seem to be the case here

541

Would be marginally more efficient not to add an element to UnitGVs that will never be used/retrieved/queried. Maybe not worth the added verbosity, though.

548

llvm::none_of?

549

I'd generally suggest using [&] rather than explicit captures if the lambda doesn't outlive its scope.

dexonsmith resigned from this revision.Oct 17 2020, 6:12 AM

Getting this off my queue since dblaikie is already providing feedback.