Page MenuHomePhabricator

IR: Allow metadata attachments on declarations, and fix lazy loaded metadata issue with globals.
ClosedPublic

Authored by pcc on Jun 6 2016, 6:48 PM.

Details

Summary

This change is motivated by an upcoming change to the metadata representation
used for CFI. The indirect function call checker needs type information for
external function declarations in order to correctly generate jump table
entries for such declarations. We currently associate such type information
with declarations using a global metadata node, but I plan [1] to move all
such metadata to global object attachments.

In bitcode, metadata attachments for function declarations appear in the
global metadata block. This seems reasonable to me because I expect metadata
attachments on declarations to be uncommon. In the long term I'd also expect
this to be the case for CFI, because we'd want to use some specialized bitcode
format for this metadata that could be read as part of the ThinLTO thin-link
phase, which would mean that it would not appear in the global metadata block.

To solve the lazy loaded metadata issue I was seeing with D20147, I use the
same bitcode representation for metadata attachments for global variables as I
do for function declarations. Since there's a use case for metadata attachments
in the global metadata block, we might as well use that representation for
global variables as well, at least until we have a mechanism for lazy loading
global variables.

In the assembly format, the metadata attachments appear after the "declare"
keyword in order to avoid a parsing ambiguity. I could make the same change
for function definition attachments if that seems reasonable.

[1] http://lists.llvm.org/pipermail/llvm-dev/2016-June/100462.html

Diff Detail

Repository
rL LLVM

Event Timeline

pcc updated this revision to Diff 59817.Jun 6 2016, 6:48 PM
pcc retitled this revision from to IR: Allow metadata attachments on declarations, and fix lazy loaded metadata issue with globals..
pcc updated this object.
pcc added reviewers: aprantl, dexonsmith, mehdi_amini.
pcc added a subscriber: llvm-commits.
pcc added a subscriber: eugenis.Jun 7 2016, 11:46 AM
pcc added a comment.Jun 14 2016, 11:13 AM

Ping. Note that D20147 is blocked on this.

dexonsmith edited edge metadata.Jun 14 2016, 1:36 PM
dexonsmith added a subscriber: dexonsmith.

Sorry for the delay. At a high-level, here are my concerns (which you may have already addressed: I haven't looked at the patch yet):

  • !dbg attachments on declarations should be disallowed (I suggest the verifier?).
  • PGO attachments on declarations should probably also be disallowed?
  • Along the same lines, I think having two !dbg (or PGO) attachments on the same function should be disallowed. I can imagine loosening this restriction in the future (with lots more work) but I think right now it would cause a bug.
  • Lazy-loading needs to be fast.

I won't have time to review this in detail for at least a few days, but I'm also happy to defer to others.

aprantl edited edge metadata.Jun 14 2016, 1:57 PM

The code itself looks fine to me.

lib/Bitcode/Writer/ValueEnumerator.cpp
351 ↗(On Diff #59817)

I'm just curious: Is this being addressed in one of the follow-up reviews? If not, can you please file a PR about it?

pcc updated this revision to Diff 60780.Jun 14 2016, 4:50 PM
pcc marked an inline comment as done.
pcc edited edge metadata.
  • Forbid !dbg and !prof attachments on declarations
  • Add PR references
pcc added a comment.Jun 14 2016, 4:50 PM

!dbg attachments on declarations should be disallowed (I suggest the verifier?).
PGO attachments on declarations should probably also be disallowed?

Done.

Along the same lines, I think having two !dbg (or PGO) attachments on the same function should be disallowed. I can imagine loosening this restriction in the future (with lots more work) but I think right now it would cause a bug.

Was already done for !dbg as part of r271358. I've committed a similar change for !prof in r272734.

Lazy-loading needs to be fast.

This change along with D20147 should not represent a performance regression in regard to global variable debug metadata. Global variable debug metadata will continue to be stored in the global metadata block and will continue to be materialized upon calling materializeMetadata.

lib/Bitcode/Writer/ValueEnumerator.cpp
351 ↗(On Diff #59817)

Filed PR28134.

Not sure if this is waiting for me, but you answered all my concerns. LGTM.

This revision was automatically updated to reflect the committed changes.