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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
