[all-commits] [llvm/llvm-project] 3bc765: [lldb][test] Add test for ASTImporter's name confl...

[Michael Buch via All-commits]

  Branch: refs/heads/main
  Home:   https://github.com/llvm/llvm-project
  Commit: 3bc765dbbf9bf0eceab1c9679b9f761b3f760d56
      https://github.com/llvm/llvm-project/commit/3bc765dbbf9bf0eceab1c9679b9f761b3f760d56
  Author: Michael Buch <michaelbuch12 at gmail.com>
  Date:   2024-10-18 (Fri, 18 Oct 2024)

  Changed paths:
    A lldb/test/API/lang/cpp/odr-handling-with-dylib/Makefile
    A lldb/test/API/lang/cpp/odr-handling-with-dylib/TestOdrHandlingWithDylib.py
    A lldb/test/API/lang/cpp/odr-handling-with-dylib/main.cpp
    A lldb/test/API/lang/cpp/odr-handling-with-dylib/plugin.cpp
    A lldb/test/API/lang/cpp/odr-handling-with-dylib/plugin.h
    A lldb/test/API/lang/cpp/odr-handling-with-dylib/service.cpp
    A lldb/test/API/lang/cpp/odr-handling-with-dylib/service.h

  Log Message:
  -----------
  [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.).



To unsubscribe from these emails, change your notification settings at https://github.com/llvm/llvm-project/settings/notifications