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

[Jason Ekstrand]

On Tue, Sep 5, 2017 at 5:32 PM, Ian Romanick <idr at freedesktop.org> wrote:

> 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.
>

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?


> 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).
>

Yeah, the side-effects are a bit desturbing.  I'm happy to change that.


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

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!

--Jason


> > +
> >  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;
> >
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20170905/6c644af8/attachment-0001.html>