[PATCH] D78249: Reland "[codeview] Reference types in type parent scopes"

[Amy Huang via Phabricator via llvm-commits]

akhuang added inline comments.


================
Comment at: llvm/lib/CodeGen/AsmPrinter/CodeViewDebug.cpp:323
+    // declaration or a complete type.
+    // Only call getTypeIndex if the type isn't already being defined.
+    if (const auto *Ty = dyn_cast<DICompositeType>(Scope)) {
----------------
rnk wrote:
> Can you add a test that fails if this check is removed? You should be able to reduce one out of the crash bug Hans filed.
Yep, not sure why I didn't add a test before 


================
Comment at: llvm/lib/CodeGen/AsmPrinter/CodeViewDebug.cpp:325-327
+      auto I = CompleteTypeIndices.find(Ty);
+      if (I == CompleteTypeIndices.end() || I->second != TypeIndex())
+        (void)getTypeIndex(Ty);
----------------
rnk wrote:
> As an alternative, I think we should be able to add this type to the DeferredCompleteTypes list. We should get the same behavior without the extra conditional.
> 
> All this type index lowering code is based on the idea that it is impossible to make a cycle in the type graph without looking through a complete struct type. i.e. you cannot have a type which points to itself. You can make a linked list that points to the struct, but that is implemented by pointing to a forward declaration.
Oh, ok. Using DeferredCompleteTypes looks good. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78249