[Mesa-dev] [PATCH] glsl: properly setting var->data.binding if explicit_binding is true

[Emil Velikov]

Hi Alejandro

On 26 April 2015 at 14:23, Alejandro Piñeiro <apinheiro at igalia.com> wrote:
> On 26/04/15 00:08, Timothy Arceri wrote:
>> On Sat, 2015-04-25 at 18:46 +0200, Alejandro Piñeiro wrote:
>>> There was a typo on commit c0cd5b, doing it when explicit_binding
>>> was false. This prevented to use any binding point different to 0.

Considering that the offending commit landed in 10.4-devel, please add
the following line in the commit message. This way we won't forget
picking this up for the stable branches.

Cc: "10.4 10.5" <mesa-stable at lists.freedesktop.org>

>>> ---
>>>
>>> Taking into account the explanation on the header about the
>>> variable binding (ast.h:553)
>>>
>>>    /**
>>>     * Binding specified via GL_ARB_shading_language_420pack's "binding" keyword.
>>>     *
>>>     * \note
>>>     * This field is only valid if \c explicit_binding is set.
>>>     */
>>>    int binding;
>>>
>>> The binding is correct (and should be updated) if explicit_binding is true.
>>> But the current behaviour was updating it if it was false.
>>>
>>> This was not detected by piglit because all the calls to
>>> glBindBufferBase(GL_ATOMIC_COUNTER_BUFFER are using 0.
>>>
>>> I tested this patch by running all piglit on my system, and I didn't
>>> detect regression. I also runned make check without issues.
>>>
>>> https://bugs.freedesktop.org/show_bug.cgi?id=90175
Please move the bug entry to the commit message and prefix with Bugzilla:

>> You should probably convert your test program to a piglit test also so
>> this bug can be detected if it happens again in the future.
>
> There are several piglit tests at spec/arb_shader_atomic_counters
> testing that you get what you expect while using atomic counters.
> Probably it would be enough to just modify some of the already existing
> tests, using a non-zero binding point (for example at semantics.c).
>
> But I don't have too much experience tweaking/creating piglit tests.
> What option would be preferred for this case? A new test or just modify
> a little one of the already available?
>
Don't think that changing the behaviour of existing tests is a good
idea. As Timothy mentioned, you can add a subtest which will cut down
the copy/pasta a bit.

Cheers,
Emil