[Mesa-dev] [PATCH 2/2] mesa/glthread: fallback to sync if count validation fails

[Nicolai Hähnle]

On 29.03.2017 07:30, Timothy Arceri wrote:
> The old code would sync and then throw a cryptic error message.
> There is no need for a custome error, we can just fallback to
> the real function and have it do proper validation.
>
> Fixes piglit test:
> glsl-uniform-out-of-bounds
>
> Which was returning the wrong error code.
> ---
>  src/mapi/glapi/gen/gl_marshal.py | 22 ++++++++++++++--------
>  1 file changed, 14 insertions(+), 8 deletions(-)
>
> diff --git a/src/mapi/glapi/gen/gl_marshal.py b/src/mapi/glapi/gen/gl_marshal.py
> index 9639f9c..92bc0e4 100644
> --- a/src/mapi/glapi/gen/gl_marshal.py
> +++ b/src/mapi/glapi/gen/gl_marshal.py
> @@ -197,71 +197,77 @@ class PrintCode(gl_XML.gl_print_base):
>                              out('{0} = NULL;'.format(p.name))
>                          out('else')
>                          with indent():
>                              out('variable_data += {0};'.format(p.size_string(False)))
>                      else:
>                          out('variable_data += {0};'.format(p.size_string(False)))
>
>              self.print_sync_call(func)
>          out('}')
>
> -    def validate_count_or_return(self, func):
> +    def validate_count_or_fallback(self, func):
>          # Check that any counts for variable-length arguments might be < 0, in
>          # which case the command alloc or the memcpy would blow up before we
>          # get to the validation in Mesa core.
>          for p in func.parameters:
>              if p.is_variable_length():
>                  out('if (unlikely({0} < 0)) {{'.format(p.size_string()))
>                  with indent():
> -                    out('_mesa_glthread_finish(ctx);')
> -                    out('_mesa_error(ctx, GL_INVALID_VALUE, "{0}({1} < 0)");'.format(func.name, p.size_string()))
> -                    out('return;')
> +                    out('goto fallback_to_sync;')
>                  out('}')
> +                return True
> +        return False
> +
>
>      def print_async_marshal(self, func):
> +        validate_count = False

I'd suggest renaming the variable to need_fallback_sync to more clearly 
describe what it really does -- maybe there'll be more goto 
fallback_sync; paths in the future.

Either way, both patches are

Reviewed-by: Nicolai Hähnle <nicolai.haehnle at amd.com>

>          out('static void GLAPIENTRY')
>          out('_mesa_marshal_{0}({1})'.format(
>                  func.name, func.get_parameter_string()))
>          out('{')
>          with indent():
>              out('GET_CURRENT_CONTEXT(ctx);')
>              struct = 'struct marshal_cmd_{0}'.format(func.name)
>              size_terms = ['sizeof({0})'.format(struct)]
>              for p in func.variable_params:
>                  size = p.size_string()
>                  if p.img_null_flag:
>                      size = '({0} ? {1} : 0)'.format(p.name, size)
>                  size_terms.append(size)
>              out('size_t cmd_size = {0};'.format(' + '.join(size_terms)))
>              out('{0} *cmd;'.format(struct))
>
>              out('debug_print_marshal("{0}");'.format(func.name))
>
> -            self.validate_count_or_return(func)
> +            validate_count = self.validate_count_or_fallback(func)
>
>              if func.marshal_fail:
>                  out('if ({0}) {{'.format(func.marshal_fail))
>                  with indent():
>                      out('_mesa_glthread_finish(ctx);')
>                      out('_mesa_glthread_restore_dispatch(ctx);')
>                      self.print_sync_dispatch(func)
>                      out('return;')
>                  out('}')
>
>              out('if (cmd_size <= MARSHAL_MAX_CMD_SIZE) {')
>              with indent():
>                  self.print_async_dispatch(func)
> -            out('} else {')
> -            with indent():
> -                self.print_sync_dispatch(func)
> +                out('return;')
>              out('}')
>
> +        out('')
> +        if validate_count:
> +            out('fallback_to_sync:')
> +        with indent():
> +            self.print_sync_dispatch(func)
> +
>          out('}')
>
>      def print_async_body(self, func):
>          out('/* {0}: marshalled asynchronously */'.format(func.name))
>          self.print_async_struct(func)
>          self.print_async_unmarshal(func)
>          self.print_async_marshal(func)
>          out('')
>          out('')
>
>


-- 
Lerne, wie die Welt wirklich ist,
Aber vergiss niemals, wie sie sein sollte.