Skip to content

Commit 2682ab2

Browse files
benlangmuirbenlangmuir
authored
Merge pull request #3503 from benlangmuir/concurrent-cursor-crash
[sourcekit] Synchronize cursor-info requests on generated interfaces
2 parents 4b01574 + 18be066 commit 2682ab2

File tree

10 files changed

+176
-27
lines changed

10 files changed

+176
-27
lines changed

‎test/SourceKit/CursorInfo/cursor_info_async.swift

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,26 @@
1+
import Foo
2+
3+
// REQUIRES: objc_interop
4+
5+
// Perform 8 concurrent cursor infos, which is often enough to cause
6+
// contention. We disable printing the requests to minimize delay.
7+
8+
// RUN: %sourcekitd-test -req=interface-gen-open -module Foo -- \
9+
// RUN: -F %S/../Inputs/libIDE-mock-sdk %mcp_opt %clang-importer-sdk \
10+
// RUN: == -async -dont-print-request -req=cursor -pos=60:15 \
11+
// RUN: == -async -dont-print-request -req=cursor -pos=60:15 \
12+
// RUN: == -async -dont-print-request -req=cursor -pos=60:15 \
13+
// RUN: == -async -dont-print-request -req=cursor -pos=60:15 \
14+
// RUN: == -async -dont-print-request -req=cursor -pos=60:15 \
15+
// RUN: == -async -dont-print-request -req=cursor -pos=60:15 \
16+
// RUN: == -async -dont-print-request -req=cursor -pos=60:15 \
17+
// RUN: == -async -dont-print-request -req=cursor -pos=60:15 | FileCheck %s -check-prefix=CHECK-FOO
18+
19+
// CHECK-FOO: <decl.struct><syntaxtype.keyword>struct</syntaxtype.keyword> <decl.name>FooRuncingOptions
20+
// CHECK-FOO: <decl.struct><syntaxtype.keyword>struct</syntaxtype.keyword> <decl.name>FooRuncingOptions
21+
// CHECK-FOO: <decl.struct><syntaxtype.keyword>struct</syntaxtype.keyword> <decl.name>FooRuncingOptions
22+
// CHECK-FOO: <decl.struct><syntaxtype.keyword>struct</syntaxtype.keyword> <decl.name>FooRuncingOptions
23+
// CHECK-FOO: <decl.struct><syntaxtype.keyword>struct</syntaxtype.keyword> <decl.name>FooRuncingOptions
24+
// CHECK-FOO: <decl.struct><syntaxtype.keyword>struct</syntaxtype.keyword> <decl.name>FooRuncingOptions
25+
// CHECK-FOO: <decl.struct><syntaxtype.keyword>struct</syntaxtype.keyword> <decl.name>FooRuncingOptions
26+
// CHECK-FOO: <decl.struct><syntaxtype.keyword>struct</syntaxtype.keyword> <decl.name>FooRuncingOptions

‎tools/SourceKit/lib/SwiftLang/SwiftASTManager.cpp

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -209,6 +209,10 @@ namespace SourceKit {
209209
EditorDiagConsumer &ASTUnit::getEditorDiagConsumer() const {
210210
return Impl.CollectDiagConsumer;
211211
}
212+
213+
void ASTUnit::performAsync(std::function<void()> Fn) {
214+
Impl.Queue.dispatch(std::move(Fn));
215+
}
212216
}
213217

214218
namespace {

‎tools/SourceKit/lib/SwiftLang/SwiftASTManager.h

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@
1717
#include "SourceKit/Core/LLVM.h"
1818
#include "llvm/ADT/ArrayRef.h"
1919
#include "llvm/ADT/StringRef.h"
20+
#include <functional>
2021
#include <string>
2122

2223
namespace llvm {
@@ -55,6 +56,10 @@ class ASTUnit : public SourceKit::ThreadSafeRefCountedBase<ASTUnit> {
5556
ArrayRef<ImmutableTextSnapshotRef> getSnapshots() const;
5657
EditorDiagConsumer &getEditorDiagConsumer() const;
5758
swift::SourceFile &getPrimarySourceFile() const;
59+
60+
/// Perform \p Fn asynchronously while preventing concurrent access to the
61+
/// AST.
62+
void performAsync(std::function<void()> Fn);
5863
};
5964

6065
typedef IntrusiveRefCntPtr<ASTUnit> ASTUnitRef;

‎tools/SourceKit/lib/SwiftLang/SwiftEditorInterfaceGen.cpp

Lines changed: 18 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -79,6 +79,10 @@ class SwiftInterfaceGenContext::Implementation {
7979
SourceTextInfo Info;
8080
// This is the non-typechecked AST for the generated interface source.
8181
CompilerInstance TextCI;
82+
// Syncronize access to the embedded compiler instance (if we don't have an
83+
// ASTUnit).
84+
WorkQueue Queue{WorkQueue::Dequeuing::Serial,
85+
"sourcekit.swift.InterfaceGenContext"};
8286
};
8387

8488
typedef SwiftInterfaceGenContext::Implementation::TextRange TextRange;
@@ -498,6 +502,14 @@ void SwiftInterfaceGenContext::reportEditorInfo(EditorConsumer &Consumer) const
498502
Consumer.finished();
499503
}
500504

505+
void SwiftInterfaceGenContext::accessASTAsync(std::function<void()> Fn) {
506+
if (Impl.AstUnit) {
507+
Impl.AstUnit->performAsync(std::move(Fn));
508+
} else {
509+
Impl.Queue.dispatch(std::move(Fn));
510+
}
511+
}
512+
501513
SwiftInterfaceGenContext::ResolvedEntity
502514
SwiftInterfaceGenContext::resolveEntityForOffset(unsigned Offset) const {
503515
// Search among the references.
@@ -633,8 +645,10 @@ void SwiftLangSupport::editorOpenInterface(EditorConsumer &Consumer,
633645
return;
634646
}
635647

636-
IFaceGenContexts.set(Name, IFaceGenRef);
637648
IFaceGenRef->reportEditorInfo(Consumer);
649+
// reportEditorInfo requires exclusive access to the AST, so don't add this
650+
// to the service cache until it has returned.
651+
IFaceGenContexts.set(Name, IFaceGenRef);
638652
}
639653

640654
class PrimaryFileInterfaceConsumer : public SwiftASTConsumer {
@@ -740,8 +754,10 @@ void SwiftLangSupport::editorOpenHeaderInterface(EditorConsumer &Consumer,
740754
return;
741755
}
742756

743-
IFaceGenContexts.set(Name, IFaceGenRef);
744757
IFaceGenRef->reportEditorInfo(Consumer);
758+
// reportEditorInfo requires exclusive access to the AST, so don't add this
759+
// to the service cache until it has returned.
760+
IFaceGenContexts.set(Name, IFaceGenRef);
745761
}
746762

747763
void SwiftLangSupport::findInterfaceDocument(StringRef ModuleName,

‎tools/SourceKit/lib/SwiftLang/SwiftInterfaceGenContext.h

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -55,6 +55,7 @@ class SwiftInterfaceGenContext :
5555

5656
bool matches(StringRef ModuleName, const swift::CompilerInvocation &Invok);
5757

58+
/// Note: requires exclusive access to the underlying AST.
5859
void reportEditorInfo(EditorConsumer &Consumer) const;
5960

6061
struct ResolvedEntity {
@@ -71,8 +72,12 @@ class SwiftInterfaceGenContext :
7172
bool isResolved() const { return Dcl || Mod; }
7273
};
7374

75+
/// Provides exclusive access to the underlying AST.
76+
void accessASTAsync(std::function<void()> Fn);
77+
7478
/// Returns the resolved entity along with a boolean indicating if it is a
7579
/// reference or not.
80+
/// Note: requires exclusive access to the underlying AST. See accessASTAsync.
7681
ResolvedEntity resolveEntityForOffset(unsigned Offset) const;
7782

7883
/// Searches for a declaration with the given USR and returns the

‎tools/SourceKit/lib/SwiftLang/SwiftSourceDocInfo.cpp

Lines changed: 18 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -1024,24 +1024,26 @@ void SwiftLangSupport::getCursorInfo(
10241024
SwiftArgs, OpArgs);
10251025
}
10261026

1027-
SwiftInterfaceGenContext::ResolvedEntity Entity;
1028-
Entity = IFaceGenRef->resolveEntityForOffset(Offset);
1029-
if (Entity.isResolved()) {
1030-
CompilerInvocation Invok;
1031-
IFaceGenRef->applyTo(Invok);
1032-
if (Entity.Mod) {
1033-
passCursorInfoForModule(Entity.Mod, IFaceGenContexts, Invok, Receiver);
1027+
IFaceGenRef->accessASTAsync([this, IFaceGenRef, Offset, Receiver] {
1028+
SwiftInterfaceGenContext::ResolvedEntity Entity;
1029+
Entity = IFaceGenRef->resolveEntityForOffset(Offset);
1030+
if (Entity.isResolved()) {
1031+
CompilerInvocation Invok;
1032+
IFaceGenRef->applyTo(Invok);
1033+
if (Entity.Mod) {
1034+
passCursorInfoForModule(Entity.Mod, IFaceGenContexts, Invok,
1035+
Receiver);
1036+
} else {
1037+
// FIXME: Should pass the main module for the interface but currently
1038+
// it's not necessary.
1039+
passCursorInfoForDecl(
1040+
Entity.Dcl, /*MainModule*/ nullptr, Type(), Type(), Entity.IsRef,
1041+
/*OrigBufferID=*/None, *this, Invok, {}, Receiver);
1042+
}
10341043
} else {
1035-
// FIXME: Should pass the main module for the interface but currently
1036-
// it's not necessary.
1037-
passCursorInfoForDecl(Entity.Dcl, /*MainModule*/nullptr, Type(), Type(),
1038-
Entity.IsRef,
1039-
/*OrigBufferID=*/None, *this, Invok,
1040-
{}, Receiver);
1044+
Receiver({});
10411045
}
1042-
} else {
1043-
Receiver({});
1044-
}
1046+
});
10451047
return;
10461048
}
10471049

‎tools/SourceKit/tools/sourcekitd-test/Options.td

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -64,6 +64,9 @@ def check_interface_is_ascii : Flag<["-"], "check-interface-ascii">,
6464
def json_request_path: Separate<["-"], "json-request-path">,
6565
HelpText<"path to read a request in JSON format">;
6666

67+
def dont_print_request : Flag<["-"], "dont-print-request">,
68+
HelpText<"Don't print the request">;
69+
6770
def print_response_as_json : Flag<["-"], "print-response-as-json">,
6871
HelpText<"Print the response as JSON output">;
6972

@@ -81,3 +84,6 @@ def synthesized_extension : Flag<["-"], "synthesized-extension">,
8184

8285
def interested_usr : Separate<["-"], "interested-usr">,
8386
HelpText<"Interested USR to calculate the containing group">;
87+
88+
def async : Flag<["-"], "async">,
89+
HelpText<"Perform request asynchronously">;

‎tools/SourceKit/tools/sourcekitd-test/TestOptions.cpp

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -216,6 +216,10 @@ bool TestOptions::parseArgs(llvm::ArrayRef<const char *> Args) {
216216
CheckInterfaceIsASCII = true;
217217
break;
218218

219+
case OPT_dont_print_request:
220+
PrintRequest = false;
221+
break;
222+
219223
case OPT_print_response_as_json:
220224
PrintResponseAsJSON = true;
221225
break;
@@ -242,12 +246,22 @@ bool TestOptions::parseArgs(llvm::ArrayRef<const char *> Args) {
242246
SynthesizedExtensions = true;
243247
break;
244248

249+
case OPT_async:
250+
isAsyncRequest = true;
251+
break;
252+
245253
case OPT_UNKNOWN:
246254
llvm::errs() << "error: unknown argument: "
247255
<< InputArg->getAsString(ParsedArgs) << '\n';
248256
return true;
249257
}
250258
}
251259

260+
if (Request == SourceKitRequest::InterfaceGenOpen && isAsyncRequest) {
261+
llvm::errs()
262+
<< "error: cannot use -async with interface-gen-open request\n";
263+
return true;
264+
}
265+
252266
return false;
253267
}

‎tools/SourceKit/tools/sourcekitd-test/TestOptions.h

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -74,10 +74,12 @@ struct TestOptions {
7474
std::string USR;
7575
bool CheckInterfaceIsASCII = false;
7676
bool UsedSema = false;
77+
bool PrintRequest = true;
7778
bool PrintResponseAsJSON = false;
7879
bool PrintRawResponse = false;
7980
bool SimplifiedDemangling = false;
8081
bool SynthesizedExtensions = false;
82+
bool isAsyncRequest = false;
8183
bool parseArgs(llvm::ArrayRef<const char *> Args);
8284
};
8385

‎tools/SourceKit/tools/sourcekitd-test/sourcekitd-test.cpp

Lines changed: 78 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -36,6 +36,10 @@ using namespace llvm;
3636
using namespace sourcekitd_test;
3737

3838
static int handleTestInvocation(ArrayRef<const char *> Args, TestOptions &InitOpts);
39+
static bool handleResponse(sourcekitd_response_t Resp, const TestOptions &Opts,
40+
const std::string &SourceFile,
41+
std::unique_ptr<llvm::MemoryBuffer> SourceBuf,
42+
TestOptions *InitOpts);
3943
static void printCursorInfo(sourcekitd_variant_t Info, StringRef Filename,
4044
llvm::raw_ostream &OS);
4145
static void printDocInfo(sourcekitd_variant_t Info, StringRef Filename);
@@ -156,6 +160,18 @@ static SourceKit::Semaphore semaSemaphore(0);
156160
static sourcekitd_response_t semaResponse;
157161
static const char *semaName;
158162

163+
namespace {
164+
struct AsyncResponseInfo {
165+
SourceKit::Semaphore semaphore{0};
166+
sourcekitd_response_t response = nullptr;
167+
TestOptions options;
168+
std::string sourceFilename;
169+
std::unique_ptr<llvm::MemoryBuffer> sourceBuffer;
170+
};
171+
}
172+
173+
static std::vector<AsyncResponseInfo> asyncResponses;
174+
159175
static int skt_main(int argc, const char **argv);
160176

161177
int main(int argc, const char **argv) {
@@ -265,17 +281,32 @@ static int skt_main(int argc, const char **argv) {
265281
}
266282
if (i == Args.size())
267283
break;
268-
int ret = handleTestInvocation(Args.slice(0, i), InitOpts);
269-
if (ret) {
284+
if (int ret = handleTestInvocation(Args.slice(0, i), InitOpts)) {
270285
sourcekitd_shutdown();
271286
return ret;
272287
}
273288
Args = Args.slice(i+1);
274289
}
275290

276-
int ret = handleTestInvocation(Args, InitOpts);
291+
if (int ret = handleTestInvocation(Args, InitOpts)) {
292+
sourcekitd_shutdown();
293+
return ret;
294+
}
295+
296+
for (auto &info : asyncResponses) {
297+
if (info.semaphore.wait(60 * 1000)) {
298+
llvm::report_fatal_error("async request timed out");
299+
}
300+
301+
if (handleResponse(info.response, info.options, info.sourceFilename,
302+
std::move(info.sourceBuffer), nullptr)) {
303+
sourcekitd_shutdown();
304+
return 1;
305+
}
306+
}
307+
277308
sourcekitd_shutdown();
278-
return ret;
309+
return 0;
279310
}
280311

281312
static inline const char *getInterfaceGenDocumentName() {
@@ -665,8 +696,45 @@ static int handleTestInvocation(ArrayRef<const char *> Args,
665696
Opts.HeaderPath.c_str());
666697
}
667698

668-
sourcekitd_request_description_dump(Req);
669-
sourcekitd_response_t Resp = sourcekitd_send_request_sync(Req);
699+
if (Opts.PrintRequest)
700+
sourcekitd_request_description_dump(Req);
701+
702+
if (!Opts.isAsyncRequest) {
703+
sourcekitd_response_t Resp = sourcekitd_send_request_sync(Req);
704+
sourcekitd_request_release(Req);
705+
return handleResponse(Resp, Opts, SourceFile, std::move(SourceBuf),
706+
&InitOpts)
707+
? 1
708+
: 0;
709+
} else {
710+
#if SOURCEKITD_HAS_BLOCKS
711+
AsyncResponseInfo info;
712+
info.options = Opts;
713+
info.sourceFilename = std::move(SourceFile);
714+
info.sourceBuffer = std::move(SourceBuf);
715+
unsigned respIndex = asyncResponses.size();
716+
asyncResponses.push_back(std::move(info));
717+
718+
sourcekitd_send_request(Req, nullptr, ^(sourcekitd_response_t resp) {
719+
auto &info = asyncResponses[respIndex];
720+
info.response = resp;
721+
info.semaphore.signal(); // Ready to be handled!
722+
});
723+
724+
#else
725+
llvm::report_fatal_error(
726+
"-async not supported when sourcekitd is built without blocks support");
727+
#endif
728+
729+
sourcekitd_request_release(Req);
730+
return 0;
731+
}
732+
}
733+
734+
static bool handleResponse(sourcekitd_response_t Resp, const TestOptions &Opts,
735+
const std::string &SourceFile,
736+
std::unique_ptr<llvm::MemoryBuffer> SourceBuf,
737+
TestOptions *InitOpts) {
670738
bool KeepResponseAlive = false;
671739
bool IsError = sourcekitd_response_is_error(Resp);
672740
if (IsError) {
@@ -742,8 +810,10 @@ static int handleTestInvocation(ArrayRef<const char *> Args,
742810

743811
case SourceKitRequest::InterfaceGenOpen:
744812
// Just initialize the options for the subsequent request.
745-
InitOpts.SourceFile = getInterfaceGenDocumentName();
746-
InitOpts.SourceText =
813+
assert(!Opts.isAsyncRequest && InitOpts &&
814+
"async interface-gen-open is not supported");
815+
InitOpts->SourceFile = getInterfaceGenDocumentName();
816+
InitOpts->SourceText =
747817
sourcekitd_variant_dictionary_get_string(Info, KeySourceText);
748818
break;
749819

@@ -832,7 +902,6 @@ static int handleTestInvocation(ArrayRef<const char *> Args,
832902

833903
if (!KeepResponseAlive)
834904
sourcekitd_response_dispose(Resp);
835-
sourcekitd_request_release(Req);
836905
return IsError;
837906
}
838907

0 commit comments

Comments
 (0)