Bug 31744 - [GLSL] overriding built-in function impacts another shader
[GLSL] overriding built-in function impacts another shader
Status: VERIFIED FIXED
Product: Mesa
Classification: Unclassified
Component: glsl-compiler
git
All Linux (All)
: medium normal
Assigned To: Ian Romanick
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2010-11-18 18:39 UTC by Gordon Jin
Modified: 2011-08-25 18:33 UTC (History)
2 users (show)

See Also:
i915 platform:
i915 features:


Attachments
test case for piglit shader_runner (436 bytes, text/plain)
2010-11-18 18:39 UTC, Gordon Jin
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Gordon Jin 2010-11-18 18:39:05 UTC
Created attachment 40402 [details]
test case for piglit shader_runner

shader is an independent unit, so a built-in function overriden in one shader should not impact its (built-in defined) usage in another shader.

The attached case fails:

Probe at (0,0)
  Expected: 0.000000 1.000000 0.000000 1.000000
  Observed: 0.000000 0.000000 0.000000 1.000000
PIGLIT: {'result': 'fail' }


Tested on Piketon (i965) with mesa master.
Comment 1 Chad Versace 2010-11-22 15:47:37 UTC
(In reply to comment #0)
> shader is an independent unit, so a built-in function overriden in one shader
> should not impact its (built-in defined) usage in another shader.

According to the GLSL 4.10 spec, shaders are not independent units. A function definition that is present in one shader is applied to all shaders, for user-defined functions at least.

    (6.1) User-defined functions can have multiple declarations,
    but only one definition.

Applying this statement by analogy to the redelcaration of built-ins, I believe we obtain this:

    (me) Built-in functions can have multiple re-declarations,
    but only one re-definition.

Unfortunately, the spec's language on the redeclaration of built-ins is quite foggy:

    (6.1) If a built-in function is redeclared in a shader (i.e., a prototype is
    visible) before a call to it, then the linker will only attempt to resolve
    that call within the set of shaders that are linked with it.
Comment 2 Chad Versace 2010-11-22 16:00:22 UTC
(In reply to comment #1)
> Applying this statement by analogy to the redelcaration of built-ins, I believe
> we obtain this:
> 
>     (me) Built-in functions can have multiple re-declarations,
>     but only one re-definition.

I should rephrase that statement more clearly.
      Built-in functions can have multiple re-declarations,
      but only one re-definition, which applies to all linked shaders.
Comment 3 Vinson Lee 2010-11-24 16:23:17 UTC
I've committed the attachment id=40402 to piglit.
Comment 4 Chad Versace 2010-12-03 18:03:54 UTC
In my opinion, this is correct behavior. Built-in functions should be able to have multiple re-declarations, at most one re-definition, and that re-definition should apply to all linked shaders.

Gordon, did this test case come from a conformance suite?

Ian, do you have an opinion on the correctness of this behavior?
Comment 5 Gordon Jin 2010-12-05 03:48:29 UTC
Yes. And the test owner still think the case correct. I'm not sure and would like to hear Ian's comment.
Comment 6 Gordon Jin 2011-02-24 18:12:05 UTC
Note this case (glsl-override-builtin-2.shader_test) passes on nvidia.

Ian, comments?
Comment 7 Gordon Jin 2011-02-28 00:21:55 UTC
<OpenGL Shading Language, 3rd Ed> from Addison Wesley stands on my side. "3.6.3 Built-in Functions" says:
"If the definition is in a different shader, just make sure a prototype is visible before calling the function. Otherwise, the built-in version is used."
Comment 8 Ian Romanick 2011-06-30 11:49:32 UTC
I have a patch that should make our behavior match what the test expects and the behavior of the NVIDIA driver.  I want to create some more test cases to be sure that the behavior actually matches.  The whole thing of over-riding built-in functions in GLSL is really hokey.

Note that this test case fails on AMD.  I haven't tested Apple.
Comment 9 Ian Romanick 2011-07-20 13:32:23 UTC
This is fixed in master by the commit below.  This has been cherry picked to 7.11 (a90b88f) and 7.10 (66f4ac9).

commit 66f4ac988d5053c9782d1390541b04f4d9c50078
Author: Ian Romanick <ian.d.romanick@intel.com>
Date:   Wed Jun 29 14:52:10 2011 -0700

    linker: Only over-ride built-ins when a prototype has been seen
    
    The GLSL spec says:
    
        "If a built-in function is redeclared in a shader (i.e., a
        prototype is visible) before a call to it, then the linker will
        only attempt to resolve that call within the set of shaders that
        are linked with it."
    
    This patch enforces this behavior.  When a function call is processed
    a flag is set in the ir_call to indicate whether the previously seen
    prototype is the built-in or not.  At link time a call will only bind
    to an instance of a function that matches the "want built-in" setting
    in the ir_call.
    
    This has the odd side effect that first call to abs() in the shader
    below will call the built-in and the second will not:
    
    float foo(float x) { return abs(x); }
    float abs(float x) { return -x; }
    float bar(float x) { return abs(x); }
    
    This seems insane, but it matches what the spec says.
    
    Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=31744
Comment 10 Ian Romanick 2011-07-20 13:38:13 UTC
(In reply to comment #9)
> This is fixed in master by the commit below.  This has been cherry picked to
> 7.11 (a90b88f) and 7.10 (66f4ac9).
                           ^^^^^^^
Obviously bogus.  This is fixed in 7.10 by f2ef603.

> commit 66f4ac988d5053c9782d1390541b04f4d9c50078
         ^^^^^^^
> Author: Ian Romanick <ian.d.romanick@intel.com>
> Date:   Wed Jun 29 14:52:10 2011 -0700
> 
>     linker: Only over-ride built-ins when a prototype has been seen
> 
>     The GLSL spec says:
> 
>         "If a built-in function is redeclared in a shader (i.e., a
>         prototype is visible) before a call to it, then the linker will
>         only attempt to resolve that call within the set of shaders that
>         are linked with it."
> 
>     This patch enforces this behavior.  When a function call is processed
>     a flag is set in the ir_call to indicate whether the previously seen
>     prototype is the built-in or not.  At link time a call will only bind
>     to an instance of a function that matches the "want built-in" setting
>     in the ir_call.
> 
>     This has the odd side effect that first call to abs() in the shader
>     below will call the built-in and the second will not:
> 
>     float foo(float x) { return abs(x); }
>     float abs(float x) { return -x; }
>     float bar(float x) { return abs(x); }
> 
>     This seems insane, but it matches what the spec says.
> 
>     Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=31744
Comment 11 Gordon Jin 2011-08-25 18:33:11 UTC
Verified.

The attached test case was committed in piglit as glsl-override-builtin-2.