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. > Mathias > >> >> > } >> > >> > /* Colormaterial -- this kindof sucks. >> > >> > _______________________________________________ >> > mesa-dev mailing list >> > mesa-dev@lists.freedesktop.org >> > https://lists.freedesktop.org/mailman/listinfo/mesa-dev _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev