Syscall: Fix Syscall::Call's X86-64 implementation for CFI unwinding

Syscall: Fix Syscall::Call's X86-64 implementation for CFI unwinding

The LEA instruction within the inline assembly statement was throwing
off glibc's backtrace() function, because it lost track of where the
stack was.  The easy fix for this is to convert SyscallAsm() to simply
use the standard C calling convention on X86-64, and make it into a
normal C function call so the compiler can ensure CFI information is
correct for us.

While here, there's no need to use the "call/pop/addq" trick to
compute a PC-relative address because we have %rip-based addressing.
So simply use "lea 2f(%rip), %rax" to compute the return address (and
avoid branch mispredictions from desync'ing the call stack).

BUG=424973
Committed: https://crrev.com/77a224a0aacac5bff5af234b5a583f499ebb146f
Cr-Commit-Position: refs/heads/master@{#300374}

[mdempsky, 2014-10-20 20:26:46]

mdempsky@chromium.org changed reviewers:
+ jorgelo@chromium.org, leecam@chromium.org

[mdempsky, 2014-10-20 20:26:47]

Seems to work locally.  Can one of you test on the PFQ or tell me how to do
that?

[Jorge Lucangeli Obes, 2014-10-20 20:31:05]

On 2014/10/20 20:26:47, mdempsky wrote:
> Seems to work locally.  Can one of you test on the PFQ or tell me how to do
> that?

cbuildbot --remote -G <CL #> amd64-generic-tot-chrome-pfq-informational

cbuildbot lives in depot_tools.

[Jorge Lucangeli Obes, 2014-10-20 20:35:47]

On 2014/10/20 20:31:05, Jorge Lucangeli Obes wrote:
> On 2014/10/20 20:26:47, mdempsky wrote:
> > Seems to work locally.  Can one of you test on the PFQ or tell me how to do
> > that?
> 
> cbuildbot --remote -G <CL #> amd64-generic-tot-chrome-pfq-informational
> 
> cbuildbot lives in depot_tools.

Although you would need to re-enable the test that I just disabled. You can pass
more than one CL to -G, enclosed in double quotes and separated by spaces. We
would need to create another CL with a revert of my earlier CL.

[mdempsky, 2014-10-20 20:37:09]

On 2014/10/20 20:35:47, Jorge Lucangeli Obes wrote:
> On 2014/10/20 20:31:05, Jorge Lucangeli Obes wrote:
> > On 2014/10/20 20:26:47, mdempsky wrote:
> > > Seems to work locally.  Can one of you test on the PFQ or tell me how to
do
> > > that?
> > 
> > cbuildbot --remote -G <CL #> amd64-generic-tot-chrome-pfq-informational
> > 
> > cbuildbot lives in depot_tools.
> 
> Although you would need to re-enable the test that I just disabled. You can
pass
> more than one CL to -G, enclosed in double quotes and separated by spaces. We
> would need to create another CL with a revert of my earlier CL.

Ah, I just re-enabled the test in patch set 2.

$ cbuildbot --remote -G 661393004 amd64-generic-tot-chrome-pfq-informational
Verifying patches...
Submitting tryjob...
Tryjob submitted!
Go to
https://uberchromegw.corp.google.com/i/chromiumos.tryserver/waterfall?committ...
to view the status of your job.

And now we play the waiting game...

... waiting game sucks, let's get lunch.

[rickyz (Google), 2014-10-20 20:39:53]

rickyz@google.com changed reviewers:
+ rickyz@google.com

[rickyz (Google), 2014-10-20 20:39:53]

Nice catch :-) Hope this passes!

https://codereview.chromium.org/661393004/diff/1/sandbox/linux/seccomp-bpf/sy...
File sandbox/linux/seccomp-bpf/syscall.cc (right):

https://codereview.chromium.org/661393004/diff/1/sandbox/linux/seccomp-bpf/sy...
sandbox/linux/seccomp-bpf/syscall.cc:92: // Check if "%rax" is negative. If so,
do not attempt to make a
Comment should probably be updated.

[jln (very slow on Chromium), 2014-10-20 21:24:01]

jln@chromium.org changed reviewers:
+ jln@chromium.org

[jln (very slow on Chromium), 2014-10-20 21:24:02]

lgtm, this should be a lot more future-proof!

https://codereview.chromium.org/661393004/diff/20001/sandbox/linux/seccomp-bp...
File sandbox/linux/seccomp-bpf/syscall.cc (right):

https://codereview.chromium.org/661393004/diff/20001/sandbox/linux/seccomp-bp...
sandbox/linux/seccomp-bpf/syscall.cc:92: // Check if "%rax" is negative. If so,
do not attempt to make a
Upate comment

https://codereview.chromium.org/661393004/diff/20001/sandbox/linux/seccomp-bp...
sandbox/linux/seccomp-bpf/syscall.cc:102: // We declared all clobbered registers
to the compiler. On x86-64,
Update comment. To make review easier, we could write that the called-saved
registers are limited to rbx, rbp and r12-r15

https://codereview.chromium.org/661393004/diff/20001/sandbox/linux/seccomp-bp...
sandbox/linux/seccomp-bpf/syscall.cc:310: ret = SyscallAsm(ret, args);
Shouldn't we pass nr instead of ret?

[mdempsky, 2014-10-20 21:41:21]

https://codereview.chromium.org/661393004/diff/1/sandbox/linux/seccomp-bpf/sy...
File sandbox/linux/seccomp-bpf/syscall.cc (right):

https://codereview.chromium.org/661393004/diff/1/sandbox/linux/seccomp-bpf/sy...
sandbox/linux/seccomp-bpf/syscall.cc:92: // Check if "%rax" is negative. If so,
do not attempt to make a
On 2014/10/20 20:39:53, rickyz (Google) wrote:
> Comment should probably be updated.

Done.

https://codereview.chromium.org/661393004/diff/20001/sandbox/linux/seccomp-bp...
File sandbox/linux/seccomp-bpf/syscall.cc (right):

https://codereview.chromium.org/661393004/diff/20001/sandbox/linux/seccomp-bp...
sandbox/linux/seccomp-bpf/syscall.cc:92: // Check if "%rax" is negative. If so,
do not attempt to make a
On 2014/10/20 21:24:02, jln wrote:
> Upate comment

Done.

https://codereview.chromium.org/661393004/diff/20001/sandbox/linux/seccomp-bp...
sandbox/linux/seccomp-bpf/syscall.cc:102: // We declared all clobbered registers
to the compiler. On x86-64,
On 2014/10/20 21:24:02, jln wrote:
> Update comment. To make review easier, we could write that the called-saved
> registers are limited to rbx, rbp and r12-r15

Done.

https://codereview.chromium.org/661393004/diff/20001/sandbox/linux/seccomp-bp...
sandbox/linux/seccomp-bpf/syscall.cc:310: ret = SyscallAsm(ret, args);
On 2014/10/20 21:24:02, jln wrote:
> Shouldn't we pass nr instead of ret?

Done.

[mdempsky, 2014-10-20 22:05:27]

jorgelo@ reports this appears to have fixed the Chrome OS buildbot, so sending
to CQ.

[mdempsky, 2014-10-20 22:05:31]

The CQ bit was checked by mdempsky@chromium.org

[commit-bot: I haz the power, 2014-10-20 22:06:27]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/661393004/60001

[commit-bot: I haz the power, 2014-10-20 23:19:33]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2014-10-20 23:20:24]

Patchset 4 (id:??) landed as
https://crrev.com/77a224a0aacac5bff5af234b5a583f499ebb146f
Cr-Commit-Position: refs/heads/master@{#300374}