[Mesa-dev] [PATCH 2/4] mesa: fix crash when an ATI_fs pass begins with an alpha inst

[Roland Scheidegger]

Am 21.11.2017 um 01:40 schrieb Ian Romanick:
> On 11/20/2017 04:07 PM, Miklós Máté wrote:
>> This fixes crash when:
>> - first pass begins with alpha inst
>> - first pass ends with color inst, second pass begins with alpha inst
>>
>> Also, use the symbolic name instead of a number.
>> Piglit: spec/ati_fragment_shader/api-alphafirst
>>
>> Signed-off-by: Miklós Máté <mtmkls at gmail.com>
>> ---
>>  src/mesa/main/atifragshader.c | 6 ++++--
>>  1 file changed, 4 insertions(+), 2 deletions(-)
>>
>> diff --git a/src/mesa/main/atifragshader.c b/src/mesa/main/atifragshader.c
>> index 49ddb6e5af..d6fc37ac8f 100644
>> --- a/src/mesa/main/atifragshader.c
>> +++ b/src/mesa/main/atifragshader.c
>> @@ -598,8 +598,10 @@ _mesa_FragmentOpXATI(GLint optype, GLuint arg_count, GLenum op, GLuint dst,
>>        curProg->cur_pass=3;
>>  
>>     /* decide whether this is a new instruction or not ... all color instructions are new,
>> -      and alpha instructions might also be new if there was no preceding color inst */
>> -   if ((optype == 0) || (curProg->last_optype == optype)) {
>> +      and alpha instructions might also be new if there was no preceding color inst,
>> +      and this may be the first inst of the pass */
> 
> I know the code previously used this same formatting, but the Mesa style
> is for the */ of a multi-line comment to be on its own line.  Each other
> line should also start with a *.  And line-wrap around 78 characters.
> Like:
> 
>    /* Decide whether this is a new instruction or not.  All color instructions
>     * are new, and alpha instructions might also be new if there was no
>     * preceding color inst.  This may also be the first inst of the pass.
>     */
> 
>> +   if ((optype == ATI_FRAGMENT_SHADER_COLOR_OP) || (curProg->last_optype == optype)
>> +         || (curProg->numArithInstr[curProg->cur_pass >> 1] == 0)) {
> 
> I lost the debate about where the || (or &&) should go... it should be
> on the previous line.  Most of the parenthesis are unnecessary, and the
> second line should line up with the opening (.
> 
> On a side topic... if anyone understands how
> ati_fragment_shader::cur_pass works, it would be really great if they
> could document it in mtypes.h.

This just indicates which pass is currently being specified. Some
instructions will trigger a new pass, some instructions are only valid
in the first or second pass and so on, and you can have a maximum of 2
passes.
I guess it's a bit awkward how this needs to work due to the shader
being specified bit-by-bit rather than all at once, but the actual
concept is very similar to the multiple phases of d3d9 and r300 (and
didn't i915 have something similar too). Of course, if you translate it
away (on everything but r200) then only the error checking aspect of it
really matters in the end.

FWIW the patches all look correct to me, apparently there were quite
some errors in there (I think it was mostly verified with doom3 at that
time...).

Roland


> 
>>        if (curProg->numArithInstr[curProg->cur_pass >> 1] > 7) {
>>  	 _mesa_error(ctx, GL_INVALID_OPERATION, "C/AFragmentOpATI(instrCount)");
>>  	 return;
>>
> 
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> https://urldefense.proofpoint.com/v2/url?u=https-3A__lists.freedesktop.org_mailman_listinfo_mesa-2Ddev&d=DwIGaQ&c=uilaK90D4TOVoH58JNXRgQ&r=_QIjpv-UJ77xEQY8fIYoQtr5qv8wKrPJc7v7_-CYAb0&m=E021TggYOgVksqpASyumbrMCSKMsRIh5__J3WER0vyw&s=ek3znIpxIGsT2v0AG80jt9petuMBJvX1nXseAg81aYA&e=
>