Closed Bug 819299 Opened 7 years ago Closed 7 years ago

IonMonkey: CreateThis doesn't return MagicValue(JS_IS_CONSTRUCTING)


(Core :: JavaScript Engine, defect)

Not set





(Reporter: h4writer, Assigned: h4writer)




(2 files, 1 obsolete file)

According to the comment above MCreateThis, this opcode should return MagicValue(JS_IS_CONSTRUCTING) for native functions. This isn't the case! There is no code in codegenerator for doing that!

So I've update MCreateThis to add this (this was the source why nightly crashed the 29th of November, because I depended on it)

While I was at it, I adjusted more things:
- Split CreateThis into CreateThis and CreateThisWithTemplate
-- CreateThis:
--- always uses the VMCall and needs Callee and Prototype.
--- return type is MIRType_Value (in order to return MagicValue)
--- gets optimized to MIRType_Object, when we are sure it doesn't need NativeCheck
-- CreateThisWithTemplate is our fastpath and only needs a templateObject
--- Don't need Callee and Prototype! Old code depended on both
Assignee: general → hv1989
Attachment #689672 - Flags: review?(dvander)
Blocks: 813773
Small fault in my code. defineBox isn't allowed for CallIncstructions
Attachment #689672 - Attachment is obsolete: true
Attachment #689672 - Flags: review?(dvander)
Attachment #689683 - Flags: review?(dvander)
FYI I'll push this testcase too:

function TestCase(n, d, e, a) {}
function reportCompare () {
  var testcase = new TestCase();
Comment on attachment 689683 [details] [diff] [review]
Add handling of MagicValue(JS_IS_CONSTRUCTING) to MCreateThis

Review of attachment 689683 [details] [diff] [review]:

::: js/src/ion/IonBuilder.cpp
@@ +3613,5 @@
>                  return createThis;
>          }
> +
> +        MDefinition *createThis = createThisScripted(callee);
> +        createThis->toCreateThis()->removeNativeCheck();

MCreateThis *createThis = ...

(if that assignment isn't valid, the toCreateThis could fail?)

::: js/src/ion/IonMacroAssembler.h
@@ +284,5 @@
>          Address address(fun, offsetof(JSFunction, nargs));
>          uint32_t bit = JSFunction::INTERPRETED << 16;
>          branchTest32(Assembler::Zero, address, Imm32(bit), label);
>      }
> +    void branchIfFunctionHasScript(Register fun, Label *label) {

nit: rename to "branchIfInterpreted" or something, since that's what you're really testing here. We might have lazy script generation at some point.
Attachment #689683 - Flags: review?(dvander) → review+
I had to add an extra line to handle JSObject* to js::Value return.
This is build on top of previous patch and just splits the lowering part into Value and Object version, to make sure the right amount of defines are given in the 32bit case
Attachment #690237 - Flags: review?(dvander)
Attachment #690237 - Flags: review?(dvander) → review+
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla20
You need to log in before you can comment on or make changes to this bug.