Re: [Piglit] [PATCH] arb_query_buffer_object: Fix format-security warning.

[Dylan Baker]

Quoting Vinson Lee (2016-05-10 17:48:07)
> qbo.c: In function ‘run_subtest_and_present’:
> qbo.c:269:2: warning: format not a string literal and no format arguments 
> [-Wformat-security]
>   piglit_report_subtest_result(r, subtest_name);
>   ^
> 
> Signed-off-by: Vinson Lee 
> ---
>  tests/spec/arb_query_buffer_object/qbo.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/tests/spec/arb_query_buffer_object/qbo.c 
> b/tests/spec/arb_query_buffer_object/qbo.c
> index bd47dd2a3989..1bcbe1035fad 100644
> --- a/tests/spec/arb_query_buffer_object/qbo.c
> +++ b/tests/spec/arb_query_buffer_object/qbo.c
> @@ -266,7 +266,7 @@ run_subtest_and_present(void)
> asprintf(_name, "query-%s-%s",
>  piglit_get_gl_enum_name(query_type),
>  sync_mode_names[sync_mode]);
> -   piglit_report_subtest_result(r, subtest_name);
> +   piglit_report_subtest_result(r, "%s", subtest_name);
> free(subtest_name);
> return r;
>  }
> -- 
> 2.7.4
> 
> ___
> Piglit mailing list
> Piglit@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/piglit

Reviewed-by: Dylan Baker 


signature.asc
Description: signature
___
Piglit mailing list
Piglit@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/piglit

Re: [Piglit] [PATCH 2/2] framework/results: Fix subtests masking crashes

[Dylan Baker]

Quoting Ilia Mirkin (2016-05-10 17:45:49)
> On Tue, May 10, 2016 at 8:05 PM, Dylan Baker  wrote:
> > Quoting Ilia Mirkin (2016-05-10 14:32:59)
> >> On Tue, May 10, 2016 at 5:32 PM, Dylan Baker  wrote:
> >> > Quoting Ilia Mirkin (2016-05-10 06:41:33)
> >> >> Shouldn't the test's result just be included into the "max" no matter 
> >> >> what?
> >> >>
> >> >
> >> > Well, the problem is that a lot of tests were retrofitted with subtests,
> >> > and now the overall status is bunk, and for new tests we generally don't
> >> > add an overall result when there's subtests.
> >>
> >> It's surprising if the status of a test says fail but piglit-summary
> >> reports it as pass. In the case where one of the subtests fails but
> >> the overall status is pass, this would still say fail, no?
> >>
> >> What situation are you protecting against by only incorporating the
> >> overall status on crash?
> >
> > I have this gut feeling that says skip or notrun might play havok here.
> > But I guess I should look test it and see if that's the case.
> >
> > The other thing I guess is that there's only about 100 tests (or, c/c++
> > files, really) using subtests. So maybe it would just be easier to audit
> > them to make sure that they do the right thing and just add the overall
> > result to the max call.
> 
> I dunno... I think in at least some cases some "stuff" happens between
> subtests that can fail. I don't think it's correct to look at a test
> as a shell for a bunch of disconnected subtests, and thus anything the
> shell itself does is meaningless. The shell can also fail. Feel free
> to audit everything :)
> 
>   -ilia

You know. Thinking back there was a point where the framework defaulted
to FAIL instead of NOTRUN. This might be an artifact of that time.
Either way I guess just seeing what happens is the easiest way to figure
out if it's going to work or not


signature.asc
Description: signature
___
Piglit mailing list
Piglit@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/piglit

[Piglit] [PATCH] arb_query_buffer_object: Fix format-security warning.

[Vinson Lee]

qbo.c: In function ‘run_subtest_and_present’:
qbo.c:269:2: warning: format not a string literal and no format arguments 
[-Wformat-security]
  piglit_report_subtest_result(r, subtest_name);
  ^

Signed-off-by: Vinson Lee 
---
 tests/spec/arb_query_buffer_object/qbo.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/spec/arb_query_buffer_object/qbo.c 
b/tests/spec/arb_query_buffer_object/qbo.c
index bd47dd2a3989..1bcbe1035fad 100644
--- a/tests/spec/arb_query_buffer_object/qbo.c
+++ b/tests/spec/arb_query_buffer_object/qbo.c
@@ -266,7 +266,7 @@ run_subtest_and_present(void)
asprintf(_name, "query-%s-%s",
 piglit_get_gl_enum_name(query_type),
 sync_mode_names[sync_mode]);
-   piglit_report_subtest_result(r, subtest_name);
+   piglit_report_subtest_result(r, "%s", subtest_name);
free(subtest_name);
return r;
 }
-- 
2.7.4

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

Re: [Piglit] [PATCH 2/2] framework/results: Fix subtests masking crashes

[Ilia Mirkin]

On Tue, May 10, 2016 at 8:05 PM, Dylan Baker  wrote:
> Quoting Ilia Mirkin (2016-05-10 14:32:59)
>> On Tue, May 10, 2016 at 5:32 PM, Dylan Baker  wrote:
>> > Quoting Ilia Mirkin (2016-05-10 06:41:33)
>> >> Shouldn't the test's result just be included into the "max" no matter 
>> >> what?
>> >>
>> >
>> > Well, the problem is that a lot of tests were retrofitted with subtests,
>> > and now the overall status is bunk, and for new tests we generally don't
>> > add an overall result when there's subtests.
>>
>> It's surprising if the status of a test says fail but piglit-summary
>> reports it as pass. In the case where one of the subtests fails but
>> the overall status is pass, this would still say fail, no?
>>
>> What situation are you protecting against by only incorporating the
>> overall status on crash?
>
> I have this gut feeling that says skip or notrun might play havok here.
> But I guess I should look test it and see if that's the case.
>
> The other thing I guess is that there's only about 100 tests (or, c/c++
> files, really) using subtests. So maybe it would just be easier to audit
> them to make sure that they do the right thing and just add the overall
> result to the max call.

I dunno... I think in at least some cases some "stuff" happens between
subtests that can fail. I don't think it's correct to look at a test
as a shell for a bunch of disconnected subtests, and thus anything the
shell itself does is meaningless. The shell can also fail. Feel free
to audit everything :)

  -ilia
___
Piglit mailing list
Piglit@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/piglit

Re: [Piglit] [PATCH 2/2] framework/results: Fix subtests masking crashes

[Dylan Baker]

Quoting Ilia Mirkin (2016-05-10 14:32:59)
> On Tue, May 10, 2016 at 5:32 PM, Dylan Baker  wrote:
> > Quoting Ilia Mirkin (2016-05-10 06:41:33)
> >> Shouldn't the test's result just be included into the "max" no matter what?
> >>
> >
> > Well, the problem is that a lot of tests were retrofitted with subtests,
> > and now the overall status is bunk, and for new tests we generally don't
> > add an overall result when there's subtests.
> 
> It's surprising if the status of a test says fail but piglit-summary
> reports it as pass. In the case where one of the subtests fails but
> the overall status is pass, this would still say fail, no?
> 
> What situation are you protecting against by only incorporating the
> overall status on crash?

I have this gut feeling that says skip or notrun might play havok here.
But I guess I should look test it and see if that's the case.

The other thing I guess is that there's only about 100 tests (or, c/c++
files, really) using subtests. So maybe it would just be easier to audit
them to make sure that they do the right thing and just add the overall
result to the max call.


signature.asc
Description: signature
___
Piglit mailing list
Piglit@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/piglit

Re: [Piglit] [PATCH 2/2] multisample/formats: Check for spec-complient integer resolves

[Matt Turner]

On Wed, May 4, 2016 at 4:23 PM, Jason Ekstrand  wrote:
> The i965 driver has historically done integer multisample resolves with a
> sort of integer averaging operation.  The spec used to not really say
> anything at all about integer formats when discussing multisampling.  Since
> the tests were written, the spec has been substantially clerified.  The new
> rule is that integer MSAA resolves are supposed to pick a single arbitrary
> sample for each texel rather than trying to combine them in any way.
>
> Signed-off-by: Jason Ekstrand 
> ---
>  tests/spec/ext_framebuffer_multisample/formats.cpp | 196 
> -
>  1 file changed, 155 insertions(+), 41 deletions(-)
>
> diff --git a/tests/spec/ext_framebuffer_multisample/formats.cpp 
> b/tests/spec/ext_framebuffer_multisample/formats.cpp
> index 4b1feba..1024b45 100644
> --- a/tests/spec/ext_framebuffer_multisample/formats.cpp
> +++ b/tests/spec/ext_framebuffer_multisample/formats.cpp
> @@ -70,6 +70,44 @@ ColorGradientSunburst *test_pattern_vec4;
>  ColorGradientSunburst *test_pattern_ivec4;
>  ColorGradientSunburst *test_pattern_uvec4;
>
> +struct int_resolve_check_s {
> +   GLuint prog;
> +   GLint u_resolve;
> +   GLint u_msaa;
> +   GLint u_samples;
> +} int_resolve_check;
> +
> +const char int_resolve_check_vs[] =
> +   "#version 130\n"
> +   "in vec4 piglit_vertex;\n"
> +   "void main() {\n"
> +   "   gl_Position = piglit_vertex;\n"
> +   "}\n";
> +
> +/* The OpenGL ES 3.2 and OpenGL 4.4 specs both state:
> + *
> + * "If the source for- mats are integer types or stencil values, a

formats

> + * single sample’s value is selected for each pixel."

smart quote

> + *
> + * This shader checks exactly that condition, namely that the result of the
> + * resolve is exactly one of the sample values in the original image.
> + */
> +const char int_resolve_check_fs[] =
> +   "#version 130\n"
> +   "#extension GL_ARB_texture_multisample: require\n"
> +   "uniform isampler2DMS msaa;\n"
> +   "uniform isampler2D resolve;\n"
> +   "uniform int samples;\n"
> +   "void main() {\n"
> +   "   gl_FragColor = vec4(1.0, 0.0, 0.0, 1.0);\n"
> +   "   const vec4 green = vec4(0.0, 1.0, 0.0, 1.0);\n"
> +   "   ivec2 pos = ivec2(gl_FragCoord.xy);\n"
> +   "   ivec4 res = texelFetch(resolve, pos, 0);\n"
> +   "   for (int s = 0; s < samples; s++) {\n"
> +   "   if (res == texelFetch(msaa, pos, s))\n"
> +   "   gl_FragColor = green;\n"

Could break here

> +   "   }\n"
> +   "}\n";
>
>  /**
>   * This class encapsulates the code necessary to draw the test pattern
> @@ -80,7 +118,7 @@ ColorGradientSunburst *test_pattern_uvec4;
>  class PatternRenderer
>  {
>  public:
> -   bool try_setup(GLenum internalformat);
> +   bool try_setup(GLenum internalformat, bool is_integer);
> void set_piglit_tolerance();
> void set_color_clamping_mode();
> void draw();
> @@ -140,11 +178,18 @@ public:
>   * incomplete.
>   */
>  bool
> -PatternRenderer::try_setup(GLenum internalformat)
> +PatternRenderer::try_setup(GLenum internalformat, bool is_integer)
>  {
> FboConfig config_downsampled(0, pattern_width, pattern_height);
> config_downsampled.color_internalformat = internalformat;
>
> +   if (is_integer) {
> +   config_downsampled.color_format = GL_RGBA_INTEGER;
> +   config_downsampled.num_rb_attachments = 0;
> +   config_downsampled.num_tex_attachments = 1;
> +   config_downsampled.use_rect = false;
> +   }
> +
> FboConfig config_msaa = config_downsampled;
> config_msaa.num_samples = num_samples;
>
> @@ -497,7 +542,9 @@ test_format(const struct format_desc *format)
>  * supported, we might have received a GL error.  In either
>  * case just skip to the next format.
>  */
> -   bool setup_success = test_renderer.try_setup(format->internalformat);
> +   bool is_integer = (test_sets[test_index].basetype == GL_INT);
> +   bool setup_success = test_renderer.try_setup(format->internalformat,
> +is_integer);
> if (glGetError() != GL_NO_ERROR) {
> printf("Error setting up test renderbuffers\n");
> return PIGLIT_SKIP;
> @@ -511,7 +558,8 @@ test_format(const struct format_desc *format)
>  * This shouldn't fail.
>  */
> setup_success = ref_renderer.try_setup(test_renderer.is_srgb ?
> -  GL_SRGB8_ALPHA8 : GL_RGBA);
> +  GL_SRGB8_ALPHA8 : GL_RGBA,
> +  false);
> if (!piglit_check_gl_error(GL_NO_ERROR)) {
> printf("Error setting up reference renderbuffers\n");
>

Re: [Piglit] [PATCH 2/2] framework/results: Fix subtests masking crashes

[Ilia Mirkin]

On Tue, May 10, 2016 at 5:32 PM, Dylan Baker  wrote:
> Quoting Ilia Mirkin (2016-05-10 06:41:33)
>> Shouldn't the test's result just be included into the "max" no matter what?
>>
>
> Well, the problem is that a lot of tests were retrofitted with subtests,
> and now the overall status is bunk, and for new tests we generally don't
> add an overall result when there's subtests.

It's surprising if the status of a test says fail but piglit-summary
reports it as pass. In the case where one of the subtests fails but
the overall status is pass, this would still say fail, no?

What situation are you protecting against by only incorporating the
overall status on crash?
___
Piglit mailing list
Piglit@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/piglit

Re: [Piglit] [PATCH 2/2] framework/results: Fix subtests masking crashes

[Dylan Baker]

Quoting Ilia Mirkin (2016-05-10 06:41:33)
> Shouldn't the test's result just be included into the "max" no matter what?
> 
> On Mon, May 9, 2016 at 5:46 PM, Dylan Baker  wrote:
> > In piglit when a test generates subtests we treat the "worst" subtest as
> > the test status, and in most cases this works as expected. There is at
> > least one case where this is not correct, and that is the case of crash.
> >
> > There are a couple of reasons that crash should not be masked. One is
> > that it is generated by the framework when the test binary hits an
> > assert or segfaults, or any number of similar cases. The second is that
> > it may mean that all of the subtests did not run, as such we don't want
> > the status to be masked by the "worst" subtest which would be fail at
> > worst.
> >
> > Signed-off-by: Dylan Baker 
> > ---
> >
> >  framework/results.py | 7 +--
> >  1 file changed, 5 insertions(+), 2 deletions(-)
> >
> > diff --git a/framework/results.py b/framework/results.py
> > index 469abeb..2095d90 100644
> > --- a/framework/results.py
> > +++ b/framework/results.py
> > @@ -173,10 +173,13 @@ class TestResult(object):
> >  """Return the result of the test.
> >
> >  If there are subtests return the "worst" value of those subtests. 
> > If
> > -there are not return the stored value of the test.
> > +there are not return the stored value of the test. There is an
> > +exception to this rule, and that's if the status is crash; since 
> > this
> > +status is set by the framework, and can be generated even when 
> > some or
> > +all unit tests pass.
> >
> >  """
> > -if self.subtests:
> > +if self.subtests and self.__result != status.CRASH:
> >  return max(six.itervalues(self.subtests))
> >  return self.__result
> >
> > --
> > 2.8.2
> >
> > ___
> > Piglit mailing list
> > Piglit@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/piglit

Well, the problem is that a lot of tests were retrofitted with subtests,
and now the overall status is bunk, and for new tests we generally don't
add an overall result when there's subtests.

Dylan


signature.asc
Description: signature
___
Piglit mailing list
Piglit@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/piglit

Re: [Piglit] [PATCH] arb_gpu_shader_fp64: Adds conversion tests generator

[Antía Puentes]

On jue, 2016-04-14 at 12:40 +0300, Andres Gomez wrote:
> Also, removed 5 redundant tests replaced by
> the generator.
> 
> Signed-off-by: Andres Gomez 
> ---
>  generated_tests/CMakeLists.txt |  24 +
>  generated_tests/gen_conversion_fp64.py | 604
> +
>  .../templates/gen_conversion_fp64/base.mako|  12 +
>  .../gen_conversion_fp64/compiler.frag.mako |   3 +
>  .../gen_conversion_fp64/compiler.geom.mako |   3 +
>  .../gen_conversion_fp64/compiler.vert.mako |   3 +
>  .../gen_conversion_fp64/compiler_base.mako |  25 +
>  .../execution-zero-sign.frag.shader_test.mako  |   8 +
>  .../execution-zero-sign.geom.shader_test.mako  |  27 +
>  .../execution-zero-sign.vert.shader_test.mako  |  16 +
>  .../execution.frag.shader_test.mako|   7 +
>  .../execution.geom.shader_test.mako|  27 +
>  .../execution.vert.shader_test.mako|  16 +
>  .../gen_conversion_fp64/execution_base.mako|  28 +
>  .../gen_conversion_fp64/shader-zero-sign.frag.mako |  18 +
>  .../gen_conversion_fp64/shader-zero-sign.geom.mako |  27 +
>  .../gen_conversion_fp64/shader-zero-sign.vert.mako |  20 +
>  .../templates/gen_conversion_fp64/shader.frag.mako |  16 +
>  .../templates/gen_conversion_fp64/shader.geom.mako |  25 +
>  .../templates/gen_conversion_fp64/shader.vert.mako |  18 +
>  .../templates/gen_conversion_fp64/shader_base.mako |  11 +
>  .../implicit-conversion-double-float-bad.vert  |  20 -
>  .../implicit-conversion-dvec2-vec2-bad.vert|  20 -
>  .../implicit-conversion-dvec3-vec3-bad.vert|  20 -
>  .../implicit-conversion-dvec4-vec4-bad.vert|  20 -
>  .../compiler/implicit-conversions.vert | 115 
>  26 files changed, 938 insertions(+), 195 deletions(-)
>  create mode 100644 generated_tests/gen_conversion_fp64.py
>  create mode 100644
> generated_tests/templates/gen_conversion_fp64/base.mako
>  create mode 100644
> generated_tests/templates/gen_conversion_fp64/compiler.frag.mako
>  create mode 100644
> generated_tests/templates/gen_conversion_fp64/compiler.geom.mako
>  create mode 100644
> generated_tests/templates/gen_conversion_fp64/compiler.vert.mako
>  create mode 100644
> generated_tests/templates/gen_conversion_fp64/compiler_base.mako
>  create mode 100644
> generated_tests/templates/gen_conversion_fp64/execution-zero-
> sign.frag.shader_test.mako
>  create mode 100644
> generated_tests/templates/gen_conversion_fp64/execution-zero-
> sign.geom.shader_test.mako
>  create mode 100644
> generated_tests/templates/gen_conversion_fp64/execution-zero-
> sign.vert.shader_test.mako
>  create mode 100644
> generated_tests/templates/gen_conversion_fp64/execution.frag.shader_t
> est.mako
>  create mode 100644
> generated_tests/templates/gen_conversion_fp64/execution.geom.shader_t
> est.mako
>  create mode 100644
> generated_tests/templates/gen_conversion_fp64/execution.vert.shader_t
> est.mako
>  create mode 100644
> generated_tests/templates/gen_conversion_fp64/execution_base.mako
>  create mode 100644
> generated_tests/templates/gen_conversion_fp64/shader-zero-
> sign.frag.mako
>  create mode 100644
> generated_tests/templates/gen_conversion_fp64/shader-zero-
> sign.geom.mako
>  create mode 100644
> generated_tests/templates/gen_conversion_fp64/shader-zero-
> sign.vert.mako
>  create mode 100644
> generated_tests/templates/gen_conversion_fp64/shader.frag.mako
>  create mode 100644
> generated_tests/templates/gen_conversion_fp64/shader.geom.mako
>  create mode 100644
> generated_tests/templates/gen_conversion_fp64/shader.vert.mako
>  create mode 100644
> generated_tests/templates/gen_conversion_fp64/shader_base.mako
>  delete mode 100644 tests/spec/arb_gpu_shader_fp64/compiler/implicit-
> conversion-double-float-bad.vert
>  delete mode 100644 tests/spec/arb_gpu_shader_fp64/compiler/implicit-
> conversion-dvec2-vec2-bad.vert
>  delete mode 100644 tests/spec/arb_gpu_shader_fp64/compiler/implicit-
> conversion-dvec3-vec3-bad.vert
>  delete mode 100644 tests/spec/arb_gpu_shader_fp64/compiler/implicit-
> conversion-dvec4-vec4-bad.vert
>  delete mode 100644 tests/spec/arb_gpu_shader_fp64/compiler/implicit-
> conversions.vert
> 
> diff --git a/generated_tests/CMakeLists.txt
> b/generated_tests/CMakeLists.txt
> index 3c5b11a..c2505e2 100644
> --- a/generated_tests/CMakeLists.txt
> +++ b/generated_tests/CMakeLists.txt
> @@ -130,6 +130,29 @@ piglit_make_generated_tests(
>   templates/gen_flat_interpolation_qualifier/template.frag.mak
> o
>   )
>  piglit_make_generated_tests(
> + conversion_fp64.list
> + gen_conversion_fp64.py
> + templates/gen_conversion_fp64/base.mako
> + templates/gen_conversion_fp64/compiler.frag.mako
> + templates/gen_conversion_fp64/compiler.geom.mako
> + templates/gen_conversion_fp64/compiler.vert.mako
> + templates/gen_conversion_fp64/compiler_base.mako
> + 

Re: [Piglit] [PATCH 2/2] framework/results: Fix subtests masking crashes

[Ilia Mirkin]

Shouldn't the test's result just be included into the "max" no matter what?

On Mon, May 9, 2016 at 5:46 PM, Dylan Baker  wrote:
> In piglit when a test generates subtests we treat the "worst" subtest as
> the test status, and in most cases this works as expected. There is at
> least one case where this is not correct, and that is the case of crash.
>
> There are a couple of reasons that crash should not be masked. One is
> that it is generated by the framework when the test binary hits an
> assert or segfaults, or any number of similar cases. The second is that
> it may mean that all of the subtests did not run, as such we don't want
> the status to be masked by the "worst" subtest which would be fail at
> worst.
>
> Signed-off-by: Dylan Baker 
> ---
>
>  framework/results.py | 7 +--
>  1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/framework/results.py b/framework/results.py
> index 469abeb..2095d90 100644
> --- a/framework/results.py
> +++ b/framework/results.py
> @@ -173,10 +173,13 @@ class TestResult(object):
>  """Return the result of the test.
>
>  If there are subtests return the "worst" value of those subtests. If
> -there are not return the stored value of the test.
> +there are not return the stored value of the test. There is an
> +exception to this rule, and that's if the status is crash; since this
> +status is set by the framework, and can be generated even when some 
> or
> +all unit tests pass.
>
>  """
> -if self.subtests:
> +if self.subtests and self.__result != status.CRASH:
>  return max(six.itervalues(self.subtests))
>  return self.__result
>
> --
> 2.8.2
>
> ___
> Piglit mailing list
> Piglit@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/piglit
___
Piglit mailing list
Piglit@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/piglit