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

[Timothy Arceri]

On Wed, 2013-09-04 at 20:09 -0700, Ian Romanick wrote:
> 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.

ok thanks I was wondering how you guys manage to keep track of
everything.

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

The AMD driver generates the error. I don't have a Nvidia card so no
idea what they do. If someone with a Nvidia card feels like testing this
this the ObjectLabel piglit tests test for both cases of this error
message:
http://lists.freedesktop.org/archives/piglit/2013-August/007139.html

> 
> 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.
> 

No problem I will get some patches together. Also thanks for taking the
time to comprehensively answer my questions. I guess I should have asked
more questions during development but my first few questions went
unanswered so I assumed everyone was busy and took a do what I think
should be done and wait for feedback approach.

I also have one more question about working on Mesa. Is there a wiki
page or something where developers register who is working on what
extension to avoid double up? I have been playing around with the
ARB_arrays_of_arrays extension but don't have a lot spare time to commit
to working on it, so it will probably be a while (if ever) before I come
up with anything. However I would hate to spend all that (precious)
spare time working on it only to find someone else had beaten me to it.
It would at least be handy to know to stop work on it if some else
wanted to do it before I had finished.

Thanks,
Tim