lkml.org 
[lkml]   [2020]   [Apr]   [2]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH v2] objtool,ftrace: Implement UNWIND_HINT_RET_OFFSET
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

\
 
 \ /
  Last update: 2020-04-02 10:59    [W:0.462 / U:0.028 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site