[Lldb-commits] [lldb] [LLDB][NativePDB] Set IsDynmaicCXXType metadata for records (PR #155853)

[Michael Buch via lldb-commits]

https://github.com/Michael137 commented:

Mostly reasonable. This patch does deviate slightly from how `IsPossibleDynamicType` behaves for DWARF. There we also mark a type dynamic if it has virtual bases (even without any virtual functions). We use this to determine the dynamic type of a derived class, for which the Itanium ABI runtime plugin uses the vtable (which exists for virtual bases too).

Re. testing, since the metadata is just an optimization, an end-to-end test that checks whether we set the metadata is tricky. Is it possible to use the Itanium ABI with PDB? If so, you could check whether `frame variable` will correctly show you the derived type when printing a pointer to a base class. I think that would go through the Itanium ABI runtime plugin. Since we currently always set it to false, I would expect that to fail. But pass with your new patch.

You could also write a targeted unit-test that creates a `CXXRecordDecl` (using `yaml2pdb` to generate the debug-info), and then check that the metadata is set correctly. `DWARFASTParserClang` has examples of tests using YAML to generate debug-info. Maybe you don't even need the `yaml2pdb`, and can hand-construct a record with the UdtRecordCompleter (not sure how convenient it is to instantiate one in a test though). `TestClangASTImporter.cpp` has examples of where we check `ClangASTMetadata` from custom record decls. 

If this ends up being overly tedious i think this change is simple enough to be landed as is. But would be nice to give testing a shot.

https://github.com/llvm/llvm-project/pull/155853