This is an archive of the discontinued LLVM Phabricator instance.

clang/Modules: Error if ReadASTBlock does not find the main module
ClosedPublic

Authored by dexonsmith on Nov 10 2019, 1:40 PM.

Details

Summary

If ReadASTBlock does not find the main module, there's something wrong
the with the PCM. Error in that case, to avoid hitting problems further
from the source.

Note that the Swift compiler sometimes finds in
CompilerInstance::loadModule that the main module mysteriously does not
have Module::IsFromModuleFile set. That will emit a confusing
warn_missing_submodule, which was never intended for a top-level module.
The recent audit of error-handling in ReadAST may have rooted out the
real problem. If not, this commit will help to clarify the real
problem, and replace a confusing warning with an error pointing at the
malformed PCM file.

Diff Detail

Event Timeline

dexonsmith created this revision.Nov 10 2019, 1:40 PM

Updated to use a new diagnostic (err_module_file_missing_definition) that includes the filename of the PCM.

Do we want to bother with a testcase for this?

clang/include/clang/Basic/DiagnosticSerializationKinds.td
78 ↗(On Diff #228626)

Should this be AST file like in the above error message?

bruno added inline comments.Nov 11 2019, 5:02 PM
clang/include/clang/Basic/DiagnosticSerializationKinds.td
78 ↗(On Diff #228626)

+1

clang/lib/Serialization/ASTReader.cpp
5504

Is this enough? Because this is done at the end of SUBMODULE_DEFINITION, could it be that only some of the submodules were read (top level or not) and this is still going to be true? I wonder if marking this true at the end of successful SUBMODULE_BLOCK instead wouldn't be a better option.

dexonsmith marked 2 inline comments as done.Nov 11 2019, 5:52 PM
dexonsmith added inline comments.
clang/include/clang/Basic/DiagnosticSerializationKinds.td
78 ↗(On Diff #228626)

I think this should be module file, because it only applies to modules. Notice that err_module_file_not_found and err_module_file_out_of_date use %select{PCH|module|AST} file; this is just a constant-folding of that.

clang/lib/Serialization/ASTReader.cpp
5504

I see three cases to distinguish between:

  1. An error is encountered somewhere in the submodule block.
  2. The submodule block is read without error, but the main module is not seen/added.
  3. The submodule block is read without error, and the main module is seen/added.

Setting a flag to true at the end of a successful SUBMODULE_BLOCK would help to distinguish between (1) and (2 or 3), but the return status already tells us that, and ReadAST already deletes all the just-loaded modules in that case.

This patch as-is helps to distinguish between (2) and (3) once the early return for (1) has been avoided.

Does that make sense? Or maybe I didn't follow your suggestion.

bruno accepted this revision.Nov 12 2019, 8:17 AM

LGTM with some minor comment below!

clang/include/clang/Basic/DiagnosticSerializationKinds.td
78 ↗(On Diff #228626)

Makes sense!

clang/lib/Serialization/ASTReader.cpp
5504

Oh, I see now! Thanks for the explanation, perhaps add this to the commit message?

This revision is now accepted and ready to land.Nov 12 2019, 8:17 AM
dexonsmith closed this revision.Nov 12 2019, 8:44 AM
dexonsmith marked 2 inline comments as done.

Committed 83dcb34b6bf4c175040b18d3e8c3c468418009fc. Thanks for the reviews!

clang/lib/Serialization/ASTReader.cpp
5504

Added a comment there, and also reworded variables / diagnostics from talking about "main module" to talking about "top-level submodule", since I think that might be more consistent (and maybe more clear).