{"action":"created","issue":{"url":"https://api.github.com/repos/paul99/v8m-rb/issues/117","labels_url":"https://api.github.com/repos/paul99/v8m-rb/issues/117/labels{/name}","comments_url":"https://api.github.com/repos/paul99/v8m-rb/issues/117/comments","events_url":"https://api.github.com/repos/paul99/v8m-rb/issues/117/events","html_url":"https://github.com/paul99/v8m-rb/issues/117","id":54650994,"number":117,"title":"Improve the fix for MIPS: split api call stubs into accessor and function call stubs.","user":{"login":"paul99","id":131475,"avatar_url":"https://avatars.githubusercontent.com/u/131475?v=3","gravatar_id":"","url":"https://api.github.com/users/paul99","html_url":"https://github.com/paul99","followers_url":"https://api.github.com/users/paul99/followers","following_url":"https://api.github.com/users/paul99/following{/other_user}","gists_url":"https://api.github.com/users/paul99/gists{/gist_id}","starred_url":"https://api.github.com/users/paul99/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/paul99/subscriptions","organizations_url":"https://api.github.com/users/paul99/orgs","repos_url":"https://api.github.com/users/paul99/repos","events_url":"https://api.github.com/users/paul99/events{/privacy}","received_events_url":"https://api.github.com/users/paul99/received_events","type":"User","site_admin":false},"labels":[{"url":"https://api.github.com/repos/paul99/v8m-rb/labels/pri2","name":"pri2","color":"eb6420"}],"state":"open","locked":false,"assignee":{"login":"palfia","id":1380499,"avatar_url":"https://avatars.githubusercontent.com/u/1380499?v=3","gravatar_id":"","url":"https://api.github.com/users/palfia","html_url":"https://github.com/palfia","followers_url":"https://api.github.com/users/palfia/followers","following_url":"https://api.github.com/users/palfia/following{/other_user}","gists_url":"https://api.github.com/users/palfia/gists{/gist_id}","starred_url":"https://api.github.com/users/palfia/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/palfia/subscriptions","organizations_url":"https://api.github.com/users/palfia/orgs","repos_url":"https://api.github.com/users/palfia/repos","events_url":"https://api.github.com/users/palfia/events{/privacy}","received_events_url":"https://api.github.com/users/palfia/received_events","type":"User","site_admin":false},"milestone":null,"comments":1,"created_at":"2015-01-17T05:04:46Z","updated_at":"2015-01-19T12:07:56Z","closed_at":null,"body":"Ákos committed Balázs patch for 6950ead0b0a (The Dan Carney CL on accessors) https://codereview.chromium.org/853323002/\r\n\r\nThat had some bugs, and we figured out that it was this strange construct:\r\n\r\n    +    lw(s0, *stack_space_operand);\r\n\r\nwhere `*stack_space_operand` is actually a `MemOperand(sp, 4 * kPointerSize);` constructed earlier. [1]\r\n\r\nThe problem is that it is used _after_ the `DirectCFunctionCall()`, which pushes 4 mips argument slots onto the stack, but before those arg slots are popped off in `__ LeaveExitFrame().`\r\n\r\nOur quick fix is to add in the arg slots: `MemOperand(sp, 4 * kPointerSize + kCArgsSlotsSize);`\r\nSee https://codereview.chromium.org/855183003/\r\n\r\nBut that is spreading the ugliness to parts of the code that should not have to know.\r\n\r\nAt the very least we should add a detailed comment to our future selves who won't remember any of this. \r\n\r\nBut it would be better to factor this differently for mips32. I don't think we need to follow the arm port super closely here if we can make this cleaner.\r\n\r\nNote that mips64 does not have argument slots, and does not need this.\r\n\r\nI'll assign this to Ákos for now, since he and I have been thinking about this, but I'd be happy to hear feedback & ideas from @kilvadyb or @dusmil \r\n\r\n\r\n[1] With git transition, and the new \"Gitiles\" interface to commits, we don't seem to be able to send line# links to code, like we could in the old SVN system. I could do that on the GH v8 mirror, but these CL's have not landed there yet. So you get plain text pasted in. Sorry."},"comment":{"url":"https://api.github.com/repos/paul99/v8m-rb/issues/comments/70483614","html_url":"https://github.com/paul99/v8m-rb/issues/117#issuecomment-70483614","issue_url":"https://api.github.com/repos/paul99/v8m-rb/issues/117","id":70483614,"user":{"login":"kilvadyb","id":1742387,"avatar_url":"https://avatars.githubusercontent.com/u/1742387?v=3","gravatar_id":"","url":"https://api.github.com/users/kilvadyb","html_url":"https://github.com/kilvadyb","followers_url":"https://api.github.com/users/kilvadyb/followers","following_url":"https://api.github.com/users/kilvadyb/following{/other_user}","gists_url":"https://api.github.com/users/kilvadyb/gists{/gist_id}","starred_url":"https://api.github.com/users/kilvadyb/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/kilvadyb/subscriptions","organizations_url":"https://api.github.com/users/kilvadyb/orgs","repos_url":"https://api.github.com/users/kilvadyb/repos","events_url":"https://api.github.com/users/kilvadyb/events{/privacy}","received_events_url":"https://api.github.com/users/kilvadyb/received_events","type":"User","site_admin":false},"created_at":"2015-01-19T12:07:56Z","updated_at":"2015-01-19T12:07:56Z","body":"My proposal fix-improvement:\r\n```\r\ncommit 4465e4a3a422c612b61299d73d45a57a0f775faf\r\nAuthor: Balazs Kilvady <balazs.kilvady@imgtec.com>\r\nDate:   Mon Jan 19 13:03:05 2015 +0100\r\n\r\n    MIPS: Fix after 'Fix after 'MIPS: split api call stubs into accessor and function call stubs'.'.\r\n\r\ndiff --git a/src/mips/code-stubs-mips.cc b/src/mips/code-stubs-mips.cc\r\nindex 894747f..d0fcf34 100644\r\n--- a/src/mips/code-stubs-mips.cc\r\n+++ b/src/mips/code-stubs-mips.cc\r\n@@ -4940,15 +4940,13 @@ static void CallApiFunctionStubHelper(MacroAssembler* masm,\r\n   }\r\n   MemOperand return_value_operand(fp, return_value_offset * kPointerSize);\r\n   int stack_space = 0;\r\n-  MemOperand is_construct_call_operand =\r\n-      MemOperand(sp, 4 * kPointerSize + kCArgsSlotsSize);\r\n-  MemOperand* stack_space_operand = &is_construct_call_operand;\r\n+  int32_t stack_space_offset = 4 * kPointerSize;\r\n   if (argc.is_immediate()) {\r\n     stack_space = argc.immediate() + FCA::kArgsLength + 1;\r\n-    stack_space_operand = NULL;\r\n+    stack_space_offset = kInvalidStackOffset;\r\n   }\r\n   __ CallApiFunctionAndReturn(api_function_address, thunk_ref, stack_space,\r\n-                              stack_space_operand, return_value_operand,\r\n+                              stack_space_offset, return_value_operand,\r\n                               &context_restore_operand);\r\n }\r\n \r\n@@ -4997,7 +4995,7 @@ void CallApiGetterStub::Generate(MacroAssembler* masm) {\r\n   ExternalReference thunk_ref =\r\n       ExternalReference::invoke_accessor_getter_callback(isolate());\r\n   __ CallApiFunctionAndReturn(api_function_address, thunk_ref,\r\n-                              kStackUnwindSpace, NULL,\r\n+                              kStackUnwindSpace, kInvalidStackOffset,\r\n                               MemOperand(fp, 6 * kPointerSize), NULL);\r\n }\r\n \r\ndiff --git a/src/mips/code-stubs-mips.h b/src/mips/code-stubs-mips.h\r\nindex 9796fd4..699d335 100644\r\n--- a/src/mips/code-stubs-mips.h\r\n+++ b/src/mips/code-stubs-mips.h\r\n@@ -285,6 +285,8 @@ class DirectCEntryStub: public PlatformCodeStub {\r\n   explicit DirectCEntryStub(Isolate* isolate) : PlatformCodeStub(isolate) {}\r\n   void GenerateCall(MacroAssembler* masm, Register target);\r\n \r\n+  static const int kSlotSize = kCArgsSlotsSize;\r\n+\r\n  private:\r\n   bool NeedsImmovableCode() OVERRIDE { return true; }\r\n \r\ndiff --git a/src/mips/constants-mips.h b/src/mips/constants-mips.h\r\nindex 668481e..22c3e6c 100644\r\n--- a/src/mips/constants-mips.h\r\n+++ b/src/mips/constants-mips.h\r\n@@ -924,6 +924,7 @@ class Instruction {\r\n // C/C++ argument slots size.\r\n const int kCArgSlotCount = 4;\r\n const int kCArgsSlotsSize = kCArgSlotCount * Instruction::kInstrSize;\r\n+const int kInvalidStackOffset = -1;\r\n // JS argument slots size.\r\n const int kJSArgsSlotsSize = 0 * Instruction::kInstrSize;\r\n // Assembly builtins argument slots size.\r\ndiff --git a/src/mips/macro-assembler-mips.cc b/src/mips/macro-assembler-mips.cc\r\nindex cf42c13..a55c1d0 100644\r\n--- a/src/mips/macro-assembler-mips.cc\r\n+++ b/src/mips/macro-assembler-mips.cc\r\n@@ -4420,7 +4420,7 @@ static int AddressOffset(ExternalReference ref0, ExternalReference ref1) {\r\n \r\n void MacroAssembler::CallApiFunctionAndReturn(\r\n     Register function_address, ExternalReference thunk_ref, int stack_space,\r\n-    MemOperand* stack_space_operand, MemOperand return_value_operand,\r\n+    int32_t stack_space_offset, MemOperand return_value_operand,\r\n     MemOperand* context_restore_operand) {\r\n   ExternalReference next_address =\r\n       ExternalReference::handle_scope_next_address(isolate());\r\n@@ -4514,13 +4514,13 @@ void MacroAssembler::CallApiFunctionAndReturn(\r\n   if (restore_context) {\r\n     lw(cp, *context_restore_operand);\r\n   }\r\n-  if (stack_space_operand != NULL) {\r\n-    lw(s0, *stack_space_operand);\r\n+  if (stack_space_offset != kInvalidStackOffset) {\r\n+    lw(s0, MemOperand(sp, stack_space_offset + DirectCEntryStub::kSlotSize));\r\n   } else {\r\n     li(s0, Operand(stack_space));\r\n   }\r\n   LeaveExitFrame(false, s0, !restore_context, EMIT_RETURN,\r\n-                 stack_space_operand != NULL);\r\n+                 stack_space_offset != kInvalidStackOffset);\r\n \r\n   bind(&promote_scheduled_exception);\r\n   {\r\ndiff --git a/src/mips/macro-assembler-mips.h b/src/mips/macro-assembler-mips.h\r\nindex 1fb2aa2..7dbfc3f 100644\r\n--- a/src/mips/macro-assembler-mips.h\r\n+++ b/src/mips/macro-assembler-mips.h\r\n@@ -1291,7 +1291,7 @@ const Operand& rt = Operand(zero_reg), BranchDelaySlot bd = PROTECT\r\n   // the additional space allocated for the fast call).\r\n   void CallApiFunctionAndReturn(Register function_address,\r\n                                 ExternalReference thunk_ref, int stack_space,\r\n-                                MemOperand* stack_space_operand,\r\n+                                int32_t stack_space_offset,\r\n                                 MemOperand return_value_operand,\r\n                                 MemOperand* context_restore_operand);\r\n \r\n```"}}