From 9c36d7c3384add44d32d9f3d8c8f7e07cc1e9b04 Mon Sep 17 00:00:00 2001 From: Bananymous Date: Mon, 4 Mar 2024 11:41:54 +0200 Subject: [PATCH] BAN/Kernel: Rework assertion/panic system BAN/Assert.h does not need any includes meaning it can be included anywhere without problems. --- BAN/BAN/Assert.cpp | 22 ++++++++++++++ BAN/CMakeLists.txt | 1 + BAN/include/BAN/Assert.h | 36 +++++------------------ BAN/include/BAN/CircularQueue.h | 1 + BAN/include/BAN/Debug.h | 8 +++-- BAN/include/BAN/PlacementNew.h | 2 ++ kernel/CMakeLists.txt | 1 + kernel/arch/x86_64/IDT.cpp | 7 +++++ kernel/arch/x86_64/PageTable.cpp | 6 ++-- kernel/include/kernel/Lock/Mutex.h | 24 +++++++-------- kernel/include/kernel/Panic.h | 17 ++++++----- kernel/kernel/FS/TmpFS/FileSystem.cpp | 10 +++---- kernel/kernel/Lock/SpinLock.cpp | 12 ++++---- kernel/kernel/Memory/FileBackedRegion.cpp | 2 +- kernel/kernel/Memory/VirtualRange.cpp | 6 ++-- kernel/kernel/Networking/E1000/E1000.cpp | 4 +-- kernel/kernel/Networking/TCPSocket.cpp | 2 +- kernel/kernel/Panic.cpp | 4 +-- kernel/kernel/Thread.cpp | 4 +-- libc/CMakeLists.txt | 2 ++ 20 files changed, 96 insertions(+), 75 deletions(-) create mode 100644 BAN/BAN/Assert.cpp diff --git a/BAN/BAN/Assert.cpp b/BAN/BAN/Assert.cpp new file mode 100644 index 00000000..6bc462ae --- /dev/null +++ b/BAN/BAN/Assert.cpp @@ -0,0 +1,22 @@ +#include + +#if __is_kernel + +#include + +[[noreturn]] void __ban_assertion_failed(const char* location, const char* msg) +{ + Kernel::panic_impl(location, msg); +} + +#else + +#include + +[[noreturn]] void __ban_assertion_failed(const char* location, const char* msg) +{ + derrorln("{}: {}", location, msg); + __builtin_trap(); +} + +#endif diff --git a/BAN/CMakeLists.txt b/BAN/CMakeLists.txt index 30aa004c..ee8dfd8b 100644 --- a/BAN/CMakeLists.txt +++ b/BAN/CMakeLists.txt @@ -3,6 +3,7 @@ cmake_minimum_required(VERSION 3.26) project(BAN CXX) set(BAN_SOURCES + BAN/Assert.cpp BAN/New.cpp BAN/String.cpp BAN/StringView.cpp diff --git a/BAN/include/BAN/Assert.h b/BAN/include/BAN/Assert.h index 53872aba..0cf82fe4 100644 --- a/BAN/include/BAN/Assert.h +++ b/BAN/include/BAN/Assert.h @@ -1,33 +1,13 @@ #pragma once -#include +#define __ban_assert_stringify_helper(s) #s +#define __ban_assert_stringify(s) __ban_assert_stringify_helper(s) -#if defined(__is_kernel) - #include +#define ASSERT(cond) \ + (__builtin_expect(!(cond), 0) \ + ? __ban_assertion_failed(__FILE__ ":" __ban_assert_stringify(__LINE__), "ASSERT(" #cond ") failed") \ + : (void)0) - #define ASSERT(cond) \ - do { \ - if (!(cond)) \ - Kernel::panic("ASSERT(" #cond ") failed"); \ - } while (false) +#define ASSERT_NOT_REACHED() ASSERT(false) - #define __ASSERT_BIN_OP(lhs, rhs, name, op) \ - do { \ - auto&& _lhs = (lhs); \ - auto&& _rhs = (rhs); \ - if (!(_lhs op _rhs)) \ - Kernel::panic(name "(" #lhs ", " #rhs ") ({} " #op " {}) failed", _lhs, _rhs); \ - } while (false) - - #define ASSERT_LT(lhs, rhs) __ASSERT_BIN_OP(lhs, rhs, "ASSERT_LT", <) - #define ASSERT_LTE(lhs, rhs) __ASSERT_BIN_OP(lhs, rhs, "ASSERT_LTE", <=) - #define ASSERT_GT(lhs, rhs) __ASSERT_BIN_OP(lhs, rhs, "ASSERT_GT", >) - #define ASSERT_GTE(lhs, rhs) __ASSERT_BIN_OP(lhs, rhs, "ASSERT_GTE", >=) - #define ASSERT_EQ(lhs, rhs) __ASSERT_BIN_OP(lhs, rhs, "ASSERT_EQ", ==) - #define ASSERT_NEQ(lhs, rhs) __ASSERT_BIN_OP(lhs, rhs, "ASSERT_NEQ", !=) - #define ASSERT_NOT_REACHED() Kernel::panic("ASSERT_NOT_REACHED() failed") -#else - #include - #define ASSERT(cond) assert((cond) && "ASSERT("#cond") failed") - #define ASSERT_NOT_REACHED() do { assert(false && "ASSERT_NOT_REACHED() failed"); __builtin_unreachable(); } while (false) -#endif +[[noreturn]] void __ban_assertion_failed(const char* location, const char* msg); diff --git a/BAN/include/BAN/CircularQueue.h b/BAN/include/BAN/CircularQueue.h index c40b24cf..4638b8a5 100644 --- a/BAN/include/BAN/CircularQueue.h +++ b/BAN/include/BAN/CircularQueue.h @@ -1,6 +1,7 @@ #pragma once #include +#include #include #include diff --git a/BAN/include/BAN/Debug.h b/BAN/include/BAN/Debug.h index 2dc87f33..0a26ca96 100644 --- a/BAN/include/BAN/Debug.h +++ b/BAN/include/BAN/Debug.h @@ -1,8 +1,10 @@ #pragma once #if __is_kernel -#error "This is userspace only file" -#endif + +#include + +#else #include #include @@ -29,3 +31,5 @@ dprintln(__VA_ARGS__); \ BAN::Formatter::print(__debug_putchar, "\e[m"); \ } while(false) + +#endif diff --git a/BAN/include/BAN/PlacementNew.h b/BAN/include/BAN/PlacementNew.h index edc861a7..b5fede7a 100644 --- a/BAN/include/BAN/PlacementNew.h +++ b/BAN/include/BAN/PlacementNew.h @@ -1,4 +1,6 @@ #pragma once +#include + inline void* operator new(size_t, void* addr) { return addr; } inline void* operator new[](size_t, void* addr) { return addr; } diff --git a/kernel/CMakeLists.txt b/kernel/CMakeLists.txt index 22604b41..4ee0f24d 100644 --- a/kernel/CMakeLists.txt +++ b/kernel/CMakeLists.txt @@ -136,6 +136,7 @@ set(LAI_SOURCES ) set(BAN_SOURCES + ../BAN/BAN/Assert.cpp ../BAN/BAN/New.cpp ../BAN/BAN/String.cpp ../BAN/BAN/StringView.cpp diff --git a/kernel/arch/x86_64/IDT.cpp b/kernel/arch/x86_64/IDT.cpp index 198cc178..39bedf0b 100644 --- a/kernel/arch/x86_64/IDT.cpp +++ b/kernel/arch/x86_64/IDT.cpp @@ -163,6 +163,13 @@ namespace Kernel::IDT extern "C" void cpp_isr_handler(uint64_t isr, uint64_t error, InterruptStack& interrupt_stack, const Registers* regs) { + if (g_paniced) + { + // FIXME: tell other processors kernel panic has occured + dprintln("Processor {} halted", Processor::current_id()); + asm volatile("cli; 1: hlt; jmp 1b"); + } + #if __enable_sse bool from_userspace = (interrupt_stack.cs & 0b11) == 0b11; if (from_userspace) diff --git a/kernel/arch/x86_64/PageTable.cpp b/kernel/arch/x86_64/PageTable.cpp index 9d690208..b468a80b 100644 --- a/kernel/arch/x86_64/PageTable.cpp +++ b/kernel/arch/x86_64/PageTable.cpp @@ -208,7 +208,7 @@ namespace Kernel void PageTable::map_fast_page(paddr_t paddr) { ASSERT(s_kernel); - ASSERT_NEQ(paddr, 0); + ASSERT(paddr); SpinLockGuard _(s_fast_page_lock); @@ -322,7 +322,7 @@ namespace Kernel ASSERT(vaddr); ASSERT(vaddr != fast_page()); if (vaddr >= KERNEL_OFFSET) - ASSERT_GTE(vaddr, (vaddr_t)g_kernel_start); + ASSERT(vaddr >= (vaddr_t)g_kernel_start); if ((vaddr >= KERNEL_OFFSET) != (this == s_kernel)) Kernel::panic("unmapping {8H}, kernel: {}", vaddr, this == s_kernel); @@ -368,7 +368,7 @@ namespace Kernel ASSERT(vaddr); ASSERT(vaddr != fast_page()); if (vaddr >= KERNEL_OFFSET && s_current) - ASSERT_GTE(vaddr, (vaddr_t)g_kernel_start); + ASSERT(vaddr >= (vaddr_t)g_kernel_start); if ((vaddr >= KERNEL_OFFSET) != (this == s_kernel)) Kernel::panic("mapping {8H} to {8H}, kernel: {}", paddr, vaddr, this == s_kernel); diff --git a/kernel/include/kernel/Lock/Mutex.h b/kernel/include/kernel/Lock/Mutex.h index 27914ae6..257c1050 100644 --- a/kernel/include/kernel/Lock/Mutex.h +++ b/kernel/include/kernel/Lock/Mutex.h @@ -21,12 +21,12 @@ namespace Kernel { auto tid = Scheduler::current_tid(); if (tid == m_locker) - ASSERT_GT(m_lock_depth, 0); + ASSERT(m_lock_depth > 0); else { while (!m_locker.compare_exchange(-1, tid)) Scheduler::get().reschedule(); - ASSERT_EQ(m_lock_depth, 0); + ASSERT(m_lock_depth == 0); } m_lock_depth++; } @@ -35,20 +35,20 @@ namespace Kernel { auto tid = Scheduler::current_tid(); if (tid == m_locker) - ASSERT_GT(m_lock_depth, 0); + ASSERT(m_lock_depth > 0); else { if (!m_locker.compare_exchange(-1, tid)) return false; - ASSERT_EQ(m_lock_depth, 0); + ASSERT(m_lock_depth == 0); } m_lock_depth++; } void unlock() { - ASSERT_EQ(m_locker.load(), Scheduler::current_tid()); - ASSERT_GT(m_lock_depth, 0); + ASSERT(m_locker == Scheduler::current_tid()); + ASSERT(m_lock_depth > 0); if (--m_lock_depth == 0) m_locker = -1; } @@ -74,7 +74,7 @@ namespace Kernel { auto tid = Scheduler::current_tid(); if (tid == m_locker) - ASSERT_GT(m_lock_depth, 0); + ASSERT(m_lock_depth > 0); else { bool has_priority = tid ? !Thread::current().is_userspace() : true; @@ -82,7 +82,7 @@ namespace Kernel m_queue_length++; while (!(has_priority || m_queue_length == 0) || !m_locker.compare_exchange(-1, tid)) Scheduler::get().reschedule(); - ASSERT_EQ(m_lock_depth, 0); + ASSERT(m_lock_depth == 0); } m_lock_depth++; } @@ -91,7 +91,7 @@ namespace Kernel { auto tid = Scheduler::current_tid(); if (tid == m_locker) - ASSERT_GT(m_lock_depth, 0); + ASSERT(m_lock_depth > 0); else { bool has_priority = tid ? !Thread::current().is_userspace() : true; @@ -99,7 +99,7 @@ namespace Kernel return false; if (has_priority) m_queue_length++; - ASSERT_EQ(m_lock_depth, 0); + ASSERT(m_lock_depth == 0); } m_lock_depth++; } @@ -107,8 +107,8 @@ namespace Kernel void unlock() { auto tid = Scheduler::current_tid(); - ASSERT_EQ(m_locker.load(), tid); - ASSERT_GT(m_lock_depth, 0); + ASSERT(m_locker == tid); + ASSERT(m_lock_depth > 0); if (--m_lock_depth == 0) { bool has_priority = tid ? !Thread::current().is_userspace() : true; diff --git a/kernel/include/kernel/Panic.h b/kernel/include/kernel/Panic.h index 753aa4fe..d734e90a 100644 --- a/kernel/include/kernel/Panic.h +++ b/kernel/include/kernel/Panic.h @@ -2,28 +2,29 @@ #include -#define panic(...) detail::panic_impl(__FILE__, __LINE__, __VA_ARGS__) +#define __panic_stringify_helper(s) #s +#define __panic_stringify(s) __panic_stringify_helper(s) -namespace Kernel::detail +#define panic(...) panic_impl(__FILE__ ":" __panic_stringify(__LINE__), __VA_ARGS__) + +namespace Kernel { - extern bool g_paniced; + extern volatile bool g_paniced; template __attribute__((__noreturn__)) - static void panic_impl(const char* file, int line, const char* message, Args&&... args) + static void panic_impl(const char* location, const char* message, Args&&... args) { asm volatile("cli"); - derrorln("Kernel panic at {}:{}", file, line); + derrorln("Kernel panic at {}", location); derrorln(message, BAN::forward(args)...); if (!g_paniced) { g_paniced = true; Debug::dump_stack_trace(); } - for (;;) - asm volatile("hlt"); - __builtin_unreachable(); + asm volatile("ud2"); } } diff --git a/kernel/kernel/FS/TmpFS/FileSystem.cpp b/kernel/kernel/FS/TmpFS/FileSystem.cpp index bc94ac50..17101010 100644 --- a/kernel/kernel/FS/TmpFS/FileSystem.cpp +++ b/kernel/kernel/FS/TmpFS/FileSystem.cpp @@ -128,9 +128,9 @@ namespace Kernel auto inode_location = find_inode(ino); PageTable::with_fast_page(inode_location.paddr, [&] { auto& inode_info = PageTable::fast_page_as_sized(inode_location.index); - ASSERT_EQ(inode_info.nlink, 0); + ASSERT(inode_info.nlink == 0); for (auto paddr : inode_info.block) - ASSERT_EQ(paddr, 0); + ASSERT(paddr == 0); inode_info = {}; }); ASSERT(!m_inode_cache.contains(ino)); @@ -166,8 +166,8 @@ namespace Kernel { LockGuard _(m_mutex); - ASSERT_GTE(ino, first_inode); - ASSERT_LT(ino, max_inodes); + ASSERT(ino >= first_inode); + ASSERT(ino < max_inodes); constexpr size_t inodes_per_page = PAGE_SIZE / sizeof(TmpInodeInfo); @@ -220,7 +220,7 @@ namespace Kernel { LockGuard _(m_mutex); - ASSERT_GT(index, 0); + ASSERT(index > 0); return find_indirect(m_data_pages, index - first_data_page, 3); } diff --git a/kernel/kernel/Lock/SpinLock.cpp b/kernel/kernel/Lock/SpinLock.cpp index d2a8b9dd..56a2c3dc 100644 --- a/kernel/kernel/Lock/SpinLock.cpp +++ b/kernel/kernel/Lock/SpinLock.cpp @@ -10,7 +10,7 @@ namespace Kernel InterruptState SpinLock::lock() { auto id = Processor::current_id(); - ASSERT_NEQ(m_locker.load(), id); + ASSERT(m_locker != id); auto state = Processor::get_interrupt_state(); Processor::set_interrupt_state(InterruptState::Disabled); @@ -23,7 +23,7 @@ namespace Kernel void SpinLock::unlock(InterruptState state) { - ASSERT_EQ(m_locker.load(), Processor::current_id()); + ASSERT(m_locker == Processor::current_id()); m_locker.store(PROCESSOR_NONE, BAN::MemoryOrder::memory_order_release); Processor::set_interrupt_state(state); } @@ -36,12 +36,12 @@ namespace Kernel Processor::set_interrupt_state(InterruptState::Disabled); if (id == m_locker) - ASSERT_GT(m_lock_depth, 0); + ASSERT(m_lock_depth > 0); else { while (!m_locker.compare_exchange(PROCESSOR_NONE, id, BAN::MemoryOrder::memory_order_acquire)) __builtin_ia32_pause(); - ASSERT_EQ(m_lock_depth, 0); + ASSERT(m_lock_depth == 0); } m_lock_depth++; @@ -51,8 +51,8 @@ namespace Kernel void RecursiveSpinLock::unlock(InterruptState state) { - ASSERT_EQ(m_locker.load(), Processor::current_id()); - ASSERT_GT(m_lock_depth, 0); + ASSERT(m_locker == Processor::current_id()); + ASSERT(m_lock_depth > 0); if (--m_lock_depth == 0) m_locker.store(PROCESSOR_NONE, BAN::MemoryOrder::memory_order_release); Processor::set_interrupt_state(state); diff --git a/kernel/kernel/Memory/FileBackedRegion.cpp b/kernel/kernel/Memory/FileBackedRegion.cpp index 09c527db..c6718d5c 100644 --- a/kernel/kernel/Memory/FileBackedRegion.cpp +++ b/kernel/kernel/Memory/FileBackedRegion.cpp @@ -129,7 +129,7 @@ namespace Kernel size_t file_offset = m_offset + (vaddr - m_vaddr); size_t bytes = BAN::Math::min(m_size - file_offset, PAGE_SIZE); - ASSERT_EQ(&PageTable::current(), &m_page_table); + ASSERT(&PageTable::current() == &m_page_table); auto read_ret = m_inode->read(file_offset, BAN::ByteSpan((uint8_t*)vaddr, bytes)); if (read_ret.is_error()) diff --git a/kernel/kernel/Memory/VirtualRange.cpp b/kernel/kernel/Memory/VirtualRange.cpp index 3186ece8..ce37e31d 100644 --- a/kernel/kernel/Memory/VirtualRange.cpp +++ b/kernel/kernel/Memory/VirtualRange.cpp @@ -158,9 +158,9 @@ namespace Kernel return; // Verify no overflow - ASSERT_LTE(bytes, size()); - ASSERT_LTE(offset, size()); - ASSERT_LTE(offset, size() - bytes); + ASSERT(bytes <= size()); + ASSERT(offset <= size()); + ASSERT(offset <= size() - bytes); if (&PageTable::current() == &m_page_table || &PageTable::kernel() == &m_page_table) { diff --git a/kernel/kernel/Networking/E1000/E1000.cpp b/kernel/kernel/Networking/E1000/E1000.cpp index 24e7f0f6..ec9d5266 100644 --- a/kernel/kernel/Networking/E1000/E1000.cpp +++ b/kernel/kernel/Networking/E1000/E1000.cpp @@ -259,7 +259,7 @@ namespace Kernel BAN::ErrorOr E1000::send_bytes(BAN::MACAddress destination, EtherType protocol, BAN::ConstByteSpan buffer) { - ASSERT_LTE(buffer.size() + sizeof(EthernetHeader), E1000_TX_BUFFER_SIZE); + ASSERT(buffer.size() + sizeof(EthernetHeader) <= E1000_TX_BUFFER_SIZE); SpinLockGuard _(m_lock); @@ -299,7 +299,7 @@ namespace Kernel auto& descriptor = reinterpret_cast(m_rx_descriptor_region->vaddr())[rx_current]; if (!(descriptor.status & 1)) break; - ASSERT_LTE((uint16_t)descriptor.length, E1000_RX_BUFFER_SIZE); + ASSERT(descriptor.length <= E1000_RX_BUFFER_SIZE); NetworkManager::get().on_receive(*this, BAN::ConstByteSpan { reinterpret_cast(m_rx_buffer_region->vaddr() + rx_current * E1000_RX_BUFFER_SIZE), diff --git a/kernel/kernel/Networking/TCPSocket.cpp b/kernel/kernel/Networking/TCPSocket.cpp index 18c46708..b53736eb 100644 --- a/kernel/kernel/Networking/TCPSocket.cpp +++ b/kernel/kernel/Networking/TCPSocket.cpp @@ -455,7 +455,7 @@ namespace Kernel if (m_send_window.data_size > 0 && m_send_window.current_ack - m_send_window.has_ghost_byte > m_send_window.start_seq) { uint32_t acknowledged_bytes = m_send_window.current_ack - m_send_window.start_seq - m_send_window.has_ghost_byte; - ASSERT_LTE(acknowledged_bytes, m_send_window.data_size); + ASSERT(acknowledged_bytes <= m_send_window.data_size); m_send_window.data_size -= acknowledged_bytes; m_send_window.start_seq += acknowledged_bytes; diff --git a/kernel/kernel/Panic.cpp b/kernel/kernel/Panic.cpp index 1c0ad8a7..ce82d162 100644 --- a/kernel/kernel/Panic.cpp +++ b/kernel/kernel/Panic.cpp @@ -1,6 +1,6 @@ #include -namespace Kernel::detail +namespace Kernel { - bool g_paniced = false; + volatile bool g_paniced = false; } diff --git a/kernel/kernel/Thread.cpp b/kernel/kernel/Thread.cpp index 0b5c4232..7e44db5e 100644 --- a/kernel/kernel/Thread.cpp +++ b/kernel/kernel/Thread.cpp @@ -170,7 +170,7 @@ namespace Kernel // Signal mask is inherited // Setup stack for returning - ASSERT_EQ(m_rsp % PAGE_SIZE, 0u); + ASSERT(m_rsp % PAGE_SIZE == 0); PageTable::with_fast_page(process().page_table().physical_address_of(m_rsp - PAGE_SIZE), [&] { uintptr_t rsp = PageTable::fast_page() + PAGE_SIZE; write_to_stack(rsp, nullptr); // alignment @@ -199,7 +199,7 @@ namespace Kernel m_signal_pending_mask = 0; m_signal_block_mask = ~0ull; - ASSERT_EQ(m_rsp % PAGE_SIZE, 0u); + ASSERT(m_rsp % PAGE_SIZE == 0); PageTable::with_fast_page(process().page_table().physical_address_of(m_rsp - PAGE_SIZE), [&] { uintptr_t rsp = PageTable::fast_page() + PAGE_SIZE; write_to_stack(rsp, nullptr); // alignment diff --git a/libc/CMakeLists.txt b/libc/CMakeLists.txt index 8d49a855..84a6e388 100644 --- a/libc/CMakeLists.txt +++ b/libc/CMakeLists.txt @@ -30,6 +30,8 @@ set(LIBC_SOURCES unistd.cpp math.S icxxabi.cpp + + ../BAN/BAN/Assert.cpp ) add_custom_target(libc-headers