This is an archive of the discontinued LLVM Phabricator instance.

[DebugInfo][CodeView] Fix lowering of UDT
AbandonedPublic

Authored by aganea on May 6 2020, 12:11 PM.

Details

Reviewers
rnk
akhuang
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.

Diff Detail

Event Timeline

aganea created this revision.May 6 2020, 12:11 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 6 2020, 12:11 PM
aganea marked an inline comment as done.May 6 2020, 12:11 PM
aganea added inline comments.
llvm/lib/CodeGen/AsmPrinter/CodeViewDebug.cpp
363

The actual change is this line.

aganea edited the summary of this revision. (Show Details)May 6 2020, 12:12 PM
rnk added a comment.May 7 2020, 9:54 AM

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 abandoned this revision.May 14 2020, 11:46 AM
aganea marked 2 inline comments as done.

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.