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

[ruiu]

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;

[mstorsjo]

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.

[mstorsjo]

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

[hans]

In D36780#844035, @mstorsjo wrote:

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

Sure, r311104.

llvm/trunk/lib/ToolDrivers/llvm-dlltool/DlltoolDriver.cpp

 };
 
 } // namespace
 
 std::vector<std::unique_ptr<MemoryBuffer>> OwningMBs;
 
 // Opens a file. Path has to be resolved already.
 // Newly created memory buffers are owned by this driver.
-MemoryBufferRef openFile(StringRef Path) {
+Optional<MemoryBufferRef> openFile(StringRef Path) {
   ErrorOr<std::unique_ptr<llvm::MemoryBuffer>> MB = MemoryBuffer::getFile(Path);
 
-  if (std::error_code EC = MB.getError())
+  if (std::error_code EC = MB.getError()) {
     llvm::errs() << "fail openFile: " << EC.message() << "\n";
+    return None;
+  }
 
   MemoryBufferRef MBRef = MB.get()->getMemBufferRef();
   OwningMBs.push_back(std::move(MB.get())); // take ownership
   return MBRef;
 }
 
 static MachineTypes getEmulation(StringRef S) {
   return StringSwitch<MachineTypes>(S)
if (!Args.hasArgNoClaim(OPT_m) && Args.hasArgNoClaim(OPT_d)) {
     llvm::errs() << "error: no target machine specified\n"
                  << "supported targets: i386, i386:x86-64, arm\n";
     return 1;
   }
 
   for (auto *Arg : Args.filtered(OPT_UNKNOWN))
     llvm::errs() << "ignoring unknown argument: " << Arg->getSpelling() << "\n";
 
-  MemoryBufferRef MB;
-  if (auto *Arg = Args.getLastArg(OPT_d))
-    MB = openFile(Arg->getValue());
+  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;
 
-  if (!MB.getBufferSize()) {
+  if (!MB->getBufferSize()) {
     llvm::errs() << "definition file empty\n";
     return 1;
   }
 
   COFF::MachineTypes Machine = IMAGE_FILE_MACHINE_UNKNOWN;
   if (auto *Arg = Args.getLastArg(OPT_m))
     Machine = getEmulation(Arg->getValue());
 
   if (Machine == IMAGE_FILE_MACHINE_UNKNOWN) {
     llvm::errs() << "unknown target\n";
     return 1;
   }
 
   Expected<COFFModuleDefinition> Def =
-      parseCOFFModuleDefinition(MB, Machine, true);
+      parseCOFFModuleDefinition(*MB, Machine, true);
 
   if (!Def) {
     llvm::errs() << "error parsing definition\n"
                  << errorToErrorCode(Def.takeError()).message();
     return 1;
   }
 
   // Do this after the parser because parseCOFFModuleDefinition sets OutputFile.