SILO/STO making safe/unsafe by sharshith1312 · Pull Request #52 · makodb/mako

sharshith1312 · 2025-12-17T05:44:28Z

No description provided.

Copilot

Pull request overview

This pull request adds safety annotations (@safe/@unsafe) to functions across the SILO/STO transactional database system codebase, with the goal of documenting which operations are memory-safe and which involve potentially unsafe operations like raw pointer manipulation, atomic operations, or external C function calls.

Key Changes

Added @safe and @unsafe annotations to hundreds of functions across transaction handling, storage, and synchronization code
Changed sync_util::sync_logger::local_replica_id from int to std::atomic<int> and updated all atomic operations to use modern C++ atomic methods
Migrated from std::map and std::unordered_map to rusty::HashMap in ThreadPool.h and common.h with Option-based API
Wrapped external C functions (usleep, add_log_to_nc) in safe wrapper functions

Reviewed changes

Copilot reviewed 25 out of 25 changed files in this pull request and generated 14 comments.

Show a summary per file

File	Description
src/mako/txn_proto2_impl.h	Added @safe annotations to simple getters and pure bitwise operations; @unsafe annotations to functions calling wait/persist operations
src/mako/txn.h	Added @safe annotations to getters and simple operations; @unsafe annotations to functions with pointer operations and marked_ptr method calls
src/mako/tuple.h	Added @safe annotations to bitwise checks, simple getters, and member reads
src/mako/thread.h	Added @safe annotation with lifetime annotation for get_name()
src/mako/masstree/nodeversion.hh	Changed @safe to @unsafe for bit manipulation methods (mark_split, mark_deleted_tree, mark_root, mark_nonroot)
src/mako/masstree/masstree_struct.hh	Changed @safe to @unsafe for suffix string operations (ksuf, ksuf_equals, ksuf_matches, ksuf_compare)
src/mako/masstree/kpermuter.hh	Changed @safe to @unsafe for bit manipulation methods (exchange, exchange_values)
src/mako/lib/shardClient.h	Added @safe annotations to all remote operation methods
src/mako/counter.h	Added @safe annotations to simple initialization and arithmetic operations with lifetime annotations
src/mako/benchmarks/tpcc_keys.h	Added @safe annotations to POD structs, constructors, comparison operators, and hash functions
src/mako/benchmarks/sto/sync_util.hh	Changed local_replica_id to std::atomic; added @unsafe annotations; added unordered_map include; formatting fix
src/mako/benchmarks/sto/stuffed_str.hh	Added @safe annotations to data() and length() methods
src/mako/benchmarks/sto/simple_str.hh	Added @safe and @unsafe annotations with detailed descriptions
src/mako/benchmarks/sto/randgen.hh	Added @safe annotations to pure arithmetic operations
src/mako/benchmarks/sto/multiversion.hh	Added @unsafe annotation to mvGET function
src/mako/benchmarks/sto/Transaction.hh	Added @unsafe annotations with explanations; formatting fix; added unordered_map include
src/mako/benchmarks/sto/Transaction.cc	Added external C function declarations; created safe wrapper functions; replaced __sync_fetch_and_add with fetch_add; changed instance-> to (*instance); updated nullptr comparisons; added @safe/@unsafe annotations throughout
src/mako/benchmarks/sto/ThreadPool.h	Added rusty/hashmap.hpp include; migrated to rusty::HashMap with Option-based get() API; added @safe/@unsafe annotations
src/mako/benchmarks/sto/ThreadPool.cc	Changed @unsafe to @safe annotation for cmpFunc2_v2
src/mako/benchmarks/sto/ReplayDB.h	Added @safe annotation to CommitInfo struct; @unsafe annotations to replay functions
src/mako/benchmarks/sto/MassTrans.cc	Added @external annotation documenting Transaction as unsafe_type
src/mako/benchmarks/sto/Interface.hh	Added getLocalPartitionID() method; added @safe annotations to virtual methods (with conflicting annotation on print())
src/mako/benchmarks/cpu_throttler.h	Added @safe annotations to getter methods
src/mako/benchmarks/common.h	Added rusty/hashmap.hpp include; migrated HashWrapper to use rusty::HashMap with Option-based API
src/mako/benchmarks/benchmark_config.h	Added @safe annotations to all simple getter methods with lifetime annotations where appropriate

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2025-12-17T05:52:48Z

src/mako/benchmarks/sto/sync_util.hh

        }

        // In previous submission, we assume the healthy shards are always INF
+        // @unsafe - uses std::unordered_map (undeclared)


The annotation comment incorrectly states "undeclared" but std::unordered_map is declared via the include at line 9. The correct safety annotation should focus on the actual unsafe operations (accessing std::unordered_map which can reallocate or throw exceptions).

Copilot · 2025-12-17T05:52:49Z

src/mako/benchmarks/sto/sync_util.hh

        }

        // Single timestamp system: ensure vector contains replicated value
+        // @unsafe - uses std::vector which is undeclared, and std::cout


The annotation incorrectly claims std::vector is "undeclared", but vector is brought into scope via "using namespace std;" at line 11. The annotation should focus on actual unsafe operations rather than incorrect claims about declarations.

Suggested change

// @unsafe - uses std::vector which is undeclared, and std::cout

// @unsafe - copies vector by value and performs logging via std::cout

Copilot · 2025-12-17T05:52:49Z

src/mako/benchmarks/common.h

 #include "benchmarks/benchmark_config.h"
+#include <rusty/hashmap.hpp>
 //#include "lib/memcached_client.h"



The header file "rusty/hashmap.hpp" is included here but does not appear to exist in the repository. This will cause a compilation failure.

Suggested change

#include "benchmarks/benchmark_config.h"

#include <rusty/hashmap.hpp>

//#include "lib/memcached_client.h"

#include <utility>

#include "benchmarks/benchmark_config.h"

//#include "lib/memcached_client.h"

// Minimal local implementation of rusty::HashMap and its Option-like return type

namespace rusty {

template <typename T>

class Option {

public:

Option() : value_(nullptr) {}

explicit Option(T value) : value_(value) {}

bool is_some() const {

return value_ != nullptr;

}

T unwrap() const {

return value_;

}

private:

T value_;

};

template <typename K, typename V>

class HashMap {

public:

HashMap() = default;

template <typename KeyLike>

void insert(KeyLike&& key, V value) {

data_.emplace(std::forward<KeyLike>(key), value);

}

Option<V*> get(const K& key) {

auto it = data_.find(key);

if (it == data_.end()) {

return Option<V*>(nullptr);

}

return Option<V*>(&it->second);

}

private:

std::unordered_map<K, V> data_;

};

}

Copilot · 2025-12-17T05:52:49Z

src/mako/benchmarks/sto/Interface.hh

    virtual void cleanup(TransItem& item, bool committed) {
        (void) item, (void) committed;
    }
+    // @safe


There are two conflicting safety annotations on the same function. Line 777 marks it as "@safe" while line 778 marks it as "@unsafe". Remove one of these annotations to avoid confusion.

Suggested change

// @safe

Copilot · 2025-12-17T05:52:50Z

src/mako/txn.h

    {
      return tuple.get();
    }
+    // @unsafe - calls marked_ptr::get (undeclared)


The annotation incorrectly claims marked_ptr::get is "undeclared". Since this is a method call on the 'tuple' member, it's likely declared in the marked_ptr class definition. The annotation should describe the actual unsafe operation rather than making incorrect claims about declarations.

Copilot · 2025-12-17T05:52:51Z

src/mako/txn.h

+    // @unsafe - calls marked_ptr::get_flags (undeclared)
    inline bool
    is_insert() const
    {
      return btr.get_flags() & FLAGS_INSERT;
    }
+    // @unsafe - calls marked_ptr::get_flags (undeclared)


The annotation incorrectly claims marked_ptr::get_flags is "undeclared". Since this is a method call on the 'btr' member, it's likely declared in the marked_ptr class definition. The annotation should describe the actual unsafe operation rather than making incorrect claims about declarations.

Suggested change

// @unsafe - calls marked_ptr::get_flags (undeclared)

inline bool

is_insert() const

{

return btr.get_flags() & FLAGS_INSERT;

}

// @unsafe - calls marked_ptr::get_flags (undeclared)

// @unsafe - inspects internal marked_ptr flag bits

inline bool

is_insert() const

{

return btr.get_flags() & FLAGS_INSERT;

}

// @unsafe - inspects internal marked_ptr flag bits

Copilot · 2025-12-17T05:52:51Z

src/mako/txn.h

+    // @unsafe - calls marked_ptr::get_flags (undeclared)
    inline bool
    is_insert() const
    {
      return btr.get_flags() & FLAGS_INSERT;
    }
+    // @unsafe - calls marked_ptr::get_flags (undeclared)


The annotation incorrectly claims marked_ptr::get_flags is "undeclared". Since this is a method call on the 'btr' member, it's likely declared in the marked_ptr class definition. The annotation should describe the actual unsafe operation rather than making incorrect claims about declarations.

Suggested change

// @unsafe - calls marked_ptr::get_flags (undeclared)

inline bool

is_insert() const

{

return btr.get_flags() & FLAGS_INSERT;

}

// @unsafe - calls marked_ptr::get_flags (undeclared)

// @unsafe - reads flag bits from marked_ptr (may be concurrently modified)

inline bool

is_insert() const

{

return btr.get_flags() & FLAGS_INSERT;

}

// @unsafe - reads flag bits from marked_ptr (may be concurrently modified)

Copilot · 2025-12-17T05:52:52Z

src/mako/txn.h

      INVARIANT(!do_write());
      btr.or_flags(FLAGS_DOWRITE);
    }
+    // @unsafe - calls marked_ptr::get (undeclared)


The annotation incorrectly claims marked_ptr::get is "undeclared". Since this is a method call on the 'btr' member, it's likely declared in the marked_ptr class definition. The annotation should describe the actual unsafe operation rather than making incorrect claims about declarations.

Copilot · 2025-12-17T05:52:52Z

src/mako/txn.h

+    // @unsafe - uses const_cast
    explicit dbtuple_write_info(const dbtuple *tuple)
      : tuple(const_cast<dbtuple *>(tuple)), entry(), pos() {}
+    // @unsafe - calls marked_ptr::get (undeclared)


The annotation incorrectly claims marked_ptr::get is "undeclared". Since this is a method call on the 'tuple' member, it's likely declared in the marked_ptr class definition. The annotation should describe the actual unsafe operation rather than making incorrect claims about declarations.

Copilot · 2025-12-17T05:52:52Z

src/mako/txn.h

      tuple.or_flags(FLAGS_LOCKED);
      INVARIANT(is_locked());
    }
+    // @unsafe - calls marked_ptr::get_flags (undeclared)


The annotation incorrectly claims marked_ptr::get_flags is "undeclared". Since this is a method call on the 'tuple' member, it's likely declared in the marked_ptr class definition. The annotation should describe the actual unsafe operation rather than making incorrect claims about declarations.

shuaimu · 2025-12-17T17:24:19Z

src/mako/benchmarks/sto/simple_str.hh

    return len > (int)capacity_;
  }

+  // @safe - returns raw pointer (allowed in @safe)


returning raw pointers. does it pass the checker?

added lifetime annotation and checker passed this

src/mako/benchmarks/sto/sync_util.hh

shuaimu · 2025-12-17T17:25:37Z

src/mako/benchmarks/sto/ThreadPool.h

    }

+    // @safe - uses rusty::HashMap::get which returns Option
    ThreadDBWrapperMbta* getDBWrapper(int par_id) {


safe functions cannot return raw pointers.

But checker is passing this

shuaimu · 2025-12-17T17:28:40Z

pls give the LOC of how many are safe and how many are unsafe.

sharshith1312 · 2025-12-18T07:38:52Z

pls give the LOC of how many are safe and how many are unsafe.

@safe 376 -------- 51.1%
@unsafe 360 ------ 48.81%

SILO/STO making safe/unsafe

270d74e

Copilot AI review requested due to automatic review settings December 17, 2025 05:44

Copilot started reviewing on behalf of sharshith1312 December 17, 2025 05:44 View session

Copilot AI reviewed Dec 17, 2025

View reviewed changes

shuaimu reviewed Dec 17, 2025

View reviewed changes

src/mako/benchmarks/sto/sync_util.hh Outdated Show resolved Hide resolved

shuaimu reviewed Dec 17, 2025

View reviewed changes

Silo/STO making unsafe to safe

9a63552

shenweihai1 force-pushed the mako-dev branch from 098b896 to 9649bf3 Compare December 18, 2025 17:39

sharshith1312 added 2 commits December 19, 2025 22:50

Merge remote-tracking branch 'upstream/mako-dev' into mako-dev

2257edb

resolve merge conflicts

6ac7a1b

	// @unsafe - uses std::vector which is undeclared, and std::cout
	// @unsafe - copies vector by value and performs logging via std::cout

-#include "benchmarks/benchmark_config.h"
-#include <rusty/hashmap.hpp>
-//#include "lib/memcached_client.h"
+#include <utility>
+#include "benchmarks/benchmark_config.h"
+//#include "lib/memcached_client.h"
+// Minimal local implementation of rusty::HashMap and its Option-like return type
+namespace rusty {
+    template <typename T>
+    class Option {
+    public:
+        Option() : value_(nullptr) {}
+        explicit Option(T value) : value_(value) {}
+        bool is_some() const {
+            return value_ != nullptr;
+        }
+        T unwrap() const {
+            return value_;
+        }
+    private:
+        T value_;
+    };
+    template <typename K, typename V>
+    class HashMap {
+    public:
+        HashMap() = default;
+        template <typename KeyLike>
+        void insert(KeyLike&& key, V value) {
+            data_.emplace(std::forward<KeyLike>(key), value);
+        }
+        Option<V*> get(const K& key) {
+            auto it = data_.find(key);
+            if (it == data_.end()) {
+                return Option<V*>(nullptr);
+            }
+            return Option<V*>(&it->second);
+        }
+    private:
+        std::unordered_map<K, V> data_;
+    };
+}

Comments

Conversation

sharshith1312 commented Dec 17, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Key Changes

Reviewed changes

Uh oh!

Copilot AI Dec 17, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 17, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 17, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 17, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 17, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 17, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 17, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 17, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 17, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 17, 2025

Choose a reason for hiding this comment

Uh oh!

shuaimu Dec 17, 2025

Choose a reason for hiding this comment

Uh oh!

sharshith1312 Dec 18, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

shuaimu Dec 17, 2025

Choose a reason for hiding this comment

Uh oh!

sharshith1312 Dec 18, 2025

Choose a reason for hiding this comment

Uh oh!

shuaimu commented Dec 17, 2025

Uh oh!

sharshith1312 commented Dec 18, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants