Bug 209432 – Add Clobberize validator for clobber top.

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Keith Miller]

We should have a validator that checks for places that DFGClobberize is blatantly wrong. The easiest of these is to check for VM reentry without: Read(World); Write(Heap);

[Radar WebKit Bug Importer]

<rdar://problem/60786138>

[Keith Miller]

Created attachment 394286 [details]
Patch

[Saam Barati]

Comment on attachment 394286 [details]
Patch

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

> Source/JavaScriptCore/ChangeLog:3
> +        Add Clobberize validator

I feel like this needs some kind of a qualifier on it. Maybe "basic Clobberize validator"

> Source/JavaScriptCore/dfg/DFGClobberize.h:137
> +        if constexpr (validateDFGClobberize)
> +            clobberTopFunctor();

let's make this a runtime option. We can default it to true when !!ASSERT_ENABLED, and can you also make some tests run with it on?

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:1846
> +        m_jit.store8(TrustedImm32(0), reinterpret_cast<char*>(&vm()) + OBJECT_OFFSETOF(VM, didEnterVM));

&vm.didEnterVM

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp:5313
> +            m_jit.load8(reinterpret_cast<char*>(&vm()) + OBJECT_OFFSETOF(VM, didEnterVM), scratch);

&vm().didEnterVM

> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:269
> +        m_out.store32As8(m_out.int32Zero, m_out.absolute(reinterpret_cast<char*>(vm) + OBJECT_OFFSETOF(VM, didEnterVM)));

&vm->didEnterVM

> Source/JavaScriptCore/interpreter/Interpreter.cpp:869
> +    auto clobberizeValidator = makeScopeExit([&] {
> +        if constexpr (validateDFGClobberize)
> +            vm.didEnterVM = true;
> +    });

do this for other entry points too?

> Source/JavaScriptCore/runtime/VM.h:292
> +constexpr bool validateDFGClobberize = ASSERT_ENABLED;

let's do a runtime option. I think it's fine to always set "didEnterVM" inside the runtime to true unconditionally, it's unlikely to cost any perf compared to vmEntry

[Saam Barati]

Comment on attachment 394286 [details]
Patch

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

> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:1631
> +                m_out.store(m_out.int32Zero, m_out.absolute(reinterpret_cast<char*>(&vm()) + OBJECT_OFFSETOF(VM, didEnterVM)));

I think you need to set this to zero on each basic block start point

[Keith Miller]

Created attachment 394525 [details]
Patch

[Saam Barati]

very red

[Keith Miller]

Created attachment 394536 [details]
Patch

[Jon Lee]

Ping; still in review!

[Yusuke Suzuki]

Comment on attachment 394536 [details]
Patch

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

r=me with comments.

> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:269
> +        m_out.store32As8(m_out.int32Zero, m_out.absolute(reinterpret_cast<char*>(vm) + OBJECT_OFFSETOF(VM, didEnterVM)));

Remove this when `!Options::validateDFGClobberize()`.

> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:514
> +

Remove this line.

> Source/JavaScriptCore/jit/JITCodeInlines.h:39
> +    vm->didEnterVM = true;

Use makeScopeExit?

> Source/JavaScriptCore/llint/LLIntThunks.h:45
> +    vm->didEnterVM = true;

Use makeScopeExit?

[Yusuke Suzuki]

Comment on attachment 394536 [details]
Patch

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

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:1863
> +            auto scratch = m_jit.scratchRegister();
> +            m_jit.load8(&vm().didEnterVM, scratch);
> +            auto ok = m_jit.branchTest32(MacroAssembler::Zero, scratch);

Why not using branchTest8() with AbsoluteAddress?

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:1906
> +                auto scratch = m_jit.scratchRegister();
> +                m_jit.load8(&vm().didEnterVM, scratch);
> +                auto ok = m_jit.branchTest32(MacroAssembler::Zero, scratch);

Why not using branchTest8 with AbsoluteAddress?

[Mark Lam]

Is there a reason we're not landing this and closing out the bug?

[Keith Miller]

Comment on attachment 394536 [details]
Patch

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

>> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:269
>> +        m_out.store32As8(m_out.int32Zero, m_out.absolute(reinterpret_cast<char*>(vm) + OBJECT_OFFSETOF(VM, didEnterVM)));
> 
> Remove this when `!Options::validateDFGClobberize()`.

Done.

>> Source/JavaScriptCore/jit/JITCodeInlines.h:39
>> +    vm->didEnterVM = true;
> 
> Use makeScopeExit?

I didn't because there's only one code path through here but I can change it if you prefer.

>> Source/JavaScriptCore/llint/LLIntThunks.h:45
>> +    vm->didEnterVM = true;
> 
> Use makeScopeExit?

Ditto.

[Keith Miller]

Created attachment 398525 [details]
Patch for landing

[EWS]

Committed r261181: <https://trac.webkit.org/changeset/261181>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 398525 [details].

[Keith Miller]

(In reply to Yusuke Suzuki from comment #10)
> Comment on attachment 394536 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=394536&action=review
> 
> > Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:1863
> > +            auto scratch = m_jit.scratchRegister();
> > +            m_jit.load8(&vm().didEnterVM, scratch);
> > +            auto ok = m_jit.branchTest32(MacroAssembler::Zero, scratch);
> 
> Why not using branchTest8() with AbsoluteAddress?
> 
> > Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:1906
> > +                auto scratch = m_jit.scratchRegister();
> > +                m_jit.load8(&vm().didEnterVM, scratch);
> > +                auto ok = m_jit.branchTest32(MacroAssembler::Zero, scratch);
> 
> Why not using branchTest8 with AbsoluteAddress?

Whoops missed this comment will add in follow up.