Bug 13753 - Numerous bugs in GLSL uniform handling
Numerous bugs in GLSL uniform handling
Status: RESOLVED FIXED
Product: Mesa
Classification: Unclassified
Component: Mesa core
unspecified
x86-64 (AMD64) Linux (All)
: medium normal
Assigned To: mesa-dev
:
: 13751 (view as bug list)
Depends on:
Blocks: 29044
  Show dependency treegraph
 
Reported: 2007-12-20 06:23 UTC by Bruce Merry
Modified: 2010-08-25 12:47 UTC (History)
0 users

See Also:


Attachments
Patch 1/5 (6.12 KB, patch)
2007-12-21 13:21 UTC, Bruce Merry
Details | Splinter Review
Patch 2/5 (3.05 KB, patch)
2007-12-21 13:22 UTC, Bruce Merry
Details | Splinter Review
Patch 3/5 (10.17 KB, patch)
2007-12-21 13:22 UTC, Bruce Merry
Details | Splinter Review
Patch 4/5 (2.70 KB, patch)
2007-12-21 13:23 UTC, Bruce Merry
Details | Splinter Review
Patch 5/5 (3.31 KB, patch)
2007-12-21 13:32 UTC, Bruce Merry
Details | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Bruce Merry 2007-12-20 06:23:11 UTC
These are all bugs I noticed while trying to track another bug through the Mesa source. I'd be happy to work on a patch during the next few weeks, but I'd to first check which are genuine bugs (i.e., oversights) and which are just due to things being incomplete and are being worked on.

- glGetUniformLocation doesn't allow array elements to be queried (there are some comments in the code that seem to indicate that this is known)
- glUniformMatrix ignores the count argument totally
- glUniform1i only allows 1 sampler to be set in an array
- glUniform* does not validate the type from the entry point against the type of the uniform
- glUniform* raises an error if location == -1, although this is defined as a no-op in the standard
- glUniform* does not convert int/float to boolean when setting a boolean uniform
- glUniform* does not allow count to exceed the size of a uniform array (which is defined as legal in the standard, with excess elements ignored)
- in glUniform*, a count of around 2^30 and 4-element vectors may lead to an integer overflow when checking for array overrun
- many of the bugs in glUniform* also apply to glUniformMatrix*
Comment 1 Bruce Merry 2007-12-20 06:26:40 UTC
Forgot to mention: this is 7.0.2, and I'm seeing the bugs in src/mesa/shaders/shader_api.c.
Comment 2 Brian Paul 2007-12-20 08:13:02 UTC
(In reply to comment #0)
> These are all bugs I noticed while trying to track another bug through the Mesa
> source. I'd be happy to work on a patch during the next few weeks, but I'd to
> first check which are genuine bugs (i.e., oversights) and which are just due to
> things being incomplete and are being worked on.
> 
> - glGetUniformLocation doesn't allow array elements to be queried (there are
> some comments in the code that seem to indicate that this is known)

A known issue.

> - glUniformMatrix ignores the count argument totally

a bug

> - glUniform1i only allows 1 sampler to be set in an array

known issue

> - glUniform* does not validate the type from the entry point against the type
> of the uniform

bug

> - glUniform* raises an error if location == -1, although this is defined as a
> no-op in the standard

bug

> - glUniform* does not convert int/float to boolean when setting a boolean
> uniform

bug


> - glUniform* does not allow count to exceed the size of a uniform array (which
> is defined as legal in the standard, with excess elements ignored)

bug


> - in glUniform*, a count of around 2^30 and 4-element vectors may lead to an
> integer overflow when checking for array overrun

bug

> - many of the bugs in glUniform* also apply to glUniformMatrix*

I probably won't get to fix these until next year.  Feel free to post patches...
Comment 3 Bruce Merry 2007-12-21 06:25:16 UTC
I've checked Mesa out of anonymous git and written 4 patches to address some of the bugs above. What's the best way to get them to you?
Comment 4 Dan Nicholson 2007-12-21 07:40:20 UTC
I think Brian will be offline until the new year, but you can just attach the patches to this bug.
Comment 5 Bruce Merry 2007-12-21 13:21:34 UTC
Created attachment 13294 [details] [review]
Patch 1/5
Comment 6 Bruce Merry 2007-12-21 13:22:07 UTC
Created attachment 13295 [details] [review]
Patch 2/5
Comment 7 Bruce Merry 2007-12-21 13:22:45 UTC
Created attachment 13296 [details] [review]
Patch 3/5
Comment 8 Bruce Merry 2007-12-21 13:23:13 UTC
Created attachment 13297 [details] [review]
Patch 4/5
Comment 9 Bruce Merry 2007-12-21 13:32:17 UTC
Created attachment 13298 [details] [review]
Patch 5/5

I've attached a series of 5 git patches (never really used git before so let me know if I stuffed it up). They fix most of these bugs that can be fixed locally (i.e. without messing with other files and causing unintended consequences). They also finish off Brian's fix to bug 13751, and add a prog/tests/shader_api regression test (maybe should get renamed to bug_13751, that's up to someone else). The regression test doesn't work on everything because some of the bugs are masked by other bugs (e.g. the shader compiler asserts when given an array of samplers).

The problems still outstanding in shader_api.c
- glGetUniformLocation and glGetActiveUniform are still brain-damaged because the compiler doesn't provide the right information.
- passing count > 1 on a non-array is now silently accepted, because again the compiler doesn't provide enough information to distinguish between a 1-element array and a non-array (unless I missed something).
- glGetActiveAttrib almost always gets the type wrong (but less wrong than before), again because the compiler isn't providing the necessary information.
Comment 10 Bruce Merry 2007-12-21 13:33:59 UTC
*** Bug 13751 has been marked as a duplicate of this bug. ***
Comment 11 Brian Paul 2008-01-01 09:01:12 UTC
I've commited your patches to git (both master and mesa_7_0_branch).

I may not get around to the remaining issues for a while though.
Comment 12 Eric Anholt 2010-08-25 12:47:55 UTC
I think these are all fixed now. At least GetActiveUniform and count > 1 should be fine, and I think shaderAPI is testing attribute types.