This is an archive of the discontinued LLVM Phabricator instance.

Reland "[codeview] Reference types in type parent scopes"
ClosedPublic

Authored by akhuang on Apr 15 2020, 3:28 PM.

Details

Summary

Original description (https://reviews.llvm/org/D69924)
Without this change, when a nested tag type of any kind (enum, class,
struct, union) is used as a variable type, it is emitted without
emitting the parent type. In CodeView, parent types point to their inner
types, and inner types do not point back to their parents. We already
walk over all of the parent scopes to build the fully qualified name.
This change simply requests their type indices as we go along to enusre
they are all emitted.

Now, while walking over the parent scopes, check whether the type is already
in the process of being emitted before requesting its type index, to avoid
trying to record the same type index twice.

Fixes PR43905

Diff Detail

Event Timeline

akhuang created this revision.Apr 15 2020, 3:28 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 15 2020, 3:28 PM
rnk added inline comments.Apr 15 2020, 4:03 PM
llvm/lib/CodeGen/AsmPrinter/CodeViewDebug.cpp
323

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.

325–327

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.

akhuang updated this revision to Diff 257910.Apr 15 2020, 5:16 PM
akhuang marked an inline comment as done.

Add test case that errors without this check

akhuang updated this revision to Diff 257915.Apr 15 2020, 5:52 PM
akhuang marked an inline comment as done.

Change to add type to DeferredCompleteTypes

rnk accepted this revision.Apr 15 2020, 5:54 PM

lgtm, thanks!

This revision is now accepted and ready to land.Apr 15 2020, 5:54 PM
akhuang added inline comments.Apr 15 2020, 5:55 PM
llvm/lib/CodeGen/AsmPrinter/CodeViewDebug.cpp
323

Yep, not sure why I didn't add a test before

325–327

Oh, ok. Using DeferredCompleteTypes looks good.

akhuang updated this revision to Diff 257916.Apr 15 2020, 5:57 PM

remove stray whitespace

amccarth added inline comments.Apr 16 2020, 8:29 AM
llvm/lib/CodeGen/AsmPrinter/CodeViewDebug.cpp
336

What's the advantage of making this file-scope static function into a method? It doesn't seem to access any member variables.

If you just want to group it with related methods, can it be class-static or at least const?

Making this one non-static seems to ripple through the next two methods as well.

rnk added inline comments.Apr 16 2020, 9:59 AM
llvm/lib/CodeGen/AsmPrinter/CodeViewDebug.cpp
336

I think you're right, this one doesn't need to be a method, it can remain a static helper. The other ones call collectParentScopes which does access a member now.

akhuang updated this revision to Diff 258114.Apr 16 2020, 11:42 AM

change formatNestedName back to a static function.

amccarth accepted this revision.Apr 16 2020, 11:44 AM

Thanks!

This revision was automatically updated to reflect the committed changes.