Re: [Piglit] [PATCH 2/6] shader_runner: Don't call glDeleteProgram on an ARB program.

2015-11-24 Thread Kenneth Graunke 
On Tuesday, November 24, 2015 02:20:10 PM Matt Turner wrote:
> On Wed, Nov 11, 2015 at 4:53 PM, Kenneth Graunke  
> wrote:
> > On Tuesday, November 10, 2015 10:46:19 PM Matt Turner wrote:
> >> I don't feel good about this check, but it is done elsewhere in the same
> >> file ("prog == 0").
> >> ---
> >>  tests/shaders/shader_runner.c | 8 ++--
> >>  1 file changed, 6 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/tests/shaders/shader_runner.c b/tests/shaders/shader_runner.c
> >> index 4597b46..bb17381 100644
> >> --- a/tests/shaders/shader_runner.c
> >> +++ b/tests/shaders/shader_runner.c
> >> @@ -3081,8 +3081,12 @@ piglit_display(void)
> >>   if (piglit_automatic) {
> >>   free_subroutine_uniforms();
> >>   /* Free our resources, useful for valgrinding. */
> >> - glDeleteProgram(prog);
> >> - glUseProgram(0);
> >> + if (prog != 0) {
> >> + glDeleteProgram(prog);
> >> + glUseProgram(0);
> >> + } else {
> >> + glDeleteProgramsARB(1, &prog);
> >> + }
> >>   }
> >>
> >>   return pass ? PIGLIT_PASS : PIGLIT_FAIL;
> >>
> >
> > Wow.  Nasty.
> >
> > Both link_and_use_shaders() [GLSL] and compile_and_bind_program() [ARB]
> > set prog to a non-zero value.  So at first glance this seems utterly bunk.
> > However, compile_and_bind_program() creates a local "GLuint prog" variable
> > that shadows the global one, so it never actually sets this.  Heh.
> >
> > I was about to say this was correct.
> >
> > However, I don't see anything actually initializing the global prog
> > variable to 0...so won't this be using an uninitialized value?  (As
> > would the existing code you patterned this after?)
> 
> Sorry for the late reply -- I apparently didn't read your reply
> carefully enough initially.
> 
> Globals in C are automatically initialized to 0. Given that, I think
> this is okay.

Oh, right.  I think it's correct then.  Still nasty, but functional.

Reviewed-by: Kenneth Graunke 


signature.asc
Description: This is a digitally signed message part.
___
Piglit mailing list
Piglit@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/piglit

Re: [Piglit] [PATCH 2/6] shader_runner: Don't call glDeleteProgram on an ARB program.

2015-11-24 Thread Matt Turner 
On Wed, Nov 11, 2015 at 4:53 PM, Kenneth Graunke  wrote:
> On Tuesday, November 10, 2015 10:46:19 PM Matt Turner wrote:
>> I don't feel good about this check, but it is done elsewhere in the same
>> file ("prog == 0").
>> ---
>>  tests/shaders/shader_runner.c | 8 ++--
>>  1 file changed, 6 insertions(+), 2 deletions(-)
>>
>> diff --git a/tests/shaders/shader_runner.c b/tests/shaders/shader_runner.c
>> index 4597b46..bb17381 100644
>> --- a/tests/shaders/shader_runner.c
>> +++ b/tests/shaders/shader_runner.c
>> @@ -3081,8 +3081,12 @@ piglit_display(void)
>>   if (piglit_automatic) {
>>   free_subroutine_uniforms();
>>   /* Free our resources, useful for valgrinding. */
>> - glDeleteProgram(prog);
>> - glUseProgram(0);
>> + if (prog != 0) {
>> + glDeleteProgram(prog);
>> + glUseProgram(0);
>> + } else {
>> + glDeleteProgramsARB(1, &prog);
>> + }
>>   }
>>
>>   return pass ? PIGLIT_PASS : PIGLIT_FAIL;
>>
>
> Wow.  Nasty.
>
> Both link_and_use_shaders() [GLSL] and compile_and_bind_program() [ARB]
> set prog to a non-zero value.  So at first glance this seems utterly bunk.
> However, compile_and_bind_program() creates a local "GLuint prog" variable
> that shadows the global one, so it never actually sets this.  Heh.
>
> I was about to say this was correct.
>
> However, I don't see anything actually initializing the global prog
> variable to 0...so won't this be using an uninitialized value?  (As
> would the existing code you patterned this after?)

Sorry for the late reply -- I apparently didn't read your reply
carefully enough initially.

Globals in C are automatically initialized to 0. Given that, I think
this is okay.
___
Piglit mailing list
Piglit@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/piglit

Re: [Piglit] [PATCH 2/6] shader_runner: Don't call glDeleteProgram on an ARB program.

2015-11-11 Thread Kenneth Graunke 
On Tuesday, November 10, 2015 10:46:19 PM Matt Turner wrote:
> I don't feel good about this check, but it is done elsewhere in the same
> file ("prog == 0").
> ---
>  tests/shaders/shader_runner.c | 8 ++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/tests/shaders/shader_runner.c b/tests/shaders/shader_runner.c
> index 4597b46..bb17381 100644
> --- a/tests/shaders/shader_runner.c
> +++ b/tests/shaders/shader_runner.c
> @@ -3081,8 +3081,12 @@ piglit_display(void)
>   if (piglit_automatic) {
>   free_subroutine_uniforms();
>   /* Free our resources, useful for valgrinding. */
> - glDeleteProgram(prog);
> - glUseProgram(0);
> + if (prog != 0) {
> + glDeleteProgram(prog);
> + glUseProgram(0);
> + } else {
> + glDeleteProgramsARB(1, &prog);
> + }
>   }
>  
>   return pass ? PIGLIT_PASS : PIGLIT_FAIL;
> 

Wow.  Nasty.

Both link_and_use_shaders() [GLSL] and compile_and_bind_program() [ARB]
set prog to a non-zero value.  So at first glance this seems utterly bunk.
However, compile_and_bind_program() creates a local "GLuint prog" variable
that shadows the global one, so it never actually sets this.  Heh.

I was about to say this was correct.

However, I don't see anything actually initializing the global prog
variable to 0...so won't this be using an uninitialized value?  (As
would the existing code you patterned this after?)


signature.asc
Description: This is a digitally signed message part.
___
Piglit mailing list
Piglit@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/piglit

[Piglit] [PATCH 2/6] shader_runner: Don't call glDeleteProgram on an ARB program.

2015-11-10 Thread Matt Turner 
I don't feel good about this check, but it is done elsewhere in the same
file ("prog == 0").
---
 tests/shaders/shader_runner.c | 8 ++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/tests/shaders/shader_runner.c b/tests/shaders/shader_runner.c
index 4597b46..bb17381 100644
--- a/tests/shaders/shader_runner.c
+++ b/tests/shaders/shader_runner.c
@@ -3081,8 +3081,12 @@ piglit_display(void)
if (piglit_automatic) {
free_subroutine_uniforms();
/* Free our resources, useful for valgrinding. */
-   glDeleteProgram(prog);
-   glUseProgram(0);
+   if (prog != 0) {
+   glDeleteProgram(prog);
+   glUseProgram(0);
+   } else {
+   glDeleteProgramsARB(1, &prog);
+   }
}
 
return pass ? PIGLIT_PASS : PIGLIT_FAIL;
-- 
2.4.9

___
Piglit mailing list
Piglit@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/piglit