Re: [PATCH 02/25] tracing: Improve "if" macro code generation

[Linus Torvalds <torvalds@linux-foundation.org>]

[-- Attachment #1: Type: text/plain, Size: 1187 bytes --]

On Wed, Mar 20, 2019 at 4:17 AM David Laight <David.Laight@aculab.com> wrote:
>
> >               ______r = !!(cond);                                     \
>
>         Is that (or maybe just the !!) needed any more??

It is, because the 'cond' expression might not be an int, it could be
a test for a pointer being non-NULL, or an u64 being non-zero, and not
having the "!!" would mean that you'd get a warning or drop bits when
assigning to 'int'.

And you do need the new temporary variable to avoid double evaluation
the way that code is written.

That said, I do think the code is really ugly. We could:

 - avoid the temporary by just simplifying things.

 - do the '!!' just once in the parent macro.

 - Steven has this crazy model of "more underscores are better". They
aren't. They don't help if things nest anyway, but what does help is
meaningful names. Both when things don't nest, and when looking at
generated asm files.

 - ,, and finally, what _is_ better is to chop things up so that they
are smaller and make each macro do only one thing

So maybe do the patch something like the attached instead? Completely
untested, but it looks sane to me.

                  Linus

[-- Attachment #2: patch.diff --]
[-- Type: text/x-patch, Size: 1490 bytes --]

 include/linux/compiler.h | 35 ++++++++++++++++++-----------------
 1 file changed, 18 insertions(+), 17 deletions(-)

diff --git a/include/linux/compiler.h b/include/linux/compiler.h
index 445348facea9..8aaf7cd026b0 100644
--- a/include/linux/compiler.h
+++ b/include/linux/compiler.h
@@ -53,23 +53,24 @@ void ftrace_likely_update(struct ftrace_likely_data *f, int val,
  * "Define 'is'", Bill Clinton
  * "Define 'if'", Steven Rostedt
  */
-#define if(cond, ...) __trace_if( (cond , ## __VA_ARGS__) )
-#define __trace_if(cond) \
-	if (__builtin_constant_p(!!(cond)) ? !!(cond) :			\
-	({								\
-		int ______r;						\
-		static struct ftrace_branch_data			\
-			__aligned(4)					\
-			__section("_ftrace_branch")			\
-			______f = {					\
-				.func = __func__,			\
-				.file = __FILE__,			\
-				.line = __LINE__,			\
-			};						\
-		______r = !!(cond);					\
-		______f.miss_hit[______r]++;					\
-		______r;						\
-	}))
+#define if(cond, ...) if ( __trace_if_var( !!(cond , ## __VA_ARGS__) ) )
+
+#define __trace_if_var(cond) (__builtin_constant_p(cond) ? (cond) : __trace_if_value(cond))
+
+#define __trace_if_value(cond) ({			\
+	static struct ftrace_branch_data		\
+		__aligned(4)				\
+		__section("_ftrace_branch")		\
+		__if_trace = {				\
+			.func = __func__,		\
+			.file = __FILE__,		\
+			.line = __LINE__,		\
+		};					\
+	(cond) ?					\
+		(__if_trace.miss_hit[1]++,1) :		\
+		(__if_trace.miss_hit[0]++,0);		\
+})
+
 #endif /* CONFIG_PROFILE_ALL_BRANCHES */
 
 #else