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

Thu Nov 18 12:16:15 PST 2021

scott.linder added a comment.

In D104827#3138627 <https://reviews.llvm.org/D104827#3138627>, @dexonsmith wrote:

> In D104827#3137807 <https://reviews.llvm.org/D104827#3137807>, @scott.linder wrote:
>
>>> Is this moving in the right direction? Does the restriction on DIArgList seem
>>> reasonable, and if so does it need more explicit documentation or more thorough
>>> enforcement?
>>
>> I pushed this before looking at the intervening comments, so the last bit is a bit redundant; it sounds like the restriction on DIArgList was intended and makes sense, so I think the remaining question is whether the Verifier check is sufficient.
>
> It seems like `DIArgList` is very similar to LocalAsMetadata. I feel like it'd be less error-prone for it to use/inherit-from `ReplaceableMetadataImpl`, just like ValueAsMetadata does. Then it has the machinery to RAUW/delete itself when necessary.
>
> (Moreover, given that it doesn't have any operands, I find it suspicious that it inherits (albeit indirectly) from `MDNode`. It should probably be a direct subclass of `Metadata`. This would mean it couldn't inherit from `DINode` which might complicate some debug info code; if you're going to clean that up I suggest doing it later as a separate patch.)

I looked into deriving `DIArgList` from `ReplaceableMetadataImpl`, and it does get awkward quickly that it is an `MDNode`. It seems like there is a fair amount of code around tracking/RAUW which relies on `MDNode`s being eventually "resolveable". The result is that I don't think I can inherit from `ReplaceableMetadataImpl` without also changing the inheritence from `MDNode` to `Metadata` as you suggest.

I'll look at what effect that has on any debug-info related code, but I do worry that no matter what there will be a fair amount of special-casing.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104827