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 <n...@scylladb.com>
---
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 osv-dev+unsubscr...@googlegroups.com.
To view this discussion on the web visit
https://groups.google.com/d/msgid/osv-dev/20210614062057.1998552-10-nyh%40scylladb.com.
