[Piglit] [PATCH 2/2] glx: add test for GLX_ARB_create_context_no_error

[Adam Jackson]

On Thu, 2019-02-07 at 14:53 -0800, Eric Anholt wrote:
> Adam Jackson <ajax at 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.

- ajax