Bug 176958 – Rationalize how WSL's operator&[] works

• Component: WebGPU
• Status: RESOLVED FIXED

[Filip Pizlo]

...

[Filip Pizlo]

There should be three of them, depending on how it's used:

operator[] for loads.

operator[]= for stores.

operator&[] for getting a pointer to an element.

[Filip Pizlo]

(In reply to Filip Pizlo from comment #1)
> There should be three of them, depending on how it's used:
> 
> operator[] for loads.
> 
> operator[]= for stores.
> 
> operator&[] for getting a pointer to an element.

Nope, we want operator&[] for all the things.

I made it work.

[Filip Pizlo]

Created attachment 320864 [details]
the patch

[Build Bot]

Attachment 320864 [details] did not pass style-queue:


ERROR: Tools/ChangeLog:31:  Please consider whether the use of security-sensitive phrasing could help someone exploit WebKit: dangling pointer  [changelog/unwantedsecurityterms] [3]
Total errors found: 1 in 19 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Filip Pizlo]

Created attachment 320958 [details]
the patch

Rebased.

[Build Bot]

Attachment 320958 [details] did not pass style-queue:


ERROR: Tools/ChangeLog:31:  Please consider whether the use of security-sensitive phrasing could help someone exploit WebKit: dangling pointer  [changelog/unwantedsecurityterms] [3]
Total errors found: 1 in 19 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Myles C. Maxfield]

Comment on attachment 320864 [details]
the patch

View in context: https://bugs.webkit.org/attachment.cgi?id=320864&action=review

> Tools/WebGPUShadingLanguageRI/Checker.js:176
> +        if (elementType instanceof ArrayRefType)
> +            throw new WTypeError(node.origin.originStrimg, "Operand to @ is an array reference: " + elementType);

I'm confused. You have a test below where you make an arrayref of an arrayref (and it results in just a single level of arrayref). unifyNode doesn't seem to solve this because ArrayRefType doesn't override unifyNode().

> Tools/WebGPUShadingLanguageRI/Checker.js:328
> +        if (node.name == "operator&[]") {

There isn't a better way to do this?

> Tools/WebGPUShadingLanguageRI/Checker.js:335
> +                node.argumentList[0].numElements = argType.numElements;

I would expect this to be an argument to the constructor.

> Tools/WebGPUShadingLanguageRI/Checker.js:338
> +                node.argumentList[0] = new MakePtrExpression(node.origin, node.argumentList[0]);

Doing this automatically is a little scary. What's the reason for this? Why not fail?

> Tools/WebGPUShadingLanguageRI/Checker.js:339
> +                argumentTypes[0] = new PtrType(node.origin, "thread", argumentTypes[0]);

Curious why you didn't choose to use symbols instead of the addressspace strings.

> Tools/WebGPUShadingLanguageRI/MakeArrayRefExpression.js:38
> +        this.__proto__ = ConvertPtrToArrayRefExpression.prototype;

😰 Isn't there a better way of doing this?

> Tools/WebGPUShadingLanguageRI/Test.js:2222
> +        thread int^ operator&[](thread Foo^ foo, uint index)

Why did we use ^ instead of *?

> Tools/WebGPUShadingLanguageRI/Test.js:2235
> +            return foo[0] + foo[1];

Adding them together doesn't actually check the body of operator&[]

> Tools/WebGPUShadingLanguageRI/Test.js:2261
> +            return foo.x + foo.y;

Ditto.

> Tools/WebGPUShadingLanguageRI/Test.js:2284
> +            return (^foo)[0] + (^foo)[1];

Ditto.

> Tools/WebGPUShadingLanguageRI/Test.js:2315
> +                return foo[0] + foo[1];

Ditto (though I guess it doesn't matter here)

[Myles C. Maxfield]

Comment on attachment 320958 [details]
the patch

See comments on previous patch.

[Filip Pizlo]

(In reply to Myles C. Maxfield from comment #7)
> Comment on attachment 320864 [details]
> the patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=320864&action=review
> 
> > Tools/WebGPUShadingLanguageRI/Checker.js:176
> > +        if (elementType instanceof ArrayRefType)
> > +            throw new WTypeError(node.origin.originStrimg, "Operand to @ is an array reference: " + elementType);
> 
> I'm confused. You have a test below where you make an arrayref of an
> arrayref (and it results in just a single level of arrayref). unifyNode
> doesn't seem to solve this because ArrayRefType doesn't override unifyNode().

If we try to make an ArrayRef from an ArrayRef, you get an error:

        if (elementType instanceof ArrayRefType)
            throw new WTypeError(node.origin.originStrimg, "Operand to @ is an array reference: " + elementType);

> 
> > Tools/WebGPUShadingLanguageRI/Checker.js:328
> > +        if (node.name == "operator&[]") {
> 
> There isn't a better way to do this?

The way I was imagining this is that it's really a SubscriptExpression, and once we know the types, we select exactly what kind of call it's going to be.

But this is equivalent.  It's not unusual for compilers to do == tests on the names of functions.

> 
> > Tools/WebGPUShadingLanguageRI/Checker.js:335
> > +                node.argumentList[0].numElements = argType.numElements;
> 
> I would expect this to be an argument to the constructor.

JS doesn't have constructor overloading.  Also, elsewhere it's the Checker's job to infer the numElements of a MakeArrayRefExpression.

> 
> > Tools/WebGPUShadingLanguageRI/Checker.js:338
> > +                node.argumentList[0] = new MakePtrExpression(node.origin, node.argumentList[0]);
> 
> Doing this automatically is a little scary. What's the reason for this? Why
> not fail?

The reason is to support [] and .field overloading for structs as well as arrays.

> 
> > Tools/WebGPUShadingLanguageRI/Checker.js:339
> > +                argumentTypes[0] = new PtrType(node.origin, "thread", argumentTypes[0]);
> 
> Curious why you didn't choose to use symbols instead of the addressspace
> strings.

I used symbols as enums once.  I found it annoying, so I stopped doing it.

To use symbols, you have to declare variables and then have them point to symbols and those symbols have to have names that reflect the variable names.  It's a lot of busy work.

> 
> > Tools/WebGPUShadingLanguageRI/MakeArrayRefExpression.js:38
> > +        this.__proto__ = ConvertPtrToArrayRefExpression.prototype;
> 
> 😰 Isn't there a better way of doing this?

The ability of an object to become another type is pretty fundamental to JS.

> 
> > Tools/WebGPUShadingLanguageRI/Test.js:2222
> > +        thread int^ operator&[](thread Foo^ foo, uint index)
> 
> Why did we use ^ instead of *?

We could still switch to *.  I'm not opposed to that.

I just thought that using ^ is a nice way to remind the programmer than this isn't really a pointer.

> 
> > Tools/WebGPUShadingLanguageRI/Test.js:2235
> > +            return foo[0] + foo[1];
> 
> Adding them together doesn't actually check the body of operator&[]

True.

> 
> > Tools/WebGPUShadingLanguageRI/Test.js:2261
> > +            return foo.x + foo.y;
> 
> Ditto.
> 
> > Tools/WebGPUShadingLanguageRI/Test.js:2284
> > +            return (^foo)[0] + (^foo)[1];
> 
> Ditto.
> 
> > Tools/WebGPUShadingLanguageRI/Test.js:2315
> > +                return foo[0] + foo[1];
> 
> Ditto (though I guess it doesn't matter here)

I'll add some test that checks the body of operator&[].

[Filip Pizlo]

Landed in https://trac.webkit.org/changeset/222116/webkit

[Radar WebKit Bug Importer]

<rdar://problem/34693268>

[Myles C. Maxfield]

Migrated to https://github.com/gpuweb/WHLSL/issues/139