[DebugInfo][CodeView] Fix lowering of UDT

[aganea]

Summary

It seems my previous changes uncovered an issue:

See build log here: http://lab.llvm.org:8011/builders/sanitizer-x86_64-linux-fast/builds/41191/steps/check-llvm%20msan/logs/stdio
And here: http://lab.llvm.org:8011/builders/sanitizer-x86_64-linux-bootstrap/builds/17119/steps/check-llvm%20asan/logs/stdio

What happens is that emitDebugInfoForGlobal calls getFullyQualifiedName, which in turn calls CodeViewDebug::collectParentScopeNames and then, DeferredCompleteTypes.push_back(Ty);

This leaves deferred types in the vector, but they are only 'applied' later, inadvertently by emitDebugInfoForUDTs while calling getCompleteTypeIndex which ends up destroying a TypeLoweringScope which applies the deferred types, which in turn creates new UDT, which resizes the vector being iterated in the first place in emitDebugInfoForUDTs.

I've added an extra TypeLoweringScope in getFullyQualifiedName() below, which should only fire if there are no other TypeLoweringScope in the stack (which is the case for emitDebugInfoForGlobal.

[aganea]

llvm/lib/CodeGen/AsmPrinter/CodeViewDebug.cpp

363

The actual change is this line.

[rnk]

Fix looks good.

I think we might benefit from splitting this class into CodeViewDebug/CodeViewTypes. Then we could clearly mark all the public entry points into type lowering, and establish the invariant that all public APIs create a type lowering scope. This bug arose because that invariant is unstated, and we added a new call into getFullyQualifiedName, which happens to trigger some type lowering. That's future work outside of the scope of this patch, though.

llvm/lib/CodeGen/AsmPrinter/CodeViewDebug.cpp

2995

It seems like there is still the possibility that getting the complete type index triggers some more type lowering which finds a new UDT, invalidating the iterators. I think it's a bug if this call ever triggers additional type lowering, because that could discover new UDTs (typedefs). I think that's impossible after your change: if all calls to addToUDTs are inside type lowering scopes, this should not happen.

To make this bug easier to find in the future (i.e. not require asan), could you please add an assertion that the size of the UDTs vector doesn't change during iteration? It will probably require changing the parameter type from ArrayRef to SmallVectorImpl, but otherwise it should be a minor change.

[aganea]

I've merged this back with D79447, I think it's better if it landed in one piece.

llvm/lib/CodeGen/AsmPrinter/CodeViewDebug.cpp

2995

Please see D79447.

llvm/lib/CodeGen/AsmPrinter/CodeViewDebug.cpp

for (StringRef QualifiedNameComponent :
        llvm::reverse(QualifiedNameComponents)) {
     FullyQualifiedName.append(std::string(QualifiedNameComponent));
     FullyQualifiedName.append("::");
   }
   FullyQualifiedName.append(std::string(TypeName));
   return FullyQualifiedName;
 }
 
-std::string CodeViewDebug::getFullyQualifiedName(const DIScope *Scope,
-                                                 StringRef Name) {
-  SmallVector<StringRef, 5> QualifiedNameComponents;
-  collectParentScopeNames(Scope, QualifiedNameComponents);
-  return formatNestedName(QualifiedNameComponents, Name);
-}
-
-std::string CodeViewDebug::getFullyQualifiedName(const DIScope *Ty) {
-  const DIScope *Scope = Ty->getScope();
-  return getFullyQualifiedName(Scope, getPrettyScopeName(Ty));
-}
-
 struct CodeViewDebug::TypeLoweringScope {
   TypeLoweringScope(CodeViewDebug &CVD) : CVD(CVD) { ++CVD.TypeEmissionLevel; }
   ~TypeLoweringScope() {
     // Don't decrement TypeEmissionLevel until after emitting deferred types, so
     // inner TypeLoweringScopes don't attempt to emit deferred types.
     if (CVD.TypeEmissionLevel == 1)
       CVD.emitDeferredCompleteTypes();
     --CVD.TypeEmissionLevel;
   }
   CodeViewDebug &CVD;
 };
 
+std::string CodeViewDebug::getFullyQualifiedName(const DIScope *Scope,
+                                                 StringRef Name) {
+  // Ensure types in the scope chain are emitted as soon as possible.
+  // This can create otherwise a situation where S_UDTs are emitted while
+  // looping in emitDebugInfoForUDTs.
+  TypeLoweringScope S(*this);
+  SmallVector<StringRef, 5> QualifiedNameComponents;
+  collectParentScopeNames(Scope, QualifiedNameComponents);
+  return formatNestedName(QualifiedNameComponents, Name);
+}
+
+std::string CodeViewDebug::getFullyQualifiedName(const DIScope *Ty) {
+  const DIScope *Scope = Ty->getScope();
+  return getFullyQualifiedName(Scope, getPrettyScopeName(Ty));
+}
+
 TypeIndex CodeViewDebug::getScopeIndex(const DIScope *Scope) {
   // No scope means global scope and that uses the zero index.
   if (!Scope || isa<DIFile>(Scope))
     return TypeIndex();
 
   assert(!isa<DIType>(Scope) && "shouldn't make a namespace scope for a type");
 
   // Check if we've already translated this scope.

 void CodeViewDebug::emitDebugInfoForUDTs(
     ArrayRef<std::pair<std::string, const DIType *>> UDTs) {
   for (const auto &UDT : UDTs) {
     const DIType *T = UDT.second;
     assert(shouldEmitUdt(T));
 
     MCSymbol *UDTRecordEnd = beginSymbolRecord(SymbolKind::S_UDT);
     OS.AddComment("Type");
     OS.emitInt32(getCompleteTypeIndex(T).getIndex());
     emitNullTerminatedSymbolName(OS, UDT.first);
     endSymbolRecord(UDTRecordEnd);
   }
 }
 
 void CodeViewDebug::collectGlobalVariableInfo() {
   DenseMap<const DIGlobalVariableExpression *, const GlobalVariable *>
       GlobalMap;