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.
With --useJIT=false, I wonder why we're JITing code.
(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());
(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.
(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.
Created attachment 252993 [details] Patch
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.
(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 ?
Created attachment 253005 [details] Patch
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().
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.
Created attachment 253008 [details] Patch
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?
(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.
> 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.
(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.
Created attachment 253144 [details] Patch
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"
Committed r184354: <http://trac.webkit.org/changeset/184354>