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

Tue Dec 14 11:35:33 PST 2021

dexonsmith added a comment.

In D104827#3189622 <https://reviews.llvm.org/D104827#3189622>, @scott.linder wrote:

> I'm pushing the (rough) patch which tries to make the switch from inheriting
> DIArglist from MDNode to ReplaceableMetadataImpl.
>
> The biggest issue I see with a change along these lines is that the
> `!DIArgList` syntax becomes misleading.

I don't see what's misleading about it. `!` is the indicator that "metadata follows", and `DIArgList` is the name of this special kind of metadata.

TBH, I find it odd that there'd be something inheriting from `MDNode` that is as special as this....

> If we really want this, I think it
> implies a bit more work, namely generalizing this to something closer to a
> `ValuesAsMetadata` (plural), with a syntax closer to:
>
>   metadata {i32 1, i64 %x, float %f}
>
> I think this doesn't add any ambiguity to the syntax, but this is just my
> initial guess at what might make sense.

This looks awkwardly similar to generic MDNode syntax (just with the distinction that generic metadata is not allowed to reference local variables). I don't think generic syntax is better than naming the type here. If you still disagree, can you explain why?

> I'm not sure I can commit to this work, but I would still like to propose making
> the UNIQUED/DISTINCT change for DIExpression and DICompileUnit. Would it be
> reasonable for me to just remove DIArgList from my original patch, as it has
> proven to be special enough not to generalize how I had hoped?

IIUC, `DIExpression` is data-driven, so it's reasonable for it always to be uniqued. Originally it was not going to inherit from MDNode, but IIRC it was useful as an incremental step for the transition from all generic metadata. At some point there was a FIXME in the code to reparent directly under Metadata but probably that was dropped eventually. I still think that'd be a reasonable thing to do.

If `DICompileUnit` already has this property, seems to me like that part could be separated into an NFC change to make it easier to review what's actually changing.

Seems reasonable to handle `DIArgList` separately from the other two. It'd be a shame to drop it entirely; it seems like there are some fundamental modelling problems with DIArgList that would be valuable to iron out.

================
Comment at: llvm/lib/Bitcode/Reader/MetadataLoader.cpp:559
   /// Upgrade the expression from previous versions.
-  Error upgradeDIExpression(uint64_t FromVersion,
+  Error upgradeDIExpression(uint64_t FromVersion, bool &IsDistinct,
                             MutableArrayRef<uint64_t> &Expr,
----------------
I don't think you need to change `upgradeDIExpression`.

================
Comment at: llvm/lib/Bitcode/Reader/MetadataLoader.cpp:1144

   bool IsDistinct = false;
   auto getMD = [&](unsigned ID) -> Metadata * {
----------------
Seems odd that this is declared here. Would be nice at some point to make it local to each `case` below that needs it in an NFC change.

================
Comment at: llvm/lib/Bitcode/Reader/MetadataLoader.cpp:2003

     IsDistinct = Record[0] & 1;
     uint64_t Version = Record[0] >> 1;
----------------
I think you should replace this line with a comment, maybe something like:
```
lang=c++
// DIExpression is never distinct. Old bitcode uses the 0-bit for that
// but it's now ignored.
```
You can drop the assignment to `IsDistinct` entirely; it's not interesting here anymore.

================
Comment at: llvm/lib/Bitcode/Reader/MetadataLoader.cpp:2008-2011
+    if (Error Err = upgradeDIExpression(Version, IsDistinct, Elts, Buffer))
       return Err;
+    if (IsDistinct)
+      return error("Invalid record");
----------------
I don't think you need this change.

================
Comment at: llvm/lib/Bitcode/Writer/BitcodeWriter.cpp:2019
   Record.reserve(N->getElements().size() + 1);
-  const uint64_t Version = 3 << 1;
+  const uint64_t Version = 4 << 1;
   Record.push_back((uint64_t)N->isDistinct() | Version);
----------------
I'm not sure we need a new version.

================
Comment at: llvm/lib/CodeGen/MIRParser/MIParser.cpp:1187
   } else if (Token.is(MIToken::md_diexpr)) {
+    // FIXME: This should be driven off of the UNIQUED property in Metadata.def
     if (parseDIExpression(Node))
----------------
I'm not sure what you mean by this comment. What is "this", and why should a metadata kind being uniqued-by-content influence it?

================
Comment at: llvm/lib/CodeGen/MIRParser/MIParser.cpp:2331
   } else if (Token.is(MIToken::md_diexpr)) {
+    // FIXME: This should be driven off of the UNIQUED property in Metadata.def
     if (parseDIExpression(Node))
----------------
I'm not sure what you mean by this comment. What is "this", and why should a metadata kind being uniqued-by-content influence it?

================
Comment at: llvm/lib/IR/AsmWriter.cpp:1232-1233

-  // Don't make slots for DIExpressions or DIArgLists. We just print them inline
-  // everywhere.
-  if (isa<DIExpression>(N) || isa<DIArgList>(N))
+  // Don't make slots for uniqued nodes or any DIArgLists. We just print them
+  // inline everywhere.
+#define HANDLE_MDNODE_LEAF_UNIQUED(CLASS)                                      \
----------------
I don't think nodes being uniqued-by-content should force them to be inlined everywhere. Those seem like independent properties to me.

================
Comment at: llvm/lib/IR/DebugInfoMetadata.cpp:1075
                                     StorageType Storage, bool ShouldCreate) {
+  assert(Storage != Distinct && "DIExpression cannot be distinct");
   DEFINE_GETIMPL_LOOKUP(DIExpression, (Elements));
----------------
Can this be `Storage == Uniqued`?

================
Comment at: llvm/lib/IR/Verifier.cpp:936
   Metadata *MD = MDV.getMetadata();
+
   if (auto *N = dyn_cast<MDNode>(MD)) {
----------------
Nit: if we add a newline here, should be in a separate NFC commit since there's no other change around.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104827