Change BufferManager::RequestBufferAccess signatures to avoid sprintfs.

gpu/command_buffer/service/buffer_manager.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "gpu/command_buffer/service/buffer_manager.h"
 
 #include <stdint.h>
 
 #include <limits>
 
@@ skipping 740 matching lines @@
 }
 
 Buffer* BufferManager::RequestBufferAccess(ContextState* context_state,
                                            GLenum target,
                                            GLintptr offset,
                                            GLsizeiptr size,
                                            const char* func_name) {
   DCHECK(context_state);
   ErrorState* error_state = context_state->GetErrorState();
 
-  std::string msg_tag = base::StringPrintf("bound to target 0x%04x", target);
   Buffer* buffer = GetBufferInfoForTarget(context_state, target);
-  if (!RequestBufferAccess(error_state, buffer, func_name, msg_tag.c_str())) {
+  if (!RequestBufferAccess(error_state, buffer, func_name,
+                           "bound to target 0x%04x", target)) {
     return nullptr;
   }
   if (!buffer->CheckRange(offset, size)) {
     std::string msg = base::StringPrintf(
-        "%s : offset/size out of range", msg_tag.c_str());
+        "bound to target 0x%04x : offset/size out of range", target);
     ERRORSTATE_SET_GL_ERROR(
         error_state, GL_INVALID_VALUE, func_name, msg.c_str());
     return nullptr;
   }
   return buffer;
 }
 
 Buffer* BufferManager::RequestBufferAccess(ContextState* context_state,
                                            GLenum target,
                                            const char* func_name) {
   DCHECK(context_state);
   ErrorState* error_state = context_state->GetErrorState();
 
-  std::string msg_tag = base::StringPrintf("bound to target 0x%04x", target);
   Buffer* buffer = GetBufferInfoForTarget(context_state, target);
   return RequestBufferAccess(
-      error_state, buffer, func_name, msg_tag.c_str()) ? buffer : nullptr;
+      error_state, buffer, func_name,
+      "bound to target 0x%04x", target) ? buffer : nullptr;
 }
 
 bool BufferManager::RequestBufferAccess(ErrorState* error_state,
                                         Buffer* buffer,
                                         const char* func_name,
-                                        const char* message_tag) {
+                                        const char* error_message_format, ...) {
   DCHECK(error_state);
 
-  if (!buffer || buffer->IsDeleted()) {
-    std::string msg = base::StringPrintf("%s : no buffer", message_tag);
-    ERRORSTATE_SET_GL_ERROR(error_state, GL_INVALID_OPERATION, func_name,
-                            msg.c_str());
-    return false;
-  }
-  if (buffer->GetMappedRange()) {
-    std::string msg = base::StringPrintf("%s : buffer is mapped", message_tag);
-    ERRORSTATE_SET_GL_ERROR(error_state, GL_INVALID_OPERATION, func_name,
-                            msg.c_str());
-    return false;
-  }
-  return true;
+  va_list varargs;
+  va_start(varargs, error_message_format);
+  bool result = RequestBufferAccessV(error_state, buffer, func_name,
+                                     error_message_format, varargs);
+  va_end(varargs);
+  return result;
 }
 
 bool BufferManager::RequestBufferAccess(ErrorState* error_state,
                                         Buffer* buffer,
                                         GLintptr offset,
                                         GLsizeiptr size,
                                         const char* func_name,
-                                        const char* message_tag) {
-  if (!RequestBufferAccess(error_state, buffer, func_name, message_tag)) {
+                                        const char* error_message_format,
+                                        ...) {
+  va_list varargs;
+  va_start(varargs, error_message_format);
+  bool request = RequestBufferAccessV(
+      error_state, buffer, func_name, error_message_format, varargs);
+  if (!request) {
+    va_end(varargs);
     return false;
   }
   if (!buffer->CheckRange(offset, size)) {
+    std::string message_tag = base::StringPrintV(error_message_format, varargs);

[piman, 2016/11/02 17:27:41]

According to http://en.cppreference.com/w/cpp/utility/variadic/va_list, we need
to call va_end (and, I assume, va_start again?) if varargs was used in
RequestBufferAccessV called from l.811, before it's used again here in the
calling function.
TBH I'm not sure of the exact semantics of passing a va_list by value... This
scares me a bit.

At a high level - do we need that level of generality? This function is always
called with the same error_message_format, and the same next argument (target).
Could we instead pass the target through and use it to generate the message
("bound to target 0x%x") at the error location?

[Ken Russell (switch to Gerrit), 2016/11/02 18:29:01]

If it was used in RequestBufferAccessV, that meant that line 811 returned false,
so va_end was called above and this function returned early.
It's legal to pass a va_list by value. I checked the Chromium sources first and
this is already done in several places where string formatting with varargs is
needed.
This variant is called from the outside, but only in one place:
GLES2DecoderImpl::ValidateCompressedTexFuncData. It can be simplified. Thanks
for the suggestion. I've uploaded a new patch doing so.

+    va_end(varargs);
     std::string msg = base::StringPrintf(
-        "%s : offset/size out of range", message_tag);
+        "%s : offset/size out of range", message_tag.c_str());
     ERRORSTATE_SET_GL_ERROR(
         error_state, GL_INVALID_OPERATION, func_name, msg.c_str());
     return false;
   }
+  va_end(varargs);
   return true;
 }
 
 bool BufferManager::RequestBuffersAccess(
     ErrorState* error_state,
     const IndexedBufferBindingHost* bindings,
     const std::vector<GLsizeiptr>& variable_sizes,
     GLsizei count,
     const char* func_name,
     const char* message_tag) {
@@ skipping 34 matching lines @@
 void BufferManager::IncreaseMappedBufferCount() {
   DCHECK_GT(std::numeric_limits<uint32_t>::max(), mapped_buffer_count_);
   mapped_buffer_count_++;
 }
 
 void BufferManager::DecreaseMappedBufferCount() {
   DCHECK_LT(0u, mapped_buffer_count_);
   mapped_buffer_count_--;
 }
 
+bool BufferManager::RequestBufferAccessV(ErrorState* error_state,
+                                         Buffer* buffer,
+                                         const char* func_name,
+                                         const char* error_message_format,
+                                         va_list varargs) {
+  DCHECK(error_state);
+
+  if (!buffer || buffer->IsDeleted()) {
+    std::string message_tag = base::StringPrintV(error_message_format, varargs);
+    std::string msg = base::StringPrintf("%s : no buffer", message_tag.c_str());
+    ERRORSTATE_SET_GL_ERROR(error_state, GL_INVALID_OPERATION, func_name,
+                            msg.c_str());
+    return false;
+  }
+  if (buffer->GetMappedRange()) {
+    std::string message_tag = base::StringPrintV(error_message_format, varargs);
+    std::string msg = base::StringPrintf("%s : buffer is mapped",
+                                         message_tag.c_str());
+    ERRORSTATE_SET_GL_ERROR(error_state, GL_INVALID_OPERATION, func_name,
+                            msg.c_str());
+    return false;
+  }
+  return true;
+}
+
 }  // namespace gles2
 }  // namespace gpu