[Mesa-dev] A few fixes for the preprocessor for GLSL 1.30

[Kenneth Graunke]

On 09/29/2011 11:00 PM, Carl Worth wrote:
> Eric recentl mentioned to me that when setting the following
> environment variables with current master mesa:
> 
> 	MESA_GL_VERSION_OVERRIDE=3.0
> 	MESA_GLSL_VERSION_OVERRIDE=130
> 
> that 8 piglit preprocessor tests start to fail.
> 
> I've investigated all of these failures and am here attaching a patch
> series for review that fixes 5 of them. In the testing I've done, I
> don't see any changes from this patch series other than the desired
> piglit fixes. But, of course, I'd be happy for any review.

For the series:
Reviewed-by: Kenneth Graunke <kenneth at whitecape.org>

Thanks for fixing these, Carl!

> For the remaining three failures, I believe the tests themselves are
> broken and should be fixed. These fall into two case:
> 
> 1. feature-macros/gl_gragment_precision_high.{frag,vert}
> 
>    In these tests, the actual preprocessor functionality being tested,
>    (that a directive of #version 130 causes the
>    GL_FRAGMENT_PRECISION_HIGH macro to be pre-defined), is working fine.
> 
>    The tests are failing due to the following code:
> 
> 	float f() { return 0; }
> 
>    Our implementation does not currently do implicit promotion here,
>    so compilation fails with a type error:
> 
> 	error: `return' with wrong type int, in function `f' returning
> 	float
> 
>    The GLSL specification is not explicit on whether this promotion
>    should be happening, but other implementations appear to do it. So
>    perhaps the promotion should be added to our implementation.

Odd.  I could've sworn that was allowed (it would make sense), but
you're right: the spec never explicitly allows that, so strictly, it's
forbidden.

GLSL 4.20 and the GL_ARB_shading_language_420pack extension explicitly
add this functionality to the shading language, so we can add it then.

I guess we could technically add GL_ARB_shading_language_420pack support
now, since it only requires GLSL 1.30...it's essentially an extension
that backports some new front-end functionality and clarifications to
older versions of GLSL.

In any case, I'm in favor of changing the test to 0.0 instead of 0.

>    Meanwhile, the test could legitimately be fixed to remove the type
>    error which is a side issue from what the test is actually trying
>    to do.
> 
> 2. if/if-arg-must-be-defined-02.frag
> 
>    This test is trying to elicit an "undefined macro" error with
>    something like the following code:
> 
> 	#if 1
> 	...
> 	#elif UNDEFINED_MACRO
> 	...
> 	#endif
> 
>    I propose changing the test to use "#if 0" instead, in which case
>    the current implementation would pass. It's not actually desirable
>    to generate an error in the above case. Consider a use of a
>    possibly undefined macro:

I agree with your analysis here.  I am in favor of changing the test to
"#if 0", as you suggest.

> 	#if PERHAPS_UNDEFINED < 5
> 	...
> 	#endif
> 
>    In order to protect this use, the user might add something like the
>    following (or similar with #ifndef):
> 
> 	#if ! defined PERHAPS_UNDEFINED
> 	... deal with it ...
> 	#elif PERHAPS_UNDEFINED
> 	...
> 	#endif
> 
>    In this case, the user's expectation is to get no error with this
>    code. Yet this is precisly the formulation the current test case
>    uses to elicit the error.
> 
> I'll be glad to push changes to piglit to fix these cases unless
> someone can argue aginst that.

Nope, sounds good to me.  Thanks!

> Thanks,
> 
> -Carl