[Lldb-commits] [PATCH] D75488: Preserve the owning module information from DWARF in the synthesized AST

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

This sounds like it could be useful though I can't say I really no much about modules.

I appreciate the effort in splitting this patch up, but I am wondering if we couldn't do this is one more step. Would it be possible to split the TypeSystem changes into a patch of its own (with some TypeSystemClang unit tests), and then have separate a patch where SymbolFileDWARF starts making use of this functionality? I think that would make easier to follow what's going in the two subsystems/plugins as well as reduce some of the noise caused by the interface changes.



================
Comment at: lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp:224
+    if (auto *rd = llvm::dyn_cast<clang::RecordDecl>(tag_decl))
+      for (auto *f : rd->fields())
+        TypeSystemClang::SetOwningModule(f, owning_module);
----------------
Maybe spell out the type here? I have no idea what's the type of this..


================
Comment at: lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.h:57-78
+  /// The Layout is as follows:
+  /// \verbatim
+  /// bit 0..30 ... Owning Module ID.
+  /// bit 31 ...... IsCompleteObjCClass.
+  /// \endverbatim
   uint32_t &m_payload;
 public:
----------------
Maybe it would be simpler to have a struct full of bitfields and then just memcpy it to/from the uint32_t in the constructor/destructor?


================
Comment at: lldb/test/Shell/SymbolFile/DWARF/Inputs/ModuleOwnership/A.h:24
+extern template struct InNamespace<int>;
+}
----------------
How about a case where A.h defines a type which references a type from B.h (e.g. contains it as a member variable) ?


================
Comment at: lldb/test/Shell/SymbolFile/DWARF/module-ownership.mm:1
+// RUN: %clang_host -g -gmodules -fmodules -fmodules-cache-path=%t.cache \
+// RUN:    -c -o %t.o %s -I%S/Inputs
----------------
I think this test will fail on windows. At least it did fail for me when I manually forced a windows target here (linux was fine though). I think the problem is that lldb does not know how to open an unlinked COFF file.

I think it would be better to hardcode a triple here instead of using %clang_host.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D75488/new/

https://reviews.llvm.org/D75488