This is an archive of the discontinued LLVM Phabricator instance.

[llvm-dlltool] Don't crash if no def file is provided or it can't be opened
ClosedPublic

Authored by mstorsjo on Aug 15 2017, 10:09 PM.

Diff Detail

Repository
rL LLVM

Event Timeline

mstorsjo created this revision.Aug 15 2017, 10:09 PM
ruiu accepted this revision.Aug 16 2017, 3:11 PM

LGTM

lib/ToolDrivers/llvm-dlltool/DlltoolDriver.cpp
67 ↗(On Diff #111305)

openFile is a function name and shouldn't be reported as part of an error message. It should be something like "cannot open file " << Path << ": " << EC.message() << "\n".

But this is not new code, so we don't have to address it in this patch.

120–128 ↗(On Diff #111305)

I feel this way is more readable.

if (!Args.hasArg(OPT_d))
  llvm::errs() << "no definition file specified\n";
  return 1;
}

Optional<MemoryBufferRef> MB = openFile(Args.getLastArg(OPT_d)->getValue());
if (!MB)
  return 1;
This revision is now accepted and ready to land.Aug 16 2017, 3:11 PM
mstorsjo added inline comments.Aug 16 2017, 10:59 PM
lib/ToolDrivers/llvm-dlltool/DlltoolDriver.cpp
67 ↗(On Diff #111305)

Ok - I can change that in a separate patch.

120–128 ↗(On Diff #111305)

Indeed, that's much more readable. Will commit in that form.

This revision was automatically updated to reflect the committed changes.
mstorsjo added a subscriber: hans.Aug 16 2017, 11:01 PM

@hans - I guess this should be backport-worthy as well?

hans added a comment.Aug 17 2017, 10:16 AM

@hans - I guess this should be backport-worthy as well?

Sure, r311104.