[PATCH] D79512: [DebugInfo][CodeView] Fix lowering of UDT

Thu May 7 10:14:21 PDT 2020

rnk added a comment.

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.

================
Comment at: llvm/lib/CodeGen/AsmPrinter/CodeViewDebug.cpp:2995
     OS.AddComment("Type");
     OS.emitInt32(getCompleteTypeIndex(T).getIndex());
     emitNullTerminatedSymbolName(OS, UDT.first);
----------------
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.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79512