This is an archive of the discontinued LLVM Phabricator instance.

[LTO] Add error message on IO error in compileOptimizedToFile.
ClosedPublic

Authored by efriedma on Nov 7 2016, 2:53 PM.

Diff Detail

Repository
rL LLVM

Event Timeline

efriedma updated this revision to Diff 77103.Nov 7 2016, 2:53 PM
efriedma retitled this revision from to [LTO] Add error message on IO error in compileOptimizedToFile..
efriedma updated this object.
efriedma added a reviewer: mehdi_amini.
efriedma set the repository for this revision to rL LLVM.
efriedma added a subscriber: llvm-commits.
davide accepted this revision.Nov 7 2016, 2:56 PM
davide added a reviewer: davide.
davide added a subscriber: davide.

This seems reasonable. We do something similar in other places (and unfortunately is very hard to test in isolation).

This revision is now accepted and ready to land.Nov 7 2016, 2:56 PM
mehdi_amini accepted this revision.Nov 7 2016, 2:56 PM
mehdi_amini edited edge metadata.

LGTM. Thanks.

lib/LTO/LTOCodeGenerator.cpp
261 ↗(On Diff #77103)

Can you get the + Filename on the same line as the initialization for ErrMsg?
(Otherwise never mind).

Just wondering: can't you get an error by providing an output path in a non-existing directory?

efriedma added inline comments.Nov 7 2016, 3:00 PM
lib/LTO/LTOCodeGenerator.cpp
261 ↗(On Diff #77103)

I guess I could write std::string ErrMsg = Twine("could not write object file: ") + Filename;. Is that better?

davide added inline comments.Nov 7 2016, 3:03 PM
lib/LTO/LTOCodeGenerator.cpp
261 ↗(On Diff #77103)

I would rather leave it as is.

If file creation fails, you hit a different error path. This is the error path you hit if the disk runs out of space or something like that.

mehdi_amini added inline comments.Nov 7 2016, 3:15 PM
lib/LTO/LTOCodeGenerator.cpp
261 ↗(On Diff #77103)

I'd write: std::string ErrMsg = "could not write object file: " + Filename.str(); (if it compiles)

davide added inline comments.Nov 7 2016, 3:18 PM
lib/LTO/LTOCodeGenerator.cpp
261 ↗(On Diff #77103)

Agree, if it compiles. But using the Twine is a little bit cumbersome here, at least to me.

This revision was automatically updated to reflect the committed changes.