Grokbase › Groups › Go › golang-dev › November 2012
FAQ
Reviewers: remyoudompheng,

Message:
Hello remyoudompheng@gmail.com (cc: golang-dev@googlegroups.com),

I'd like you to review this change to
https://code.google.com/p/go/


Description:
runtime: duplicate code for runtime.nanotime to avoid stack overflow in
vDSO clock_gettime
Fixes issue 4402.

Please review this at http://codereview.appspot.com/6842063/

Affected files:
M src/pkg/runtime/sys_linux_amd64.s


Index: src/pkg/runtime/sys_linux_amd64.s
===================================================================
--- a/src/pkg/runtime/sys_linux_amd64.s
+++ b/src/pkg/runtime/sys_linux_amd64.s
@@ -126,15 +126,29 @@
RET

TEXT runtime·nanotime(SB), 7, $32
- CALL time·now(SB)
- MOVQ 0(SP), AX // sec
- MOVL 8(SP), DX // nsec
-
- // sec is in AX, usec in DX
+ MOVQ runtime·__vdso_clock_gettime_sym(SB), AX
+ CMPQ AX, $0
+ JEQ fallback_gtod_nt
+ MOVL $0, DI // CLOCK_REALTIME
+ LEAQ 8(SP), SI
+ CALL AX
+ MOVQ 8(SP), AX // sec
+ MOVQ 16(SP), DX // nsec
+exit_nt:
+ // sec is in AX, nsec in DX
// return nsec in AX
IMULQ $1000000000, AX
ADDQ DX, AX
RET
+fallback_gtod_nt:
+ LEAQ 8(SP), DI
+ MOVQ $0, SI
+ MOVQ runtime·__vdso_gettimeofday_sym(SB), AX
+ CALL AX
+ MOVQ 8(SP), AX // sec
+ MOVL 16(SP), DX // usec
+ IMULQ $1000, DX
+ JMP exit_nt

TEXT runtime·rtsigprocmask(SB),7,$0-32
MOVL 8(SP), DI

Search Discussions

  • Remyoudompheng at Nov 18, 2012 at 9:15 am ⇧
    http://codereview.appspot.com/6842063/diff/1002/src/pkg/runtime/sys_linux_amd64.s
    File src/pkg/runtime/sys_linux_amd64.s (right):

    http://codereview.appspot.com/6842063/diff/1002/src/pkg/runtime/sys_linux_amd64.s#newcode150
    src/pkg/runtime/sys_linux_amd64.s:150: IMULQ $1000, DX
    the exit_nt looks a bit hard to read to me, why not simply
    insert the IMULQ+ADDQ here ?

    http://codereview.appspot.com/6842063/
  • Minux Ma at Nov 18, 2012 at 9:20 am ⇧
    PTAL.
    On 2012/11/18 09:15:16, remyoudompheng wrote:
    http://codereview.appspot.com/6842063/diff/1002/src/pkg/runtime/sys_linux_amd64.s#newcode150
    src/pkg/runtime/sys_linux_amd64.s:150: IMULQ $1000, DX
    the exit_nt looks a bit hard to read to me, why not simply
    insert the IMULQ+ADDQ here ?
    I reverted to the patch set 2.

    https://codereview.appspot.com/6842063/
  • Minux Ma at Nov 25, 2012 at 8:39 pm ⇧
    gentle ping.

    https://codereview.appspot.com/6842063/
  • Remyoudompheng at Nov 25, 2012 at 8:47 pm ⇧
    It fixed the issue for me on ArchLinux but I'd like confirmation from
    the other people who had problems. Notably shivakumar.gn who observed it
    on CentOS.

    https://codereview.appspot.com/6842063/
  • Remyoudompheng at Nov 25, 2012 at 8:50 pm ⇧
    Do you think it would be possible to define:

    TEXT runtime·vdso_gettimeofday(SB), 7, $somevalue
    MOVQ runtime·__vdso_gettimeofday_sym(SB), AX
    JMP AX

    so that the stack check in the linker is effective? $somevalue should be
    set to the appropriate value and it would avoid future problems, WDYT?

    https://codereview.appspot.com/6842063/
  • Minux at Nov 25, 2012 at 9:28 pm ⇧

    On Mon, Nov 26, 2012 at 4:50 AM, wrote:

    Do you think it would be possible to define:

    TEXT runtime·vdso_gettimeofday(SB), 7, $somevalue
    we can use this approach if we can determine a suitable `somevalue'.
    however i can't think of a reliable (future proof) way get that value.
    MOVQ runtime·__vdso_gettimeofday_**sym(SB), AX
    also note that you will need to adjust SP here, as the linker will allocate
    somevalue byte of stack for you.
    JMP AX

    so that the stack check in the linker is effective? $somevalue should be
    set to the appropriate value and it would avoid future problems, WDYT?
    and the problem with this approach is:
    the linker's check of non-split stack is static, but the binary might run on
    any kernel version, and the suitable value for `somevalue' might be larger
    than the one used to compile the binary, and the binary might still fail
    on that kernel.

    as I pointed out earlier in the issue tracker, i still think this is a
    kernel bug,
    as vDSO version of "syscall" relies on enough space (but we don't know
    the actual requirement! it could be arbitrarily large [for example, maybe
    a buggy gcc is used to compile the vDSO code]) available on user space
    stack.

    all we can do is to use as little stack space as possible in our code (only
    16-byte is actually used), and hope the kernel won't use more than 100
    bytes of stack.
  • Minux Ma at Nov 25, 2012 at 9:15 pm ⇧

    On 2012/11/25 20:47:00, remyoudompheng wrote:
    It fixed the issue for me on ArchLinux but I'd like confirmation from the other
    people who had problems. Notably shivakumar.gn who observed it on
    CentOS.
    +CC gns, random0x00.

    I've updated my kernel to 3.6.6, however I still can't reproduce the
    problem.

    https://codereview.appspot.com/6842063/
  • Shivakumar Gn at Nov 26, 2012 at 5:34 am ⇧

    On 2012/11/25 21:15:55, minux wrote:
    +CC gns, random0x00.
    I've updated my kernel to 3.6.6, however I still can't reproduce the
    problem.

    With patch set 5, problem does not occur.
    Test was on CentOS6.3 where problem was consistently reproducible
    without the patch.


    https://codereview.appspot.com/6842063/
  • Rsc at Nov 26, 2012 at 5:19 pm ⇧
    LGTM but please add the comments

    This is awful.



    https://codereview.appspot.com/6842063/diff/11002/src/pkg/runtime/sys_linux_amd64.s
    File src/pkg/runtime/sys_linux_amd64.s (right):

    https://codereview.appspot.com/6842063/diff/11002/src/pkg/runtime/sys_linux_amd64.s#newcode105
    src/pkg/runtime/sys_linux_amd64.s:105:
    MOVQ runtime·__vdso_clock_gettime_sym(SB), AX
    // Be careful. We're calling a function with gcc calling convention
    here.
    // We're guaranteed 128 bytes on entry, and we've taken 16, and the
    // call uses another 8.
    // That leaves 104 for the gettime code to use. Hope that's enough!

    https://codereview.appspot.com/6842063/diff/11002/src/pkg/runtime/sys_linux_amd64.s#newcode129
    src/pkg/runtime/sys_linux_amd64.s:129:
    MOVQ runtime·__vdso_clock_gettime_sym(SB), AX
    // Duplicate time.now here to avoid using up precious stack space.
    // See comment above in time.now.

    https://codereview.appspot.com/6842063/
  • Russ Cox at Nov 26, 2012 at 5:20 pm ⇧
    By the way, thank you very much for tracking this down.
  • Minux Ma at Nov 26, 2012 at 5:42 pm ⇧
    *** Submitted as
    http://code.google.com/p/go/source/detail?r=3ec963539561 ***

    runtime: duplicate code for runtime.nanotime to avoid stack overflow in
    vDSO clock_gettime
    Fixes issue 4402.

    R=remyoudompheng, shivakumar.gn, random0x00, rsc
    CC=golang-dev
    http://codereview.appspot.com/6842063


    http://codereview.appspot.com/6842063/

Related Discussions

Discussion Navigation
viewthread | post
Discussion Overview
groupgolang-dev @
categoriesgo
postedNov 18, '12 at 9:09a
activeNov 26, '12 at 5:42p
posts12
users4
websitegolang.org

4 users in discussion

Minux Ma: 6 posts Remyoudompheng: 3 posts Russ Cox: 2 posts Shivakumar Gn: 1 post

Content

People

Support

Translate

site design / logo © 2022 Grokbase