[Mesa-dev] [PATCH] avoid crashing in mod by 0 with llvmpipe

[Roland Scheidegger]

Ok, pushed.

Roland

Am 15.01.2016 um 21:01 schrieb Jeff Muizelaar:
> Do you mind fixing up the language to match what you're looking for
> and committing it?
> 
> Thanks,
> 
> -Jeff
> 
> On Fri, Jan 15, 2016 at 3:00 PM, Roland Scheidegger <sroland at vmware.com> wrote:
>> Am 15.01.2016 um 20:13 schrieb Jeff Muizelaar:
>>> This adds code that is basically the same as the code in smod, div and idiv.
>> That would be umod, not smod, above (and udiv).
>>
>>> However, unlike idiv we return -1.
>>>
>>> diff --git a/src/gallium/auxiliary/gallivm/lp_bld_tgsi_action.c
>>> b/src/gallium/auxiliary/gallivm/lp_bld_tgsi_action.c
>>> index 0ad78b0..d17ef31 100644
>>> --- a/src/gallium/auxiliary/gallivm/lp_bld_tgsi_action.c
>>> +++ b/src/gallium/auxiliary/gallivm/lp_bld_tgsi_action.c
>>> @@ -1524,18 +1524,32 @@ min_emit_cpu(
>>>
>>>  /* TGSI_OPCODE_MOD (CPU Only) */
>>>  static void
>>>  mod_emit_cpu(
>>>     const struct lp_build_tgsi_action * action,
>>>     struct lp_build_tgsi_context * bld_base,
>>>     struct lp_build_emit_data * emit_data)
>>>  {
>>> -   emit_data->output[emit_data->chan] = lp_build_mod(&bld_base->int_bld,
>>> -                                   emit_data->args[0], emit_data->args[1]);
>>> +   LLVMBuilderRef builder = bld_base->base.gallivm->builder;
>>> +   LLVMValueRef div_mask = lp_build_cmp(&bld_base->uint_bld,
>>> +                    PIPE_FUNC_EQUAL, emit_data->args[1],
>>> +                    bld_base->uint_bld.zero);
>>> +   /* We want to make sure that we never divide/mod by zero to not
>>> +    * generate sigfpe. We don't want to crash just because the
>>> +    * shader is doing something weird. */
>>> +   LLVMValueRef divisor = LLVMBuildOr(builder,
>>> +                      div_mask,
>>> +                      emit_data->args[1], "");
>>> +   LLVMValueRef result = lp_build_mod(&bld_base->int_bld,
>>> +                      emit_data->args[0], divisor);
>>> +   /* umod by zero is guaranteed to return 0xffffffff */
>> I think this should use language along the lines of what idiv does, i.e.
>> no guaranteed value, pick -1. Because we don't say anything about that
>> in the gallium tgsi docs section about that (unlike udiv/umod).
>> (AFAIK we only really care for udiv/umod due to d3d10, which doesn't
>> have the signed versions - glsl doesn't care either way.)
>>
>>> +   emit_data->output[emit_data->chan] = LLVMBuildOr(builder,
>>> +                            div_mask,
>>> +                            result, "");
>>>  }
>>>
>>>  /* TGSI_OPCODE_NOT */
>>>  static void
>>>  not_emit_cpu(
>>>     const struct lp_build_tgsi_action * action,
>>>     struct lp_build_tgsi_context * bld_base,
>>>     struct lp_build_emit_data * emit_data)
>>
>> Subject should probably say: gallivm: ...
>>
>> With that fixed: Reviewed-by: Roland Scheidegger <sroland at vmware.com>
>>