On Fri, Oct 22, 2021 at 01:37:37PM +0000, Visa Hankala wrote:
> Mutex and rwlock assertions turn into no-ops when compiling without
> the DIAGNOSTIC option, which is the desired outcome. However, in some
> peculiar cases, such as klist_mutex_assertlk() and klist_rwlock_assertlk(),
> the lock variable insidiously becomes unused in the function. This could
> be avoided if the non-DIAGNOSTIC assertions had builtin void casts of
> the macro parameter. The compiler should optimize away any resulting
> side-effect-free code.
I have received some positive feedback for this patch. Nevertheless,
I am still somewhat unsettled on whether this patch is an actual
improvement.
Here are a few more thoughts:
I think the main merit of this is that the code becomes more consistent
relative to DIAGNOSTIC.
However, it is no longer strongly guaranteed that the expression x in
MUTEX_ASSERT_LOCKED(x) really gets eliminated in non-DIAGNOSTIC builds.
The expression will likely not be eliminated for example if it uses
a function from outside the current C module. Such code is quite
unusual, though.
The possibly resulting extra code comes silently, whereas the original
non-DIAGNOSTIC code gives a visible compiler error should the lock
variable become unused.
Also, this change can be seen as an argument for tweaking KASSERT() and
others, which might not be a good thing.
> Index: arch/hppa/include/mutex.h
> ===================================================================
> RCS file: src/sys/arch/hppa/include/mutex.h,v
> retrieving revision 1.9
> diff -u -p -r1.9 mutex.h
> --- arch/hppa/include/mutex.h 13 Jan 2018 15:18:11 -0000 1.9
> +++ arch/hppa/include/mutex.h 22 Oct 2021 12:17:55 -0000
> @@ -83,8 +83,8 @@ void __mtx_init(struct mutex *, int);
> panic("mutex %p held in %s", (mtx), __func__); \
> } while (0)
> #else
> -#define MUTEX_ASSERT_LOCKED(mtx) do { } while (0)
> -#define MUTEX_ASSERT_UNLOCKED(mtx) do { } while (0)
> +#define MUTEX_ASSERT_LOCKED(mtx) ((void)(mtx))
> +#define MUTEX_ASSERT_UNLOCKED(mtx) ((void)(mtx))
> #endif
>
> #define MUTEX_LOCK_OBJECT(mtx) (&(mtx)->mtx_lock_obj)
> Index: arch/m88k/include/mutex.h
> ===================================================================
> RCS file: src/sys/arch/m88k/include/mutex.h,v
> retrieving revision 1.8
> diff -u -p -r1.8 mutex.h
> --- arch/m88k/include/mutex.h 26 May 2020 11:55:10 -0000 1.8
> +++ arch/m88k/include/mutex.h 22 Oct 2021 12:17:56 -0000
> @@ -79,8 +79,8 @@ void __mtx_init(struct mutex *, int);
>
> #else
>
> -#define MUTEX_ASSERT_LOCKED(mtx) do { /* nothing */ } while (0)
> -#define MUTEX_ASSERT_UNLOCKED(mtx) do { /* nothing */ } while (0)
> +#define MUTEX_ASSERT_LOCKED(mtx) ((void)(mtx))
> +#define MUTEX_ASSERT_UNLOCKED(mtx) ((void)(mtx))
>
> #endif
>
> Index: sys/mutex.h
> ===================================================================
> RCS file: src/sys/sys/mutex.h,v
> retrieving revision 1.18
> diff -u -p -r1.18 mutex.h
> --- sys/mutex.h 23 Apr 2019 13:35:12 -0000 1.18
> +++ sys/mutex.h 22 Oct 2021 12:17:57 -0000
> @@ -84,8 +84,8 @@ void __mtx_init(struct mutex *, int);
> panic("mutex %p held in %s", (mtx), __func__); \
> } while (0)
> #else
> -#define MUTEX_ASSERT_LOCKED(mtx) do { } while (0)
> -#define MUTEX_ASSERT_UNLOCKED(mtx) do { } while (0)
> +#define MUTEX_ASSERT_LOCKED(mtx) ((void)(mtx))
> +#define MUTEX_ASSERT_UNLOCKED(mtx) ((void)(mtx))
> #endif
>
> #define MUTEX_LOCK_OBJECT(mtx) (&(mtx)->mtx_lock_obj)
> Index: sys/rwlock.h
> ===================================================================
> RCS file: src/sys/sys/rwlock.h,v
> retrieving revision 1.28
> diff -u -p -r1.28 rwlock.h
> --- sys/rwlock.h 11 Jan 2021 18:49:38 -0000 1.28
> +++ sys/rwlock.h 22 Oct 2021 12:17:57 -0000
> @@ -158,10 +158,10 @@ void rw_assert_rdlock(struct rwlock *);
> void rw_assert_anylock(struct rwlock *);
> void rw_assert_unlocked(struct rwlock *);
> #else
> -#define rw_assert_wrlock(rwl) ((void)0)
> -#define rw_assert_rdlock(rwl) ((void)0)
> -#define rw_assert_anylock(rwl) ((void)0)
> -#define rw_assert_unlocked(rwl) ((void)0)
> +#define rw_assert_wrlock(rwl) ((void)(rwl))
> +#define rw_assert_rdlock(rwl) ((void)(rwl))
> +#define rw_assert_anylock(rwl) ((void)(rwl))
> +#define rw_assert_unlocked(rwl) ((void)(rwl))
> #endif
>
> int rw_enter(struct rwlock *, int);
>
