1081435 - Implement self-hosted CallNonGenericMethod

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Comment 1

[Tom S [:evilpie]]

Attachment: non-generic

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.

Comment 2

[Tom S [:evilpie]]

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?

Comment 3

[Till Schneidereit [: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. ;)

Comment 4

[Tom S [:evilpie]]

Do you mean I should just use the wrong arguments length? This doesn't feel like an acceptable answer to me, really.

Comment 5

[Till Schneidereit [:till]]

(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.

Comment 6

[Tom S [:evilpie]]

Attachment: v2

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.

Comment 7

[Tom S [:evilpie]]

Attachment: non-generic

Comment 8

[Jan de Mooij [:jandem]]

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?

Comment 9

[Tom S [:evilpie]]

Thanks Jan! As far as I can tell shared typed arrays don't have find or findIndex.

Comment 10

[Jan de Mooij [:jandem]]

(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?

Comment 11

[Tom S [:evilpie]]

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.

Comment 12

[Lars T Hansen [:lth]]

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.)

Comment 13

[Tom S [:evilpie]]

Okay, so Is<TypedArrayObject> it is.

Comment 14

[Tom S [:evilpie]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/27bbd05d18cc

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/27bbd05d18cc