Memory allocated with aligned_alloc() should be freed with free(), not with delete - although in our actual implemention those are really the same thing. gcc 11 warns about using sched::thread::make() - which uses align_alloc() - and the deleting it with "delete", using std::unique_ptr.
Gcc only warns about this issue in sched.cc so we only fix that file, but note that other places of the code also have unique_ptr<thread> and should eventually use a similar fix. The fix is a new method sched::thread::make_unique<> which returns a std::unique_ptr that holds a thread a knows how to properly delete it. Signed-off-by: Nadav Har'El <[email protected]> --- include/osv/sched.hh | 13 +++++++++++++ core/sched.cc | 12 ++++++------ 2 files changed, 19 insertions(+), 6 deletions(-) diff --git a/include/osv/sched.hh b/include/osv/sched.hh index 1b2847be..010c4766 100644 --- a/include/osv/sched.hh +++ b/include/osv/sched.hh @@ -428,6 +428,19 @@ public: } return new(p) thread(std::forward<Args>(args)...); } + // Since make() doesn't necessarily allocate with "new", dispose() should + // be used to free it, not "delete". However, in practice our new and + // delete are the same, so delete is fine. + static void dispose(thread* p) { + p->~thread(); + free(p); + } + using thread_unique_ptr = std::unique_ptr<thread, decltype(&thread::dispose)>; + template <typename... Args> + static thread_unique_ptr make_unique(Args&&... args) { + return thread_unique_ptr(make(std::forward<Args>(args)...), + thread::dispose); + } private: explicit thread(std::function<void ()> func, attr attributes = attr(), bool main = false, bool app = false); diff --git a/core/sched.cc b/core/sched.cc index 74b4ab95..52cc5472 100644 --- a/core/sched.cc +++ b/core/sched.cc @@ -135,7 +135,7 @@ public: private: mutex _mtx; std::list<thread*> _zombies; - std::unique_ptr<thread> _thread; + thread_unique_ptr _thread; }; cpu::cpu(unsigned _id) @@ -552,7 +552,7 @@ void thread::pin(cpu *target_cpu) // load balancer thread) but a "good-enough" dirty solution is to // temporarily create a new ad-hoc thread, "wakeme". bool do_wakeme = false; - std::unique_ptr<thread> wakeme(thread::make([&] () { + thread_unique_ptr wakeme(thread::make_unique([&] () { wait_until([&] { return do_wakeme; }); t.wake(); }, sched::thread::attr().pin(source_cpu))); @@ -590,7 +590,7 @@ void thread::pin(thread *t, cpu *target_cpu) // where the target thread is currently running. We start here a new // helper thread to follow the target thread's CPU. We could have also // re-used an existing thread (e.g., the load balancer thread). - std::unique_ptr<thread> helper(thread::make([&] { + thread_unique_ptr helper(thread::make_unique([&] { WITH_LOCK(irq_lock) { // This thread started on the same CPU as t, but by now t might // have moved. If that happened, we need to move too. @@ -701,7 +701,7 @@ void thread::unpin() } return; } - std::unique_ptr<thread> helper(thread::make([this] { + thread_unique_ptr helper(thread::make_unique([this] { WITH_LOCK(preempt_lock) { // helper thread started on the same CPU as "this", but by now // "this" might migrated. If that happened helper need to migrate. @@ -973,7 +973,7 @@ thread::thread(std::function<void ()> func, attr attr, bool main, bool app) , _migration_lock_counter(0) , _pinned(false) , _id(0) - , _cleanup([this] { delete this; }) + , _cleanup([this] { dispose(this); }) , _app(app) , _joiner(nullptr) { @@ -1600,7 +1600,7 @@ bool operator<(const timer_base& t1, const timer_base& t2) } thread::reaper::reaper() - : _mtx{}, _zombies{}, _thread(thread::make([=] { reap(); })) + : _mtx{}, _zombies{}, _thread(thread::make_unique([=] { reap(); })) { _thread->start(); } -- 2.31.1 -- You received this message because you are subscribed to the Google Groups "OSv Development" group. To unsubscribe from this group and stop receiving emails from it, send an email to [email protected]. To view this discussion on the web visit https://groups.google.com/d/msgid/osv-dev/20210614062057.1998552-10-nyh%40scylladb.com.
