Closed GoogleCodeExporter closed 9 years ago
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
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
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
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
[deleted comment]
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
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
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
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
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
Original comment by nicolas....@gmail.com
on 18 Oct 2012 at 1:21
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
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
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
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
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
Original comment by nico...@transgaming.com
on 26 Jun 2013 at 3:39
Fixed in c66cd563c310 (master) and d4ceab122e10 (legacy).
Original comment by nico...@transgaming.com
on 19 Jul 2013 at 9:28
Original issue reported on code.google.com by
kusmab...@gmail.com
on 9 Oct 2012 at 2:28