[ Resending again with a "reply-all" instead of just "reply" ]
On Thu, 16 Mar 2017 10:40:24 -0700 Linus Torvalds <[email protected]> wrote: > On Thu, Mar 16, 2017 at 10:20 AM, Steven Rostedt <[email protected]> wrote: > > > > When ftrace_regs_caller was created, it was designed to preserve flags as > > much as possible as it needed to act just like a breakpoint triggered on the > > same location. But the design is over complicated as it treated all > > operations as modifying flags. But push, mov and lea do not modify flags. > > This means the code can become more simplified by allowing flags to be > > stored further down. > > It still looks overly complicated to me. > > The snippet below is the patch without the "-" lines, so it's the end result: > > > ENTRY(ftrace_regs_caller) > > /* > > * i386 does not save SS and ESP when coming from kernel. > > * Instead, to get sp, ®s->sp is used (see ptrace.h). > > * Unfortunately, that means eflags must be at the same location > > * as the current return ip is. We move the return ip into the > > + * regs->ip location, and move flags into the return ip location. > > + */ > > + pushl $__KERNEL_CS > > + pushl 4(%esp) /* Save the return ip */ > > + > > + /* temporarily save flags in the orig_ax location */ > > + pushf > > > > pushl %gs > > pushl %fs > > pushl %es > > pushl %ds > > pushl %eax > > + > > + /* move flags into the location of where the return ip was */ > > + movl 5*4(%esp), %eax > > + movl $0, 5*4(%esp) /* Load 0 into orig_ax */ > > + movl %eax, 8*4(%esp) /* Load flags in return ip > > */ > > Why do you do that silly "temporarily save flags" thing? > > Why not just push $0 there? > > Afaik, the sequence could/should be: > > pushl $__KERNEL_CS > pushl 4(%esp) /* Save the return ip */ > pushl $0 > pushl %gs > pushl %fs > pushl %es > pushl %ds > pushl %eax > > /* Fix up eflags now that we have a scratch register */ > pushfl > popl %eax > movl %eax,8(%rsp) > > Or something. There's no point in the unnecessary "shuffle values back > and forth with odd stack offsets". Sure, I can do this. This is the issue of trying to do too much at first and then eliminating what you don't need. :-p I think previous versions (where I was trying to horribly add a stack frame here) had some more logic. -- Steve
