[ClangImporter] Handle allowing PCM/PCH errors #40273
Conversation
|
@swift-ci please test |
bnbarham
force-pushed
the
pch-allow-errors
branch
from
70b9159 to
5703e89
Compare
|
@swift-ci please test |
1 similar comment
|
@swift-ci please test |
| }; | ||
|
|
||
| //--- use.swift | ||
| func use(s: SomeTy) {} |
There was a problem hiding this comment.
I'm a little nervous about how thin these tests are. If I understand correctly, this change will allow invalid clang ASTs to appear in many parts of the ClangImporter where they were previously impossible, but your test case only exercises the tiny fraction of the importer that is involved in importing a struct without any interesting attributes or features, and there isn't actually anything invalid in this struct itself. Any other failure would not be caught here and would presumably surface as a weird crash in ClangImporter that we'd need to triage and test later.
Can you think of a way to (a) significantly improve test coverage (maybe you could re-run existing ClangImporter tests but with a broken code added to the headers?) and (b) make sure that crash logs clearly indicate that invalid clang ASTs were allowed?
There was a problem hiding this comment.
Invalid ASTs are already possible when -fallow-pcm-with-compiler-errors is passed in when loading PCMs (there's no check for diagnostics in that case). But I agree the test coverage isn't great here.
Regarding the crash logs, I had originally added a While allowing modules with compiler errors enabled line to the pretty stack trace a while ago, but have since removed that as it duplicates the information in the program arguments. I'm happy to put that back in if you think it'd be better to make it more obvious.
There was a problem hiding this comment.
I think it'd be good to add that back. The program arguments are often truncated and always difficult to visually parse.
There was a problem hiding this comment.
Unfortunately if the program arguments are truncated then the extra line won't be output anyway (since the arguments are the first thing printed). But I can add it in to address the second part (difficult to visually parse them).
There was a problem hiding this comment.
I've added that line back in as an extension to the existing frontend "Compiling with..."
If we think it's worth it I could also do some LLVM changes to allow printing the pretty stacktrace in reverse order. That would print arguments last rather than first (which then truncates everything). It currently reverses the stored "frames" in order to print, so should be pretty simple.
|
Build failed |
Ignore errors when emitting/reading a PCM/PCH and Clang has `-fallow-(pcm|pch)-with-compiler-errors` set. Resolves rdar://85576570.
While this will be in the compiler arguments, it's easy to miss when skimming over the pretty stacktrace. Add an explicit message to make it easier to see while looking over crashes.
bnbarham
force-pushed
the
pch-allow-errors
branch
from
5703e89 to
efcb5e3
Compare
|
@swift-ci please test |
|
@beccadax given the addition of the pretty stacktrace line and that invalid ASTs are already allowed for modules (this change is just for the bridging header), would you be okay with this being merged? Happy to talk more about tests, but it would be good to get this in so that allowing errors works for bridging headers as well. |
|
Let's get this in and we can follow-up with further enhancements in a separate PR. |
Ignore errors when emitting/reading a PCM/PCH and Clang has
-fallow-(pcm|pch)-with-compiler-errorsset.Resolves rdar://85576570.