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

tapthaker · 2018-06-10T07:04:26Z

This PR adds a diagnostic message when a .swiftmodule is found but doesn't have the same cpu architecture as the target module. It would emit message such as:
could not find module '<module_name>' for architecture 'x86_64'

Resolves SR-7160.

…ure is not found

tapthaker · 2018-06-10T07:08:24Z

@jrose-apple : Trying to fix the SR-7160 as we discussed in #TrySwift

beccadax · 2018-06-10T11:14:34Z

test/Serialization/load-invalid-arch.swift

+// RUN: touch %t/new_module.swiftmodule/arm64.swiftmodule
+// RUN: %target-swift-frontend %s -typecheck -I %t -verify -show-diagnostics-after-fatal
+
+import new_module // expected-error {{no such module 'new_module'}} expected-error {{could not find module 'new_module' for architecture 'x86_64'}}


So this one statement will emit two different errors? I don't think that's usually how we do things. Maybe the statement identifying the architecture should be a note instead, or maybe the generic "no such module" error should be omitted and only the architecture one should be shown.

@brentdax : First off, nice meeting you @ tryswift :)
Makes sense to not emit 2 errors, however the way code is currently structured it is difficult to not emit the no such module 'new_module' error. That would need some level of refactoring.

Will try emitting note instead.

beccadax · 2018-06-10T11:20:51Z

lib/Serialization/SerializedModuleLoader.cpp

@@ -122,6 +124,13 @@ findModule(ASTContext &ctx, AccessPathElem moduleID,
                            archFile.str(), archDocFile.str(),
                            moduleBuffer, moduleDocBuffer,
                            scratch);
+
+      if(err == std::errc::no_such_file_or_directory) {
+        ctx.Diags.diagnose(moduleID.second, diag::sema_no_import_arch,


Possibly a pie-in-the-sky request, but is there some way we could identify and list the architectures which are available? That might help someone go "Oh, duh, I built the iOS one, not the Mac one".

You could enumerate the files under the swiftmodule's directory and do a bit of string processing to emit some notes. The cost of the enumeration should be fairly low, but the results should be cached so we keep the failure path zippy.

…with it

CodaFi · 2018-06-11T05:49:48Z

lib/Serialization/SerializedModuleLoader.cpp

+  for (; directoryIterator != endIterator; directoryIterator.increment(errorCode)) {
+    auto entry = *directoryIterator;
+    llvm::StringRef filePath(entry.path());
+    foundArchs = foundArchs + (foundArchs.length() > 0 ? ", " : "")


A swift module contains swiftdoc files as well. This will pick those up and double-diagnose.

True. Let me add a check for that !! And update the test accordingly.

tapthaker · 2018-06-11T05:51:44Z

lib/Serialization/SerializedModuleLoader.cpp

+
+  ctx.Diags.diagnose(sourceLocation, diag::sema_no_import_arch,
+                     moduleName, archName, foundArchs);
+}


@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

How do I cached the foundArchs for a module ?

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

jrose-apple

This is a great start, Tapan! I have a bunch of comments that hopefully aren't too burdensome. I do share the concern about emitting two separate messages, but that's something that already happens today with Clang modules that can't be loaded, so I think it's okay to deal with that in a separate SR. This is still an improvement over what's there today.

jrose-apple · 2018-06-11T16:15:04Z

lib/Serialization/SerializedModuleLoader.cpp

+                                               currPath);
+    }
+
+    return false;


I think this was supposed to go inside your new if statement, right?

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

The following line should suffice

if (!err) return true;

jrose-apple · 2018-06-11T16:15:59Z

include/swift/AST/DiagnosticsSema.def

@@ -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,
+      "could not find module '%0' for architecture '%1' "
+      "found: %2", (StringRef, StringRef, StringRef))


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

@brentdax's suggestion 😄

jrose-apple · 2018-06-11T16:16:58Z

include/swift/AST/DiagnosticsSema.def

@@ -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,


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.

jrose-apple · 2018-06-11T16:19:20Z

lib/Serialization/SerializedModuleLoader.cpp

+                             SourceLoc sourceLocation,
+                             llvm::SmallString<64> moduleName,
+                             llvm::SmallString<16> archName,
+                             llvm::SmallString<128> directoryPath) {


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.

jrose-apple · 2018-06-11T16:19:46Z

lib/Serialization/SerializedModuleLoader.cpp

+                                                      true);
+  llvm::sys::fs::directory_iterator endIterator;
+
+  if(errorCode) {


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

jrose-apple · 2018-06-11T16:21:08Z

lib/Serialization/SerializedModuleLoader.cpp

+    }
+
+  std::string foundArchs;
+  for (; directoryIterator != endIterator; directoryIterator.increment(errorCode)) {


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

jrose-apple · 2018-06-11T16:21:24Z

lib/Serialization/SerializedModuleLoader.cpp

+    auto entry = *directoryIterator;
+    llvm::StringRef filePath(entry.path());
+    llvm::StringRef extension = llvm::sys::path::extension(filePath);
+    if(extension.startswith(".") &&


Nitpick: Space after if again.

jrose-apple · 2018-06-11T16:22:12Z

lib/Serialization/SerializedModuleLoader.cpp

+  for (; directoryIterator != endIterator; directoryIterator.increment(errorCode)) {
+    auto entry = *directoryIterator;
+    llvm::StringRef filePath(entry.path());
+    llvm::StringRef extension = llvm::sys::path::extension(filePath);


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

jrose-apple · 2018-06-11T16:23:24Z

lib/Serialization/SerializedModuleLoader.cpp

+    if(extension.startswith(".") &&
+       extension.drop_front() == SERIALIZED_MODULE_EXTENSION) {
+        foundArchs = foundArchs + (foundArchs.length() > 0 ? ", " : "")
+                                + llvm::sys::path::stem(filePath).str();


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.

test/Serialization/load-invalid-arch.swift

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


jrose-apple · 2018-06-11T16:33:57Z

(In particular, I still think the right answer in this case is for SerializedModuleLoader to realize it found something it couldn't load and return an empty ModuleDecl, like it does for invalid modules later on.)

tapthaker · 2018-06-12T05:54:43Z

(In particular, I still think the right answer in this case is for SerializedModuleLoader to realize it found something it couldn't load and return an empty ModuleDecl, like it does for invalid modules later on.)

I agree, just confirming that we can implement this feature as a separate PR, right ?

jrose-apple · 2018-06-12T15:56:06Z

test/Serialization/load-invalid-arch.swift

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


You can drop the -I %t here.

jrose-apple · 2018-06-12T15:56:16Z

lib/Serialization/SerializedModuleLoader.cpp

-    }
-
-    return false;
+      if(err == std::errc::no_such_file_or_directory) {


(still missing a space here)

jrose-apple · 2018-06-12T15:56:51Z

lib/Serialization/SerializedModuleLoader.cpp

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


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

tapthaker · 2018-06-12T17:14:42Z

@jrose-apple : Made the required changes.

Also, is there a linter that we could use for our cpp code ? Don't want you folks to spend time in pointing out the obvious styling issues.

jrose-apple · 2018-06-13T00:02:52Z

No one's set up a linter but running your changes through clang-format would get the right style. It's a good idea, though.

jrose-apple · 2018-06-13T00:04:07Z

@swift-ci Please test

swift-ci · 2018-06-13T00:05:47Z

Build failed
Swift Test OS X Platform
Git Sha - ea3a9f4

swift-ci · 2018-06-13T00:05:49Z

Build failed
Swift Test Linux Platform
Git Sha - ea3a9f4

tapthaker · 2018-06-13T01:39:03Z

Got the reason why the load-invalid-arch.swift test failed:
Expected:
could not find module 'new_module' for architecture 'x86_64' found: powerpc64, arm64
Got:
could not find module 'new_module' for architecture 'i386' found: powerpc64, arm64

I hardcoded the assertion for my computer. Will see if there is a template string for this.

tapthaker · 2018-06-13T02:54:27Z

Feels like a bit indirect though

jrose-apple · 2018-06-13T16:33:26Z

test/Serialization/load-invalid-arch.swift.gyb.swift

+// RUN: %gyb -DTARGET_ARCHITECTURE=%target-cpu %s -o %t/load-invalid-arc.swift
+// RUN: %target-swift-frontend %t/load-invalid-arc.swift -F %t -typecheck -verify -show-diagnostics-after-fatal
+
+import new_module // expected-error {{no such module 'new_module'}} expected-error {{could not find module 'new_module' for architecture '${TARGET_ARCHITECTURE}' found: powerpc64, arm64}}


This seems reasonable. Another approach would be to give up on -verify diagnostics and instead pipe the output into %FileCheck.

One note, though: we actually do run these tests for arm64 targets too, and someone could try to run them for powerpc64. It might be better to use obviously invalid architecture names instead.

jrose-apple · 2018-06-13T16:34:18Z

include/swift/AST/DiagnosticsSema.def

@@ -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' "


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

tapthaker · 2018-06-13T17:01:21Z

Makes sense, adding non-existing arch : i387 & ppc65

jrose-apple · 2018-06-13T17:33:35Z

test/Serialization/load-invalid-arch.swift.gyb.swift

+// RUN: %gyb -DTARGET_ARCHITECTURE=%target-cpu %s -o %t/load-invalid-arc.swift
+// RUN: %target-swift-frontend %t/load-invalid-arc.swift -F %t -typecheck -verify -show-diagnostics-after-fatal
+
+import new_module // expected-error {{no such module 'new_module'}} expected-error {{could not find module 'new_module' for architecture '${TARGET_ARCHITECTURE}'; found: ppc65, i387}}


Ooh, just thought of another issue: the order of files returned by directory_iterator probably isn't guaranteed. We might need to switch to FileCheck and a regex match after all.

(Alternately, you could collect the names in a SmallVector and then llvm::sort them.)

Doesn't make sense in sorting them. Will switch to FileCheck instead.

…lues of CPU architectures

tapthaker · 2018-06-13T20:16:34Z

Learnt a lot about testing in Swift Complier with this PR

jrose-apple

All right, this is pretty good. Let's take it! Thanks for sticking with this through all the unexpected pitfalls and detours. :-)

jrose-apple · 2018-06-14T16:17:40Z

@swift-ci Please test

swift-ci · 2018-06-14T16:19:32Z

Build failed
Swift Test OS X Platform
Git Sha - 4b9262e

tapthaker · 2018-06-14T16:41:07Z

Thank you for helping me out along the way. Standby for more PRs :-)

Last question: Why do we get Build failed even when the tests are still running ?
Super confusing to me !!

jrose-apple · 2018-06-14T17:45:58Z

There's a GitHub/Jenkins plugin bug where it presents the wrong commit after a force-push. We've added code to check for that circumstance and automatically restart the build in that case. You can see that if you click through into the failed build log, but it's long been on @shahmishal's queue to expose that more visibly.

tapthaker · 2018-06-18T23:54:45Z

@jrose-apple: I think we can now merge this to master

jrose-apple · 2018-06-19T00:41:08Z

Oops, sorry I let this slip! Thanks so much for working on this!

jrose-apple · 2018-06-19T00:41:57Z

Ah, I didn't exactly mean to do Squash and Merge, but it shouldn't harm anything.

tapthaker · 2018-06-19T03:24:40Z

I was expecting you to Squash it 😄 . Thanks.

Print proper error message when the swiftmodule for correct architect…

ea3a9f4

…ure is not found

beccadax reviewed Jun 10, 2018

View reviewed changes

Make the diagnostic message at a Note and append found architectures …

eebc8de

…with it

tapthaker force-pushed the master branch from f0b5c20 to eebc8de Compare June 11, 2018 05:41

CodaFi reviewed Jun 11, 2018

View reviewed changes

tapthaker commented Jun 11, 2018

View reviewed changes

Ensure that the foundArchs error only contain .swiftmodule files

00b8f82

jrose-apple reviewed Jun 11, 2018

View reviewed changes

Fix nits and implement the print error for frameworks as well

f15cae9

jrose-apple reviewed Jun 12, 2018

View reviewed changes

tapthaker added 3 commits June 12, 2018 10:03

Short circuit and return if there is an architecture mismatch

4b32103

Minor formatting - space after if

f850c53

Remove import search path for load-invalid-arch framework test

4b9262e

Make load-invalid-arch test pass for all target cpu

b6eb1d5

jrose-apple reviewed Jun 13, 2018

View reviewed changes

Use non-existing achitectures for the test to make it more robust

3e938a9

jrose-apple reviewed Jun 13, 2018

View reviewed changes

Use file-check instead of -verify diagnostics to assert on dynamic va…

6f96bc8

…lues of CPU architectures

jrose-apple approved these changes Jun 14, 2018

View reviewed changes

jrose-apple merged commit 359385b into swiftlang:master Jun 19, 2018

swift-ci mentioned this pull request Jun 19, 2018

[SR-7160] Error if Foo.swiftmodule/ exists but not with the correct architecture #49708

Closed

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

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

Uh oh!

Conversation

tapthaker commented Jun 10, 2018

Uh oh!

tapthaker commented Jun 10, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jrose-apple left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment was marked as resolved.

Uh oh!

jrose-apple commented Jun 11, 2018

Uh oh!

tapthaker commented Jun 12, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jrose-apple Jun 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tapthaker commented Jun 12, 2018

Uh oh!

jrose-apple commented Jun 13, 2018

Uh oh!

jrose-apple commented Jun 13, 2018

Uh oh!

swift-ci commented Jun 13, 2018

Uh oh!

swift-ci commented Jun 13, 2018

Uh oh!

tapthaker commented Jun 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tapthaker commented Jun 13, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jrose-apple Jun 12, 2018 •

edited

Loading

tapthaker commented Jun 13, 2018 •

edited

Loading