Implement ClangImporter importSourceLoc and importSourceRange #40010
Conversation
NuriAmari
force-pushed
the
importSourceLoc
branch
from
6a63476 to
c357a06
Compare
|
The change to A SourceRange when created asserts that both the start and end locations are either both invalid, or both valid. When you call getSourceRange or something similar on an AbstractFunctionDecl, it gives you a range from the |
|
/cc @plotfi |
|
@swift-ci please test |
|
Build failed |
|
@swift-ci please test Linux platform |
|
I think Linux failed because we started testing between #9723 and swiftlang/swift-markdown#11. Hopefully it works now. I don't know if macOS will fail in the same way later. |
|
Build failed |
|
Looks good to me. |
|
Rebuild locally with LLDB and I don't get the segfault. I will keep trying, but firing macOS again just in case it was just flaky. @swift-ci please test macOS platform |
|
@swift-ci please test Linux platform |
|
Build failed |
Do you mean between #39723 and [swiftlang/swift-markdown#11]? And can you confirm the macOS failure has something to do with this issue? I'm currently working on add a PR [swiftlang/swift-markdown#13] to revert the "revert" PR [swiftlang/swift-markdown#11]. |
The macOS failure looks to be something else. |
Correct, #39723. I must have copy/pasted wrong. The Linux failure was something related to DocC, I looked around and found that recent PR being merged, and the revert in the other repository. As it shows in the second Linux run, those problems seems to be gone. The macOS problems seems to be unrelated to those issues, and seems to be related to these changes. |
|
@swift-ci please smoke test macOS platform |
|
After a long divide and conquer session, the problem happens from three invocations: diff --git i/lib/ClangImporter/ImportDecl.cpp w/lib/ClangImporter/ImportDecl.cpp
index e0182ef422a..2c8fec87191 100644
--- i/lib/ClangImporter/ImportDecl.cpp
+++ w/lib/ClangImporter/ImportDecl.cpp
@@ -3376,8 +3376,13 @@ namespace {
if (alreadyImportedResult != Impl.ImportedDecls.end())
return alreadyImportedResult->second;
result = Impl.createDeclWithClangNode<StructDecl>(
- decl, AccessLevel::Public, Impl.importSourceLoc(decl->getBeginLoc()),
- name, Impl.importSourceLoc(decl->getLocation()), None, nullptr, dc);
+ decl, AccessLevel::Public,
+ // FIXME: Impl.importSourceLocBAD(decl->getBeginLoc()),
+ SourceLoc(),
+ name,
+ // FIXME: Impl.importSourceLocBAD(decl->getLocation()),
+ SourceLoc(),
+ None, nullptr, dc);
Impl.ImportedDecls[{decl->getCanonicalDecl(), getVersion()}] = result;
// FIXME: Figure out what to do with superclasses in C++. One possible
@@ -4215,7 +4220,8 @@ namespace {
Impl.createDeclWithClangNode<VarDecl>(decl, AccessLevel::Public,
/*IsStatic*/ false,
VarDecl::Introducer::Var,
- Impl.importSourceLoc(decl->getLocation()),
+ // FIXME: Impl.importSourceLocBAD(decl->getLocation()), // BAD import
+ SourceLoc(),
name, dc);
if (decl->getType().isConstQualified()) {
// Note that in C++ there are ways to change the values of constThese invocations are related to A possible approach is leaving those three invocations with just One thing that I don't know if it happens automatically or not is that the test seems to be rewriting the source map, and also providing an overlay. I don't know if the current implementation of I also have a spindump of the Python process when it was failing, but I don't know if it actually captures close to the segfault or not (it is complicated to capture a spindump of the exact thing that's happening, but I was hoping to see an infinite recursion or similar). |
NuriAmari
force-pushed
the
importSourceLoc
branch
from
c357a06 to
aca7c03
Compare
|
@swift-ci please smoke test |
NuriAmari
force-pushed
the
importSourceLoc
branch
from
aca7c03 to
cb0b8e7
Compare
|
@swift-ci please smoke test |
|
The linux one seems unrelated (something with DocC). Seems that non-smoke is passing, so… @swift-ci please test Linux platform |
|
Build failed |
|
FWIW I put a PR up to implement these a while back (#39701) but let it fall off my radar. Most of the PR is changes for actually returning the imported source location in The other part is changing how line directives are handled. I never actually tested the performance, but I was concerned with the current implementation when line directives are involved - it grabs the presumed location for every location and adds a "VirtualFile" for every imported line. Not super important when it's just diagnostics making locations, but IMO would be good to avoid if we actually import locations for all decls. It also doesn't handle EOF properly. One of the main issues is that it relies on the SourceManager internals and a LLVM change, which I never put up (basically just making all line entries accessible). |
|
@bnbarham: thanks for the feedback. We have been seeing better reporting by using the PD: the Linux test failed again with the DocC problem. I see some green ones in the bots now, I will restart in a moment. |
|
@swift-ci please smoke test Linux platform |
|
@swift-ci Please test compiler performance |
|
|
I'd love to see these changes land. Is there anything currently blocking this? @swift-ci please test linux platform. |
|
@swift-ci please test linux platform |
Nothing beyond me wanting to check that I understood the benchmark results correctly, and those "regressions" are just talking about number of files, and not some number of seconds. @bnbarham: from the benchmarks above, it looks like, as expected, the number of source buffers grows significantly, but nothing else regressed, specially in time. Merging this would allow you to continue your work in that other PR without a lot of conflict, but tell us if we should hold this for any other reason. Thanks! |
Looks like it's just the number of files to me, so seems fine. I'll close the other PR, if people are interested in having |
Implement
ClangImporter::Implementation::importSourceLocandClangImporter::Implementation::importSourceRange.