[PATCH] D105328: [Frontend] Only compile modules if not already finalized

[Volodymyr Sapsai via Phabricator via cfe-commits]

vsapsai added inline comments.


================
Comment at: clang/lib/Frontend/CompilerInstance.cpp:1063
+        << ModuleName;
+    return ImportingInstance.getFrontendOpts().AllowPCMWithCompilerErrors;
+  }
----------------
Can we get in infinite loop with `AllowPCMWithCompilerErrors = true`? Specifically, I'm thinking about the scenario

1. `compileModuleAndReadAST` obtains a file lock and calls `compileModule`
2. `compileModule` calls `compileModuleImpl`
3. Module is finalized but `AllowPCMWithCompilerErrors` is true, so `compileModuleImpl` returns true
4. `compileModule` returns true
5. `compileModuleAndReadAST` tries to read AST because compilation was successful
6. AST is out of date, so `compileModuleAndReadAST` decides to try again, goto 1

Haven't tried to reproduce it locally but even if this scenario is impossible, a corresponding test case can be useful.


================
Comment at: clang/lib/Serialization/ASTReader.cpp:2923
           if (!BuildDir || *BuildDir != M->Directory) {
-            if ((ClientLoadCapabilities & ARR_OutOfDate) == 0)
+            if (diagnoseOutOfDate(F.FileName, ClientLoadCapabilities))
               Diag(diag::err_imported_module_relocated)
----------------
I'm thinking if in case of finalized modules diagnostic messages are good enough. One concern is it won't be clear why a module wasn't rebuilt. It can be already confusing for precompiled headers and I'm afraid we won't be able to detect `isPCMFinal` code path without a debugger. Though I don't know how bad that would be in practice.

Another concern is that retrying a compilation should succeed as for a new process we have a new InMemoryModuleCache and `isPCMFinal` should return false. So we might have non-deterministic behavior and some of the valid error messages can seem to be non-deterministic and not reliable. I was thinking about adding a note in case we are dealing with `isPCMFinal` to distinguish these cases but not sure it is a good idea.


================
Comment at: clang/lib/Serialization/ASTReader.cpp:5929
 
+bool ASTReader::diagnoseOutOfDate(StringRef ModuleFileName,
+                                  unsigned int ClientLoadCapabilities) {
----------------
Based on the rest of the code in clang, the expectation for `diagnose...` methods is to emit diagnostic in some cases. Personally, I'm often confused what true/false means for these methods, so I'm thinking about renaming the method to something like isRecoverableOutOfDateModule, canRecoverOutOfDateModule or some such. Feel free to pick a name you believe is appropriate, mine are just examples.


================
Comment at: clang/unittests/Serialization/ModuleCacheTest.cpp:125-126
+  ASSERT_TRUE(Invocation2);
+  CompilerInstance Instance2(Instance.getPCHContainerOperations(),
+                             &Instance.getModuleCache());
+  Instance2.setDiagnostics(Diags.get());
----------------
bnbarham wrote:
> vsapsai wrote:
> > Haven't rechecked the code more carefully but had an idea is that if we want to allow InMemoryModuleCache reuse between multiple CompilerInstance, safer API would be to transfer ModuleCache ownership to the new CompilerInstance and maybe make all records in the cache purgeable. But that's applicable only if ModuleCache reuse is important.
> Every module compilation runs in a separate thread with a separate CompilerInstance (but shared cache). So ownership would have to be transferred back to the original instance once complete. I might be misunderstanding what you mean here though.
> 
> Another point to note is that eg. Swift doesn't actually create another CompilerInstance, it's just re-used across module loading and `CompilerInstance::loadModule` is called directly. I went with this way in the test as otherwise we'd need to inline most of `ExecuteAction` and have a custom `FrontendAction`.
Thanks for the explanation. I haven't considered how the API is used right now, so please discard my earlier comment.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D105328/new/

https://reviews.llvm.org/D105328