[Mesa-dev] [PATCH] main: fix basename match's check if it's an array or struct
Samuel Iglesias Gonsálvez
siglesias at igalia.com
Wed Oct 28 06:02:26 PDT 2015
On 28/10/15 13:41, Tapani Pälli wrote:
> On 10/28/2015 12:16 PM, Samuel Iglesias Gonsálvez wrote:
>>
>> On 28/10/15 11:13, Samuel Iglesias Gonsálvez wrote:
>>>
>>> On 28/10/15 10:31, Tapani Pälli wrote:
>>>> On 10/28/2015 09:09 AM, Samuel Iglesias Gonsálvez wrote:
>>>>> On 28/10/15 06:53, Tapani Pälli wrote:
>>>>>> On 10/27/2015 04:04 PM, Samuel Iglesias Gonsalvez wrote:
>>>>>>> Commit 4565b6f did not update the basename match's check for
>>>>>>> the case that string would exactly match the name of the
>>>>>>> variable if the suffix "[0]" were appended to it.
>>>>>>>
>>>>>>> Fixes two dEQP-GLES31 tests:
>>>>>>>
>>>>>>> dEQP-GLES31.functional.program_interface_query.shader_storage_block.resource_list.block_array
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> dEQP-GLES31.functional.program_interface_query.shader_storage_block.resource_list.block_array_single_element
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>> These tests are passing already with commit 4565b6f right? I think
>>>>>> the
>>>>>> 'found' boolean already takes care of this, I need to step through
>>>>>> again
>>>>>> to make sure though.
>>>>> No, they are failing in those cases because 'found' is true but
>>>>> then for
>>>>> either Uniform block or shader storage block, we check if the name
>>>>> is an
>>>>> array or struct:
>>>>>
>>>>> if (found) {
>>>>> switch (programInterface) {
>>>>> case GL_UNIFORM_BLOCK:
>>>>> case GL_SHADER_STORAGE_BLOCK:
>>>>> /* Basename match, check if array or struct. */
>>>>> if (name[baselen] == '\0' ||
>>>>> name[baselen] == '[' ||
>>>>> name[baselen] == '.') {
>>>>> [...]
>>>>>
>>>>> As 'found' can be true because of two different cases, we need to take
>>>>> care of both in this 'if' condition.
>>>>>
>>>>> $ ./deqp-gles31 -n
>>>>> dEQP-GLES31.functional.program_interface_query.shader_storage_block.resource_list.block_array*
>>>>>
>>>>>
>>>>> dEQP Core git-deb902685b9309a64b5e08c48157d3117cf27750 (0xdeb90268)
>>>>> starting..
>>>>> target implementation = 'Default'
>>>>>
>>>>> Test case
>>>>> 'dEQP-GLES31.functional.program_interface_query.shader_storage_block.resource_list.block_array'..
>>>>>
>>>>>
>>>>> Compute shader compile time = 16.732000 ms
>>>>> Link time = 14.251000 ms
>>>>> Test case duration in microseconds = 43993 us
>>>>> Fail (GetProgramResourceIndex returned unexpected values)
>>>>>
>>>>> Test case
>>>>> 'dEQP-GLES31.functional.program_interface_query.shader_storage_block.resource_list.block_array_single_element'..
>>>>>
>>>>>
>>>>> Compute shader compile time = 1.114000 ms
>>>>> Link time = 2.714000 ms
>>>>> Test case duration in microseconds = 8858 us
>>>>> Fail (GetProgramResourceIndex returned unexpected values)
>>>>>
>>>>> DONE!
>>>>>
>>>>> Test run totals:
>>>>> Passed: 0/2 (0.0%)
>>>>> Failed: 2/2 (100.0%)
>>>>> Not supported: 0/2 (0.0%)
>>>>> Warnings: 0/2 (0.0%)
>>>>>
>>>>> $ glxinfo | grep git
>>>>>
>>>>>
>>>>> OpenGL core profile version string: 3.3 (Core Profile) Mesa
>>>>> 11.1.0-devel
>>>>> (git-4565b6f)
>>>>> OpenGL version string: 3.0 Mesa 11.1.0-devel (git-4565b6f)
>>>>> OpenGL ES profile version string: OpenGL ES 3.1 Mesa 11.1.0-devel
>>>>> (git-4565b6f)
>>>> That is strange, for me these pass fine, I can see that our deqp
>>>> version
>>>> differ:
>>>>
>>>> [tpalli at tpalli-mobl2 gles31]$ mesa_run ./deqp-gles31
>>>> --deqp-case=dEQP-GLES31.functional.program_interface_query.shader_storage_block.resource_list.block_array*
>>>>
>>>>
>>>> dEQP Core git-7d804783aef83f8f2784d99812d98bcfcf21e95f (0x7d804783)
>>>> starting..
>>>> target implementation = 'Default'
>>>>
>>>> Test case
>>>> 'dEQP-GLES31.functional.program_interface_query.shader_storage_block.resource_list.block_array'..
>>>>
>>>>
>>>> Compute shader compile time = 4.585000 ms
>>>> Link time = 3.678000 ms
>>>> Test case duration in microseconds = 12021 us
>>>> Pass (Pass)
>>>>
>>>> Test case
>>>> 'dEQP-GLES31.functional.program_interface_query.shader_storage_block.resource_list.block_array_single_element'..
>>>>
>>>>
>>>> Compute shader compile time = 0.822000 ms
>>>> Link time = 1.225000 ms
>>>> Test case duration in microseconds = 3682 us
>>>> Pass (Pass)
>>>>
>>>> DONE!
>>>>
>>>> Test run totals:
>>>> Passed: 2/2 (100.0%)
>>>> Failed: 0/2 (0.0%)
>>>> Not supported: 0/2 (0.0%)
>>>> Warnings: 0/2 (0.0%)
>>>>
>>>>
>>>>
>>>> This is with
>>>>
>>>> MESA_EXTENSION_OVERRIDE=GL_ARB_compute_shader
>>>> MESA_GLES_VERSION_OVERRIDE=3.1
>>>>
>>>> and current Mesa head (03c92ff), tested on Ivybridge laptop.
>>>>
>>> I checked out the same commits for both dEQP and Mesa. Run it on HSW
>>> laptop with MESA_EXTENSION_OVERRIDE=GL_ARB_compute_shader and
>>> MESA_GLES_VERSION_OVERRIDE=3.1. Both tests failed for me.
>>>
>>> $ ./deqp-gles31 -n
>>> dEQP-GLES31.functional.program_interface_query.shader_storage_block.resource_list.block_array*
>>>
>>> dEQP Core git-7d804783aef83f8f2784d99812d98bcfcf21e95f (0x7d804783)
>>> starting..
>>> target implementation = 'Default'
>>>
>>> Test case
>>> 'dEQP-GLES31.functional.program_interface_query.shader_storage_block.resource_list.block_array'..
>>>
>>> Compute shader compile time = 10.610000 ms
>>> Link time = 8.145000 ms
>>> Test case duration in microseconds = 26574 us
>>> Fail (GetProgramResourceIndex returned unexpected values)
>>>
>>> Test case
>>> 'dEQP-GLES31.functional.program_interface_query.shader_storage_block.resource_list.block_array_single_element'..
>>>
>>> Compute shader compile time = 1.038000 ms
>>> Link time = 2.763000 ms
>>> Test case duration in microseconds = 7789 us
>>> Fail (GetProgramResourceIndex returned unexpected values)
>>>
>>> DONE!
>>>
>>> Test run totals:
>>> Passed: 0/2 (0.0%)
>>> Failed: 2/2 (100.0%)
>>> Not supported: 0/2 (0.0%)
>>> Warnings: 0/2 (0.0%)
>>> $ glxinfo | grep git
>>> OpenGL core profile version string: 3.3 (Core Profile) Mesa 11.1.0-devel
>>> (git-0325f68)
>>> OpenGL version string: 3.0 Mesa 11.1.0-devel (git-0325f68)
>>> OpenGL ES profile version string: OpenGL ES 3.1 Mesa 11.1.0-devel
>>> (git-0325f68)
>>>
>> Sorry, I used a later commit, but with 03c92ff I get the same result.
>
> I debugged this a bit, for me all the index queries with both of these
> tests hit GL_SHADER_STORAGE_BLOCK in that switch and for all of those
> name[baselen] == '\0' is true, that's why it passes. And this happens
> with luck because baselen is length of rname and rname can be longer in
> some cases than name, so we are actually reading out of bounds with that
> check :/
>
Oh, you are right. I am going to write a second version of this patch
taking that into account.
Thanks!
Sam
>
>> Sam
>>
>>> Verify you don't have uncommitted local changes that can modify this
>>> result. If not, this is very strange indeed (or maybe HSW is doing
>>> something different when adding the resources).
>>>
>>> Sam
>>>
>>>>> This function is becoming a bit of a pain because
>>>>>> different resources have different naming schemes, I'll have to
>>>>>> see if
>>>>>> it could be somehow refactored simpler.
>>>>>>
>>>>> OK, thanks.
>>>>>
>>>>> Sam
>>>>>
>>>>>>> Signed-off-by: Samuel Iglesias Gonsalvez <siglesias at igalia.com>
>>>>>>> ---
>>>>>>> src/mesa/main/shader_query.cpp | 3 ++-
>>>>>>> 1 file changed, 2 insertions(+), 1 deletion(-)
>>>>>>>
>>>>>>> diff --git a/src/mesa/main/shader_query.cpp
>>>>>>> b/src/mesa/main/shader_query.cpp
>>>>>>> index 59ec3d7..6cc91de 100644
>>>>>>> --- a/src/mesa/main/shader_query.cpp
>>>>>>> +++ b/src/mesa/main/shader_query.cpp
>>>>>>> @@ -592,7 +592,8 @@ _mesa_program_resource_find_name(struct
>>>>>>> gl_shader_program *shProg,
>>>>>>> /* Basename match, check if array or struct. */
>>>>>>> if (name[baselen] == '\0' ||
>>>>>>> name[baselen] == '[' ||
>>>>>>> - name[baselen] == '.') {
>>>>>>> + name[baselen] == '.' ||
>>>>>>> + rname_has_array_index_zero) {
>>>>>>> return res;
>>>>>>> }
>>>>>>> break;
>>>>
>
>
More information about the mesa-dev
mailing list