This is an archive of the discontinued LLVM Phabricator instance.

[NativePDB] Improved support for nested type reconstruction
ClosedPublic

Authored by zturner on Nov 9 2018, 2:34 PM.

Details

Summary

In a previous patch, we pre-processed the TPI stream in order to build the reverse mapping from nested type -> parent type so that we could accurately reconstruct a DeclContext hierarchy.

However, there were some issues. An LF_NESTTYPE record is really just a typedef, so although it happens to be used to indicate the name of the nested type and referring to the global record which defines the type, it is also used for every other kind of nested typedef. When we rebuild the DeclContext hierarchy, we want it to be as accurate as possible, which means that if we have something like:

struct A {
  struct B {};
  using C = B;
};

We don't want to create two CXXRecordDecls in the AST each with the exact same definition. We just want to create one for B and then define C as an alias to B. Previously, however, it would not be able to distinguish between the two cases and it would treat A::B and A::C as being two classes each with separate definitions. We address the first half of improving the pre-processing logic so that only actual definitions are treated this way.

Later, in a followup patch, we can handle the case of nested typedefs since we're already going to be enumerating the field list anyway and this patch introduces the general framework for distinguishing between the two cases.

Diff Detail

Repository
rL LLVM

Event Timeline

zturner created this revision.Nov 9 2018, 2:34 PM

This change looks reasonable to me for solving the problem with the current LF_NESTTYPE approach. But I'm not sure... Now we all the same need to analyze mangled names and make a decision based on this. Does the current LF_NESTTYPE approach still has advantages over the "parse-scope" approach? I'm afraid that we will have to fully analyze a mangled name in the future for scoped types, so if the LF_NESTTYPE approach doesn't have a significant advantages over the "parse-scope" approach, do we really need to support them both?

lldb/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp
560–576 ↗(On Diff #173447)

May be it would be a good idea to make the demangler (or to implement a mangler) more flexible to hide these details there? I do not mean to do it exactly in this commit, but what do you think about this at all?

881–887 ↗(On Diff #173447)

Is this a clang only bug, or MSVC also does so? I do not have anything against this solution for now, I just mean, may be we'll replace it with an assert when the bug will be fixed?

aleksandr.urakov accepted this revision.Nov 12 2018, 7:52 AM
This revision is now accepted and ready to land.Nov 12 2018, 7:52 AM
This revision was automatically updated to reflect the committed changes.