Re: [Mesa-dev] [PATCH 3/3] u_dynarray: turn util_dynarray_{grow, resize} into element-oriented macros
On 04.05.19 17:55, Gustaw Smolarczyk wrote:
> sob., 4 maj 2019 o 15:25 Nicolai Hähnle napisał(a):
>> 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);
>
> Just a nit: couldn't you swap the arguments to resize_bytes? The
> compiler will probably figure it out, but you are performing the
> following operation in the resize_bytes function:
>
>if (unlikely(1 > UINT_MAX / from_buf->size))
>
> which is a division of a constant by a variable. With the arguments
> swapped it would be:
>
>if (unlikely(from_buf->size > UINT_MAX / 1))
>
> which should be easier to optimize away (also in non-optimized
> version). This way, the eltsize parameter will probably always be a
> constant too.
Good catch, I've fixed that locally.
Cheers,
Nicolai
>
> Regards,
> Gustaw Smolarczyk
>
>> 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);
>> + 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
Re: [Mesa-dev] [PATCH 3/3] u_dynarray: turn util_dynarray_{grow, resize} into element-oriented macros
On 05.05.19 01:19, Bas Nieuwenhuizen wrote:
>> /* 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.
I guess there are two different schools of thought on this. Your way
makes sense if callers actually test the return value -- but if they
don't, this way is worse because the caller is subsequently very likely
to overwrite random memory instead of just crashing.
> 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.
util_dynarray_fini leaves the array in a valid state, so this is not a
concern.
Cheers,
Nicolai
>
>> + }
>> +
>> + 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
Re: [Mesa-dev] [PATCH 3/3] u_dynarray: turn util_dynarray_{grow, resize} into element-oriented macros
On Sat, May 4, 2019 at 3:25 PM Nicolai Hähnle wrote:
>
> From: Nicolai Hähnle
>
> 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(>imm_data, sizeof(float) * 4), v, 4 *
> sizeof(float));
> + memcpy(util_dynarray_grow(>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(>global_residents, req_size);
> + util_dynarray_resize(>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(
> >global_residents, struct pipe_resource *, start);
>for (i = 0; i < nr; ++i) {
> pipe_resource_reference([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(>global_residents, req_size);
> + util_dynarray_resize(>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(
> >global_residents, struct pipe_resource *, start);
>for (i = 0; i < nr; ++i) {
> pipe_resource_reference([i], resources[i]);
>
Re: [Mesa-dev] [PATCH 3/3] u_dynarray: turn util_dynarray_{grow, resize} into element-oriented macros
sob., 4 maj 2019 o 15:25 Nicolai Hähnle napisał(a):
>
> From: Nicolai Hähnle
>
> 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(>imm_data, sizeof(float) * 4), v, 4 *
> sizeof(float));
> + memcpy(util_dynarray_grow(>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(>global_residents, req_size);
> + util_dynarray_resize(>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(
> >global_residents, struct pipe_resource *, start);
>for (i = 0; i < nr; ++i) {
> pipe_resource_reference([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(>global_residents, req_size);
> + util_dynarray_resize(>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(
> >global_residents, struct pipe_resource *, start);
>for (i = 0; i < nr; ++i) {
> pipe_resource_reference([i], resources[i]);
>
