Bug 193437 – [WHLSL] Implement the return checker and the unreachable code checker

• Status: RESOLVED
• Resolution: DUPLICATE
• Component: New Bugs

[Myles C. Maxfield]

[WHLSL] Implement the return checker

[Myles C. Maxfield]

Created attachment 359140 [details]
Patch

[Myles C. Maxfield]

Created attachment 359142 [details]
Patch

[Robin Morisset]

Comment on attachment 359142 [details]
Patch

Why do we want this pass?
There is nothing in the spec about requiring functions to return, and it is undecidable in general. And indeed isBoolCastFromTrue treats true differently from 2+2==4.
I am strongly opposed to keeping this pass, especially as I don't think it buys us anything (is there a difference for us between a function that never terminates and one that returns in a trillion years?)

The unreachable code checker is less unreasonable, but because of its design does not match at all what the spec says (for example it correctly catches {return 42; do_stuff();}, but it misses {break; do_stuff();}.
Please also skip this pass for now, it is not necessary to having a testable compiler, and I will try to hack something based on the spec in its stead.

[Myles C. Maxfield]

(In reply to Robin Morisset from comment #3)
> Comment on attachment 359142 [details]
> Patch
> 
> Why do we want this pass?
> There is nothing in the spec about requiring functions to return, and it is
> undecidable in general. And indeed isBoolCastFromTrue treats true
> differently from 2+2==4.
> I am strongly opposed to keeping this pass, especially as I don't think it
> buys us anything (is there a difference for us between a function that never
> terminates and one that returns in a trillion years?)
> 
> The unreachable code checker is less unreasonable, but because of its design
> does not match at all what the spec says (for example it correctly catches
> {return 42; do_stuff();}, but it misses {break; do_stuff();}.
> Please also skip this pass for now, it is not necessary to having a testable
> compiler, and I will try to hack something based on the spec in its stead.

A function that is supposed to return an int but has no return statements would break the type system, wouldn’t it?

[Robin Morisset]

(In reply to Myles C. Maxfield from comment #4)
> (In reply to Robin Morisset from comment #3)
> > Comment on attachment 359142 [details]
> > Patch
> > 
> > Why do we want this pass?
> > There is nothing in the spec about requiring functions to return, and it is
> > undecidable in general. And indeed isBoolCastFromTrue treats true
> > differently from 2+2==4.
> > I am strongly opposed to keeping this pass, especially as I don't think it
> > buys us anything (is there a difference for us between a function that never
> > terminates and one that returns in a trillion years?)
> > 
> > The unreachable code checker is less unreasonable, but because of its design
> > does not match at all what the spec says (for example it correctly catches
> > {return 42; do_stuff();}, but it misses {break; do_stuff();}.
> > Please also skip this pass for now, it is not necessary to having a testable
> > compiler, and I will try to hack something based on the spec in its stead.
> 
> A function that is supposed to return an int but has no return statements
> would break the type system, wouldn’t it?

More precisely: A non-void function that can reach the last statement of its body without hitting a return statement of the right type would break the type system.
Same for a function hitting a return statement of the wrong type.
A function that loops forever on the other hand is perfectly safe (in our type system), even if we claim it returns int.

This pass is not testing the type of returns (I guess it is verified in some part of the type checker I have not yet reviewed), and it tries to verify a lot more than just "no reaching the end of the function body without having returned", including preventing (some) infinite loops.

I won't prevent you from landing it if you really need it to get the compiler running, but I am uncomfortable with having these useless checks that do not match anything in the spec.

[Myles C. Maxfield]

*** This bug has been marked as a duplicate of bug 193487 ***