[Mesa-dev] [PATCH 29/29] vbo: Use a bitmask to track the active arrays in vbo_save*.
Mark Janes
mark.a.janes at intel.com
Mon Jun 20 17:33:42 UTC 2016
Mathias.Froehlich at gmx.net writes:
> From: Mathias Fröhlich <mathias.froehlich at web.de>
>
> The use of a bitmask makes functions iterating only active
> attributes less visible in profiles.
>
> Signed-off-by: Mathias Fröhlich <Mathias.Froehlich at 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.
> }
>
> /* Colormaterial -- this kindof sucks.
> --
> 2.5.5
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
More information about the mesa-dev
mailing list