Bug 144921 – Enforce options coherency

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Basile Clement]

Created attachment 252975 [details]
Failing test

Running the following command segfaults on the system jsc:

$ jsc --useLLInt=false --useJIT=false native_call.js
OK
OK
Segmentation fault: 11

With a ToT debug build I get:

$ DYLD_FRAMEWORK_PATH=WebKitBuild/Debug/ WebKitBuild/Debug/jsc --useLLInt=false --useJIT=false native_call.js 
OK
ASSERTION FAILED: offset == static_cast<int32_t>(offset)
/Volumes/Data/secondary/OpenSource/Source/JavaScriptCore/assembler/X86Assembler.h(2327) : static void JSC::X86Assembler::setRel32(void *, void *)
1   0x10ad9d7e0 WTFCrash
2   0x10a3b9159 JSC::X86Assembler::setRel32(void*, void*)
3   0x10a98c0fd JSC::X86Assembler::relinkCall(void*, void*)
4   0x10a98c0d8 JSC::AbstractMacroAssembler<JSC::X86Assembler, JSC::MacroAssemblerX86Common>::repatchNearCall(JSC::CodeLocationNearCall, JSC::CodeLocationLabel)
5   0x10acb2861 JSC::RepatchBuffer::relink(JSC::CodeLocationNearCall, JSC::MacroAssemblerCodePtr)
6   0x10acaa408 JSC::linkFor(JSC::ExecState*, JSC::CallLinkInfo&, JSC::CodeBlock*, JSC::JSFunction*, JSC::MacroAssemblerCodePtr, JSC::CodeSpecializationKind, JSC::RegisterPreservationMode)
7   0x10a9aa5ad linkFor
8   0x10a9a4cb6 operationLinkCall
9   0x2ec507e0103c
10  0x2ec507e01b5e
11  0x2ec507e016b4
12  0x10ab31a49 vmEntryToJavaScript
13  0x10a993c6a JSC::JITCode::execute(JSC::VM*, JSC::ProtoCallFrame*)
14  0x10a9773a1 JSC::Interpreter::execute(JSC::ProgramExecutable*, JSC::ExecState*, JSC::JSObject*)
15  0x10a4a03c0 JSC::evaluate(JSC::ExecState*, JSC::SourceCode const&, JSC::JSValue, JSC::JSValue*)
16  0x10a30fa24 runWithScripts(GlobalObject*, WTF::Vector<Script, 0ul, WTF::CrashOnOverflow, 16ul> const&, bool)
17  0x10a30efe6 jscmain(int, char**)
18  0x10a30eb01 main
19  0x7fff904435c9 start
Segmentation fault: 11

The attachment uses print() but any native function seem to work as well.

[Geoffrey Garen]

With --useJIT=false, I wonder why we're JITing code.

[Basile Clement]

(In reply to comment #1)
> With --useJIT=false, I wonder why we're JITing code.

Indeed...
Looks like the culprit is around line 340 in runtime/Executable.cpp:

339     if (Options::useLLInt())
340         setupLLInt(vm, codeBlock.get());
341     else
342         setupJIT(vm, codeBlock.get());

[Michael Saboff]

(In reply to comment #2)
> (In reply to comment #1)
> > With --useJIT=false, I wonder why we're JITing code.
> 
> Indeed...
> Looks like the culprit is around line 340 in runtime/Executable.cpp:
> 
> 339     if (Options::useLLInt())
> 340         setupLLInt(vm, codeBlock.get());
> 341     else
> 342         setupJIT(vm, codeBlock.get());

If both useJIT and useLLInt are false, we don't have a path to execute JS.  We should add an early check that at least one of these options is true and fail if not.

[Basile Clement]

(In reply to comment #3)
> (In reply to comment #2)
> > (In reply to comment #1)
> > > With --useJIT=false, I wonder why we're JITing code.
> > 
> > Indeed...
> > Looks like the culprit is around line 340 in runtime/Executable.cpp:
> > 
> > 339     if (Options::useLLInt())
> > 340         setupLLInt(vm, codeBlock.get());
> > 341     else
> > 342         setupJIT(vm, codeBlock.get());
> 
> If both useJIT and useLLInt are false, we don't have a path to execute JS. 
> We should add an early check that at least one of these options is true and
> fail if not.

OK that makes sense ; I was not sure it was the case. I will put up a patch for failing early when both of those are false.

[Basile Clement]

Created attachment 252993 [details]
Patch

[Mark Lam]

Comment on attachment 252993 [details]
Patch

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

> Source/JavaScriptCore/runtime/Options.cpp:443
> +bool Options::optionsAreCoherent(FILE* stream)

I think you should enforce these checks on all uses of JavaScriptCore, not just the esc executable.

How about converting this function to "void Options::ensureOptionsAreCoherent()" and calling it at the end of Options::initialize()?  You can print the error messages using dataLog(), and make it crash if the options are not coherent.

While it’s nice to be able to yield the decision of how to handle the error to the client app (e.g. jsc), I think that it may not be worth the effort for now.

Please also update the ChangeLog to reflect the change accordingly.

[Basile Clement]

(In reply to comment #6)
> Comment on attachment 252993 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=252993&action=review
> 
> > Source/JavaScriptCore/runtime/Options.cpp:443
> > +bool Options::optionsAreCoherent(FILE* stream)
> 
> I think you should enforce these checks on all uses of JavaScriptCore, not
> just the esc executable.
> 
> How about converting this function to "void
> Options::ensureOptionsAreCoherent()" and calling it at the end of
> Options::initialize()?  You can print the error messages using dataLog(),
> and make it crash if the options are not coherent.
> 
> While it’s nice to be able to yield the decision of how to handle the error
> to the client app (e.g. jsc), I think that it may not be worth the effort
> for now.
> 
> Please also update the ChangeLog to reflect the change accordingly.

Correct me if I am wrong, but it seems that Options::initialize() is called first and only takes care of environment variables, while command line arguments are parsed later in jscmain.

Wouldn't your proposition then only handle

  JSC_useLLInt=false JSC_useJIT=false jsc script.js

while still having undefined behavior on

  jsc --useLLInt=false --useJIT=false script.js

?

[Basile Clement]

Created attachment 253005 [details]
Patch

[Mark Lam]

Comment on attachment 253005 [details]
Patch

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

> Source/JavaScriptCore/ChangeLog:14
> +        (such as the jsc executable) should expicitely ask JSC::Options::initialize()

s/should expicitely/may/.  I don't think we need to make this a requirement.

> Source/JavaScriptCore/ChangeLog:17
> +        not to perform the option coherency check. They are then responsible to
> +        check for coherency once all additional options have been set, and
> +        before starting to execute JavaScript. This is also implemented for the

s/not to/to not/.
s/option/optional/.
s/They are responsible to check for coherency once all additional options have been set, and before starting/The client is responsible to check for coherency, if any additional options have been set, before starting/

> Source/JavaScriptCore/runtime/InitializeThreading.cpp:52
> +void initializeThreading(bool needsCoherentOptions)

Let's call the arg "needsCoherentOptionsCheck" instead.

> Source/JavaScriptCore/runtime/Options.cpp:454
> +bool Options::optionsAreCoherent(FILE* stream)
> +{
> +    bool coherent = true;
> +    if (!(useLLInt() || useJIT())) {
> +        coherent = false;
> +        fprintf(stream, "INCOHERENT OPTIONS: at least one of useLLInt or useJIT must be true\n");
> +    }
> +    return coherent;
> +}

I still think you should use dataLog() here instead of fprintf().  This because JSC is sometimes used with clients where stdout is silenced.  Using WTF::dataLog() provides a mechanism where we can redirect the message to a file without changing the client code.  This is the common practice we employ throughout JSC code.

As a result, you can implement this function as "void ensureOptionsAreCoherent()" and have the call to CRASH() in here instead of in initialize().

[Basile Clement]

Comment on attachment 253005 [details]
Patch

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

>> Source/JavaScriptCore/runtime/InitializeThreading.cpp:52
>> +void initializeThreading(bool needsCoherentOptions)
> 
> Let's call the arg "needsCoherentOptionsCheck" instead.

OK.

>> Source/JavaScriptCore/runtime/Options.cpp:454
>> +}
> 
> I still think you should use dataLog() here instead of fprintf().  This because JSC is sometimes used with clients where stdout is silenced.  Using WTF::dataLog() provides a mechanism where we can redirect the message to a file without changing the client code.  This is the common practice we employ throughout JSC code.
> 
> As a result, you can implement this function as "void ensureOptionsAreCoherent()" and have the call to CRASH() in here instead of in initialize().

OK, I was not aware of that. Done.

[Basile Clement]

Created attachment 253008 [details]
Patch

[Geoffrey Garen]

Comment on attachment 253008 [details]
Patch

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

> Source/JavaScriptCore/jsc.cpp:1218
> +    JSC::initializeThreading(false);

We usually avoid boolean parameters like this because it's not possible to tell at the call site what the parameter means. The two ways to fix this are (1) use an enum instead of a bool (2) put the bool in a local variable that matches the name of the parameter.

That said, is it really worth all this complexity to avoid the startup coherency check? Even though jsc can set additional options on the command line, what's the harm in checking that the initial set of options passed through environment variables is coherent?

[Basile Clement]

(In reply to comment #12)
> Comment on attachment 253008 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=253008&action=review
> 
> > Source/JavaScriptCore/jsc.cpp:1218
> > +    JSC::initializeThreading(false);
> 
> We usually avoid boolean parameters like this because it's not possible to
> tell at the call site what the parameter means. The two ways to fix this are
> (1) use an enum instead of a bool (2) put the bool in a local variable that
> matches the name of the parameter.

I was thinking of using option (2), which would avoid cluttering initializeThreading completely, I can change that.

> 
> That said, is it really worth all this complexity to avoid the startup
> coherency check? Even though jsc can set additional options on the command
> line, what's the harm in checking that the initial set of options passed
> through environment variables is coherent?

The reason is that I wanted to avoid confusing error messages in a situation such as:

 $ export JSC_useLLInt=false
 $ ...
 $ JSC_useJIT=false jsc --useLLInt=true
 INCOHERENT OPTIONS: at least one of useLLInt or useJIT must be true

If you think that is not worth it, we can just have an unconditional call to ensureOptionsAreCoherent() in Options::initialize() and another one by the client app after changing the options.

[Geoffrey Garen]

> The reason is that I wanted to avoid confusing error messages in a situation
> such as:
> 
>  $ export JSC_useLLInt=false
>  $ ...
>  $ JSC_useJIT=false jsc --useLLInt=true
>  INCOHERENT OPTIONS: at least one of useLLInt or useJIT must be true
> 
> If you think that is not worth it, we can just have an unconditional call to
> ensureOptionsAreCoherent() in Options::initialize() and another one by the
> client app after changing the options.

I see your point here. Still, on balance, I think it's not worth it to worry about this case.

[Basile Clement]

(In reply to comment #14)
> > The reason is that I wanted to avoid confusing error messages in a situation
> > such as:
> > 
> >  $ export JSC_useLLInt=false
> >  $ ...
> >  $ JSC_useJIT=false jsc --useLLInt=true
> >  INCOHERENT OPTIONS: at least one of useLLInt or useJIT must be true
> > 
> > If you think that is not worth it, we can just have an unconditional call to
> > ensureOptionsAreCoherent() in Options::initialize() and another one by the
> > client app after changing the options.
> 
> I see your point here. Still, on balance, I think it's not worth it to worry
> about this case.

OK, simplifying this then.

[Basile Clement]

Created attachment 253144 [details]
Patch

[Mark Lam]

Comment on attachment 253144 [details]
Patch

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

r=me

> Source/JavaScriptCore/ChangeLog:15
> +        Client applications able to add or change additional options  are

extra space between “options  are"

[Basile Clement]

Committed r184354: <http://trac.webkit.org/changeset/184354>