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