Bug 177044 – WSL needs a way to verify that structs are not cyclic

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Filip Pizlo]

...

[Filip Pizlo]

Created attachment 321412 [details]
the patch

[Myles C. Maxfield]

Comment on attachment 321412 [details]
the patch

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

> Tools/WebGPUShadingLanguageRI/Checker.js:64
> +        let shaderType = node;

This doesn’t seem right. Should this be shaderType = node.shaderType;?

> Tools/WebGPUShadingLanguageRI/Checker.js:85
>          switch (node.shaderType) {

... and you can use it here?

> Tools/WebGPUShadingLanguageRI/RecursiveTypeChecker.js:44
> +        this._visiting.doVisit(node, () => super.visitStructType(node));

So linked lists will work, but struct Foo { Foo x; int y; } won’t.

> Tools/WebGPUShadingLanguageRI/Test.js:4436
> +        () => checkInt(program, callFunction(program, "foo", [], []), -511),

I thought callFunction() caught theTrapError?

> Tools/WebGPUShadingLanguageRI/Test.js:4463
> +}

Can we test linked lists?

> Tools/WebGPUShadingLanguageRI/Visitor.js:-98
> -        Node.visit(node.type, this);

Why not? Because this type would be visited in some other part of the tree?

[Filip Pizlo]

(In reply to Myles C. Maxfield from comment #2)
> Comment on attachment 321412 [details]
> the patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=321412&action=review
> 
> > Tools/WebGPUShadingLanguageRI/Checker.js:64
> > +        let shaderType = node;
> 
> This doesn’t seem right. Should this be shaderType = node.shaderType;?

This should say "let shaderFunc = node"

> 
> > Tools/WebGPUShadingLanguageRI/Checker.js:85
> >          switch (node.shaderType) {
> 
> ... and you can use it here?
> 
> > Tools/WebGPUShadingLanguageRI/RecursiveTypeChecker.js:44
> > +        this._visiting.doVisit(node, () => super.visitStructType(node));
> 
> So linked lists will work, but struct Foo { Foo x; int y; } won’t.

Yeah

> 
> > Tools/WebGPUShadingLanguageRI/Test.js:4436
> > +        () => checkInt(program, callFunction(program, "foo", [], []), -511),
> 
> I thought callFunction() caught theTrapError?

I will fix this.  We need two versions of callFunction():

- A low-level version that does not catch trap error.
- A high-level version that does catch trap error.

We need the low-level one to write tests.

> 
> > Tools/WebGPUShadingLanguageRI/Test.js:4463
> > +}
> 
> Can we test linked lists?

I can add that.

> 
> > Tools/WebGPUShadingLanguageRI/Visitor.js:-98
> > -        Node.visit(node.type, this);
> 
> Why not? Because this type would be visited in some other part of the tree?

If we visit it here, then we'll get a JS stack overflow error anytime we try to typecheck a program with a linked list.

[Filip Pizlo]

(In reply to Filip Pizlo from comment #3)
> (In reply to Myles C. Maxfield from comment #2)
> > Comment on attachment 321412 [details]
> > the patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=321412&action=review
> > 
> > > Tools/WebGPUShadingLanguageRI/Checker.js:64
> > > +        let shaderType = node;
> > 
> > This doesn’t seem right. Should this be shaderType = node.shaderType;?
> 
> This should say "let shaderFunc = node"
> 
> > 
> > > Tools/WebGPUShadingLanguageRI/Checker.js:85
> > >          switch (node.shaderType) {
> > 
> > ... and you can use it here?
> > 
> > > Tools/WebGPUShadingLanguageRI/RecursiveTypeChecker.js:44
> > > +        this._visiting.doVisit(node, () => super.visitStructType(node));
> > 
> > So linked lists will work, but struct Foo { Foo x; int y; } won’t.
> 
> Yeah
> 
> > 
> > > Tools/WebGPUShadingLanguageRI/Test.js:4436
> > > +        () => checkInt(program, callFunction(program, "foo", [], []), -511),
> > 
> > I thought callFunction() caught theTrapError?
> 
> I will fix this.  We need two versions of callFunction():
> 
> - A low-level version that does not catch trap error.
> - A high-level version that does catch trap error.
> 
> We need the low-level one to write tests.

Oh snap.  This "worked" because you added a TrapException instead of using WTrapError, and then you didn't convert all of the places that used WTrapError to use TrapException.

Please use WTrapError. :-)  I'll fix that part of your code, and refactor callFunction.

[Filip Pizlo]

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

[Radar WebKit Bug Importer]

<rdar://problem/34694297>

[Myles C. Maxfield]

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