[LLD][ELF][Windows] Allow LLD to overwrite existing output files that are in use

[bd1976llvm]

Summary

This patch allows LLD to write over an existing output file that is in use - if the program using it has opened it in FILE_SHARE_DELETE mode.

https://reviews.llvm.org/D82542 must land first otherwise this will introduce a 2s delay if the output file does not exist.

[jhenderson]

Somebody else should look at the functional aspects of this.

lld/test/ELF/link-open-file.py

1 ↗

(On Diff #273381)

1. This test won't run unless executed explicitly with lit, because it has a .py extension, which is not a recognised suffix. Try breaking the test, and then running the lld testsuite as a whole - you'll not see the failure (that is unless somebody's changed the config very recently...). You'll either need to change the file extension to e.g. .test, or update lld's test config.

2. Nit: Many newer tests use '##' for comments to distinguish them from the lit and FileCheck directives.

3. Does this test work for both python 2 and python 3? I believe the LLVM policy is still to support both.

55 ↗

(On Diff #273381)

Nit: missing trailing full stop.

63 ↗

(On Diff #273381)

f.read(4) is simpler than the array slicing.

[bd1976llvm]

Thanks James - you were correct that the test was not being executed!

• I have renamed the test to have the extension ".test". I am also looking into the possibility of adding a diagnostic to LIT for this problem.
• I verified that the test is compatible with both pythons 2 and 3.
• I have addressed your other comments.

[jhenderson]

lld/test/ELF/link-open-file.test

64

You've not addressed my f.read(4) suggestion!

[bd1976llvm]

Add back python improvement that was somehow dropped during test rename!

[grimar]

Looks probably good to me, have 3 nits though.

lld/Common/Filesystem.cpp

70

You should probably wrap the internal if logic into curly braces:

if (!sys::fs::createUniqueFile(path + "%%%%%%%%.tmp", tmpName)) {
  if (!sys::fs::rename(path, tmpName))
    path = tmpName;
  else
    sys::fs::remove(tmpName);
}

Otherwise you perhaps might get a compiler warning. At least GCC would complain with:
""error: add explicit braces to avoid dangling else [-Werror,-Wdangling-else]",
see b21f32fcecd012fa2c2f8c61d7259079a7f1865e.

I believe this place is not expected to be compiled with GCC, but I guess some similar warning might be reported by MSVS compiler too.

lld/test/ELF/link-open-file.test

6

Shouldn't it be # REQUIRES: system-windows, x86?

8

This can be a single line:

echo '.globl _start; _start:' > %t.s

[psmith]

This looks OK to me, happy for George to approve . I'm not a Windows filesystem expert by any means, it seems like the rename then delete follows the pattern in https://boostgsoc13.github.io/boost.afio/doc/html/afio/FAQ/deleting_open_files.html though.

lld/test/ELF/link-open-file.test

6

Yes in theory someone could run this on Windows on Arm without an X86 backend compiled into llvm-mc. Vanishingly small probability of it happening but worth guarding against.

[bd1976llvm]

Thanks for the suggested improvements.

I have put https://reviews.llvm.org/D83069 to avoid others writing a lit test that won't actually be run.

[grimar]

LGTM

lld/Common/Filesystem.cpp

 //
 //===----------------------------------------------------------------------===//
 
 #include "lld/Common/Filesystem.h"
 #include "llvm/Config/llvm-config.h"
 #include "llvm/Support/FileOutputBuffer.h"
 #include "llvm/Support/FileSystem.h"
 #include "llvm/Support/Parallel.h"
+#include "llvm/Support/Path.h"
 #if LLVM_ON_UNIX
 #include <unistd.h>
 #endif
 #include <thread>
 
 using namespace llvm;
 using namespace lld;
 

 // 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)
+  // On Windows co-operative programs can be expected to open LLD's
+  // output in FILE_SHARE_DELETE mode. This allows us to delete the
+  // file (by moving it to a temporary filename and then deleting
+  // it) so that we can link another output file that overwrites
+  // the existing file, even if the current file is in use.
+  //
+  // This is done on a best effort basis - we do not error if the
+  // operation fails. The consequence is merely that the user
+  // experiences an inconvenient work-flow.
+  //
+  // The code here allows LLD to work on all versions of Windows.
+  // However, at Windows 10 1903 it seems that the behavior of
+  // Windows has changed, so that we could simply delete the output 
+  // file. This code should be simplified once support for older
+  // versions of Windows is dropped.
+  //
+  // Warning: It seems that the WINVER and _WIN32_WINNT preprocessor
+  // defines affect the behavior of the Windows versions of the calls
+  // we are using here. If this code stops working this is worth
+  // bearing in mind.
+  SmallString<128> tmpName;
+  if (!sys::fs::createUniqueFile(path + "%%%%%%%%.tmp", tmpName)) {
+    if (!sys::fs::rename(path, tmpName))
+      path = tmpName;
+    else
+      sys::fs::remove(tmpName);
+  }
   sys::fs::remove(path);
 #else
   if (parallel::strategy.ThreadsRequested == 1 || !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.

lld/test/ELF/link-open-file.test

+## On Windows co-operative applications can be expected to open LLD's output
+## with FILE_SHARE_DELETE included in the sharing mode. This allows us to link
+## over the top of an existing file even if it is in use by another application.
+
+# REQUIRES: system-windows, x86
+# RUN: echo '.globl _start; _start:' > %t.s
+# RUN: llvm-mc -filetype=obj -triple=x86_64-unknown-unknown %t.s -o %t.o
+
+## FILE_SHARE_READ   = 1
+## FILE_SHARE_WRITE  = 2
+## FILE_SHARE_DELETE = 4
+
+# RUN:     %python %s %t.o 7
+# RUN: not %python %s %t.o 3 2>&1 | FileCheck %s
+# CHECK: error: failed to write to the output file
+
+import contextlib
+import ctypes
+from ctypes import wintypes as w
+import os
+import shutil
+import subprocess
+import platform
+import sys
+import time
+
+object_file = sys.argv[1]
+share_flags = int(sys.argv[2])
+
[e-mail]
+def open_with_share_flags(filename, share_flags):
+    GENERIC_READ          = 0x80000000
+    FILE_ATTRIBUTE_NORMAL = 0x80
+    OPEN_EXISTING         = 0x3
+    INVALID_HANDLE_VALUE = w.HANDLE(-1).value
+
+    CreateFileA = ctypes.windll.kernel32.CreateFileA
+    CreateFileA.restype = w.HANDLE
+    h = CreateFileA(filename.encode('mbcs'), GENERIC_READ, share_flags,
+                    None, OPEN_EXISTING, FILE_ATTRIBUTE_NORMAL, None)
+
+    assert h != INVALID_HANDLE_VALUE, 'Failed to open ' + filename
+    try:
+        yield
+    finally:
+        ctypes.windll.kernel32.CloseHandle(h)
+
+## Ensure we have an empty directory for the output.
+outdir = os.path.basename(__file__) + '.dir'
+if os.path.exists(outdir):
+    shutil.rmtree(outdir)
+os.makedirs(outdir)
+
+## Link on top of an open file.
+elf = os.path.join(outdir, 'output_file.elf')
+open(elf, 'wb').close()
+with open_with_share_flags(elf, share_flags):
+    subprocess.check_call(['ld.lld.exe', object_file, '-o', elf])
+
+## Check the linker wrote the output file.
+with open(elf, 'rb') as f:
+    assert f.read(4) == b'\x7fELF', "linker did not write output file correctly"
+
+## Check no temp files are left around.
+## It might take a while for Windows to remove them, so loop.
+deleted = lambda: len(os.listdir(outdir)) == 1
+for _ in range(10):
+    if not deleted():
+        time.sleep (1)
+
+assert deleted(), "temp file(s) not deleted after grace period"