Re: [Mesa-dev] [PATCH v2] glsl: Add blob.c---a simple interface for serializing data
On Fri, Dec 12 2014, Ian Romanick wrote: > In addition to the (mostly) nits below, this seems ripe for some unit > tests. Especially for the overrun cases and the data alignment cases. Thanks for the review, Ian. Testing makes sense. I'll cook something up. And thanks for the nit-picking. I've cleaned those up, and I'll just comment on one or two here: >> +grow_to_fit (struct blob *blob, size_t additional) > ^ No space here or other similar places. I'm happy to conform to Mesa style here, (and I've already changed the patch). But in passing, let me say that I've learned to really love having a space between a function name and its left parenthesis, (in spite of it looking totally bizarre at first). Give it a try in some small project some time and see what you think. For me, it improves readability just as much as the space we use between "if" and its parenthsis. >> + if (blob->allocated == 0) >> + to_allocate = BLOB_INITIAL_SIZE; > > Can additional ever be > BLOB_INITIAL_SIZE? Good point. In the current code, no, but there's no reason to have this fragility in place. So I've now got the MAX2 in place like this: if (blob->allocated == 0) to_allocate = BLOB_INITIAL_SIZE; else to_allocate = blob->allocated * 2; to_allocate = MAX2(to_allocate, blob->allocated + additional); (That's the only real change in the functionality of the code, so I won't bother re-sending a patch with the whitespace and comment fixes) >> +/* When done reading, the caller can ensure that everything was consumed by >> + * checking the following: >> + * >> + * 1. blob->data should be equal to blob->end, (if not, too little was >> + * read). > > blob->data or blob->current? Yes, blob->current. A good fix. -Carl pgp4zu4RDVdSe.pgp Description: PGP signature ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH v2] glsl: Add blob.c---a simple interface for serializing data
In addition to the (mostly) nits below, this seems ripe for some unit
tests. Especially for the overrun cases and the data alignment cases.
On 12/12/2014 02:43 PM, Carl Worth wrote:
> This new interface allows for writing a series of objects to a chunk
> of memory (a "blob").. The allocated memory is maintained within the
> blob itself, (and re-allocated by doubling when necessary).
>
> There are also functions for reading objects from a blob as well. If
> code attempts to read beyond the available memory, the read functions
> return 0 values (or its moral equivalent) without reading past the
> allocated memory. Once the caller is done with the reads, it can check
> blob->overrun to ensure whether any invalid values were previously
> returned due to attempts to read too far.
> ---
>
> Jason,
>
> This second revision of my patch is in response to your review.
>
> I went to move the align_blob_reader function down by all the other
> blob_reader functions, but I decided not to. You had a concern about the
> alignment code, and it it's wrong then both align_blob() and
> align_blob_reader() will need to be fixed. So it seems useful to me to keep
> them close to each other.
>
> -Carl
>
> src/glsl/Makefile.sources | 3 +-
> src/glsl/blob.c | 295
> ++
> src/glsl/blob.h | 245 ++
> 3 files changed, 542 insertions(+), 1 deletion(-)
> create mode 100644 src/glsl/blob.c
> create mode 100644 src/glsl/blob.h
>
> diff --git a/src/glsl/Makefile.sources b/src/glsl/Makefile.sources
> index 676fa0d..875e4bf 100644
> --- a/src/glsl/Makefile.sources
> +++ b/src/glsl/Makefile.sources
> @@ -105,7 +105,8 @@ LIBGLSL_FILES = \
> $(GLSL_SRCDIR)/opt_swizzle_swizzle.cpp \
> $(GLSL_SRCDIR)/opt_tree_grafting.cpp \
> $(GLSL_SRCDIR)/opt_vectorize.cpp \
> - $(GLSL_SRCDIR)/s_expression.cpp
> + $(GLSL_SRCDIR)/s_expression.cpp \
> + $(GLSL_SRCDIR)/blob.c
Alphabetize
>
> # glsl_compiler
>
> diff --git a/src/glsl/blob.c b/src/glsl/blob.c
> new file mode 100644
> index 000..75d3cda
> --- /dev/null
> +++ b/src/glsl/blob.c
> @@ -0,0 +1,295 @@
> +/*
> + * Copyright © 2014 Intel Corporation
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a
> + * copy of this software and associated documentation files (the "Software"),
> + * to deal in the Software without restriction, including without limitation
> + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
> + * and/or sell copies of the Software, and to permit persons to whom the
> + * Software is furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice (including the next
> + * paragraph) shall be included in all copies or substantial portions of the
> + * Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
> DEALINGS
> + * IN THE SOFTWARE.
> + */
> +
> +#include
> +
> +#include "main/macros.h"
> +#include "util/ralloc.h"
> +#include "blob.h"
> +
> +#define BLOB_INITIAL_SIZE 4096
> +
> +/* Ensure that \blob will be able to fit an additional object of size
> + * \additional. The growing (if any) will occur by doubling the existing
> + * allocation.
> + */
> +static bool
> +grow_to_fit (struct blob *blob, size_t additional)
^ No space here or other similar places.
> +{
> + size_t to_allocate;
> + uint8_t *new_data;
> +
> + if (blob->size + additional <= blob->allocated)
> + return true;
> +
> + if (blob->allocated == 0)
> + to_allocate = BLOB_INITIAL_SIZE;
Can additional ever be > BLOB_INITIAL_SIZE?
> + else
> + to_allocate = MAX2(blob->allocated * 2, additional);
> +
> + new_data = reralloc_size(blob, blob->data, to_allocate);
> + if (new_data == NULL)
> + return false;
> +
> + blob->data = new_data;
> + blob->allocated = to_allocate;
> +
> + return true;
> +}
> +
> +/* Align the blob->size so that reading or writing a value at (blob->data +
> + * blob->size) will result in an access aligned to a granularity of
> \alignment
> + * bytes.
> + *
> + * \return True unless allocation fails
> + */
> +static bool
> +align_blob (struct blob *blob, size_t alignment)
> +{
> + size_t new_size;
> +
> + new_size = ALIGN(blob->size, alignment);
const size_t new_size = ALIGN(blob->size, alignment);
> +
> + if (! grow_to_fit (blob, new_size - blob->size))
^ No space here or other similar places.
> + return false;
Re: [Mesa-dev] [PATCH v2] glsl: Add blob.c---a simple interface for serializing data
On Fri, Dec 12 2014, Jason Ekstrand wrote: > Should be MAX2(blob->allocated * 2, blob->size + additional) Yikes! Yes. That's really embarrassing. > Something to tuck in the back of your brain in case you have a need to grow > this datastructure: The above write functions could be generated with three > invocations of a macro that takes a function name and a type. That would > prevent copy-and-paste typos. For only 3 functions, this probably isn't > worth it. An excellent point. And before I sent this out the first time, I actually sat down to re-write it exactly that way. And just as you mention here, when I saw that it's only the three read functions that benefit, (read_string() is special), I decided it's more readable/maintainable in its current form. But yes, if it grows any, we can definitely do that. So I can at least capture that idea in a comment. > Other than the couple comments here and the alignment thing (different > e-mail). > > Reviewed-by: Jason Ekstrand Thanks. I'll catch the missing mention of blob.h in Makefile.sources as Matt pointed out as well. And like I said, I think I will just hold onto this patch until landing some code that uses it. (It's not like there's going to be any difficulty rebasing it across future changes to Mesa.) -Carl PS. Aside from Gmail's whacky quoting/wrapping, would it be easy for you to omit from code-review emails the parts of the code that you're not replying to? I'm nervous that I might miss some useful comment when I'm cutting those pieces out of my replies. Thanks. pgphYaqwkse5v.pgp Description: PGP signature ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH v2] glsl: Add blob.c---a simple interface for serializing data
On Fri, Dec 12, 2014 at 2:43 PM, Carl Worth wrote:
>
> This new interface allows for writing a series of objects to a chunk
> of memory (a "blob").. The allocated memory is maintained within the
> blob itself, (and re-allocated by doubling when necessary).
>
> There are also functions for reading objects from a blob as well. If
> code attempts to read beyond the available memory, the read functions
> return 0 values (or its moral equivalent) without reading past the
> allocated memory. Once the caller is done with the reads, it can check
> blob->overrun to ensure whether any invalid values were previously
> returned due to attempts to read too far.
> ---
>
> Jason,
>
> This second revision of my patch is in response to your review.
>
> I went to move the align_blob_reader function down by all the other
> blob_reader functions, but I decided not to. You had a concern about the
> alignment code, and it it's wrong then both align_blob() and
> align_blob_reader() will need to be fixed. So it seems useful to me to keep
> them close to each other.
>
> -Carl
>
> src/glsl/Makefile.sources | 3 +-
> src/glsl/blob.c | 295
> ++
> src/glsl/blob.h | 245 ++
> 3 files changed, 542 insertions(+), 1 deletion(-)
> create mode 100644 src/glsl/blob.c
> create mode 100644 src/glsl/blob.h
>
> diff --git a/src/glsl/Makefile.sources b/src/glsl/Makefile.sources
> index 676fa0d..875e4bf 100644
> --- a/src/glsl/Makefile.sources
> +++ b/src/glsl/Makefile.sources
> @@ -105,7 +105,8 @@ LIBGLSL_FILES = \
> $(GLSL_SRCDIR)/opt_swizzle_swizzle.cpp \
> $(GLSL_SRCDIR)/opt_tree_grafting.cpp \
> $(GLSL_SRCDIR)/opt_vectorize.cpp \
> - $(GLSL_SRCDIR)/s_expression.cpp
> + $(GLSL_SRCDIR)/s_expression.cpp \
> + $(GLSL_SRCDIR)/blob.c
>
> # glsl_compiler
>
> diff --git a/src/glsl/blob.c b/src/glsl/blob.c
> new file mode 100644
> index 000..75d3cda
> --- /dev/null
> +++ b/src/glsl/blob.c
> @@ -0,0 +1,295 @@
> +/*
> + * Copyright © 2014 Intel Corporation
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a
> + * copy of this software and associated documentation files (the
> "Software"),
> + * to deal in the Software without restriction, including without
> limitation
> + * the rights to use, copy, modify, merge, publish, distribute,
> sublicense,
> + * and/or sell copies of the Software, and to permit persons to whom the
> + * Software is furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice (including the
> next
> + * paragraph) shall be included in all copies or substantial portions of
> the
> + * Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
> EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
> MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT
> SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR
> OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
> DEALINGS
> + * IN THE SOFTWARE.
> + */
> +
> +#include
> +
> +#include "main/macros.h"
> +#include "util/ralloc.h"
> +#include "blob.h"
> +
> +#define BLOB_INITIAL_SIZE 4096
> +
> +/* Ensure that \blob will be able to fit an additional object of size
> + * \additional. The growing (if any) will occur by doubling the existing
> + * allocation.
> + */
> +static bool
> +grow_to_fit (struct blob *blob, size_t additional)
> +{
> + size_t to_allocate;
> + uint8_t *new_data;
> +
> + if (blob->size + additional <= blob->allocated)
> + return true;
> +
> + if (blob->allocated == 0)
> + to_allocate = BLOB_INITIAL_SIZE;
> + else
> + to_allocate = MAX2(blob->allocated * 2, additional);
>
Should be MAX2(blob->allocated * 2, blob->size + additional)
> +
> + new_data = reralloc_size(blob, blob->data, to_allocate);
> + if (new_data == NULL)
> + return false;
> +
> + blob->data = new_data;
> + blob->allocated = to_allocate;
> +
> + return true;
> +}
> +
> +/* Align the blob->size so that reading or writing a value at (blob->data
> +
> + * blob->size) will result in an access aligned to a granularity of
> \alignment
> + * bytes.
> + *
> + * \return True unless allocation fails
> + */
> +static bool
> +align_blob (struct blob *blob, size_t alignment)
> +{
> + size_t new_size;
> +
> + new_size = ALIGN(blob->size, alignment);
> +
> + if (! grow_to_fit (blob, new_size - blob->size))
> + return false;
> +
> + blob->size = new_size;
> +
> + return true;
> +}
> +
> +static void
> +align_blob_reader (struct blob_reader *blob, size_t alignment)
> +{
> + blob->current = blob->data + ALIGN(blob->current - blob->data,
> alignment);
> +}
> +
> +struct blob *
> +blob_create (void *mem_ctx
Re: [Mesa-dev] [PATCH v2] glsl: Add blob.c---a simple interface for serializing data
On Fri, Dec 12, 2014 at 2:43 PM, Carl Worth wrote: > diff --git a/src/glsl/Makefile.sources b/src/glsl/Makefile.sources > index 676fa0d..875e4bf 100644 > --- a/src/glsl/Makefile.sources > +++ b/src/glsl/Makefile.sources > @@ -105,7 +105,8 @@ LIBGLSL_FILES = \ > $(GLSL_SRCDIR)/opt_swizzle_swizzle.cpp \ > $(GLSL_SRCDIR)/opt_tree_grafting.cpp \ > $(GLSL_SRCDIR)/opt_vectorize.cpp \ > - $(GLSL_SRCDIR)/s_expression.cpp > + $(GLSL_SRCDIR)/s_expression.cpp \ > + $(GLSL_SRCDIR)/blob.c > Please add blob.h to this list as well before committing. ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
