Messages in this thread | | | Date | Wed, 17 Jan 2018 09:18:33 +0100 | From | Peter Zijlstra <> | Subject | Re: [PATCH v2 10/10] objtool: More complex static jump implementation |
| |
On Tue, Jan 16, 2018 at 09:05:31PM -0600, Josh Poimboeuf wrote: > On Tue, Jan 16, 2018 at 03:28:35PM +0100, Peter Zijlstra wrote: > > When using something like: > > > > -#define sched_feat(x) (static_branch_##x(&sched_feat_keys[__SCHED_FEAT_##x])) > > +#define sched_feat(x) (static_branch_##x(&sched_feat_keys[__SCHED_FEAT_##x]) && \ > > + (arch_static_assert(), true)) > > > > we get an objtool assertion fail like: > > > > kernel/sched/fair.o: warning: objtool: hrtick_update()+0xd: static assert FAIL > > > > where: > > > > 0000000000001140 <hrtick_update>: > > 1140: 0f 1f 44 00 00 nopl 0x0(%rax,%rax,1) > > 1145: c3 retq > > 1146: 48 8b b7 30 09 00 00 mov 0x930(%rdi),%rsi > > 114d: 8b 87 d8 09 00 00 mov 0x9d8(%rdi),%eax > > 1153: 48 0f a3 05 00 00 00 bt %rax,0x0(%rip) # 115b <hrtick_update+0x1b> > > 115a: 00 > > 1157: R_X86_64_PC32 __cpu_active_mask-0x4 > > > > and: > > > > RELOCATION RECORDS FOR [__jump_table]: > > 0000000000000150 R_X86_64_64 .text+0x0000000000001140 > > 0000000000000158 R_X86_64_64 .text+0x0000000000001146 > > > > RELOCATION RECORDS FOR [.discard.jump_assert]: > > 0000000000000028 R_X86_64_64 .text+0x000000000000114d > > > > IOW, GCC managed to place the assertion 1 instruction _after_ the > > static jump target. > > > > So while the code generation is fine, the assertion gets placed wrong. > > We can 'fix' this by not only considering the immediate static jump > > locations but also all the unconditional code after it, terminating > > the basic block on any unconditional instruction or branch entry > > point. > > > > Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org> > > This is pretty similar to something I've been wanting to do, which is to > track all basic blocks. But this is fine enough for now.
Right, if we'd have that, we could just mark the entire block as 'static' and be done with it.
> One nit, can you rename "branch_target" to "jump_dest" for consistency > with the existing naming?
Can't, insn->jump_dest already exists.
| |