azalea/kotlin-fork
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

[K/N] Avoid unitialized "fake" values

Browse Source 
Do not use `char[sizeof(T)]` as an internal utility replacement for `T` (e.g. in intrusive_list::head_).
    It's UB to access members of such values before a propper constructor of T is called.

    Merge-request: KT-MR-14076
This commit is contained in:
Aleksei.Glushko
2024-01-15 18:28:05 +01:00
committed by Space Team
parent 1f39bc9a18
commit 5396a6f8da
4 changed files with 50 additions and 57 deletions
Download Patch File Download Diff File Expand all files Collapse all files
kotlin-native/runtime/src/main/cpp/IntrusiveList.hpp
+31 -37
View File
@@ -22,6 +22,8 @@ struct DefaultIntrusiveForwardListTraits {
static void setNext(T& value, T* next) noexcept { value.setNext(next); }
static bool trySetNext(T& value, T* next) noexcept { return value.trySetNext(next); }
static T createFakeNode() noexcept { return T(); }
};
// Intrusive variant of `std::forward_list`.
@@ -140,8 +142,8 @@ public:
// Complexity: O(1)
intrusive_forward_list(intrusive_forward_list&& rhs) noexcept {
// Since tail() is shared, there's no need to update the last node's next_.
setNext(head(), next(rhs.head()));
// Since tail_ is shared, there's no need to update the last node's next_.
setNext(&head_, next(&rhs.head_));
rhs.clear();
}
@@ -165,13 +167,13 @@ public:
// Complexity: O(1)
void swap(intrusive_forward_list& rhs) noexcept {
// Since tail() is shared, there's no need to swap the last nodes' next_.
// Since tail_ is shared, there's no need to swap the last nodes' next_.
using std::swap;
auto thisNext = next(head());
auto rhsNext = next(rhs.head());
auto thisNext = next(&head_);
auto rhsNext = next(&rhs.head_);
swap(thisNext, rhsNext);
setNext(head(), thisNext);
setNext(rhs.head(), rhsNext);
setNext(&head_, thisNext);
setNext(&rhs.head_, rhsNext);
}
// Rewrite the contents of `this` with nodes from range `[first, last)`.
@@ -184,42 +186,42 @@ public:
}
// Complexity: O(1)
reference front() noexcept { return *next(head()); }
reference front() noexcept { return *next(&head_); }
// Complexity: O(1)
const_reference front() const noexcept { return *next(head()); }
const_reference front() const noexcept { return *next(&head_); }
// Iterator before the first node. Cannot be dereferenced.
// Complexity: O(1)
iterator before_begin() noexcept { return iterator(head()); }
iterator before_begin() noexcept { return iterator(&head_); }
// Iterator before the first node. Cannot be dereferenced.
// Complexity: O(1)
const_iterator before_begin() const noexcept { return const_iterator(head()); }
const_iterator before_begin() const noexcept { return const_iterator(&head_); }
// Iterator before the first node. Cannot be dereferenced.
// Complexity: O(1)
const_iterator cbefore_begin() const noexcept { return const_iterator(head()); }
const_iterator cbefore_begin() const noexcept { return const_iterator(&head_); }
// Complexity: O(1)
iterator begin() noexcept { return iterator(next(head())); }
iterator begin() noexcept { return iterator(next(&head_)); }
// Complexity: O(1)
const_iterator begin() const noexcept { return const_iterator(next(head())); }
const_iterator begin() const noexcept { return const_iterator(next(&head_)); }
// Complexity: O(1)
const_iterator cbegin() const noexcept { return const_iterator(next(head())); }
const_iterator cbegin() const noexcept { return const_iterator(next(&head_)); }
// Complexity: O(1)
iterator end() noexcept { return iterator(tail()); }
iterator end() noexcept { return iterator(&tail_); }
// Complexity: O(1)
const_iterator end() const noexcept { return const_iterator(tail()); }
const_iterator end() const noexcept { return const_iterator(&tail_); }
// Complexity: O(1)
const_iterator cend() const noexcept { return const_iterator(tail()); }
const_iterator cend() const noexcept { return const_iterator(&tail_); }
// Complexity: O(1)
bool empty() const noexcept { return next(head()) == tail(); }
bool empty() const noexcept { return next(&head_) == &tail_; }
// Complexity: O(1)
size_type max_size() const noexcept { return std::numeric_limits<size_type>::max(); }
// Complexity: O(1)
void clear() noexcept { setNext(head(), tail()); }
void clear() noexcept { setNext(&head_, &tail_); }
// Insert `value` after `pos`. `pos` can be in range `[before_begin(), end())`.
// Returns iterator to the newly inserted element
@@ -258,7 +260,7 @@ public:
iterator erase_after(iterator pos) noexcept {
RuntimeAssert(pos != end(), "Attempted to erase_after end()");
RuntimeAssert(pos != iterator(), "Attempted to erase_after empty iterator");
RuntimeAssert(next(pos.node_) != tail(), "Attempted to erase_after the last node");
RuntimeAssert(next(pos.node_) != &tail_, "Attempted to erase_after the last node");
pointer nextNode = next(next(pos.node_));
setNext(pos.node_, nextNode);
return iterator(nextNode);
@@ -273,7 +275,7 @@ public:
iterator erase_after(iterator first, iterator last) noexcept {
RuntimeAssert(first != end(), "Attempted to erase_after starting at end()");
RuntimeAssert(first != iterator(), "Attempted to erase_after starting at empty iterator");
RuntimeAssert(next(first.node_) != tail(), "Attempted to erase_after starting at the last node");
RuntimeAssert(next(first.node_) != &tail_, "Attempted to erase_after starting at the last node");
RuntimeAssert(last != iterator(), "Attempted to erase_after ending at empty iterator");
setNext(first.node_, last.node_);
return last;
@@ -302,11 +304,11 @@ public:
// This does not destroy the erased node and does not change its next pointer.
// Complexity: O(1)
pointer try_pop_front() noexcept {
pointer top = next(head());
if (top == tail()) {
pointer top = next(&head_);
if (top == &tail_) {
return nullptr;
}
setNext(head(), next(top));
setNext(&head_, next(top));
return top;
}
@@ -324,9 +326,9 @@ public:
// Complexity: O(n)
template <typename P>
void remove_if(P p) noexcept(noexcept(p(std::declval<const_reference>()))) {
pointer prev = head();
pointer prev = &head_;
pointer node = next(prev);
while (node != tail()) {
while (node != &tail_) {
if (p(*node)) {
// The node is being removed.
node = next(node);
@@ -367,11 +369,6 @@ private:
static bool trySetNext(pointer node, pointer next) noexcept { return Traits::trySetNext(*node, next); }
pointer head() noexcept { return &head_; }
const_pointer head() const noexcept { return &head_; }
static pointer tail() noexcept { return reinterpret_cast<pointer>(tailStorage_); }
// TODO: Consider making public.
std::optional<iterator> try_insert_after(iterator pos, reference value) noexcept {
RuntimeAssert(pos != end(), "Attempted to try_insert_after end()");
@@ -383,11 +380,8 @@ private:
return iterator(&value);
}
union {
value_type head_; // for debugger
alignas(value_type) char headStorage_[sizeof(value_type)] = {0};
};
alignas(value_type) static inline char tailStorage_[sizeof(value_type)] = {0};
value_type head_ = Traits::createFakeNode();
static inline value_type tail_ = Traits::createFakeNode();
};
template <typename InputIt>
kotlin-native/runtime/src/main/cpp/IntrusiveListTest.cpp
+1
View File
@@ -49,6 +49,7 @@ public:
private:
friend struct DefaultIntrusiveForwardListTraits<Node>;
Node() : Node(37) {}
Node* next() const noexcept { return next_; }
void setNext(Node* next) noexcept {
kotlin-native/runtime/src/mm/cpp/SpecialRefRegistry.cpp
+8 -8
View File
@@ -45,13 +45,13 @@ mm::SpecialRefRegistry& mm::SpecialRefRegistry::instance() noexcept {
mm::SpecialRefRegistry::Node* mm::SpecialRefRegistry::nextRoot(Node* current) noexcept {
RuntimeAssert(current != nullptr, "current cannot be null");
RuntimeAssert(current != rootsTail(), "current cannot be tail");
RuntimeAssert(current != &rootsTail_, "current cannot be tail");
Node* candidate = current->nextRoot_.load(std::memory_order_relaxed);
// Not an infinite loop, `candidate` always moves forward and since insertions can only
// happen in the head, they will always happen before `candidate`.
while (true) {
RuntimeAssert(candidate != nullptr, "candidate cannot be null");
if (candidate == rootsTail())
if (candidate == &rootsTail_)
// Reached tail, nothing to do anymore
return candidate;
if (candidate->rc_.load(std::memory_order_relaxed) > 0) {
@@ -92,8 +92,8 @@ mm::SpecialRefRegistry::Node* mm::SpecialRefRegistry::nextRoot(Node* current) no
std::pair<mm::SpecialRefRegistry::Node*, mm::SpecialRefRegistry::Node*> mm::SpecialRefRegistry::eraseFromRoots(
Node* prev, Node* node) noexcept {
RuntimeAssert(node != rootsHead(), "node cannot be head");
RuntimeAssert(node != rootsTail(), "node cannot be tail");
RuntimeAssert(node != &rootsHead_, "node cannot be head");
RuntimeAssert(node != &rootsTail_, "node cannot be tail");
Node* next = node->nextRoot_.load(std::memory_order_acquire);
RuntimeAssert(next != nullptr, "node@%p next cannot be null", node);
do {
@@ -106,15 +106,15 @@ std::pair<mm::SpecialRefRegistry::Node*, mm::SpecialRefRegistry::Node*> mm::Spec
return {prev, next};
}
prev = prevExpectedNext;
RuntimeAssert(prev != rootsHead(), "prev cannot be head");
RuntimeAssert(prev != rootsTail(), "prev cannot be tail");
RuntimeAssert(prev != &rootsHead_, "prev cannot be head");
RuntimeAssert(prev != &rootsTail_, "prev cannot be tail");
// We moved `prev` forward, nothing can insert after `prev` anymore, this
// cannot be an infinite loop, then.
} while (true);
}
void mm::SpecialRefRegistry::insertIntoRootsHead(Node& node) noexcept {
Node* next = rootsHead()->nextRoot_.load(std::memory_order_acquire);
Node* next = rootsHead_.nextRoot_.load(std::memory_order_acquire);
Node* nodeExpectedNext = nullptr;
do {
RuntimeAssert(next != nullptr, "head's next cannot be null");
@@ -129,7 +129,7 @@ void mm::SpecialRefRegistry::insertIntoRootsHead(Node& node) noexcept {
}
// CAS was successfull, so we need to update the expected value of node.nextRoot_
nodeExpectedNext = next;
} while (!rootsHead()->nextRoot_.compare_exchange_weak(next, &node, std::memory_order_release, std::memory_order_acquire));
} while (!rootsHead_.nextRoot_.compare_exchange_weak(next, &node, std::memory_order_release, std::memory_order_acquire));
}
std::list<mm::SpecialRefRegistry::Node>::iterator mm::SpecialRefRegistry::findAliveNode(
kotlin-native/runtime/src/mm/cpp/SpecialRefRegistry.hpp
+10 -12
View File
@@ -66,6 +66,8 @@ class SpecialRefRegistry : private Pinned {
RuntimeAssert(rc >= 0, "Creating StableRef with negative rc %d", rc);
}
Node() noexcept : obj_(nullptr), rc_(disposedMarker) {}
~Node() {
if (compiler::runtimeAssertsEnabled()) {
auto rc = rc_.load(std::memory_order_relaxed);
@@ -165,10 +167,10 @@ class SpecialRefRegistry : private Pinned {
// Synchronization between GC and mutators happens via enabling/disabling
// the barriers.
// TODO: Try to handle it atomically only when the GC is in progress.
std::atomic<ObjHeader*> obj_ = nullptr;
std::atomic<ObjHeader*> obj_;
// Only ever updated using relaxed memory ordering. Any synchronization
// with nextRoot_ is achieved via acquire-release of nextRoot_.
std::atomic<Rc> rc_ = 0; // After dispose() will be disposedMarker.
std::atomic<Rc> rc_; // After dispose() will be disposedMarker.
// Singly linked lock free list. Using acquire-release throughout.
std::atomic<Node*> nextRoot_ = nullptr;
// This and the next one only serve fast deletion optimization for shortly lived StableRefs.
@@ -278,9 +280,9 @@ public:
class RootsIterable : private MoveOnly {
public:
RootsIterator begin() const noexcept { return RootsIterator(*owner_, owner_->nextRoot(owner_->rootsHead())); }
RootsIterator begin() const noexcept { return RootsIterator(*owner_, owner_->nextRoot(&owner_->rootsHead_)); }
RootsIterator end() const noexcept { return RootsIterator(*owner_, owner_->rootsTail()); }
RootsIterator end() const noexcept { return RootsIterator(*owner_, &owner_->rootsTail_); }
private:
friend class SpecialRefRegistry;
@@ -327,14 +329,14 @@ public:
std::unique_lock<Mutex> guard_;
};
SpecialRefRegistry() noexcept { rootsHead()->nextRoot_.store(rootsTail(), std::memory_order_relaxed); }
SpecialRefRegistry() noexcept { rootsHead_.nextRoot_.store(&rootsTail_, std::memory_order_relaxed); }
~SpecialRefRegistry() = default;
static SpecialRefRegistry& instance() noexcept;
void clearForTests() noexcept {
rootsHead()->nextRoot_ = rootsTail();
rootsHead_.nextRoot_ = &rootsTail_;
for (auto& node : all_) {
// Allow the tests not to run the finalizers for weaks.
node.rc_ = Node::disposedMarker;
@@ -362,17 +364,13 @@ private:
void insertIntoRootsHead(Node& node) noexcept;
std::list<Node>::iterator findAliveNode(std::list<Node>::iterator it) noexcept;
Node* rootsHead() noexcept { return reinterpret_cast<Node*>(rootsHeadStorage_); }
const Node* rootsHead() const noexcept { return reinterpret_cast<const Node*>(rootsHeadStorage_); }
static Node* rootsTail() noexcept { return reinterpret_cast<Node*>(rootsTailStorage_); }
// TODO: Iteration over `all_` will be slow, because it's `std::list`
// collected at different times from different threads, and so the nodes
// are all over the memory. Consider using custom allocator for that.
std::list<Node> all_;
Mutex mutex_;
alignas(Node) char rootsHeadStorage_[sizeof(Node)] = {0};
alignas(Node) static inline char rootsTailStorage_[sizeof(Node)] = {0};
Node rootsHead_{};
static inline Node rootsTail_{};
};
} // namespace kotlin::mm