On Monday, June 20, 2016 11:42:19 Mark Janes wrote:
> Mathias Fröhlich <mathias.froehl...@gmx.net> writes:
>
> > On Monday, June 20, 2016 10:33:42 Mark Janes wrote:
> >> mathias.froehl...@gmx.net writes:
> >>
> >> > From: Mathias Fröhlich <mathias.froehl...@web.de>
> >> >
> >> > The use of a bitmask makes functions iterating only active
> >> > attributes less visible in profiles.
> >> >
> >> > Signed-off-by: Mathias Fröhlich <mathias.froehl...@web.de>
> >> > ---
> >> > src/mesa/vbo/vbo_save.h | 2 ++
> >> > src/mesa/vbo/vbo_save_api.c | 70
> >> > ++++++++++++++++++++++++++------------------
> >> > src/mesa/vbo/vbo_save_draw.c | 55 ++++++++++++++++++----------------
> >> > 3 files changed, 72 insertions(+), 55 deletions(-)
> >> >
> >> > diff --git a/src/mesa/vbo/vbo_save.h b/src/mesa/vbo/vbo_save.h
> >> > index 8032db8..e3b86bc 100644
> >> > --- a/src/mesa/vbo/vbo_save.h
> >> > +++ b/src/mesa/vbo/vbo_save.h
> >> > @@ -61,6 +61,7 @@ struct vbo_save_copied_vtx {
> >> > * compiled using the fallback opcode mechanism provided by dlist.c.
> >> > */
> >> > struct vbo_save_vertex_list {
> >> > + GLbitfield64 enabled;/**< mask of enabled vbo arrays. */
> >> > GLubyte attrsz[VBO_ATTRIB_MAX];
> >> > GLenum attrtype[VBO_ATTRIB_MAX];
> >> > GLuint vertex_size; /**< size in GLfloats */
> >> > @@ -126,6 +127,7 @@ struct vbo_save_context {
> >> > struct gl_client_array arrays[VBO_ATTRIB_MAX];
> >> > const struct gl_client_array *inputs[VBO_ATTRIB_MAX];
> >> >
> >> > + GLbitfield64 enabled;/**< mask of enabled vbo arrays. */
> >> > GLubyte attrsz[VBO_ATTRIB_MAX]; /**< 1, 2, 3 or 4 */
> >> > GLenum attrtype[VBO_ATTRIB_MAX]; /**< GL_FLOAT, GL_INT, etc */
> >> > GLubyte active_sz[VBO_ATTRIB_MAX]; /**< 1, 2, 3 or 4 */
> >> > diff --git a/src/mesa/vbo/vbo_save_api.c b/src/mesa/vbo/vbo_save_api.c
> >> > index 97a1dfd..b178060 100644
> >> > --- a/src/mesa/vbo/vbo_save_api.c
> >> > +++ b/src/mesa/vbo/vbo_save_api.c
> >> > @@ -429,6 +429,7 @@ _save_compile_vertex_list(struct gl_context *ctx)
> >> >
> >> > /* Duplicate our template, increment refcounts to the storage
> >> > structs:
> >> > */
> >> > + node->enabled = save->enabled;
> >> > memcpy(node->attrsz, save->attrsz, sizeof(node->attrsz));
> >> > memcpy(node->attrtype, save->attrtype, sizeof(node->attrtype));
> >> > node->vertex_size = save->vertex_size;
> >> > @@ -624,14 +625,16 @@ static void
> >> > _save_copy_to_current(struct gl_context *ctx)
> >> > {
> >> > struct vbo_save_context *save = &vbo_context(ctx)->save;
> >> > - GLuint i;
> >> > + GLbitfield64 enabled = save->enabled &
> >> > (~BITFIELD64_BIT(VBO_ATTRIB_POS));
> >> >
> >> > - for (i = VBO_ATTRIB_POS + 1; i < VBO_ATTRIB_MAX; i++) {
> >> > - if (save->attrsz[i]) {
> >> > - save->currentsz[i][0] = save->attrsz[i];
> >> > - COPY_CLEAN_4V_TYPE_AS_UNION(save->current[i], save->attrsz[i],
> >> > - save->attrptr[i],
> >> > save->attrtype[i]);
> >> > - }
> >> > + while (enabled) {
> >> > + int i = ffsll(enabled) - 1;
> >> > + enabled ^= BITFIELD64_BIT(i);
> >> > + assert(save->attrsz[i]);
> >> > +
> >> > + save->currentsz[i][0] = save->attrsz[i];
> >> > + COPY_CLEAN_4V_TYPE_AS_UNION(save->current[i], save->attrsz[i],
> >> > + save->attrptr[i], save->attrtype[i]);
> >> > }
> >> > }
> >> >
> >> > @@ -640,9 +643,12 @@ static void
> >> > _save_copy_from_current(struct gl_context *ctx)
> >> > {
> >> > struct vbo_save_context *save = &vbo_context(ctx)->save;
> >> > - GLint i;
> >> > + GLbitfield64 enabled = save->enabled &
> >> > (~BITFIELD64_BIT(VBO_ATTRIB_POS));
> >> > +
> >> > + while (enabled) {
> >> > + int i = ffsll(enabled) - 1;
> >> > + enabled ^= BITFIELD64_BIT(i);
> >> >
> >> > - for (i = VBO_ATTRIB_POS + 1; i < VBO_ATTRIB_MAX; i++) {
> >> > switch (save->attrsz[i]) {
> >> > case 4:
> >> > save->attrptr[i][3] = save->current[i][3];
> >> > @@ -652,7 +658,9 @@ _save_copy_from_current(struct gl_context *ctx)
> >> > save->attrptr[i][1] = save->current[i][1];
> >> > case 1:
> >> > save->attrptr[i][0] = save->current[i][0];
> >> > + break;
> >> > case 0:
> >> > + assert(0);
> >> > break;
> >> > }
> >> > }
> >> > @@ -691,6 +699,7 @@ _save_upgrade_vertex(struct gl_context *ctx, GLuint
> >> > attr, GLuint newsz)
> >> > */
> >> > oldsz = save->attrsz[attr];
> >> > save->attrsz[attr] = newsz;
> >> > + save->enabled |= BITFIELD64_BIT(attr);
> >> >
> >> > save->vertex_size += newsz - oldsz;
> >> > save->max_vert = ((VBO_SAVE_BUFFER_SIZE - save->vertex_store->used) /
> >> > @@ -723,7 +732,6 @@ _save_upgrade_vertex(struct gl_context *ctx, GLuint
> >> > attr, GLuint newsz)
> >> > if (save->copied.nr) {
> >> > const fi_type *data = save->copied.buffer;
> >> > fi_type *dest = save->buffer;
> >> > - GLuint j;
> >> >
> >> > /* Need to note this and fix up at runtime (or loopback):
> >> > */
> >> > @@ -733,27 +741,29 @@ _save_upgrade_vertex(struct gl_context *ctx,
> >> > GLuint attr, GLuint newsz)
> >> > }
> >> >
> >> > for (i = 0; i < save->copied.nr; i++) {
> >> > - for (j = 0; j < VBO_ATTRIB_MAX; j++) {
> >> > - if (save->attrsz[j]) {
> >> > - if (j == attr) {
> >> > - if (oldsz) {
> >> > - COPY_CLEAN_4V_TYPE_AS_UNION(dest, oldsz, data,
> >> > - save->attrtype[j]);
> >> > - data += oldsz;
> >> > - dest += newsz;
> >> > - }
> >> > - else {
> >> > - COPY_SZ_4V(dest, newsz, save->current[attr]);
> >> > - dest += newsz;
> >> > - }
> >> > + GLbitfield64 enabled = save->enabled;
> >> > + while (enabled) {
> >> > + int j = ffsll(enabled) - 1;
> >> > + enabled ^= BITFIELD64_BIT(j);
> >> > + assert(save->attrsz[j]);
> >> > + if (j == attr) {
> >> > + if (oldsz) {
> >> > + COPY_CLEAN_4V_TYPE_AS_UNION(dest, oldsz, data,
> >> > + save->attrtype[j]);
> >> > + data += oldsz;
> >> > + dest += newsz;
> >> > }
> >> > else {
> >> > - GLint sz = save->attrsz[j];
> >> > - COPY_SZ_4V(dest, sz, data);
> >> > - data += sz;
> >> > - dest += sz;
> >> > + COPY_SZ_4V(dest, newsz, save->current[attr]);
> >> > + dest += newsz;
> >> > }
> >> > }
> >> > + else {
> >> > + GLint sz = save->attrsz[j];
> >> > + COPY_SZ_4V(dest, sz, data);
> >> > + data += sz;
> >> > + dest += sz;
> >> > + }
> >> > }
> >> > }
> >> >
> >> > @@ -803,9 +813,11 @@ static void
> >> > _save_reset_vertex(struct gl_context *ctx)
> >> > {
> >> > struct vbo_save_context *save = &vbo_context(ctx)->save;
> >> > - GLuint i;
> >> >
> >> > - for (i = 0; i < VBO_ATTRIB_MAX; i++) {
> >> > + while (save->enabled) {
> >> > + int i = ffsll(save->enabled) - 1;
> >> > + save->enabled ^= BITFIELD64_BIT(i);
> >> > + assert(save->attrsz[i]);
> >> > save->attrsz[i] = 0;
> >> > save->active_sz[i] = 0;
> >> > }
> >> > diff --git a/src/mesa/vbo/vbo_save_draw.c b/src/mesa/vbo/vbo_save_draw.c
> >> > index b1fd689..d75805d 100644
> >> > --- a/src/mesa/vbo/vbo_save_draw.c
> >> > +++ b/src/mesa/vbo/vbo_save_draw.c
> >> > @@ -49,7 +49,8 @@ _playback_copy_to_current(struct gl_context *ctx,
> >> > struct vbo_context *vbo = vbo_context(ctx);
> >> > fi_type vertex[VBO_ATTRIB_MAX * 4];
> >> > fi_type *data;
> >> > - GLuint i, offset;
> >> > + GLbitfield64 mask;
> >> > + GLuint offset;
> >> >
> >> > if (node->current_size == 0)
> >> > return;
> >> > @@ -73,35 +74,37 @@ _playback_copy_to_current(struct gl_context *ctx,
> >> > data += node->attrsz[0]; /* skip vertex position */
> >> > }
> >> >
> >> > - for (i = VBO_ATTRIB_POS+1 ; i < VBO_ATTRIB_MAX ; i++) {
> >> > - if (node->attrsz[i]) {
> >> > - fi_type *current = (fi_type *)vbo->currval[i].Ptr;
> >> > - fi_type tmp[4];
> >> > -
> >> > - COPY_CLEAN_4V_TYPE_AS_UNION(tmp,
> >> > - node->attrsz[i],
> >> > - data,
> >> > - node->attrtype[i]);
> >> > + mask = node->enabled & (~BITFIELD64_BIT(VBO_ATTRIB_POS));
> >> > + while (mask) {
> >> > + int i = ffsll(mask) - 1;
> >> > + fi_type *current = (fi_type *)vbo->currval[i].Ptr;
> >> > + fi_type tmp[4];
> >> > + assert(node->attrsz[i]);
> >> > + mask ^= BITFIELD64_BIT(i);
> >> > +
> >> > + COPY_CLEAN_4V_TYPE_AS_UNION(tmp,
> >> > + node->attrsz[i],
> >> > + data,
> >> > + node->attrtype[i]);
> >> > +
> >> > + if (node->attrtype[i] != vbo->currval[i].Type ||
> >> > + memcmp(current, tmp, 4 * sizeof(GLfloat)) != 0) {
> >> > + memcpy(current, tmp, 4 * sizeof(GLfloat));
> >> >
> >> > - if (node->attrtype[i] != vbo->currval[i].Type ||
> >> > - memcmp(current, tmp, 4 * sizeof(GLfloat)) != 0) {
> >> > - memcpy(current, tmp, 4 * sizeof(GLfloat));
> >> > -
> >> > - vbo->currval[i].Size = node->attrsz[i];
> >> > - vbo->currval[i]._ElementSize = vbo->currval[i].Size *
> >> > sizeof(GLfloat);
> >> > - vbo->currval[i].Type = node->attrtype[i];
> >> > - vbo->currval[i].Integer =
> >> > - vbo_attrtype_to_integer_flag(node->attrtype[i]);
> >> > + vbo->currval[i].Size = node->attrsz[i];
> >> > + vbo->currval[i]._ElementSize = vbo->currval[i].Size *
> >> > sizeof(GLfloat);
> >> > + vbo->currval[i].Type = node->attrtype[i];
> >> > + vbo->currval[i].Integer =
> >> > + vbo_attrtype_to_integer_flag(node->attrtype[i]);
> >> >
> >> > - if (i >= VBO_ATTRIB_FIRST_MATERIAL &&
> >> > - i <= VBO_ATTRIB_LAST_MATERIAL)
> >> > - ctx->NewState |= _NEW_LIGHT;
> >> > + if (i >= VBO_ATTRIB_FIRST_MATERIAL &&
> >> > + i <= VBO_ATTRIB_LAST_MATERIAL)
> >> > + ctx->NewState |= _NEW_LIGHT;
> >> >
> >> > - ctx->NewState |= _NEW_CURRENT_ATTRIB;
> >> > - }
> >> > -
> >> > - data += node->attrsz[i];
> >> > + ctx->NewState |= _NEW_CURRENT_ATTRIB;
> >> > }
> >> > +
> >> > + data += node->attrsz[i];
> >>
> >> This line triggered Coverity 1362776. Above, you check that
> >> i <= VBO_ATTRIB_LAST_MATERIAL, which implies that it may be larger. In
> >> that case, you will overrun the attrsz array.
> >
> > I think that will not happen.
> > But I see: previously the loop was terminated with
> > i < VBO_ATTRIB_MAX == VBO_ATTRIB_LAST_MATERIAL + 1.
> > Now the bitmask potentially allows for more. But the code setting up the
> > bitmask does only allow bits that result in bits that lead to
> > i >= VBO_ATTRIB_MAX to be set.
> >
> > The previous code had that redundant check already. Well, may be
> > redundant with the current order of the VBO_ATTRIB values. I thought
> > I will leave that as is as the VBO_ATTRIB values may get reordered
> > at some point? At least I understood this redundant check in this way.
> >
> > Does Coverity understand this when we put an
> > assert(i < VBO_ATTRIB_MAX) in there?
> > Or do we need to remove the redundant check?
>
> My understanding is that Coverity understands assert() and assume(). I
> wouldn't add an assertion just for Coverity. I would change it if you
> think an additional assertion might make the code safer/clearer for
> future modifications.
I tend to keep the redundant check that triggers coverity. This made
the original code more robust against future changes and IMO that
should stay. But I would be fine with an additional assert.
So, can you confirm that the attached patch helps against the new warning?
Mathias
>From b0b2a4a60caf76a1149d514abc04a105775e7158 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Mathias=20Fr=C3=B6hlich?= <mathias.froehl...@web.de>
Date: Wed, 22 Jun 2016 18:30:55 +0200
Subject: [PATCH] mesa: Help coverity to see that we are correct.
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Signed-off-by: Mathias Fröhlich <mathias.froehl...@web.de>
---
src/mesa/vbo/vbo_save_draw.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/src/mesa/vbo/vbo_save_draw.c b/src/mesa/vbo/vbo_save_draw.c
index 7881ce1..e8bdc6c 100644
--- a/src/mesa/vbo/vbo_save_draw.c
+++ b/src/mesa/vbo/vbo_save_draw.c
@@ -80,6 +80,7 @@ _playback_copy_to_current(struct gl_context *ctx,
const int i = u_bit_scan64(&mask);
fi_type *current = (fi_type *)vbo->currval[i].Ptr;
fi_type tmp[4];
+ assert(i < VBO_ATTRIB_MAX);
assert(node->attrsz[i]);
COPY_CLEAN_4V_TYPE_AS_UNION(tmp,
--
2.5.5
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
