Bug 83508 - [UBO] Assertion for array of blocks
Summary: [UBO] Assertion for array of blocks
Status: RESOLVED FIXED
Alias: None
Product: Mesa
Classification: Unclassified
Component: glsl-compiler (show other bugs)
Version: git
Hardware: Other All
: medium normal
Assignee: Ian Romanick
QA Contact: Intel 3D Bugs Mailing List
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-09-04 18:19 UTC by Ian Romanick
Modified: 2015-09-28 12:28 UTC (History)
3 users (show)

See Also:
i915 platform:
i915 features:


Attachments
Test case (249 bytes, text/plain)
2014-09-04 18:19 UTC, Ian Romanick
Details

Description Ian Romanick 2014-09-04 18:19:51 UTC
Created attachment 105763 [details]
Test case

Attached test case causes the following assertion failure.  In non-debug builds, it generates a bunch of valgrind rage followed by a segfault.

shader_runner: ../../src/glsl/link_uniform_blocks.cpp:215: unsigned int link_uniform_blocks(void*, gl_shader_program*, gl_shader**, unsigned int, gl_uniform_block**): Assertion `(b->num_array_elements > 0) == b->type->is_array()' failed.
Comment 1 Samuel Iglesias Gonsálvez 2015-07-27 08:29:28 UTC
This is going to be fixed by Antía's patch:

http://lists.freedesktop.org/archives/mesa-dev/2015-March/079102.html

We are planning to send an updated version of this patch in the coming days.
Comment 2 Timothy Arceri 2015-09-19 04:59:08 UTC
(In reply to Samuel Iglesias from comment #1)
> This is going to be fixed by Antía's patch:
> 
> http://lists.freedesktop.org/archives/mesa-dev/2015-March/079102.html
> 
> We are planning to send an updated version of this patch in the coming days.

That will work around the issue for 'shared' and 'std140' but if a shader has a ubo with a 'packed' layout and is unused in the shader it will still hit this same problem right?

The problem is the linking code always expects there to be a least one active use of the ubo array in the shader, I attempted to fix this but it REALLY expects there to be a use so its not quite as simple as you would expect.

I may give it another go when I have some time as I'm playing around with this code for AoA support currently.
Comment 3 Antia Puentes 2015-09-22 11:11:35 UTC
(In reply to Timothy Arceri from comment #2)
> (In reply to Samuel Iglesias from comment #1)
> > This is going to be fixed by Antía's patch:
> > 
> > http://lists.freedesktop.org/archives/mesa-dev/2015-March/079102.html
> > 
> > We are planning to send an updated version of this patch in the coming days.

Hi! Timothy,

I have a limited knowledge about the linking code but I did take a look at how the link_uniform_block_active_visitor works.

> That will work around the issue for 'shared' and 'std140' but if a shader
> has a ubo with a 'packed' layout and is unused in the shader it will still
> hit this same problem right?

I think it will not. The 'shared' and 'std140' ubo arrays are always marked as active, regardless of they being referenced or not in the code. However, for 'packed' ubos I can see that they are marked as active by the link_uniform_block_active_visitor *only if* there is a reference to them. By marking them as active, I mean that they will be added by the visitor to the 'block_hash' hashtable  declared in the "link_uniform_blocks" method.

> The problem is the linking code always expects there to be a least one
> active use of the ubo array in the shader, I attempted to fix this but it
> REALLY expects there to be a use so its not quite as simple as you would
> expect.

Could you point out where that is expected?. Thanks.
My understanding is that the linking process would only care about what we have inside that 'block_hash' active uniform blocks hashtable and, in the case of 'packed' ubos, they only be there if they have been referenced.

> I may give it another go when I have some time as I'm playing around with
> this code for AoA support currently.
Comment 4 Timothy Arceri 2015-09-23 03:38:35 UTC
(In reply to Antia Puentes from comment #3)
> (In reply to Timothy Arceri from comment #2)
> > (In reply to Samuel Iglesias from comment #1)
> > > This is going to be fixed by Antía's patch:
> > > 
> > > http://lists.freedesktop.org/archives/mesa-dev/2015-March/079102.html
> > > 
> > > We are planning to send an updated version of this patch in the coming days.
> 
> Hi! Timothy,
> 
> I have a limited knowledge about the linking code but I did take a look at
> how the link_uniform_block_active_visitor works.
> 
> > That will work around the issue for 'shared' and 'std140' but if a shader
> > has a ubo with a 'packed' layout and is unused in the shader it will still
> > hit this same problem right?
> 
> I think it will not. The 'shared' and 'std140' ubo arrays are always marked
> as active, regardless of they being referenced or not in the code. However,
> for 'packed' ubos I can see that they are marked as active by the
> link_uniform_block_active_visitor *only if* there is a reference to them.

ok, so this was the problem. The code was broken for arrays, I've sent a patch to the mailing list [1] and Cc'd yourself and Ian. Also sent piglit test [2]

[1] http://lists.freedesktop.org/archives/mesa-dev/2015-September/095070.html
[2] http://lists.freedesktop.org/archives/piglit/2015-September/017247.html
Comment 5 Antia Puentes 2015-09-25 08:56:00 UTC
Should be fixed by:

commit e92c35a8724efd36a35ac9106e5977c5ec2cb332
Author: Antia Puentes <apuentes@igalia.com>
Date:   Wed Jul 29 16:01:24 2015 +0200

    glsl: Mark as active all elements of shared/std140 block arrays
    
    Commit 1ca25ab (glsl: Do not eliminate 'shared' or 'std140' blocks
    or block members) considered as active 'shared' and 'std140' uniform
    blocks and uniform block arrays, but did not include the block array
    elements. Because of that, it was possible to have an active uniform
    block array without any elements marked as used, making the assertion
       ((b->num_array_elements > 0) == b->type->is_array())
    in link_uniform_blocks() fail.
    
    Fixes the following 5 dEQP tests:
    
     * dEQP-GLES3.functional.ubo.random.nested_structs_instance_arrays.18
     * dEQP-GLES3.functional.ubo.random.nested_structs_instance_arrays.24
     * dEQP-GLES3.functional.ubo.random.nested_structs_arrays_instance_arrays.19
     * dEQP-GLES3.functional.ubo.random.all_per_block_buffers.49
     * dEQP-GLES3.functional.ubo.random.all_shared_buffer.36
    
    Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=83508
    Tested-by: Tapani Pälli <tapani.palli@intel.com>
    Reviewed-by: Timothy Arceri <t_arceri@yahoo.com.au>
    Reviewed-by: Kristian Høgsberg <krh@bitplanet.net>


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.