refactor: add unique_mmap smart pointer and use it

Author: diegonehab

src/jsonrpc-remote-machine.cpp

@@ -1138,7 +1138,7 @@ static json jsonrpc_machine_read_memory_handler(const json &j, const std::shared
     auto args = parse_args<uint64_t, uint64_t>(j, param_name);
     auto address = std::get<0>(args);
     auto length = std::get<1>(args);
-    auto data = cartesi::unique_calloc<unsigned char>(length);
+    auto data = cartesi::make_unique_calloc<unsigned char>(length);
     session->handler->machine->read_memory(address, data.get(), length);
     return jsonrpc_response_ok(j, cartesi::encode_base64(data.get(), length));
 }
@@ -1172,7 +1172,7 @@ static json jsonrpc_machine_read_virtual_memory_handler(const json &j, const std
     auto args = parse_args<uint64_t, uint64_t>(j, param_name);
     auto address = std::get<0>(args);
     auto length = std::get<1>(args);
-    auto data = cartesi::unique_calloc<unsigned char>(length);
+    auto data = cartesi::make_unique_calloc<unsigned char>(length);
     session->handler->machine->read_virtual_memory(address, data.get(), length);
     return jsonrpc_response_ok(j, cartesi::encode_base64(data.get(), length));
 }

src/machine.cpp

@@ -51,7 +51,6 @@
 #include "machine-memory-range-descr.h"
 #include "machine-reg.h"
 #include "machine-runtime-config.h"
-#include "os.h"
 #include "plic-factory.h"
 #include "pma-constants.h"
 #include "pma-defines.h"
@@ -384,7 +383,7 @@ machine::machine(machine_config c, machine_runtime_config r) : m_c{std::move(c)}
         }
         // Auto detect flash drive image length
         if (f.length == UINT64_C(-1)) {
-            auto fp = unique_fopen(f.image_filename.c_str(), "rb");
+            auto fp = make_unique_fopen(f.image_filename.c_str(), "rb");
             if (fseek(fp.get(), 0, SEEK_END) != 0) {
                 throw std::system_error{errno, std::generic_category(),
                     "unable to obtain length of image file '"s + f.image_filename + "' when initializing "s +
@@ -524,11 +523,8 @@ machine::machine(machine_config c, machine_runtime_config r) : m_c{std::move(c)}
     // Initialize TLB device.
     // This must be done after all PMA entries are already registered, so we can lookup page addresses
     if (!m_c.tlb.image_filename.empty()) {
-        unsigned char *buf = os_map_file(m_c.tlb.image_filename.c_str(), PMA_SHADOW_TLB_LENGTH, false);
-        // NOLINTNEXTLINE(cppcoreguidelines-pro-type-reinterpret-cast)
-        const auto &shadow_tlb = *reinterpret_cast<const shadow_tlb_state *>(buf);
-        init_tlb(shadow_tlb);
-        os_unmap_file(buf, PMA_SHADOW_TLB_LENGTH);
+        auto shadow_tlb = make_unique_mmap<shadow_tlb_state>(m_c.tlb.image_filename.c_str(), 1, false /* not shared */);
+        init_tlb(*shadow_tlb);
     } else {
         init_tlb();
     }
@@ -573,7 +569,7 @@ machine::machine(machine_config c, machine_runtime_config r) : m_c{std::move(c)}
 
 static void load_hash(const std::string &dir, machine::hash_type &h) {
     auto name = dir + "/hash";
-    auto fp = unique_fopen(name.c_str(), "rb");
+    auto fp = make_unique_fopen(name.c_str(), "rb");
     if (fread(h.data(), 1, h.size(), fp.get()) != h.size()) {
         throw std::runtime_error{"error reading from '" + name + "'"};
     }
@@ -737,9 +733,9 @@ static void store_device_pma(const machine &m, const pma_entry &pma, const std::
     if (!pma.get_istart_IO()) {
         throw std::runtime_error{"attempt to save non-device PMA"};
     }
-    auto scratch = unique_calloc<unsigned char>(PMA_PAGE_SIZE); // will throw if it fails
+    auto scratch = make_unique_calloc<unsigned char>(PMA_PAGE_SIZE); // will throw if it fails
     auto name = machine_config::get_image_filename(dir, pma.get_start(), pma.get_length());
-    auto fp = unique_fopen(name.c_str(), "wb");
+    auto fp = make_unique_fopen(name.c_str(), "wb");
     for (uint64_t page_start_in_range = 0; page_start_in_range < pma.get_length();
         page_start_in_range += PMA_PAGE_SIZE) {
         const unsigned char *page_data = nullptr;
@@ -762,7 +758,7 @@ static void store_memory_pma(const pma_entry &pma, const std::string &dir) {
         throw std::runtime_error{"attempt to save non-memory PMA"};
     }
     auto name = machine_config::get_image_filename(dir, pma.get_start(), pma.get_length());
-    auto fp = unique_fopen(name.c_str(), "wb");
+    auto fp = make_unique_fopen(name.c_str(), "wb");
     const pma_memory &mem = pma.get_memory();
     if (fwrite(mem.get_host_memory(), 1, pma.get_length(), fp.get()) != pma.get_length()) {
         throw std::runtime_error{"error writing to '" + name + "'"};
@@ -938,7 +934,7 @@ void machine::store_pmas(const machine_config &c, const std::string &dir) const
 
 static void store_hash(const machine::hash_type &h, const std::string &dir) {
     auto name = dir + "/hash";
-    auto fp = unique_fopen(name.c_str(), "wb");
+    auto fp = make_unique_fopen(name.c_str(), "wb");
     if (fwrite(h.data(), 1, h.size(), fp.get()) != h.size()) {
         throw std::runtime_error{"error writing to '" + name + "'"};
     }
@@ -1820,7 +1816,7 @@ bool machine::verify_dirty_page_maps() const {
     static_assert(PMA_PAGE_SIZE == machine_merkle_tree::get_page_size(),
         "PMA and machine_merkle_tree page sizes must match");
     machine_merkle_tree::hasher_type h;
-    auto scratch = unique_calloc<unsigned char>(PMA_PAGE_SIZE, std::nothrow_t{});
+    auto scratch = make_unique_calloc<unsigned char>(PMA_PAGE_SIZE, std::nothrow_t{});
     if (!scratch) {
         return false;
     }
@@ -1885,7 +1881,7 @@ bool machine::update_merkle_tree() const {
         // runtime config or as the hardware supports.
         const uint64_t n = get_task_concurrency(m_r.concurrency.update_merkle_tree);
         const bool succeeded = os_parallel_for(n, [&](int j, const parallel_for_mutex &mutex) -> bool {
-            auto scratch = unique_calloc<unsigned char>(PMA_PAGE_SIZE, std::nothrow_t{});
+            auto scratch = make_unique_calloc<unsigned char>(PMA_PAGE_SIZE, std::nothrow_t{});
             if (!scratch) {
                 return false;
             }
@@ -1949,7 +1945,7 @@ bool machine::update_merkle_tree_page(uint64_t address) {
     pma_entry &pma = find_pma_entry(m_merkle_pmas, address, sizeof(uint64_t));
     const uint64_t page_start_in_range = address - pma.get_start();
     machine_merkle_tree::hasher_type h;
-    auto scratch = unique_calloc<unsigned char>(PMA_PAGE_SIZE, std::nothrow_t{});
+    auto scratch = make_unique_calloc<unsigned char>(PMA_PAGE_SIZE, std::nothrow_t{});
     if (!scratch) {
         return false;
     }
@@ -1999,7 +1995,7 @@ machine::hash_type machine::get_merkle_tree_node_hash(uint64_t address, int log2
         throw std::domain_error{"address not aligned to log2_size"};
     }
     const auto size = UINT64_C(1) << log2_size;
-    auto scratch = unique_calloc<unsigned char>(size);
+    auto scratch = make_unique_calloc<unsigned char>(size);
     read_memory(address, scratch.get(), size);
     machine_merkle_tree::hasher_type h;
     machine_merkle_tree::hash_type hash;
@@ -2064,7 +2060,7 @@ machine_merkle_tree::proof_type machine::get_proof(uint64_t address, int log2_si
     if (log2_size < machine_merkle_tree::get_log2_page_size()) {
         const uint64_t length = UINT64_C(1) << log2_size;
         const pma_entry &pma = find_pma_entry(m_merkle_pmas, address, length);
-        auto scratch = unique_calloc<unsigned char>(PMA_PAGE_SIZE);
+        auto scratch = make_unique_calloc<unsigned char>(PMA_PAGE_SIZE);
         const unsigned char *page_data = nullptr;
         // If the PMA range is empty, we know the desired range is
         // entirely outside of any non-pristine PMA.
@@ -2491,16 +2487,15 @@ interpreter_break_reason machine::log_step(uint64_t mcycle_count, const std::str
 interpreter_break_reason machine::verify_step(const hash_type &root_hash_before, const std::string &filename,
     uint64_t mcycle_count, const hash_type &root_hash_after) {
     auto data_length = os_get_file_length(filename.c_str(), "step log file");
-    auto *data = os_map_file(filename.c_str(), data_length, false /* not shared */);
+    auto data = make_unique_mmap<unsigned char>(filename.c_str(), data_length, false /* not shared */);
     replay_step_state_access::context context;
-    replay_step_state_access a(context, data, data_length, root_hash_before);
+    replay_step_state_access a(context, data.get(), data_length, root_hash_before);
     uint64_t mcycle_end{};
     if (__builtin_add_overflow(a.read_mcycle(), mcycle_count, &mcycle_end)) {
         mcycle_end = UINT64_MAX;
     }
     auto break_reason = interpret(a, mcycle_end);
     a.finish(root_hash_after);
-    os_unmap_file(data, data_length);
     return break_reason;
 }
 

src/merkle-tree-hash.cpp

@@ -231,7 +231,7 @@ int main(int argc, char *argv[]) {
     // Read from stdin if no input name was given
     auto input_file = unique_file_ptr{stdin};
     if (input_name != nullptr) {
-        input_file = unique_fopen(input_name, "ro", std::nothrow_t{});
+        input_file = make_unique_fopen(input_name, "ro", std::nothrow_t{});
         if (!input_file) {
             error("unable to open input file '%s'\n", input_name);
             return 1;
@@ -240,7 +240,7 @@ int main(int argc, char *argv[]) {
 
     // Allocate buffer for leaf data
     const uint64_t leaf_size = UINT64_C(1) << log2_leaf_size;
-    auto leaf_buf = unique_calloc<unsigned char>(leaf_size, std::nothrow_t{});
+    auto leaf_buf = make_unique_calloc<unsigned char>(leaf_size, std::nothrow_t{});
     if (!leaf_buf) {
         error("unable to allocate leaf buffer\n");
         return 1;

src/os.cpp

@@ -557,7 +557,7 @@ int os_mkdir(const char *path, [[maybe_unused]] int mode) {
 #endif // HAVE_MKDIR
 }
 
-unsigned char *os_map_file(const char *path, uint64_t length, bool shared) {
+void *os_map_file(const char *path, uint64_t length, bool shared) {
     if ((path == nullptr) || *path == '\0') {
         throw std::runtime_error{"image file path must be specified"s};
     }
@@ -589,8 +589,7 @@ unsigned char *os_map_file(const char *path, uint64_t length, bool shared) {
 
     // Try to map image file to host memory
     const int mflag = shared ? MAP_SHARED : MAP_PRIVATE;
-    auto *host_memory =
-        static_cast<unsigned char *>(mmap(nullptr, length, PROT_READ | PROT_WRITE, mflag, backing_file, 0));
+    void *host_memory = mmap(nullptr, length, PROT_READ | PROT_WRITE, mflag, backing_file, 0);
     if (host_memory == MAP_FAILED) { // NOLINT(cppcoreguidelines-pro-type-cstyle-cast,performance-no-int-to-ptr)
         close(backing_file);
         throw std::system_error{errno, std::generic_category(), "could not map image file '"s + path + "' to memory"s};
@@ -636,7 +635,7 @@ unsigned char *os_map_file(const char *path, uint64_t length, bool shared) {
     }
 
     DWORD dwDesiredAccess = shared ? FILE_MAP_WRITE : FILE_MAP_COPY;
-    auto *host_memory = static_cast<unsigned char *>(MapViewOfFile(hFileMappingObject, dwDesiredAccess, 0, 0, length));
+    void *host_memory = MapViewOfFile(hFileMappingObject, dwDesiredAccess, 0, 0, length);
     if (!host_memory) {
         _close(backing_file);
         throw std::system_error{errno, std::generic_category(), "could not map image file '"s + path + "' to memory"s};
@@ -687,7 +686,7 @@ unsigned char *os_map_file(const char *path, uint64_t length, bool shared) {
 #endif // HAVE_MMAP
 }
 
-void os_unmap_file(unsigned char *host_memory, [[maybe_unused]] uint64_t length) {
+void os_unmap_file(void *host_memory, [[maybe_unused]] uint64_t length) {
 #ifdef HAVE_MMAP
     munmap(host_memory, length);
 
@@ -981,7 +980,7 @@ int os_double_fork([[maybe_unused]] bool emancipate, [[maybe_unused]] const char
 }
 
 int64_t os_get_file_length(const char *filename, const char *text) {
-    auto fp = unique_fopen(filename, "rb");
+    auto fp = make_unique_fopen(filename, "rb");
     if (fseek(fp.get(), 0, SEEK_END) != 0) {
         throw std::system_error{errno, std::generic_category(),
             "unable to obtain length of file '"s + filename + "' "s + text};

src/os.h

@@ -94,10 +94,10 @@ void os_silence_putchar(bool yes);
 int os_mkdir(const char *path, int mode);
 
 /// \brief Maps a file to memory
-unsigned char *os_map_file(const char *path, uint64_t length, bool shared);
+void *os_map_file(const char *path, uint64_t length, bool shared);
 
 /// \brief Unmaps a file from memory
-void os_unmap_file(unsigned char *host_memory, uint64_t length);
+void os_unmap_file(void *host_memory, uint64_t length);
 
 /// \brief Get time elapsed since its first call with microsecond precision
 int64_t os_now_us();

src/pma-constants.h

@@ -69,7 +69,6 @@ enum PMA_ranges : uint64_t {
 enum PMA_constants : uint64_t {
     PMA_PAGE_SIZE_LOG2 = EXPAND_UINT64_C(PMA_PAGE_SIZE_LOG2_DEF), ///< log<sub>2</sub> of physical memory page size.
     PMA_PAGE_SIZE = (UINT64_C(1) << PMA_PAGE_SIZE_LOG2_DEF),      ///< Physical memory page size.
-    PMA_WORD_SIZE = EXPAND_UINT64_C(PMA_WORD_SIZE_DEF),           ///< Physical memory word size.
     PMA_MAX = EXPAND_UINT64_C(PMA_MAX_DEF)                        ///< Maximum number of PMAs
 };
 

src/pma-defines.h

@@ -48,7 +48,6 @@
 #define PMA_RAM_START_DEF 0x80000000 ///< RAM start address
 
 #define PMA_PAGE_SIZE_LOG2_DEF 12 ///< log<sub>2</sub> of physical memory page size.
-#define PMA_WORD_SIZE_DEF 8       ///< Physical memory word size.
 #define PMA_MAX_DEF 32            ///< Maximum number of PMAs
 #define PMA_PLIC_MAX_IRQ_DEF 31   ///< Maximum PLIC interrupt
 

src/pma.cpp

@@ -37,48 +37,18 @@ namespace cartesi {
 
 using namespace std::string_literals;
 
-void pma_memory::release() {
-    if (m_mmapped) {
-        os_unmap_file(m_host_memory, m_length);
-        m_mmapped = false;
-    } else {
-        std::free(m_host_memory); // NOLINT(cppcoreguidelines-no-malloc,hicpp-no-malloc)
-    }
-    m_host_memory = nullptr;
-    m_length = 0;
-}
-
-pma_memory::~pma_memory() {
-    release();
-}
-
-pma_memory::pma_memory(pma_memory &&other) noexcept :
-    m_length{other.m_length},
-    m_host_memory{other.m_host_memory},
-    m_mmapped{other.m_mmapped} {
-    // set other to safe state
-    other.m_host_memory = nullptr;
-    other.m_mmapped = false;
-    other.m_length = 0;
-}
-
-pma_memory::pma_memory(const std::string &description, uint64_t length, const callocd & /*c*/) :
-    m_length{length},
-    m_host_memory{nullptr},
-    m_mmapped{false} {
-    // use calloc to improve performance
-    // NOLINTNEXTLINE(cppcoreguidelines-no-malloc,hicpp-no-malloc,cppcoreguidelines-prefer-member-initializer)
-    m_host_memory = static_cast<unsigned char *>(std::calloc(1, length));
-    if (m_host_memory == nullptr) {
-        throw std::runtime_error{"error allocating memory for "s + description};
-    }
+pma_memory::pma_memory(const std::string &description, uint64_t length, const callocd & /*c*/) try :
+    m_ptr{make_unique_calloc<unsigned char>(length)},
+    m_host_memory{std::get<callocd_ptr>(m_ptr).get()} {
+} catch (std::exception &e) {
+    throw std::runtime_error{e.what() + " when initializing "s + description};
 }
 
 pma_memory::pma_memory(const std::string &description, uint64_t length, const std::string &path, const callocd &c) :
     pma_memory{description, length, c} {
     // Try to load image file, if any
     if (!path.empty()) {
-        auto fp = unique_fopen(path.c_str(), "rb", std::nothrow_t{});
+        auto fp = make_unique_fopen(path.c_str(), "rb", std::nothrow_t{});
         if (!fp) {
             throw std::system_error{errno, std::generic_category(),
                 "error opening image file '"s + path + "' when initializing "s + description};
@@ -88,47 +58,28 @@ pma_memory::pma_memory(const std::string &description, uint64_t length, const st
             throw std::system_error{errno, std::generic_category(),
                 "error obtaining length of image file '"s + path + "' when initializing "s + description};
         }
-        auto file_length = ftell(fp.get());
+        const auto file_length = static_cast<uint64_t>(ftello(fp.get()));
         if (fseek(fp.get(), 0, SEEK_SET) != 0) {
             throw std::system_error{errno, std::generic_category(),
                 "error obtaining length of image file '"s + path + "' when initializing "s + description};
         }
         // Check against PMA range size
-        if (static_cast<uint64_t>(file_length) > length) {
+        if (file_length > length) {
             throw std::runtime_error{"image file '"s + path + "' of "s + description + " is too large for range"s};
         }
         // Read to host memory
-        std::ignore = fread(m_host_memory, 1, length, fp.get());
-        if (ferror(fp.get()) != 0) {
-            throw std::system_error{errno, std::generic_category(),
-                "error reading from image file '"s + path + "' when initializing "s + description};
+        const auto read_length = static_cast<uint64_t>(fread(m_host_memory, 1, length, fp.get()));
+        if (read_length != file_length) {
+            throw std::runtime_error{"error reading from image file '"s + path + "' when initializing "s + description};
         }
     }
 }
 
-pma_memory::pma_memory(const std::string &description, uint64_t length, const std::string &path, const mmapd &m) :
-    m_length{length},
-    m_host_memory{nullptr},
-    m_mmapped{false} {
-    try {
-        m_host_memory = os_map_file(path.c_str(), length, m.shared);
-        m_mmapped = true;
-    } catch (std::exception &e) {
-        throw std::runtime_error{e.what() + " when initializing "s + description};
-    }
-}
-
-pma_memory &pma_memory::operator=(pma_memory &&other) noexcept {
-    release();
-    // copy from other
-    m_host_memory = other.m_host_memory;
-    m_mmapped = other.m_mmapped;
-    m_length = other.m_length;
-    // set other to safe state
-    other.m_host_memory = nullptr;
-    other.m_mmapped = false;
-    other.m_length = 0;
-    return *this;
+pma_memory::pma_memory(const std::string &description, uint64_t length, const std::string &path, const mmapd &m) try :
+    m_ptr{make_unique_mmap<unsigned char>(path.c_str(), length, m.shared)},
+    m_host_memory{std::get<mmapd_ptr>(m_ptr).get()} {
+} catch (std::exception &e) {
+    throw std::runtime_error{e.what() + " when initializing "s + description};
 }
 
 uint64_t pma_entry::get_istart() const {