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

Review URL: https://codereview.chromium.org/132183002

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@244504 0039d316-1c4b-4281-b951-d872f2087c98

3 files changed, 71 insertions, 36 deletions

diff --git a/gpu/command_buffer/service/program_manager.cc b/gpu/command_buffer/service/program_manager.cc
index 9f1d10d..32b9140 100644
--- a/gpu/command_buffer/service/program_manager.cc
+++ b/gpu/command_buffer/service/program_manager.cc
@@ -180,7 +180,7 @@ std::string Program::ProcessLogInfo(
   std::string prior_log;
   std::string hashed_name;
   while (RE2::Consume(&input,
-                      "(.*)(webgl_[0123456789abcdefABCDEF]+)",
+                      "(.*?)(webgl_[0123456789abcdefABCDEF]+)",
                       &prior_log,
                       &hashed_name)) {
     output += prior_log;
@@ -358,13 +358,13 @@ void Program::Update() {
 #if !defined(NDEBUG)
   if (CommandLine::ForCurrentProcess()->HasSwitch(
       switches::kEnableGPUServiceLoggingGPU)) {
-    DLOG(INFO) << "----: attribs for service_id: " << service_id();
+    DVLOG(1) << "----: attribs for service_id: " << service_id();
     for (size_t ii = 0; ii < attrib_infos_.size(); ++ii) {
       const VertexAttrib& info = attrib_infos_[ii];
-      DLOG(INFO) << ii << ": loc = " << info.location
-                 << ", size = " << info.size
-                 << ", type = " << GLES2Util::GetStringEnum(info.type)
-                 << ", name = " << info.name;
+      DVLOG(1) << ii << ": loc = " << info.location
+               << ", size = " << info.size
+               << ", type = " << GLES2Util::GetStringEnum(info.type)
+               << ", name = " << info.name;
     }
   }
 #endif
@@ -436,14 +436,14 @@ void Program::Update() {
 #if !defined(NDEBUG)
   if (CommandLine::ForCurrentProcess()->HasSwitch(
       switches::kEnableGPUServiceLoggingGPU)) {
-    DLOG(INFO) << "----: uniforms for service_id: " << service_id();
+    DVLOG(1) << "----: uniforms for service_id: " << service_id();
     for (size_t ii = 0; ii < uniform_infos_.size(); ++ii) {
       const UniformInfo& info = uniform_infos_[ii];
       if (info.IsValid()) {
-        DLOG(INFO) << ii << ": loc = " << info.element_locations[0]
-                   << ", size = " << info.size
-                   << ", type = " << GLES2Util::GetStringEnum(info.type)
-                   << ", name = " << info.name;
+        DVLOG(1) << ii << ": loc = " << info.element_locations[0]
+                 << ", size = " << info.size
+                 << ", type = " << GLES2Util::GetStringEnum(info.type)
+                 << ", name = " << info.name;
       }
     }
   }
@@ -533,18 +533,24 @@ bool Program::Link(ShaderManager* manager,
     set_log_info("glBindAttribLocation() conflicts");
     return false;
   }
-  if (DetectUniformsMismatch()) {
-    set_log_info("Uniforms with the same name but different type/precision");
+  std::string conflicting_name;
+  if (DetectUniformsMismatch(&conflicting_name)) {
+    std::string info_log = "Uniforms with the same name but different "
+                           "type/precision: " + conflicting_name;
+    set_log_info(ProcessLogInfo(info_log).c_str());
     return false;
   }
-  if (DetectVaryingsMismatch()) {
-    set_log_info("Varyings with the same name but different type, "
-                 "or statically used varyings in fragment shader are not "
-                 "declared in vertex shader");
+  if (DetectVaryingsMismatch(&conflicting_name)) {
+    std::string info_log = "Varyings with the same name but different type, "
+                           "or statically used varyings in fragment shader are "
+                           "not declared in vertex shader: " + conflicting_name;
+    set_log_info(ProcessLogInfo(info_log).c_str());
     return false;
   }
-  if (DetectGlobalNameConflicts()) {
-    set_log_info("Name conflicts between an uniform and an attribute");
+  if (DetectGlobalNameConflicts(&conflicting_name)) {
+    std::string info_log = "Name conflicts between an uniform and an "
+                           "attribute: " + conflicting_name;
+    set_log_info(ProcessLogInfo(info_log).c_str());
     return false;
   }
   if (!CheckVaryingsPacking()) {
@@ -1005,7 +1011,7 @@ bool Program::DetectAttribLocationBindingConflicts() const {
   return false;
 }
 
-bool Program::DetectUniformsMismatch() const {
+bool Program::DetectUniformsMismatch(std::string* conflicting_name) const {
   typedef std::map<std::string, UniformType> UniformMap;
   UniformMap uniform_map;
   for (int ii = 0; ii < kMaxAttachedShaders; ++ii) {
@@ -1024,6 +1030,7 @@ bool Program::DetectUniformsMismatch() const {
         // declared by other shader, then the type and precision must match.
         if (map_entry->second == type)
           continue;
+        *conflicting_name = name;
         return true;
       }
     }
@@ -1031,7 +1038,7 @@ bool Program::DetectUniformsMismatch() const {
   return false;
 }
 
-bool Program::DetectVaryingsMismatch() const {
+bool Program::DetectVaryingsMismatch(std::string* conflicting_name) const {
   DCHECK(attached_shaders_[0] &&
          attached_shaders_[0]->shader_type() == GL_VERTEX_SHADER &&
          attached_shaders_[1] &&
@@ -1051,20 +1058,24 @@ bool Program::DetectVaryingsMismatch() const {
     ShaderTranslator::VariableMap::const_iterator hit =
         vertex_varyings->find(name);
     if (hit == vertex_varyings->end()) {
-      if (iter->second.static_use)
+      if (iter->second.static_use) {
+        *conflicting_name = name;
         return true;
+      }
       continue;
     }
 
     if (hit->second.type != iter->second.type ||
-        hit->second.size != iter->second.size)
+        hit->second.size != iter->second.size) {
+      *conflicting_name = name;
       return true;
+    }
 
   }
   return false;
 }
 
-bool Program::DetectGlobalNameConflicts() const {
+bool Program::DetectGlobalNameConflicts(std::string* conflicting_name) const {
   DCHECK(attached_shaders_[0] &&
          attached_shaders_[0]->shader_type() == GL_VERTEX_SHADER &&
          attached_shaders_[1] &&
@@ -1078,8 +1089,10 @@ bool Program::DetectGlobalNameConflicts() const {
   for (ShaderTranslator::VariableMap::const_iterator iter =
            attribs->begin(); iter != attribs->end(); ++iter) {
     for (int ii = 0; ii < 2; ++ii) {
-      if (uniforms[ii]->find(iter->first) != uniforms[ii]->end())
+      if (uniforms[ii]->find(iter->first) != uniforms[ii]->end()) {
+        *conflicting_name = iter->first;
         return true;
+      }
     }
   }
   return false;
diff --git a/gpu/command_buffer/service/program_manager.h b/gpu/command_buffer/service/program_manager.h
index 258a3c3..0da039d 100644
--- a/gpu/command_buffer/service/program_manager.h
+++ b/gpu/command_buffer/service/program_manager.h
@@ -188,15 +188,16 @@ class GPU_EXPORT Program : public base::RefCounted<Program> {
 
   // Detects if there are uniforms of the same name but different type
   // or precision in vertex/fragment shaders.
-  // Return true if such cases are detected.
-  bool DetectUniformsMismatch() const;
+  // Return true and set the first found conflicting hashed name to
+  // conflicting_name if such cases are detected.
+  bool DetectUniformsMismatch(std::string* conflicting_name) const;
 
   // Return true if a varying is statically used in fragment shader, but it
   // is not declared in vertex shader.
-  bool DetectVaryingsMismatch() const;
+  bool DetectVaryingsMismatch(std::string* conflicting_name) const;
 
   // Return true if an uniform and an attribute share the same name.
-  bool DetectGlobalNameConflicts() const;
+  bool DetectGlobalNameConflicts(std::string* conflicting_name) const;
 
   // Return false if varyings can't be packed into the max available
   // varying registers.
diff --git a/gpu/command_buffer/service/program_manager_unittest.cc b/gpu/command_buffer/service/program_manager_unittest.cc
index 2c51c88..461aadf 100644
--- a/gpu/command_buffer/service/program_manager_unittest.cc
+++ b/gpu/command_buffer/service/program_manager_unittest.cc
@@ -1255,7 +1255,10 @@ TEST_F(ProgramManagerWithShaderTest, UniformsPrecisionMismatch) {
   EXPECT_TRUE(program->AttachShader(&shader_manager_, vshader));
   EXPECT_TRUE(program->AttachShader(&shader_manager_, fshader));
 
-  EXPECT_TRUE(program->DetectUniformsMismatch());
+  std::string conflicting_name;
+
+  EXPECT_TRUE(program->DetectUniformsMismatch(&conflicting_name));
+  EXPECT_EQ("a", conflicting_name);
   EXPECT_TRUE(LinkAsExpected(program, false));
 }
 
@@ -1269,7 +1272,10 @@ TEST_F(ProgramManagerWithShaderTest, VaryingTypeMismatch) {
   Program* program = SetupShaderVariableTest(
       &kVertexVarying, 1, &kFragmentVarying, 1);
 
-  EXPECT_TRUE(program->DetectVaryingsMismatch());
+  std::string conflicting_name;
+
+  EXPECT_TRUE(program->DetectVaryingsMismatch(&conflicting_name));
+  EXPECT_EQ("a", conflicting_name);
   EXPECT_TRUE(LinkAsExpected(program, false));
 }
 
@@ -1283,7 +1289,10 @@ TEST_F(ProgramManagerWithShaderTest, VaryingArraySizeMismatch) {
   Program* program = SetupShaderVariableTest(
       &kVertexVarying, 1, &kFragmentVarying, 1);
 
-  EXPECT_TRUE(program->DetectVaryingsMismatch());
+  std::string conflicting_name;
+
+  EXPECT_TRUE(program->DetectVaryingsMismatch(&conflicting_name));
+  EXPECT_EQ("a", conflicting_name);
   EXPECT_TRUE(LinkAsExpected(program, false));
 }
 
@@ -1297,7 +1306,10 @@ TEST_F(ProgramManagerWithShaderTest, VaryingPrecisionMismatch) {
   Program* program = SetupShaderVariableTest(
       &kVertexVarying, 1, &kFragmentVarying, 1);
 
-  EXPECT_FALSE(program->DetectVaryingsMismatch());
+  std::string conflicting_name;
+
+  EXPECT_FALSE(program->DetectVaryingsMismatch(&conflicting_name));
+  EXPECT_TRUE(conflicting_name.empty());
   EXPECT_TRUE(LinkAsExpected(program, true));
 }
 
@@ -1309,7 +1321,10 @@ TEST_F(ProgramManagerWithShaderTest, VaryingMissing) {
   Program* program = SetupShaderVariableTest(
       NULL, 0, &kFragmentVarying, 1);
 
-  EXPECT_TRUE(program->DetectVaryingsMismatch());
+  std::string conflicting_name;
+
+  EXPECT_TRUE(program->DetectVaryingsMismatch(&conflicting_name));
+  EXPECT_EQ("a", conflicting_name);
   EXPECT_TRUE(LinkAsExpected(program, false));
 }
 
@@ -1322,7 +1337,10 @@ TEST_F(ProgramManagerWithShaderTest, InactiveVarying) {
   Program* program = SetupShaderVariableTest(
       NULL, 0, &kFragmentVarying, 1);
 
-  EXPECT_FALSE(program->DetectVaryingsMismatch());
+  std::string conflicting_name;
+
+  EXPECT_FALSE(program->DetectVaryingsMismatch(&conflicting_name));
+  EXPECT_TRUE(conflicting_name.empty());
   EXPECT_TRUE(LinkAsExpected(program, true));
 }
 
@@ -1337,7 +1355,10 @@ TEST_F(ProgramManagerWithShaderTest, AttribUniformNameConflict) {
   Program* program = SetupShaderVariableTest(
       &kVertexAttribute, 1, &kFragmentUniform, 1);
 
-  EXPECT_TRUE(program->DetectGlobalNameConflicts());
+  std::string conflicting_name;
+
+  EXPECT_TRUE(program->DetectGlobalNameConflicts(&conflicting_name));
+  EXPECT_EQ("a", conflicting_name);
   EXPECT_TRUE(LinkAsExpected(program, false));
 }