Page MenuHomePhabricator

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

Authored by bd1976llvm on Jun 25 2020, 8:48 AM.

Details

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.

Diff Detail

Event Timeline

bd1976llvm created this revision.Jun 25 2020, 8:48 AM

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.
  1. Nit: Many newer tests use '##' for comments to distinguish them from the lit and FileCheck directives.
  1. 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.

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 added inline comments.Jul 1 2020, 12:36 AM
lld/test/ELF/link-open-file.test
64

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

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

bd1976llvm marked an inline comment as done.Jul 1 2020, 12:48 AM
grimar added a comment.Jul 1 2020, 1:54 AM

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 added a subscriber: psmith.Jul 1 2020, 1:01 PM

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 marked 4 inline comments as done.

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 accepted this revision.Jul 3 2020, 1:01 AM

LGTM

This revision is now accepted and ready to land.Jul 3 2020, 1:01 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptJul 3 2020, 5:21 AM