xuxiandi / angleproject

Automatically exported from code.google.com/p/angleproject
Other
0 stars 0 forks source link

Incorrect 'for' loop body scope #254

Closed GoogleCodeExporter closed 9 years ago

GoogleCodeExporter commented 9 years ago
void main() {
  int k = 0;
  for (int i = 0; i < 10; i++) { int i = k+i; } // ok, compound nests
  gl_FragColor = vec4(float(k));
}

What is the expected output? What do you see instead?
This is legal. Angle claims i is redefined.

What version of the product are you using? On what operating system?
r887

Please provide any additional information below.
http://www.khronos.org/registry/gles/specs/2.0/GLSL_ES_Specification_1.0.17.pdf 
Section 4.2.2

Original issue reported on code.google.com by kosmo...@gmail.com on 23 Nov 2011 at 11:43

GoogleCodeExporter commented 9 years ago

Original comment by dan...@transgaming.com on 5 Mar 2012 at 3:45

GoogleCodeExporter commented 9 years ago

Original comment by nicolas....@gmail.com on 8 Mar 2012 at 9:28

GoogleCodeExporter commented 9 years ago
Just like issue 253, this seems to be invalid. The spec says that:

"If [a name] is declared in a while test or a for statement, then it is scoped 
to the end of the following statement-no-new-scope."

And the for loop grammar is specified as: FOR LEFT_PAREN for_init_statement 
for_rest_statement RIGHT_PAREN statement_no_new_scope

So the scope starts at the left parenthesis '(' and ends at the end of the 
loop's body statement.

Also note that GLSL tries to stick as close as possible to C syntax and the for 
loop in the above example fail to compile in C with a "redefinition of i" error.

Original comment by nicolas....@gmail.com on 10 Mar 2012 at 12:05

GoogleCodeExporter commented 9 years ago
The for loop grammar is the answer to your question.

FOR LEFT_PAREN for_init_statement for_rest_statement RIGHT_PAREN 
statement_no_new_scope

This implies that declarations in for_init_statements are scoped until the end 
of the statement_no_new_scope which does not introduce scope.

See the production for statement_no_new_scope:
statement_no_new_scope:
        compound_statement_with_scope
        simple_statement 

When authors use curly braces in for statements, they introduce another scope 
inside the for_init_statement scope.

Incidentally, I tried this:

gcc -Wall -std=c99 test.c -o

on this test.c:
int main() {
  int k = 0;
  for (int i = 0; i < 10; i++) { int i = k+i; }
  return 0;
}

and it compiled without issue.

Original comment by kosmo...@gmail.com on 10 Mar 2012 at 12:19

GoogleCodeExporter commented 9 years ago
Thanks for pointing that out. It seems quite inconsistent since that would mean 
the following:

for(int i = 0; i < 10; i++) {int i = i;}   // Ok, nested cope, counter 'i' 
assigned to body 'i'
for(int i = 0; i < 10; i++} int i = i;     // Error, redefinition of 'i'

It looks like the GLSL ES spec is ambiguous about the intended behavior. 
However, note that the desktop GLSL spec is very clear about it 
(http://www.opengl.org/registry/doc/GLSLangSpec.4.20.8.clean.pdf):

for (int i = 0; i < 10; i++) {
    int i;  // redeclaration error
}

Also note that its grammar of statement_no_new_scope is defined differently:

statement_no_new_scope :
    compound_statement_no_new_scope
    simple_statement

This seems more consistent to me so we'll have to check with the spec authors 
what's intended...

Original comment by nicolas....@gmail.com on 10 Mar 2012 at 1:46

GoogleCodeExporter commented 9 years ago
I do not see the inconsistency between

for(int i = 0; i < 10; i++) {int i = i;}   // Ok, nested cope, counter 'i' 
assigned to body 'i'
for(int i = 0; i < 10; i++) int i = i;     // Error, redefinition of 'i'

Specifically, a for loop is a binary operator on a(n implicit) state and a 
statement. The C and GLSL languages have constructions for compound statements 
which introduce scope. Why would the curly braces in a for-loop body not 
introduce scope (and be optional) but would introduce scope when standing 
alone? That is what seems inconsistent to me.

In the second loop, gcc will complain about parsing the statement following the 
loop due to no matching for-loop body production for naked variable 
declarations.

From this perspective, it appears that desktop GLSL is inconsistent with its 
spiritual heritage of C but ESSL is more true to C's loop body scope semantics.

Where is the OpenGL ES 2.0 GLSL public mailing list to which you will post this 
spec inquiry?

Original comment by kosmo...@gmail.com on 12 Mar 2012 at 12:51

GoogleCodeExporter commented 9 years ago
I see your logic, but I believe it can be argued both ways. Note for instance 
that for function definitions the scope starts after the function name and ends 
at the closing bracket:

void foo(int foo)   // OK, argument the same name as the function
{
    int foo;   // Error, redefinition of argument
}

So the brackets are inherently part of the function definition and are not just 
a compound statement with its own scope.

For loops could be considered a similar 'whole' instead of just for(...) 
followed by a statement. Unlike functions, the brackets are optional, but that 
seems more about code brevity than intentionally allowing a difference in scope.

It's also probably a good thing to get an error when using the same variable 
name as the loop counter, since more often than not it's a mistake and wouldn't 
lead to desired behavior when allowed.

On the other hand it seems that you're correct about C99 creating a new scope 
for the loop body. In fact, its spec even demands to create a new scope when 
the loop body is not a compound statement! So it behaves like this instead:

for(int i = 0; i < 10; i++) {int i = i;}   // OK
for(int i = 0; i < 10; i++) int i = i;     // OK too

Anyway, it's not up to me to decide which of these three possible scoping rules 
should be implemented. The spec is what it is. I've asked for clarification on 
https://cvs.khronos.org/bugzilla/show_bug.cgi?id=8656 and I'm awaiting a clear 
answer. Personally I don't think it's desirable to diverge from desktop GLSL.

Thanks again for your input!

Original comment by nicolas....@gmail.com on 12 Mar 2012 at 4:51

GoogleCodeExporter commented 9 years ago
> I see your logic, but I believe it can be argued both ways. Note for instance 
that for function definitions the
> scope starts after the function name and ends at the closing bracket:

> void foo(int foo)   // OK, argument the same name as the function
> {
>     int foo;   // Error, redefinition of argument
> }

The context in which function declarations are used is external_declaration not 
statement. Additionally, Section 4.2.2 says:
"A function body has a scope nested inside the function’s definition."

and so the curly braces (required) in a function declaration do indeed start a 
new scope inside of the function parameters' scope.

C99's behavior seems quite logical. ESSL has already diverged significantly 
from desktop GLSL and both ESSL and GLSL specs clearly contain errors, 
omissions, and mistakes.

I'm glad that this conversation can continue behind a paywall where no vendors 
can have their egos bruised for writing broken compilers. I guess the whole 
World Wide Web will have to suffer because Khronos can't figure out formal 
semantics. :-(

Original comment by kosmo...@gmail.com on 12 Mar 2012 at 7:27

GoogleCodeExporter commented 9 years ago
Both the GLSL and ESSL spec authors agree that the case of a FOR statement, 
that the scope starts after the "for" and that the braces after the loop 
condition do not add an additional scope.

    for (int k = ...) // k is in a nested new scope
    { // not a nested scope, in k's scope, nesting already happened

    for (int k = ...) // k is in a nested new scope
        ; // definitely not another nesting

Hopefully there will be a spec clarification about this.

For the record, GLSL tries to more closely match C++ rules rather than C rules.
Visual Studio (2008 and 2010) also claims that 'i' is redefined in the earlier 
examples.

I believe this matches what we currently have implemented, so closing as 
WontFix.

Original comment by dan...@transgaming.com on 21 Mar 2012 at 8:09