Bug 101666 - bitfieldExtract is marked as a built-in function on OpenGL ES 3.0, but was added in OpenGL ES 3.1
Summary: bitfieldExtract is marked as a built-in function on OpenGL ES 3.0, but was ad...
Status: RESOLVED FIXED
Alias: None
Product: Mesa
Classification: Unclassified
Component: glsl-compiler (show other bugs)
Version: 17.1
Hardware: All All
: medium normal
Assignee: mesa-dev
QA Contact: Intel 3D Bugs Mailing List
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2017-07-01 02:18 UTC by Dwayne Slater
Modified: 2017-07-06 00:12 UTC (History)
0 users

See Also:
i915 platform:
i915 features:


Attachments
attachment-11561-0.html (2.09 KB, text/html)
2017-07-02 19:10 UTC, Ilia Mirkin
Details
attachment-15206-0.html (4.58 KB, text/html)
2017-07-02 19:38 UTC, Ilia Mirkin
Details

Description Dwayne Slater 2017-07-01 02:18:51 UTC
In the Ubershaders Dolphin PR a bitfieldExtract function is defined since the context that Dolphin uses on Android is an OpenGL ES 3.0 context.

During shader compilation, Mesa throws an error when encountering bitfieldExtract, even though bitfieldExtract is not defined in OpenGL ES 3.0.

Relevant Khronos reference page here: https://www.khronos.org/registry/OpenGL-Refpages/es3.1/html/bitfieldExtract.xhtml

To replicate this bug, define bitfieldExtract in an OpenGL ES 3.0 context. The actual implementation shouldn't matter, but just to be safe, heres the implementation used inside of Dolphin:

uint bitfieldExtract(uint val, int off, int size) {
    uint mask = uint((1 << size) - 1);
    return uint(val >> off) & mask;
}
Comment 1 Ilia Mirkin 2017-07-02 13:59:10 UTC
Despite the code looking just fine, I can repro this with a shader_test:

[require]
GL ES >= 3.0
GLSL ES >= 3.00

[vertex shader]
#version 300 es

uint bitfieldExtract(uint val, int off, int size) {
    uint mask = uint((1 << size) - 1);
    return uint(val >> off) & mask;
}

void main() {
  gl_Position = vec4(bitfieldExtract(1u, 2, 3));
}

[fragment shader]
#version 300 es
out vec4 color;
void main() { color = vec4(0,1,0,1); }

[test]
link success


results in

Failed to compile VS: 0:3(6): error: A shader cannot redefine or overload built-in function `bitfieldExtract' in GLSL ES 3.00
0:9(21): error: no matching function for call to `bitfieldExtract(uint, int, int)'; candidates are:
0:9(16): error: cannot construct `vec4' from a non-numeric data type

Will require some code reading, because the builtin_functions.cpp signature seems fine. It must not be getting validated at the right times...

[And to avoid any confusion, this was on SNB, so ES 3.1 wasn't even an option.]
Comment 2 Ilia Mirkin 2017-07-02 14:10:52 UTC
Aha. I see.

_mesa_glsl_has_builtin_function() needs to take a state, and only return true if sig->is_builtin_available(state). Unclear if _mesa_glsl_find_builtin_function needs to do the same.

Should be easy enough. I can hack it up tonight maybe, or someone else can take it.
Comment 3 Kenneth Graunke 2017-07-02 19:08:43 UTC
I don't think you need to change any infrastructure, we just didn't add the proper built-in availability predicate to the bitfieldExtract functions in builtin_functions.cpp.  it should be trivial to fix - this is by no means a new case...
Comment 4 Ilia Mirkin 2017-07-02 19:10:13 UTC
Created attachment 132398 [details]
attachment-11561-0.html

The predicate is correct, just never checked.

On Jul 2, 2017 15:08, <bugzilla-daemon@freedesktop.org> wrote:

> *Comment # 3 <https://bugs.freedesktop.org/show_bug.cgi?id=101666#c3> on
> bug 101666 <https://bugs.freedesktop.org/show_bug.cgi?id=101666> from
> Kenneth Graunke <kenneth@whitecape.org> *
>
> I don't think you need to change any infrastructure, we just didn't add the
> proper built-in availability predicate to the bitfieldExtract functions in
> builtin_functions.cpp.  it should be trivial to fix - this is by no means a new
> case...
>
> ------------------------------
> You are receiving this mail because:
>
>    - You are the assignee for the bug.
>
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>
>
Comment 5 Ilia Mirkin 2017-07-02 19:38:30 UTC
Created attachment 132400 [details]
attachment-15206-0.html

To be clear, I'm saying this is broken in Mesa for *any* situation that
involves redefining a function that appears in a future GL version or ext.
There are some limitations to this, based on when the error is triggered, I
don't have the code in front of me. At least it happens for es 3.0.

On Jul 2, 2017 15:10, <bugzilla-daemon@freedesktop.org> wrote:

*Comment # 4 <https://bugs.freedesktop.org/show_bug.cgi?id=101666#c4> on
bug 101666 <https://bugs.freedesktop.org/show_bug.cgi?id=101666> from Ilia
Mirkin <imirkin@alum.mit.edu> *

The predicate is correct, just never checked.

On Jul 2, 2017 15:08, <bugzilla-daemon@freedesktop.org> wrote:

> *Comment # 3 <https://bugs.freedesktop.org/show_bug.cgi?id=101666#c3> <https://bugs.freedesktop.org/show_bug.cgi?id=101666#c3> on
> bug 101666 <https://bugs.freedesktop.org/show_bug.cgi?id=101666> <https://bugs.freedesktop.org/show_bug.cgi?id=101666> from
> Kenneth Graunke <kenneth@whitecape.org> *

>> I don't think you need to change any infrastructure, we just didn't add the
> proper built-in availability predicate to the bitfieldExtract functions in
> builtin_functions.cpp.  it should be trivial to fix - this is by no means a new
> case...
>
> ------------------------------
> You are receiving this mail because:
>>    - You are the assignee for the bug.

>
>> _______________________________________________
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>
>

------------------------------
You are receiving this mail because:

   - You are the assignee for the bug.


_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Comment 6 Ilia Mirkin 2017-07-03 20:50:54 UTC
Just to illustrate my point further (and apologies for not properly compensating for bz's unfortunate email-to-bug integration):

 $ cat foo.shader_test 
[require]
GL ES >= 3.0
GLSL ES >= 3.00

[vertex shader]
#version 300 es

uint bitfieldExtract(uint val, int off, int size) {
    uint mask = uint((1 << size) - 1);
    return uint(val >> off) & mask;
}

uint bitfieldReverse(uint bits) {
  return bits;
}

float interpolateAtCentroid(float foo) {
  return foo;
}

void main() {
  gl_Position = vec4(bitfieldExtract(1u, 2, 3));
}

[fragment shader]
#version 300 es
out vec4 color;
void main() { color = vec4(0,1,0,1); }

[test]
link success

$ bin/shader_runner_gles3 foo.shader_test -auto
Failed to compile VS: 0:3(6): error: A shader cannot redefine or overload built-in function `bitfieldExtract' in GLSL ES 3.00
0:8(6): error: A shader cannot redefine or overload built-in function `bitfieldReverse' in GLSL ES 3.00
0:12(7): error: A shader cannot redefine or overload built-in function `interpolateAtCentroid' in GLSL ES 3.00
0:17(21): error: no matching function for call to `bitfieldExtract(uint, int, int)'; candidates are:
0:17(16): error: cannot construct `vec4' from a non-numeric data type

PIGLIT: {"result": "fail" }
Comment 7 Ilia Mirkin 2017-07-03 21:51:52 UTC
https://patchwork.freedesktop.org/patch/164980/

should do the trick.
Comment 8 Ilia Mirkin 2017-07-06 00:12:01 UTC
Fix pushed to master:

commit 880f21f55d579fe2183255d031c23343da30f69e (origin/master, origin/HEAD)
Author: Ilia Mirkin <imirkin@alum.mit.edu>
Date:   Mon Jul 3 17:08:12 2017 -0400

    glsl: check if any of the named builtins are available first

It should be cherry-picked to the 'stable' mesa tree over time, when the next release is made. Thanks for reporting the issue!


Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.