https://github.com/Michael137 updated https://github.com/llvm/llvm-project/pull/112566
>From d8369837a10f0e4750dd6c0f6c6b4467931b5012 Mon Sep 17 00:00:00 2001 From: Michael Buch <michaelbuc...@gmail.com> Date: Wed, 16 Oct 2024 15:48:47 +0100 Subject: [PATCH 1/4] [lldb][test] Add test for ASTImporter's name conflict resolution 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.). --- .../Expr/Inputs/name-conflict-test/main.cpp | 21 +++++++++++++++++++ .../Expr/Inputs/name-conflict-test/plugin.cpp | 15 +++++++++++++ .../Expr/Inputs/name-conflict-test/plugin.h | 10 +++++++++ .../Inputs/name-conflict-test/service.cpp | 15 +++++++++++++ .../Expr/Inputs/name-conflict-test/service.h | 20 ++++++++++++++++++ .../Shell/Expr/TestODRHandlingWithDylib.test | 17 +++++++++++++++ 6 files changed, 98 insertions(+) create mode 100644 lldb/test/Shell/Expr/Inputs/name-conflict-test/main.cpp create mode 100644 lldb/test/Shell/Expr/Inputs/name-conflict-test/plugin.cpp create mode 100644 lldb/test/Shell/Expr/Inputs/name-conflict-test/plugin.h create mode 100644 lldb/test/Shell/Expr/Inputs/name-conflict-test/service.cpp create mode 100644 lldb/test/Shell/Expr/Inputs/name-conflict-test/service.h create mode 100644 lldb/test/Shell/Expr/TestODRHandlingWithDylib.test diff --git a/lldb/test/Shell/Expr/Inputs/name-conflict-test/main.cpp b/lldb/test/Shell/Expr/Inputs/name-conflict-test/main.cpp new file mode 100644 index 00000000000000..74d40d04ed4e20 --- /dev/null +++ b/lldb/test/Shell/Expr/Inputs/name-conflict-test/main.cpp @@ -0,0 +1,21 @@ +#include "service.h" +#include <cassert> +#include <dlfcn.h> + +#ifndef PLUGIN_PATH +#error "Expected PLUGIN_PATH to be defined" +#endif // !PLUGIN_PATH + +int main() { + void *handle = dlopen(PLUGIN_PATH, RTLD_NOW); + assert(handle != nullptr); + void (*plugin_init)(void) = (void (*)(void))dlsym(handle, "plugin_init"); + assert(plugin_init != nullptr); + void (*plugin_entry)(void) = (void (*)(void))dlsym(handle, "plugin_entry"); + assert(plugin_entry != nullptr); + + exported(); + plugin_init(); + plugin_entry(); + return 0; +} diff --git a/lldb/test/Shell/Expr/Inputs/name-conflict-test/plugin.cpp b/lldb/test/Shell/Expr/Inputs/name-conflict-test/plugin.cpp new file mode 100644 index 00000000000000..8f8e98ed5c4b01 --- /dev/null +++ b/lldb/test/Shell/Expr/Inputs/name-conflict-test/plugin.cpp @@ -0,0 +1,15 @@ +#include "service.h" +#include "plugin.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/Shell/Expr/Inputs/name-conflict-test/plugin.h b/lldb/test/Shell/Expr/Inputs/name-conflict-test/plugin.h new file mode 100644 index 00000000000000..a04b0974165e71 --- /dev/null +++ b/lldb/test/Shell/Expr/Inputs/name-conflict-test/plugin.h @@ -0,0 +1,10 @@ +#ifndef PLUGIN_H_IN +#define PLUGIN_H_IN + +extern "C" { +void plugin_entry(void); +void plugin_init(void); +} + +#endif // _H_IN + diff --git a/lldb/test/Shell/Expr/Inputs/name-conflict-test/service.cpp b/lldb/test/Shell/Expr/Inputs/name-conflict-test/service.cpp new file mode 100644 index 00000000000000..5fd008c2aa8b80 --- /dev/null +++ b/lldb/test/Shell/Expr/Inputs/name-conflict-test/service.cpp @@ -0,0 +1,15 @@ +#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/Shell/Expr/Inputs/name-conflict-test/service.h b/lldb/test/Shell/Expr/Inputs/name-conflict-test/service.h new file mode 100644 index 00000000000000..53762a6e00357a --- /dev/null +++ b/lldb/test/Shell/Expr/Inputs/name-conflict-test/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; } + +#if HIDE_FROM_PLUGIN + int __resv1; +#endif // !HIDE_FROM_PLUGIN + + Service *__owner; + ServiceAux *aux; +}; + +void exported(); + +#endif diff --git a/lldb/test/Shell/Expr/TestODRHandlingWithDylib.test b/lldb/test/Shell/Expr/TestODRHandlingWithDylib.test new file mode 100644 index 00000000000000..7ee34ff4431d8b --- /dev/null +++ b/lldb/test/Shell/Expr/TestODRHandlingWithDylib.test @@ -0,0 +1,17 @@ +# REQUIRES: system-darwin + +# RUN: %clangxx_host %p/Inputs/name-conflict-test/plugin.cpp -shared -gdwarf -O0 -o %t.plugin.dylib +# +# RUN: %clangxx_host %p/Inputs/name-conflict-test/main.cpp \ +# RUN: %p/Inputs/name-conflict-test/service.cpp \ +# RUN: -DPLUGIN_PATH=\"%t.plugin.dylib\" \ +# RUN: -DHIDE_FROM_PLUGIN \ +# RUN: -gdwarf -O0 -o %t.a.out +# +# RUN: %lldb %t.a.out \ +# RUN: -o "b plugin_entry" \ +# RUN: -o run \ +# RUN: -o "expr *gProxyThis" \ +# RUN: -o exit | FileCheck %s + +# CHECK: (lldb) expr *gProxyThis >From 207912749aca18d08e137f4f61cd942d330d9b85 Mon Sep 17 00:00:00 2001 From: Michael Buch <michaelbuc...@gmail.com> Date: Wed, 16 Oct 2024 16:22:24 +0100 Subject: [PATCH 2/4] fixup! clang-format --- lldb/test/Shell/Expr/Inputs/name-conflict-test/plugin.cpp | 3 +-- lldb/test/Shell/Expr/Inputs/name-conflict-test/plugin.h | 1 - lldb/test/Shell/Expr/Inputs/name-conflict-test/service.cpp | 1 - 3 files changed, 1 insertion(+), 4 deletions(-) diff --git a/lldb/test/Shell/Expr/Inputs/name-conflict-test/plugin.cpp b/lldb/test/Shell/Expr/Inputs/name-conflict-test/plugin.cpp index 8f8e98ed5c4b01..190388000a3c8f 100644 --- a/lldb/test/Shell/Expr/Inputs/name-conflict-test/plugin.cpp +++ b/lldb/test/Shell/Expr/Inputs/name-conflict-test/plugin.cpp @@ -1,5 +1,5 @@ -#include "service.h" #include "plugin.h" +#include "service.h" struct Proxy : public Service { State *proxyState; @@ -12,4 +12,3 @@ void plugin_init() { gProxyThis = new Proxy; } void plugin_entry() {} } - diff --git a/lldb/test/Shell/Expr/Inputs/name-conflict-test/plugin.h b/lldb/test/Shell/Expr/Inputs/name-conflict-test/plugin.h index a04b0974165e71..66053f4e755e7c 100644 --- a/lldb/test/Shell/Expr/Inputs/name-conflict-test/plugin.h +++ b/lldb/test/Shell/Expr/Inputs/name-conflict-test/plugin.h @@ -7,4 +7,3 @@ void plugin_init(void); } #endif // _H_IN - diff --git a/lldb/test/Shell/Expr/Inputs/name-conflict-test/service.cpp b/lldb/test/Shell/Expr/Inputs/name-conflict-test/service.cpp index 5fd008c2aa8b80..9a65e33d8ab29b 100644 --- a/lldb/test/Shell/Expr/Inputs/name-conflict-test/service.cpp +++ b/lldb/test/Shell/Expr/Inputs/name-conflict-test/service.cpp @@ -12,4 +12,3 @@ void exported() { Service service; service.start(0); } - >From f0adb9e5b9c947d62719a2ae9a2328b5a41515ca Mon Sep 17 00:00:00 2001 From: Michael Buch <michaelbuc...@gmail.com> Date: Thu, 17 Oct 2024 16:25:14 +0100 Subject: [PATCH 3/4] fixup! turn test into API test; no need for dlopen/dlsym --- .../lang/cpp/odr-handling-with-dylib/Makefile | 6 ++++++ .../TestOdrHandlingWithDylib.py | 18 ++++++++++++++++ .../lang/cpp/odr-handling-with-dylib/main.cpp | 11 ++++++++++ .../cpp/odr-handling-with-dylib}/plugin.cpp | 0 .../cpp/odr-handling-with-dylib}/plugin.h | 0 .../cpp/odr-handling-with-dylib}/service.cpp | 1 + .../cpp/odr-handling-with-dylib}/service.h | 2 +- .../Expr/Inputs/name-conflict-test/main.cpp | 21 ------------------- .../Shell/Expr/TestODRHandlingWithDylib.test | 17 --------------- 9 files changed, 37 insertions(+), 39 deletions(-) create mode 100644 lldb/test/API/lang/cpp/odr-handling-with-dylib/Makefile create mode 100644 lldb/test/API/lang/cpp/odr-handling-with-dylib/TestOdrHandlingWithDylib.py create mode 100644 lldb/test/API/lang/cpp/odr-handling-with-dylib/main.cpp rename lldb/test/{Shell/Expr/Inputs/name-conflict-test => API/lang/cpp/odr-handling-with-dylib}/plugin.cpp (100%) rename lldb/test/{Shell/Expr/Inputs/name-conflict-test => API/lang/cpp/odr-handling-with-dylib}/plugin.h (100%) rename lldb/test/{Shell/Expr/Inputs/name-conflict-test => API/lang/cpp/odr-handling-with-dylib}/service.cpp (89%) rename lldb/test/{Shell/Expr/Inputs/name-conflict-test => API/lang/cpp/odr-handling-with-dylib}/service.h (91%) delete mode 100644 lldb/test/Shell/Expr/Inputs/name-conflict-test/main.cpp delete mode 100644 lldb/test/Shell/Expr/TestODRHandlingWithDylib.test 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..c39a160fae2b3d --- /dev/null +++ b/lldb/test/API/lang/cpp/odr-handling-with-dylib/TestOdrHandlingWithDylib.py @@ -0,0 +1,18 @@ +import lldb +from lldbsuite.test.decorators import * +from lldbsuite.test.lldbtest import * +from lldbsuite.test import lldbutil + + +class OdrHandlingWithDylibTestCase(TestBase): + def test(self): + """ + TODO + """ + 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/Shell/Expr/Inputs/name-conflict-test/plugin.cpp b/lldb/test/API/lang/cpp/odr-handling-with-dylib/plugin.cpp similarity index 100% rename from lldb/test/Shell/Expr/Inputs/name-conflict-test/plugin.cpp rename to lldb/test/API/lang/cpp/odr-handling-with-dylib/plugin.cpp diff --git a/lldb/test/Shell/Expr/Inputs/name-conflict-test/plugin.h b/lldb/test/API/lang/cpp/odr-handling-with-dylib/plugin.h similarity index 100% rename from lldb/test/Shell/Expr/Inputs/name-conflict-test/plugin.h rename to lldb/test/API/lang/cpp/odr-handling-with-dylib/plugin.h diff --git a/lldb/test/Shell/Expr/Inputs/name-conflict-test/service.cpp b/lldb/test/API/lang/cpp/odr-handling-with-dylib/service.cpp similarity index 89% rename from lldb/test/Shell/Expr/Inputs/name-conflict-test/service.cpp rename to lldb/test/API/lang/cpp/odr-handling-with-dylib/service.cpp index 9a65e33d8ab29b..6302a45483495e 100644 --- a/lldb/test/Shell/Expr/Inputs/name-conflict-test/service.cpp +++ b/lldb/test/API/lang/cpp/odr-handling-with-dylib/service.cpp @@ -1,3 +1,4 @@ +#define HIDE_FROM_PLUGIN 1 #include "service.h" struct ServiceAux { diff --git a/lldb/test/Shell/Expr/Inputs/name-conflict-test/service.h b/lldb/test/API/lang/cpp/odr-handling-with-dylib/service.h similarity index 91% rename from lldb/test/Shell/Expr/Inputs/name-conflict-test/service.h rename to lldb/test/API/lang/cpp/odr-handling-with-dylib/service.h index 53762a6e00357a..37c6b9aeb2d9b8 100644 --- a/lldb/test/Shell/Expr/Inputs/name-conflict-test/service.h +++ b/lldb/test/API/lang/cpp/odr-handling-with-dylib/service.h @@ -7,7 +7,7 @@ struct Service { struct State; bool start(State *) { return true; } -#if HIDE_FROM_PLUGIN +#ifdef HIDE_FROM_PLUGIN int __resv1; #endif // !HIDE_FROM_PLUGIN diff --git a/lldb/test/Shell/Expr/Inputs/name-conflict-test/main.cpp b/lldb/test/Shell/Expr/Inputs/name-conflict-test/main.cpp deleted file mode 100644 index 74d40d04ed4e20..00000000000000 --- a/lldb/test/Shell/Expr/Inputs/name-conflict-test/main.cpp +++ /dev/null @@ -1,21 +0,0 @@ -#include "service.h" -#include <cassert> -#include <dlfcn.h> - -#ifndef PLUGIN_PATH -#error "Expected PLUGIN_PATH to be defined" -#endif // !PLUGIN_PATH - -int main() { - void *handle = dlopen(PLUGIN_PATH, RTLD_NOW); - assert(handle != nullptr); - void (*plugin_init)(void) = (void (*)(void))dlsym(handle, "plugin_init"); - assert(plugin_init != nullptr); - void (*plugin_entry)(void) = (void (*)(void))dlsym(handle, "plugin_entry"); - assert(plugin_entry != nullptr); - - exported(); - plugin_init(); - plugin_entry(); - return 0; -} diff --git a/lldb/test/Shell/Expr/TestODRHandlingWithDylib.test b/lldb/test/Shell/Expr/TestODRHandlingWithDylib.test deleted file mode 100644 index 7ee34ff4431d8b..00000000000000 --- a/lldb/test/Shell/Expr/TestODRHandlingWithDylib.test +++ /dev/null @@ -1,17 +0,0 @@ -# REQUIRES: system-darwin - -# RUN: %clangxx_host %p/Inputs/name-conflict-test/plugin.cpp -shared -gdwarf -O0 -o %t.plugin.dylib -# -# RUN: %clangxx_host %p/Inputs/name-conflict-test/main.cpp \ -# RUN: %p/Inputs/name-conflict-test/service.cpp \ -# RUN: -DPLUGIN_PATH=\"%t.plugin.dylib\" \ -# RUN: -DHIDE_FROM_PLUGIN \ -# RUN: -gdwarf -O0 -o %t.a.out -# -# RUN: %lldb %t.a.out \ -# RUN: -o "b plugin_entry" \ -# RUN: -o run \ -# RUN: -o "expr *gProxyThis" \ -# RUN: -o exit | FileCheck %s - -# CHECK: (lldb) expr *gProxyThis >From 110b0d6ba90be4041a880a9a9963019850782e75 Mon Sep 17 00:00:00 2001 From: Michael Buch <michaelbuc...@gmail.com> Date: Thu, 17 Oct 2024 16:40:22 +0100 Subject: [PATCH 4/4] fixup! skip for now --- .../TestOdrHandlingWithDylib.py | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) 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 index c39a160fae2b3d..6db23f637a1011 100644 --- a/lldb/test/API/lang/cpp/odr-handling-with-dylib/TestOdrHandlingWithDylib.py +++ b/lldb/test/API/lang/cpp/odr-handling-with-dylib/TestOdrHandlingWithDylib.py @@ -5,9 +5,18 @@ class OdrHandlingWithDylibTestCase(TestBase): + @skipIf(bugnumber="https://github.com/llvm/llvm-project/issues/50375, rdar://135551810") def test(self): """ - TODO + 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() _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
