Report relevant variable names in info logs on failed shader link

Report relevant variable names in info logs on failed shader link

In case there's a variable precision mismatch or name conflict, the
variable name is now printed in the program info log to ease fixing the
issue in complex shaders. Also fixes a bug where multiple hashed names
would not all be converted to the original names in info logs by
preferring to match less characters in the regex, instead of preferring
to match more and possibly eating up all the hashed names except the
last.

BUG=332798
TEST=gpu_unittests, WebGL GLSL conformance tests

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=244504

[piman, 2014-01-09 18:16:46]

Mostly good, aside from some nits.

https://codereview.chromium.org/132183002/diff/1/gpu/command_buffer/service/p...
File gpu/command_buffer/service/program_manager.h (right):

https://codereview.chromium.org/132183002/diff/1/gpu/command_buffer/service/p...
gpu/command_buffer/service/program_manager.h:193: bool
DetectUniformsMismatch(std::string& conflicting_name) const;
nit: style doesn't allow non-const references, instead using pointers (or
returning the string if it makes sense).

https://codereview.chromium.org/132183002/diff/1/gpu/command_buffer/service/p...
File gpu/command_buffer/service/program_manager_unittest.cc (right):

https://codereview.chromium.org/132183002/diff/1/gpu/command_buffer/service/p...
gpu/command_buffer/service/program_manager_unittest.cc:1278:
EXPECT_FALSE(conflicting_name.empty());
nit: maybe check that conficting_name is "a"

https://codereview.chromium.org/132183002/diff/1/gpu/command_buffer/service/p...
gpu/command_buffer/service/program_manager_unittest.cc:1295:
EXPECT_FALSE(conflicting_name.empty());
nit: maybe check that conficting_name is "a"

https://codereview.chromium.org/132183002/diff/1/gpu/command_buffer/service/p...
gpu/command_buffer/service/program_manager_unittest.cc:1327:
EXPECT_FALSE(conflicting_name.empty());
nit: maybe check that conficting_name is "a"

https://codereview.chromium.org/132183002/diff/1/gpu/command_buffer/service/p...
gpu/command_buffer/service/program_manager_unittest.cc:1361:
EXPECT_FALSE(conflicting_name.empty());
nit: maybe check that conficting_name is "a"

[oetuaho-nv, 2014-01-09 20:09:52]

On 2014/01/09 18:16:46, piman wrote:
> Mostly good, aside from some nits.
> 
>
https://codereview.chromium.org/132183002/diff/1/gpu/command_buffer/service/p...
> File gpu/command_buffer/service/program_manager.h (right):
> 
>
https://codereview.chromium.org/132183002/diff/1/gpu/command_buffer/service/p...
> gpu/command_buffer/service/program_manager.h:193: bool
> DetectUniformsMismatch(std::string& conflicting_name) const;
> nit: style doesn't allow non-const references, instead using pointers (or
> returning the string if it makes sense).
> 
>
https://codereview.chromium.org/132183002/diff/1/gpu/command_buffer/service/p...
> File gpu/command_buffer/service/program_manager_unittest.cc (right):
> 
>
https://codereview.chromium.org/132183002/diff/1/gpu/command_buffer/service/p...
> gpu/command_buffer/service/program_manager_unittest.cc:1278:
> EXPECT_FALSE(conflicting_name.empty());
> nit: maybe check that conficting_name is "a"
> 
>
https://codereview.chromium.org/132183002/diff/1/gpu/command_buffer/service/p...
> gpu/command_buffer/service/program_manager_unittest.cc:1295:
> EXPECT_FALSE(conflicting_name.empty());
> nit: maybe check that conficting_name is "a"
> 
>
https://codereview.chromium.org/132183002/diff/1/gpu/command_buffer/service/p...
> gpu/command_buffer/service/program_manager_unittest.cc:1327:
> EXPECT_FALSE(conflicting_name.empty());
> nit: maybe check that conficting_name is "a"
> 
>
https://codereview.chromium.org/132183002/diff/1/gpu/command_buffer/service/p...
> gpu/command_buffer/service/program_manager_unittest.cc:1361:
> EXPECT_FALSE(conflicting_name.empty());
> nit: maybe check that conficting_name is "a"

Nits have been fixed. I was on the edge originally about comparing to "a" since
outside the context of the unit test the function sets conflicting_name to the
hashed name, but of course comparing to "a" gives a bit more coverage.

Presubmit checks complained about DLOG(INFO) statements, but those are behind a
switch and not added by me.

[piman, 2014-01-09 21:29:36]

LGTM.
You should be able to replace the DLOG(INFO) by DVLOG(1) for the presubmit
(consensus is to clean up files as they are modified).

[Ken Russell (switch to Gerrit), 2014-01-09 21:40:02]

LGTM

https://codereview.chromium.org/132183002/diff/70001/gpu/command_buffer/servi...
File gpu/command_buffer/service/program_manager_unittest.cc (right):

https://codereview.chromium.org/132183002/diff/70001/gpu/command_buffer/servi...
gpu/command_buffer/service/program_manager_unittest.cc:1327:
EXPECT_EQ(conflicting_name, "a");
The constant should come first in these expressions: e.g. EXPECT_EQ("a",
conflicting_name).

[oetuaho-nv, 2014-01-10 12:12:02]

On 2014/01/09 21:40:02, Ken Russell wrote:
> LGTM
> 
>
https://codereview.chromium.org/132183002/diff/70001/gpu/command_buffer/servi...
> File gpu/command_buffer/service/program_manager_unittest.cc (right):
> 
>
https://codereview.chromium.org/132183002/diff/70001/gpu/command_buffer/servi...
> gpu/command_buffer/service/program_manager_unittest.cc:1327:
> EXPECT_EQ(conflicting_name, "a");
> The constant should come first in these expressions: e.g. EXPECT_EQ("a",
> conflicting_name).

The trybot failures look unrelated. Since you gave LGTM, do you still want me to
fix the things you commented on, or can you commit this?

[Ken Russell (switch to Gerrit), 2014-01-10 23:10:13]

On 2014/01/10 12:12:02, oetuaho-nv wrote:
> On 2014/01/09 21:40:02, Ken Russell wrote:
> > LGTM
> > 
> >
>
https://codereview.chromium.org/132183002/diff/70001/gpu/command_buffer/servi...
> > File gpu/command_buffer/service/program_manager_unittest.cc (right):
> > 
> >
>
https://codereview.chromium.org/132183002/diff/70001/gpu/command_buffer/servi...
> > gpu/command_buffer/service/program_manager_unittest.cc:1327:
> > EXPECT_EQ(conflicting_name, "a");
> > The constant should come first in these expressions: e.g. EXPECT_EQ("a",
> > conflicting_name).
> 
> The trybot failures look unrelated. Since you gave LGTM, do you still want me
to
> fix the things you commented on, or can you commit this?

I'd like you to fix the minor things. You can then click the Commit box at your
convenience (I believe -- let me know if that doesn't work.)

[commit-bot: I haz the power, 2014-01-13 13:21:41]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/oetuaho@nvidia.com/132183002/220001

[commit-bot: I haz the power, 2014-01-13 15:07:34]

Change committed as 244504