<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Tue, Sep 5, 2017 at 5:32 PM, Ian Romanick <span dir="ltr"><<a href="mailto:idr@freedesktop.org" target="_blank">idr@freedesktop.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="HOEnZb"><div class="h5">On 08/17/2017 10:22 AM, Jason Ekstrand wrote:<br>
> These helpers are much nicer than just using assert because they don't<br>
> kill your process.  Instead, it longjmps back to spirv_to_nir(), cleans<br>
> up all the temporary memory, and nicely returns NULL.  While crashing is<br>
> completely OK in the Vulkan world, it's not considered to be quite so<br>
> nice in GL.  This should help us to make SPIR-V parsing much more<br>
> robust.  The one downside here is that vtn_assert is not compiled out in<br>
> release builds like assert() is so it isn't free.<br>
> ---<br>
>  src/compiler/spirv/spirv_to_<wbr>nir.c | 20 ++++++++++++++++++++<br>
>  src/compiler/spirv/vtn_<wbr>private.h  | 31 ++++++++++++++++++++++++++++++<wbr>+<br>
>  2 files changed, 51 insertions(+)<br>
><br>
> diff --git a/src/compiler/spirv/spirv_to_<wbr>nir.c b/src/compiler/spirv/spirv_to_<wbr>nir.c<br>
> index e59f2b2..af542e8 100644<br>
> --- a/src/compiler/spirv/spirv_to_<wbr>nir.c<br>
> +++ b/src/compiler/spirv/spirv_to_<wbr>nir.c<br>
> @@ -104,6 +104,20 @@ _vtn_warn(struct vtn_builder *b, const char *file, unsigned line,<br>
>     va_end(args);<br>
>  }<br>
><br>
> +void<br>
> +_vtn_fail(struct vtn_builder *b, const char *file, unsigned line,<br>
> +          const char *fmt, ...)<br>
> +{<br>
> +   va_list args;<br>
> +<br>
> +   va_start(args, fmt);<br>
> +   vtn_log_err(b, NIR_SPIRV_DEBUG_LEVEL_ERROR, "SPIR-V parsing FAILED:\n",<br>
> +               file, line, fmt, args);<br>
> +   va_end(args);<br>
> +<br>
> +   longjmp(b->fail_jump, 1);<br>
> +}<br>
> +<br>
>  struct spec_constant_value {<br>
>     bool is_double;<br>
>     union {<br>
> @@ -3420,6 +3434,12 @@ spirv_to_nir(const uint32_t *words, size_t word_count,<br>
>     b->entry_point_name = entry_point_name;<br>
>     b->ext = ext;<br>
><br>
> +   /* See also _vtn_fail() */<br>
> +   if (setjmp(b->fail_jump)) {<br>
> +      ralloc_free(b);<br>
> +      return NULL;<br>
> +   }<br>
> +<br>
>     const uint32_t *word_end = words + word_count;<br>
><br>
>     /* Handle the SPIR-V header (first 4 dwords)  */<br>
> diff --git a/src/compiler/spirv/vtn_<wbr>private.h b/src/compiler/spirv/vtn_<wbr>private.h<br>
> index 3eb601d..f640289 100644<br>
> --- a/src/compiler/spirv/vtn_<wbr>private.h<br>
> +++ b/src/compiler/spirv/vtn_<wbr>private.h<br>
> @@ -28,6 +28,8 @@<br>
>  #ifndef _VTN_PRIVATE_H_<br>
>  #define _VTN_PRIVATE_H_<br>
><br>
> +#include <setjmp.h><br>
> +<br>
>  #include "nir/nir.h"<br>
>  #include "nir/nir_builder.h"<br>
>  #include "util/u_dynarray.h"<br>
> @@ -49,6 +51,32 @@ void _vtn_warn(struct vtn_builder *b, const char *file, unsigned line,<br>
>                 const char *fmt, ...) PRINTFLIKE(4, 5);<br>
>  #define vtn_warn(...) _vtn_warn(b, __FILE__, __LINE__, __VA_ARGS__)<br>
><br>
> +/** Fail SPIR-V parsing<br>
> + *<br>
> + * This function logs an error and then bails out of the shader compile using<br>
> + * longjmp.  This being safe relies on two things:<br>
> + *<br>
> + *  1) We must guarantee that setjmp is called after allocating the builder<br>
> + *     and setting up b->debug (so that logging works) but before before any<br>
> + *     errors have a chance to occur.<br>
> + *<br>
> + *  2) While doing the SPIR-V -> NIR conversion, we need to be careful to<br>
> + *     ensure that all heap allocations happen through ralloc and are parented<br>
> + *     to the builder.<br>
> + *<br>
> + * So long as these two things continue to hold, we can easily longjmp back to<br>
> + * spirv_to_nir(), clean up the builder, and return NULL.<br>
> + */<br>
> +void _vtn_fail(struct vtn_builder *b, const char *file, unsigned line,<br>
> +               const char *fmt, ...) NORETURN PRINTFLIKE(4, 5);<br>
> +#define vtn_fail(...) _vtn_fail(b, __FILE__, __LINE__, __VA_ARGS__)<br>
> +<br>
> +#define vtn_assert(expr) \<br>
> +   do { \<br>
> +      if (!likely(expr)) \<br>
> +         vtn_fail("%s", #expr); \<br>
> +   } while (0)<br>
<br>
</div></div>I'm not a huge fan of this particular detail.  When you see "assert" in<br>
a name, that carries a bunch of implicit information with it.  In this<br>
case, that information is, by design, not true.  Primarily, it does<br>
happen in release builds.  It does still lead to an abrupt failure, but<br>
a different sort.  Maybe vtn_fail_when() would be better... the down<br>
side of that is all the conditions would have to inverted in the next<br>
patch.  Ugh.<br></blockquote><div><br></div><div>Yeah, I understand both of those reservations.  The one that concerns me the most is that it happens in debug builds; that's definitely unexpected.  As far as aborting, it does perform a full stop, it's just not quite the same.  I'd be ok with switching over to something else.  How about vtn_fail_if?  Or maybe we could follow the perl pattern and do vtn_or_fail.  Thoughs?<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
For that reason, it makes me uncomfortable when I see things with<br>
side-effects in a thing called foo_assert() (the SpvOpExtInst in the<br>
next patch).<br></blockquote><div><br></div><div>Yeah, the side-effects are a bit desturbing.  I'm happy to change that.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Hm... I'm not sure what to suggest, and this series has been out for a<br>
couple weeks.  What are your thoughts?<br><div class="HOEnZb"><div class="h5"></div></div></blockquote><div><br></div><div>No worries.  I'm not in too much of a rush.  But being in a little bit of a rush can sometimes encourage patch review to actually happen. :-)  Thanks!</div><div><br></div><div>--Jason<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="HOEnZb"><div class="h5">
> +<br>
>  enum vtn_value_type {<br>
>     vtn_value_type_invalid = 0,<br>
>     vtn_value_type_undef,<br>
> @@ -474,6 +502,9 @@ struct vtn_decoration {<br>
>  struct vtn_builder {<br>
>     nir_builder nb;<br>
><br>
> +   /* Used by vtn_fail to jump back to the beginning of SPIR-V compilation */<br>
> +   jmp_buf fail_jump;<br>
> +<br>
>     const uint32_t *spirv;<br>
><br>
>     nir_shader *shader;<br>
><br>
<br>
</div></div></blockquote></div><br></div></div>