[Lldb-commits] [lldb] 3bc765d - [lldb][test] Add test for ASTImporter's name conflict resolution (#112566)

[via lldb-commits]

Author: Michael Buch
Date: 2024-10-18T14:19:42+01:00
New Revision: 3bc765dbbf9bf0eceab1c9679b9f761b3f760d56

URL: https://github.com/llvm/llvm-project/commit/3bc765dbbf9bf0eceab1c9679b9f761b3f760d56
DIFF: https://github.com/llvm/llvm-project/commit/3bc765dbbf9bf0eceab1c9679b9f761b3f760d56.diff

LOG: [lldb][test] Add test for ASTImporter's name conflict resolution (#112566)

This is a reduced test case from a crash we've observed in the past. The
assertion that this test triggers is:
```
Assertion failed: ((Pos == ImportedDecls.end() || Pos->second == To) && "Try to import an already imported Decl"), function MapImported, file ASTImporter.cpp, line 10494.
```

In a non-asserts build we crash later on in the ASTImporter. The root
cause is, as the assertion above points out, that we erroneously replace
an existing `From->To` decl mapping with a `To` decl that isn't
complete. Then we try to complete it but it has no definition and we
dereference a nullptr.

The reason this happens is basically what's been described in
https://reviews.llvm.org/D67803?id=220956#1676588

The dylib contains a definition of `Service` which is different to the
one in the main executable. When we start dumping the children of the
variable we're printing, we start completing it's members,
`ASTImport`ing fields in the process. When the ASTImporter realizes
there's been a name conflict (i.e., a structural mismatch on the
`Service` type) it would usually report back an error. However, LLDB
uses `ODRHandlingType::Liberal`, which means we create a new decl for
the ODR'd type instead of re-using the previously mapped decl.
Eventually this leads us to crash.

Ideally we'd be using `ODRHandlingType::Conservative` and warn/error,
though LLDB relies on this in some cases (particularly for
distinguishing template specializations, though maybe there's better a
way to deal with those).

We should really warn the user when this happens and not crash. To avoid
the crash we'd need to know to not create a decl for the ODR violation,
and instead re-use the definition we've previously seen. Though I'm not
yet sure that's viable for all of LLDB's use-cases (where ODR violations
might legimiately occur in a program, e.g., with opaque definitions,
etc.).

Added: 
    lldb/test/API/lang/cpp/odr-handling-with-dylib/Makefile
    lldb/test/API/lang/cpp/odr-handling-with-dylib/TestOdrHandlingWithDylib.py
    lldb/test/API/lang/cpp/odr-handling-with-dylib/main.cpp
    lldb/test/API/lang/cpp/odr-handling-with-dylib/plugin.cpp
    lldb/test/API/lang/cpp/odr-handling-with-dylib/plugin.h
    lldb/test/API/lang/cpp/odr-handling-with-dylib/service.cpp
    lldb/test/API/lang/cpp/odr-handling-with-dylib/service.h

Modified: 
    

Removed: 
    


################################################################################
diff  --git a/lldb/test/API/lang/cpp/odr-handling-with-dylib/Makefile b/lldb/test/API/lang/cpp/odr-handling-with-dylib/Makefile
new file mode 100644
index 00000000000000..91eadaa37282ee
--- /dev/null
+++ b/lldb/test/API/lang/cpp/odr-handling-with-dylib/Makefile
@@ -0,0 +1,6 @@
+CXX_SOURCES := main.cpp service.cpp
+
+DYLIB_CXX_SOURCES := plugin.cpp
+DYLIB_NAME := plugin
+
+include Makefile.rules

diff  --git a/lldb/test/API/lang/cpp/odr-handling-with-dylib/TestOdrHandlingWithDylib.py b/lldb/test/API/lang/cpp/odr-handling-with-dylib/TestOdrHandlingWithDylib.py
new file mode 100644
index 00000000000000..f67d933f6ae51a
--- /dev/null
+++ b/lldb/test/API/lang/cpp/odr-handling-with-dylib/TestOdrHandlingWithDylib.py
@@ -0,0 +1,29 @@
+import lldb
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import lldbutil
+
+
+class OdrHandlingWithDylibTestCase(TestBase):
+    @skipIf(
+        bugnumber="https://github.com/llvm/llvm-project/issues/50375, rdar://135551810"
+    )
+    def test(self):
+        """
+        Tests that the expression evaluator is able to deal with types
+        whose definitions conflict across multiple LLDB modules (in this
+        case the definition for 'class Service' in the main executable
+        has an additional field compared to the definition found in the
+        dylib). This causes the ASTImporter to detect a name conflict
+        while importing 'Service'. With LLDB's liberal ODRHandlingType
+        the ASTImporter happily creates a conflicting AST node for
+        'Service' in the scratch ASTContext, leading to a crash down
+        the line.
+        """
+        self.build()
+
+        lldbutil.run_to_source_breakpoint(
+            self, "plugin_entry", lldb.SBFileSpec("plugin.cpp")
+        )
+
+        self.expect_expr("*gProxyThis")

diff  --git a/lldb/test/API/lang/cpp/odr-handling-with-dylib/main.cpp b/lldb/test/API/lang/cpp/odr-handling-with-dylib/main.cpp
new file mode 100644
index 00000000000000..f3372e0fbe7018
--- /dev/null
+++ b/lldb/test/API/lang/cpp/odr-handling-with-dylib/main.cpp
@@ -0,0 +1,11 @@
+#include "plugin.h"
+
+#define HIDE_FROM_PLUGIN 1
+#include "service.h"
+
+int main() {
+  exported();
+  plugin_init();
+  plugin_entry();
+  return 0;
+}

diff  --git a/lldb/test/API/lang/cpp/odr-handling-with-dylib/plugin.cpp b/lldb/test/API/lang/cpp/odr-handling-with-dylib/plugin.cpp
new file mode 100644
index 00000000000000..190388000a3c8f
--- /dev/null
+++ b/lldb/test/API/lang/cpp/odr-handling-with-dylib/plugin.cpp
@@ -0,0 +1,14 @@
+#include "plugin.h"
+#include "service.h"
+
+struct Proxy : public Service {
+  State *proxyState;
+};
+
+Proxy *gProxyThis = 0;
+
+extern "C" {
+void plugin_init() { gProxyThis = new Proxy; }
+
+void plugin_entry() {}
+}

diff  --git a/lldb/test/API/lang/cpp/odr-handling-with-dylib/plugin.h b/lldb/test/API/lang/cpp/odr-handling-with-dylib/plugin.h
new file mode 100644
index 00000000000000..9d4ba5df5a83a3
--- /dev/null
+++ b/lldb/test/API/lang/cpp/odr-handling-with-dylib/plugin.h
@@ -0,0 +1,9 @@
+#ifndef PLUGIN_H_IN
+#define PLUGIN_H_IN
+
+extern "C" {
+void plugin_entry(void);
+void plugin_init(void);
+}
+
+#endif // PLUGIN_H_IN

diff  --git a/lldb/test/API/lang/cpp/odr-handling-with-dylib/service.cpp b/lldb/test/API/lang/cpp/odr-handling-with-dylib/service.cpp
new file mode 100644
index 00000000000000..6302a45483495e
--- /dev/null
+++ b/lldb/test/API/lang/cpp/odr-handling-with-dylib/service.cpp
@@ -0,0 +1,15 @@
+#define HIDE_FROM_PLUGIN 1
+#include "service.h"
+
+struct ServiceAux {
+  Service *Owner;
+};
+
+struct Service::State {};
+
+void exported() {
+  // Make sure debug-info for definition of Service is
+  // emitted in this CU.
+  Service service;
+  service.start(0);
+}

diff  --git a/lldb/test/API/lang/cpp/odr-handling-with-dylib/service.h b/lldb/test/API/lang/cpp/odr-handling-with-dylib/service.h
new file mode 100644
index 00000000000000..37c6b9aeb2d9b8
--- /dev/null
+++ b/lldb/test/API/lang/cpp/odr-handling-with-dylib/service.h
@@ -0,0 +1,20 @@
+#ifndef SERVICE_H_IN
+#define SERVICE_H_IN
+
+struct ServiceAux;
+
+struct Service {
+  struct State;
+  bool start(State *) { return true; }
+
+#ifdef HIDE_FROM_PLUGIN
+  int __resv1;
+#endif // !HIDE_FROM_PLUGIN
+
+  Service *__owner;
+  ServiceAux *aux;
+};
+
+void exported();
+
+#endif