[Mesa-dev] [PATCH v2 8/8] mesa: rework the meaning of gl_debug_message::length

[Emil Velikov]

On 26 November 2015 at 05:26, Timothy Arceri
<timothy.arceri at collabora.com> wrote:
> On Thu, 2015-11-26 at 00:36 +0000, Emil Velikov wrote:
>> Currently it stores strlen(buf) whenever the user originally provided
>> a
>> negative value for length.
>>
>> Although I've not seen any explicit text in the spec, CTS requires
>> that
>> the very same length (be that negative value or not) is returned back
>> on
>> Pop.
>>
>> So let's push down the length < 0 checks, tweak the meaning of
>> gl_debug_message::length and fix GetDebugMessageLog to add and count
>> the
>> null terminators, as required by the spec.
>>
>> v2: return correct total length in GetDebugMessageLog
>>
>> XXX:
>>  - should we use -1, or strlen on ENOMEM in debug_message_store ?
>>  - split out the GetDebugMessageLog fixes into separate patch ?
>>  - split the "push the length < 0 check further down" into separate
>> patch ?
>>
>> Signed-off-by: Emil Velikov <emil.l.velikov at gmail.com>
>> ---
>>  src/mesa/main/errors.c | 43 ++++++++++++++++++++++++----------------
>> ---
>>  1 file changed, 24 insertions(+), 19 deletions(-)
>>
>> diff --git a/src/mesa/main/errors.c b/src/mesa/main/errors.c
>> index 79149a9..78d21cc 100644
>> --- a/src/mesa/main/errors.c
>> +++ b/src/mesa/main/errors.c
>> @@ -76,6 +76,8 @@ struct gl_debug_message
>>     enum mesa_debug_type type;
>>     GLuint id;
>>     enum mesa_debug_severity severity;
>> +   /* length as given by the user - if message was explicitly null
>> terminated,
>> +    * length can be negative */
>>     GLsizei length;
>>     GLcharARB *message;
>>  };
>> @@ -211,14 +213,19 @@ debug_message_store(struct gl_debug_message
>> *msg,
>>                      enum mesa_debug_severity severity,
>>                      GLsizei len, const char *buf)
>>  {
>> +   GLsizei length = len;
>> +
>>     assert(!msg->message && !msg->length);
>>
>> -   msg->message = malloc(len+1);
>> +   if (length < 0)
>> +      length = strlen(buf);
>> +
>> +   msg->message = malloc(length+1);
>>     if (msg->message) {
>> -      (void) strncpy(msg->message, buf, (size_t)len);
>> -      msg->message[len] = '\0';
>> +      (void) strncpy(msg->message, buf, (size_t)length);
>> +      msg->message[length] = '\0';
>>
>> -      msg->length = len+1;
>> +      msg->length = len;
>>        msg->source = source;
>>        msg->type = type;
>>        msg->id = id;
>> @@ -229,7 +236,7 @@ debug_message_store(struct gl_debug_message *msg,
>>
>>        /* malloc failed! */
>>        msg->message = out_of_memory;
>> -      msg->length = strlen(out_of_memory)+1;
>> +      msg->length = -1;
>
> Why change this?
>
Because negative length now means "message is a null terminated
string". This isn't a user provided string so it shouldn't matter. I'd
rather keep things consistent though :-)

>>        msg->source = MESA_DEBUG_SOURCE_OTHER;
>>        msg->type = MESA_DEBUG_TYPE_ERROR;
>>        msg->id = oom_msg_id;
>> @@ -607,7 +614,7 @@ debug_log_message(struct gl_debug_state *debug,
>>     GLint nextEmpty;
>>     struct gl_debug_message *emptySlot;
>>
>> -   assert(len >= 0 && len < MAX_DEBUG_MESSAGE_LENGTH);
>> +   assert(len < MAX_DEBUG_MESSAGE_LENGTH);
>>
>>     if (log->NumMessages == MAX_DEBUG_LOGGED_MESSAGES)
>>        return;
>> @@ -1004,8 +1011,6 @@ _mesa_DebugMessageInsert(GLenum source, GLenum
>> type, GLuint id,
>>     if (!validate_params(ctx, INSERT, callerstr, source, type,
>> severity))
>>        return; /* GL_INVALID_ENUM */
>>
>> -   if (length < 0)
>> -      length = strlen(buf);
>
> This should be in the previous patch.
>
>>     if (!validate_length(ctx, callerstr, length, buf))
>>        return; /* GL_INVALID_VALUE */
>>
>> @@ -1047,23 +1052,28 @@ _mesa_GetDebugMessageLog(GLuint count,
>> GLsizei logSize, GLenum *sources,
>>
>>     for (ret = 0; ret < count; ret++) {
>>        const struct gl_debug_message *msg =
>> debug_fetch_message(debug);
>> +      GLsizei len;
>>
>>        if (!msg)
>>           break;
>>
>> -      if (logSize < msg->length && messageLog != NULL)
>> +      len = msg->length;
>> +      if (len < 0)
>> +         len = strlen(msg->message);
>> +
>> +      if (logSize < len+1 && messageLog != NULL)
>>           break;
>>
>>        if (messageLog) {
>> -         assert(msg->message[msg->length-1] == '\0');
>> -         (void) strncpy(messageLog, msg->message, (size_t)msg
>> ->length);
>> +         assert(msg->message[len] == '\0');
>> +         (void) strncpy(messageLog, msg->message, (size_t)len+1);
>>
>> -         messageLog += msg->length;
>> -         logSize -= msg->length;
>> +         messageLog += len+1;
>> +         logSize -= len+1;
>>        }
>>
>>        if (lengths)
>> -         *lengths++ = msg->length;
>> +         *lengths++ = len+1;
>>        if (severities)
>>           *severities++ = debug_severity_enums[msg->severity];
>>        if (sources)
>> @@ -1173,8 +1183,6 @@ _mesa_PushDebugGroup(GLenum source, GLuint id,
>> GLsizei length,
>>        return;
>>     }
>>
>> -   if (length < 0)
>> -      length = strlen(message);
>
> This should be in the previous patch.
>
Doing this without the addition [of similar hunk] in
debug_message_store() will break things badly. If we pull the latter
into the previous patch then we'll bust _mesa_GetDebugMessageLog. I
want to split things up, but I cannot see a way that doesn't break
things. Any ideas ?

>>     if (!validate_length(ctx, callerstr, length, message))
>>        return; /* GL_INVALID_VALUE */
>>
>> @@ -1626,9 +1634,6 @@ _mesa_shader_debug( struct gl_context *ctx,
>> GLenum type, GLuint *id,
>>
>>     debug_get_id(id);
>>
>> -   if (len < 0)
>> -      len = strlen(msg);
>
> This should be in a separate patch. Also we could just change
>  _mesa_shader_debug to only pass the message and not the length an
> change this to to get the string length There seems to be only a single
> caller to this and its doing.
>
> _mesa_shader_debug(ctx, type, &msg_id, msg, strlen(msg));
>
_mesa_shader_debug is used in different context, so it makes sense to
have a slightly different declaration.
We can do this just before this series - I'll send a patch later on today.

-Emil