[Mesa-dev] [PATCH 06/11] compiler/blob: make blob_reserve_bytes() more useful

[Nicolai Hähnle]

On 11.10.2017 22:38, Jason Ekstrand wrote:
> From: Connor Abbott <cwabbott0 at gmail.com>
> 
> Despite the name, it could only be used if you immediately wrote to the
> pointer. Noboby was using it outside of one test, so clearly this
> behavior wasn't that useful. Instead, make it return an offset into the
> data buffer so that the result isn't invalidated if you later write to
> the blob. In conjunction with blob_overwrite_bytes(), this will be
> useful for leaving a placeholder and then filling it in later, which
> we'll need to do for handling phi nodes when serializing NIR.
> 
> v2 (Jason Ekstrand):
>   - Improve the blob_overwrite_uint32 documentation
>   - Detect overflow in the offset + to_write computation
>   - Add a blob_reserve_uint32 helper
>   - Add a blob_overwrite_intptr helper
> ---
>   src/compiler/blob.c                 | 37 +++++++++++++++++++++----
>   src/compiler/blob.h                 | 54 ++++++++++++++++++++++++++-----------
>   src/compiler/glsl/tests/blob_test.c |  4 +--
>   3 files changed, 73 insertions(+), 22 deletions(-)
> 
> diff --git a/src/compiler/blob.c b/src/compiler/blob.c
> index 59ad8a3..c5ed9f5 100644
> --- a/src/compiler/blob.c
> +++ b/src/compiler/blob.c
> @@ -130,7 +130,7 @@ blob_overwrite_bytes(struct blob *blob,
>                        size_t to_write)
>   {
>      /* Detect an attempt to overwrite data out of bounds. */
> -   if (blob->size < offset + to_write)
> +   if (offset + to_write < offset || blob->size < offset + to_write)
>         return false;
>   
>      VG(VALGRIND_CHECK_MEM_IS_DEFINED(bytes, to_write));
> @@ -156,20 +156,34 @@ blob_write_bytes(struct blob *blob, const void *bytes, size_t to_write)
>      return true;
>   }
>   
> -uint8_t *
> +ssize_t
>   blob_reserve_bytes(struct blob *blob, size_t to_write)
>   {
> -   uint8_t *ret;
> +   ssize_t ret;
>   
>      if (! grow_to_fit (blob, to_write))
> -      return NULL;
> +      return -1;
>   
> -   ret = blob->data + blob->size;
> +   ret = blob->size;
>      blob->size += to_write;
>   
>      return ret;
>   }
>   
> +ssize_t
> +blob_reserve_uint32(struct blob *blob)
> +{
> +   align_blob(blob, sizeof(uint32_t));
> +   return blob_reserve_bytes(blob, sizeof(uint32_t));
> +}
> +
> +ssize_t
> +blob_reserve_intptr(struct blob *blob)
> +{
> +   align_blob(blob, sizeof(intptr_t));
> +   return blob_reserve_bytes(blob, sizeof(intptr_t));
> +}
> +
>   bool
>   blob_write_uint32(struct blob *blob, uint32_t value)
>   {
> @@ -178,11 +192,15 @@ blob_write_uint32(struct blob *blob, uint32_t value)
>      return blob_write_bytes(blob, &value, sizeof(value));
>   }
>   
> +#define ASSERT_ALIGNED(_offset, _align) \
> +   assert(ALIGN((_offset), (_align)) == (_offset))
> +
>   bool
>   blob_overwrite_uint32 (struct blob *blob,
>                          size_t offset,
>                          uint32_t value)
>   {
> +   ASSERT_ALIGNED(offset, sizeof(value));
>      return blob_overwrite_bytes(blob, offset, &value, sizeof(value));
>   }
>   
> @@ -203,6 +221,15 @@ blob_write_intptr(struct blob *blob, intptr_t value)
>   }
>   
>   bool
> +blob_overwrite_intptr (struct blob *blob,
> +                       size_t offset,
> +                       intptr_t value)
> +{
> +   ASSERT_ALIGNED(offset, sizeof(value));
> +   return blob_overwrite_bytes(blob, offset, &value, sizeof(value));
> +}
> +
> +bool
>   blob_write_string(struct blob *blob, const char *str)
>   {
>      return blob_write_bytes(blob, str, strlen(str) + 1);
> diff --git a/src/compiler/blob.h b/src/compiler/blob.h
> index 1ef6d99..ad4b6fa 100644
> --- a/src/compiler/blob.h
> +++ b/src/compiler/blob.h
> @@ -126,24 +126,32 @@ blob_write_bytes(struct blob *blob, const void *bytes, size_t to_write);
>    * Reserve space in \blob for a number of bytes.
>    *
>    * Space will be allocated within the blob for these byes, but the bytes will
> - * be left uninitialized. The caller is expected to use the return value to
> - * write directly (and immediately) to these bytes.
> + * be left uninitialized. The caller is expected to use \sa
> + * blob_overwrite_bytes to write to these bytes.
>    *
> - * \note The return value is valid immediately upon return, but can be
> - * invalidated by any other call to a blob function. So the caller should call
> - * blob_reserve_byes immediately before writing through the returned pointer.
> - *
> - * This function is intended to be used when interfacing with an existing API
> - * that is not aware of the blob API, (so that blob_write_bytes cannot be
> - * called).
> - *
> - * \return A pointer to space allocated within \blob to which \to_write bytes
> - * can be written, (or NULL in case of any allocation error).
> + * \return An offset to space allocated within \blob to which \to_write bytes
> + * can be written, (or -1 in case of any allocation error).
>    */
> -uint8_t *
> +ssize_t
>   blob_reserve_bytes(struct blob *blob, size_t to_write);
>   
>   /**
> + * Similar to \sa blob_reserve_bytes, but only reserves an uint32_t worth of
> + * space. Note that this must be used if later reading with \sa
> + * blob_read_uint32, since it aligns the offset correctly.
> + */
> +ssize_t
> +blob_reserve_uint32(struct blob *blob);
> +
> +/**
> + * Similar to \sa blob_reserve_bytes, but only reserves an intptr_t worth of
> + * space. Note that this must be used if later reading with \sa
> + * blob_read_intptr, since it aligns the offset correctly.
> + */
> +ssize_t
> +blob_reserve_intptr(struct blob *blob);
> +
> +/**
>    * Overwrite some data previously written to the blob.
>    *
>    * Writes data to an existing portion of the blob at an offset of \offset.
> @@ -186,8 +194,7 @@ blob_write_uint32(struct blob *blob, uint32_t value);
>    *
>    *	size_t offset;
>    *
> - *	offset = blob->size;
> - *	blob_write_uint32 (blob, 0); // placeholder
> + *	offset = blob_reserve_uint32(blob);
>    *	... various blob write calls, writing N items ...
>    *	blob_overwrite_uint32 (blob, offset, N);
>    *
> @@ -226,6 +233,23 @@ bool
>   blob_write_intptr(struct blob *blob, intptr_t value);
>   
>   /**
> + * Overwrite a uint32_t previously written to the blob.

s/a uint32_t/an intptr_t/

Also on the line just below.

Cheers,
Nicolai

> + *
> + * Writes a uint32_t value to an existing portion of the blob at an offset of
> + * \offset.  This data range must have previously been written to the blob by
> + * one of the blob_write_* calls.
> + *
> + * For example usage, see blob_overwrite_uint32
> + *
> + * \return True unless the requested position or position+to_write lie outside
> + * the current blob's size.
> + */
> +bool
> +blob_overwrite_intptr(struct blob *blob,
> +                      size_t offset,
> +                      intptr_t value);
> +
> +/**
>    * Add a NULL-terminated string to a blob, (including the NULL terminator).
>    *
>    * \return True unless allocation failed.
> diff --git a/src/compiler/glsl/tests/blob_test.c b/src/compiler/glsl/tests/blob_test.c
> index df0e91a..1b3ab6e 100644
> --- a/src/compiler/glsl/tests/blob_test.c
> +++ b/src/compiler/glsl/tests/blob_test.c
> @@ -120,7 +120,7 @@ test_write_and_read_functions (void)
>   {
>      struct blob *blob;
>      struct blob_reader reader;
> -   uint8_t *reserved;
> +   ssize_t reserved;
>      size_t str_offset, uint_offset;
>      uint8_t reserve_buf[sizeof(reserve_test_str)];
>   
> @@ -131,7 +131,7 @@ test_write_and_read_functions (void)
>      blob_write_bytes(blob, bytes_test_str, sizeof(bytes_test_str));
>   
>      reserved = blob_reserve_bytes(blob, sizeof(reserve_test_str));
> -   memcpy(reserved, reserve_test_str, sizeof(reserve_test_str));
> +   blob_overwrite_bytes(blob, reserved, reserve_test_str, sizeof(reserve_test_str));
>   
>      /* Write a placeholder, (to be replaced later via overwrite_bytes) */
>      str_offset = blob->size;
> 


-- 
Lerne, wie die Welt wirklich ist,
Aber vergiss niemals, wie sie sein sollte.