Skip to content

[ClangImporter] Handle allowing PCM/PCH errors #40273

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Dec 4, 2021
Merged

[ClangImporter] Handle allowing PCM/PCH errors #40273

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
594faba
[ClangImporter] Handle allowing PCM/PCH errors
bnbarham Nov 20, 2021
efcb5e3
[Frontend] Add whether allowing compiler errors to the pretty stacktrace
bnbarham Nov 30, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 11 additions & 5 deletions lib/ClangImporter/ClangImporter.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -856,10 +856,12 @@ bool ClangImporter::canReadPCH(StringRef PCHFilename) {
std::make_shared<clang::CompilerInvocation>(*Impl.Invocation);
invocation->getPreprocessorOpts().DisablePCHOrModuleValidation =
clang::DisableValidationForModuleKind::None;
invocation->getPreprocessorOpts().AllowPCHWithCompilerErrors = false;
invocation->getHeaderSearchOpts().ModulesValidateSystemHeaders = true;
invocation->getLangOpts()->NeededByPCHOrCompilationUsesPCH = true;
invocation->getLangOpts()->CacheGeneratedPCH = true;
// If the underlying invocation is allowing PCH errors, then it "can be read",
// even if it has its error bit set. Thus, don't override
// `AllowPCHWithCompilerErrors`.

// ClangImporter::create adds a remapped MemoryBuffer that we don't need
// here. Moreover, it's a raw pointer owned by the preprocessor options; if
Expand Down Expand Up @@ -1365,7 +1367,8 @@ bool ClangImporter::Implementation::importHeader(
// Don't even try to load the bridging header if the Clang AST is in a bad
// state. It could cause a crash.
auto &clangDiags = getClangASTContext().getDiagnostics();
if (clangDiags.hasUnrecoverableErrorOccurred())
if (clangDiags.hasUnrecoverableErrorOccurred() &&
!getClangInstance()->getPreprocessorOpts().AllowPCHWithCompilerErrors)
return true;

assert(adapter);
Expand Down Expand Up @@ -1465,7 +1468,8 @@ bool ClangImporter::Implementation::importHeader(
getBufferImporterForDiagnostics());

// FIXME: What do we do if there was already an error?
if (!hadError && clangDiags.hasErrorOccurred()) {
if (!hadError && clangDiags.hasErrorOccurred() &&
!getClangInstance()->getPreprocessorOpts().AllowPCHWithCompilerErrors) {
diagnose(diagLoc, diag::bridging_header_error, headerName);
return true;
}
Expand Down Expand Up @@ -1668,7 +1672,8 @@ ClangImporter::emitBridgingPCH(StringRef headerPath,
FrontendOpts, std::make_unique<clang::GeneratePCHAction>());
emitInstance->ExecuteAction(*action);

if (emitInstance->getDiagnostics().hasErrorOccurred()) {
if (emitInstance->getDiagnostics().hasErrorOccurred() &&
!emitInstance->getPreprocessorOpts().AllowPCHWithCompilerErrors) {
Impl.diagnose({}, diag::bridging_header_pch_error,
outputPCHPath, headerPath);
return true;
Expand Down Expand Up @@ -1728,7 +1733,8 @@ bool ClangImporter::emitPrecompiledModule(StringRef moduleMapPath,
std::make_unique<clang::GenerateModuleFromModuleMapAction>());
emitInstance->ExecuteAction(*action);

if (emitInstance->getDiagnostics().hasErrorOccurred()) {
if (emitInstance->getDiagnostics().hasErrorOccurred() &&
!FrontendOpts.AllowPCMWithCompilerErrors) {
Impl.diagnose({}, diag::emit_pcm_error, outputPath, moduleMapPath);
return true;
}
Expand Down
13 changes: 8 additions & 5 deletions lib/FrontendTool/FrontendTool.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1813,19 +1813,22 @@ createJSONFixItDiagnosticConsumerIfNeeded(

/// A PrettyStackTraceEntry to print frontend information useful for debugging.
class PrettyStackTraceFrontend : public llvm::PrettyStackTraceEntry {
const LangOptions &LangOpts;
const CompilerInvocation &Invocation;

public:
PrettyStackTraceFrontend(const LangOptions &langOpts)
: LangOpts(langOpts) {}
PrettyStackTraceFrontend(const CompilerInvocation &invocation)
: Invocation(invocation) {}

void print(llvm::raw_ostream &os) const override {
auto effective = LangOpts.EffectiveLanguageVersion;
auto effective = Invocation.getLangOptions().EffectiveLanguageVersion;
if (effective != version::Version::getCurrentLanguageVersion()) {
os << "Compiling with effective version " << effective;
} else {
os << "Compiling with the current language version";
}
if (Invocation.getFrontendOptions().AllowModuleWithCompilerErrors) {
os << " while allowing modules with compiler errors";
}
os << "\n";
};
};
Expand Down Expand Up @@ -1936,7 +1939,7 @@ int swift::performFrontend(ArrayRef<const char *> Args,
/// (leaks checking is not thread safe).
Invocation.getSILOptions().checkSILModuleLeaks = true;

PrettyStackTraceFrontend frontendTrace(Invocation.getLangOptions());
PrettyStackTraceFrontend frontendTrace(Invocation);

// Make an array of PrettyStackTrace objects to dump the configuration files
// we used to parse the arguments. These are RAII objects, so they and the
Expand Down
29 changes: 29 additions & 0 deletions test/ClangImporter/AllowErrors/invalid-pch-bridging-header.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
// REQUIRES: objc_interop

// RUN: %empty-directory(%t)
// RUN: mkdir -p %t/pch %t/pch-dir
// RUN: split-file %s %t

// Check that the pch is output even though it has errors
// RUN: %target-swift-frontend -emit-pch -o %t/pch/bridging-header.pch -Xcc -Xclang -Xcc -fallow-pcm-with-compiler-errors -Xcc -Xclang -Xcc -fmodule-format=raw %t/bridging-header.h
// RUN: %target-swift-frontend -typecheck -verify -import-objc-header %t/pch/bridging-header.pch -Xcc -Xclang -Xcc -fallow-pcm-with-compiler-errors -Xcc -Xclang -Xcc -fmodule-format=raw %t/use.swift
// RUN: ls %t/pch/*.pch | count 1

// Same but with implicit PCH instead
// RUN: %target-swift-frontend -typecheck -verify -import-objc-header %t/bridging-header.h -pch-output-dir %t/pch-dir -Xcc -Xclang -Xcc -fallow-pcm-with-compiler-errors -Xcc -Xclang -Xcc -fmodule-format=raw %t/use.swift
// RUN: ls %t/pch-dir/*.pch | count 1

// Second implicit run since we may go down a different path if the PCH already
// exists
// RUN: %target-swift-frontend -typecheck -verify -import-objc-header %t/bridging-header.h -pch-output-dir %t/pch-dir -Xcc -Xclang -Xcc -fallow-pcm-with-compiler-errors -Xcc -Xclang -Xcc -fmodule-format=raw %t/use.swift
// RUN: ls %t/pch-dir/*.pch | count 1

//--- bridging-header.h
@import DoesNotExist;

struct SomeTy {
int a;
};

//--- use.swift
func use(s: SomeTy) {}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it'd be good to add that back. The program arguments are often truncated and always difficult to visually parse.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

21 changes: 21 additions & 0 deletions test/ClangImporter/AllowErrors/invalid-pcm.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
// RUN: %empty-directory(%t)
// RUN: split-file %s %t

// RUN: %target-swift-emit-pcm -module-name m -o %t/m.pcm -Xcc -Xclang -Xcc -fallow-pcm-with-compiler-errors -Xcc -Xclang -Xcc -fmodule-format=raw %t/mods/module.map
// RUN: %target-swift-frontend -typecheck -verify -Xcc -Xclang -Xcc -fallow-pcm-with-compiler-errors -Xcc -fmodule-file=%t/m.pcm %t/use.swift

//--- mods/module.map
module m {
header "m.h"
}

//--- mods/m.h
@import DoesNotExist;

struct SomeTy {
int a;
};

//--- use.swift
import m
func use(s: SomeTy) {}
7 changes: 3 additions & 4 deletions test/Frontend/crash.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
// RUN: not --crash %target-swift-frontend -typecheck -debug-crash-after-parse -filelist %t.filelist.txt 2>&1 | %FileCheck %s

// Check that we see the contents of the input file list in the crash log.
// CHECK-NOT: Compiling with {{.*}} while allowing modules with compiler errors enabled
// CHECK-NOT: while allowing modules with compiler errors
// CHECK-LABEL: Stack dump
// CHECK-NEXT: Program arguments: {{.*swift(-frontend)?(c?)(\.exe)?}}
// CHECK-NEXT: Swift version
Expand All @@ -14,12 +14,11 @@

// RUN: not --crash %target-swift-frontend -typecheck -debug-crash-after-parse -experimental-allow-module-with-compiler-errors %s 2>&1 | %FileCheck -check-prefix CHECK-ALLOW %s
// CHECK-ALLOW: Program arguments: {{.*}} -experimental-allow-module-with-compiler-errors
// CHECK-ALLOW: Compiling with effective version
// CHECK-ALLOW: Compiling with effective version {{.*}} while allowing modules with compiler errors

// RUN: not --crash %target-swift-frontend -typecheck -debug-crash-after-parse -experimental-allow-module-with-compiler-errors -swift-version 5 %s 2>&1 | %FileCheck -check-prefix CHECK-CURRENT %s
// CHECK-CURRENT: Program arguments: {{.*}} -experimental-allow-module-with-compiler-errors
// CHECK-CURRENT: Compiling with the current language version
// CHECK-CURRENT: Compiling with the current language version while allowing modules with compiler errors

func anchor() {}
anchor()