[Mesa-dev] [PATCH 2/2] Make shader refcounting atomic.

[Steinar H. Gunderson]

These were racy when using the same shaders (seemingly even from different
program objects) on multiple theads sharing the same objects, leading to
issues such as (excerpts from an apitrace dump from a real application):

  1097 @0 glCreateProgram() = 9
  1099 @0 glAttachShader(program = 9, shader = 7)
  1101 @0 glAttachShader(program = 9, shader = 8)
  [...]
  18122 @2 glCreateProgram() = 137
  18128 @2 glAttachShader(program = 137, shader = 7)
  18130 @2 glAttachShader(program = 137, shader = 8)
  [...]
  437559 @0 glUseProgram(program = 9)
  437582 @2 glUseProgram(program = 137)
  437613 @2 glUseProgram(program = 9)
  437614 @2 glGetError() = GL_INVALID_VALUE

with nothing deleting the shaders or programs in-between; just racy
refcounting, as confirmed by Helgrind:

  ==13727== Possible data race during read of size 4 at 0x2B3B2648 by thread #1
  ==13727== Locks held: none
  ==13727==    at 0x1EEBF613: _mesa_reference_shader_program_ (shaderobj.c:247)
  ==13727==    by 0x1EEBDFB2: _mesa_use_program (shaderapi.c:1259)
  ==13727==    by 0x60FA618: movit::EffectChain::execute_phase(movit::Phase*, bool, std::set<int, std::less<int>, std::allocator<int> >*, std::map<movit::Phase*, unsigned int, std::less<movit::Phase*>, std::allocator<std::pair<movit::Phase* const, unsigned int> > >*, std::set<movit::Phase*, std::less<movit::Phase*>, std::allocator<movit::Phase*> >*) (effect_chain.cpp:1885)
  [...]
  ==13727== This conflicts with a previous write of size 4 by thread #20
  ==13727== Locks held: none
  ==13727==    at 0x1EEBF600: _mesa_reference_shader_program_ (shaderobj.c:236)
  ==13727==    by 0x1EEBDFB2: _mesa_use_program (shaderapi.c:1259)
  ==13727==    by 0x60FA618: movit::EffectChain::execute_phase(movit::Phase*, bool, std::set<int, std::less<int>, std::allocator<int> >*, std::map<movit::Phase*, unsigned int, std::less<movit::Phase*>, std::allocator<std::pair<movit::Phase* const, unsigned int> > >*, std::set<movit::Phase*, std::less<movit::Phase*>, std::allocator<movit::Phase*> >*) (effect_chain.cpp:1885)

Cc: 11.2 12.0 13.0 <mesa-stable at lists.freedesktop.org>
Signed-off-by: Steinar H. Gunderson <steinar+mesa at gunderson.no>
---
 src/mesa/main/shaderobj.c | 23 +++++++++--------------
 1 file changed, 9 insertions(+), 14 deletions(-)

diff --git a/src/mesa/main/shaderobj.c b/src/mesa/main/shaderobj.c
index 8fd574e..08e4379 100644
--- a/src/mesa/main/shaderobj.c
+++ b/src/mesa/main/shaderobj.c
@@ -41,6 +41,7 @@
 #include "program/prog_parameter.h"
 #include "util/ralloc.h"
 #include "util/string_to_uint_map.h"
+#include "util/u_atomic.h"
 
 /**********************************************************************/
 /*** Shader object functions                                        ***/
@@ -64,14 +65,11 @@ _mesa_reference_shader(struct gl_context *ctx, struct gl_shader **ptr,
    }
    if (*ptr) {
       /* Unreference the old shader */
-      GLboolean deleteFlag = GL_FALSE;
       struct gl_shader *old = *ptr;
+      int old_refcount = p_atomic_dec_return(&old->RefCount);
 
-      assert(old->RefCount > 0);
-      old->RefCount--;
-      deleteFlag = (old->RefCount == 0);
-
-      if (deleteFlag) {
+      assert(old_refcount >= 0);
+      if (old_refcount == 0) {
 	 if (old->Name != 0)
 	    _mesa_HashRemove(ctx->Shared->ShaderObjects, old->Name);
          _mesa_delete_shader(ctx, old);
@@ -83,7 +81,7 @@ _mesa_reference_shader(struct gl_context *ctx, struct gl_shader **ptr,
 
    if (sh) {
       /* reference new */
-      sh->RefCount++;
+      p_atomic_inc(&sh->RefCount);
       *ptr = sh;
    }
 }
@@ -226,14 +224,11 @@ _mesa_reference_shader_program_(struct gl_context *ctx,
    }
    if (*ptr) {
       /* Unreference the old shader program */
-      GLboolean deleteFlag = GL_FALSE;
       struct gl_shader_program *old = *ptr;
+      int old_refcount = p_atomic_dec_return(&old->RefCount);
 
-      assert(old->RefCount > 0);
-      old->RefCount--;
-      deleteFlag = (old->RefCount == 0);
-
-      if (deleteFlag) {
+      assert(old_refcount >= 0);
+      if (old_refcount == 0) {
 	 if (old->Name != 0)
 	    _mesa_HashRemove(ctx->Shared->ShaderObjects, old->Name);
          _mesa_delete_shader_program(ctx, old);
@@ -244,7 +239,7 @@ _mesa_reference_shader_program_(struct gl_context *ctx,
    assert(!*ptr);
 
    if (shProg) {
-      shProg->RefCount++;
+      p_atomic_inc(&shProg->RefCount);
       *ptr = shProg;
    }
 }
-- 
2.8.0.rc3.226.g39d4020