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
Summary: We should have a way of preventing a caller from making a tail call and we sh...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Saam Barati
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2017-09-13 11:09 PDT by Saam Barati
Modified: 2017-09-27 12:59 PDT (History)
15 users (show)

See Also:


Attachments
patch (14.58 KB, patch)
2017-09-14 17:03 PDT, Saam Barati
keith_miller: review-
Details | Formatted Diff | Diff
patch (14.91 KB, patch)
2017-09-14 17:57 PDT, Saam Barati
no flags Details | Formatted Diff | Diff
patch (14.91 KB, patch)
2017-09-14 17:58 PDT, Saam Barati
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Saam Barati 2017-09-13 11:09:19 PDT
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();

    ...;
}
Comment 1 Keith Miller 2017-09-13 12:19:43 PDT
Yeah, it looks like at least clang respects the NoTailCalls class so this seems like a much better solution.
Comment 2 Yusuke Suzuki 2017-09-13 12:29:46 PDT
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
Comment 3 Filip Pizlo 2017-09-13 17:16:40 PDT
(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
Comment 4 Yusuke Suzuki 2017-09-14 03:18:53 PDT
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 :)
Comment 5 Yusuke Suzuki 2017-09-14 03:54:00 PDT
(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.
Comment 6 JF Bastien 2017-09-14 08:30:18 PDT
On clang you should use __attribute__((disable_tail_calls))
Comment 7 Saam Barati 2017-09-14 11:11:35 PDT
(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
Comment 8 Saam Barati 2017-09-14 17:03:20 PDT
Created attachment 320847 [details]
patch
Comment 9 Saam Barati 2017-09-14 17:04:28 PDT
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
Comment 10 Keith Miller 2017-09-14 17:28:16 PDT
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.
Comment 11 Keith Miller 2017-09-14 17:29:11 PDT
Comment on attachment 320847 [details]
patch

Nvm, it looks like this isn't working.
Comment 12 Saam Barati 2017-09-14 17:57:01 PDT
Created attachment 320858 [details]
patch
Comment 13 Saam Barati 2017-09-14 17:58:45 PDT
Created attachment 320859 [details]
patch
Comment 14 Keith Miller 2017-09-14 18:00:06 PDT
Comment on attachment 320859 [details]
patch

r=me if tests pass.
Comment 15 WebKit Commit Bot 2017-09-14 19:24:02 PDT
Comment on attachment 320859 [details]
patch

Clearing flags on attachment: 320859

Committed r222071: <http://trac.webkit.org/changeset/222071>
Comment 16 WebKit Commit Bot 2017-09-14 19:24:04 PDT
All reviewed patches have been landed.  Closing bug.
Comment 17 Radar WebKit Bug Importer 2017-09-27 12:59:46 PDT
<rdar://problem/34694446>