Page MenuHomePhabricator

[DebugInfo][CodeView] Fix lowering of UDT

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



It seems my previous changes uncovered an issue:

See build log here:
And here:

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.Wed, May 6, 12:11 PM
Herald added a project: Restricted Project. · View Herald TranscriptWed, May 6, 12:11 PM
aganea marked an inline comment as done.Wed, May 6, 12:11 PM
aganea added inline comments.

The actual change is this line.

aganea edited the summary of this revision. (Show Details)Wed, May 6, 12:12 PM
rnk added a comment.Thu, May 7, 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.


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.Thu, May 14, 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.


Please see D79447.