This is an archive of the discontinued LLVM Phabricator instance.

[CodeView] omit forward references for unnamed structs and unions
ClosedPublic

Authored by bwyma on Apr 25 2017, 11:04 AM.

Details

Summary

When a forward reference is emitted, Visual Studio uses the name to match the forward reference to the definition. For unnamed structs and unions there is no name to perform this matching. Instead, references to unnamed types are expected to refer to the complete type definition.

The changes to CodeViewDebug omit the forward declaration for unnamed types.
The test DebugInfo/COFF/unnamed.ll was added to verify the correct emission of unnamed types.
The test DebugInfo/COFF/bitfields.ll needed an update because the source contains an unnamed struct.

Diff Detail

Event Timeline

bwyma created this revision.Apr 25 2017, 11:04 AM
aaboud added a subscriber: llvm-commits.
rnk edited edge metadata.May 2 2017, 6:29 PM

Isn't this problematic if the unnamed struct has methods? That's the usual way you get cycles in the type graph.

If this only applies to plain C structs, I would rather start inventing "unique" names by hashing the source location in some way and then continuing to emit forward decls. That solves problems with named types in anonymous namespaces, which we currently don't give unique names.

bwyma added a comment.May 8 2017, 5:38 AM

Unnamed C++ structs are given a name as of rL274401, so unnamed structs containing a method will have a name assigned to them and will not create a cycle in this case.

If you feel strongly about naming these then I can improvise a name as you suggested, but I believe the current proposal has two minor advantages:

  1. Visual Studio omits the forward reference for these types and compatibility is generally a good thing.
  2. Omitting the forward reference and eliminating the manufactured name will use less space, and keeping the debug information size small whenever possible is desirable.

If you are willing to accept this suggestion but are still concerned about potential cycles in the type graph, then I can add additional code to prevent them.

Let me know what you think.

rnk added a comment.May 12 2017, 2:59 PM

I think I've convinced myself that it's impossible to create a cycle in the C type graph. I'll sleep better if you check for an empty identifier in addition to an empty name on the DICompositeType. That is essentially a proxy for "is this a C++ type" because all C++ types have identifiers, which are effectively mangled names.

However, it's still possible to construct a .ll file that has a cycle by taking a C++ example with a cycle (consider any unnamed struct with a method) and removing the name and identifier. We should add that as an LLVM test and make sure we gracefully detect it and error out. Calling either report_fatal_error or the LLVMContext dianostic handler would be fine, and should be testable in lit.

bwyma added a comment.May 18 2017, 5:10 AM

Thank you Reid. I'll make the changes you requested and upload a new patch. Much appreciated.

bwyma updated this revision to Diff 128941.Jan 8 2018, 8:48 AM

I made the changes you requested. Specifically:

  • I check for an empty identifier in addition to an empty name on the composite type.
  • I updated the code to call report_fatal_error when an unexpected cycle occurs in the type graph.
  • I added a lit test containing a cycle in the type graph to make sure the error is caught.
rnk accepted this revision.Jan 17 2018, 10:43 AM

lgtm with one nit, thanks!

lib/CodeGen/AsmPrinter/CodeViewDebug.cpp
1846–1847 ↗(On Diff #128941)

Can you wrap this condition (which is duplicated below) into a helper, something like shouldAlwaysEmitCompleteClassType?

This revision is now accepted and ready to land.Jan 17 2018, 10:43 AM
This revision was automatically updated to reflect the committed changes.
bwyma reopened this revision.Apr 9 2018, 5:32 AM

This patch was reverted in rL327414 due a build bot failure.
It depends upon a fix for C++ lambdas which hasn't landed yet. Once that patch lands, this one will be reapplied exactly as seen here.

This revision is now accepted and ready to land.Apr 9 2018, 5:32 AM
bwyma updated this revision to Diff 150666.Jun 10 2018, 6:31 PM

Update the tests to use retainedNodes instead of variables following the changes made in rL331841.

Closed by commit rL334382 (authored by bwyma). · Explain WhyJun 10 2018, 6:44 PM
This revision was automatically updated to reflect the committed changes.