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

[Ben Barham via Phabricator via cfe-commits]

bnbarham marked 2 inline comments as done.
bnbarham added inline comments.


================
Comment at: clang/lib/Frontend/CompilerInstance.cpp:1063
+        << ModuleName;
+    return ImportingInstance.getFrontendOpts().AllowPCMWithCompilerErrors;
+  }
----------------
vsapsai wrote:
> bnbarham wrote:
> > bnbarham wrote:
> > > vsapsai wrote:
> > > > 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.
> > > Nice catch, that does seem likely - I'll see if I can add a test for this.
> > It doesn't end up causing an infinite recursion as `false` will end up being returned from `compileModuleAndReadAST`, but in that case there's no point returning `true` from `compileModuleImpl` in the first place so I've changed it anyway. Have also added that as a test case, just to make sure.
> Thanks for investigating it and adding a test case.
> 
> And appreciate that the error message mentions the module name. It is so hard to work with errors like "cannot rebuild this module".
Note that this error is really just a fallback - it will be silenced by the previous fatal errors in ASTReader.


================
Comment at: clang/lib/Serialization/ASTReader.cpp:2859-2860
               << F.FileName << !F.ModuleName.empty() << F.ModuleName;
+        if (recompileFinalized)
+          Diag(diag::note_module_file_conflict);
 
----------------
The failing test was because this note was being output when if `OutOfDate` wasn't in `Capabilities`. It should only get output when `OutOfDate` can be handled (ie. it'd be recompiled) *and* the module is already finalized.


================
Comment at: clang/lib/Serialization/ASTReader.cpp:5929
 
+bool ASTReader::diagnoseOutOfDate(StringRef ModuleFileName,
+                                  unsigned int ClientLoadCapabilities) {
----------------
vsapsai wrote:
> bnbarham wrote:
> > bnbarham wrote:
> > > vsapsai wrote:
> > > > 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.
> > > Fair enough, `canRecoverOutOfDateModule` sounds reasonable to me. Or maybe `canRecoverFromOutOfDate`?
> > I went with the latter.
> I think with the current naming we need to flip the conditions to opposite. Currently,
> 
> ```lang=c++
>   return !(ClientLoadCapabilities & ARR_OutOfDate) ||
>          getModuleManager().getModuleCache().isPCMFinal(ModuleFileName);
> ```
> 
> corresponds to `cannotRecoverFromOutOfDate`. But to avoid double negations it is better to have `canRecoverFromOutOfDate` (with `(ClientLoadCapabilities & ARR_OutOfDate) && !isPCMFinal(ModuleFileName)`) and call it as `!canRecoverFromOutOfDate` when necessary.
Argh, sorry - just did a find + replace without thinking. Have flipped that function and its calls.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105328