Bug 97569

Created attachment 165638 [details] Patch

Comment on attachment 165638 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=165638&action=review > Source/JavaScriptCore/assembler/MacroAssemblerCodeRef.h:39 > +#if CPU(ARM_THUMB2) && !ENABLE(COMPUTED_GOTO_OPCODES) Your comment in the ChangeLog suggests that this issue only manifests when LLINT_C_LOOP is enabled. I would be more comfortable if you change the above to the following instead: #if CPU(ARM_THUMB2) && !(ENABLE(LLINT_C_LOOP) && ENABLE(COMPUTED_GOTO_OPCODES)) This way, the assertion is not disabled for the more common use case where the llint C++ backend is not in use. > Source/JavaScriptCore/assembler/MacroAssemblerCodeRef.h:46 > +// when we enable features that use labels-as-values. I suggest "... use labels-as-values e.g. ENABLE(LLINT_C_LOOP) with ENABLE(COMPUTED_GOTO_OPCODES).

(In reply to comment #2) > Your comment in the ChangeLog suggests that this issue only manifests when LLINT_C_LOOP is enabled. I would be more comfortable if you change the above to the following instead: > > #if CPU(ARM_THUMB2) && !(ENABLE(LLINT_C_LOOP) && ENABLE(COMPUTED_GOTO_OPCODES)) While I noticed this while using LLINT_C_LOOP, I am not entirely sure that the failure only occurs in this case. I will look around for other possible failing scenarios (i.e. where else are labels-as-values being used). > > Source/JavaScriptCore/assembler/MacroAssemblerCodeRef.h:46 > > +// when we enable features that use labels-as-values. > > I suggest "... use labels-as-values e.g. ENABLE(LLINT_C_LOOP) with ENABLE(COMPUTED_GOTO_OPCODES). Ok, I will make the comment more complete. Thank you very much.

(In reply to comment #3) > (In reply to comment #2) > > Your comment in the ChangeLog suggests that this issue only manifests when LLINT_C_LOOP is enabled. I would be more comfortable if you change the above to the following instead: > > > > #if CPU(ARM_THUMB2) && !(ENABLE(LLINT_C_LOOP) && ENABLE(COMPUTED_GOTO_OPCODES)) > > While I noticed this while using LLINT_C_LOOP, I am not entirely sure that the failure only occurs in this case. I will look around for other possible failing scenarios (i.e. where else are labels-as-values being used). Here's an idea: Can you confirm if the assertion you saw only came from "static MacroAssemblerCodePtr createLLIntCodePtr(LLIntCode codeId)" calling createFromExecutableAddress()? If so, for testing purposes, you can hand inline createFromExecutableAddress(*) directly into createLLIntCodePtr() minus the assertion, and see if that makes your issues go away. If so, then that shows that this is a LLINT_C_LOOP only issue.

(In reply to comment #4) > Here's an idea: > Can you confirm if the assertion you saw only came from "static MacroAssemblerCodePtr createLLIntCodePtr(LLIntCode codeId)" calling createFromExecutableAddress()? If so, for testing purposes, you can hand inline createFromExecutableAddress(*) directly into createLLIntCodePtr() minus the assertion, and see if that makes your issues go away. If so, then that shows that this is a LLINT_C_LOOP only issue. I like your idea, although I cannot confirm that it only comes from that place. I have also found other unrelated places (specifically, in the DFG JIT) where this assertion fails, and right now I am looking into whether the failure is diagnosing a real problem. Incidentally, the build that's failing on me right now has CLoop disabled. If it eventually turns out that the failures I'm currently seeing come from elsewhere, then I will go on and implement your suggestion.

Created attachment 167385 [details] Patch Since createFromExecutableAddress takes pointers from LLInt::getCodePtr() only, I think this the best place to isolate disabling the check for decorated code pointers. (ASSERT_VALID_CODE_POINTER has also failed in other places on our machines, but we were able to resolve those failures without having to disable the decorated pointer check globally.)

Comment on attachment 167385 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=167385&action=review > Source/JavaScriptCore/assembler/MacroAssemblerCodeRef.h:291 > +#if CPU(ARM_THUMB2) && ENABLE(COMPUTED_GOTO_OPCODES) > + // ASSERT_VALID_CODE_POINTER is not applicable to opcodes coming from > + // addresses of labels, because they are not decorated on ARM Thumb. > + ASSERT(value); > +#else createFromExecutableAddress() is called from createLLIntCodePtr() amongst other places. createLLIntCodePtr() takes a LLIntCode which gets casted to the void* arg passed to createFromExecutableAddress(). And LLintCode is always an OpcodeId when ENABLE(LLINT_C_LOOP). Hence, LLIntCode can be an OpcodeId regardless of whether ENABLE(COMPUTED_GOTO_OPCODES) or not. Since, OpcodeIds are not guaranteed to have the low bit set, you'll never want to use ASSERT_VALID_CODE_POINTER() when building for CPU(ARM_THUMB2). Hence, you should omit the ENABLE(COMPUTED_GOTO_OPCODES) condition altogether. Please fix and upload another patch.

Thank you, Mark. Essentially, ASSERT_VALID_CODE_POINTER becomes a mere ASSERT. But since I filed this patch, many moons ago, I discovered other failures, similar in nature, but due to a different cause: thumb-interwork thunks. See the following backtrace snippet (collected a while ago on ARM Linux): #1 0x76adda10 in JSC::FunctionPtr::FunctionPtr<int, double> (this=0x7effadb4, value=0xc6c0 <JSC::toInt32(double)>) at ~/WebKit/Source/JavaScriptCore/assembler/MacroAssemblerCodeRef.h:105 #2 0x76ad9bec in JSC::DFG::SpeculativeJIT::callOperation (this=0x7effd798, operation=0xc6c0 <JSC::toInt32(double)>, result=JSC::ARMRegisters::r0, arg1=JSC::ARMRegisters::d0) at ~/WebKit/Source/JavaScriptCore/dfg/DFGSpeculativeJIT.h:1314 #3 0x76aca8b8 in JSC::DFG::SpeculativeJIT::compileValueToInt32 (this=0x7effd798, node=0x738e14f8) at ~/WebKit/Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:2311 This is happening because JSC::toInt32 is exported from libJavaScriptCore.so, and the exports are done via thunks. So removing the check for decorated code pointers inside createFromExecutableAddress() isn't sufficient. It also needs to be removed from the FunctionPtr constructor. What do you think about that?

(In reply to comment #8) > Thank you, Mark. Essentially, ASSERT_VALID_CODE_POINTER becomes a mere ASSERT. > > But since I filed this patch, many moons ago, I discovered other failures, similar in nature, but due to a different cause: thumb-interwork thunks. See the following backtrace snippet (collected a while ago on ARM Linux): > > #1 0x76adda10 in JSC::FunctionPtr::FunctionPtr<int, double> (this=0x7effadb4, value=0xc6c0 <JSC::toInt32(double)>) > at ~/WebKit/Source/JavaScriptCore/assembler/MacroAssemblerCodeRef.h:105 > #2 0x76ad9bec in JSC::DFG::SpeculativeJIT::callOperation (this=0x7effd798, operation=0xc6c0 <JSC::toInt32(double)>, > result=JSC::ARMRegisters::r0, arg1=JSC::ARMRegisters::d0) > at ~/WebKit/Source/JavaScriptCore/dfg/DFGSpeculativeJIT.h:1314 > #3 0x76aca8b8 in JSC::DFG::SpeculativeJIT::compileValueToInt32 (this=0x7effd798, node=0x738e14f8) > at ~/WebKit/Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:2311 > > This is happening because JSC::toInt32 is exported from libJavaScriptCore.so, and the exports are done via thunks. > > So removing the check for decorated code pointers inside createFromExecutableAddress() isn't sufficient. It also needs to be removed from the FunctionPtr constructor. What do you think about that? Cosmin, I believe you filed this bug to deal with an "assertion failure in non-JIT'ed LLInt on ARM Thumb". The stack trace you showed above is for a DFG build. If there's a DFG issue, please file a separate bug. Anyway, while I was investigating this issue, I found more build issues in the current CLoop LLINT build on the ARM Thumb build. It was just easier for me to go ahead and just fix it. I'll upload a patch for the complete fix shortly.

Created attachment 215848 [details] the patch.

Comment on attachment 215848 [details] the patch. r=me

Thanks for the review. Landed in r158541: <http://trac.webkit.org/r158541>. Cosmin, according to my testing, the non-JIT'ed LLINT (aka. the C Loop LLINT) now builds and runs as expected on ARM Thumb2. If you are still encountering issues with the C Loop LLINT with this fix, please feel free to reopen this bug or file another. If you are encountering JIT issues, please file a separate bug for that. Thanks.