Module: retry building modules that were just compiled by the same instance and are are out of date

[manmanren]

Summary

Even though this instance just built module "A", it is likely that "A" imports another module "B" and B.pcm becomes out of date when we try to load module "A", because another instance overwrites "B.pcm" due to changes in warning options. This patch tries to fix the error: "Module file ‘<some-file path>.pcm' is out of date and needs to be rebuilt" by simply trying again.

This seems to be the last case where we call ReadAST without the ability to handle out-of-date.

If there are other ways to fix the issue, please let me know.
Also I don't quite know how to add a testing case when two compiling instances are involved and running in parallel.

rdar://problem/26676111

[benlangmuir]

B.pcm becomes out of date when we try to load module "A", because another instance overwrites "B.pcm"

How can this happen? We intentionally keep the file descriptors of modules open. If you just built A, then you will have B.pcm still open. When you read A, you will use the same B that you used when you built A even if the file on disk has been replaced (and we use rename to replace the file, so the existing one is never modified).

[manmanren]

In D22636#491679, @benlangmuir wrote:

B.pcm becomes out of date when we try to load module "A", because another instance overwrites "B.pcm"

How can this happen? We intentionally keep the file descriptors of modules open. If you just built A, then you will have B.pcm still open. When you read A, you will use the same B that you used when you built A even if the file on disk has been replaced (and we use rename to replace the file, so the existing one is never modified).

Can you point me to the source codes where we use rename to replace the file? I am curious on how this all works out.

What I described is a scenario I thought possible that can cause "out-of-date" error:
module "B" is out of date and needs to be rebuilt
note: imported by module "A"

The only invocation of ReadAST that reads a module file and can't handle out-of-date modules, is the path where we just built module "A" and tries to load module "A" (here ModuleLoadCapabilities will be ARR_Missing).

I am still working with the project owner to collect more debugging messages.

If this is not a possible scenario, do you have any suggestion on what can cause this error?

Cheers,
Manman

[benlangmuir]

Can you point me to the source codes where we use rename to replace the file? I am curious on how this all works out.

This is the same as clang's handling of other output files. See {{CompilerInstance::createOutputFile}} and {{clearOutputFiles}}. When we are performing the GenerateModuleAction, the .pcm file will be the output file.

What I described is a scenario I thought possible that can cause "out-of-date" error:
module "B" is out of date and needs to be rebuilt
note: imported by module "A"
The only invocation of ReadAST that reads a module file and can't handle out-of-date modules, is the path where we just built module "A" and tries to load module "A" (here ModuleLoadCapabilities will be ARR_Missing).

Right, here and in PCH loading (I assume your case doesn't involve a PCH). I'm concerned about adding this loop, because it removes the guarantee of progress. We should never need to build a module more than once in the same compilation. If we have a bug that's causing us to not be able to load a module that we just compiled, I think we need to fix the underlying problem, not just retry the build.

[manmanren]

In D22636#493072, @benlangmuir wrote:

Can you point me to the source codes where we use rename to replace the file? I am curious on how this all works out.

This is the same as clang's handling of other output files. See {{CompilerInstance::createOutputFile}} and {{clearOutputFiles}}. When we are performing the GenerateModuleAction, the .pcm file will be the output file.

Thanks for the info!

What I described is a scenario I thought possible that can cause "out-of-date" error:
module "B" is out of date and needs to be rebuilt
note: imported by module "A"
The only invocation of ReadAST that reads a module file and can't handle out-of-date modules, is the path where we just built module "A" and tries to load module "A" (here ModuleLoadCapabilities will be ARR_Missing).

Right, here and in PCH loading (I assume your case doesn't involve a PCH). I'm concerned about adding this loop, because it removes the guarantee of progress. We should never need to build a module more than once in the same compilation. If we have a bug that's causing us to not be able to load a module that we just compiled, I think we need to fix the underlying problem, not just retry the build.

Yes, I agree. We should try to figure out the root cause. I gave the project owner a root with debugging messages, but with this root, the error disappeared :(

If you have suggestions on debugging this, let me know.

Thanks,
Manman

[manmanren]

Abandon this change for now.

lib/Frontend/CompilerInstance.cpp

while (1) {
 
     case llvm::LockFileManager::LFS_Owned:
       // We're responsible for building the module ourselves.
       if (!compileModuleImpl(ImportingInstance, ModuleNameLoc, Module,
                              ModuleFileName)) {
         diagnoseBuildFailure();
         return false;
       }
+      ModuleLoadCapabilities |= ASTReader::ARR_OutOfDate;
       break;
 
     case llvm::LockFileManager::LFS_Shared:
       // Someone else is responsible for building the module. Wait for them to
       // finish.
       switch (Locked.waitForUnlock()) {
       case llvm::LockFileManager::Res_Success:
         ModuleLoadCapabilities |= ASTReader::ARR_OutOfDate;
while (1) {
 
     // Try to read the module file, now that we've compiled it.
     ASTReader::ASTReadResult ReadResult =
         ImportingInstance.getModuleManager()->ReadAST(
             ModuleFileName, serialization::MK_ImplicitModule, ImportLoc,
             ModuleLoadCapabilities);
 
     if (ReadResult == ASTReader::OutOfDate &&
-        Locked == llvm::LockFileManager::LFS_Shared) {
+        (Locked == llvm::LockFileManager::LFS_Shared ||
+         Locked == llvm::LockFileManager::LFS_Owned)) {
       // The module may be out of date in the presence of file system races,
       // or if one of its imports depends on header search paths that are not
       // consistent with this ImportingInstance.  Try again...
+
+      // The module may be out of date right after we rebuild it if a module
+      // it imports is overwritten by another process. Try again...
       continue;
     } else if (ReadResult == ASTReader::Missing) {
       diagnoseBuildFailure();
     } else if (ReadResult != ASTReader::Success &&
                !Diags.hasErrorOccurred()) {
       // The ASTReader didn't diagnose the error, so conservatively report it.
       diagnoseBuildFailure();
     }