Bug 176863 – We should have a way of preventing a caller from making a tail call and we should use it for ProxyObject instead of using build flags

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Saam Barati]

I talked to both Geoff and Keith about this in the last couple of days, and it's a bit gross how we do this now. We should probably just having something like:
struct NoTailCalls {
    ~NoTailCalls() { m_bit = false; }
    volatile bool m_bit;
};

And then a macro like:

#define NO_TAIL_CALLS() NoTailCalls noTailCalls ## __counter__

And then inside your function, you can do:

static void foo()
{
    NO_TAIL_CALLS();

    ...;
}

[Keith Miller]

Yeah, it looks like at least clang respects the NoTailCalls class so this seems like a much better solution.

[Yusuke Suzuki]

I've ensured that this works with GCC.

#define NEVER_INLINE __attribute__((__noinline__))

struct NoTailCalls {
    ~NoTailCalls() { m_bit = false; }
    volatile bool m_bit;
};

NEVER_INLINE int target2()
{
    return 42;
}

NEVER_INLINE int target()
{
    NoTailCalls noTailCalls;
    return target2();
}

int main(int argc, char const* argv[])
{
    target();
    return 0;
}


================ target function with -O3

_Z6targetv:
.LFB4:
        .cfi_startproc
        subq    $16, %rsp
        .cfi_def_cfa_offset 24
        call    _Z7target2v
        movb    $0, 15(%rsp)
        addq    $16, %rsp
        .cfi_def_cfa_offset 8
        ret
        .cfi_endproc

Without NoTailCalls, it becomes

_Z6targetv:
.LFB4:
        .cfi_startproc
        jmp     _Z7target2v
        .cfi_endproc

[Filip Pizlo]

(In reply to Yusuke Suzuki from comment #2)
> I've ensured that this works with GCC.
> 
> #define NEVER_INLINE __attribute__((__noinline__))
> 
> struct NoTailCalls {
>     ~NoTailCalls() { m_bit = false; }

I bet you just need this to call compilerFence().

>     volatile bool m_bit;

Then you don't need this.

> };
> 
> NEVER_INLINE int target2()
> {
>     return 42;
> }
> 
> NEVER_INLINE int target()
> {
>     NoTailCalls noTailCalls;
>     return target2();
> }
> 
> int main(int argc, char const* argv[])
> {
>     target();
>     return 0;
> }
> 
> 
> ================ target function with -O3
> 
> _Z6targetv:
> .LFB4:
>         .cfi_startproc
>         subq    $16, %rsp
>         .cfi_def_cfa_offset 24
>         call    _Z7target2v
>         movb    $0, 15(%rsp)
>         addq    $16, %rsp
>         .cfi_def_cfa_offset 8
>         ret
>         .cfi_endproc
> 
> Without NoTailCalls, it becomes
> 
> _Z6targetv:
> .LFB4:
>         .cfi_startproc
>         jmp     _Z7target2v
>         .cfi_endproc

[Yusuke Suzuki]

Don and Alex, Could you test whether this compilerFence() / `volatile bool` NoTailCalls works on Windows (with MSVC)? If it is ok, NoTailCalls becomes platform-independent way to do this :)

[Yusuke Suzuki]

(In reply to Filip Pizlo from comment #3)
> (In reply to Yusuke Suzuki from comment #2)
> > I've ensured that this works with GCC.
> > 
> > #define NEVER_INLINE __attribute__((__noinline__))
> > 
> > struct NoTailCalls {
> >     ~NoTailCalls() { m_bit = false; }
> 
> I bet you just need this to call compilerFence().
> 
> >     volatile bool m_bit;
> 
> Then you don't need this.

Nice. I've just checked it on GCC in https://godbolt.org.

#define NEVER_INLINE __attribute__((__noinline__))

struct NoTailCalls {
    ~NoTailCalls() { asm volatile ("" ::: "memory"); }
};

NEVER_INLINE int target2()
{
    return 42;
}

NEVER_INLINE int target()
{
    NoTailCalls noTailCalls;
    return target2();
}

int main(int argc, char const* argv[])
{
    target();
    return 0;
}

==============

target2():
  mov eax, 42
  ret
target():
  call target2()
  ret
main:
  call target()
  xor eax, eax
  ret

Yeah, it seems good.

[JF Bastien]

On clang you should use __attribute__((disable_tail_calls))

[Saam Barati]

(In reply to JF Bastien from comment #6)
> On clang you should use __attribute__((disable_tail_calls))

That’s not what we want here. That disables a callee from being tail called. It doesn’t defend against a caller performing a tail call

[Saam Barati]

Created attachment 320847 [details]
patch

[Saam Barati]

Comment on attachment 320847 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=320847&action=review

> Source/WTF/wtf/NoTailCalls.h:55
> +#define NO_TAIL_CALLS() WTF::NoTailCalls _noTailCalls_

An alternate name could be:
DONT_TAIL_CALL

[Keith Miller]

Comment on attachment 320847 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=320847&action=review

r=me with nit.

> Source/WTF/wtf/NoTailCalls.h:42
> +// from within a given function. For example, if you don't want foo

nit: you could say scope instead of function. here.

>> Source/WTF/wtf/NoTailCalls.h:55
>> +#define NO_TAIL_CALLS() WTF::NoTailCalls _noTailCalls_
> 
> An alternate name could be:
> DONT_TAIL_CALL

I think NO_TAIL_CALLS makes more sense. DONT_TAIL_CALL implies to me that the function can't be tail called, which is the opposite of what this does.

[Keith Miller]

Comment on attachment 320847 [details]
patch

Nvm, it looks like this isn't working.

[Saam Barati]

Created attachment 320858 [details]
patch

[Saam Barati]

Created attachment 320859 [details]
patch

[Keith Miller]

Comment on attachment 320859 [details]
patch

r=me if tests pass.

[WebKit Commit Bot]

Comment on attachment 320859 [details]
patch

Clearing flags on attachment: 320859

Committed r222071: <http://trac.webkit.org/changeset/222071>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/34694446>