bnbarham added inline comments.
================
Comment at: clang/lib/Frontend/CompilerInstance.cpp:1063
+ << ModuleName;
+ return ImportingInstance.getFrontendOpts().AllowPCMWithCompilerErrors;
+ }
----------------
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.
================
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)
----------------
vsapsai wrote:
> 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.
> 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.
The error messages will mention a module in the module cache, which would be
the main way to tell. We could add a note here as you suggest below, but I'm
not quite sure what it would say... something about there being two modules
with the same name?
> 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.
The errors should be deterministic I believe. If one process has the issue then
a new one will have the issue as well. For what it's worth, I don't think these
crashes are possible from the clang frontend. They require messing around with
search paths such that between two compilations in the same process, different
modules are found.
================
Comment at: clang/lib/Serialization/ASTReader.cpp:5929
+bool ASTReader::diagnoseOutOfDate(StringRef ModuleFileName,
+ unsigned int ClientLoadCapabilities) {
----------------
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`?
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
