<div dir="ltr"> <div class="gmail_extra"> <div class="gmail_quote">On Thu, Aug 17, 2017 at 10:29 AM, Erik Faye-Lund <<a href="mailto:kusmabite@gmail.com" target="_blank">kusmabite@gmail.com</a>> wrote: <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 Thu, Aug 17, 2017 at 7:22 PM, Jason Ekstrand <<a href="mailto:jason@jlekstrand.net">jason@jlekstrand.net</a>> 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. > + */ </div></div>Technically speaking, there's probably some more requirements, but they might not be relevant here, for instance that no failures can happen while mutex locks are being held. </blockquote><div> </div></div></div><div class="gmail_extra">Right. That wouldn't be bad to state. Fortunately, all uses of mutexes from within spirv_to_nir are encapsulated in glsl_type functions so we have no way to get back into spirv_to_nir code while a mutex is held. I'll add that one to the list.</div><div class="gmail_extra"> </div><div class="gmail_extra">--Jason </div></div>