On Sat, May 4, 2019 at 3:25 PM Nicolai Hähnle <nhaeh...@gmail.com> wrote: > > From: Nicolai Hähnle <nicolai.haeh...@amd.com> > > The main motivation for this change is API ergonomics: most operations > on dynarrays are really on elements, not on bytes, so it's weird to have > grow and resize as the odd operations out. > > The secondary motivation is memory safety. Users of the old byte-oriented > functions would often multiply a number of elements with the element size, > which could overflow, and checking for overflow is tedious. > > With this change, we only need to implement the overflow checks once. > The checks are cheap: since eltsize is a compile-time constant and the > functions should be inlined, they only add a single comparison and an > unlikely branch. > --- > .../drivers/nouveau/nv30/nvfx_fragprog.c | 2 +- > src/gallium/drivers/nouveau/nv50/nv50_state.c | 5 +-- > src/gallium/drivers/nouveau/nvc0/nvc0_state.c | 5 +-- > .../compiler/brw_nir_analyze_ubo_ranges.c | 2 +- > src/mesa/drivers/dri/i965/brw_bufmgr.c | 4 +- > src/util/u_dynarray.h | 38 +++++++++++++------ > 6 files changed, 35 insertions(+), 21 deletions(-) > > diff --git a/src/gallium/drivers/nouveau/nv30/nvfx_fragprog.c > b/src/gallium/drivers/nouveau/nv30/nvfx_fragprog.c > index 86e3599325e..2bcb62b97d8 100644 > --- a/src/gallium/drivers/nouveau/nv30/nvfx_fragprog.c > +++ b/src/gallium/drivers/nouveau/nv30/nvfx_fragprog.c > @@ -66,21 +66,21 @@ release_temps(struct nvfx_fpc *fpc) > fpc->r_temps &= ~fpc->r_temps_discard; > fpc->r_temps_discard = 0ULL; > } > > static inline struct nvfx_reg > nvfx_fp_imm(struct nvfx_fpc *fpc, float a, float b, float c, float d) > { > float v[4] = {a, b, c, d}; > int idx = fpc->imm_data.size >> 4; > > - memcpy(util_dynarray_grow(&fpc->imm_data, sizeof(float) * 4), v, 4 * > sizeof(float)); > + memcpy(util_dynarray_grow(&fpc->imm_data, float, 4), v, 4 * > sizeof(float)); > return nvfx_reg(NVFXSR_IMM, idx); > } > > static void > grow_insns(struct nvfx_fpc *fpc, int size) > { > struct nv30_fragprog *fp = fpc->fp; > > fp->insn_len += size; > fp->insn = realloc(fp->insn, sizeof(uint32_t) * fp->insn_len); > diff --git a/src/gallium/drivers/nouveau/nv50/nv50_state.c > b/src/gallium/drivers/nouveau/nv50/nv50_state.c > index 55167a27c09..228feced5d1 100644 > --- a/src/gallium/drivers/nouveau/nv50/nv50_state.c > +++ b/src/gallium/drivers/nouveau/nv50/nv50_state.c > @@ -1256,24 +1256,23 @@ nv50_set_global_bindings(struct pipe_context *pipe, > struct pipe_resource **resources, > uint32_t **handles) > { > struct nv50_context *nv50 = nv50_context(pipe); > struct pipe_resource **ptr; > unsigned i; > const unsigned end = start + nr; > > if (nv50->global_residents.size <= (end * sizeof(struct pipe_resource > *))) { > const unsigned old_size = nv50->global_residents.size; > - const unsigned req_size = end * sizeof(struct pipe_resource *); > - util_dynarray_resize(&nv50->global_residents, req_size); > + util_dynarray_resize(&nv50->global_residents, struct pipe_resource *, > end); > memset((uint8_t *)nv50->global_residents.data + old_size, 0, > - req_size - old_size); > + nv50->global_residents.size - old_size); > } > > if (resources) { > ptr = util_dynarray_element( > &nv50->global_residents, struct pipe_resource *, start); > for (i = 0; i < nr; ++i) { > pipe_resource_reference(&ptr[i], resources[i]); > nv50_set_global_handle(handles[i], resources[i]); > } > } else { > diff --git a/src/gallium/drivers/nouveau/nvc0/nvc0_state.c > b/src/gallium/drivers/nouveau/nvc0/nvc0_state.c > index 12e21862ee0..2ab51c8529e 100644 > --- a/src/gallium/drivers/nouveau/nvc0/nvc0_state.c > +++ b/src/gallium/drivers/nouveau/nvc0/nvc0_state.c > @@ -1363,24 +1363,23 @@ nvc0_set_global_bindings(struct pipe_context *pipe, > struct pipe_resource **resources, > uint32_t **handles) > { > struct nvc0_context *nvc0 = nvc0_context(pipe); > struct pipe_resource **ptr; > unsigned i; > const unsigned end = start + nr; > > if (nvc0->global_residents.size <= (end * sizeof(struct pipe_resource > *))) { > const unsigned old_size = nvc0->global_residents.size; > - const unsigned req_size = end * sizeof(struct pipe_resource *); > - util_dynarray_resize(&nvc0->global_residents, req_size); > + util_dynarray_resize(&nvc0->global_residents, struct pipe_resource *, > end); > memset((uint8_t *)nvc0->global_residents.data + old_size, 0, > - req_size - old_size); > + nvc0->global_residents.size - old_size); > } > > if (resources) { > ptr = util_dynarray_element( > &nvc0->global_residents, struct pipe_resource *, start); > for (i = 0; i < nr; ++i) { > pipe_resource_reference(&ptr[i], resources[i]); > nvc0_set_global_handle(handles[i], resources[i]); > } > } else { > diff --git a/src/intel/compiler/brw_nir_analyze_ubo_ranges.c > b/src/intel/compiler/brw_nir_analyze_ubo_ranges.c > index ab7a2705c9a..4c5e03380e1 100644 > --- a/src/intel/compiler/brw_nir_analyze_ubo_ranges.c > +++ b/src/intel/compiler/brw_nir_analyze_ubo_ranges.c > @@ -274,21 +274,21 @@ brw_nir_analyze_ubo_ranges(const struct brw_compiler > *compiler, > * bitfield. There are no more ranges to process. > */ > first_hole = 64; > offsets = 0; > } else { > /* We've processed all bits before first_hole. Mask them off. */ > offsets &= ~((1ull << first_hole) - 1); > } > > struct ubo_range_entry *entry = > - util_dynarray_grow(&ranges, sizeof(struct ubo_range_entry)); > + util_dynarray_grow(&ranges, struct ubo_range_entry, 1); > > entry->range.block = b; > entry->range.start = first_bit; > /* first_hole is one beyond the end, so we don't need to add 1 */ > entry->range.length = first_hole - first_bit; > entry->benefit = 0; > > for (int i = 0; i < entry->range.length; i++) > entry->benefit += info->uses[first_bit + i]; > } > diff --git a/src/mesa/drivers/dri/i965/brw_bufmgr.c > b/src/mesa/drivers/dri/i965/brw_bufmgr.c > index 7b0ddfb64dd..a89db7b8e40 100644 > --- a/src/mesa/drivers/dri/i965/brw_bufmgr.c > +++ b/src/mesa/drivers/dri/i965/brw_bufmgr.c > @@ -287,21 +287,21 @@ bucket_vma_alloc(struct brw_bufmgr *bufmgr, > * memory for 64 blocks from a larger allocator (either a larger > * bucket or util_vma). > * > * We align the address to the node size (64 blocks) so that > * bucket_vma_free can easily compute the starting address of this > * block by rounding any address we return down to the node size. > * > * Set the first bit used, and return the start address. > */ > uint64_t node_size = 64ull * bucket->size; > - node = util_dynarray_grow(vma_list, sizeof(struct vma_bucket_node)); > + node = util_dynarray_grow(vma_list, struct vma_bucket_node, 1); > > if (unlikely(!node)) > return 0ull; > > uint64_t addr = vma_alloc(bufmgr, memzone, node_size, node_size); > node->start_address = gen_48b_address(addr); > node->bitmap = ~1ull; > return node->start_address; > } > > @@ -344,21 +344,21 @@ bucket_vma_free(struct bo_cache_bucket *bucket, > uint64_t address) > > util_dynarray_foreach(vma_list, struct vma_bucket_node, cur) { > if (cur->start_address == start) { > node = cur; > break; > } > } > > if (!node) { > /* No node - the whole group of 64 blocks must have been in-use. */ > - node = util_dynarray_grow(vma_list, sizeof(struct vma_bucket_node)); > + node = util_dynarray_grow(vma_list, struct vma_bucket_node, 1); > > if (unlikely(!node)) > return; /* bogus, leaks some GPU VMA, but nothing we can do... */ > > node->start_address = start; > node->bitmap = 0ull; > } > > /* Set the bit to return the memory. */ > assert((node->bitmap & (1ull << bit)) == 0ull); > diff --git a/src/util/u_dynarray.h b/src/util/u_dynarray.h > index f6a81609dbe..14403e91bd2 100644 > --- a/src/util/u_dynarray.h > +++ b/src/util/u_dynarray.h > @@ -22,20 +22,21 @@ > * OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION > * WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE. > * > **************************************************************************/ > > #ifndef U_DYNARRAY_H > #define U_DYNARRAY_H > > #include <stdlib.h> > #include <string.h> > +#include <limits.h> > #include "ralloc.h" > > #ifdef __cplusplus > extern "C" { > #endif > > /* A zero-initialized version of this is guaranteed to represent an > * empty array. > * > * Also, size <= capacity and data != 0 if and only if capacity != 0 > @@ -92,49 +93,61 @@ util_dynarray_ensure_cap(struct util_dynarray *buf, > unsigned newcap) > } else { > buf->data = realloc(buf->data, buf->capacity); > } > if (!buf->data) > return 0; > } > > return (void *)((char *)buf->data + buf->size); > } > > -static inline void * > -util_dynarray_grow_cap(struct util_dynarray *buf, int diff) > -{ > - return util_dynarray_ensure_cap(buf, buf->size + diff); > -} > - > /* use util_dynarray_trim to reduce the allocated storage */ > static inline void * > -util_dynarray_resize(struct util_dynarray *buf, unsigned newsize) > +util_dynarray_resize_bytes(struct util_dynarray *buf, unsigned nelts, size_t > eltsize) > { > + if (unlikely(nelts > UINT_MAX / eltsize)) { > + util_dynarray_fini(buf); > + return 0;
Can we not do the util_dynarray_fini? I really like the principle that if we fail then nothing is modified and this deviates form that. Also if someone handles the error seriously there is a large probably that the containing structure is going to be destructed which probably expexts a valid dynarray. If nobody handles the error (see e.g. the below util_dynarray_clone), then things are going to blow up either way. > + } > + > + unsigned newsize = nelts * eltsize; > void *p = util_dynarray_ensure_cap(buf, newsize); > buf->size = newsize; > > return p; > } > > static inline void > util_dynarray_clone(struct util_dynarray *buf, void *mem_ctx, > struct util_dynarray *from_buf) > { > util_dynarray_init(buf, mem_ctx); > - util_dynarray_resize(buf, from_buf->size); > + util_dynarray_resize_bytes(buf, 1, from_buf->size); > memcpy(buf->data, from_buf->data, from_buf->size); > } > > static inline void * > -util_dynarray_grow(struct util_dynarray *buf, int diff) > +util_dynarray_grow_bytes(struct util_dynarray *buf, unsigned ngrow, size_t > eltsize) > { > - return util_dynarray_resize(buf, buf->size + diff); > + unsigned growbytes = ngrow * eltsize; > + > + if (unlikely(ngrow > (UINT_MAX / eltsize) || > + growbytes > UINT_MAX - buf->size)) { > + util_dynarray_fini(buf); Can we not do the util_dynarray_fini, see above? > + return 0; > + } > + > + unsigned newsize = buf->size + growbytes; > + void *p = util_dynarray_ensure_cap(buf, newsize); > + buf->size = newsize; > + > + return p; > } > > static inline void > util_dynarray_trim(struct util_dynarray *buf) > { > if (buf->size != buf->capacity) { > if (buf->size) { > if (buf->mem_ctx) { > buf->data = reralloc_size(buf->mem_ctx, buf->data, buf->size); > } else { > @@ -146,21 +159,24 @@ util_dynarray_trim(struct util_dynarray *buf) > ralloc_free(buf->data); > } else { > free(buf->data); > } > buf->data = NULL; > buf->capacity = 0; > } > } > } > > -#define util_dynarray_append(buf, type, v) do {type __v = (v); > memcpy(util_dynarray_grow((buf), sizeof(type)), &__v, sizeof(type));} while(0) > +#define util_dynarray_append(buf, type, v) do {type __v = (v); > memcpy(util_dynarray_grow_bytes((buf), 1, sizeof(type)), &__v, > sizeof(type));} while(0) > +/* Returns a pointer to the space of the first new element (in case of > growth) or NULL on failure. */ > +#define util_dynarray_resize(buf, type, nelts) > util_dynarray_resize_bytes(buf, (nelts), sizeof(type)) > +#define util_dynarray_grow(buf, type, ngrow) util_dynarray_grow_bytes(buf, > (ngrow), sizeof(type)) > #define util_dynarray_top_ptr(buf, type) (type*)((char*)(buf)->data + > (buf)->size - sizeof(type)) > #define util_dynarray_top(buf, type) *util_dynarray_top_ptr(buf, type) > #define util_dynarray_pop_ptr(buf, type) (type*)((char*)(buf)->data + > ((buf)->size -= sizeof(type))) > #define util_dynarray_pop(buf, type) *util_dynarray_pop_ptr(buf, type) > #define util_dynarray_contains(buf, type) ((buf)->size >= sizeof(type)) > #define util_dynarray_element(buf, type, idx) ((type*)(buf)->data + (idx)) > #define util_dynarray_begin(buf) ((buf)->data) > #define util_dynarray_end(buf) ((void*)util_dynarray_element((buf), char, > (buf)->size)) > #define util_dynarray_num_elements(buf, type) ((buf)->size / sizeof(type)) > > -- > 2.20.1 > > _______________________________________________ > 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