[Mesa-dev] [PATCH 16/17] util: Add Mesa ARB_get_program_binary helper functions

[Nicolai Hähnle]

On 09.11.2017 07:42, Jordan Justen wrote:
> Signed-off-by: Jordan Justen <jordan.l.justen at intel.com>
> ---
>   src/util/Makefile.sources |   2 +
>   src/util/meson.build      |   2 +
>   src/util/program_binary.c | 322 ++++++++++++++++++++++++++++++++++++++++++++++
>   src/util/program_binary.h |  91 +++++++++++++
>   4 files changed, 417 insertions(+)
>   create mode 100644 src/util/program_binary.c
>   create mode 100644 src/util/program_binary.h
> 
> diff --git a/src/util/Makefile.sources b/src/util/Makefile.sources
> index c7f6516a992..d9048bbd182 100644
> --- a/src/util/Makefile.sources
> +++ b/src/util/Makefile.sources
> @@ -21,6 +21,8 @@ MESA_UTIL_FILES := \
>   	macros.h \
>   	mesa-sha1.c \
>   	mesa-sha1.h \
> +	program_binary.c \
> +	program_binary.h \
>   	sha1/sha1.c \
>   	sha1/sha1.h \
>   	ralloc.c \
> diff --git a/src/util/meson.build b/src/util/meson.build
> index c9cb3e861e9..9bc10222a72 100644
> --- a/src/util/meson.build
> +++ b/src/util/meson.build
> @@ -45,6 +45,8 @@ files_mesa_util = files(
>     'macros.h',
>     'mesa-sha1.c',
>     'mesa-sha1.h',
> +  'program_binary.c',
> +  'program_binary.h',
>     'sha1/sha1.c',
>     'sha1/sha1.h',
>     'ralloc.c',
> diff --git a/src/util/program_binary.c b/src/util/program_binary.c
> new file mode 100644
> index 00000000000..4447dd632d9
> --- /dev/null
> +++ b/src/util/program_binary.c
> @@ -0,0 +1,322 @@
> +/*
> + * Mesa 3-D graphics library
> + *
> + * Copyright (c) 2017 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 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.
> + */
> +
> +/**
> + * \file program_binary.c
> + *
> + * Helper functions for serializing a binary program.
> + */
> +
> +
> +#include "main/mtypes.h"
> +#include "crc32.h"
> +#include "program_binary.h"
> +#include "zlib.h"
> +
> +/**
> + * Mesa supports one binary format, but it must differentiate between formats
> + * produced by different drivers and different Mesa versions.
> + *
> + * Mesa uses a uint32_t value to specify an internal format. The only format
> + * defined has one uint32_t value of 0, followed by 20 bytes specifying a sha1
> + * that uniquely identifies the Mesa driver type and version.
> + */
> +
> +struct program_binary_header {
> +   /* If internal_format is 0, it must be followed by the 20 byte sha1 that
> +    * identifies the Mesa driver and version supported. If we want to support
> +    * something besides a sha1, then a new internal_format value can be added.
> +    */
> +   uint32_t internal_format;
> +   uint8_t sha1[20];
> +   /* Fields following sha1 can be changed since the sha1 will guarantee that
> +    * the binary only works with the same Mesa version.
> +    */
> +   uint32_t deflated_size;
> +   uint32_t inflated_size;
> +   uint32_t crc32;
> +};
> +
> +unsigned
> +get_program_binary_max_size(unsigned payload_size)
> +{
> +   return sizeof(struct program_binary_header) + payload_size;

What about arithmetic overflows?


[snip]
> +/**
> + * Performs basic checks on a binary to see if it is valid.
> + *
> + * The contents of the payload are not checked.
> + */
> +extern bool
> +is_program_binary_valid(GLenum binary_format, const void *sha1,
> +                        const void *binary, unsigned length);
> +
> +/**
> + * Finds the offset of the payload within the binary if the payload is
> + * uncompressed.
> + *
> + * If -1 is returned, then the payload was compressed, and
> + * extract_program_binary_payload must be used.
> + */
> +extern int
> +program_binary_payload_offset(const void *binary, unsigned length);
> +
> +/**
> + * Decompresses the compressed payload within a binary.
> + *
> + * Before using this function, call program_binary_payload_offset to see if
> + * the payload data is uncompressed in the binary.
> + *
> + * If the data is decompressed, then the returned buffer is must be freed by
> + * the caller with ralloc_free.
> + */
> +extern void
> +extract_program_binary_payload(const void *binary, unsigned length,
> +                               void **payload, unsigned *payload_length);

I think it would be cleaner if these three functions were integrated 
into one:

bool
extract_program_binary_payload(const void *binary, unsigned length,
                                const void *sha1,
                                void **payload, unsigned *payload_length,
                                void **tempmem);
// caller is supposed to call ralloc_free(*tempmem) unconditionally 
(though it may be NULL)

This would allow inlining the IMHO awkward simple_header_checks 
functions and would save all callers from having to duplicate the call 
sequence for these three functions.

Though checking the binary_format enum would be up to the caller, which 
also makes more sense since we don't usually deal with GL stuff in src/util.

Cheers,
Nicolai

> +
> +#ifdef __cplusplus
> +}
> +#endif
> +
> +#endif /* PROGRAM_BINARY_H */
> 


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