Re: [PATCH v2] objtool,ftrace: Implement UNWIND_HINT_RET_OFFSET

[Miroslav Benes]

On Thu, 2 Apr 2020, Julien Thierry wrote:

> 
> 
> On 4/2/20 9:17 AM, Peter Zijlstra wrote:
> > On Thu, Apr 02, 2020 at 09:50:36AM +0200, Peter Zijlstra wrote:
> >> On Thu, Apr 02, 2020 at 07:41:46AM +0100, Julien Thierry wrote:
> > 
> >>> Also, instead of adding a special "arch_exception_frame_size", I could
> >>> suggest:
> >>> - Picking this patch [1] from a completely arbitrary source
> >>> - Getting rid of INSN_STACK type, any instruction could then include stack
> >>> ops on top of their existing semantics, they can just have an empty list
> >>> if
> >>> they don't touch SP/BP
> >>> - x86 decoder adds a stack_op to the iret to modify the stack pointer by
> >>> the
> >>> right amount
> >>
> >> That's not the worst idea, lemme try that.
> > 
> > Something like so then?
> > 
> 
> Yes, you could even remove INSN_STACK from insn_type and just always call
> handle_insn_ops() before the switch statement on insn->type. If the list is
> empty it does nothing.
> This way you wouldn't need to call it for the INSN_EXCEPTION_RETURN case, and
> any type of instructions could use stack_ops.
> 
> 
> And the other suggestion is my other email was that you don't even need to add
> INSN_EXCEPTION_RETURN. You can keep IRET as INSN_CONTEXT_SWITCH by default and
> x86 decoder lookups the symbol conaining an iret. If it's a function symbol,
> it can just set the type to INSN_OTHER so that it caries on to the next
> instruction after having handled the stack_op.
> 
> And everything fits under tools/objtool/arch/x86 :) .
> 
> Or is it too far-fetch'd?

Imho no. Well, it depends. I can see benefits of both approach. PeterZ's 
patch is quite minimal and it demonstrates itself as a one-off hack quite 
well. On the other hand, it is in a generic code, which is not nice 
especially when other archs do not have such thing. So your proposal would 
indeed make sense to hide it in arch-specific code. Especially for the 
future. And INSN_STACK is not used much in the code, so it can be removed 
easily as you proposed.

And going more in direction of supporting more archs in the future, I'd 
say it would make sense to allow more generic things such as "forget about 
INSN_STACK. If an instruction has non-empty stack_ops list, just process 
it". It is definitely more flexible.

So yes, I think it make sense unless I am missing something.

Miroslav