[ClangImporter] Load the stdlib even if there is a `-fmodule-map-file` argument pointing to a missing file #37252

ahoppen · 2021-05-04T18:50:38Z

If there is a -fmodule-map-file argument whose file doesn’t exist and SwiftShims is not in the module cache, we fail to build it, because clang throws an error about the missing module map. This causes SourceKit to drop all semantic functionality, even if the missing module map isn’t required.

To work around this, drop all -fmodule-map-file arguments with missing files from the clang importer’s arguments, reporting the eror that clang would throw manually.

Fixes rdar://77449671

akyrtzi · 2021-05-04T19:04:35Z

lib/ClangImporter/ClangImporter.cpp

+  std::vector<std::string> FilteredModuleMapFiles;
+  for (auto ModuleMapFile : CI->getFrontendOpts().ModuleMapFiles) {
+    llvm::sys::fs::file_status status;
+    (void)llvm::sys::fs::status(ModuleMapFile, status);


Could you look into using the VFS file system instead of checking directly from the file system?
For reference see here where it gets used: https://github.com/apple/swift/blob/main/lib/ClangImporter/ClangImporter.cpp#L1089

Updated it to use a VFS, although it’s a bit of a chicken-and-egg problem. We need the compiler invocation to get the VFS and need the VFS to construct the compiler invocation. But I think, it should be fine since the compiler invocation should have everything in there when we generate the VFS except the filtering of the module maps and that shouldn’t affect the VFS AFAICT.

nkcsgexi

Can we sanitize these arguments from invocationArgStrs so explicit module builds could also benefit from it?

ahoppen · 2021-05-05T13:20:56Z

Can we sanitize these arguments from invocationArgStrs so explicit module builds could also benefit from it?

@nkcsgexi I would very much like not to do that because it would mean that we’d need to parse the -fmodule-map-file ourselves. At the moment it’s only supported as -fmodule-map-file=some/file, but should that change we’d also need to maintain our custom parser logic.

What exactly are you thinking about that is using the invocationArgStrs?

nkcsgexi · 2021-05-05T14:11:09Z

Can we sanitize these arguments from invocationArgStrs so explicit module builds could also benefit from it?

@nkcsgexi I would very much like not to do that because it would mean that we’d need to parse the -fmodule-map-file ourselves. At the moment it’s only supported as -fmodule-map-file=some/file, but should that change we’d also need to maintain our custom parser logic.

What exactly are you thinking about that is using the invocationArgStrs?

We use the arguments in invocationArgStrs to construct a compiler invocation that builds pcm files explicitly, but I guess we can revisit when we actually hit the same issue in that scenario.

akyrtzi · 2021-05-05T20:28:16Z

We use the arguments in invocationArgStrs to construct a compiler invocation that builds pcm files explicitly, but I guess we can revisit when we actually hit the same issue in that scenario.

Couldn't the clang importer of the "build a pcm" invocation also sanitize the FrontendOpts similar to Alex's change?
It may even be the case that Alex's change covers this case, though I'm not sure if it follows same code path.

Ah, that's a good point. The "build pcm" invocation should also go through Alex's change so we can get the same sanitization under the hood. The only missing part is that build log still shows these sanitized arguments.

akyrtzi · 2021-05-05T20:31:34Z

lib/ClangImporter/ClangImporter.cpp

+  // resilient and provide a module even if there were building it.
+  std::vector<std::string> FilteredModuleMapFiles;
+  for (auto ModuleMapFile : CI->getFrontendOpts().ModuleMapFiles) {
+    auto VFS = clang::createVFSFromCompilerInvocation(


I'd suggest to move VFS construction out of the loop, since you don't need a new one for each module map file.

Oh, yeah. Good idea. Thanks!

nkcsgexi · 2021-05-05T20:36:54Z

We use the arguments in invocationArgStrs to construct a compiler invocation that builds pcm files explicitly, but I guess we can revisit when we actually hit the same issue in that scenario.

Couldn't the clang importer of the "build a pcm" invocation also sanitize the FrontendOpts similar to Alex's change?
It may even be the case that Alex's change covers this case, though I'm not sure if it follows same code path.

(I meant to reply) Ah, that's a good point. The "build pcm" invocation should also go through Alex's change so we can get the same sanitization under the hood. The only missing part is that build log still shows these sanitized arguments.

akyrtzi · 2021-05-05T22:53:29Z

The only missing part is that build log still shows these sanitized arguments.

Could you clarify what you mean? They way I understand it is there will be an error in the build log about a missing module-map file, at which point the build will error out. In the build log you will see the failing invocation including the offending -fmodule-map-file=some/file argument but that's as expected.

nkcsgexi · 2021-05-05T23:18:29Z

The only missing part is that build log still shows these sanitized arguments.

Could you clarify what you mean? They way I understand it is there will be an error in the build log about a missing module-map file, at which point the build will error out. In the build log you will see the failing invocation including the offending -fmodule-map-file=some/file argument but that's as expected.

That's my understanding as well. The compiler invocation will fail no matter whether we sanitize those non-existing module map arguments or not. However, that loops back to the original question why the driver passes down these flags in the first place.

akyrtzi · 2021-05-05T23:57:44Z

However, that loops back to the original question why the driver passes down these flags in the first place.

Normally with a regular build the build system will have the module maps in place before invoking the compiler, but when using sourcekitd for editor use we may use the compiler arguments from the build system ASAP, before the module maps are in place.

In any case, it should be the underlying system's responsibility to report an error and recover when a particular argument relevant to that system is problematic, the driver should not be concerned with sanitizing every flag that is given to it.

…` argument pointing to a missing file If there is a `-fmodule-map-file` argument whose file doesn’t exist and SwiftShims is not in the module cache, we fail to build it, because clang throws an error about the missing module map. This causes SourceKit to drop all semantic functionality, even if the missing module map isn’t required. To work around this, drop all `-fmodule-map-file` arguments with missing files from the clang importer’s arguments, reporting the eror that `clang` would throw manually. Fixes rdar://77449671

ahoppen · 2021-05-06T08:24:18Z

@swift-ci Please test

swift-ci · 2021-05-06T10:21:02Z

Build failed
Swift Test Linux Platform
Git Sha - aef9d51

swift-ci · 2021-05-06T11:13:22Z

Build failed
Swift Test OS X Platform
Git Sha - aef9d51

akyrtzi · 2021-05-06T16:02:11Z

Unrelated failures:
mac:

Failed Tests (1):
  lldb-shell :: Reproducer/SwiftREPL/SwiftREPLReproducer.test

linux:

The following tests FAILED:
	1091 - TestFoundation.TestOperationQueue-test_CustomOperationReady (Failed)

ahoppen requested review from akyrtzi and nkcsgexi May 4, 2021 18:50

ahoppen mentioned this pull request May 4, 2021

[5.5][ClangImporter] Load the stdlib even if there is a -fmodule-map-file argument pointing to a missing file #37253

Merged

akyrtzi requested changes May 4, 2021

View reviewed changes

nkcsgexi requested changes May 4, 2021

View reviewed changes

ahoppen force-pushed the pr/missing-module-map branch from ca740c5 to c7d398c Compare May 5, 2021 13:10

ahoppen force-pushed the pr/missing-module-map branch from c7d398c to 5c98058 Compare May 5, 2021 13:35

akyrtzi reviewed May 5, 2021

View reviewed changes

nkcsgexi approved these changes May 6, 2021

View reviewed changes

ahoppen force-pushed the pr/missing-module-map branch from 5c98058 to aef9d51 Compare May 6, 2021 08:24

akyrtzi approved these changes May 6, 2021

View reviewed changes

akyrtzi merged commit c417464 into swiftlang:main May 6, 2021

ahoppen deleted the pr/missing-module-map branch May 17, 2021 17:09

ahoppen mentioned this pull request May 18, 2021

[5.4][ClangImporter] Load the stdlib even if there is a -fmodule-map-file argument pointing to a missing file #37481

Merged

[ClangImporter] Load the stdlib even if there is a -fmodule-map-file argument pointing to a missing file #37252

[ClangImporter] Load the stdlib even if there is a -fmodule-map-file argument pointing to a missing file #37252

Uh oh!

Conversation

ahoppen commented May 4, 2021

Uh oh!

akyrtzi May 4, 2021

Choose a reason for hiding this comment

Uh oh!

ahoppen May 5, 2021 • 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!

nkcsgexi left a comment

Choose a reason for hiding this comment

Uh oh!

ahoppen commented May 5, 2021

Uh oh!

nkcsgexi commented May 5, 2021

Uh oh!

akyrtzi commented May 5, 2021 • edited by nkcsgexi Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

akyrtzi May 5, 2021

Choose a reason for hiding this comment

Uh oh!

ahoppen May 6, 2021

Choose a reason for hiding this comment

Uh oh!

nkcsgexi commented May 5, 2021

Uh oh!

akyrtzi commented May 5, 2021

Uh oh!

nkcsgexi commented May 5, 2021

Uh oh!

akyrtzi commented May 5, 2021

Uh oh!

ahoppen commented May 6, 2021

Uh oh!

swift-ci commented May 6, 2021

Uh oh!

swift-ci commented May 6, 2021

Uh oh!

akyrtzi commented May 6, 2021

Uh oh!

Uh oh!

[ClangImporter] Load the stdlib even if there is a `-fmodule-map-file` argument pointing to a missing file #37252

[ClangImporter] Load the stdlib even if there is a `-fmodule-map-file` argument pointing to a missing file #37252

ahoppen May 5, 2021 •

edited

Loading

akyrtzi commented May 5, 2021 •

edited by nkcsgexi

Loading