[CAS] Add MappedFileRegionArena #114099
[CAS] Add MappedFileRegionArena #114099
Conversation
Created using spr 1.3.5 [skip ci]
Created using spr 1.3.5
Created using spr 1.3.6 [skip ci]
Created using spr 1.3.6
cachemeifyoucan
changed the base branch from
users/cachemeifyoucan/spr/main.cas-add-mappedfileregionbumpptr
to
main
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
Created using spr 1.3.6
Created using spr 1.3.6
Created using spr 1.3.6
|
@llvm/pr-subscribers-platform-windows @llvm/pr-subscribers-llvm-support Author: Steven Wu (cachemeifyoucan) ChangesAdd MappedFileRegionBumpPtr which can be served as a file system backed The implementation relies on the POSIX compliance of file system and The allocator works by using a atomically updated bump ptr at a location Patch is 33.29 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/114099.diff 12 Files Affected:
diff --git a/llvm/CMakeLists.txt b/llvm/CMakeLists.txt
index b672cb9365284..7e53472754cf6 100644
--- a/llvm/CMakeLists.txt
+++ b/llvm/CMakeLists.txt
@@ -891,6 +891,14 @@ option (LLVM_ENABLE_OCAMLDOC "Build OCaml bindings documentation." ON)
option (LLVM_ENABLE_BINDINGS "Build bindings." ON)
option (LLVM_ENABLE_TELEMETRY "Enable the telemetry library. If set to OFF, library cannot be enabled after build (eg., at runtime)" ON)
+# Default to build OnDiskCAS on 64-bit systems.
+if(CMAKE_SIZEOF_VOID_P GREATER_EQUAL 8)
+ set(LLVM_ENABLE_ONDISK_CAS_default ON)
+else()
+ set(LLVM_ENABLE_ONDISK_CAS_default OFF)
+endif()
+option(LLVM_ENABLE_ONDISK_CAS "Build OnDiskCAS." ${LLVM_ENABLE_ONDISK_CAS_default})
+
set(LLVM_INSTALL_DOXYGEN_HTML_DIR "${CMAKE_INSTALL_DOCDIR}/llvm/doxygen-html"
CACHE STRING "Doxygen-generated HTML documentation install directory")
set(LLVM_INSTALL_OCAMLDOC_HTML_DIR "${CMAKE_INSTALL_DOCDIR}/llvm/ocaml-html"
diff --git a/llvm/include/llvm/CAS/MappedFileRegionBumpPtr.h b/llvm/include/llvm/CAS/MappedFileRegionBumpPtr.h
new file mode 100644
index 0000000000000..395205cde4e50
--- /dev/null
+++ b/llvm/include/llvm/CAS/MappedFileRegionBumpPtr.h
@@ -0,0 +1,132 @@
+//===- MappedFileRegionBumpPtr.h --------------------------------*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+//
+/// \file
+/// This file declares interface for MappedFileRegionBumpPtr, a bump pointer
+/// allocator, backed by a memory-mapped file.
+///
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_CAS_MAPPEDFILEREGIONBUMPPTR_H
+#define LLVM_CAS_MAPPEDFILEREGIONBUMPPTR_H
+
+#include "llvm/Support/Alignment.h"
+#include "llvm/Support/FileSystem.h"
+#include <atomic>
+
+namespace llvm::cas {
+
+/// Allocator for an owned mapped file region that supports thread-safe and
+/// process-safe bump pointer allocation.
+///
+/// This allocator is designed to create a sparse file when supported by the
+/// filesystem's \c ftruncate so that it can be used with a large maximum size.
+/// It will also attempt to shrink the underlying file down to its current
+/// allocation size when the last concurrent mapping is closed.
+///
+/// Process-safe. Uses file locks when resizing the file during initialization
+/// and destruction.
+///
+/// Thread-safe, assuming all threads use the same instance to talk to a given
+/// file/mapping. Unsafe to have multiple instances talking to the same file
+/// in the same process since file locks will misbehave. Clients should
+/// coordinate (somehow).
+///
+/// \note Currently we allocate the whole file without sparseness on Windows.
+///
+/// Provides 8-byte alignment for all allocations.
+class MappedFileRegionBumpPtr {
+public:
+ using RegionT = sys::fs::mapped_file_region;
+
+ /// Create a \c MappedFileRegionBumpPtr.
+ ///
+ /// \param Path the path to open the mapped region.
+ /// \param Capacity the maximum size for the mapped file region.
+ /// \param BumpPtrOffset the offset at which to store the bump pointer.
+ /// \param NewFileConstructor is for constructing new files. It has exclusive
+ /// access to the file. Must call \c initializeBumpPtr.
+ static Expected<MappedFileRegionBumpPtr>
+ create(const Twine &Path, uint64_t Capacity, int64_t BumpPtrOffset,
+ function_ref<Error(MappedFileRegionBumpPtr &)> NewFileConstructor);
+
+ /// Finish initializing the bump pointer. Must be called by
+ /// \c NewFileConstructor.
+ void initializeBumpPtr(int64_t BumpPtrOffset);
+
+ /// Minimum alignment for allocations, currently hardcoded to 8B.
+ static constexpr Align getAlign() {
+ // Trick Align into giving us '8' as a constexpr.
+ struct alignas(8) T {};
+ static_assert(alignof(T) == 8, "Tautology failed?");
+ return Align::Of<T>();
+ }
+
+ /// Allocate at least \p AllocSize. Rounds up to \a getAlign().
+ Expected<char *> allocate(uint64_t AllocSize) {
+ auto Offset = allocateOffset(AllocSize);
+ if (LLVM_UNLIKELY(!Offset))
+ return Offset.takeError();
+ return data() + *Offset;
+ }
+ /// Allocate, returning the offset from \a data() instead of a pointer.
+ Expected<int64_t> allocateOffset(uint64_t AllocSize);
+
+ char *data() const { return Region.data(); }
+ uint64_t size() const { return H->BumpPtr; }
+ uint64_t capacity() const { return Region.size(); }
+
+ RegionT &getRegion() { return Region; }
+
+ ~MappedFileRegionBumpPtr() { destroyImpl(); }
+
+ MappedFileRegionBumpPtr() = default;
+ MappedFileRegionBumpPtr(MappedFileRegionBumpPtr &&RHS) { moveImpl(RHS); }
+ MappedFileRegionBumpPtr &operator=(MappedFileRegionBumpPtr &&RHS) {
+ destroyImpl();
+ moveImpl(RHS);
+ return *this;
+ }
+
+ MappedFileRegionBumpPtr(const MappedFileRegionBumpPtr &) = delete;
+ MappedFileRegionBumpPtr &operator=(const MappedFileRegionBumpPtr &) = delete;
+
+private:
+ // The file size increment to extend the storage size.
+ // The minimum increment is a page, but allocate more to amortize the cost.
+ static constexpr int64_t Increment = 4 * 1024 * 1024; // 4 MB
+
+ // Extend the AllocatedSize to be enough to hold NewEnd.
+ Error extendSpaceImpl(int64_t NewEnd);
+
+ void destroyImpl();
+ void moveImpl(MappedFileRegionBumpPtr &RHS) {
+ std::swap(Region, RHS.Region);
+ std::swap(H, RHS.H);
+ std::swap(Path, RHS.Path);
+ std::swap(FD, RHS.FD);
+ std::swap(SharedLockFD, RHS.SharedLockFD);
+ }
+
+private:
+ struct Header {
+ std::atomic<int64_t> BumpPtr;
+ std::atomic<int64_t> AllocatedSize;
+ };
+ RegionT Region;
+ Header *H = nullptr;
+ std::string Path;
+ // File descriptor for the main storage file.
+ std::optional<int> FD;
+ // File descriptor for the file used as reader/writer lock.
+ std::optional<int> SharedLockFD;
+};
+
+} // namespace llvm::cas
+
+#endif // LLVM_CAS_MAPPEDFILEREGIONBUMPPTR_H
diff --git a/llvm/include/llvm/Config/llvm-config.h.cmake b/llvm/include/llvm/Config/llvm-config.h.cmake
index 39136bc45c292..2a5c580f336f2 100644
--- a/llvm/include/llvm/Config/llvm-config.h.cmake
+++ b/llvm/include/llvm/Config/llvm-config.h.cmake
@@ -146,4 +146,7 @@
coverage bugs, and to 0 otherwise. */
#cmakedefine01 LLVM_ENABLE_DEBUGLOC_TRACKING_ORIGIN
+/* Define to 1 to enable LLVM OnDisk Content Addressable Storage*/
+#cmakedefine01 LLVM_ENABLE_ONDISK_CAS
+
#endif
diff --git a/llvm/include/llvm/Support/FileSystem.h b/llvm/include/llvm/Support/FileSystem.h
index ee7a77c578747..9bc2753e878a5 100644
--- a/llvm/include/llvm/Support/FileSystem.h
+++ b/llvm/include/llvm/Support/FileSystem.h
@@ -410,6 +410,11 @@ LLVM_ABI std::error_code copy_file(const Twine &From, int ToFD);
/// platform-specific error_code.
LLVM_ABI std::error_code resize_file(int FD, uint64_t Size);
+/// Resize path to size with sparse files explicitly enabled. It uses
+/// FSCTL_SET_SPARSE On Windows. This is the same as resize_file on
+/// non-Windows
+LLVM_ABI std::error_code resize_file_sparse(int FD, uint64_t Size);
+
/// Resize \p FD to \p Size before mapping \a mapped_file_region::readwrite. On
/// non-Windows, this calls \a resize_file(). On Windows, this is a no-op,
/// since the subsequent mapping (via \c CreateFileMapping) automatically
diff --git a/llvm/lib/CAS/CMakeLists.txt b/llvm/lib/CAS/CMakeLists.txt
index b2825a171ec31..5f9aaede0576a 100644
--- a/llvm/lib/CAS/CMakeLists.txt
+++ b/llvm/lib/CAS/CMakeLists.txt
@@ -1,7 +1,9 @@
add_llvm_component_library(LLVMCAS
BuiltinCAS.cpp
InMemoryCAS.cpp
+ MappedFileRegionBumpPtr.cpp
ObjectStore.cpp
+ OnDiskCommon.cpp
ADDITIONAL_HEADER_DIRS
${LLVM_MAIN_INCLUDE_DIR}/llvm/CAS
diff --git a/llvm/lib/CAS/MappedFileRegionBumpPtr.cpp b/llvm/lib/CAS/MappedFileRegionBumpPtr.cpp
new file mode 100644
index 0000000000000..211fc67706cda
--- /dev/null
+++ b/llvm/lib/CAS/MappedFileRegionBumpPtr.cpp
@@ -0,0 +1,319 @@
+//===----------------------------------------------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+/// \file Implements MappedFileRegionBumpPtr.
+///
+/// A bump pointer allocator, backed by a memory-mapped file.
+///
+/// The effect we want is:
+///
+/// 1. If it doesn't exist, create the file with an initial size.
+/// 2. Reserve virtual memory large enough for the max file size.
+/// 3. Map the file into memory in the reserved region.
+/// 4. Increase the file size and update the mapping when necessary.
+///
+/// However, updating the mapping is challenging when it needs to work portably,
+/// and across multiple processes without locking for every read. Our current
+/// implementation strategy is:
+///
+/// 1. Use \c ftruncate (\c sys::fs::resize_file) to grow the file to its max
+/// size (typically several GB). Many modern filesystems will create a sparse
+/// file, so that the trailing unused pages do not take space on disk.
+/// 2. Call \c mmap (\c sys::fs::mapped_file_region)
+/// 3. [Automatic as part of 2.]
+/// 4. [Automatic as part of 2.]
+///
+/// Additionally, we attempt to resize the file to its actual data size when
+/// closing the mapping, if this is the only concurrent instance. This is done
+/// using file locks. Shrinking the file mitigates problems with having large
+/// files: on filesystems without sparse files it avoids unnecessary space use;
+/// it also avoids allocating the full size if another process copies the file,
+/// which typically loses sparseness. These mitigations only work while the file
+/// is not in use.
+///
+/// TODO: we assume that all concurrent users of the file will use the same
+/// value for Capacity. Otherwise a process with a larger capacity can write
+/// data that is "out of bounds" for processes with smaller capacity. Currently
+/// this is true in the CAS.
+///
+/// To support resizing, we use two separate file locks:
+/// 1. We use a shared reader lock on a ".shared" file until destruction.
+/// 2. We use a lock on the main file during initialization - shared to check
+/// the status, upgraded to exclusive to resize/initialize the file.
+///
+/// Then during destruction we attempt to get exclusive access on (1), which
+/// requires no concurrent readers. If so, we shrink the file. Using two
+/// separate locks simplifies the implementation and enables it to work on
+/// platforms (e.g. Windows) where a shared/reader lock prevents writing.
+//===----------------------------------------------------------------------===//
+
+#include "llvm/CAS/MappedFileRegionBumpPtr.h"
+#include "OnDiskCommon.h"
+
+#if LLVM_ON_UNIX
+#include <sys/stat.h>
+#if __has_include(<sys/param.h>)
+#include <sys/param.h>
+#endif
+#ifdef DEV_BSIZE
+#define MAPPED_FILE_BSIZE DEV_BSIZE
+#elif __linux__
+#define MAPPED_FILE_BSIZE 512
+#endif
+#endif
+
+using namespace llvm;
+using namespace llvm::cas;
+using namespace llvm::cas::ondisk;
+
+namespace {
+struct FileLockRAII {
+ std::string Path;
+ int FD;
+ enum LockKind { Shared, Exclusive };
+ std::optional<LockKind> Locked;
+
+ FileLockRAII(std::string Path, int FD) : Path(std::move(Path)), FD(FD) {}
+ ~FileLockRAII() { consumeError(unlock()); }
+
+ Error lock(LockKind LK) {
+ if (std::error_code EC = lockFileThreadSafe(FD, LK == Exclusive))
+ return createFileError(Path, EC);
+ Locked = LK;
+ return Error::success();
+ }
+
+ Error unlock() {
+ if (Locked) {
+ Locked = std::nullopt;
+ if (std::error_code EC = unlockFileThreadSafe(FD))
+ return createFileError(Path, EC);
+ }
+ return Error::success();
+ }
+};
+
+struct FileSizeInfo {
+ uint64_t Size;
+ uint64_t AllocatedSize;
+
+ static ErrorOr<FileSizeInfo> get(sys::fs::file_t File);
+};
+} // end anonymous namespace
+
+Expected<MappedFileRegionBumpPtr> MappedFileRegionBumpPtr::create(
+ const Twine &Path, uint64_t Capacity, int64_t BumpPtrOffset,
+ function_ref<Error(MappedFileRegionBumpPtr &)> NewFileConstructor) {
+ MappedFileRegionBumpPtr Result;
+ Result.Path = Path.str();
+ // Open the main file.
+ int FD;
+ if (std::error_code EC = sys::fs::openFileForReadWrite(
+ Result.Path, FD, sys::fs::CD_OpenAlways, sys::fs::OF_None))
+ return createFileError(Path, EC);
+ Result.FD = FD;
+
+ // Open the shared lock file. See file comment for details of locking scheme.
+ SmallString<128> SharedLockPath(Result.Path);
+ SharedLockPath.append(".shared");
+ int SharedLockFD;
+ if (std::error_code EC = sys::fs::openFileForReadWrite(
+ SharedLockPath, SharedLockFD, sys::fs::CD_OpenAlways,
+ sys::fs::OF_None))
+ return createFileError(SharedLockPath, EC);
+ Result.SharedLockFD = SharedLockFD;
+
+ // Take shared/reader lock that will be held until we close the file; unlocked
+ // by destroyImpl.
+ if (std::error_code EC =
+ lockFileThreadSafe(SharedLockFD, /*Exclusive=*/false))
+ return createFileError(Path, EC);
+
+ // Take shared/reader lock for initialization.
+ FileLockRAII InitLock(Result.Path, FD);
+ if (Error E = InitLock.lock(FileLockRAII::Shared))
+ return std::move(E);
+
+ sys::fs::file_t File = sys::fs::convertFDToNativeFile(FD);
+ auto FileSize = FileSizeInfo::get(File);
+ if (!FileSize)
+ return createFileError(Result.Path, FileSize.getError());
+
+ if (FileSize->Size < Capacity) {
+ // Lock the file exclusively so only one process will do the initialization.
+ if (Error E = InitLock.unlock())
+ return std::move(E);
+ if (Error E = InitLock.lock(FileLockRAII::Exclusive))
+ return std::move(E);
+ // Retrieve the current size now that we have exclusive access.
+ FileSize = FileSizeInfo::get(File);
+ if (!FileSize)
+ return createFileError(Result.Path, FileSize.getError());
+ }
+
+ // At this point either the file is still under-sized, or we have the size for
+ // the completely initialized file.
+
+ if (FileSize->Size < Capacity) {
+ // We are initializing the file; it may be empty, or may have been shrunk
+ // during a previous close.
+ // FIXME: Detect a case where someone opened it with a smaller capacity.
+ // FIXME: On Windows we should use FSCTL_SET_SPARSE and FSCTL_SET_ZERO_DATA
+ // to make this a sparse region, if supported.
+ assert(InitLock.Locked == FileLockRAII::Exclusive);
+ if (std::error_code EC = sys::fs::resize_file_sparse(FD, Capacity))
+ return createFileError(Result.Path, EC);
+ } else {
+ // Someone else initialized it.
+ Capacity = FileSize->Size;
+ }
+
+ // Create the mapped region.
+ {
+ std::error_code EC;
+ sys::fs::mapped_file_region Map(
+ File, sys::fs::mapped_file_region::readwrite, Capacity, 0, EC);
+ if (EC)
+ return createFileError(Result.Path, EC);
+ Result.Region = std::move(Map);
+ }
+
+ if (FileSize->Size == 0) {
+ assert(InitLock.Locked == FileLockRAII::Exclusive);
+ // We are creating a new file; run the constructor.
+ if (Error E = NewFileConstructor(Result))
+ return std::move(E);
+ } else {
+ Result.initializeBumpPtr(BumpPtrOffset);
+ }
+
+ if (FileSize->Size < Capacity && FileSize->AllocatedSize < Capacity) {
+ // We are initializing the file; sync the allocated size in case it
+ // changed when truncating or during construction.
+ FileSize = FileSizeInfo::get(File);
+ if (!FileSize)
+ return createFileError(Result.Path, FileSize.getError());
+ assert(InitLock.Locked == FileLockRAII::Exclusive);
+ Result.H->AllocatedSize.exchange(FileSize->AllocatedSize);
+ }
+
+ return Result;
+}
+
+void MappedFileRegionBumpPtr::destroyImpl() {
+ if (!FD)
+ return;
+
+ // Drop the shared lock indicating we are no longer accessing the file.
+ if (SharedLockFD)
+ (void)unlockFileThreadSafe(*SharedLockFD);
+
+ // Attempt to truncate the file if we can get exclusive access. Ignore any
+ // errors.
+ if (H) {
+ assert(SharedLockFD && "Must have shared lock file open");
+ if (tryLockFileThreadSafe(*SharedLockFD) == std::error_code()) {
+ size_t Size = size();
+ size_t Capacity = capacity();
+ assert(Size < Capacity);
+ // sync to file system to make sure all contents are up-to-date.
+ (void)Region.sync();
+ // unmap the file before resizing since that is the requirement for
+ // some platforms.
+ Region.unmap();
+ (void)sys::fs::resize_file(*FD, Size);
+ (void)unlockFileThreadSafe(*SharedLockFD);
+ }
+ }
+
+ auto Close = [](std::optional<int> &FD) {
+ if (FD) {
+ sys::fs::file_t File = sys::fs::convertFDToNativeFile(*FD);
+ sys::fs::closeFile(File);
+ FD = std::nullopt;
+ }
+ };
+
+ // Close the file and shared lock.
+ Close(FD);
+ Close(SharedLockFD);
+}
+
+void MappedFileRegionBumpPtr::initializeBumpPtr(int64_t BumpPtrOffset) {
+ assert(capacity() < (uint64_t)INT64_MAX && "capacity must fit in int64_t");
+ int64_t BumpPtrEndOffset = BumpPtrOffset + sizeof(decltype(*H));
+ assert(BumpPtrEndOffset <= (int64_t)capacity() &&
+ "Expected end offset to be pre-allocated");
+ assert(isAligned(Align::Of<decltype(*H)>(), BumpPtrOffset) &&
+ "Expected end offset to be aligned");
+ H = reinterpret_cast<decltype(H)>(data() + BumpPtrOffset);
+
+ int64_t ExistingValue = 0;
+ if (!H->BumpPtr.compare_exchange_strong(ExistingValue, BumpPtrEndOffset))
+ assert(ExistingValue >= BumpPtrEndOffset &&
+ "Expected 0, or past the end of the BumpPtr itself");
+}
+
+static Error createAllocatorOutOfSpaceError() {
+ return createStringError(std::make_error_code(std::errc::not_enough_memory),
+ "memory mapped file allocator is out of space");
+}
+
+Expected<int64_t> MappedFileRegionBumpPtr::allocateOffset(uint64_t AllocSize) {
+ AllocSize = alignTo(AllocSize, getAlign());
+ int64_t OldEnd = H->BumpPtr.fetch_add(AllocSize);
+ int64_t NewEnd = OldEnd + AllocSize;
+ if (LLVM_UNLIKELY(NewEnd > (int64_t)capacity())) {
+ // Return the allocation. If the start already passed the end, that means
+ // some other concurrent allocations already consumed all the capacity.
+ // There is no need to return the original value. If the start was not
+ // passed the end, current allocation certainly bumped it passed the end.
+ // All other allocation afterwards must have failed and current allocation
+ // is in charge of return the allocation back to a valid value.
+ if (OldEnd <= (int64_t)capacity())
+ (void)H->BumpPtr.exchange(OldEnd);
+
+ return createAllocatorOutOfSpaceError();
+ }
+
+ int64_t DiskSize = H->AllocatedSize;
+ if (LLVM_UNLIKELY(NewEnd > DiskSize)) {
+ int64_t NewSize;
+ // The minimum increment is a page, but allocate more to amortize the cost.
+ constexpr int64_t Increment = 1 * 1024 * 1024; // 1 MB
+ if (Error E = preallocateFileTail(*FD, DiskSize, DiskSize + Increment)
+ .moveInto(NewSize))
+ return std::move(E);
+ assert(NewSize >= DiskSize + Increment);
+ // FIXME: on Darwin this can under-count the size if there is a race to
+ // preallocate disk, because the semantics of F_PREALLOCATE are to add bytes
+ // to the end of the file, not to allocate up to a fixed size.
+ // Any discrepancy will be resolved the next time the file is truncated and
+ // then reopend.
+ while (DiskSize < NewSize)
+ H->AllocatedSize.compare_exchange_strong(DiskSize, NewSize);
+ }
+ return OldEnd;
+}
+
+ErrorOr<FileSizeInfo> FileSizeInfo::get(sys::fs::file_t File) {
+#if LLVM_ON_UNIX && defined(MAPPED_FILE_BSIZE)
+ struct stat Status;
+ int StatRet = ::fstat(File, &Status);
+ if (StatRet)
+ return errnoAsErrorCode();
+ uint64_t AllocatedSize = uint64_t(Status.st_blksize) * MAPPED_FILE_BSIZE;
+ return FileSizeInfo{uint64_t(Status.st_size), AllocatedSize};
+#else
+ // Fallback: assume the file is fully allocated. Note: this may result in
+ // data loss on out-of-space.
+ sys::fs::file_status Status;
+ if (std::error_code EC = sys::fs::status(File, Status))
+ return EC;
+ return FileSizeInfo{Status.getSize(), Statu...
[truncated]
|
Created using spr 1.3.6
cachemeifyoucan
requested review from
bogner,
adrian-prantl,
ilovepi,
rnk and
dwblaikie
Created using spr 1.3.6
Created using spr 1.3.6
|
ping |
Created using spr 1.3.6
Created using spr 1.3.6
Created using spr 1.3.6
Created using spr 1.3.6
Created using spr 1.3.6
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/154/builds/21514 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/13/builds/9575 Here is the relevant piece of the build log for the reference |
|
This patch breaks both the Solaris/sparcv9 and Solaris/amd64 buildbots. |
|
This breaks compilation with older macOS SDKs (Xcode 13 and older, with the macOS 12 SDK or older), with errors like these: And if building with a new enough SDK that contains this constant, how does it behave it attempting to execute it on an older version of macOS? |
|
(cross posting from Discord) Tests crashing on armv8 32-bit as well - https://lab.llvm.org/buildbot/#/builders/154/builds/21514 Actually I'm not sure this is a "crash" as such, maybe it's just printing the traceback that got it to the assert. Hard to tell. Going to reproduce it locally. |
|
I've reproduced it. Getting a debug build on 32-bit is nigh on impossible these days so this is a bit of guess work. This is where some thread crashes. I think it crashing then causes the assert failure in the main thread. This is doing:
We load from C = 12 so we are not 8 byte aligned here. https://developer.arm.com/documentation/ddi0360/e/programmer-s-model/additional-instructions/ldrexd I think this is generated from: And I suspect it's the HeaderOffset member of: If we assume that So: Cannot be totally true, it must be at the correct alignment for these types. Maybe you intended it to be, and there's an ABI difference on Arm 32-bit that breaks this assumption. E.g. a size_t next to a uint64_t. I'll try a few things but I need to look at this header offset math first. |
| uint64_t HeaderEndOffset = HeaderOffset + sizeof(decltype(*H)); | ||
| assert(HeaderEndOffset <= capacity() && | ||
| "Expected end offset to be pre-allocated"); | ||
| assert(isAligned(Align::Of<decltype(*H)>(), HeaderOffset) && |
There was a problem hiding this comment.
Here you check that the offset is aligned, but not that the final address of the header will be aligned.
The assert message says end offset but I think it means the beginning. Unless the header offset is the offset to the end of the header but I don't think so:
// Offset from the beginning of the file to this header (for verification).
There was a problem hiding this comment.
If the final address is not aligned, that means the mmap doesn't return an aligned address. I guess that is possible, and can be a risk when using double word instructions that has alignment requirements. GCC might have compiled with a different instruction without alignment requirement.
The workaround should disable it on 32 bit system, where this can be a problem. I guess one solution is maybe I can make the structure packed on 32 bit system but that is not super desirable. Let me think about it.
There was a problem hiding this comment.
Can you point me to where the mmap call is made? And any other places you might be making assumptions, I can double check what's coming back.
There was a problem hiding this comment.
I think mmap is supposed to be page aligned, which should be a very strong guarantee.
The reason why checking HeaderOffset alignment is enough is because that is the offset from the beginning of the memory mapped region. I think I know what might have happened. It is a different read and can you try this patch if you have the setup? #159128
|
Actually I checked a bunch of points along the way and the addresses appear to be ok. Compiling the same thing with the host gcc does not crash, but I don't know yet if it's codegen or some implementation defined decision making the difference. |
|
Feels like a code generation issue but I'm not sure of it. I tried various versions of clang and all of them had the same problem. Next step is to reduce the input down to either stumble upon a rouge +4, or get something I can reduce to IR and report a bug. So leave this feature disabled for 32-bit and I will handle this problem for now. |
I will disable solaris as well. I need to read some documentation about that. It seems the OS is running out of lock resources? I don't think too much locks are used so I will workaround while I investigate. |
Let me fix that. This code path is not strictly needed for correctness. This code path only introduces safety margin when disk is almost out of place. |
@rorth Is Solaris using NFS? |
|
QEMU instructions are here - https://linaro.atlassian.net/wiki/spaces/TCWGPUB/pages/29360783374/How+to+Reproduce+32+bit+Arm+Builds+Without+Hardware I have not attempted to reproduce it that way yet, it may not especially with a gcc build. Try it if you like but no requirement to do so. |
Thanks, though the bots will take a while to catch up.
Not on the bots: everything is local there. I've meanwhile run a local build which reproduces the errors. In that case, the source tree comes per NFS while the build directory is local. However, I cannot yet make sense of what I'm seeing there: running under but I don't see it returned/set anywhere in the LLVM sources either. |
|
@rorth It seems that the |
#159133 should address that. |
There's something very wrong here: Solaris has neither |
... which explains what's going on: I searched for |
Ah, that makes sense. I guess |
|
|
|
Ideally, we need |
|
Hi, the AIX bot was not building |
I guess it would be way better to either skip or xfail the affected tests in |
Fix: #159594 |
The CMake check has updated to checking I definitely want to create a centralized document for FIXME and TODO for the CAS implementations. Maybe a GitHub issue or just added in the doc page: https://llvm.org/docs/ContentAddressableStorage.html (I don't know if there are precedents for doing that). |
Add MappedFileRegionArena which can be served as a file system backed persistent memory allocator. The allocator works like a BumpPtrAllocator, and is designed to be thread safe and process safe. The implementation relies on the POSIX compliance of file system and doesn't work on all file systems. If the file system supports lazy tail (doesn't allocate disk space if the tail of the large file is not used), user has more flexibility to declare a larger capacity. The allocator works by using a atomically updated bump ptr at a location that can be customized by the user. The atomic pointer points to the next available space to allocate, and the allocator will resize/truncate to current usage once all clients closed the allocator. Windows implementation contributed by: @hjyamauchi
Fix a buildbot failure on some bots using gcc: ``` MappedFileRegionArena.cpp:275:10: error: could not convert ‘Result’ from ‘llvm::cas::MappedFileRegionArena’ to ‘llvm::Expected<llvm::cas::MappedFileRegionArena>’ ``` This is caused by RVO not activated causing the type check error.
I think most people file a top level github issues and then track/link related issues as sub tasks. There's also milestones and some other features that I've seen various folks use to track work. |
Add MappedFileRegionArena which can be served as a file system backed persistent memory allocator. The allocator works like a BumpPtrAllocator, and is designed to be thread safe and process safe. The implementation relies on the POSIX compliance of file system and doesn't work on all file systems. If the file system supports lazy tail (doesn't allocate disk space if the tail of the large file is not used), user has more flexibility to declare a larger capacity. The allocator works by using a atomically updated bump ptr at a location that can be customized by the user. The atomic pointer points to the next available space to allocate, and the allocator will resize/truncate to current usage once all clients closed the allocator. Windows implementation contributed by: @hjyamauchi
Fix a buildbot failure on some bots using gcc: ``` MappedFileRegionArena.cpp:275:10: error: could not convert ‘Result’ from ‘llvm::cas::MappedFileRegionArena’ to ‘llvm::Expected<llvm::cas::MappedFileRegionArena>’ ``` This is caused by RVO not activated causing the type check error.
Add MappedFileRegionArena which can be served as a file system backed
persistent memory allocator. The allocator works like a BumpPtrAllocator,
and is designed to be thread safe and process safe.
The implementation relies on the POSIX compliance of file system and
doesn't work on all file systems. If the file system supports lazy tail
(doesn't allocate disk space if the tail of the large file is not used),
user has more flexibility to declare a larger capacity.
The allocator works by using a atomically updated bump ptr at a location
that can be customized by the user. The atomic pointer points to the
next available space to allocate, and the allocator will resize/truncate
to current usage once all clients closed the allocator.
Windows implementation contributed by: @hjyamauchi