Skip to content

Commit 3bc765d

Browse files
authored
[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.).
1 parent c89d731 commit 3bc765d

File tree

7 files changed

+104
-0
lines changed

7 files changed

+104
-0
lines changed
Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,6 @@
1+
CXX_SOURCES := main.cpp service.cpp
2+
3+
DYLIB_CXX_SOURCES := plugin.cpp
4+
DYLIB_NAME := plugin
5+
6+
include Makefile.rules
Lines changed: 29 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,29 @@
1+
import lldb
2+
from lldbsuite.test.decorators import *
3+
from lldbsuite.test.lldbtest import *
4+
from lldbsuite.test import lldbutil
5+
6+
7+
class OdrHandlingWithDylibTestCase(TestBase):
8+
@skipIf(
9+
bugnumber="https://github.com/llvm/llvm-project/issues/50375, rdar://135551810"
10+
)
11+
def test(self):
12+
"""
13+
Tests that the expression evaluator is able to deal with types
14+
whose definitions conflict across multiple LLDB modules (in this
15+
case the definition for 'class Service' in the main executable
16+
has an additional field compared to the definition found in the
17+
dylib). This causes the ASTImporter to detect a name conflict
18+
while importing 'Service'. With LLDB's liberal ODRHandlingType
19+
the ASTImporter happily creates a conflicting AST node for
20+
'Service' in the scratch ASTContext, leading to a crash down
21+
the line.
22+
"""
23+
self.build()
24+
25+
lldbutil.run_to_source_breakpoint(
26+
self, "plugin_entry", lldb.SBFileSpec("plugin.cpp")
27+
)
28+
29+
self.expect_expr("*gProxyThis")
Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,11 @@
1+
#include "plugin.h"
2+
3+
#define HIDE_FROM_PLUGIN 1
4+
#include "service.h"
5+
6+
int main() {
7+
exported();
8+
plugin_init();
9+
plugin_entry();
10+
return 0;
11+
}
Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,14 @@
1+
#include "plugin.h"
2+
#include "service.h"
3+
4+
struct Proxy : public Service {
5+
State *proxyState;
6+
};
7+
8+
Proxy *gProxyThis = 0;
9+
10+
extern "C" {
11+
void plugin_init() { gProxyThis = new Proxy; }
12+
13+
void plugin_entry() {}
14+
}
Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,9 @@
1+
#ifndef PLUGIN_H_IN
2+
#define PLUGIN_H_IN
3+
4+
extern "C" {
5+
void plugin_entry(void);
6+
void plugin_init(void);
7+
}
8+
9+
#endif // PLUGIN_H_IN
Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,15 @@
1+
#define HIDE_FROM_PLUGIN 1
2+
#include "service.h"
3+
4+
struct ServiceAux {
5+
Service *Owner;
6+
};
7+
8+
struct Service::State {};
9+
10+
void exported() {
11+
// Make sure debug-info for definition of Service is
12+
// emitted in this CU.
13+
Service service;
14+
service.start(0);
15+
}
Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,20 @@
1+
#ifndef SERVICE_H_IN
2+
#define SERVICE_H_IN
3+
4+
struct ServiceAux;
5+
6+
struct Service {
7+
struct State;
8+
bool start(State *) { return true; }
9+
10+
#ifdef HIDE_FROM_PLUGIN
11+
int __resv1;
12+
#endif // !HIDE_FROM_PLUGIN
13+
14+
Service *__owner;
15+
ServiceAux *aux;
16+
};
17+
18+
void exported();
19+
20+
#endif

0 commit comments

Comments
 (0)