linux-kernel - Re: [PATCH v2 02/11] kcov: apply clang-format to kcov code

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20250627080248.GQ1613200@noisy.programming.kicks-ass.net>
Date: Fri, 27 Jun 2025 10:02:48 +0200
From: Peter Zijlstra <peterz@...radead.org>
To: Alexander Potapenko <glider@...gle.com>
Cc: quic_jiangenj@...cinc.com, linux-kernel@...r.kernel.org,
	kasan-dev@...glegroups.com, Aleksandr Nogikh <nogikh@...gle.com>,
	Andrey Konovalov <andreyknvl@...il.com>,
	Borislav Petkov <bp@...en8.de>,
	Dave Hansen <dave.hansen@...ux.intel.com>,
	Dmitry Vyukov <dvyukov@...gle.com>, Ingo Molnar <mingo@...hat.com>,
	Josh Poimboeuf <jpoimboe@...nel.org>,
	Marco Elver <elver@...gle.com>,
	Thomas Gleixner <tglx@...utronix.de>
Subject: Re: [PATCH v2 02/11] kcov: apply clang-format to kcov code

On Thu, Jun 26, 2025 at 03:41:49PM +0200, Alexander Potapenko wrote:
> kcov used to obey clang-format style, but somehow diverged over time.
> This patch applies clang-format to kernel/kcov.c and
> include/linux/kcov.h, no functional change.

I'm not sure I agree this is in fact a good thing. Very questionable
style choices made.

I had to kill clang-format hard in my nvim-lsp-clangd setup, because
clang-format is such a piece of shit.


> -static inline void kcov_task_init(struct task_struct *t) {}
> -static inline void kcov_task_exit(struct task_struct *t) {}
> -static inline void kcov_prepare_switch(struct task_struct *t) {}
> -static inline void kcov_finish_switch(struct task_struct *t) {}
> -static inline void kcov_remote_start(u64 handle) {}
> -static inline void kcov_remote_stop(void) {}
> +static inline void kcov_task_init(struct task_struct *t)
> +{
> +}
> +static inline void kcov_task_exit(struct task_struct *t)
> +{
> +}
> +static inline void kcov_prepare_switch(struct task_struct *t)
> +{
> +}
> +static inline void kcov_finish_switch(struct task_struct *t)
> +{
> +}
> +static inline void kcov_remote_start(u64 handle)
> +{
> +}
> +static inline void kcov_remote_stop(void)
> +{
> +}

This is not an improvement.

> @@ -52,36 +53,36 @@ struct kcov {
>  	 *  - task with enabled coverage (we can't unwire it from another task)
>  	 *  - each code section for remote coverage collection
>  	 */
> -	refcount_t		refcount;
> +	refcount_t refcount;
>  	/* The lock protects mode, size, area and t. */
> -	spinlock_t		lock;
> -	enum kcov_mode		mode;
> +	spinlock_t lock;
> +	enum kcov_mode mode;
>  	/* Size of arena (in long's). */
> -	unsigned int		size;
> +	unsigned int size;
>  	/* Coverage buffer shared with user space. */
> -	void			*area;
> +	void *area;
>  	/* Task for which we collect coverage, or NULL. */
> -	struct task_struct	*t;
> +	struct task_struct *t;
>  	/* Collecting coverage from remote (background) threads. */
> -	bool			remote;
> +	bool remote;
>  	/* Size of remote area (in long's). */
> -	unsigned int		remote_size;
> +	unsigned int remote_size;
>  	/*
>  	 * Sequence is incremented each time kcov is reenabled, used by
>  	 * kcov_remote_stop(), see the comment there.
>  	 */
> -	int			sequence;
> +	int sequence;
>  };
>  
>  struct kcov_remote_area {
> -	struct list_head	list;
> -	unsigned int		size;
> +	struct list_head list;
> +	unsigned int size;
>  };
>  
>  struct kcov_remote {
> -	u64			handle;
> -	struct kcov		*kcov;
> -	struct hlist_node	hnode;
> +	u64 handle;
> +	struct kcov *kcov;
> +	struct hlist_node hnode;
>  };
>  
>  static DEFINE_SPINLOCK(kcov_remote_lock);
> @@ -89,14 +90,14 @@ static DEFINE_HASHTABLE(kcov_remote_map, 4);
>  static struct list_head kcov_remote_areas = LIST_HEAD_INIT(kcov_remote_areas);
>  
>  struct kcov_percpu_data {
> -	void			*irq_area;
> -	local_lock_t		lock;
> -
> -	unsigned int		saved_mode;
> -	unsigned int		saved_size;
> -	void			*saved_area;
> -	struct kcov		*saved_kcov;
> -	int			saved_sequence;
> +	void *irq_area;
> +	local_lock_t lock;
> +
> +	unsigned int saved_mode;
> +	unsigned int saved_size;
> +	void *saved_area;
> +	struct kcov *saved_kcov;
> +	int saved_sequence;
>  };
>  
>  static DEFINE_PER_CPU(struct kcov_percpu_data, kcov_percpu_data) = {

This is just plain wrong. Making something that was readable into a
trainwreck.


Please either teach clang-format sensible style choices, or refrain from
using it.