On Tue, Sep 5, 2017 at 5:32 PM, Ian Romanick <i...@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; > > > >
_______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev