[-gmodules] Let LLDB log a warning if the Clang module hash mismatches.

[aprantl]

Summary

This feature is mostly there to aid debugging of Clang module issues, since the only useful actual the end-user can to is to recompile their program. Dsymutil prints a similar warning in this case.

[jingham]

LGTM

lldb/source/Host/common/Host.cpp

297

On macOS, SystemLog vsprintf's to stderr. So you probably don't want to put this out always. Maybe since you don't know where SystemLog is going to print things, it would be better to only output this if the log channel is set to verbose. That would still allow you to use it in tests, but wouldn't introduce any new output in the normal case?

[labath]

lldb/include/lldb/Symbol/SymbolFile.h

283–285 ↗

(On Diff #229400)

Can we remove this and put a cast in SymbolFileDWARF::UpdateExternalModuleListIfNeeded instead? It looks like that function should check that it has really found a dwarf file (and not a pdb for instance) anyway...

lldb/source/Host/common/Host.cpp

301

Are you sure it's legal to recycle the va_list this way? Should you maybe re-initialize it?

lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp

1570

DWARFDebugInfoEntry objects assume that they are living in one big vector and do pointer arithmetic on their this pointers. I don't think it's wise to copy them even if that happens to work in this case...

1579

With this implementation, the function will return the dwo id of the first skeleton unit in the main module in the dwo scenario. I think this is very unexpected and not very useful. I think this should check that the file contains a just a single compile unit, at least.

1724–1725

Maybe you could have GetDWOId return Optional<uint>. That way, the FIXME will be inside that function, and not in all of it's callers.

[aprantl]

lldb/include/lldb/Symbol/SymbolFile.h

283–285 ↗

(On Diff #229400)

Can we remove this and put a cast in SymbolFileDWARF::UpdateExternalModuleListIfNeeded instead?

We don't have a mechanism to dynamic-cast between SymbolFiles at the moment and I think it's difficult to add one, since it's a base class of an open-ended plugin hierarchy. We could require all plugins to register in a global enum if we don't care about extensibility. I also thought about each class identifying itself via a string that is its class name, but then we couldn't implement casting to a base-class easily.

If you have any idea for how to do this elegantly I'm more than happy to implement it.

It looks like that function should check that it has really found a dwarf file (and not a pdb for instance) anyway...

Technically, yes, but you'd need DWARF that encodes the path of a PDB to get into that situation, so I'm not super concerned about this.

[labath]

lldb/include/lldb/Symbol/SymbolFile.h

283–285 ↗

(On Diff #229400)

If you have any idea for how to do this elegantly I'm more than happy to implement it.

I know at least of two ways of doing that:

• the "old" way. This is pretty similar to your string approach and involves doing something like:

if (symfile->GetPluginName() == SymbolFileDWARF::GetPluginNameStatic())
  static_cast<SymbolFileDWARF*>(symfile)->stuff()

and probably doesn't require any extra coding (the presence of SymbolFileDWARFDebugMap makes things a bit complicated, but I don't think you need that here(?))

• for the new way, you can look at how we've implemented open-ended casting hierarchies in other plugins (last example here: D70070). This works with llvm::dyn_cast and everything. Not all plugins support that yet, but it is a direction we're going right now.

[aprantl]

lldb/source/Host/common/Host.cpp

297

Turns out LIBLLDB has no VERBOSE channel, and I can't add one because all 32 bits are already defined as channels. In the end this is probably not too bad — how many users are running with the host log enabled?

What do you think about making it either/or? It goes to the syslog by default and to the host log if it is enabled?

lldb/include/lldb/Utility/Log.h

public:
   void Warning(const char *fmt, ...) __attribute__((format(printf, 2, 3)));
 
   const Flags GetOptions() const;
 
   const Flags GetMask() const;
 
   bool GetVerbose() const;
 
-private:
   void VAPrintf(const char *format, va_list args);
   void VAError(const char *format, va_list args);
 
+private:
   Channel &m_channel;
 
   // The mutex makes sure enable/disable operations are thread-safe. The
   // options and mask variables are atomic to enable their reading in
   // Channel::GetLogIfAny without taking the mutex to speed up the fast path.
   // Their modification however, is still protected by this mutex.
   llvm::sys::RWMutex m_mutex;
 

lldb/packages/Python/lldbsuite/test/lang/objc/modules-hash-mismatch/Makefile

+OBJC_SOURCES := main.m
+CFLAGS_EXTRAS = -I$(BUILDDIR)
+USE_PRIVATE_MODULE_CACHE = YES
+
+.PHONY: update-module
+
+all: $(EXE)
+  $(MAKE) -f $(SRCDIR)/Makefile update-module
+
+include Makefile.rules
+
+update-module:
+  echo "forcing an update of f.pcm"
+  echo "typedef int something_other;" > $(BUILDDIR)/f.h
+  $(CC) $(CFLAGS) $(MANDATORY_MODULE_BUILD_CFLAGS) \
+    -c $(SRCDIR)/other.m -o $(BUILDDIR)/other.o

lldb/packages/Python/lldbsuite/test/lang/objc/modules-hash-mismatch/TestClangModulesHashMismatch.py

+from __future__ import print_function
+
+import unittest2
+import os
+import shutil
+
+import lldb
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import lldbutil
+
+
+class TestClangModuleHashMismatch(TestBase):
+    mydir = TestBase.compute_mydir(__file__)
+
+    def setUp(self):
+        TestBase.setUp(self)
+
+    @skipUnlessDarwin
+    @skipIf(debug_info=no_match(["gmodules"]))
+    def test_expr(self):
+        with open(self.getBuildArtifact("module.modulemap"), "w") as f:
+            f.write("""
+                    module Foo { header "f.h" }
+                    """)
+        with open(self.getBuildArtifact("f.h"), "w") as f:
+            f.write("""
+                    typedef int my_int;
+                    void f() {}
+                    """)
+
+        mod_cache = self.getBuildArtifact("private-module-cache")
+        if os.path.isdir(mod_cache):
+          shutil.rmtree(mod_cache)
+        self.build()
+        self.assertTrue(os.path.isdir(mod_cache), "module cache exists")
+
+        logfile = self.getBuildArtifact("host.log")
+        self.runCmd("log enable -v -f %s lldb host" % logfile)
+        target, _, _, _ = lldbutil.run_to_source_breakpoint(
+            self, "break here", lldb.SBFileSpec("main.m"))
+        target.GetModuleAtIndex(0).FindTypes('my_int') 
+
+        found = False
+        with open(logfile, 'r') as f:
+            for line in f:
+                if "hash mismatch" in line and "Foo" in line:
+                    found = True
+        self.assertTrue(found)

lldb/packages/Python/lldbsuite/test/lang/objc/modules-hash-mismatch/main.m

+#include "f.h"
+int main(int argc, char **argv) {
+  my_int i = argc;
+  f(); // break here
+  return 0;
+}

lldb/packages/Python/lldbsuite/test/lang/objc/modules-hash-mismatch/other.m

+#include "f.h"
+something_other f() {
+  return 0;
+}

lldb/packages/Python/lldbsuite/test/make/Makefile.rules

 # FRAMEWORK_MODULES := "module.modulemap"
 #
 # Also might be of interest:
 # FRAMEWORK_INCLUDES (Darwin only) :=
 # CFLAGS_EXTRAS :=
 # LD_EXTRAS :=
 # SPLIT_DEBUG_SYMBOLS := YES
 # CROSS_COMPILE :=
+# USE_PRIVATE_MODULE_CACHE := YES
 #
 # And test/functionalities/archives/Makefile:
 # MAKE_DSYM := NO
 # ARCHIVE_NAME := libfoo.a
 # ARCHIVE_C_SOURCES := a.c b.c
 
 # Uncomment line below for debugging shell commands
 # SHELL = /bin/sh -x

 else
   CFLAGS_NO_DEBUG = -O0 $(ARCHFLAG)$(ARCH) $(FRAMEWORK_INCLUDES) $(ARCH_CFLAGS) $(CFLAGS_EXTRAS)
 endif
 
 ifeq "$(MAKE_DWO)" "YES"
   CFLAGS += -gsplit-dwarf
 endif
 
-MODULE_BASE_FLAGS := -fmodules -gmodules -fmodules-cache-path=$(CLANG_MODULE_CACHE_DIR)
+ifeq "$(USE_PRIVATE_MODULE_CACHE)" "YES"
+THE_CLANG_MODULE_CACHE_DIR := $(BUILDDIR)/private-module-cache
+else
+THE_CLANG_MODULE_CACHE_DIR := $(CLANG_MODULE_CACHE_DIR)
+endif
+
+MODULE_BASE_FLAGS := -fmodules -gmodules -fmodules-cache-path=$(THE_CLANG_MODULE_CACHE_DIR)
 MANDATORY_MODULE_BUILD_CFLAGS := $(MODULE_BASE_FLAGS) -gmodules
 # Build flags for building with C++ modules.
 # -glldb is necessary for emitting information about what modules were imported.
 MANDATORY_CXXMODULE_BUILD_CFLAGS := $(MODULE_BASE_FLAGS) -fcxx-modules -glldb
 
 ifeq "$(OS)" "Darwin"
   MANDATORY_MODULE_BUILD_CFLAGS += -fcxx-modules
 endif

lldb/source/Host/common/Host.cpp

 
 void Host::SystemLog(SystemLogType type, const char *format, va_list args) {
   vfprintf(stderr, format, args);
 }
 
 #endif
 
 void Host::SystemLog(SystemLogType type, const char *format, ...) {
+  {
     va_list args;
     va_start(args, format);
     SystemLog(type, format, args);
     va_end(args);
   }
 
+  Log *log(GetLogIfAllCategoriesSet(LIBLLDB_LOG_HOST));
+  if (log && log->GetVerbose()) {
+    // Log to log channel. This allows testcases to grep for log output.
+    va_list args;
+    va_start(args, format);
+    log->VAPrintf(format, args);
+    va_end(args);
+  }
+}
+
 lldb::pid_t Host::GetCurrentProcessID() { return ::getpid(); }
 
 #ifndef _WIN32
 
 lldb::thread_t Host::GetCurrentThread() {
   return lldb::thread_t(pthread_self());
 }
 

lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.h

public:
 
   // For regular SymbolFileDWARF instances the method returns nullptr,
   // for the instances of the subclass SymbolFileDWARFDwo
   // the method returns a pointer to the base compile unit.
   virtual DWARFCompileUnit *GetBaseCompileUnit() { return nullptr; }
 
   virtual llvm::Optional<uint32_t> GetDwoNum() { return llvm::None; }
 
+  /// If this is a DWARF object with a single CU, return its DW_AT_dwo_id.
+  llvm::Optional<uint64_t> GetDWOId();
+
   static bool
   DIEInDeclContext(const lldb_private::CompilerDeclContext *parent_decl_ctx,
                    const DWARFDIE &die);
 
   std::vector<lldb_private::CallEdge>
   ParseCallEdgesInFunction(UserID func_id) override;
 
   void Dump(lldb_private::Stream &s) override;

lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp

SymbolFileDWARF::GetDIE(const DIERef &die_ref) {
 
   DWARFDebugInfo *debug_info = DebugInfo();
   if (debug_info)
     return debug_info->GetDIE(die_ref);
   else
     return DWARFDIE();
 }
 
+/// Return the DW_AT_(GNU_)dwo_name.
+static const char *GetDWOName(DWARFCompileUnit &dwarf_cu,
+                              const DWARFDebugInfoEntry &cu_die) {
+  const char *dwo_name =
+      cu_die.GetAttributeValueAsString(&dwarf_cu, DW_AT_GNU_dwo_name, nullptr);
+  if (!dwo_name)
+    dwo_name =
+        cu_die.GetAttributeValueAsString(&dwarf_cu, DW_AT_dwo_name, nullptr);
+  return dwo_name;
+}
+
+/// Return the DW_AT_(GNU_)dwo_id.
+/// FIXME: Technically 0 is a valid hash.
+static uint64_t GetDWOId(DWARFCompileUnit &dwarf_cu,
+                         const DWARFDebugInfoEntry &cu_die) {
+  uint64_t dwo_id =
+      cu_die.GetAttributeValueAsUnsigned(&dwarf_cu, DW_AT_GNU_dwo_id, 0);
+  if (!dwo_id)
+    dwo_id = cu_die.GetAttributeValueAsUnsigned(&dwarf_cu, DW_AT_dwo_id, 0);
+  return dwo_id;
+}
+
+llvm::Optional<uint64_t> SymbolFileDWARF::GetDWOId() {
+  if (GetNumCompileUnits() == 1) {
+    if (auto comp_unit = GetCompileUnitAtIndex(0))
+      if (DWARFCompileUnit *cu = llvm::dyn_cast_or_null<DWARFCompileUnit>(
+              GetDWARFCompileUnit(comp_unit.get())))
+        if (DWARFDebugInfoEntry *cu_die = cu->DIE().GetDIE())
+          if (uint64_t dwo_id = ::GetDWOId(*cu, *cu_die))
+            return dwo_id;
+  }
+  return {};
+}
+
 std::unique_ptr<SymbolFileDWARFDwo>
 SymbolFileDWARF::GetDwoSymbolFileForCompileUnit(
     DWARFUnit &unit, const DWARFDebugInfoEntry &cu_die) {
   // If this is a Darwin-style debug map (non-.dSYM) symbol file,
   // never attempt to load ELF-style DWO files since the -gmodules
   // support uses the same DWO machanism to specify full debug info
   // files for modules. This is handled in
   // UpdateExternalModuleListIfNeeded().
   if (GetDebugMapSymfile())
     return nullptr;
 
   DWARFCompileUnit *dwarf_cu = llvm::dyn_cast<DWARFCompileUnit>(&unit);
   // Only compile units can be split into two parts.
   if (!dwarf_cu)
     return nullptr;
 
-  const char *dwo_name =
-      cu_die.GetAttributeValueAsString(dwarf_cu, DW_AT_GNU_dwo_name, nullptr);
+  const char *dwo_name = GetDWOName(*dwarf_cu, cu_die);
   if (!dwo_name)
     return nullptr;
 
   SymbolFileDWARFDwp *dwp_symfile = GetDwpSymbolFile();
   if (dwp_symfile) {
-    uint64_t dwo_id =
-        cu_die.GetAttributeValueAsUnsigned(dwarf_cu, DW_AT_GNU_dwo_id, 0);
+    uint64_t dwo_id = ::GetDWOId(*dwarf_cu, cu_die);
     std::unique_ptr<SymbolFileDWARFDwo> dwo_symfile =
         dwp_symfile->GetSymbolFileForDwoId(*dwarf_cu, dwo_id);
     if (dwo_symfile)
       return dwo_symfile;
   }
 
   FileSpec dwo_file(dwo_name);
   FileSystem::Instance().Resolve(dwo_file);
SymbolFileDWARF::GetDwoSymbolFileForCompileUnit(
 
   return std::make_unique<SymbolFileDWARFDwo>(dwo_obj_file, *dwarf_cu);
 }
 
 void SymbolFileDWARF::UpdateExternalModuleListIfNeeded() {
   if (m_fetched_external_modules)
     return;
   m_fetched_external_modules = true;
-
   DWARFDebugInfo *debug_info = DebugInfo();
 
   // Follow DWO skeleton unit breadcrumbs.
   const uint32_t num_compile_units = GetNumCompileUnits();
   for (uint32_t cu_idx = 0; cu_idx < num_compile_units; ++cu_idx) {
-    DWARFUnit *dwarf_cu = debug_info->GetUnitAtIndex(cu_idx);
+    auto *dwarf_cu =
+        llvm::dyn_cast<DWARFCompileUnit>(debug_info->GetUnitAtIndex(cu_idx));
+    if (!dwarf_cu)
+      continue;
+
     const DWARFBaseDIE die = dwarf_cu->GetUnitDIEOnly();
-    if (!die || die.HasChildren())
+    if (!die || die.HasChildren() || !die.GetDIE())
       continue;
 
     const char *name = die.GetAttributeValueAsString(DW_AT_name, nullptr);
     if (!name)
       continue;
 
     ConstString const_name(name);
     ModuleSP &module_sp = m_external_type_modules[const_name];
     if (module_sp)
       continue;
 
-    const char *dwo_path =
-        die.GetAttributeValueAsString(DW_AT_GNU_dwo_name, nullptr);
-    if (!dwo_path)
-      dwo_path = die.GetAttributeValueAsString(DW_AT_dwo_name, nullptr);
+    const char *dwo_path = GetDWOName(*dwarf_cu, *die.GetDIE());
     if (!dwo_path)
       continue;
 
     ModuleSpec dwo_module_spec;
     dwo_module_spec.GetFileSpec().SetFile(dwo_path, FileSpec::Style::native);
     if (dwo_module_spec.GetFileSpec().IsRelative()) {
       const char *comp_dir =
           die.GetAttributeValueAsString(DW_AT_comp_dir, nullptr);
for (uint32_t cu_idx = 0; cu_idx < num_compile_units; ++cu_idx) {
     }
 
     Status error = ModuleList::GetSharedModule(dwo_module_spec, module_sp,
                                                nullptr, nullptr, nullptr);
     if (!module_sp) {
       GetObjectFile()->GetModule()->ReportWarning(
           "0x%8.8x: unable to locate module needed for external types: "
           "%s\nerror: %s\nDebugging will be degraded due to missing "
-          "types. Rebuilding your project will regenerate the needed "
+          "types. Rebuilding the project will regenerate the needed "
           "module files.",
           die.GetOffset(), dwo_module_spec.GetFileSpec().GetPath().c_str(),
           error.AsCString("unknown error"));
       continue;
     }
+
+    // Verify the DWO hash.
+    // FIXME: Technically "0" is a valid hash.
+    uint64_t dwo_id = ::GetDWOId(*dwarf_cu, *die.GetDIE());
+    if (!dwo_id)
+      continue;
+
+    auto *dwo_symfile =
+        llvm::dyn_cast_or_null<SymbolFileDWARF>(module_sp->GetSymbolFile());
+    if (!dwo_symfile)
+      continue;
+    llvm::Optional<uint64_t> dwo_dwo_id = dwo_symfile->GetDWOId();
+    if (!dwo_dwo_id)
+      continue;
+
+    if (dwo_id != dwo_dwo_id) {
+      GetObjectFile()->GetModule()->ReportWarning(
+          "0x%8.8x: Module %s is out-of-date (hash mismatch). Type information "
+          "from this module may be incomplete or inconsistent with the rest of "
+          "the program. Rebuilding the project will regenerate the needed "
+          "module files.",
+          die.GetOffset(), dwo_module_spec.GetFileSpec().GetPath().c_str());
+    }
   }
 }
 
 SymbolFileDWARF::GlobalVariableMap &SymbolFileDWARF::GetGlobalAranges() {
   if (!m_global_aranges_up) {
     m_global_aranges_up.reset(new GlobalVariableMap());
 
     ModuleSP module_sp = GetObjectFile()->GetModule();