[Mesa-dev] [PATCH 16/26] glsl: Clean up shading language mixing check for GLSL 3.00 ES.

[Ian Romanick]

On 12/01/2012 04:52 AM, Kenneth Graunke wrote:
> On 11/30/2012 10:07 AM, Ian Romanick wrote:
>> From: Paul Berry <stereotype441 at gmail.com>
>>
>> Previously, we prohibited mixing of shading language versions if
>> min_version == 100 or max_version >= 130.  This was technically
>> correct (since desktop GLSL 1.30 and beyond prohibit mixing of shading
>> language versions, as does GLSL 1.00 ES), but it was confusing.  Also,
>> we asserted that all shading language versions were between 1.00 and
>> 1.40, which was unnecessary (since the parser already checks shading
>> language versions) and doesn't work for GLSL 3.00 ES.
>>
>> This patch changes the code to explicitly check that (a) ES shaders
>> aren't mixed with desktop shaders, (b) shaders aren't mixed between ES
>> versions, and (c) shaders aren't mixed between desktop GLSL versions
>> when at least one shader is GLSL 1.30 or greater.  Also, it removes
>> the unnecessary assertion.
>>
>> Reviewed-by: Ian Romanick <ian.d.romanick at intel.com>
>> ---
>>   src/glsl/linker.cpp | 16 +++++++++++++---
>>   1 file changed, 13 insertions(+), 3 deletions(-)
>>
>> diff --git a/src/glsl/linker.cpp b/src/glsl/linker.cpp
>> index 3b2ab96..1bae043 100644
>> --- a/src/glsl/linker.cpp
>> +++ b/src/glsl/linker.cpp
>> @@ -2421,9 +2421,19 @@ link_shaders(struct gl_context *ctx, struct
>> gl_shader_program *prog)
>>
>>      unsigned min_version = UINT_MAX;
>>      unsigned max_version = 0;
>> +   bool is_es_prog = false;
>>      for (unsigned i = 0; i < prog->NumShaders; i++) {
>>         min_version = MIN2(min_version, prog->Shaders[i]->Version);
>>         max_version = MAX2(max_version, prog->Shaders[i]->Version);
>> +      if (i == 0) {
>> +         is_es_prog = prog->Shaders[i]->IsEsShader;
>> +      } else {
>
> I really dislike loops that contain conditional code for particular
> indices.  It almost seems like you want:
>
> bool is_es_prog = prog->Shaders[0]->IsEsShader;
>
> then you can just omit this code within the loop.  Nice and clean.  The
> only trouble is that doing so assumes there is at least one shader.
>
> I *believe* that calling glLinkProgram with no attached shaders is an
> error and should be rejected (oglconform claims it's invalid).  However,
> I can't find any spec text about it and I'm fairly sure we don't verify
> that NumShaders >= 1 when linking.

When you link, one shader for each stage must define main.  If there are 
no shaders, no shader defines main.  We should catch this case with our 
check for main.

> But it's not a big deal...just a pet peeve I picked up after reading
> code in another project which was essentially:
> for (int i = 0; i < POUND_DEFINE_THAT_HAPPENS_TO_BE_THREE; i++) {
>     if (i == 0) {...} else if (i == 1) {...} else if (i == 2) {...}
>     /* seriously, why the hell did you make this a loop? */
> }
>
> Your code here is sensible and gets a R-b either way.
>
>> +         if (prog->Shaders[i]->IsEsShader != is_es_prog) {
>> +            linker_error(prog, "all shaders must use same shading "
>> +                         "language version\n");
>> +            goto done;
>> +         }
>> +      }
>>
>>         switch (prog->Shaders[i]->Type) {
>>         case GL_VERTEX_SHADER:
>> @@ -2444,10 +2454,10 @@ link_shaders(struct gl_context *ctx, struct
>> gl_shader_program *prog)
>>      /* Previous to GLSL version 1.30, different compilation units
>> could mix and
>>       * match shading language versions.  With GLSL 1.30 and later,
>> the versions
>>       * of all shaders must match.
>> +    *
>> +    * GLSL ES has never allowed mixing of shading language versions.
>>       */
>> -   assert(min_version >= 100);
>> -   assert(max_version <= 140);
>> -   if ((max_version >= 130 || min_version == 100)
>> +   if ((is_es_prog || max_version >= 130)
>>          && min_version != max_version) {
>>         linker_error(prog, "all shaders must use same shading "
>>              "language version\n");
>>
>