Rename F_no_mmap to F_mmap

[chestnykh]

The F_no_mmap flag was introduced by 6814232

[llvmbot]

@llvm/pr-subscribers-lld-elf

@llvm/pr-subscribers-llvm-support

Author: Dmitry Chestnykh (chestnykh)

Changes

---

Full diff: https://github.com/llvm/llvm-project/pull/134787.diff

5 Files Affected:

• (modified) lld/ELF/Arch/ARM.cpp (+1-1)
• (modified) lld/ELF/Writer.cpp (+2-2)
• (modified) llvm/include/llvm/Support/FileOutputBuffer.h (+2-3)
• (modified) llvm/lib/Support/FileOutputBuffer.cpp (+1-1)
• (modified) llvm/unittests/Support/FileOutputBufferTest.cpp (+1-1)

diff --git a/lld/ELF/Arch/ARM.cpp b/lld/ELF/Arch/ARM.cpp
index e667fdc0633c5..e45dd4d354afb 100644
--- a/lld/ELF/Arch/ARM.cpp
+++ b/lld/ELF/Arch/ARM.cpp
@@ -1489,7 +1489,7 @@ template <typename ELFT> void elf::writeARMCmseImportLib(Ctx &ctx) {
   const uint64_t fileSize =
       sectionHeaderOff + shnum * sizeof(typename ELFT::Shdr);
   const unsigned flags =
-      ctx.arg.mmapOutputFile ? 0 : (unsigned)FileOutputBuffer::F_no_mmap;
+      ctx.arg.mmapOutputFile ? (unsigned)FileOutputBuffer::F_mmap : 0;
   unlinkAsync(ctx.arg.cmseOutputLib);
   Expected<std::unique_ptr<FileOutputBuffer>> bufferOrErr =
       FileOutputBuffer::create(ctx.arg.cmseOutputLib, fileSize, flags);
diff --git a/lld/ELF/Writer.cpp b/lld/ELF/Writer.cpp
index 2cea6a44b391a..10ec1acba68c8 100644
--- a/lld/ELF/Writer.cpp
+++ b/lld/ELF/Writer.cpp
@@ -2902,8 +2902,8 @@ template <class ELFT> void Writer<ELFT>::openFile() {
   unsigned flags = 0;
   if (!ctx.arg.relocatable)
     flags |= FileOutputBuffer::F_executable;
-  if (!ctx.arg.mmapOutputFile)
-    flags |= FileOutputBuffer::F_no_mmap;
+  if (ctx.arg.mmapOutputFile)
+    flags |= FileOutputBuffer::F_mmap;
   Expected<std::unique_ptr<FileOutputBuffer>> bufferOrErr =
       FileOutputBuffer::create(ctx.arg.outputFile, fileSize, flags);
 
diff --git a/llvm/include/llvm/Support/FileOutputBuffer.h b/llvm/include/llvm/Support/FileOutputBuffer.h
index d4b73522115db..f98e7a5470b55 100644
--- a/llvm/include/llvm/Support/FileOutputBuffer.h
+++ b/llvm/include/llvm/Support/FileOutputBuffer.h
@@ -31,9 +31,8 @@ class FileOutputBuffer {
     /// Set the 'x' bit on the resulting file.
     F_executable = 1,
 
-    /// Don't use mmap and instead write an in-memory buffer to a file when this
-    /// buffer is closed.
-    F_no_mmap = 2,
+    /// Use mmap for in-memory file buffer.
+    F_mmap = 2,
   };
 
   /// Factory method to create an OutputBuffer object which manages a read/write
diff --git a/llvm/lib/Support/FileOutputBuffer.cpp b/llvm/lib/Support/FileOutputBuffer.cpp
index 58a06a34e8cf3..a2396d7629488 100644
--- a/llvm/lib/Support/FileOutputBuffer.cpp
+++ b/llvm/lib/Support/FileOutputBuffer.cpp
@@ -186,7 +186,7 @@ FileOutputBuffer::create(StringRef Path, size_t Size, unsigned Flags) {
   case fs::file_type::regular_file:
   case fs::file_type::file_not_found:
   case fs::file_type::status_error:
-    if (Flags & F_no_mmap)
+    if (Flags & F_mmap)
       return createInMemoryBuffer(Path, Size, Mode);
     else
       return createOnDiskBuffer(Path, Size, Mode);
diff --git a/llvm/unittests/Support/FileOutputBufferTest.cpp b/llvm/unittests/Support/FileOutputBufferTest.cpp
index f7bb0833e5a0e..423a6e12240c0 100644
--- a/llvm/unittests/Support/FileOutputBufferTest.cpp
+++ b/llvm/unittests/Support/FileOutputBufferTest.cpp
@@ -123,7 +123,7 @@ TEST(FileOutputBuffer, Test) {
   File5.append("/file5");
   {
     Expected<std::unique_ptr<FileOutputBuffer>> BufferOrErr =
-        FileOutputBuffer::create(File5, 8000, FileOutputBuffer::F_no_mmap);
+        FileOutputBuffer::create(File5, 8000, FileOutputBuffer::F_mmap);
     ASSERT_NO_ERROR(errorToErrorCode(BufferOrErr.takeError()));
     std::unique_ptr<FileOutputBuffer> &Buffer = *BufferOrErr;
     // Start buffer with special header.

[llvmbot]

@llvm/pr-subscribers-lld

Author: Dmitry Chestnykh (chestnykh)

Changes

---

Full diff: https://github.com/llvm/llvm-project/pull/134787.diff

5 Files Affected:

• (modified) lld/ELF/Arch/ARM.cpp (+1-1)
• (modified) lld/ELF/Writer.cpp (+2-2)
• (modified) llvm/include/llvm/Support/FileOutputBuffer.h (+2-3)
• (modified) llvm/lib/Support/FileOutputBuffer.cpp (+1-1)
• (modified) llvm/unittests/Support/FileOutputBufferTest.cpp (+1-1)

diff --git a/lld/ELF/Arch/ARM.cpp b/lld/ELF/Arch/ARM.cpp
index e667fdc0633c5..e45dd4d354afb 100644
--- a/lld/ELF/Arch/ARM.cpp
+++ b/lld/ELF/Arch/ARM.cpp
@@ -1489,7 +1489,7 @@ template <typename ELFT> void elf::writeARMCmseImportLib(Ctx &ctx) {
   const uint64_t fileSize =
       sectionHeaderOff + shnum * sizeof(typename ELFT::Shdr);
   const unsigned flags =
-      ctx.arg.mmapOutputFile ? 0 : (unsigned)FileOutputBuffer::F_no_mmap;
+      ctx.arg.mmapOutputFile ? (unsigned)FileOutputBuffer::F_mmap : 0;
   unlinkAsync(ctx.arg.cmseOutputLib);
   Expected<std::unique_ptr<FileOutputBuffer>> bufferOrErr =
       FileOutputBuffer::create(ctx.arg.cmseOutputLib, fileSize, flags);
diff --git a/lld/ELF/Writer.cpp b/lld/ELF/Writer.cpp
index 2cea6a44b391a..10ec1acba68c8 100644
--- a/lld/ELF/Writer.cpp
+++ b/lld/ELF/Writer.cpp
@@ -2902,8 +2902,8 @@ template <class ELFT> void Writer<ELFT>::openFile() {
   unsigned flags = 0;
   if (!ctx.arg.relocatable)
     flags |= FileOutputBuffer::F_executable;
-  if (!ctx.arg.mmapOutputFile)
-    flags |= FileOutputBuffer::F_no_mmap;
+  if (ctx.arg.mmapOutputFile)
+    flags |= FileOutputBuffer::F_mmap;
   Expected<std::unique_ptr<FileOutputBuffer>> bufferOrErr =
       FileOutputBuffer::create(ctx.arg.outputFile, fileSize, flags);
 
diff --git a/llvm/include/llvm/Support/FileOutputBuffer.h b/llvm/include/llvm/Support/FileOutputBuffer.h
index d4b73522115db..f98e7a5470b55 100644
--- a/llvm/include/llvm/Support/FileOutputBuffer.h
+++ b/llvm/include/llvm/Support/FileOutputBuffer.h
@@ -31,9 +31,8 @@ class FileOutputBuffer {
     /// Set the 'x' bit on the resulting file.
     F_executable = 1,
 
-    /// Don't use mmap and instead write an in-memory buffer to a file when this
-    /// buffer is closed.
-    F_no_mmap = 2,
+    /// Use mmap for in-memory file buffer.
+    F_mmap = 2,
   };
 
   /// Factory method to create an OutputBuffer object which manages a read/write
diff --git a/llvm/lib/Support/FileOutputBuffer.cpp b/llvm/lib/Support/FileOutputBuffer.cpp
index 58a06a34e8cf3..a2396d7629488 100644
--- a/llvm/lib/Support/FileOutputBuffer.cpp
+++ b/llvm/lib/Support/FileOutputBuffer.cpp
@@ -186,7 +186,7 @@ FileOutputBuffer::create(StringRef Path, size_t Size, unsigned Flags) {
   case fs::file_type::regular_file:
   case fs::file_type::file_not_found:
   case fs::file_type::status_error:
-    if (Flags & F_no_mmap)
+    if (Flags & F_mmap)
       return createInMemoryBuffer(Path, Size, Mode);
     else
       return createOnDiskBuffer(Path, Size, Mode);
diff --git a/llvm/unittests/Support/FileOutputBufferTest.cpp b/llvm/unittests/Support/FileOutputBufferTest.cpp
index f7bb0833e5a0e..423a6e12240c0 100644
--- a/llvm/unittests/Support/FileOutputBufferTest.cpp
+++ b/llvm/unittests/Support/FileOutputBufferTest.cpp
@@ -123,7 +123,7 @@ TEST(FileOutputBuffer, Test) {
   File5.append("/file5");
   {
     Expected<std::unique_ptr<FileOutputBuffer>> BufferOrErr =
-        FileOutputBuffer::create(File5, 8000, FileOutputBuffer::F_no_mmap);
+        FileOutputBuffer::create(File5, 8000, FileOutputBuffer::F_mmap);
     ASSERT_NO_ERROR(errorToErrorCode(BufferOrErr.takeError()));
     std::unique_ptr<FileOutputBuffer> &Buffer = *BufferOrErr;
     // Start buffer with special header.

[chestnykh]

The windows bot error happened on infra step

[MaskRay]

Thanks! Can you add a link of the patch that introduced F_no_mmap to the description?

[chestnykh]

Thanks! Can you add a link of the patch that introduced F_no_mmap to the description?

Ofc, done

[dgg5503]

Hi @chestnykh ,

We noticed that these changes have caused the LIT test lld\test\ELF\link-open-file.test to fail under our upstream Windows Build Bot here: https://lab.llvm.org/buildbot/#/builders/46/builds/14847

I've confirmed locally that reverting these changes allow the LIT test to pass.

May you please investigate & fix this failure or revert your change to get this build bot back to green?

[jrtc27]

In future please include the why in the commit message, not just the what. There is no explanation for why this way round is better.

(Part of https://llvm.org/docs/DeveloperPolicy.html#commit-messages, but also just generally good software engineering practice)

[dgg5503]

Hi @chestnykh ,

I have reverted these changes in an attempt to get the llvm-clang-x86_64-sie-win Build Bot passing again. I did a small amount of investigation into the failure -- I am curious if the logic here:

llvm-project/llvm/lib/Support/FileOutputBuffer.cpp

Lines 189 to 192 in d6c8e89

	if (Flags & F_mmap)
	return createInMemoryBuffer(Path, Size, Mode);
	else
	return createOnDiskBuffer(Path, Size, Mode);

needs to be flipped. If my understanding is correct, then I believe flags should also be set to 0 in

llvm-project/llvm/unittests/Support/FileOutputBufferTest.cpp

Line 126 in d6c8e89

FileOutputBuffer::create(File5, 8000, FileOutputBuffer::F_mmap);

Please let me know what you think.

[MaskRay]

Hi @chestnykh ,

I have reverted these changes in an attempt to get the llvm-clang-x86_64-sie-win Build Bot passing again. I did a small amount of investigation into the failure -- I am curious if the logic here:

llvm-project/llvm/lib/Support/FileOutputBuffer.cpp

Lines 189 to 192 in d6c8e89

	if (Flags & F_mmap)
	return createInMemoryBuffer(Path, Size, Mode);
	else
	return createOnDiskBuffer(Path, Size, Mode);

needs to be flipped. If my understanding is correct, then I believe flags should also be set to 0 in

llvm-project/llvm/unittests/Support/FileOutputBufferTest.cpp

Line 126 in d6c8e89

FileOutputBuffer::create(File5, 8000, FileOutputBuffer::F_mmap);

Please let me know what you think.

--no-mmap-output-file is actually our default. This PR did not change the default, and actually switched from createOnDiskBuffer to createInMemoryBuffer.

I've sent a reland #139836 with an explanation.

@dgg5503 Can you investigate why lld::link-open-file.test failed with createInMemoryBuffer?

   return createInMemoryBuffer(Path, Size, Mode); 

[dgg5503]

Hi @MaskRay,

--no-mmap-output-file is actually our default. This PR did not change the default, and actually switched from createOnDiskBuffer to createInMemoryBuffer.

Ah, that explains it. Thanks for picking up on this detail & creating a PR to reland this change.

I can confirm that with #139836, the test lld\test\ELF\link-open-file.test passes under Windows using a configuration similar to llvm-clang-x86_64-sie-win.

@dgg5503 Can you investigate why lld::link-open-file.test failed with createInMemoryBuffer?

Yes! I did some investigation this morning and discovered that changing OF_None to OF_Delete here:

llvm-project/llvm/lib/Support/FileOutputBuffer.cpp

Line 102 in 8c43588

openFileForWrite(FinalPath, FD, CD_CreateAlways, OF_None, Mode))

will allow the test lld\test\ELF\link-open-file.test to pass if it were augmented to also test --mmap-output-file.

In createInMemoryBuffer, an InMemoryBuffer is returned which will commit the file via openFileForWrite:

llvm-project/llvm/lib/Support/FileOutputBuffer.cpp

Lines 117 to 125 in 8c43588

	static Expected<std::unique_ptr<InMemoryBuffer>>
	createInMemoryBuffer(StringRef Path, size_t Size, unsigned Mode) {
	std::error_code EC;
	MemoryBlock MB = Memory::allocateMappedMemory(
	Size, nullptr, sys::Memory::MF_READ | sys::Memory::MF_WRITE, EC);
	if (EC)
	return errorCodeToError(EC);
	return std::make_unique<InMemoryBuffer>(Path, MB, Size, Mode);
	}

llvm-project/llvm/lib/Support/FileOutputBuffer.cpp

Lines 91 to 107 in 8c43588

	Error commit() override {
	if (FinalPath == "-") {
	llvm::outs() << StringRef((const char *)Buffer.base(), BufferSize);
	llvm::outs().flush();
	return Error::success();
	}
	using namespace sys::fs;
	int FD;
	std::error_code EC;
	if (auto EC =
	openFileForWrite(FinalPath, FD, CD_CreateAlways, OF_None, Mode))
	return errorCodeToError(EC);
	raw_fd_ostream OS(FD, /*shouldClose=*/true, /*unbuffered=*/true);
	OS << StringRef((const char *)Buffer.base(), BufferSize);
	return Error::success();
	}

I think adding OF_Delete to openFileForWrite in InMemoryBuffer::commit makes sense because in createOnDiskBuffer, TempFile::create is called which also sets OF_Delete here:

llvm-project/llvm/lib/Support/FileOutputBuffer.cpp

Lines 127 to 130 in 8c43588

	static Expected<std::unique_ptr<FileOutputBuffer>>
	createOnDiskBuffer(StringRef Path, size_t Size, unsigned Mode) {
	Expected<fs::TempFile> FileOrErr =
	fs::TempFile::create(Path + ".tmp%%%%%%%", Mode);

llvm-project/llvm/lib/Support/Path.cpp

Lines 1329 to 1330 in 61c1f6d

	if (std::error_code EC =
	createUniqueFile(Model, FD, ResultPath, OF_Delete | ExtraFlags, Mode))

The Windows only test lld\test\ELF\link-open-file.test is verifying that a shared file opened with FILE_SHARE_DELETE is capable of being overwritten by LLD , and a shared file opened without FILE_SHARE_DELETE raises an error under LLD when attempted to be overwritten:

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

Lines 1 to 5 in 61c1f6d

	## 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

If I'm understanding MSDN documentation correctly, when the test opens the output file without FILE_SHARE_DELETE, it would only fail if the file or device has been opened for delete access which, in LLD, TempFile does but openFileForWrite does not.

So I suppose we should set OF_Delete accordingly, does that sound correct? If you agree with my findings, I'd be happy to create a PR and small associated test.

[MaskRay]

So I suppose we should set OF_Delete accordingly, does that sound correct? If you agree with my findings, I'd be happy to create a PR and small associated test.

Thanks for the investigation! I think I'm lost in the Windows details (I am not familiar with it) but please create a PR and update the test :)