Re: [PATCH v2 11/10] objtool: Even more complex static block checks

[Peter Zijlstra]

On Tue, Jan 16, 2018 at 09:12:32PM -0600, Josh Poimboeuf wrote:
> On Tue, Jan 16, 2018 at 08:49:17PM +0100, Peter Zijlstra wrote:
> > Subject: objtool: Even more complex static block checks
> > From: Peter Zijlstra <peterz@xxxxxxxxxxxxx>
> > Date: Tue Jan 16 20:17:01 CET 2018
> > 
> > I've observed GCC transform:
> > 
> >   f()
> >   {
> > 	  if (!static_branch_unlikely())
> > 		  return;
> > 
> > 	  static_assert();
> > 	  A;
> >   }
> > 
> >   g()
> >   {
> > 	  f();
> >   }
> > 
> > Into:
> > 
> >   f()
> >   {
> > 	  static_assert();
> > 	  A;
> >   }
> > 
> >   g()
> >   {
> > 	  if (static_branch_unlikely())
> > 		  f();
> >   }
> > 
> > Which results in the assertion landing at f+0. The transformation is
> > valid and useful; it avoids a pointless CALL+RET sequence, so we'll
> > have to teach objtool how to deal with this.
> > 
> > Do this by marking all CALL destinations with static_call when called
> > from a static_block and non_static_call when called outside a
> > static_block. This allows us to identify functions called exclusively
> > from a static_block and start them with a static_block.
> 
> Ew... where'd you place the assertion to trigger this?

Its the patch I pastebin'ed you earlier, also see below.

> It's late and my brain has already clocked out, so I'll need to revisit
> this tomorrow.  But now I'm wondering if my basic block idea would be a
> better way to solve this.

I would think basic-blocks are inside functions, and this patch goes
across functions, something you'd still need even if you had basic
blocks.

Also, basic blocks are non-trivial because they can overlap. I've
implemented something like that before for perf, see commit:

  70fbe0574558 ("perf annotate: Add branch stack / basic block")

We could probably lift that code fairly easily.

---
Subject: jump_label: Add static assertion to every static_branch
From: Peter Zijlstra <peterz@xxxxxxxxxxxxx>
Date: Tue Jan 16 15:27:36 CET 2018

for testing.. not sure we wants this in general

Signed-off-by: Peter Zijlstra (Intel) <peterz@xxxxxxxxxxxxx>
---
 arch/x86/include/asm/jump_label.h |    1 +
 include/linux/jump_label.h        |    8 ++++++--
 2 files changed, 7 insertions(+), 2 deletions(-)

--- a/arch/x86/include/asm/jump_label.h
+++ b/arch/x86/include/asm/jump_label.h
@@ -76,6 +76,7 @@ static __always_inline bool arch_static_
  *
  * Also works with static_cpu_has().
  */
+#define arch_static_assert arch_static_assert
 static __always_inline void arch_static_assert(void)
 {
 	asm volatile ("1:\n\t"
--- a/include/linux/jump_label.h
+++ b/include/linux/jump_label.h
@@ -323,6 +323,10 @@ extern bool ____wrong_branch_error(void)
 
 #ifdef HAVE_JUMP_LABEL
 
+#ifndef arch_static_assert
+#define arch_static_assert (void)
+#endif
+
 /*
  * Combine the right initial value (type) with the right branch order
  * to generate the desired result.
@@ -388,7 +392,7 @@ extern bool ____wrong_branch_error(void)
 		branch = !arch_static_branch_jump(&(x)->key, true);		\
 	else									\
 		branch = ____wrong_branch_error();				\
-	branch;									\
+	branch && (arch_static_assert(), true);					\
 })
 
 #define static_branch_unlikely(x)						\
@@ -400,7 +404,7 @@ extern bool ____wrong_branch_error(void)
 		branch = arch_static_branch(&(x)->key, false);			\
 	else									\
 		branch = ____wrong_branch_error();				\
-	branch;									\
+	branch && (arch_static_assert(), true);					\
 })
 
 #else /* !HAVE_JUMP_LABEL */
> Josh