[Lldb-commits] [lldb] [lldb][test] Add test for ASTImporter's name conflict resolution (PR #112566)
Michael Buch via lldb-commits
lldb-commits at lists.llvm.org
Thu Oct 17 09:03:47 PDT 2024
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 <michaelbuch12 at gmail.com>
Date: Wed, 16 Oct 2024 15:48:47 +0100
Subject: [PATCH 1/5] [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 <michaelbuch12 at gmail.com>
Date: Wed, 16 Oct 2024 16:22:24 +0100
Subject: [PATCH 2/5] 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 <michaelbuch12 at gmail.com>
Date: Thu, 17 Oct 2024 16:25:14 +0100
Subject: [PATCH 3/5] 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 <michaelbuch12 at gmail.com>
Date: Thu, 17 Oct 2024 16:40:22 +0100
Subject: [PATCH 4/5] 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()
>From 18d5b5e3c3ba1c558a339a6fa4ba4b7e92566a42 Mon Sep 17 00:00:00 2001
From: Michael Buch <michaelbuch12 at gmail.com>
Date: Thu, 17 Oct 2024 17:03:35 +0100
Subject: [PATCH 5/5] fixup! python format
---
.../cpp/odr-handling-with-dylib/TestOdrHandlingWithDylib.py | 4 +++-
1 file changed, 3 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 6db23f637a1011..f67d933f6ae51a 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,7 +5,9 @@
class OdrHandlingWithDylibTestCase(TestBase):
- @skipIf(bugnumber="https://github.com/llvm/llvm-project/issues/50375, rdar://135551810")
+ @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
More information about the lldb-commits
mailing list