Closed Bug 1081435 Opened 10 years ago Closed 9 years ago

Implement self-hosted CallNonGenericMethod

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla37

People

(Reporter: 446240525, Assigned: evilpie)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 2 obsolete files)

      No description provided.
Blocks: es6
Blocks: 1071668
No longer blocks: 1078975
Assignee: nobody → evilpies
Attached patch non-generic (obsolete) — Splinter Review
Turns out this code already exists, it was introduced in Bug 987560. However while working on this I found an issue with the argument count handling. I think because nargs != fun.length, stuff goes totally wrong during cloning and we need to specify the "wrong" arguments count.
Attachment #8527103 - Attachment is patch: true
Till, can you please look into why the arguments count has to include the default parameters to avoid asserting when cloning selfhosted functions in this case?
Flags: needinfo?(till)
(In reply to Tom Schuster [:evilpie] from comment #2)
> Till, can you please look into why the arguments count has to include the
> default parameters to avoid asserting when cloning selfhosted functions in
> this case?

The reason for that is that JSFunction::nargs() returns the number of all formal arguments, including ones with default values and ...rest args. In that, it's different from Function#length, which is backed by JSScript::funLength(). This is all slightly unfortunate, but for now look at it not as the wrong value, but as a different value than you expected. ;)
Flags: needinfo?(till)
Do you mean I should just use the wrong arguments length? This doesn't feel like an acceptable answer to me, really.
(In reply to Tom Schuster [:evilpie] from comment #4)
> Do you mean I should just use the wrong arguments length? This doesn't feel
> like an acceptable answer to me, really.

I guess depending on how you look at it you could either say that that's what I'm doing, or that we're talking about two different things.

One way to look at it is that we happen to have a function JSFunction::nargs() that returns a value that doesn't in all cases reflect what the ES spec would want us to return for Function#length. It does return something that's self-consistent and useful in some circumstances, though, and it happens to be the only thing we can use in this place without more work. If you want to fix that, I'd be delighted, but I don't think it needs to hold up landing something here.
Attached patch v2 (obsolete) — Splinter Review
Okay I think I was wrong, the arguments count if you do "newGlobal().Int8Array().find.length" is 1, even while we have 2 in the cpp file.
Attachment #8527103 - Attachment is obsolete: true
Attachment #8532121 - Flags: review?(jdemooij)
Attached patch non-genericSplinter Review
Attachment #8532121 - Attachment is obsolete: true
Attachment #8532121 - Flags: review?(jdemooij)
Attachment #8532122 - Flags: review?(jdemooij)
Comment on attachment 8532122 [details] [diff] [review]
non-generic

Review of attachment 8532122 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/tests/ecma_6/TypedArray/cross-compartment.js
@@ +6,5 @@
> +var BUGNUMBER = 1081435;
> +var summary = "Implement self-hosted CallNonGenericMethod";
> +print(BUGNUMBER + ": " + summary);
> +
> +var global = newGlobal();

This test will fail in the browser (no newGlobal). Either make it a jit-test or mark it as shell-only (shell-only jstests don't run on tinderbox atm so I prefer jit-tests).

::: js/src/vm/SelfHosting.cpp
@@ +844,5 @@
>      return true;
>  }
>  
> +MOZ_ALWAYS_INLINE bool
> +IsTypedArray(HandleValue v)

Nit: we can remove the MOZ_ALWAYS_INLINE here and below (pre-existing).

@@ +846,5 @@
>  
> +MOZ_ALWAYS_INLINE bool
> +IsTypedArray(HandleValue v)
> +{
> +    return v.isObject() && IsAnyTypedArray(&v.toObject());

Just to be sure, these self-hosted functions have to work with shared typed arrays?
Attachment #8532122 - Flags: review?(jdemooij) → review+
Thanks Jan! As far as I can tell shared typed arrays don't have find or findIndex.
(In reply to Tom Schuster [:evilpie] from comment #9)
> Thanks Jan! As far as I can tell shared typed arrays don't have find or
> findIndex.

OK, then you could use Is<TypedArrayObject> instead of IsTypedArray right?
As Till said on IRC we probably actually want this to support shared typed arrays. So we just have to add the functions to the shared typed array prototype later.
Somewhat likely the implementations of these methods (indeed all methods) on SharedTypedArray will be split from those on TypedArray, since we do not want there to be a chance that code that assumes it's operating on private memory is handed shared (racy) memory.  There is also not any particular reason why STA should have all the methods of TA, since the use cases for shared memory are often quite different from those of private memory.  (IMO the method suite on TA is overly large in any case.)
Okay, so Is<TypedArrayObject> it is.
https://hg.mozilla.org/mozilla-central/rev/27bbd05d18cc
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
You need to log in before you can comment on or make changes to this bug.