[Mesa-dev] [PATCH 7/9] spirv: Add vtn_fail and vtn_assert helpers

[Ian Romanick]

On 08/17/2017 10:22 AM, Jason Ekstrand wrote:
> These helpers are much nicer than just using assert because they don't
> kill your process.  Instead, it longjmps back to spirv_to_nir(), cleans
> up all the temporary memory, and nicely returns NULL.  While crashing is
> completely OK in the Vulkan world, it's not considered to be quite so
> nice in GL.  This should help us to make SPIR-V parsing much more
> robust.  The one downside here is that vtn_assert is not compiled out in
> release builds like assert() is so it isn't free.
> ---
>  src/compiler/spirv/spirv_to_nir.c | 20 ++++++++++++++++++++
>  src/compiler/spirv/vtn_private.h  | 31 +++++++++++++++++++++++++++++++
>  2 files changed, 51 insertions(+)
> 
> diff --git a/src/compiler/spirv/spirv_to_nir.c b/src/compiler/spirv/spirv_to_nir.c
> index e59f2b2..af542e8 100644
> --- a/src/compiler/spirv/spirv_to_nir.c
> +++ b/src/compiler/spirv/spirv_to_nir.c
> @@ -104,6 +104,20 @@ _vtn_warn(struct vtn_builder *b, const char *file, unsigned line,
>     va_end(args);
>  }
>  
> +void
> +_vtn_fail(struct vtn_builder *b, const char *file, unsigned line,
> +          const char *fmt, ...)
> +{
> +   va_list args;
> +
> +   va_start(args, fmt);
> +   vtn_log_err(b, NIR_SPIRV_DEBUG_LEVEL_ERROR, "SPIR-V parsing FAILED:\n",
> +               file, line, fmt, args);
> +   va_end(args);
> +
> +   longjmp(b->fail_jump, 1);
> +}
> +
>  struct spec_constant_value {
>     bool is_double;
>     union {
> @@ -3420,6 +3434,12 @@ spirv_to_nir(const uint32_t *words, size_t word_count,
>     b->entry_point_name = entry_point_name;
>     b->ext = ext;
>  
> +   /* See also _vtn_fail() */
> +   if (setjmp(b->fail_jump)) {
> +      ralloc_free(b);
> +      return NULL;
> +   }
> +
>     const uint32_t *word_end = words + word_count;
>  
>     /* Handle the SPIR-V header (first 4 dwords)  */
> diff --git a/src/compiler/spirv/vtn_private.h b/src/compiler/spirv/vtn_private.h
> index 3eb601d..f640289 100644
> --- a/src/compiler/spirv/vtn_private.h
> +++ b/src/compiler/spirv/vtn_private.h
> @@ -28,6 +28,8 @@
>  #ifndef _VTN_PRIVATE_H_
>  #define _VTN_PRIVATE_H_
>  
> +#include <setjmp.h>
> +
>  #include "nir/nir.h"
>  #include "nir/nir_builder.h"
>  #include "util/u_dynarray.h"
> @@ -49,6 +51,32 @@ void _vtn_warn(struct vtn_builder *b, const char *file, unsigned line,
>                 const char *fmt, ...) PRINTFLIKE(4, 5);
>  #define vtn_warn(...) _vtn_warn(b, __FILE__, __LINE__, __VA_ARGS__)
>  
> +/** Fail SPIR-V parsing
> + *
> + * This function logs an error and then bails out of the shader compile using
> + * longjmp.  This being safe relies on two things:
> + *
> + *  1) We must guarantee that setjmp is called after allocating the builder
> + *     and setting up b->debug (so that logging works) but before before any
> + *     errors have a chance to occur.
> + *
> + *  2) While doing the SPIR-V -> NIR conversion, we need to be careful to
> + *     ensure that all heap allocations happen through ralloc and are parented
> + *     to the builder.
> + *
> + * So long as these two things continue to hold, we can easily longjmp back to
> + * spirv_to_nir(), clean up the builder, and return NULL.
> + */
> +void _vtn_fail(struct vtn_builder *b, const char *file, unsigned line,
> +               const char *fmt, ...) NORETURN PRINTFLIKE(4, 5);
> +#define vtn_fail(...) _vtn_fail(b, __FILE__, __LINE__, __VA_ARGS__)
> +
> +#define vtn_assert(expr) \
> +   do { \
> +      if (!likely(expr)) \
> +         vtn_fail("%s", #expr); \
> +   } while (0)

I'm not a huge fan of this particular detail.  When you see "assert" in
a name, that carries a bunch of implicit information with it.  In this
case, that information is, by design, not true.  Primarily, it does
happen in release builds.  It does still lead to an abrupt failure, but
a different sort.  Maybe vtn_fail_when() would be better... the down
side of that is all the conditions would have to inverted in the next
patch.  Ugh.

For that reason, it makes me uncomfortable when I see things with
side-effects in a thing called foo_assert() (the SpvOpExtInst in the
next patch).

Hm... I'm not sure what to suggest, and this series has been out for a
couple weeks.  What are your thoughts?

> +
>  enum vtn_value_type {
>     vtn_value_type_invalid = 0,
>     vtn_value_type_undef,
> @@ -474,6 +502,9 @@ struct vtn_decoration {
>  struct vtn_builder {
>     nir_builder nb;
>  
> +   /* Used by vtn_fail to jump back to the beginning of SPIR-V compilation */
> +   jmp_buf fail_jump;
> +
>     const uint32_t *spirv;
>  
>     nir_shader *shader;
>