[ASTContext] Add setter for PreModuleImportCallback #69730

chelcassanova · 2023-11-08T22:28:54Z

PreModuleImportCallback is a variable for a callback function called before module importing in the Swift compiler. It is currently being used by LLDB to report progress updates on importing Swift modules in LLDB. This commit adds a setter for PreModuleImportCallback for greater flexibility in LLDB and removes the reference to this variable in the constructor.

Related to swiftlang/llvm-project#7769

rdar://105286354

`PreModuleImportCallback` is a variable for a callback function used specifically by LLDB to report progress updates on importing Swift modules in LLDB. This commit adds a setter for `PreModuleImportCallback` for greater flexibility in LLDB and removes the reference to this variable in the constructor. Related to swiftlang/llvm-project#7769 rdar://105286354

medismailben · 2023-11-08T22:33:19Z

include/swift/AST/ASTContext.h

-      llvm::IntrusiveRefCntPtr<llvm::vfs::OutputBackend> OutBackend = nullptr,
-      std::function<bool(llvm::StringRef, bool)> PreModuleImportCallback = {});
+      llvm::IntrusiveRefCntPtr<llvm::vfs::OutputBackend> OutBackend = nullptr
+      );


Interesting formatting. Is that really clang-format's output ?

Yeah it is, other than removing the callback in the constructor I didn't change anything else here.

medismailben · 2023-11-08T22:34:59Z

lib/AST/ASTContext.cpp

@@ -715,6 +714,10 @@ ASTContext::~ASTContext() {
  getImpl().~Implementation();
 }

+void ASTContext::SetPreModuleImportCallback(std::function<bool(llvm::StringRef ModuleName, bool IsOverlay)> callback) {
+  ASTContext::PreModuleImportCallback = callback;


Why do you need to qualify PreModuleImportCallback here ?

I don't, but I think I originally thought that PreModuleImportCallback was static even though it's not so I can remove the qualification.

medismailben · 2023-11-08T22:39:04Z

FWIW, although this callback is used mainly as an LLDB module loading progress callback, it could also be used for/by other things. I'd retitle this PR to be more generic.

chelcassanova · 2023-11-08T23:14:20Z

@swift-ci test

chelcassanova · 2023-11-08T23:15:57Z

Good point about PreModuleImportCallback being generic. Since LLDB was the only one use it I thought it was created specifically for LLDB, I'll update the PR description to reflect this.

adrian-prantl · 2023-11-09T00:53:13Z

include/swift/AST/ASTContext.h

@@ -294,6 +294,8 @@ class ASTContext final {
  /// OutputBackend for writing outputs.
  llvm::IntrusiveRefCntPtr<llvm::vfs::OutputBackend> OutputBackend;

+  void SetPreModuleImportCallback(std::function<bool(llvm::StringRef ModuleName, bool IsOverlay)> callback);


Doxygen comment?

Yes, I'll add one here

Sets the `PreModuleImportCallback`'s setter return type to void to account for the lambda functions in LLDB's progress reporting not actually using its return value. Also adds a Doxygen comment for `SetPreModuleImportCallback`

chelcassanova · 2023-11-10T21:25:33Z

@swift-ci test

chelcassanova · 2023-11-11T05:38:09Z

@swift-ci smoke test

chelcassanova · 2023-11-13T18:14:32Z

swiftlang/llvm-project#7769
@swift-ci test

chelcassanova · 2023-11-13T19:13:18Z

swiftlang/llvm-project#7769
@swift-ci please test

chelcassanova · 2023-11-13T19:21:45Z

swiftlang/llvm-project#7769
@swift-ci please smoke test

augusto2112 · 2023-11-14T23:40:32Z

lib/AST/ASTContext.cpp

@@ -715,6 +714,10 @@ ASTContext::~ASTContext() {
  getImpl().~Implementation();
 }

+void ASTContext::SetPreModuleImportCallback(std::function<void(llvm::StringRef ModuleName, bool IsOverlay)> callback) {


Was this line clang-formatted? Looks pretty long

It should've been, but I can also shorten it myself if clang-format is ignoring it for some reason

If clang-format says it's ok then it's fine

chelcassanova · 2023-11-15T19:09:52Z

swiftlang/llvm-project#7769
@swift-ci please test

chelcassanova · 2023-11-16T22:17:06Z

swiftlang/llvm-project#7769
@swift-ci please test

Run `clang-format`

chelcassanova · 2023-11-17T23:14:39Z

swiftlang/llvm-project#7769
@swift-ci please test

chelcassanova · 2023-11-28T23:48:11Z

swiftlang/llvm-project#7769
@swift-ci please test

chelcassanova · 2023-11-29T18:48:04Z

swiftlang/llvm-project#7769
@swift-ci please test

LLDB's progress reporting infrastructure has had the ability to add details to an overall progress report (added in https://reviews.llvm.org/D143690) but this had yet to be implemented in existing progress reports. This commit adds this functionality to progress reports made for Swift operations (such as caching user imports) so that instead of sending several individual progress reports for each step in these operations, one progress report is created and then incrementally updated. Importing Swift modules takes place in the Swift compiler which uses a callback function so that LLDB can perform its progress report. This originally called into a function in `lldb_private::SwiftASTContext` but will instead use a lambda function to increment a locally created progress report. Related to: swiftlang/swift#69730 rdar://105286354

LLDB's progress reporting infrastructure has had the ability to add details to an overall progress report (added in https://reviews.llvm.org/D143690) but this had yet to be implemented in existing progress reports. This commit adds this functionality to progress reports made for Swift operations (such as caching user imports) so that instead of sending several individual progress reports for each step in these operations, one progress report is created and then incrementally updated. Importing Swift modules takes place in the Swift compiler which uses a callback function so that LLDB can perform its progress report. This originally called into a function in `lldb_private::SwiftASTContext` but will instead use a lambda function to increment a locally created progress report. Related to: swiftlang/swift#69730 rdar://105286354 (cherry picked from commit 4106480)

chelcassanova requested review from adrian-prantl and medismailben November 8, 2023 22:28

chelcassanova requested review from hborla, slavapestov and xedin as code owners November 8, 2023 22:28

chelcassanova mentioned this pull request Nov 8, 2023

[lldb][progress] Improve Swift progress reporting swiftlang/llvm-project#7769

Merged

medismailben reviewed Nov 8, 2023

View reviewed changes

chelcassanova changed the title ~~[ASTContext] Add setter for LLDB callback~~ [ASTContext] Add setter for PreModuleImportCallback Nov 8, 2023

adrian-prantl approved these changes Nov 9, 2023

View reviewed changes

Change callback return type and add Doxygen comment

c708ef1

Sets the `PreModuleImportCallback`'s setter return type to void to account for the lambda functions in LLDB's progress reporting not actually using its return value. Also adds a Doxygen comment for `SetPreModuleImportCallback`

chelcassanova requested a review from augusto2112 November 14, 2023 20:52

augusto2112 reviewed Nov 14, 2023

View reviewed changes

fixup! Change callback return type and add Doxygen comment

b32a0ee

Run `clang-format`

chelcassanova merged commit 04316be into swiftlang:main Dec 1, 2023

chelcassanova mentioned this pull request Feb 1, 2024

[lldb][progress] Improve Swift progress reporting (#7769) swiftlang/llvm-project#8098

Merged

[ASTContext] Add setter for PreModuleImportCallback #69730

[ASTContext] Add setter for PreModuleImportCallback #69730

Uh oh!

Conversation

chelcassanova commented Nov 8, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

medismailben commented Nov 8, 2023

Uh oh!

chelcassanova commented Nov 8, 2023

Uh oh!

chelcassanova commented Nov 8, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chelcassanova commented Nov 10, 2023

Uh oh!

chelcassanova commented Nov 11, 2023

Uh oh!

chelcassanova commented Nov 13, 2023

Uh oh!

chelcassanova commented Nov 13, 2023

Uh oh!

chelcassanova commented Nov 13, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chelcassanova commented Nov 15, 2023

Uh oh!

chelcassanova commented Nov 16, 2023

Uh oh!

chelcassanova commented Nov 17, 2023

Uh oh!

chelcassanova commented Nov 28, 2023

Uh oh!

chelcassanova commented Nov 29, 2023

Uh oh!

Uh oh!

chelcassanova commented Nov 8, 2023 •

edited

Loading