https://github.com/Michael137 created https://github.com/llvm/llvm-project/pull/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.). >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] [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 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
