[Mesa-dev] [PATCH 3/3] u_dynarray: turn util_dynarray_{grow, resize} into element-oriented macros
Bas Nieuwenhuizen
bas at basnieuwenhuizen.nl
Sat May 4 23:19:50 UTC 2019
On Sat, May 4, 2019 at 3:25 PM Nicolai Hähnle <nhaehnle at gmail.com> wrote:
>
> From: Nicolai Hähnle <nicolai.haehnle at 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 at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
More information about the mesa-dev
mailing list