Skip to content

Print proper error message when the swiftmodule for architecture not found #17092

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.

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 10 commits into from
Jun 19, 2018
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
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
3 changes: 3 additions & 0 deletions include/swift/AST/DiagnosticsSema.def
Original file line number Diff line number Diff line change
Expand Up @@ -562,6 +562,9 @@ ERROR(cannot_return_value_from_void_func,none,

ERROR(sema_no_import,Fatal,
"no such module '%0'", (StringRef))
ERROR(sema_no_import_arch,Fatal,
"could not find module '%0' for architecture '%1' "
Copy link
Contributor

Choose a reason for hiding this comment

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

Nitpick: can you add a semicolon after the '%1' here?

"found: %2", (StringRef, StringRef, StringRef))
Copy link
Contributor

Choose a reason for hiding this comment

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

I like this touch of including the architectures that are present! Good thinking.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@brentdax's suggestion 😄

ERROR(sema_no_import_repl,none,
"no such module '%0'", (StringRef))
NOTE(sema_no_import_no_sdk,none,
Expand Down
64 changes: 59 additions & 5 deletions lib/Serialization/SerializedModuleLoader.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -81,12 +81,48 @@ openModuleFiles(StringRef DirName, StringRef ModuleFilename,
return std::error_code();
}

static void addDiagnosticInfoForArchitectureMismatch(ASTContext &ctx,
SourceLoc sourceLocation,
StringRef moduleName,
StringRef archName,
StringRef directoryPath) {

std::error_code errorCode;
llvm::sys::fs::directory_iterator directoryIterator(directoryPath, errorCode,
true);
llvm::sys::fs::directory_iterator endIterator;

if (errorCode) {
return;
}

std::string foundArchs;
for (; directoryIterator != endIterator;
directoryIterator.increment(errorCode)) {
if (errorCode) {
return;
}
auto entry = *directoryIterator;
StringRef filePath(entry.path());
StringRef extension = llvm::sys::path::extension(filePath);
if (extension.startswith(".") &&
extension.drop_front() == SERIALIZED_MODULE_EXTENSION) {
foundArchs = foundArchs + (foundArchs.length() > 0 ? ", " : "") +
llvm::sys::path::stem(filePath).str();
}
}

ctx.Diags.diagnose(sourceLocation, diag::sema_no_import_arch, moduleName,
archName, foundArchs);
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@brentdax @CodaFi : I see 2 problems with the current implementation:

foundArchs = foundArchs + (foundArchs.length() > 0 ? ", " : "")
                        + llvm::sys::path::stem(filePath).str();

might not be the most optimal was of appending strings

  1. How do I cached the foundArchs for a module ?

Pretty new to the codebase, so any suggestions will be helpful.


static bool
findModule(ASTContext &ctx, AccessPathElem moduleID,
std::unique_ptr<llvm::MemoryBuffer> *moduleBuffer,
std::unique_ptr<llvm::MemoryBuffer> *moduleDocBuffer,
bool &isFramework) {
llvm::SmallString<64> moduleFilename(moduleID.first.str());
llvm::SmallString<64> moduleName(moduleID.first.str());
llvm::SmallString<64> moduleFilename(moduleName);
moduleFilename += '.';
moduleFilename += SERIALIZED_MODULE_EXTENSION;

Expand All @@ -96,9 +132,10 @@ findModule(ASTContext &ctx, AccessPathElem moduleID,

// FIXME: Which name should we be using here? Do we care about CPU subtypes?
// FIXME: At the very least, don't hardcode "arch".
llvm::SmallString<16> archFile{
llvm::SmallString<16> archName{
ctx.LangOpts.getPlatformConditionValue(PlatformConditionKind::Arch)};
llvm::SmallString<16> archDocFile{archFile};
llvm::SmallString<16> archFile{archName};
llvm::SmallString<16> archDocFile{archName};
if (!archFile.empty()) {
archFile += '.';
archFile += SERIALIZED_MODULE_EXTENSION;
Expand All @@ -122,6 +159,11 @@ findModule(ASTContext &ctx, AccessPathElem moduleID,
archFile.str(), archDocFile.str(),
moduleBuffer, moduleDocBuffer,
scratch);

if(err == std::errc::no_such_file_or_directory) {
Copy link
Contributor

Choose a reason for hiding this comment

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

(still missing a space here)

addDiagnosticInfoForArchitectureMismatch(
ctx, moduleID.second, moduleName, archName, currPath);
Copy link
Contributor

@jrose-apple jrose-apple Jun 12, 2018

Choose a reason for hiding this comment

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

Hm. I think we still do need to exit early, though (here and below). Otherwise we could go on to a different search path and find a different module with the same name.

(Yes, it's still possible for a different module loader to find another module with the same name right now, but I think it's okay not to worry about that at the moment.)

}
}
if (!err)
return true;
Expand All @@ -134,12 +176,24 @@ findModule(ASTContext &ctx, AccessPathElem moduleID,

auto tryFrameworkImport = [&](StringRef frameworkPath) -> bool {
currPath = frameworkPath;
llvm::sys::path::append(currPath, moduleFramework.str(),
llvm::sys::path::append(currPath, moduleFramework.str());
// Check if the framework directory exists
if (!llvm::sys::fs::is_directory(currPath)) {
return false;
}

llvm::sys::path::append(currPath,
"Modules", moduleFilename.str());
auto err = openModuleFiles(currPath,
archFile.str(), archDocFile.str(),
moduleBuffer, moduleDocBuffer,
scratch);

if (err == std::errc::no_such_file_or_directory) {
addDiagnosticInfoForArchitectureMismatch(
ctx, moduleID.second, moduleName, archName, currPath);
}

return !err;
};

Expand Down Expand Up @@ -642,4 +696,4 @@ SerializedASTFile::getDiscriminatorForPrivateValue(const ValueDecl *D) const {
Identifier discriminator = File.getDiscriminatorForPrivateValue(D);
assert(!discriminator.empty() && "no discriminator found for value");
return discriminator;
}
}
18 changes: 18 additions & 0 deletions test/Serialization/load-invalid-arch.swift
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@

// RUN: %empty-directory(%t)
// RUN: mkdir %t/new_module.swiftmodule
// RUN: touch %t/new_module.swiftmodule/arm64.swiftmodule
// RUN: touch %t/new_module.swiftmodule/powerpc64.swiftmodule
// RUN: touch %t/new_module.swiftmodule/arm64.swiftdoc
// RUN: touch %t/new_module.swiftmodule/powerpc64.swiftdoc
// RUN: %target-swift-frontend %s -typecheck -I %t -verify -show-diagnostics-after-fatal

This comment was marked as resolved.


// RUN: %empty-directory(%t)
// RUN: mkdir -p %t/new_module.framework/Modules/new_module.swiftmodule/
// RUN: touch %t/new_module.framework/Modules/new_module.swiftmodule/powerpc64.swiftmodule
// RUN: touch %t/new_module.framework/Modules/new_module.swiftmodule/arm64.swiftmodule
// RUN: %target-swift-frontend %s -F %t -typecheck -I %t -verify -show-diagnostics-after-fatal
Copy link
Contributor

Choose a reason for hiding this comment

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

You can drop the -I %t here.


import new_module // expected-error {{no such module 'new_module'}} expected-error {{could not find module 'new_module' for architecture 'x86_64' found: powerpc64, arm64}}

new_module.foo() // expected-error {{use of unresolved identifier 'new_module'}}