[waffle] [PATCH v2 1/4] waffle utils: add wflinfo utility

[Chad Versace]

On Mon, Dec 23, 2013 at 09:48:33PM -0800, Jordan Justen wrote:
> glxinfo for waffle, but using the command line interface
> of waffle's gl_basic test.
> 
> v2:
>  * Various cleanups suggested by Chad
>  * Print context flags rather than verifying them.
>  * Only print extensions when --verbose is used
> 
> Signed-off-by: Jordan Justen <jordan.l.justen at intel.com>
> ---
> 
> Note: This has only been tested with platforms gbm, glx and x11_egl.
> It has only been tested with the Intel Mesa driver.

Regarding Mac support... When review finishes on this patch, I'll commit
it after verifying it doesn't break the Mac build.  Then I'll fix any
Mac bugs myself in follow-up patches.

[snip]

> +static void __attribute__((noreturn))
> +error_printf(const char *fmt, ...)
> +{
> +    va_list ap;
> +
> +    fflush(stdout);
> +
> +    va_start(ap, fmt);
> +    fprintf(stderr, "wflinfo: error: ");
> +    vfprintf(stderr, fmt, ap);
> +    fprintf(stderr, "\n");
> +    va_end(ap);
> +
> +    exit(EXIT_FAILURE);
> +}

Need a newline here between functions.

> +static void __attribute__((noreturn))
> +usage_error_printf(const char *fmt, ...)
> +{
> +    fflush(stdout);
> +    fprintf(stderr, "wflinfo: usage error");
> +
> +    if (fmt) {
> +        va_list ap;
> +        va_start(ap, fmt);
> +        fprintf(stderr, ": ");
> +        vfprintf(stderr, fmt, ap);
> +        va_end(ap);
> +    }
> +
> +    fprintf(stderr, "\n");
> +    fprintf(stderr, "\n");
> +    fprintf(stderr, "%s", usage_message);
> +
> +    exit(EXIT_FAILURE);
> +}

[snip]

> +static void
> +print_extensions(bool use_stringi)
> +{
> +    GLint count = 0, i;
> +    const char *ext;
> +
> +    printf("OpenGL extensions: ");
> +    if (use_stringi) {
> +        glGetIntegerv(GL_NUM_EXTENSIONS, &count);
> +        if (glGetError() != GL_NO_ERROR) {
> +            printf("GL ERROR");

The space in "GL ERROR" could confuse applications that parse wflinfo's
output, because extension names do not contain spaces. Let's instead
print "GL_ERROR".

Also, this "GL_ERROR" lacks a trailing newline, the lack of which will
corrupt the next line of output.

To ensure the presence of a newline, I think it best to place
`printf("\n")` at the bottom of the function.  This requires that puts()
be replaced by printf() and that the "\n" be removed from
`printf("%s%s...)`. Of course, there are other ways to ensure the
newline; choose whivever method you think best.

> +        } else {
> +            for (i = 0; i < count; i++) {
> +              ext = (const char *) glGetStringi(GL_EXTENSIONS, i);
> +              if (glGetError() != GL_NO_ERROR)
> +                  ext = "GL ERROR";

Same as above regarding s/GL ERROR/GL_ERROR/.

> +              printf("%s%s", ext, (i + 1) < count ? " " : "\n");
> +            }
> +        }
> +    } else {
> +        const char *extensions = (const char *) glGetString(GL_EXTENSIONS);
> +        if (glGetError() != GL_NO_ERROR)
> +            printf("GL ERROR");

Same as above regarding s/GL ERROR/GL_ERROR/.

> +        else
> +            puts(extensions);
> +    }
> +}
> +
> +static void
> +print_context_flags(void)
> +{
> +    static struct {
> +        GLint flag;
> +        char *str;
> +    } flags[] = {
> +        { GL_CONTEXT_FLAG_FORWARD_COMPATIBLE_BIT, "FORWARD_COMPATIBLE" },
> +        { GL_CONTEXT_FLAG_DEBUG_BIT, "DEBUG" },
> +        { GL_CONTEXT_FLAG_ROBUST_ACCESS_BIT_ARB, "ROBUST_ACCESS" },
> +    };
> +    int flag_count = sizeof(flags) / sizeof(flags[0]);
> +    GLint context_flags = 0;
> +
> +    glGetIntegerv(GL_CONTEXT_FLAGS, &context_flags);

A convention throughout waffle's code is to avoid excessive indentation
by returning early on errors and moving the normal codepath out of the
if-block. Please do that here on glGetError(), because the main portion
of this function unnecessarily resides in an if-block.

> +    if (glGetError() == GL_NO_ERROR) {
> +        printf("OpenGL context flags:");
> +        if (context_flags == 0) {
> +            printf(" (none)\n");
> +            return;
> +        }
> +
> +        for (int i = 0; i < flag_count; i++) {
> +            if ((flags[i].flag & context_flags) != 0) {
> +                printf(" %s", flags[i].str);
> +                context_flags = context_flags & ~flags[i].flag;
> +            }
> +        }
> +        for (int i = 0; context_flags != 0; context_flags >>= 1, i++) {
> +            if ((context_flags & 1) != 0) {
> +                printf(" 0x%x", 1 << i);
> +            }
> +        }
> +        printf("\n");
> +    }
> +}
> +
> +/// @brief Print out information about the context that was created.
> +static bool
> +print_wflinfo(struct options *opts)
> +{
> +

Remove the above extra newline after the opening brace.

> +    while(glGetError() != GL_NO_ERROR)
> +        /* Clear all errors */;

Ooh! That dangling ';' is just waiting to become a bug. Please surround
the while-body with braces to prevent future bugs.

    while (glGetError() != GL_NO_ERROR) {
        /* Clear all errors */
    }

> +
> +    const char *vendor = (const char *) glGetString(GL_VENDOR);
> +    if (glGetError() != GL_NO_ERROR || vendor == NULL)
> +        vendor = "GL ERROR";
> +
> +    const char *renderer = (const char *) glGetString(GL_RENDERER);
> +    if (glGetError() != GL_NO_ERROR || renderer == NULL)
> +        renderer = "GL ERROR";
> +
> +    const char *version_str = (const char *) glGetString(GL_VERSION);
> +    if (glGetError() != GL_NO_ERROR || version_str == NULL)
> +        version_str = "GL ERROR";
> +
> +    int version = parse_version(version_str);
> +
> +    bool need_getstringi =
> +            opts->context_api == WAFFLE_CONTEXT_OPENGL &&
> +            version >= 30;
> +
> +    if (!glGetStringi && need_getstringi)
> +        error_get_gl_symbol("glGetStringi");
> +
> +    printf("OpenGL vendor string: %s\n", vendor);
> +    printf("OpenGL renderer string: %s\n", renderer);
> +    printf("OpenGL version string: %s\n", version_str);

Let's print the above info before failing with
error_get_gl_symbol("glGetStringi"). This info usefully identifies much
about the GL implementation, so let's give it to the user even if we
can't give the user everything they asked for (that is, the extensions).

> +
> +    if (opts->context_api == WAFFLE_CONTEXT_OPENGL && version >= 31) {
> +        print_context_flags();
> +    }

I believe context flags exist elsewhere other then OpenGL >= 3.1.
However, let's postpone fixing that after wflinfo gets committed.
I don't perceive lack of context flags for some contexts as a blocking
issue for this patch.

> +
> +    if (opts->verbose)
> +        print_extensions(need_getstringi);

Hmm... I believe that wflinfo should print extensions by default.
Usually, a verbose flag enables printing non-essential info, but
extensions are not non-essential.  Extensions comprise critical
information about the context, perhaps just as significant as the
context version. 

Why did you choose to print the extensions only with verbose? Let's
discuss it at the office.

> +
> +    return true;
> +}

Below here, I see no issues with the patch.