From: Nadav Har'El <n...@scylladb.com>
Committer: Nadav Har'El <n...@scylladb.com>
Branch: master
Clean mutex_init()/mutex_destroy() warnings in gcc 8
In theory, the functions mutex_init()/mutex_destroy() are not needed at all
in C++ code, because creating a mutex object already initializes (zeros) it,
and it is destructed (doing nothing) automatically too. However, for
historic
reasons, in a lot of code that started out as C and later moved to C++,
we do have calls to those functions in C++ code.
But, mutex_init() uses memset(), and Gcc starting in version 8 complains
about code which does memset and friends to a non-trivial C++ object.
So our fix is 3-pronged:
1. Reduce the usage of mutex_init()/mutex_destroy() in C++ code - e.g., if
we just "new"ed a mutex, or it is a static variable, we don't need to
initialize again.
2. For the little code remaining which was too messy to fix, and still
calls mutex_init()/mutex_destroy() we leave these functions - but as
*macros* instead of functions. This allows the compiler to complain
only for the call site - not in every file which #includes mutex.h.
3. For the few mutex_init() call sites which remain and generate the gcc 8
warnings, we will add (in a following patch) -Wno-class-memaccess
to tell gcc to not test for this in those specific files.
Fixes #957
Signed-off-by: Nadav Har'El <n...@scylladb.com>
---
diff --git a/bsd/porting/synch.cc b/bsd/porting/synch.cc
--- a/bsd/porting/synch.cc
+++ b/bsd/porting/synch.cc
@@ -31,8 +31,8 @@ struct synch_thread {
class synch_port {
public:
- synch_port() { mutex_init(&_lock); }
- virtual ~synch_port() { mutex_destroy(&_lock); }
+ synch_port() { }
+ virtual ~synch_port() { }
static synch_port* instance() {
if (_instance == nullptr) {
diff --git a/bsd/sys/net/routecache.hh b/bsd/sys/net/routecache.hh
--- a/bsd/sys/net/routecache.hh
+++ b/bsd/sys/net/routecache.hh
@@ -71,7 +71,9 @@ public:
memcpy((void*) this, (const void*) &other, sizeof(*this));
// try to catch some monkey business
rt_refcnt = -1;
+#if 0
mutex_init(&rt_mtx._mutex);
+#endif
}
nonlockable_rtentry() {
// This shouldn't be necessary, but cache[dst] = ... uses it
@@ -81,7 +83,9 @@ public:
memcpy((void*) this, (const void*) &other, sizeof(*this));
// try to catch some monkey business
rt_refcnt = -1;
+#if 0
mutex_init(&rt_mtx._mutex);
+#endif
return *this;
}
};
@@ -156,7 +160,9 @@ public:
memcpy(ret, ro.ro_rt, sizeof(*ret));
RO_RTFREE(&ro);
ret->rt_refcnt = -1; // try to catch some monkey-business
+#if 0
mutex_init(&ret->rt_mtx._mutex); // try to catch some
monkey-business?
+#endif
// Add the result to the cache
WITH_LOCK(cache_mutex) {
auto *old_cache = cache.read_by_owner();
diff --git a/bsd/sys/net/rtsock.cc b/bsd/sys/net/rtsock.cc
--- a/bsd/sys/net/rtsock.cc
+++ b/bsd/sys/net/rtsock.cc
@@ -155,7 +155,6 @@ rts_init(void)
if (TUNABLE_INT_FETCH("net.route.netisr_maxqlen", &tmp))
rtsock_nh.nh_qlimit = tmp;
#endif
- mutex_init(&rtsock_mtx);
netisr_register(&rtsock_nh);
}
SYSINIT(rtsock, SI_SUB_PROTO_DOMAIN, SI_ORDER_THIRD, rts_init, 0);
diff --git a/bsd/sys/netinet/in_pcb.h b/bsd/sys/netinet/in_pcb.h
--- a/bsd/sys/netinet/in_pcb.h
+++ b/bsd/sys/netinet/in_pcb.h
@@ -334,9 +334,12 @@ struct inpcbinfo {
#ifdef _KERNEL
-#define INP_LOCK_INIT(inp, d, t) \
- mutex_init(&(inp)->inp_lock)
-#define INP_LOCK_DESTROY(inp) mutex_destroy(&(inp)->inp_lock)
+// No need to do any initialization to the lock, if the inp object was
+// created in C++ and the constructor ran (i.e., with new)
+//#define INP_LOCK_INIT(inp, d, t) mutex_init(&(inp)->inp_lock)
+//#define INP_LOCK_DESTROY(inp) mutex_destroy(&(inp)->inp_lock)
+#define INP_LOCK_INIT(inp, d, t)
+#define INP_LOCK_DESTROY(inp, d, t)
#define INP_LOCK(inp) mutex_lock(&(inp)->inp_lock)
#define INP_TRY_LOCK(inp) mutex_try_lock(&(inp)->inp_lock)
#define INP_UNLOCK(inp) mutex_unlock(&(inp)->inp_lock)
diff --git a/fs/vfs/vfs_bio.cc b/fs/vfs/vfs_bio.cc
--- a/fs/vfs/vfs_bio.cc
+++ b/fs/vfs/vfs_bio.cc
@@ -358,7 +358,6 @@ bio_init(void)
auto* bp = &buf_table[i];
bp->b_flags = B_INVAL;
bp->b_data = malloc(BSIZE);
- mutex_init(&bp->b_lock);
free_list.push_back(*bp);
}
sem_init(&free_sem, 0, NBUFS);
diff --git a/fs/vfs/vfs_dentry.cc b/fs/vfs/vfs_dentry.cc
--- a/fs/vfs/vfs_dentry.cc
+++ b/fs/vfs/vfs_dentry.cc
@@ -82,7 +82,6 @@ dentry_alloc(struct dentry *parent_dp, struct vnode *vp,
const char *path)
dp->d_vnode = vp;
dp->d_mount = mp;
dp->d_path = strdup(path);
- mutex_init(&dp->d_lock);
LIST_INIT(&dp->d_children);
if (parent_dp) {
diff --git a/fs/vfs/vfs_vnode.cc b/fs/vfs/vfs_vnode.cc
--- a/fs/vfs/vfs_vnode.cc
+++ b/fs/vfs/vfs_vnode.cc
@@ -184,27 +184,23 @@ vget(struct mount *mp, uint64_t ino, struct vnode
**vpp)
return 1;
}
- if (!(vp = new vnode)) {
+ if (!(vp = new vnode())) {
VNODE_UNLOCK();
return 0;
}
- memset(vp, 0, sizeof(struct vnode));
-
LIST_INIT(&vp->v_names);
vp->v_ino = ino;
vp->v_mount = mp;
vp->v_refcnt = 1;
vp->v_op = mp->m_op->vfs_vnops;
- mutex_init(&vp->v_lock);
vp->v_nrlocks = 0;
/*
* Request to allocate fs specific data for vnode.
*/
if ((error = VFS_VGET(mp, vp)) != 0) {
VNODE_UNLOCK();
- mutex_destroy(&vp->v_lock);
delete vp;
return error;
}
@@ -250,7 +246,6 @@ vput(struct vnode *vp)
vp->v_nrlocks--;
ASSERT(vp->v_nrlocks == 0);
mutex_unlock(&vp->v_lock);
- mutex_destroy(&vp->v_lock);
delete vp;
}
@@ -296,7 +291,6 @@ vrele(struct vnode *vp)
*/
VOP_INACTIVE(vp);
vfs_unbusy(vp->v_mount);
- mutex_destroy(&vp->v_lock);
delete vp;
}
diff --git a/include/osv/mutex.h b/include/osv/mutex.h
--- a/include/osv/mutex.h
+++ b/include/osv/mutex.h
@@ -61,9 +61,23 @@ static inline void mutex_unlock(mutex_t *m) {
lockfree_mutex_unlock(m); }
static inline bool mutex_trylock(mutex_t *m) { return
lockfree_mutex_try_lock(m); }
static inline bool mutex_owned(mutex_t *m) { return
lockfree_mutex_owned(m); }
#endif
-/** both C and C++ code currently use these, though they should be C-only
**/
+#ifndef __cplusplus
static inline void mutex_init(mutex_t* m) { memset(m, 0, sizeof(mutex_t));
}
static inline void mutex_destroy(mutex_t* m) { }
+#else
+// In C++ code, mutex_init()/mutex_destroy() should NOT be used. Rather,
+// mutex's constructor and destructor will work. Importantly, both the C
+// mutex_init() and the C++ constructor do the same thing (set all the
+// structure's fields to zero).
+// Nevertheless, for compatibility with old C code which was transfered to
C++
+// we want to support mutex_init()/mutex_destroy(). If they are functions,
+// gcc 8 complains *here* that the memset call is not a good idea on an
+// object (and it usually isn't!). So we make them macros, so the compiler
+// will only complain at the call site (and the check can be disabled with
+// -Wno-class-memaccess for each call site).
+#define mutex_init(m) memset(m, 0, sizeof(mutex_t))
+#define mutex_destroy(m)
+#endif
#define MUTEX_INITIALIZER {}
#ifdef __cplusplus
--
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.
For more options, visit https://groups.google.com/d/optout.
