Messages in this thread | | | Date | Thu, 26 Mar 2020 13:58:44 +0100 | From | Peter Zijlstra <> | Subject | Re: [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.
| |