lkml.org 
[lkml]   [2020]   [Mar]   [26]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH v4 01/13] objtool: Remove CFI save/restore special case
On Thu, Mar 26, 2020 at 12:30:50PM +0100, Peter Zijlstra wrote:

> There is a special case in the UNWIND_HINT_RESTORE code. When, upon
> looking for the UNWIND_HINT_SAVE instruction to restore from, it finds
> the instruction hasn't been visited yet, it normally issues a WARN,
> except when this HINT_SAVE instruction is the first instruction of
> this branch.
>
> The reason for this special case comes apparent when we remove it;
> code like:
>
> if (cond) {
> UNWIND_HINT_SAVE
> // do stuff
> UNWIND_HINT_RESTORE
> }
> // more stuff
>
> will now trigger the warning. This is because UNWIND_HINT_RESTORE is
> just a label, and there is nothing keeping it inside the (extended)
> basic block covered by @cond. It will attach itself to the first
> instruction of 'more stuff' and we'll hit it outside of the @cond,
> confusing things.
>
> I don't much like this special case, it confuses things and will come
> apart horribly if/when the annotation needs to support nesting.
> Instead extend the affected code to at least form an extended basic
> block.
>
> In particular, of the 2 users of this annotation: ftrace_regs_caller()
> and sync_core(), only the latter suffers this problem. Extend it's
> code sequence with a NOP to make it an extended basic block.
>
> This isn't ideal either; stuffing code with NOPs just to make
> annotations work is certainly sub-optimal, but given that sync_core()
> is stupid expensive in any case, one extra nop isn't going to be a
> problem here.

So instr_begin() / instr_end() have this exact problem, but worse. Those
actually do nest and I've ran into the following situation:

if (cond1) {
instr_begin();
// code1
instr_end();
}
// code

if (cond2) {
instr_begin();
// code2
instr_end();
}
// tail

Where objtool then finds the path: !cond1, cond2, which ends up at code2
with 0, instead of 1.

I've also seen:

if (cond) {
instr_begin();
// code1
instr_end();
}
instr_begin();
// code2
instr_end();

Where instr_end() and instr_begin() merge onto the same instruction of
code2 as a 0, and again code2 will issue a false warning.

You can also not make objtool lift the end marker to the previous
instruction, because then:

if (cond1) {
instr_begin();
if (cond2) {
// code2
}
instr_end();
}

Suffers the reverse problem, instr_end() becomes part of the @cond2
block and cond1 grows a path that misses it entirely.

So far I've not had any actual solution except adding a NOP to anchor
the annotation on.

\
 
 \ /
  Last update: 2020-03-26 13:59    [W:0.119 / U:1.052 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site