On 26 November 2015 at 05:26, Timothy Arceri
<timothy.arc...@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.veli...@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
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
