Messages in this thread | | | Date | Thu, 26 Mar 2020 16:04:13 +0100 (CET) | From | Miroslav Benes <> | Subject | Re: [PATCH v4 01/13] objtool: Remove CFI save/restore special case |
| |
On Thu, 26 Mar 2020, Miroslav Benes wrote:
> On Thu, 26 Mar 2020, Peter Zijlstra wrote: > > > 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. > > One could argue that this is really nasty and the correct way should be > > if (cond1) { > if (cond2) { > instr_begin(); > // code2 > instr_end(); > } > } > > Then it should work if instr_begin() marks the next instruction and > instr_end() marks the previous one, no? There is a corner case when code2 > is exactly one instruction, so instr counting would have to be updated.
if (cond1) { instr_begin() if (cond2) { // code2 } // code1 instr_end(); }
is a counter example though, so I take it back.
M
| |