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.

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 10 commits into from
Jun 19, 2018
Merged

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

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
ea3a9f4
Print proper error message when the swiftmodule for correct architect…
tapthaker Jun 10, 2018
eebc8de
Make the diagnostic message at a Note and append found architectures …
tapthaker Jun 11, 2018
00b8f82
Ensure that the foundArchs error only contain .swiftmodule files
tapthaker Jun 11, 2018
f15cae9
Fix nits and implement the print error for frameworks as well
tapthaker Jun 12, 2018
4b32103
Short circuit and return if there is an architecture mismatch
tapthaker Jun 12, 2018
f850c53
Minor formatting - space after if
tapthaker Jun 12, 2018
4b9262e
Remove import search path for load-invalid-arch framework test
tapthaker Jun 12, 2018
b6eb1d5
Make load-invalid-arch test pass for all target cpu
tapthaker Jun 13, 2018
3e938a9
Use non-existing achitectures for the test to make it more robust
tapthaker Jun 13, 2018
6f96bc8
Use file-check instead of -verify diagnostics to assert on dynamic va…
tapthaker Jun 13, 2018
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
View file
Open in desktop
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))
NOTE(sema_no_import_arch,Fatal,
Copy link
Contributor

Choose a reason for hiding this comment

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

This part isn't going to work, unfortunately. Notes always have to be attached to an error or warning, but one hasn't been emitted yet in this case. You'll have to make this an error on its own.

"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
51 changes: 48 additions & 3 deletions lib/Serialization/SerializedModuleLoader.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,12 +81,46 @@ openModuleFiles(StringRef DirName, StringRef ModuleFilename,
return std::error_code();
}

static void addDiagnosticInfoForArchitectureMismatch(ASTContext &ctx,
SourceLoc sourceLocation,
llvm::SmallString<64> moduleName,
llvm::SmallString<16> archName,
llvm::SmallString<128> directoryPath) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Style notes:

  • For parameters that would break the 80-column rule, please double-indent them from the left, rather than right-aligning them. (You can also use clang-format on the code you changed to match our style if you want to set that up.)
  • Since you aren't planning to modify any of these strings, the StringRef type is probably a better choice. There's an implicit conversion from SmallString and then the strings wouldn't get copied when you call the function.


std::error_code errorCode;
llvm::Twine twineDirPath(directoryPath);
Copy link
Contributor

Choose a reason for hiding this comment

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

Twine also has a bunch of implicit conversions, so you should be able to pass directoryPath directly to the iterator constructor.

llvm::sys::fs::directory_iterator directoryIterator(twineDirPath,
errorCode,
true);
llvm::sys::fs::directory_iterator endIterator;

if(errorCode) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Style nitpick: please put a space after if, and check the indentation of the following lines!

return;
}

std::string foundArchs;
for (; directoryIterator != endIterator; directoryIterator.increment(errorCode)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

You should probably stop the loop if there's ever an error, too, even if it means having to emit a more generic diagnostic.

auto entry = *directoryIterator;
llvm::StringRef filePath(entry.path());
llvm::StringRef extension = llvm::sys::path::extension(filePath);
Copy link
Contributor

Choose a reason for hiding this comment

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

Nitpick: StringRef and a handful of other types from the LLVM namespace are so common that it's conventional not to use llvm:: with them in Swift code. You can see the list of such types in the "swift/Basic/LLVM.h" header.

(You'll notice SmallString is in this list too, so clearly we're not perfectly consistent about it ourselves. But StringRef is super common.)

if(extension.startswith(".") &&
Copy link
Contributor

Choose a reason for hiding this comment

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

Nitpick: Space after if again.

extension.drop_front() == SERIALIZED_MODULE_EXTENSION) {
foundArchs = foundArchs + (foundArchs.length() > 0 ? ", " : "")
+ llvm::sys::path::stem(filePath).str();
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's okay that this isn't very efficient. It's not a diagnostic we expect people to hit very often, and failing to import something means compilation stops early anyway.

}
}

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 +130,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 +157,16 @@ findModule(ASTContext &ctx, AccessPathElem moduleID,
archFile.str(), archDocFile.str(),
moduleBuffer, moduleDocBuffer,
scratch);

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

return false;
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 this was supposed to go inside your new if statement, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

actually I don't need the return false; statement.

The following line should suffice

if (!err)
  return true;

}
if (!err)
return true;
Expand Down
12 changes: 12 additions & 0 deletions test/Serialization/load-invalid-arch.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@

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

Sign in to view


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

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