[Support] Set OF_Delete for InMemoryBuffer's call to openFileForWrite under commit
#140109
Conversation
… under commit llvm#134787 unintentionally enabled `--mmap-output-file` by default under LLD which caused the Windows-only test `lld\test\ELF\link-open-file.test` to fail. This failure uncovered what appears to be an inconsistency on Windows between `createOnDiskBuffer` and `createInMemoryBuffer` with respect to `DELETE` access for the output file. The output file created by `createOnDiskBuffer` sets the flag `OF_Delete` as part of `fs::TempFile::create` while the output file created by `createInMemoryBuffer` sets `OF_None` under `InMemoryBuffer::commit`. The test `lld\test\ELF\link-open-file.test` ensures that if `FILE_SHARE_DELETE` is _not_ specified for an output file that LLD is expected to overwrite, LLD should fail. This only happens if: "the file or device has been opened for delete access" which is only done for `fs::TempFile::create`. See https://learn.microsoft.com/en-us/windows/win32/api/fileapi/nf-fileapi-createfilew#FILE_SHARE_DELETE. Therefore, I propose setting `OF_Delete` for `InMemoryBuffer::commit`'s call to `openFileForWrite` to stay consistent with `fs::TempFile::create`.
|
@llvm/pr-subscribers-llvm-support Author: Douglas (dgg5503) Changes#134787 unintentionally enabled The output file created by The test See https://learn.microsoft.com/en-us/windows/win32/api/fileapi/nf-fileapi-createfilew#FILE_SHARE_DELETE. Therefore, I propose setting Full diff: https://github.com/llvm/llvm-project/pull/140109.diff 2 Files Affected:
diff --git a/lld/test/ELF/link-open-file.test b/lld/test/ELF/link-open-file.test
index 17c7ba95e6ebe..8693a53ead5d5 100644
--- a/lld/test/ELF/link-open-file.test
+++ b/lld/test/ELF/link-open-file.test
@@ -10,8 +10,10 @@
## 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
+# RUN: %python %s %t.o 7 false
+# RUN: not %python %s %t.o 3 false 2>&1 | FileCheck %s
+# RUN: %python %s %t.o 7 true
+# RUN: not %python %s %t.o 3 true 2>&1 | FileCheck %s
# CHECK: error: failed to write output '{{.*}}': {{.*}}
import contextlib
@@ -26,6 +28,7 @@ import time
object_file = sys.argv[1]
share_flags = int(sys.argv[2])
+use_mmap = bool(sys.argv[3])
@contextlib.contextmanager
def open_with_share_flags(filename, share_flags):
@@ -55,7 +58,10 @@ os.makedirs(outdir)
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])
+ args = ['ld.lld.exe', object_file, '-o', elf]
+ if use_mmap:
+ args.append("--mmap-output-file")
+ subprocess.check_call(args)
## Check the linker wrote the output file.
with open(elf, 'rb') as f:
diff --git a/llvm/lib/Support/FileOutputBuffer.cpp b/llvm/lib/Support/FileOutputBuffer.cpp
index a2396d7629488..c8b035986fb5b 100644
--- a/llvm/lib/Support/FileOutputBuffer.cpp
+++ b/llvm/lib/Support/FileOutputBuffer.cpp
@@ -99,7 +99,7 @@ class InMemoryBuffer : public FileOutputBuffer {
int FD;
std::error_code EC;
if (auto EC =
- openFileForWrite(FinalPath, FD, CD_CreateAlways, OF_None, Mode))
+ openFileForWrite(FinalPath, FD, CD_CreateAlways, OF_Delete, Mode))
return errorCodeToError(EC);
raw_fd_ostream OS(FD, /*shouldClose=*/true, /*unbuffered=*/true);
OS << StringRef((const char *)Buffer.base(), BufferSize);
|
|
@llvm/pr-subscribers-lld Author: Douglas (dgg5503) Changes#134787 unintentionally enabled The output file created by The test See https://learn.microsoft.com/en-us/windows/win32/api/fileapi/nf-fileapi-createfilew#FILE_SHARE_DELETE. Therefore, I propose setting Full diff: https://github.com/llvm/llvm-project/pull/140109.diff 2 Files Affected:
diff --git a/lld/test/ELF/link-open-file.test b/lld/test/ELF/link-open-file.test
index 17c7ba95e6ebe..8693a53ead5d5 100644
--- a/lld/test/ELF/link-open-file.test
+++ b/lld/test/ELF/link-open-file.test
@@ -10,8 +10,10 @@
## 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
+# RUN: %python %s %t.o 7 false
+# RUN: not %python %s %t.o 3 false 2>&1 | FileCheck %s
+# RUN: %python %s %t.o 7 true
+# RUN: not %python %s %t.o 3 true 2>&1 | FileCheck %s
# CHECK: error: failed to write output '{{.*}}': {{.*}}
import contextlib
@@ -26,6 +28,7 @@ import time
object_file = sys.argv[1]
share_flags = int(sys.argv[2])
+use_mmap = bool(sys.argv[3])
@contextlib.contextmanager
def open_with_share_flags(filename, share_flags):
@@ -55,7 +58,10 @@ os.makedirs(outdir)
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])
+ args = ['ld.lld.exe', object_file, '-o', elf]
+ if use_mmap:
+ args.append("--mmap-output-file")
+ subprocess.check_call(args)
## Check the linker wrote the output file.
with open(elf, 'rb') as f:
diff --git a/llvm/lib/Support/FileOutputBuffer.cpp b/llvm/lib/Support/FileOutputBuffer.cpp
index a2396d7629488..c8b035986fb5b 100644
--- a/llvm/lib/Support/FileOutputBuffer.cpp
+++ b/llvm/lib/Support/FileOutputBuffer.cpp
@@ -99,7 +99,7 @@ class InMemoryBuffer : public FileOutputBuffer {
int FD;
std::error_code EC;
if (auto EC =
- openFileForWrite(FinalPath, FD, CD_CreateAlways, OF_None, Mode))
+ openFileForWrite(FinalPath, FD, CD_CreateAlways, OF_Delete, Mode))
return errorCodeToError(EC);
raw_fd_ostream OS(FD, /*shouldClose=*/true, /*unbuffered=*/true);
OS << StringRef((const char *)Buffer.base(), BufferSize);
|
|
@llvm/pr-subscribers-lld-elf Author: Douglas (dgg5503) Changes#134787 unintentionally enabled The output file created by The test See https://learn.microsoft.com/en-us/windows/win32/api/fileapi/nf-fileapi-createfilew#FILE_SHARE_DELETE. Therefore, I propose setting Full diff: https://github.com/llvm/llvm-project/pull/140109.diff 2 Files Affected:
diff --git a/lld/test/ELF/link-open-file.test b/lld/test/ELF/link-open-file.test
index 17c7ba95e6ebe..8693a53ead5d5 100644
--- a/lld/test/ELF/link-open-file.test
+++ b/lld/test/ELF/link-open-file.test
@@ -10,8 +10,10 @@
## 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
+# RUN: %python %s %t.o 7 false
+# RUN: not %python %s %t.o 3 false 2>&1 | FileCheck %s
+# RUN: %python %s %t.o 7 true
+# RUN: not %python %s %t.o 3 true 2>&1 | FileCheck %s
# CHECK: error: failed to write output '{{.*}}': {{.*}}
import contextlib
@@ -26,6 +28,7 @@ import time
object_file = sys.argv[1]
share_flags = int(sys.argv[2])
+use_mmap = bool(sys.argv[3])
@contextlib.contextmanager
def open_with_share_flags(filename, share_flags):
@@ -55,7 +58,10 @@ os.makedirs(outdir)
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])
+ args = ['ld.lld.exe', object_file, '-o', elf]
+ if use_mmap:
+ args.append("--mmap-output-file")
+ subprocess.check_call(args)
## Check the linker wrote the output file.
with open(elf, 'rb') as f:
diff --git a/llvm/lib/Support/FileOutputBuffer.cpp b/llvm/lib/Support/FileOutputBuffer.cpp
index a2396d7629488..c8b035986fb5b 100644
--- a/llvm/lib/Support/FileOutputBuffer.cpp
+++ b/llvm/lib/Support/FileOutputBuffer.cpp
@@ -99,7 +99,7 @@ class InMemoryBuffer : public FileOutputBuffer {
int FD;
std::error_code EC;
if (auto EC =
- openFileForWrite(FinalPath, FD, CD_CreateAlways, OF_None, Mode))
+ openFileForWrite(FinalPath, FD, CD_CreateAlways, OF_Delete, Mode))
return errorCodeToError(EC);
raw_fd_ostream OS(FD, /*shouldClose=*/true, /*unbuffered=*/true);
OS << StringRef((const char *)Buffer.base(), BufferSize);
|
There was a problem hiding this comment.
Thanks!
I now see that https://reviews.llvm.org/D82567 introduced the Windows-specific test lld/test/ELF/link-open-file.test .
The test appears to open the output file with FILE_SHARE_DELETE using Python, then invoke lld. If lld opens the output file with FILE_SHARE_DELETE, it will be able to delete it.
#134787 unintentionally enabled
--mmap-output-fileby default under LLD which caused the Windows-only testlld\test\ELF\link-open-file.testto fail. This failure uncovered what appears to be an inconsistency on Windows betweencreateOnDiskBufferandcreateInMemoryBufferwith respect toDELETEaccess for the output file.The output file created by
createOnDiskBuffersets the flagOF_Deleteas part offs::TempFile::createwhile the output file created bycreateInMemoryBuffersetsOF_NoneunderInMemoryBuffer::commit.The test
lld\test\ELF\link-open-file.testensures that ifFILE_SHARE_DELETEis not specified for an output file that LLD is expected to overwrite, LLD should fail. This only happens if: "the file or device has been opened for delete access" which is only done forfs::TempFile::create.See https://learn.microsoft.com/en-us/windows/win32/api/fileapi/nf-fileapi-createfilew#FILE_SHARE_DELETE.
Therefore, I propose setting
OF_DeleteforInMemoryBuffer::commit's call toopenFileForWriteto stay consistent withfs::TempFile::create.This PR stems from my findings here: #134787 (comment)