[Lldb-commits] [clang] [lldb] [ASTImporter][lldb] Avoid implicit imports in VisitFieldDecl (PR #107828)

[Andrew Savonichev via lldb-commits]

https://github.com/asavonic created 
https://github.com/llvm/llvm-project/pull/107828

Fields from external sources are typically loaded implicitly by `RecordDecl` or 
`DeclContext` iterators and other functions (see LoadFieldsFromExternalStorage 
function and its uses). The assumption is that we only need to load such fields 
whenever a complete list of fields is necessary. However, there are cases where 
implicit loads are not expected and they may break AST importer logic.

In ASTNodeImporter::VisitFieldDecl:

  1) We first check that a field is not imported already.
  2) Then proceed to import it.
  3) Finally add it to the record with `setLexicalDeclContext` and 
`addDeclInternal`.

If an implicit import happens between (2) and (3), it may indirectly bring the 
same field into the context. When (3) happens we add it again, duplicating the 
field and breaking the record. This is not detected until we crash later during 
layout computation:

llvm/clang/lib/AST/RecordLayoutBuilder.cpp:81
Assertion `FieldOffsets.count(FD) && "Field does not have an external 
offset"' failed.

Detecting a possible duplication is difficult, especially considering that 
`addDeclInternal` may cause an implicit import as well.

The patch attempts to workaround this problem by triggering implicit imports 
before (1). However, it is hard to tell if it covers all the cases, because 
some of them are nested: `DeclContext::addHiddenDecl` calls 
`CXXRecordDecl::addedMember`, which calls `Type::isLiteralType` on a base type, 
which tries to iterate over fields and cause an implicit load.

It is quite tricky to get a reproducer for such problems, because they depend 
on order of imports of fields and records. Debugging Unreal Engine v5.3.2 with 
LLDB shows this problem once issue #90938 is fixed or workarounded. Only some 
UE classes are affected. Reducing it down to a LIT test is problematic due to 
size of libraries involved, and "flaky" nature of the problem.

TestCppReferenceToOuterClass shows an improvement with the patch, but it likely 
has no relation to the problem.

>From ee3a6a5b4eef069c5cef2820aeab5ab773df69a4 Mon Sep 17 00:00:00 2001
From: Andrew Savonichev 
Date: Mon, 9 Sep 2024 17:56:20 +0900
Subject: [PATCH] [ASTImporter][lldb] Avoid implicit imports in VisitFieldDecl

Fields from external sources are typically loaded implicitly by
`RecordDecl` or `DeclContext` iterators and other functions (see
LoadFieldsFromExternalStorage function and its uses). The assumption
is that we only need to load such fields whenever a complete list of
fields is necessary. However, there are cases where implicit loads are
not expected and they may break AST importer logic.

In ASTNodeImporter::VisitFieldDecl:

  1) We first check that a field is not imported already.
  2) Then proceed to import it.
  3) Finally add it to the record with `setLexicalDeclContext` and 
`addDeclInternal`.

If an implicit import happens between (2) and (3), it may indirectly
bring the same field into the context. When (3) happens we add it
again, duplicating the field and breaking the record. This is not
detected until we crash later during layout computation:

  llvm/clang/lib/AST/RecordLayoutBuilder.cpp:81
  Assertion `FieldOffsets.count(FD) && "Field does not have an external 
offset"' failed.

Detecting a possible duplication is difficult, especially considering
that `addDeclInternal` may cause an implicit import as well.

The patch attempts to workaround this problem by triggering implicit
imports before (1). However, it is hard to tell if it covers all the
cases, because some of them are nested: `DeclContext::addHiddenDecl`
calls `CXXRecordDecl::addedMember`, which calls `Type::isLiteralType`
on a base type, which tries to iterate over fields and cause an
implicit load.

It is quite tricky to get a reproducer for such problems, because they
depend on order of imports of fields and records. Debugging Unreal
Engine v5.3.2 with LLDB shows this problem once issue #90938 is fixed
or workarounded. Only some UE classes are affected. Reducing it down
to a LIT test is problematic due to size of libraries involved, and
"flaky" nature of the problem.

TestCppReferenceToOuterClass shows an improvement with the patch, but
it likely has no relation to the problem.
---
 clang/lib/AST/ASTImporter.cpp | 22 +--
 .../TestCppReferenceToOuterClass.py   |  1 -
 2 files changed, 20 insertions(+), 3 deletions(-)

diff --git a/clang/lib/AST/ASTImporter.cpp b/clang/lib/AST/ASTImporter.cpp
index c2fb7dddcfc637..6e48f21c57d9d6 100644
--- a/clang/lib/AST/ASTImporter.cpp
+++ b/clang/lib/AST/ASTImporter.cpp
@@ -4172,6 +4172,26 @@ ExpectedDecl ASTNodeImporter::VisitFieldDecl(FieldDecl 
*D) {
   if (ToD)
 return ToD;
 
+  // Implicit imports of external fields may import the same field
+  // *after* we check for its presence with findDeclsInToCtx. If this
+  // happens we may import the field twice and break the record
+  // type.
+  //
+

[Lldb-commits] [clang] [lldb] [ASTImporter][lldb] Avoid implicit imports in VisitFieldDecl (PR #107828)

[Andrew Savonichev via lldb-commits]

asavonic wrote:

Thanks Michael, I'm glad I'm not the only one seeing this problem.

> > If an implicit import happens between (2) and (3), it may indirectly bring 
> > the same field into the context. When (3) happens we add it again, 
> > duplicating the field and breaking the record. This is not detected until 
> > we crash later during layout computation:
> 
> And the LLDB test that was XFAILed, is a special case of exactly this problem 
> (summarized well in https://reviews.llvm.org/D102993).

Right, I saw this patch too. It tries to workaround the problem in LLDB, but 
seem to cause other issues. For example:
```
clang/lib/AST/ASTImporter.cpp:1933:
 Error clang::ASTNodeImporter::ImportDeclContext(DeclContext *, bool):
 Assertion `ToDC == ToD->getLexicalDeclContext() && 
ToDC->containsDecl(ToD)' failed.
```

> The more complete solution here is to make LLDB stop relying on this sort of 
> import-pattern. We've been told in the past that the ASTImporter just wasn't 
> designed for this use-case. Ideally, we'd remove the need for minimally 
> importing record types (instead we should just always use `IDK_Everything` 
> for importing definitions). In my opinion, adding complexity like this into 
> the ASTImporter just to support LLDB is going to make it a maintenance burden 
> on both. So I'd vote for trying to solve this properly (which is actually 
> something I've been working on, though I can't promise a timeline of when 
> this is supposed to land).

Oh, I think a redesign with this problem in mind would be great. 

As I understand it, minimal import is used in LLDB for performance reasons, so 
we don't waste time parsing and loading AST elements that we don't need. It 
sounds like a fine idea in general. Implicit imports of external sources in 
Clang, however, turn import process into a minefield, where you have no idea if 
a "pure" operation can cause a major change the target AST. "Complete" types 
are really incomplete, and there is no single point at which they are all 
guaranteed to be finalized.

Perhaps this approach was taken to minimize impact of external sources on the 
rest of Clang, to avoid introducing a concept of "not-fully-loaded-AST" 
everywhere. Understandable, but we have these issues now.

I don't know if I see the whole picture here. I might be wrong on some of these 
points or reasons behind them.

I'm curious what your plan is to address this problem.

> I do wonder whether there's something about the build configuration that 
> makes you hit this crash more likely in UE. Are you building with precompiled 
> headers by any chance? Or with `-flimit-debug-info`?

Unfortunately, I don't know. I'm using pre-built binaries downloaded from Epic.
PCHs are likely used, not sure about `-flimit-debug-info`.

> I'm actually looking at a similar crash at the moment for which I have a 
> reproducer. I'm trying to extract it into a smaller test-case.

Please let me know if you can get it. 

I reduced mine down to ~200KB of DWARF. It crashes on the same assertion in 
`RecordLayoutBuilder.cpp`, but this patch no longer helps. Perhaps there is 
another case of the issue, or the reduced DWARF is somehow invalid.

https://github.com/llvm/llvm-project/pull/107828
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [clang] [lldb] [ASTImporter][lldb] Avoid implicit imports in VisitFieldDecl (PR #107828)

[Andrew Savonichev via lldb-commits]

asavonic wrote:

> Our idea is summarized in 
> https://discourse.llvm.org/t/rfc-lldb-more-reliable-completion-of-record-types/77442.
>  Basically the goal is to guarantee that a call to `getDefinition`, _will_ 
> fetch the definition. This is something that Clang already does, but we just 
> never implement (via `CompleteRedeclChain`). You're right that the "minimal 
> import" mode was implemented for perf. reasons. Though we should probably 
> revisit this. I think we can make LLDB use non-minimal import for record 
> types while keeping the number of transitive imports contained. It would 
> still require changes to the importer to be smarter about what we import, but 
> we wouldn't have these "complete but incomplete" types floating around.

Thanks a lot Michael. It's a lot of work to get the redesign ready, considering 
both functional and performance requirements.

In the meantime, should we merge this patch to fix at least some current 
problems?


https://github.com/llvm/llvm-project/pull/107828
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [lldb] Clear ModuleList shared modules in SBDebugger::Clear (PR #147289)

[Andrew Savonichev via lldb-commits]

https://github.com/asavonic created 
https://github.com/llvm/llvm-project/pull/147289

Shared modules are stored in a global `ModuleList` cache, and it is 
intentionally leaked to avoid doing cleanup when lldb exits.

However, when lldb is used as a library, we need a way to manage opened modules 
to avoid problems with file locks (on some systems) for modules that we no 
longer need.

It should be possible to record all loaded modules and use 
`ModuleList::RemoveSharedModule` and `RemoveOrphanSharedModules` functions to 
clear the cache, but these functions are not available in the API. This 
approach is also way too complicated when we just need to cleanup the library.

The patch adds the following:

  - `ModuleList::ClearSharedModules` function to clear all shared modules.

  - `SBDebugger::SetClearSharedModules(bool)` API to enable clearing during 
`SBDebugger::Clear()`.

  - `settings set clear-shared-modules true` command line option that is 
equivalent to `SetClearSharedModules(true)`.

This new behaviour is turned off by default: the debugger does not release 
these shared modules until the process exits.

>From 0737ec086d765dc59c01750de924891af6de77b2 Mon Sep 17 00:00:00 2001
From: Andrew Savonichev 
Date: Mon, 7 Jul 2025 17:33:50 +0900
Subject: [PATCH] [lldb] Clear ModuleList shared modules in SBDebugger::Clear

Shared modules are stored in a global ModuleList cache, and it is
intentionally leaked to avoid doing cleanup when lldb exits.

However, when lldb is used as a library, we need a way to manage
opened modules to avoid problems with file locks (on some systems) for
modules that we no longer need.

It should be possible to record all loaded modules and use
ModuleList::RemoveSharedModule and RemoveOrphanSharedModules functions
to clear the cache, but these functions are not available in the API.
This approach is also way too complicated when we just need to cleanup
the library.

The patch adds the following:

  - ModuleList::ClearSharedModules function to clear all shared
modules.

  - SBDebugger::SetClearSharedModules(bool) API to enable clearing
during SBDebugger::Clear().

  - `settings set clear-shared-modules true` command line option
that is equivalent to SetClearSharedModules(true).

This new behaviour is turned off by default: the debugger does not
release these shared modules until the process exits.
---
 lldb/include/lldb/API/SBDebugger.h|   2 +
 lldb/include/lldb/Core/Debugger.h |   4 +
 lldb/include/lldb/Core/ModuleList.h   |   3 +
 lldb/source/API/SBDebugger.cpp|   6 +
 lldb/source/Core/CoreProperties.td|   4 +
 lldb/source/Core/Debugger.cpp |  13 ++
 lldb/source/Core/ModuleList.cpp   |   7 +
 .../python_api/debugger/TestDebuggerAPI.py|  31 +++
 lldb/unittests/Target/CMakeLists.txt  |   1 +
 .../unittests/Target/SharedModuleListTest.cpp | 209 ++
 10 files changed, 280 insertions(+)
 create mode 100644 lldb/unittests/Target/SharedModuleListTest.cpp

diff --git a/lldb/include/lldb/API/SBDebugger.h 
b/lldb/include/lldb/API/SBDebugger.h
index 192fbee9c0c6d..6a2f76f2d5685 100644
--- a/lldb/include/lldb/API/SBDebugger.h
+++ b/lldb/include/lldb/API/SBDebugger.h
@@ -319,6 +319,8 @@ class LLDB_API SBDebugger {
 
   bool SetShowInlineDiagnostics(bool);
 
+  bool SetClearSharedModules(bool);
+
   bool SetUseSourceCache(bool use_source_cache);
 
   bool GetUseSourceCache() const;
diff --git a/lldb/include/lldb/Core/Debugger.h 
b/lldb/include/lldb/Core/Debugger.h
index 504f936fe317a..4cf7fc75bafd4 100644
--- a/lldb/include/lldb/Core/Debugger.h
+++ b/lldb/include/lldb/Core/Debugger.h
@@ -373,6 +373,10 @@ class Debugger : public 
std::enable_shared_from_this,
 
   bool SetShowInlineDiagnostics(bool);
 
+  bool GetClearSharedModules() const;
+
+  bool SetClearSharedModules(bool);
+
   bool LoadPlugin(const FileSpec &spec, Status &error);
 
   void RunIOHandlers();
diff --git a/lldb/include/lldb/Core/ModuleList.h 
b/lldb/include/lldb/Core/ModuleList.h
index 909ee08f9ba62..587843dd05a4d 100644
--- a/lldb/include/lldb/Core/ModuleList.h
+++ b/lldb/include/lldb/Core/ModuleList.h
@@ -482,6 +482,9 @@ class ModuleList {
 
   static bool RemoveSharedModuleIfOrphaned(const Module *module_ptr);
 
+  /// Empty global cache of modules to release memory, file locks, etc.
+  static void ClearSharedModules();
+
   /// Applies 'callback' to each module in this ModuleList.
   /// If 'callback' returns false, iteration terminates.
   /// The 'module_sp' passed to 'callback' is guaranteed to
diff --git a/lldb/source/API/SBDebugger.cpp b/lldb/source/API/SBDebugger.cpp
index 603e306497841..221c02cfe66ed 100644
--- a/lldb/source/API/SBDebugger.cpp
+++ b/lldb/source/API/SBDebugger.cpp
@@ -1466,6 +1466,12 @@ bool SBDebugger::SetShowInlineDiagnostics(bool value) {
   return (m_opaque_sp ? m_opaque_sp->SetShowInlineDiagnostics(value) : false);
 }
 
+bool SBDebugger::SetClearSharedModules(bool va

[Lldb-commits] [lldb] [lldb] Clear ModuleList shared modules in SBDebugger::Clear (PR #147289)

[Andrew Savonichev via lldb-commits]

asavonic wrote:

> Python code formatter, darker found issues in your code.

Fixed.

https://github.com/llvm/llvm-project/pull/147289
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [lldb] Clear ModuleList shared modules in SBDebugger::Clear (PR #147289)

[Andrew Savonichev via lldb-commits]

https://github.com/asavonic updated 
https://github.com/llvm/llvm-project/pull/147289

>From a01eb1943afb03d1dc9439e9ae8f3a01b8c15398 Mon Sep 17 00:00:00 2001
From: Andrew Savonichev 
Date: Mon, 7 Jul 2025 17:33:50 +0900
Subject: [PATCH] [lldb] Clear ModuleList shared modules in SBDebugger::Clear

Shared modules are stored in a global ModuleList cache, and it is
intentionally leaked to avoid doing cleanup when lldb exits.

However, when lldb is used as a library, we need a way to manage
opened modules to avoid problems with file locks (on some systems) for
modules that we no longer need.

It should be possible to record all loaded modules and use
ModuleList::RemoveSharedModule and RemoveOrphanSharedModules functions
to clear the cache, but these functions are not available in the API.
This approach is also way too complicated when we just need to cleanup
the library.

The patch adds the following:

  - ModuleList::ClearSharedModules function to clear all shared
modules.

  - SBDebugger::SetClearSharedModules(bool) API to enable clearing
during SBDebugger::Clear().

  - `settings set clear-shared-modules true` command line option
that is equivalent to SetClearSharedModules(true).

This new behaviour is turned off by default: the debugger does not
release these shared modules until the process exits.
---
 lldb/include/lldb/API/SBDebugger.h|   2 +
 lldb/include/lldb/Core/Debugger.h |   4 +
 lldb/include/lldb/Core/ModuleList.h   |   3 +
 lldb/source/API/SBDebugger.cpp|   6 +
 lldb/source/Core/CoreProperties.td|   4 +
 lldb/source/Core/Debugger.cpp |  13 ++
 lldb/source/Core/ModuleList.cpp   |   7 +
 .../python_api/debugger/TestDebuggerAPI.py|  30 +++
 lldb/unittests/Target/CMakeLists.txt  |   1 +
 .../unittests/Target/SharedModuleListTest.cpp | 209 ++
 10 files changed, 279 insertions(+)
 create mode 100644 lldb/unittests/Target/SharedModuleListTest.cpp

diff --git a/lldb/include/lldb/API/SBDebugger.h 
b/lldb/include/lldb/API/SBDebugger.h
index 192fbee9c0c6d..6a2f76f2d5685 100644
--- a/lldb/include/lldb/API/SBDebugger.h
+++ b/lldb/include/lldb/API/SBDebugger.h
@@ -319,6 +319,8 @@ class LLDB_API SBDebugger {
 
   bool SetShowInlineDiagnostics(bool);
 
+  bool SetClearSharedModules(bool);
+
   bool SetUseSourceCache(bool use_source_cache);
 
   bool GetUseSourceCache() const;
diff --git a/lldb/include/lldb/Core/Debugger.h 
b/lldb/include/lldb/Core/Debugger.h
index 504f936fe317a..4cf7fc75bafd4 100644
--- a/lldb/include/lldb/Core/Debugger.h
+++ b/lldb/include/lldb/Core/Debugger.h
@@ -373,6 +373,10 @@ class Debugger : public 
std::enable_shared_from_this,
 
   bool SetShowInlineDiagnostics(bool);
 
+  bool GetClearSharedModules() const;
+
+  bool SetClearSharedModules(bool);
+
   bool LoadPlugin(const FileSpec &spec, Status &error);
 
   void RunIOHandlers();
diff --git a/lldb/include/lldb/Core/ModuleList.h 
b/lldb/include/lldb/Core/ModuleList.h
index 909ee08f9ba62..587843dd05a4d 100644
--- a/lldb/include/lldb/Core/ModuleList.h
+++ b/lldb/include/lldb/Core/ModuleList.h
@@ -482,6 +482,9 @@ class ModuleList {
 
   static bool RemoveSharedModuleIfOrphaned(const Module *module_ptr);
 
+  /// Empty global cache of modules to release memory, file locks, etc.
+  static void ClearSharedModules();
+
   /// Applies 'callback' to each module in this ModuleList.
   /// If 'callback' returns false, iteration terminates.
   /// The 'module_sp' passed to 'callback' is guaranteed to
diff --git a/lldb/source/API/SBDebugger.cpp b/lldb/source/API/SBDebugger.cpp
index 603e306497841..221c02cfe66ed 100644
--- a/lldb/source/API/SBDebugger.cpp
+++ b/lldb/source/API/SBDebugger.cpp
@@ -1466,6 +1466,12 @@ bool SBDebugger::SetShowInlineDiagnostics(bool value) {
   return (m_opaque_sp ? m_opaque_sp->SetShowInlineDiagnostics(value) : false);
 }
 
+bool SBDebugger::SetClearSharedModules(bool value) {
+  LLDB_INSTRUMENT_VA(this, value);
+
+  return (m_opaque_sp ? m_opaque_sp->SetClearSharedModules(value) : false);
+}
+
 bool SBDebugger::SetUseSourceCache(bool value) {
   LLDB_INSTRUMENT_VA(this, value);
 
diff --git a/lldb/source/Core/CoreProperties.td 
b/lldb/source/Core/CoreProperties.td
index 53dd333f045c9..1a6ba1a9af84e 100644
--- a/lldb/source/Core/CoreProperties.td
+++ b/lldb/source/Core/CoreProperties.td
@@ -268,4 +268,8 @@ let Definition = "debugger" in {
 Global,
 DefaultFalse,
 Desc<"Controls whether diagnostics can refer directly to the command 
input, drawing arrows to it. If false, diagnostics will echo the input.">;
+  def ClearSharedModules: Property<"clear-shared-modules", "Boolean">,
+Global,
+DefaultFalse,
+Desc<"Controls whether the debugger clears internal shared modules as it 
exits.">;
 }
diff --git a/lldb/source/Core/Debugger.cpp b/lldb/source/Core/Debugger.cpp
index ed674ee1275c7..fbd2f37960e19 100644
--- a/lldb/source/Core/Debugger.cpp
+++ b/lldb/source/Core/Debugger.cpp

[Lldb-commits] [lldb] [lldb] Clear ModuleList shared modules in SBDebugger::Clear (PR #147289)

[Andrew Savonichev via lldb-commits]

asavonic wrote:

> > However, when lldb is used as a library, we need a way to manage opened 
> > modules to avoid problems with file locks (on some systems) for modules 
> > that we no longer need.
> 
> Can you give a concrete example? This seems like its own issue that should be 
> solved rather than worked around. I wouldn't expect us to need to keep files 
> open for as long as a module exists.

Right, it is a separate issue. I'll follow up later.

> 
> > It should be possible to record all loaded modules and use 
> > `ModuleList::RemoveSharedModule` and `RemoveOrphanSharedModules` functions 
> > to clear the cache, but these functions are not available in the API. This 
> > approach is also way too complicated when we just need to cleanup the 
> > library.
> 
> This is exposed from the SB API through `SBDebugger::MemoryPressureDetected`. 
> Not sure what's "complicated" about calling his function, but I do agree that 
> this shouldn't necessary be an explicit operation and would make a good 
> default. The natural place to do this is in `SBDebugger::Destroy`. You can 
> have many debuggers and it seems like a bad idea to clear the modules when 
> you clear one of them.

How should a proper debugger shutdown sequence look like?

1. Release all `SBModule` pointers.
2. Call `SBDebugger::MemoryPressureDetected`.
3. Call `SBDebugger::Destroy`.

`SBDebugger::MemoryPressureDetected` only removes shared pointers to modules 
with `use_count == 1`. Assuming that the calling program releases all its 
modules in (1), we need to make sure that the debugger does not keep any 
pointers elsewhere before (2). Can we rely on this?

If any module survives `MemoryPressureDetected`, they will not be released by 
`SBDebugger::Destroy`.

https://github.com/llvm/llvm-project/pull/147289
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [lldb] Clear ModuleList shared modules in SBDebugger::Clear (PR #147289)

[Andrew Savonichev via lldb-commits]

https://github.com/asavonic updated 
https://github.com/llvm/llvm-project/pull/147289

>From a01eb1943afb03d1dc9439e9ae8f3a01b8c15398 Mon Sep 17 00:00:00 2001
From: Andrew Savonichev 
Date: Mon, 7 Jul 2025 17:33:50 +0900
Subject: [PATCH 1/3] [lldb] Clear ModuleList shared modules in
 SBDebugger::Clear

Shared modules are stored in a global ModuleList cache, and it is
intentionally leaked to avoid doing cleanup when lldb exits.

However, when lldb is used as a library, we need a way to manage
opened modules to avoid problems with file locks (on some systems) for
modules that we no longer need.

It should be possible to record all loaded modules and use
ModuleList::RemoveSharedModule and RemoveOrphanSharedModules functions
to clear the cache, but these functions are not available in the API.
This approach is also way too complicated when we just need to cleanup
the library.

The patch adds the following:

  - ModuleList::ClearSharedModules function to clear all shared
modules.

  - SBDebugger::SetClearSharedModules(bool) API to enable clearing
during SBDebugger::Clear().

  - `settings set clear-shared-modules true` command line option
that is equivalent to SetClearSharedModules(true).

This new behaviour is turned off by default: the debugger does not
release these shared modules until the process exits.
---
 lldb/include/lldb/API/SBDebugger.h|   2 +
 lldb/include/lldb/Core/Debugger.h |   4 +
 lldb/include/lldb/Core/ModuleList.h   |   3 +
 lldb/source/API/SBDebugger.cpp|   6 +
 lldb/source/Core/CoreProperties.td|   4 +
 lldb/source/Core/Debugger.cpp |  13 ++
 lldb/source/Core/ModuleList.cpp   |   7 +
 .../python_api/debugger/TestDebuggerAPI.py|  30 +++
 lldb/unittests/Target/CMakeLists.txt  |   1 +
 .../unittests/Target/SharedModuleListTest.cpp | 209 ++
 10 files changed, 279 insertions(+)
 create mode 100644 lldb/unittests/Target/SharedModuleListTest.cpp

diff --git a/lldb/include/lldb/API/SBDebugger.h 
b/lldb/include/lldb/API/SBDebugger.h
index 192fbee9c0c6d..6a2f76f2d5685 100644
--- a/lldb/include/lldb/API/SBDebugger.h
+++ b/lldb/include/lldb/API/SBDebugger.h
@@ -319,6 +319,8 @@ class LLDB_API SBDebugger {
 
   bool SetShowInlineDiagnostics(bool);
 
+  bool SetClearSharedModules(bool);
+
   bool SetUseSourceCache(bool use_source_cache);
 
   bool GetUseSourceCache() const;
diff --git a/lldb/include/lldb/Core/Debugger.h 
b/lldb/include/lldb/Core/Debugger.h
index 504f936fe317a..4cf7fc75bafd4 100644
--- a/lldb/include/lldb/Core/Debugger.h
+++ b/lldb/include/lldb/Core/Debugger.h
@@ -373,6 +373,10 @@ class Debugger : public 
std::enable_shared_from_this,
 
   bool SetShowInlineDiagnostics(bool);
 
+  bool GetClearSharedModules() const;
+
+  bool SetClearSharedModules(bool);
+
   bool LoadPlugin(const FileSpec &spec, Status &error);
 
   void RunIOHandlers();
diff --git a/lldb/include/lldb/Core/ModuleList.h 
b/lldb/include/lldb/Core/ModuleList.h
index 909ee08f9ba62..587843dd05a4d 100644
--- a/lldb/include/lldb/Core/ModuleList.h
+++ b/lldb/include/lldb/Core/ModuleList.h
@@ -482,6 +482,9 @@ class ModuleList {
 
   static bool RemoveSharedModuleIfOrphaned(const Module *module_ptr);
 
+  /// Empty global cache of modules to release memory, file locks, etc.
+  static void ClearSharedModules();
+
   /// Applies 'callback' to each module in this ModuleList.
   /// If 'callback' returns false, iteration terminates.
   /// The 'module_sp' passed to 'callback' is guaranteed to
diff --git a/lldb/source/API/SBDebugger.cpp b/lldb/source/API/SBDebugger.cpp
index 603e306497841..221c02cfe66ed 100644
--- a/lldb/source/API/SBDebugger.cpp
+++ b/lldb/source/API/SBDebugger.cpp
@@ -1466,6 +1466,12 @@ bool SBDebugger::SetShowInlineDiagnostics(bool value) {
   return (m_opaque_sp ? m_opaque_sp->SetShowInlineDiagnostics(value) : false);
 }
 
+bool SBDebugger::SetClearSharedModules(bool value) {
+  LLDB_INSTRUMENT_VA(this, value);
+
+  return (m_opaque_sp ? m_opaque_sp->SetClearSharedModules(value) : false);
+}
+
 bool SBDebugger::SetUseSourceCache(bool value) {
   LLDB_INSTRUMENT_VA(this, value);
 
diff --git a/lldb/source/Core/CoreProperties.td 
b/lldb/source/Core/CoreProperties.td
index 53dd333f045c9..1a6ba1a9af84e 100644
--- a/lldb/source/Core/CoreProperties.td
+++ b/lldb/source/Core/CoreProperties.td
@@ -268,4 +268,8 @@ let Definition = "debugger" in {
 Global,
 DefaultFalse,
 Desc<"Controls whether diagnostics can refer directly to the command 
input, drawing arrows to it. If false, diagnostics will echo the input.">;
+  def ClearSharedModules: Property<"clear-shared-modules", "Boolean">,
+Global,
+DefaultFalse,
+Desc<"Controls whether the debugger clears internal shared modules as it 
exits.">;
 }
diff --git a/lldb/source/Core/Debugger.cpp b/lldb/source/Core/Debugger.cpp
index ed674ee1275c7..fbd2f37960e19 100644
--- a/lldb/source/Core/Debugger.cpp
+++ b/lldb/source/Core/Debugger

[Lldb-commits] [lldb] [lldb] Clear ModuleList shared modules in SBDebugger::Clear (PR #147289)

[Andrew Savonichev via lldb-commits]

asavonic wrote:

Updated the patch to [use 
RemoveOrphanSharedModules](https://github.com/llvm/llvm-project/pull/147289/commits/7a27fe962726c3190ce6ad7d635e3ae30a0f52f7)
 instead of force-clearing the cache.
Changed the description as well.

> No one SBDebugger should force-clear the Shared Module Cache. That's a 
> resource shared among all the SBDebuggers, and no SBDebugger can know what 
> the other debugger's intentions are w.r.t. it. That seems wrong to me.

Agree.

> The most an SBDebugger should do on Destroy or Clear is call 
> MemoryPressureDetected or a similar lower-level API to clear all the modules 
> that it is the only owner of. It should not clear the cache out from under 
> other users.

The last revision should do that. The downside is that the program must release 
all shared pointers to modules before calling `Destroy`, or they leak again.

https://github.com/llvm/llvm-project/pull/147289
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits