[PATCH] D104827: [DebugInfo] Enforce implicit constraints on `distinct` MDNodes

[Scott Linder via Phabricator via llvm-commits]

scott.linder added inline comments.


================
Comment at: llvm/lib/IR/DebugInfoMetadata.cpp:1668-1673
+  auto *Uniqued = uniquify();
+  if (Uniqued != this) {
+    auto &Store = getContext().pImpl->MetadataAsValues;
+    auto MAV = Store.find(this);
+    if (MAV != Store.end())
+      MAV->second->handleChangedMetadata(Uniqued);
----------------
dexonsmith wrote:
> I'm sorry; I'm lacking some context here; I'd appreciate a bit of hand-holding.
> 
> I see how this updates the MetadataAsValue reference.
> 
> Can you walk me through in way way it's enforced that DIArgList is only referenced by MetadataAsValue? It sounds like textual IR has something, but can you confirm? What about the C++ API? The verifier? And what about historical bitcode... i.e., what can be found there?
> - If there are holes in enforcement somehow, what's going to happen to the dangling references to this node?
> - Can/should we delete this node now? And/or somehow mark it as dead, so that ValueAsMetadata will look through it?
> Can you walk me through in way way it's enforced that DIArgList is only referenced by MetadataAsValue?

I don't know that it is enforced currently, but I believe it was an intended restriction. The LLVM IR langref entry for [[ https://llvm.org/docs/LangRef.html#diarglist | DIArgList  ]] currently says (emphasis mine):

> Because a DIArgList may refer to local values within a function, **it must only be used as a function argument**, must always be inlined, and cannot appear in named metadata.

The only way I can interpret the bold portion is to make it equivalent to "it must only be used through a MetadataAsValue", as that is the only means of using metadata as a function argument.

> It sounds like textual IR has something, but can you confirm? What about the C++ API? The verifier? And what about historical bitcode... i.e., what can be found there?

Would it be appropriate to just add as a verifier check? No matter how the IR is produced it would then fail to verify, so we would cover any current or future APIs. For bitcode I've implemented an upgrade path to force `IsDistinct=false` when loading old bitcode.

> If there are holes in enforcement somehow, what's going to happen to the dangling references to this node?
> Can/should we delete this node now? And/or somehow mark it as dead, so that ValueAsMetadata will look through it?

I need to look closer at the lifetime of metadata nodes, it seems clear there is more to do here I'm just not sure exactly what that is.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D104827/new/

https://reviews.llvm.org/D104827