Redeclaring a function name does not produce an error

[GoogleCodeExporter]

What steps will reproduce the problem?

compile the following shader:
---8<---
void f(int);
struct f {int x;};
void main() {}
---8<---

What is the expected output? What do you see instead?

The ESSL spec suggests an error along the lines of "Error: type 'f' conflicts 
with function 'f'", but ANGLE gladly outputs a HLSL-shader that redefine _f.

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

SVN r1289, Windows 7.

Please provide any additional information below.

The OpenGL ES Shading Language, section 4.2.7 explicitly lists the two first 
lines of the provided code as one of the combinations that are disallowed.

Original issue reported on code.google.com by kusmab...@gmail.com on 9 Oct 2012 at 2:28

[GoogleCodeExporter]

Something along these lines seems to fix the problem for me:

Index: D:/Users/Erik/src/angleproject/src/compiler/SymbolTable.h
===================================================================
--- D:/Users/Erik/src/angleproject/src/compiler/SymbolTable.h   (revision 1289)
+++ D:/Users/Erik/src/angleproject/src/compiler/SymbolTable.h   (working copy)
@@ -204,6 +204,21 @@
         // returning true means symbol was added to the table
         //
         tInsertResult result;
+        if (symbol.isFunction()) {
+            tLevel::iterator prev = level.find(symbol.getName());
+
+            //
+            // check for previous non-function declaration under the same name,
+            // and make sure we prevent future such declarations for entering
+            // the table
+            //
+
+            if (prev != level.end()) {
+                if (!prev->second->isFunction())
+                    return false;
+            } else
+                result = level.insert(tLevelPair(symbol.getName(), &symbol));
+        }
         result = level.insert(tLevelPair(symbol.getMangledName(), &symbol));

         return result.second;

Original comment by kusmab...@gmail.com on 9 Oct 2012 at 3:04

[GoogleCodeExporter]

Thanks for reporting this and providing a patch. Does it catch both the cases 
in which the function is declared first, and when the structure is declared 
first?

Original comment by nicolas....@gmail.com on 9 Oct 2012 at 3:24

• Changed state: Accepted

[GoogleCodeExporter]

Note that when providing patches, please make sure you have filled out the CLA 
and followed the instructions on the wiki: 
http://code.google.com/p/angleproject/wiki/ContributingCode.

Original comment by dan...@transgaming.com on 9 Oct 2012 at 3:31

[GoogleCodeExporter]

I thought it did, but it does not. It seems the call to insert is not checked 
on the call-site, so the rest of the code expects it to pass.

The logic should probably be lifted up to the function_prototype rule in the 
glslang.y instead.

Original comment by kusmab...@gmail.com on 9 Oct 2012 at 3:41

[GoogleCodeExporter]

[deleted comment]

[GoogleCodeExporter]

And here's a patch to do just that:

Index: src/compiler/glslang.y
===================================================================
--- src/compiler/glslang.y  (revision 1289)
+++ src/compiler/glslang.y  (working copy)
@@ -1017,7 +1017,22 @@
             }
         }

+        //
+        // Check for previous non-function declaration under the same name,
+        // and make sure we prevent future such declarations for entering
+        // the table
         //
+        TSymbol* prevSym = context->symbolTable.find($1->getName());
+        if (prevSym) {
+            if (!prevSym->isFunction()) {
+                context->error($2.line, "redefinition", $1.string->c_str(), 
"function");
+                context->recover();
+            }
+        } else
+          context->symbolTable.getOuterLevel()->insert($1->getName(), $1);
+
+
+        //
         // If this is a redeclaration, it could also be a definition,
         // in which case, we want to use the variable names from this one, and not the one that's
         // being redeclared.  So, pass back up this declaration, not the one in the symbol table.
Index: src/compiler/glslang_tab.cpp
===================================================================
--- src/compiler/glslang_tab.cpp    (revision 1289)
+++ src/compiler/glslang_tab.cpp    (working copy)
@@ -3116,6 +3116,20 @@
         }

         //
+        // Check for previous non-function declaration under the same name,
+        // and make sure we prevent future such declarations for entering
+        // the table
+        //
+        TSymbol* prevSym = context->symbolTable.find((yyvsp[(1) - 
(2)].interm.function)->getName());
+        if (prevSym) {
+            if (!prevSym->isFunction()) {
+                context->error((yyvsp[(2) - (2)].lex).line, "redefinition", 
(yyvsp[(2) - (2)].lex).string->c_str(), "function");
+                context->recover();
+            }
+        } else
+          context->symbolTable.getOuterLevel()->insert((yyvsp[(1) - 
(2)].interm.function)->getName(), *(yyval.interm).function);
+
+        //
         // If this is a redeclaration, it could also be a definition,
         // in which case, we want to use the variable names from this one, and not the one that's
         // being redeclared.  So, pass back up this declaration, not the one in the symbol table.
Index: src/compiler/SymbolTable.h
===================================================================
--- src/compiler/SymbolTable.h  (revision 1289)
+++ src/compiler/SymbolTable.h  (working copy)
@@ -198,17 +198,22 @@
     TSymbolTableLevel() { }
     ~TSymbolTableLevel();

-    bool insert(TSymbol& symbol) 
+    bool insert(const TString& name, TSymbol& symbol) 
     {
         //
         // returning true means symbol was added to the table
         //
         tInsertResult result;
-        result = level.insert(tLevelPair(symbol.getMangledName(), &symbol));
+        result = level.insert(tLevelPair(name, &symbol));

         return result.second;
     }

+    bool insert(TSymbol& symbol) 
+    {
+        return insert(symbol.getMangledName(), symbol);
+    }
+
     TSymbol* find(const TString& name) const
     {
         tLevel::const_iterator it = level.find(name);

Original comment by kusmab...@gmail.com on 9 Oct 2012 at 3:52

[GoogleCodeExporter]

daniel: I did sign the CLA, but the whole gcl thingie seems just... uhm, like 
it's going to take a lot more time than the whole process of nailing down and 
fixing the issue did (or "hacking around it", if you will).

Would it be possible for someone with gcl-fu to pick up the patch? You should 
have the legal rights now that I've signed the CLA. If so, feel free to take 
the credit as well; I just stumbled over this and thought "well, maybe it's a 
quick fix" :P

Original comment by kusmab...@gmail.com on 9 Oct 2012 at 4:02

[GoogleCodeExporter]

That's fine. I'll test and integrate the patch when I get the chance. Thanks!

Original comment by nicolas....@gmail.com on 9 Oct 2012 at 4:08

[GoogleCodeExporter]

That's fine if you don't want to use gcl (I don't either FWIW..) but I do need 
you to do step 3.2 (i.e. email me the required info please).  I have no other 
way of knowing if people signed the CLA.  Thanks.

Original comment by dan...@transgaming.com on 9 Oct 2012 at 4:27

[GoogleCodeExporter]

nm.. there it is! Thanks for tracking down the fix as well.

Original comment by dan...@transgaming.com on 9 Oct 2012 at 4:28

[GoogleCodeExporter]

Original comment by nicolas....@gmail.com on 18 Oct 2012 at 1:21

• Changed state: Started

[GoogleCodeExporter]

I gave it a try but unfortunately that patch causes lots of regressions. The 
problem is that it also marks overloaded functions as invalid reuse of the same 
name.

Original comment by nicolas....@gmail.com on 19 Oct 2012 at 7:19

• Changed state: Accepted

[GoogleCodeExporter]

Conceptually, it shouldn't. The attempt is to use the unmangled name as a 
barrier that only allows overloads functions into the symbol. In my simple 
tests that seemed to work, so I'd be interested in seeing cases where it 
doesn't.

Original comment by kusmab...@gmail.com on 20 Oct 2012 at 11:09

[GoogleCodeExporter]

A large number of the WebGL conformance tests fail with the above patch 
applied: 
https://www.khronos.org/registry/webgl/conformance-suites/1.0.1/webgl-conformanc
e-tests.html

Indeed it doesn't appear that any redefinition error is generated. Instead only 
one return type of overloaded functions is recognised (e.g. "float abs(float)" 
and "vec2 abs(vec2)" are both considered to return float), leading to type 
conversion errors. The issue originates in TParseContext::findFunction, where 
it first tries to look up the unmangled name and then the mangled one. Since 
you're adding unmangled function names, it returns that one for all overloaded 
functions, and with it the return type of the first one that got added.

Original comment by nicolas....@gmail.com on 23 Oct 2012 at 2:45

[GoogleCodeExporter]

Right. My simple test only had differing parameters, not return values. And 
when I change the return value, I start getting senseless type-errors, indeed.

Thanks for pointing that out. The following hunk hacks around it, but I didn't 
(yet) test it properly. However, at this point it starts to smell like the 
proper solution might be to change the symbol-table to store a list of 
functions, keyed on the unmangled name instead.

Index: D:/Users/Erik/src/angleproject/src/compiler/ParseHelper.cpp
===================================================================
--- D:/Users/Erik/src/angleproject/src/compiler/ParseHelper.cpp (revision 1289)
+++ D:/Users/Erik/src/angleproject/src/compiler/ParseHelper.cpp (working copy)
@@ -975,7 +975,7 @@
     // First find by unmangled name to check whether the function name has been
     // hidden by a variable name or struct typename.
     const TSymbol* symbol = symbolTable.find(call->getName(), builtIn);
-    if (symbol == 0) {
+    if (symbol == 0 || symbol->isFunction()) {
         symbol = symbolTable.find(call->getMangledName(), builtIn);
     }

Original comment by kusmab...@gmail.com on 23 Oct 2012 at 3:13

[GoogleCodeExporter]

Yes, that appears to fix things.

I'm not violently opposed to having both mangled and unmangled function names 
in the symbol table, but we should at least strip the return type and argument 
list from the unmangled one. Perhaps indeed storing a list of symbols keyed on 
unmangled names could be more elegant, but it might as well have a few 
complications of its own.

Original comment by nicolas....@gmail.com on 23 Oct 2012 at 4:02

[GoogleCodeExporter]

Original comment by nico...@transgaming.com on 26 Jun 2013 at 3:39

• Changed state: Started

[GoogleCodeExporter]

Fixed in c66cd563c310 (master) and d4ceab122e10 (legacy).

Original comment by nico...@transgaming.com on 19 Jul 2013 at 9:28

• Changed state: Fixed