[Mesa-dev] [PATCH v2 8/8] mesa: rework the meaning of gl_debug_message::length
Timothy Arceri
timothy.arceri at collabora.com
Fri Nov 27 13:47:05 PST 2015
On Thu, 2015-11-26 at 14:31 +0000, Emil Velikov wrote:
> 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 :-)
Ok makes sense I guess.
>
> > > 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 ?
Right I see the problem now. I dont see anyway around this and its not
a big deal. So with the hunk below removed.
Reviewed-by: Timothy Arceri <timothy.arceri at collabora.com>
>
> > > 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
More information about the mesa-dev
mailing list