Merge 24f818820b into e03f74a357

Fixes https://github.com/rui314/mold/issues/1299

common/common.h

@@ -735,7 +735,7 @@ template <typename Context>
 class OutputFile {
 public:
   static std::unique_ptr<OutputFile<Context>>
-  open(Context &ctx, std::string path, i64 filesize, i64 perm);
+  open(Context &ctx, std::string path, i64 filesize, int perm);
 
   virtual void close(Context &ctx) = 0;
   virtual ~OutputFile() = default;
@@ -743,7 +743,7 @@ public:
   u8 *buf = nullptr;
   std::vector<u8> buf2;
   std::string path;
-  i64 fd = -1;
+  int fd = -1;
   i64 filesize = 0;
   bool is_mmapped = false;
   bool is_unmapped = false;
@@ -756,7 +756,7 @@ protected:
 template <typename Context>
 class MallocOutputFile : public OutputFile<Context> {
 public:
-  MallocOutputFile(Context &ctx, std::string path, i64 filesize, i64 perm)
+  MallocOutputFile(Context &ctx, std::string path, i64 filesize, int perm)
     : OutputFile<Context>(path, filesize, false), ptr(new u8[filesize]),
       perm(perm) {
     this->buf = ptr.get();
@@ -792,7 +792,15 @@ public:
 
 private:
   std::unique_ptr<u8[]> ptr;
-  i64 perm;
+  int perm;
+};
+
+template <typename Context>
+class LockingOutputFile : public OutputFile<Context> {
+public:
+  LockingOutputFile(Context &ctx, std::string path, int perm);
+  void resize(Context &ctx, i64 filesize);
+  void close(Context &ctx) override;
 };
 
 //

common/output-file-unix.h

@@ -2,6 +2,7 @@
 
 #include <fcntl.h>
 #include <filesystem>
+#include <sys/file.h>
 #include <sys/mman.h>
 #include <sys/stat.h>
 #include <sys/types.h>
@@ -15,50 +16,47 @@ inline u32 get_umask() {
 }
 
 template <typename Context>
-static std::pair<i64, char *>
-open_or_create_file(Context &ctx, std::string path, i64 filesize, i64 perm) {
-  std::string tmpl = filepath(path).parent_path() / ".mold-XXXXXX";
-  char *path2 = (char *)save_string(ctx, tmpl).data();
-
-  i64 fd = mkstemp(path2);
-  if (fd == -1)
-    Fatal(ctx) << "cannot open " << path2 <<  ": " << errno_string();
-
+static int
+open_or_create_file(Context &ctx, std::string path, std::string tmpfile,
+                    int perm) {
   // Reuse an existing file if exists and writable because on Linux,
   // writing to an existing file is much faster than creating a fresh
   // file and writing to it.
-  if (ctx.overwrite_output_file && rename(path.c_str(), path2) == 0) {
-    ::close(fd);
-    fd = ::open(path2, O_RDWR | O_CREAT, perm);
-    if (fd != -1 && !ftruncate(fd, filesize) && !fchmod(fd, perm & ~get_umask()))
-      return {fd, path2};
-
-    unlink(path2);
-    fd = ::open(path2, O_RDWR | O_CREAT, perm);
-    if (fd == -1)
-      Fatal(ctx) << "cannot open " << path2 << ": " << errno_string();
+  if (ctx.overwrite_output_file && rename(path.c_str(), tmpfile.c_str()) == 0) {
+    i64 fd = ::open(tmpfile.c_str(), O_RDWR | O_CREAT, perm);
+    if (fd != -1)
+      return fd;
+    unlink(tmpfile.c_str());
   }
 
-  if (fchmod(fd, (perm & ~get_umask())) == -1)
-    Fatal(ctx) << "fchmod failed: " << errno_string();
-
-#ifdef __linux__
-  if (fallocate(fd, 0, 0, filesize) == 0)
-    return {fd, path2};
-#endif
-
-  if (ftruncate(fd, filesize) == -1)
-    Fatal(ctx) << "ftruncate failed: " << errno_string();
-  return {fd, path2};
+  i64 fd = ::open(tmpfile.c_str(), O_RDWR | O_CREAT, perm);
+  if (fd == -1)
+    Fatal(ctx) << "cannot open " << tmpfile << ": " << errno_string();
+  return fd;
 }
 
 template <typename Context>
 class MemoryMappedOutputFile : public OutputFile<Context> {
 public:
-  MemoryMappedOutputFile(Context &ctx, std::string path, i64 filesize, i64 perm)
+  MemoryMappedOutputFile(Context &ctx, std::string path, i64 filesize, int perm)
     : OutputFile<Context>(path, filesize, true) {
-    std::tie(this->fd, output_tmpfile) =
-      open_or_create_file(ctx, path, filesize, perm);
+    std::filesystem::path dir = filepath(path).parent_path();
+    std::string filename = filepath(path).filename().string();
+    std::string tmpfile = dir / ("." + filename + "." + std::to_string(getpid()));
+
+    this->fd = open_or_create_file(ctx, path, tmpfile, perm);
+
+    if (fchmod(this->fd, perm & ~get_umask()) == -1)
+      Fatal(ctx) << "fchmod failed: " << errno_string();
+
+    if (ftruncate(this->fd, filesize) == -1)
+      Fatal(ctx) << "ftruncate failed: " << errno_string();
+
+    output_tmpfile = (char *)save_string(ctx, tmpfile).data();
+
+#ifdef __linux__
+    fallocate(this->fd, 0, 0, filesize);
+#endif
 
     this->buf = (u8 *)mmap(nullptr, filesize, PROT_READ | PROT_WRITE,
                            MAP_SHARED, this->fd, 0);
@@ -107,7 +105,7 @@ private:
 
 template <typename Context>
 std::unique_ptr<OutputFile<Context>>
-OutputFile<Context>::open(Context &ctx, std::string path, i64 filesize, i64 perm) {
+OutputFile<Context>::open(Context &ctx, std::string path, i64 filesize, int perm) {
   Timer t(ctx, "open_file");
 
   if (path.starts_with('/') && !ctx.arg.chroot.empty())
@@ -146,4 +144,52 @@ OutputFile<Context>::open(Context &ctx, std::string path, i64 filesize, i64 perm
   return std::unique_ptr<OutputFile>(file);
 }
 
+// LockingOutputFile is similar to MemoryMappedOutputFile, but it doesn't
+// rename output files and instead acquires file lock using flock().
+template <typename Context>
+LockingOutputFile<Context>::LockingOutputFile(Context &ctx, std::string path,
+                                              int perm)
+  : OutputFile<Context>(path, 0, true) {
+  this->fd = ::open(path.c_str(), O_RDWR | O_CREAT, perm);
+  if (this->fd == -1)
+    Fatal(ctx) << "cannot open " << path << ": " << errno_string();
+  flock(this->fd, LOCK_EX);
+
+  // We may be overwriting to an existing debug info file. We want to
+  // make the file unusable so that gdb won't use it by accident until
+  // it's ready.
+  u8 buf[256] = {};
+  (void)!!write(this->fd, buf, sizeof(buf));
+}
+
+template <typename Context>
+void LockingOutputFile<Context>::resize(Context &ctx, i64 filesize) {
+  if (ftruncate(this->fd, filesize) == -1)
+    Fatal(ctx) << "ftruncate failed: " << errno_string();
+
+  this->buf = (u8 *)mmap(nullptr, filesize, PROT_READ | PROT_WRITE,
+                         MAP_SHARED, this->fd, 0);
+  if (this->buf == MAP_FAILED)
+    Fatal(ctx) << this->path << ": mmap failed: " << errno_string();
+
+  this->filesize = filesize;
+  mold::output_buffer_start = this->buf;
+  mold::output_buffer_end = this->buf + filesize;
+}
+
+template <typename Context>
+void LockingOutputFile<Context>::close(Context &ctx) {
+  if (!this->is_unmapped)
+    munmap(this->buf, this->filesize);
+
+  if (!this->buf2.empty()) {
+    FILE *out = fdopen(this->fd, "w");
+    fseek(out, 0, SEEK_END);
+    fwrite(&this->buf2[0], this->buf2.size(), 1, out);
+    fclose(out);
+  }
+
+  ::close(this->fd);
+}
+
 } // namespace mold

common/output-file-win32.h

@@ -9,36 +9,35 @@ namespace mold {
 template <typename Context>
 class MemoryMappedOutputFile : public OutputFile<Context> {
 public:
-  MemoryMappedOutputFile(Context &ctx, std::string path, i64 filesize, i64 perm)
+  MemoryMappedOutputFile(Context &ctx, std::string path, i64 filesize, int perm)
       : OutputFile<Context>(path, filesize, true) {
     // TODO: use intermediate temporary file for output.
-    DWORD file_attrs =
-        (perm & 0200) ? FILE_ATTRIBUTE_NORMAL : FILE_ATTRIBUTE_READONLY;
-    file_handle =
-        CreateFileA(path.c_str(), GENERIC_READ | GENERIC_WRITE,
-                    FILE_SHARE_READ | FILE_SHARE_WRITE | FILE_SHARE_DELETE,
-                    nullptr, CREATE_ALWAYS, file_attrs, nullptr);
-    if (file_handle == INVALID_HANDLE_VALUE)
+    DWORD attrs = (perm & 0200) ? FILE_ATTRIBUTE_NORMAL : FILE_ATTRIBUTE_READONLY;
+
+    handle = CreateFileA(path.c_str(), GENERIC_READ | GENERIC_WRITE,
+                         FILE_SHARE_READ | FILE_SHARE_WRITE | FILE_SHARE_DELETE,
+                         nullptr, CREATE_ALWAYS, attrs, nullptr);
+    if (handle == INVALID_HANDLE_VALUE)
       Fatal(ctx) << "cannot open " << path << ": " << GetLastError();
 
-    HANDLE mapping_handle = CreateFileMapping(
-        file_handle, nullptr, PAGE_READWRITE, 0, filesize, nullptr);
-    if (!mapping_handle)
+    HANDLE map = CreateFileMapping(handle, nullptr, PAGE_READWRITE, 0,
+                                   filesize, nullptr);
+    if (!map)
       Fatal(ctx) << path << ": CreateFileMapping failed: " << GetLastError();
 
-    this->buf =
-        (u8 *)MapViewOfFile(mapping_handle, FILE_MAP_WRITE, 0, 0, filesize);
-    CloseHandle(mapping_handle);
+    this->buf = (u8 *)MapViewOfFile(map, FILE_MAP_WRITE, 0, 0, filesize);
     if (!this->buf)
       Fatal(ctx) << path << ": MapViewOfFile failed: " << GetLastError();
 
+    CloseHandle(map);
+
     mold::output_buffer_start = this->buf;
     mold::output_buffer_end = this->buf + filesize;
   }
 
   ~MemoryMappedOutputFile() {
-    if (file_handle != INVALID_HANDLE_VALUE)
-      CloseHandle(file_handle);
+    if (handle != INVALID_HANDLE_VALUE)
+      CloseHandle(handle);
   }
 
   void close(Context &ctx) override {
@@ -47,28 +46,27 @@ public:
     UnmapViewOfFile(this->buf);
 
     if (!this->buf2.empty()) {
-      if (SetFilePointer(file_handle, 0, nullptr, FILE_END) ==
-          INVALID_SET_FILE_POINTER)
-        Fatal(ctx) << this->path
-                   << ": SetFilePointer failed: " << GetLastError();
+      if (SetFilePointer(handle, 0, nullptr, FILE_END) == INVALID_SET_FILE_POINTER)
+        Fatal(ctx) << this->path << ": SetFilePointer failed: "
+                   << GetLastError();
 
       DWORD written;
-      if (!WriteFile(file_handle, this->buf2.data(), this->buf2.size(),
-                     &written, nullptr))
+      if (!WriteFile(handle, this->buf2.data(), this->buf2.size(), &written,
+                     nullptr))
         Fatal(ctx) << this->path << ": WriteFile failed: " << GetLastError();
     }
 
-    CloseHandle(file_handle);
-    file_handle = INVALID_HANDLE_VALUE;
+    CloseHandle(handle);
+    handle = INVALID_HANDLE_VALUE;
   }
 
 private:
-  HANDLE file_handle;
+  HANDLE handle;
 };
 
 template <typename Context>
 std::unique_ptr<OutputFile<Context>>
-OutputFile<Context>::open(Context &ctx, std::string path, i64 filesize, i64 perm) {
+OutputFile<Context>::open(Context &ctx, std::string path, i64 filesize, int perm) {
   Timer t(ctx, "open_file");
 
   if (path.starts_with('/') && !ctx.arg.chroot.empty())
@@ -78,14 +76,13 @@ OutputFile<Context>::open(Context &ctx, std::string path, i64 filesize, i64 perm
   if (path == "-") {
     is_special = true;
   } else {
-    HANDLE file_handle =
-        CreateFileA(path.c_str(), GENERIC_READ,
-                    FILE_SHARE_READ | FILE_SHARE_WRITE | FILE_SHARE_DELETE,
-                    nullptr, OPEN_EXISTING, FILE_ATTRIBUTE_NORMAL, nullptr);
-    if (file_handle != INVALID_HANDLE_VALUE) {
-      if (GetFileType(file_handle) != FILE_TYPE_DISK)
+    HANDLE h = CreateFileA(path.c_str(), GENERIC_READ,
+                           FILE_SHARE_READ | FILE_SHARE_WRITE | FILE_SHARE_DELETE,
+                           nullptr, OPEN_EXISTING, FILE_ATTRIBUTE_NORMAL, nullptr);
+    if (h != INVALID_HANDLE_VALUE) {
+      if (GetFileType(h) != FILE_TYPE_DISK)
         is_special = true;
-      CloseHandle(file_handle);
+      CloseHandle(h);
     }
   }
 
@@ -100,4 +97,17 @@ OutputFile<Context>::open(Context &ctx, std::string path, i64 filesize, i64 perm
   return std::unique_ptr<OutputFile<Context>>(file);
 }
 
+template <typename Context>
+LockingOutputFile<Context>::LockingOutputFile(Context &ctx, std::string path,
+                                              int perm)
+  : OutputFile<Context>(path, 0, true) {
+  Fatal(ctx) << "LockingOutputFile is not supported on Windows";
+}
+
+template <typename Context>
+void LockingOutputFile<Context>::resize(Context &ctx, i64 filesize) {}
+
+template <typename Context>
+void LockingOutputFile<Context>::close(Context &ctx) {}
+
 } // namespace mold

docs/design.md

@@ -163,7 +163,7 @@ tool.
   (i.e. `_start`) or a few other root sections. In mold, we are using
   multiple threads to mark sections concurrently.
 
-- Similarly, BFD, gold an lld support Identical Comdat Folding (ICF)
+- Similarly, BFD, gold and lld support Identical Comdat Folding (ICF)
   as yet another size optimization. ICF merges two or more read-only
   sections that happen to have the same contents and relocations.
   To do that, we have to find isomorphic subgraphs from larger graphs.
@@ -381,7 +381,7 @@ not plan to implement and why I turned them down.
   fixing the final file layout.
 
   The other reason to reject this idea is because there's good a
-  chance for this idea to have a negative impact on linker's overall
+  chance for this idea to have a negative impact on the linker's overall
   performance. If we copy file contents before fixing the layout, we
   can't apply relocations to them while copying because symbol
   addresses are not available yet. If we fix the file layout first, we

elf/mold.h

@@ -1944,6 +1944,7 @@ struct Context {
   NotePropertySection<E> *note_property = nullptr;
   GdbIndexSection<E> *gdb_index = nullptr;
   RelroPaddingSection<E> *relro_padding = nullptr;
+  MergedSection<E> *comment = nullptr;
 
   [[no_unique_address]] ContextExtras<E> extra;
 

elf/output-chunks.cc

@@ -2070,6 +2070,33 @@ MergedSection<E>::insert(Context<E> &ctx, std::string_view data, u64 hash,
   return frag;
 }
 
+template <typename E>
+static std::string get_cmdline_args(Context<E> &ctx) {
+  std::stringstream ss;
+  ss << ctx.cmdline_args[1];
+  for (i64 i = 2; i < ctx.cmdline_args.size(); i++)
+    ss << " " << ctx.cmdline_args[i];
+  return ss.str();
+}
+
+// Add strings to .comment
+template <typename E>
+static void add_comment_strings(Context<E> &ctx) {
+  auto add = [&](std::string str) {
+    std::string_view buf = save_string(ctx, str);
+    std::string_view data(buf.data(), buf.size() + 1);
+    ctx.comment->insert(ctx, data, hash_string(data), 0);
+  };
+
+  // Add an identification string to .comment.
+  add(get_mold_version());
+
+  // Embed command line arguments for debugging.
+  char *env = getenv("MOLD_DEBUG");
+  if (env && env[0])
+    add("mold command line: " + get_cmdline_args(ctx));
+}
+
 template <typename E>
 void MergedSection<E>::resolve(Context<E> &ctx) {
   tbb::parallel_for_each(members, [&](MergeableSection<E> *sec) {
@@ -2083,6 +2110,8 @@ void MergedSection<E>::resolve(Context<E> &ctx) {
     sec->resolve_contents(ctx);
   });
 
+  if (this == ctx.comment)
+    add_comment_strings(ctx);
   resolved = true;
 }
 

elf/passes.cc

@@ -1,5 +1,6 @@
 #include "mold.h"
 #include "blake3.h"
+#include "../common/output-file.h"
 
 #include <fstream>
 #include <functional>
@@ -173,6 +174,13 @@ void create_synthetic_sections(Context<E> &ctx) {
   ctx.note_package = push(new NotePackageSection<E>);
   ctx.note_property = push(new NotePropertySection<E>);
 
+  if (!ctx.arg.oformat_binary) {
+    ElfShdr<E> shdr = {};
+    shdr.sh_type = SHT_PROGBITS;
+    shdr.sh_flags = SHF_MERGE | SHF_STRINGS;
+    ctx.comment = MergedSection<E>::get_instance(ctx, ".comment", shdr);
+  }
+
   if constexpr (is_riscv<E>)
     ctx.extra.riscv_attributes = push(new RiscvAttributesSection<E>);
 
@@ -421,33 +429,6 @@ void create_merged_sections(Context<E> &ctx) {
   tbb::parallel_for_each(ctx.objs, [&](ObjectFile<E> *file) {
     file->reattach_section_pieces(ctx);
   });
-
-  // Add strings to .comment
-  if (!ctx.arg.oformat_binary) {
-    ElfShdr<E> shdr = {};
-    shdr.sh_type = SHT_PROGBITS;
-    shdr.sh_flags = SHF_MERGE | SHF_STRINGS;
-
-    MergedSection<E> *sec = MergedSection<E>::get_instance(ctx, ".comment", shdr);
-    if (!sec->resolved) {
-      sec->map.resize(4096);
-      sec->resolved = true;
-    }
-
-    auto add = [&](std::string str) {
-      std::string_view buf = save_string(ctx, str);
-      std::string_view data(buf.data(), buf.size() + 1);
-      sec->insert(ctx, data, hash_string(data), 0);
-    };
-
-    // Add an identification string to .comment.
-    add(get_mold_version());
-
-    // Embed command line arguments for debugging.
-    char *env = getenv("MOLD_DEBUG");
-    if (env && env[0])
-      add("mold command line: " + get_cmdline_args(ctx));
-  }
 }
 
 template <typename E>
@@ -459,15 +440,6 @@ void convert_common_symbols(Context<E> &ctx) {
   });
 }
 
-template <typename E>
-static std::string get_cmdline_args(Context<E> &ctx) {
-  std::stringstream ss;
-  ss << ctx.cmdline_args[1];
-  for (i64 i = 2; i < ctx.cmdline_args.size(); i++)
-    ss << " " << ctx.cmdline_args[i];
-  return ss.str();
-}
-
 template <typename E>
 static bool has_ctors_and_init_array(Context<E> &ctx) {
   bool x = false;
@@ -1340,7 +1312,7 @@ void compute_section_sizes(Context<E> &ctx) {
   Timer t(ctx, "compute_section_sizes");
 
   if constexpr (needs_thunk<E>) {
-    // Chunk<E>::compute_section_size may obtain a global lock to create
+    // Chunk<E>::compute_section_size obtains a global lock to create
     // range extension thunks. I don't know why, but using parallel_for
     // loop both inside and outside of the lock may cause a deadlock. It
     // might be a bug in TBB. For now, I'll avoid using parallel_for_each
@@ -3056,6 +3028,10 @@ template <typename E>
 void write_separate_debug_file(Context<E> &ctx) {
   Timer t(ctx, "write_separate_debug_file");
 
+  // Open an output file early
+  LockingOutputFile<Context<E>> *file =
+    new LockingOutputFile<Context<E>>(ctx, ctx.arg.separate_debug_file, 0666);
+
   // We want to write to the debug info file in background so that the
   // user doesn't have to wait for it to complete.
   if (ctx.arg.detach)
@@ -3087,11 +3063,9 @@ void write_separate_debug_file(Context<E> &ctx) {
 
   // Write to the debug info file as if it were a regular output file.
   compute_section_headers(ctx);
-  i64 filesize = set_osec_offsets(ctx);
+  file->resize(ctx, set_osec_offsets(ctx));
 
-  ctx.output_file =
-    OutputFile<Context<E>>::open(ctx, ctx.arg.separate_debug_file,
-                                 filesize, 0666);
+  ctx.output_file.reset(file);
   ctx.buf = ctx.output_file->buf;
 
   copy_chunks(ctx);
@@ -3102,7 +3076,7 @@ void write_separate_debug_file(Context<E> &ctx) {
   // Reverse-compute a CRC32 value so that the CRC32 checksum embedded to
   // the .gnu_debuglink section in the main executable matches with the
   // debug info file's CRC32 checksum.
-  u32 crc = compute_crc32(0, ctx.buf, filesize);
+  u32 crc = compute_crc32(0, ctx.buf, ctx.output_file->filesize);
 
   std::vector<u8> &buf2 = ctx.output_file->buf2;
   if (!buf2.empty())

test/elf/separate-debug-file.sh

@@ -16,11 +16,12 @@ $CC -c -o $t/a.o $t/a.c -g
 $CC -B. -o $t/exe1 $t/a.o -Wl,--separate-debug-file
 readelf -SW $t/exe1 | grep -Fq .gnu_debuglink
 
+flock $t/exe1 true
+gdb $t/exe1 -ex 'list main' -ex 'quit' | grep -Fq printf
+
 $CC -c -o $t/a.o $t/a.c -g
 $CC -B. -o $t/exe2 $t/a.o -Wl,--separate-debug-file -Wl,--no-build-id
 readelf -SW $t/exe2 | grep -Fq .gnu_debuglink
 
-sleep 1
-
-gdb $t/exe1 -ex 'list main' -ex 'quit' | grep -Fq printf
+flock $t/exe2 true
 gdb $t/exe2 -ex 'list main' -ex 'quit' | grep -Fq printf