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

[Ian Romanick]

On 09/04/2013 05:43 PM, Timothy Arceri wrote:
>>> +/**
> 
>>> + * 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.

Yeah, I noticed that there were some v2 and v3 patches on the list.  If
you're using git-send-email, you can use --in-reply-to to make v2 and v3
patches show up as replies to the originals.  This usually works well
you're sending out updates to individual patches (as opposed to
re-sending the whole series).  Not everyone does this, but it does make
it harder for reviewers to accidentally review old patches.

>>> +         /* 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.

Which behavior are you say AMD has?  Generate the error or truncate the
string?  Any idea what NVIDIA does?  If both AMD and NVIDIA do the same
thing that's different from what the spec says, I typically submit a
spec bug.

Assuming that at least one of them follows the spec, I was suggesting we
do one of two things:

A. Set MAX_LABEL_LENGTH to the minimum required by the spec, and include
some method to statically allocate a buffer of that size for every
object.  I looked at patch 7, and there are a couple of ways to do that.
 I suspect that AMD puts 'char Label[0];' at the end of their data
structures.  If it's a debug context, they just allocate and extra 256
bytes for each of their structures.

B. Set MAX_LABEL_LENGTH so large, such as MAX_INT (0x7fffffff) that it
is impossible to have a value that is larger (due to the limited range
of GLsizei).  Eliminate the checks for length < MAX_LABEL_LENGTH.

After working through the mental exercise of A, I'm not very excited
about it.  It saves a pointer in every data structure in non-debug
contexts, but I'm not sure it's worth it.  Hmm... since the patch that
landed includes the missing check, it's probably not worth B either.  We
haven't done any intense cache analysis of Mesa data structures to even
know if having that extra pointer in the middle of the structure will
cause performance problems.  Let's avoid the evil of premature
optimization and leave the code as-is.

>>> +            /* 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?

I don't think reverting a bunch of stuff is going to help at this
point... and it will probably discourage you even further.  That also
won't help.

After replying to your e-mail about patch 2, I think any significant
work should wait until there is a conclusion from Khronos.  It would be
truly awful to re-work the patches only to find that everyone else
treats the KHR and ARB entry points distinctly.

In the mean time, we should land Brian's patch to fix 'make check'.
Patches to fix coding standards issues and add some comments are also in
order.