[Mesa-dev] [PATCH 10/15] mesa: Implement KHR_debug ObjectLabel functions

[Timothy Arceri]

>> +/**

>> + * Helper for _mesa_ObjectLabel() and _mesa_ObjectPtrLabel().
>> + */
>> +static void
>> +set_label(struct gl_context *ctx, char **labelPtr, const char *label,
>> +          int length, const char *caller)
>> +{
>> +   if (*labelPtr) {
>> +      /* free old label string */
>> +      free(*labelPtr);
>> +   }
>> +
>> +   if (label) {
>> +   /* set new label string */
>> +
>> +      if (length >= 0) {
>
>This should be > 0.  malloc(0) is not portable.
>
>Shouldn't there also be a MAX_LABEL_LENGTH test for this patch?

I think you are reviewing an old version of the patch. New version has that test.

>> +         /* explicit length */
>> +         *labelPtr = (char *) malloc(length);
>> +         if (*labelPtr) {
>> +            memcpy(*labelPtr, label, length);
>> +         }
>> +      }
>> +      else {
>> +         /* null-terminated string */
>> +         int len = strlen(label);
>> +         if (len >= MAX_LABEL_LENGTH) {

>The reason MAX_LABEL_LENGTH exists is so that you can have a fixed-size
>array in your structure (so you don't have to malloc a buffer.  Either
>make a fixed size buffer, or make MAX_LABEL_LENGTH be the maximum size
>representable in a GLsizei (and eliminate this check).

Ok makes sense. However the check is still valid its in the spec. We shouldn't just truncate the string I know for sure that the AMD driver does the same thing. I have posted extensive tests for the objectlabel code on the piglit list.

>> +            /* An INVALID_VALUE error is generated if the number of characters
>> +             * in <label>, excluding the null terminator when <length> is
>> +             * negative, is not less than the value of MAX_LABEL_LENGTH.
>> +             */
>> +            _mesa_error(ctx, GL_INVALID_VALUE,
>> +                        "%s(length=%d, which is not less than "
>> +                        "GL_MAX_LABEL_LENGTH=%d)", caller, length,
>> +                        MAX_LABEL_LENGTH);
>> +            return;
>> +         }
>> +         *labelPtr = _mesa_strdup(label);
>> +      }
>> +   }
>> +}
>> +
>> +/**
>> + * Helper for _mesa_GetObjectLabel() and _mesa_GetObjectPtrLabel().
>> + */
>> +static void
>> +copy_label(char **labelPtr, char *label, int *length, int bufSize)
>> +{
>> +   int labelLen = 0;
>> +
>> +   if (*labelPtr)
>> +      labelLen = strlen(*labelPtr);
>> +
>> +   if (label) {
>
>There should be a spec quote here explaining why this value is returned.
>Other places in OpenGL include the NUL terminator in the length.
>
>   /* The KHR_debug spec says:
>    *
>    *     "The string representation of the message is stored in
>    *     <message> and its length (excluding the null-terminator)
>    *     is stored in <length>"
>    */

ok

So are you going to revert the patchset or do I create a fixup patchset?