Re: [Mesa-dev] [PATCH 10/15] mesa: Implement KHR_debug ObjectLabel functions
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 (0x7fff) 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
Re: [Mesa-dev] [PATCH 10/15] mesa: Implement KHR_debug ObjectLabel functions
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 (0x7fff) 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
Re: [Mesa-dev] [PATCH 10/15] mesa: Implement KHR_debug ObjectLabel functions
On 09/04/2013 09:09 PM, Ian Romanick wrote: In the mean time, we should land Brian's patch to fix 'make check'. Does that imply your R-b? -Brian ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 10/15] mesa: Implement KHR_debug ObjectLabel functions
On 09/05/2013 01:05 AM, Timothy Arceri wrote: 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? There's no such page right now. People sometimes will post a message to say they're working on a new feature. I'd be happy to have a wiki page that gives a heads-up on who's doing what. -Brian ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 10/15] mesa: Implement KHR_debug ObjectLabel functions
On 09/05/2013 09:57 AM, Brian Paul wrote: On 09/04/2013 09:09 PM, Ian Romanick wrote: In the mean time, we should land Brian's patch to fix 'make check'. Does that imply your R-b? Yeah. I meant to go ahead and reply (again) to the patch. It was getting late, and I forgot. -Brian ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 10/15] mesa: Implement KHR_debug ObjectLabel functions
On 08/26/2013 03:43 AM, Timothy Arceri wrote:
Signed-off-by: Timothy Arceri t_arc...@yahoo.com.au
---
src/mesa/main/objectlabel.c | 277
+++
src/mesa/main/objectlabel.h | 61 ++
2 files changed, 338 insertions(+)
create mode 100644 src/mesa/main/objectlabel.c
create mode 100644 src/mesa/main/objectlabel.h
diff --git a/src/mesa/main/objectlabel.c b/src/mesa/main/objectlabel.c
new file mode 100644
index 000..78f9b33
--- /dev/null
+++ b/src/mesa/main/objectlabel.c
@@ -0,0 +1,277 @@
+/*
+ * Mesa 3-D graphics library
+ *
+ * Copyright (C) 2013 Timothy Arceri All Rights Reserved.
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a
+ * copy of this software and associated documentation files (the Software),
+ * to deal in the Software without restriction, including without limitation
+ * the rights to use, copy, modify, merge, publish, distribute, sublicense,
+ * and/or sell copies of the Software, and to permit persons to whom the
+ * Software is furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included
+ * in all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED AS IS, WITHOUT WARRANTY OF ANY KIND, EXPRESS
+ * OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR
+ * OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE,
+ * ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
+ * OTHER DEALINGS IN THE SOFTWARE.
+ */
+
+
+#include arrayobj.h
+#include bufferobj.h
+#include context.h
+#include dlist.h
+#include enums.h
+#include fbobject.h
+#include objectlabel.h
+#include queryobj.h
+#include samplerobj.h
+#include shaderobj.h
+#include syncobj.h
+#include texobj.h
+#include transformfeedback.h
+
+
+/**
+ * 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?
+ /* 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).
+/* 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
*/
+ if (bufSize = labelLen)
+ labelLen = bufSize-1;
+
+ memcpy(label, *labelPtr, labelLen);
+ label[labelLen] = '\0';
+ }
+
+ if (length)
+ *length = labelLen;
+}
+
+/**
+ * Helper for _mesa_ObjectLabel() and _mesa_GetObjectLabel().
+ */
+static char **
+get_label_pointer(struct gl_context *ctx, GLenum identifier, GLuint name,
+ const char *caller)
+{
+ char **labelPtr = NULL;
+
+ switch (identifier) {
+ case GL_BUFFER:
+ {
Coding style fail. The { goes on the same line as the case, and there's
only one level of indent
+ struct
Re: [Mesa-dev] [PATCH 10/15] mesa: Implement KHR_debug ObjectLabel functions
+/**
+ * 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?
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 10/15] mesa: Implement KHR_debug ObjectLabel functions
+ /* 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.
Ignore my comment I see now you are talking about the malloc check I thought
you meant the MAX_LABEL_LENGTH test in the last line.
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 10/15] mesa: Implement KHR_debug ObjectLabel functions
On 08/26/2013 04:43 AM, Timothy Arceri wrote:
Signed-off-by: Timothy Arceri t_arc...@yahoo.com.au
---
src/mesa/main/objectlabel.c | 277 +++
src/mesa/main/objectlabel.h | 61 ++
2 files changed, 338 insertions(+)
create mode 100644 src/mesa/main/objectlabel.c
create mode 100644 src/mesa/main/objectlabel.h
diff --git a/src/mesa/main/objectlabel.c b/src/mesa/main/objectlabel.c
new file mode 100644
index 000..78f9b33
--- /dev/null
+++ b/src/mesa/main/objectlabel.c
@@ -0,0 +1,277 @@
+/*
+ * Mesa 3-D graphics library
+ *
+ * Copyright (C) 2013 Timothy Arceri All Rights Reserved.
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a
+ * copy of this software and associated documentation files (the Software),
+ * to deal in the Software without restriction, including without limitation
+ * the rights to use, copy, modify, merge, publish, distribute, sublicense,
+ * and/or sell copies of the Software, and to permit persons to whom the
+ * Software is furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included
+ * in all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED AS IS, WITHOUT WARRANTY OF ANY KIND, EXPRESS
+ * OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR
+ * OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE,
+ * ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
+ * OTHER DEALINGS IN THE SOFTWARE.
+ */
+
+
+#include arrayobj.h
+#include bufferobj.h
+#include context.h
+#include dlist.h
+#include enums.h
+#include fbobject.h
+#include objectlabel.h
+#include queryobj.h
+#include samplerobj.h
+#include shaderobj.h
+#include syncobj.h
+#include texobj.h
+#include transformfeedback.h
+
+
+/**
+ * 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 */
Indentation
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
