[Mesa-dev] [PATCH] main: fix basename match's check if it's an array or struct
Tapani Pälli
tapani.palli at intel.com
Wed Oct 28 05:41:44 PDT 2015
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 :/
> 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