[Mesa-dev] [PATCH 4/9] glsl: report errors via GL_ARB_debug_output

[nobled]

On Fri, Apr 13, 2012 at 2:04 PM, Ian Romanick <idr at freedesktop.org> wrote:
> On 04/13/2012 08:51 AM, nobled wrote:
>>
>> ---
>>  src/glsl/glsl_parser_extras.cpp     |   14 ++++++++++++++
>>  src/glsl/standalone_scaffolding.cpp |    6 ++++++
>>  src/glsl/standalone_scaffolding.h   |    4 ++++
>>  3 files changed, 24 insertions(+), 0 deletions(-)
>>
>> diff --git a/src/glsl/glsl_parser_extras.cpp
>> b/src/glsl/glsl_parser_extras.cpp
>> index aade2e1..d6a156d 100644
>> --- a/src/glsl/glsl_parser_extras.cpp
>> +++ b/src/glsl/glsl_parser_extras.cpp
>> @@ -139,13 +139,27 @@ static void
>>  _mesa_glsl_msg(const YYLTYPE *locp, _mesa_glsl_parse_state *state,
>>                 bool error, const char *fmt, va_list ap)
>>  {
>> +   struct gl_context *ctx = state->ctx;
>
>
> Since this is C++ code, I'd suggest moving the declarations down to the
> assignments.
Sure.
>
>
>> +   int msg_offset;
>> +   const char *msg;
>> +
>>     assert(state->info_log != NULL);
>> +
>> +   /* Get the offset that the new message will be written to. */
>> +   msg_offset = strlen(state->info_log);
>> +
>>     ralloc_asprintf_append(&state->info_log, "%u:%u(%u): %s: ",
>>                                            locp->source,
>>                                            locp->first_line,
>>                                            locp->first_column,
>>                                            error ? "error" : "warning");
>>     ralloc_vasprintf_append(&state->info_log, fmt, ap);
>> +
>> +   msg =&state->info_log[msg_offset];
>
>
> So this would become
>
>    const char *const msg = &state->info_log[msg_offset];
>
>
>> +   /* Report the error via GL_ARB_debug_output. */
>> +   if (error)
>> +      _mesa_shader_error(ctx, SHADER_ERROR_UNKNOWN, msg, strlen(msg));
>
>
> Should warnings also be reported?  Maybe use GL_DEBUG_TYPE_OTHER_ARB? We
> don't report that many warnings, so it may not matter that much. Same
> comment in patch 9/9 for the preprocessor warnings.
I only wanted to do the minimum necessary to emit errors for now,
because 'are you sure you meant to write this?' sanity-check warnings
would go in OTHER, but some warnings should go in a more specific
category, which'd require a new (possibly overloaded)
_mesa_glsl_warning*() function to indicate which. Use of an extension
that was set to 'warn' is definitely GL_DEBUG_TYPE_PORTABILITY_ARB,
for example.

>
>
>> +
>>     ralloc_strcat(&state->info_log, "\n");
>>  }
>>
>> diff --git a/src/glsl/standalone_scaffolding.cpp
>> b/src/glsl/standalone_scaffolding.cpp
>> index 24cc64a..99079b1 100644
>> --- a/src/glsl/standalone_scaffolding.cpp
>> +++ b/src/glsl/standalone_scaffolding.cpp
>> @@ -41,6 +41,12 @@ _mesa_reference_shader(struct gl_context *ctx,
>> struct gl_shader **ptr,
>>     *ptr = sh;
>>  }
>>
>> +void
>> +_mesa_shader_error(struct gl_context *, gl_shader_error,
>> +                   const char *, int)
>
>
> Is that valid C++?  I thought you could only leave the names out in the
> prototype.  This makes it a bit odd that the names in included in the
> prototype (in standalone_scaffolding.h, below).
Yeah, this compiles fine. I just left them out to avoid 'unused'
warnings instead of casting with (void).
>
>
>> +{
>> +}
>> +
>>  struct gl_shader *
>>  _mesa_new_shader(struct gl_context *ctx, GLuint name, GLenum type)
>>  {
>> diff --git a/src/glsl/standalone_scaffolding.h
>> b/src/glsl/standalone_scaffolding.h
>> index 8773320..8dedbaa 100644
>> --- a/src/glsl/standalone_scaffolding.h
>> +++ b/src/glsl/standalone_scaffolding.h
>> @@ -40,6 +40,10 @@ _mesa_reference_shader(struct gl_context *ctx,
>> struct gl_shader **ptr,
>>  extern "C" struct gl_shader *
>>  _mesa_new_shader(struct gl_context *ctx, GLuint name, GLenum type);
>>
>> +extern "C" void
>> +_mesa_shader_error(struct gl_context *ctx, gl_shader_error err,
>> +                   const char *msg, int len);
>> +
>>  /**
>>   * Initialize the given gl_context structure to a reasonable set of
>>   * defaults representing the minimum capabilities required by the