Merge 46c4b826aa 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/bugs.md

@@ -88,9 +88,9 @@ Problem: A statically-linked "hello world" program crashes after
 reading a thread-local variable.
 
 Investigation: Thread-local variables are very different from other
-types of varaibles because there may be more than one instance of the
+types of variables because there may be more than one instance of the
 same variable in memory. Each thread has its copy of thread-local
-varaibles. `%fs` segment register points the end of the variable area
+variables. `%fs` segment register points the end of the variable area
 for the current thread, and the variables are accessed as an offset
 from `%fs`.
 

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