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 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
