Fail early if an output file is not writable.

[ruiu]

Summary

Fixes https://bugs.llvm.org/show_bug.cgi?id=36478

[zturner]

lld/COFF/Driver.cpp

1278 ↗

(On Diff #135575)

There's still a race condition here, in that a program could open the file after this call succeeds but before we get around to writing it. It would be nice if there were a way for this function to grab the file handle for exclusive write access up front, but this seems hard to do due to the several layers of indirection between here and when we actually write the output file.

[ruiu]

Yes, there's a chance that this test succeeds but the final output fails, but that should be acceptable. File writing could fail for other reasons, like disk full situation, and there's no way to know that before we actually write bytes to a file, so I think it's not worth to spend too much effort to be more accurate.

lld/trunk/COFF/Driver.cpp

 #include "MarkLive.h"
 #include "MinGW.h"
 #include "SymbolTable.h"
 #include "Symbols.h"
 #include "Writer.h"
 #include "lld/Common/Args.h"
 #include "lld/Common/Driver.h"
 #include "lld/Common/ErrorHandler.h"
+#include "lld/Common/Filesystem.h"
 #include "lld/Common/Memory.h"
 #include "lld/Common/Threads.h"
 #include "lld/Common/Timer.h"
 #include "lld/Common/Version.h"
 #include "llvm/ADT/Optional.h"
 #include "llvm/ADT/StringSwitch.h"
 #include "llvm/BinaryFormat/Magic.h"
 #include "llvm/Object/ArchiveWriter.h"
void LinkerDriver::link(ArrayRef<const char *> ArgsArr) {
   }
 
   // Set default image name if neither /out or /def set it.
   if (Config->OutputFile.empty()) {
     Config->OutputFile =
         getOutputPath((*Args.filtered(OPT_INPUT).begin())->getValue());
   }
 
+  // Fail early if an output file is not writable.
+  if (auto E = tryCreateFile(Config->OutputFile)) {
+    error("cannot open output file " + Config->OutputFile + ": " + E.message());
+    return;
+  }
+
   if (ShouldCreatePDB) {
     // Put the PDB next to the image if no /pdb flag was passed.
     if (Config->PDBPath.empty()) {
       Config->PDBPath = Config->OutputFile;
       sys::path::replace_extension(Config->PDBPath, ".pdb");
     }
 
     // The embedded PDB path should be the absolute path to the PDB if no

lld/trunk/Common/CMakeLists.txt

set_source_files_properties("${version_inc}"
   HEADER_FILE_ONLY TRUE)
 
 set_property(SOURCE Version.cpp APPEND PROPERTY
   COMPILE_DEFINITIONS "HAVE_VCS_VERSION_INC")
 
 add_lld_library(lldCommon
   Args.cpp
   ErrorHandler.cpp
+  Filesystem.cpp
   Memory.cpp
   Reproduce.cpp
   Strings.cpp
   TargetOptionsCommandFlags.cpp
   Threads.cpp
   Timer.cpp
   VCSVersion.inc
   Version.cpp

lld/trunk/Common/Filesystem.cpp

+//===- Filesystem.cpp -----------------------------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+//
+// This file contains a few utility functions to handle files.
+//
+//===----------------------------------------------------------------------===//
+
+#include "lld/Common/Filesystem.h"
+#include "lld/Common/Threads.h"
+#include "llvm/Config/llvm-config.h"
+#include "llvm/Support/FileOutputBuffer.h"
+#include "llvm/Support/FileSystem.h"
+#if LLVM_ON_UNIX
+#include <unistd.h>
+#endif
+#include <thread>
+
+using namespace llvm;
+using namespace lld;
+
+// Removes a given file asynchronously. This is a performance hack,
+// so remove this when operating systems are improved.
+//
+// On Linux (and probably on other Unix-like systems), unlink(2) is a
+// noticeably slow system call. As of 2016, unlink takes 250
+// milliseconds to remove a 1 GB file on ext4 filesystem on my machine.
+//
+// To create a new result file, we first remove existing file. So, if
+// you repeatedly link a 1 GB program in a regular compile-link-debug
+// cycle, every cycle wastes 250 milliseconds only to remove a file.
+// Since LLD can link a 1 GB binary in about 5 seconds, that waste
+// actually counts.
+//
+// This function spawns a background thread to remove the file.
+// The calling thread returns almost immediately.
+void lld::unlinkAsync(StringRef Path) {
+// Removing a file is async on windows.
+#if defined(_WIN32)
+  sys::fs::remove(Path);
+#else
+  if (!ThreadsEnabled || !sys::fs::exists(Path) ||
+      !sys::fs::is_regular_file(Path))
+    return;
+
+  // We cannot just remove path from a different thread because we are now going
+  // to create path as a new file.
+  // Instead we open the file and unlink it on this thread. The unlink is fast
+  // since the open fd guarantees that it is not removing the last reference.
+  int FD;
+  std::error_code EC = sys::fs::openFileForRead(Path, FD);
+  sys::fs::remove(Path);
+
+  if (EC)
+    return;
+
+  // close and therefore remove TempPath in background.
+  std::mutex M;
+  std::condition_variable CV;
+  bool Started = false;
+  std::thread([&, FD] {
+    {
+      std::lock_guard<std::mutex> L(M);
+      Started = true;
+      CV.notify_all();
+    }
+    ::close(FD);
+  }).detach();
+
+  // GLIBC 2.26 and earlier have race condition that crashes an entire process
+  // if the main thread calls exit(2) while other thread is starting up.
+  std::unique_lock<std::mutex> L(M);
+  CV.wait(L, [&] { return Started; });
+#endif
+}
+
+// Simulate file creation to see if Path is writable.
+//
+// Determining whether a file is writable or not is amazingly hard,
+// and after all the only reliable way of doing that is to actually
+// create a file. But we don't want to do that in this function
+// because LLD shouldn't update any file if it will end in a failure.
+// We also don't want to reimplement heuristics to determine if a
+// file is writable. So we'll let FileOutputBuffer do the work.
+//
+// FileOutputBuffer doesn't touch a desitnation file until commit()
+// is called. We use that class without calling commit() to predict
+// if the given file is writable.
+std::error_code lld::tryCreateFile(StringRef Path) {
+  if (Path.empty())
+    return std::error_code();
+  if (Path == "-")
+    return std::error_code();
+  return errorToErrorCode(FileOutputBuffer::create(Path, 1).takeError());
+}

lld/trunk/ELF/CMakeLists.txt

add_lld_library(lldELF
   Arch/SPARCV9.cpp
   Arch/X86.cpp
   Arch/X86_64.cpp
   CallGraphSort.cpp
   DWARF.cpp
   Driver.cpp
   DriverUtils.cpp
   EhFrame.cpp
-  Filesystem.cpp
   ICF.cpp
   InputFiles.cpp
   InputSection.cpp
   LTO.cpp
   LinkerScript.cpp
   MapFile.cpp
   MarkLive.cpp
   OutputSections.cpp

lld/trunk/ELF/Driver.cpp

 // be harmful when you are doing cross-linking. Therefore, in LLD, we
 // simply trust the compiler driver to pass all required options and
 // don't try to make effort on our side.
 //
 //===----------------------------------------------------------------------===//
 
 #include "Driver.h"
 #include "Config.h"
-#include "Filesystem.h"
 #include "ICF.h"
 #include "InputFiles.h"
 #include "InputSection.h"
 #include "LinkerScript.h"
 #include "MarkLive.h"
 #include "OutputSections.h"
 #include "ScriptParser.h"
 #include "SymbolTable.h"
 #include "Symbols.h"
 #include "SyntheticSections.h"
 #include "Target.h"
 #include "Writer.h"
 #include "lld/Common/Args.h"
 #include "lld/Common/Driver.h"
 #include "lld/Common/ErrorHandler.h"
+#include "lld/Common/Filesystem.h"
 #include "lld/Common/Memory.h"
 #include "lld/Common/Strings.h"
 #include "lld/Common/TargetOptionsCommandFlags.h"
 #include "lld/Common/Threads.h"
 #include "lld/Common/Version.h"
 #include "llvm/ADT/SetVector.h"
 #include "llvm/ADT/StringExtras.h"
 #include "llvm/ADT/StringSwitch.h"

lld/trunk/ELF/Filesystem.h

-//===- Filesystem.h ---------------------------------------------*- C++ -*-===//
-//
-// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
-// See https://llvm.org/LICENSE.txt for license information.
-// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
-//
-//===----------------------------------------------------------------------===//
-
-#ifndef LLD_ELF_FILESYSTEM_H
-#define LLD_ELF_FILESYSTEM_H
-
-#include "lld/Common/LLVM.h"
-#include <system_error>
-
-namespace lld {
-namespace elf {
-void unlinkAsync(StringRef Path);
-std::error_code tryCreateFile(StringRef Path);
-} // namespace elf
-} // namespace lld
-
-#endif

lld/trunk/ELF/Filesystem.cpp

-//===- Filesystem.cpp -----------------------------------------------------===//
-//
-// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
-// See https://llvm.org/LICENSE.txt for license information.
-// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
-//
-//===----------------------------------------------------------------------===//
-//
-// This file contains a few utility functions to handle files.
-//
-//===----------------------------------------------------------------------===//
-
-#include "Filesystem.h"
-#include "Config.h"
-#include "lld/Common/Threads.h"
-#include "llvm/Config/llvm-config.h"
-#include "llvm/Support/FileOutputBuffer.h"
-#include "llvm/Support/FileSystem.h"
-#if LLVM_ON_UNIX
-#include <unistd.h>
-#endif
-#include <thread>
-
-using namespace llvm;
-
-using namespace lld;
-using namespace lld::elf;
-
-// Removes a given file asynchronously. This is a performance hack,
-// so remove this when operating systems are improved.
-//
-// On Linux (and probably on other Unix-like systems), unlink(2) is a
-// noticeably slow system call. As of 2016, unlink takes 250
-// milliseconds to remove a 1 GB file on ext4 filesystem on my machine.
-//
-// To create a new result file, we first remove existing file. So, if
-// you repeatedly link a 1 GB program in a regular compile-link-debug
-// cycle, every cycle wastes 250 milliseconds only to remove a file.
-// Since LLD can link a 1 GB binary in about 5 seconds, that waste
-// actually counts.
-//
-// This function spawns a background thread to remove the file.
-// The calling thread returns almost immediately.
-void elf::unlinkAsync(StringRef Path) {
-// Removing a file is async on windows.
-#if defined(_WIN32)
-  sys::fs::remove(Path);
-#else
-  if (!ThreadsEnabled || !sys::fs::exists(Path) ||
-      !sys::fs::is_regular_file(Path))
-    return;
-
-  // We cannot just remove path from a different thread because we are now going
-  // to create path as a new file.
-  // Instead we open the file and unlink it on this thread. The unlink is fast
-  // since the open fd guarantees that it is not removing the last reference.
-  int FD;
-  std::error_code EC = sys::fs::openFileForRead(Path, FD);
-  sys::fs::remove(Path);
-
-  if (EC)
-    return;
-
-  // close and therefore remove TempPath in background.
-  std::mutex M;
-  std::condition_variable CV;
-  bool Started = false;
-  std::thread([&, FD] {
-    {
-      std::lock_guard<std::mutex> L(M);
-      Started = true;
-      CV.notify_all();
-    }
-    ::close(FD);
-  }).detach();
-
-  // GLIBC 2.26 and earlier have race condition that crashes an entire process
-  // if the main thread calls exit(2) while other thread is starting up.
-  std::unique_lock<std::mutex> L(M);
-  CV.wait(L, [&] { return Started; });
-#endif
-}
-
-// Simulate file creation to see if Path is writable.
-//
-// Determining whether a file is writable or not is amazingly hard,
-// and after all the only reliable way of doing that is to actually
-// create a file. But we don't want to do that in this function
-// because LLD shouldn't update any file if it will end in a failure.
-// We also don't want to reimplement heuristics to determine if a
-// file is writable. So we'll let FileOutputBuffer do the work.
-//
-// FileOutputBuffer doesn't touch a desitnation file until commit()
-// is called. We use that class without calling commit() to predict
-// if the given file is writable.
-std::error_code elf::tryCreateFile(StringRef Path) {
-  if (Path.empty())
-    return std::error_code();
-  if (Path == "-")
-    return std::error_code();
-  return errorToErrorCode(FileOutputBuffer::create(Path, 1).takeError());
-}

lld/trunk/ELF/Writer.cpp

 //===- Writer.cpp ---------------------------------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "Writer.h"
 #include "AArch64ErrataFix.h"
 #include "CallGraphSort.h"
 #include "Config.h"
-#include "Filesystem.h"
 #include "LinkerScript.h"
 #include "MapFile.h"
 #include "OutputSections.h"
 #include "Relocations.h"
 #include "SymbolTable.h"
 #include "Symbols.h"
 #include "SyntheticSections.h"
 #include "Target.h"
+#include "lld/Common/Filesystem.h"
 #include "lld/Common/Memory.h"
 #include "lld/Common/Strings.h"
 #include "lld/Common/Threads.h"
 #include "llvm/ADT/StringMap.h"
 #include "llvm/ADT/StringSwitch.h"
 #include <climits>
 
 using namespace llvm;

lld/trunk/include/lld/Common/Filesystem.h

+//===- Filesystem.h ---------------------------------------------*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLD_FILESYSTEM_H
+#define LLD_FILESYSTEM_H
+
+#include "lld/Common/LLVM.h"
+#include <system_error>
+
+namespace lld {
+void unlinkAsync(StringRef Path);
+std::error_code tryCreateFile(StringRef Path);
+} // namespace lld
+
+#endif

lld/trunk/test/COFF/driver.test

 # RUN: not lld-link /out:%t.exe %t.dll 2>&1 | FileCheck -check-prefix=BADFILE %s
 BADFILE: bad file type. Did you specify a DLL instead of an import library?
 
 # RUN: lld-link /lib /help | FileCheck -check-prefix=LIBHELP %s
 LIBHELP: OVERVIEW: LLVM Lib
 
 # RUN: not lld-link /WX /lib 2>&1 | FileCheck -check-prefix=LIBBAD %s
 LIBBAD: ignoring /lib since it's not the first argument
+
+# RUN: yaml2obj < %p/Inputs/hello32.yaml > %t.obj
+# RUN: not lld-link /out:/ %t.obj 2>&1 | FileCheck -check-prefix=DIR %s
+DIR: cannot open output file