[llvm-link] Improve missing file error message #82514
Conversation
mhalk
force-pushed
the
mhalk/feature/llvm_link_missing_file_err_improvement
branch
from
0314d0f
to
da7f21e
Compare
llvm/tools/llvm-link/llvm-link.cpp
Outdated
| std::unique_ptr<MemoryBuffer> Buffer = | ||
| ExitOnErr(errorOrToExpected(MemoryBuffer::getFileOrSTDIN(File))); | ||
| ExitOnErr(errorOrToExpected(std::move(ErrOrExpected))); |
There was a problem hiding this comment.
Unsure if it's necessary to error on all missing files at the same time. I would recommend just returning an error on this file if it's not found, and using something like createStringError("No such file or directory: %s", F.str()).
There was a problem hiding this comment.
Unsure if it's necessary to error on all missing files at the same time.
I have no strong preference here, as long as I get the filename instead of nothing.
Q: So you'd prefer only one error message mentioning the first missing file?
I would recommend just returning an error on this file if it's not found [...]
Like this? (If not: what do you mean by 'returning'?)
// When we encounter a missing file, print all missing files to stderr.
if (auto EC = ErrOrExpected.getError())
if (EC == std::errc::no_such_file_or_directory)
ExitOnErr(createStringError(EC, "No such file or directory: %s", File.c_str()));
There was a problem hiding this comment.
Yeah, that's what I had in mind.
mhalk
force-pushed
the
mhalk/feature/llvm_link_missing_file_err_improvement
branch
from
da7f21e
to
ba85225
Compare
llvm/tools/llvm-link/llvm-link.cpp
Outdated
| @@ -393,8 +393,16 @@ static bool linkFiles(const char *argv0, LLVMContext &Context, Linker &L, | |||
| // Similar to some flags, internalization doesn't apply to the first file. | |||
| bool InternalizeLinkedSymbols = false; | |||
| for (const auto &File : Files) { | |||
| auto ErrOrExpected = MemoryBuffer::getFileOrSTDIN(File); | |||
There was a problem hiding this comment.
Nit, this variable name is a little weird, maybe BufferOrErr?
There was a problem hiding this comment.
Check! I'll keep that in mind and address along with further potential feedback.
(Sorry, did not see this comment at the time of hitting the button.)
Add error message showing the missing filename. Currently, we only get 'No such file or directory' without any(!) further info. This patch will (upon ENOENT error) create an error which exposes the filename.
mhalk
force-pushed
the
mhalk/feature/llvm_link_missing_file_err_improvement
branch
from
ba85225
to
73bbd41
Compare
|
Should we add something that (lit-)checks the changed error message? |
If there's an existing check it may still pass since it checks the prefix. I don't think it's an issue personally. |
Add a simple test, which is trying to link a non-existent file, since the error message will be expanded by the missing file's name. Depends: llvm/llvm-project#82514
Add a simple test, which is trying to link a non-existent file, since the error message will be expanded by the missing file's name. Depends: llvm/llvm-project#82514
Add error messages showing the missing filenames. Currently, we only get 'No such file or directory' without any(!) further info. This patch will (only upon ENOENT error) iterate over all requested files and print which ones are actually missing. Change-Id: I2a240e66576980e6878891e10ad3c5972b47ea1f
Add error messages showing the missing filenames.
Currently, we only get 'No such file or directory' without any(!) further info. This patch will (only upon ENOENT error) iterate over all requested files and print which ones are actually missing.