Adam Jackson <a...@redhat.com> writes:
> On Thu, 2019-02-07 at 14:53 -0800, Eric Anholt wrote:
>> Adam Jackson <a...@redhat.com> writes:
>>
>> > +static void
>> > +fold_results(enum piglit_result *a, enum piglit_result b)
>> > +{
>> > + if (*a == PIGLIT_FAIL || b == PIGLIT_FAIL)
>> > + *a = PIGLIT_FAIL;
>> > + else if (*a == PIGLIT_PASS || b == PIGLIT_PASS)
>> > + *a = PIGLIT_PASS;
>> > + else
>> > + *a = PIGLIT_SKIP;
>> > +}
>>
>> This is just piglit_merge_result()
>
> Indeed.
>
>> > +static enum piglit_result check_no_error(bool flag, bool debug, bool
>> > robust)
>> > +{
>> > + int ctx_flags = 0;
>> > + ctx_flags |= debug ? GLX_CONTEXT_DEBUG_BIT_ARB : 0;
>> > + ctx_flags |= robust ? GLX_CONTEXT_ROBUST_ACCESS_BIT_ARB : 0;
>> > + const int attribs[] = {
>> > + GLX_CONTEXT_MAJOR_VERSION_ARB, 2,
>> > + GLX_CONTEXT_MINOR_VERSION_ARB, 0,
>> > + GLX_CONTEXT_OPENGL_NO_ERROR_ARB, flag,
>> > + GLX_CONTEXT_FLAGS_ARB, ctx_flags,
>> > + None
>> > + };
>> > + static bool is_dispatch_init = false;
>> > + GLXContext ctx;
>> > + enum piglit_result pass = PIGLIT_SKIP;
>> > +
>> > + printf("info: no_error=%s, debug=%s, robustness=%s\n",
>> > + BOOLSTR(flag), BOOLSTR(debug), BOOLSTR(robust));
>> > +
>> > + ctx = glXCreateContextAttribsARB(dpy, fbconfig, NULL, True, attribs);
>> > + XSync(dpy, 0);
>>
>> Needs to check that we have robusness or debug and skip if they're
>> missing.
>
> There's not a separate extension string for debug, it's part of
> GLX_ARB_create_context. And asking for a debug context is allowed to be
> a no-op, so you can't even query the context bit afterward. True enough
> about robustness though.
>
>> > + /* The number of texture units is a small, unsigned number. Craft an
>> > + * illegal call by using a very large number that should fail on any
>> > + * OpenGL implementation in practice.
>> > + */
>> > + glActiveTexture(-1);
>> > + if (glGetError() != 0 && flag) {
>> > + printf("error: error observed with KHR_no_error enabled\n");
>> > + pass = PIGLIT_FAIL;
>> > + goto done;
>> > + }
>>
>> I'm a reluctant to trigger undefined behavior that allows anything
>> including application termination in a testcase.
>
> Yeah, this seems like a mistake. Removed.
>
>> > +int main(int argc, char **argv)
>> > +{
>> > + enum piglit_result pass = PIGLIT_SKIP;
>> > +
>> > + GLX_ARB_create_context_setup();
>> > + piglit_require_glx_extension(dpy, "GLX_ARB_create_context_no_error");
>> > +
>> > + /* "Control group": Check that errors are indeed generated without
>> > + * KHR_no_error enabled. */
>> > + fold_results(&pass, check_no_error(false, false, false));
>> > + fold_results(&pass, check_no_error(false, false, false));
>> > + fold_results(&pass, check_no_error(false, true, false));
>> > + fold_results(&pass, check_no_error(false, true, false));
>>
>> We don't actually verify that errors are generated without KHR_no_error
>> -- the checks are under if (flag). However, the rest of piglit covers
>> that, so let's just delete these cases and drop the first arg to
>> check_no_error.
>
> Will do.
>
>> > +
>> > + /* Check that KHR_no_error gets enabled and its interaction with debug
>> > and
>> > + * robustness context flags. */
>> > + fold_results(&pass, check_no_error(true, false, false));
>> > + fold_results(&pass, check_no_error(true, false, false));
>> > + fold_results(&pass, check_no_error(true, true, false));
>> > + fold_results(&pass, check_no_error(true, true, false));
>> > + fold_results(&pass, check_no_error(true, false, true));
>> > + fold_results(&pass, check_no_error(true, true, true));
>>
>> Looks like there are 2 duplicated cases here.
>
> Will do this too.
>
> The above review seems to apply equally to the EGL test (which also has
> at least one logic error). Will resubmit both with the above feedback
> addressed.Sounds good!
signature.asc
Description: PGP signature
_______________________________________________ Piglit mailing list Piglit@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/piglit
