The intent of this change initially was to:
- standardize LLD's PDB errors/messages
- provide more context when an error occurs
- add test coverage for all possible PDB warnings/errors kinds in LLD
- prepare errors for the upcoming precompiled headers ( D45213 )
It seemed along the way that some other changes were needed:
- It is sometimes hard to cleanly represent the 'source' of an error. We know the error, but not which file caused it. I've added a minimal FileError for that purpose (llvm/Support/Error.h). Some more changes will be needed later on, such as a takeError() function, currently not required.
- All the error types in llvm/.../DebugInfo/... were having the same kind of functionality, so I normalized that into DebugInfoError (llvm/Support/Error.h) - a better class name suggestion is welcome.
- I realized that from a UX perspective, the overall printing of error & warnings in LLVM is not uniform, uses different wording, different layout, etc. (see below) It would be good on the long run to: a. decide on a consistent output for most common cases; b. maybe raise the abstraction for the error system to constrain how errors are printed.
Other changes in this patch at a glance:
- Renamed GenericError into PDBError (llvm/Debuginfo/PDB/GenericError.h)
- Changed some PDBError messages because they weren't consistent with their origin.
- Clarified the error type when the PDB signature doesn't match.
- The same error message is now printed when a PDB fails loading a second time (referenced from different .objs).
- Retry loading a PDB with filename works only if the previous error was "No such file or directory"
- Add more context when a PDB (or a precompiled headers .obj) error occurs. Previously, only the .obj name was displayed.
As for the error inconsistencies I've noted so far:
Bad: Inconsistent period at the end of error strings:
- _readobj_error_category::message() has it:
std::string _readobj_error_category::message(int EV) const { switch (static_cast<readobj_error>(EV)) { case readobj_error::success: return "Success"; case readobj_error::file_not_found: return "No such file.";
- CoverageMappingErrorCategoryType::message() does not:
static std::string getCoverageMapErrString(coveragemap_error Err) { switch (Err) { case coveragemap_error::success: return "Success"; case coveragemap_error::eof: return "End of File"; case coveragemap_error::no_data_found: return "No coverage data found";
I'm currently torn between whether a std::error_code should print the ending period of not. At first sight, it seems not printing it would be better.
Good: Most error_category::message() implementations start with an uppercase.
Bad: Tool error messages sometimes start with an uppercase...
(LLD/COFF/PDB.cpp) warn("Cannot open input file: " + File);
...sometimes with a lowercase:
(llvm-objdump.cpp) errs() << ToolName << ": error reading file: " << EC.message() << ".\n";
Bad: ExitOnError vs. printing errors "by hand" - ExitOnError uses logAllUnhandledErrors() which currently does not allow for adding an ending period which isn't there.
Bad: Some printers inherently add two CRs, one by going through logAllUnhandledErrors() and one externally while logging the message.
Bad: Some printers add an extra ending period character, one from the original error message, and one from the manual error printer (see llvm-objdump example above).
Those topics should probably be discussed, but I simply want to have you feeling overall.
We might be doing a cross-build, so this should probably be native path style.