[Mesa-dev] [PATCH] gallium: fixed modulo zero crashes in tgsi interpreter
softpipe throws integer division by zero exceptions on windows when using % with integers in a geometry shader. --- src/gallium/auxiliary/tgsi/tgsi_exec.c | 24 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/src/gallium/auxiliary/tgsi/tgsi_exec.c b/src/gallium/auxiliary/tgsi/tgsi_exec.c index c41954c..abd2d16 100644 --- a/src/gallium/auxiliary/tgsi/tgsi_exec.c +++ b/src/gallium/auxiliary/tgsi/tgsi_exec.c @@ -866,20 +866,20 @@ static void micro_u64mod(union tgsi_double_channel *dst, const union tgsi_double_channel *src) { - dst->u64[0] = src[0].u64[0] % src[1].u64[0]; - dst->u64[1] = src[0].u64[1] % src[1].u64[1]; - dst->u64[2] = src[0].u64[2] % src[1].u64[2]; - dst->u64[3] = src[0].u64[3] % src[1].u64[3]; + dst->u64[0] = src[1].u64[0] ? src[0].u64[0] % src[1].u64[0] : UINT64_MAX; + dst->u64[1] = src[1].u64[1] ? src[0].u64[1] % src[1].u64[1] : UINT64_MAX; + dst->u64[2] = src[1].u64[2] ? src[0].u64[2] % src[1].u64[2] : UINT64_MAX; + dst->u64[3] = src[1].u64[3] ? src[0].u64[3] % src[1].u64[3] : UINT64_MAX; } static void micro_i64mod(union tgsi_double_channel *dst, const union tgsi_double_channel *src) { - dst->i64[0] = src[0].i64[0] % src[1].i64[0]; - dst->i64[1] = src[0].i64[1] % src[1].i64[1]; - dst->i64[2] = src[0].i64[2] % src[1].i64[2]; - dst->i64[3] = src[0].i64[3] % src[1].i64[3]; + dst->i64[0] = src[1].i64[0] ? src[0].i64[0] % src[1].i64[0] : INT64_MAX; + dst->i64[1] = src[1].i64[1] ? src[0].i64[1] % src[1].i64[1] : INT64_MAX; + dst->i64[2] = src[1].i64[2] ? src[0].i64[2] % src[1].i64[2] : INT64_MAX; + dst->i64[3] = src[1].i64[3] ? src[0].i64[3] % src[1].i64[3] : INT64_MAX; } static void @@ -4653,10 +4653,10 @@ micro_mod(union tgsi_exec_channel *dst, const union tgsi_exec_channel *src0, const union tgsi_exec_channel *src1) { - dst->i[0] = src0->i[0] % src1->i[0]; - dst->i[1] = src0->i[1] % src1->i[1]; - dst->i[2] = src0->i[2] % src1->i[2]; - dst->i[3] = src0->i[3] % src1->i[3]; + dst->i[0] = src1->i[0] ? src0->i[0] % src1->i[0] : INT_MAX; + dst->i[1] = src1->i[1] ? src0->i[1] % src1->i[1] : INT_MAX; + dst->i[2] = src1->i[2] ? src0->i[2] % src1->i[2] : INT_MAX; + dst->i[3] = src1->i[3] ? src0->i[3] % src1->i[3] : INT_MAX; } static void -- 2.9.2.windows.1 ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] gallium: fixed modulo zero crashes in tgsi interpreter
I can fix the remaining integer div/mod opcodes, no problem. I think a consistent error value would be beneficial, would opt for ~0u for 32-bit values and ~0ull for 64 bit values to keep it consistent with the only existing requirement (that is d3d10). Should I just submit another patch based on my original one with the fixed values? I am unsure about the correct procedure, still new to this whole mailing list procedure. As far as src/gallium/docs/source/tgsi.rst is concerned, the docs don't seem to mention any error value anyway. Marius Am 08.06.2017 um 22:28 schrieb Roland Scheidegger: The behavior is probably undefined for most of the opcodes (signed 32bit div, all 64bit div/mod), the docs don't state anything. But in general, this is all undefined in all apis (opencl, glsl, spir-v), with the only exception being d3d10 - which only has udiv and umod, hence these stating in the gallium docs the required result (~0u). So, I suppose we could let the docs say it's actually undefined, right now gallivm will actually return 0 for idiv but all-ones for idiv64 and so on, it's not really consistent... Roland Am 08.06.2017 um 21:51 schrieb Brian Paul: Marius, As long as you're working on this, would you review src/gallium/docs/source/tgsi.rst to check if all the div/mod instructions document div/mod by zero behavior? Thanks. -Brian On 06/08/2017 11:10 AM, Roland Scheidegger wrote: I don't really know if it makes sense to have different "error values" for signed vs. unsigned modulo 0 - maybe the "all bits set" approach would do too (the gallivm code does this, because it is actually easier). But since it's undefined in any case pretty much everywhere, I suppose any value will do (only the 32bit umod really has a requirement for all bits set due do d3d10 requirements, but d3d has neither signed nor 64bit versions of it). And I don't know why it would only crash in geometry shaders... Reviewed-by: Roland Scheidegger Am 08.06.2017 um 18:28 schrieb Marius Gräfe: softpipe throws integer division by zero exceptions on windows when using % with integers in a geometry shader. --- src/gallium/auxiliary/tgsi/tgsi_exec.c | 24 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/src/gallium/auxiliary/tgsi/tgsi_exec.c b/src/gallium/auxiliary/tgsi/tgsi_exec.c index c41954c..abd2d16 100644 --- a/src/gallium/auxiliary/tgsi/tgsi_exec.c +++ b/src/gallium/auxiliary/tgsi/tgsi_exec.c @@ -866,20 +866,20 @@ static void micro_u64mod(union tgsi_double_channel *dst, const union tgsi_double_channel *src) { - dst->u64[0] = src[0].u64[0] % src[1].u64[0]; - dst->u64[1] = src[0].u64[1] % src[1].u64[1]; - dst->u64[2] = src[0].u64[2] % src[1].u64[2]; - dst->u64[3] = src[0].u64[3] % src[1].u64[3]; + dst->u64[0] = src[1].u64[0] ? src[0].u64[0] % src[1].u64[0] : UINT64_MAX; + dst->u64[1] = src[1].u64[1] ? src[0].u64[1] % src[1].u64[1] : UINT64_MAX; + dst->u64[2] = src[1].u64[2] ? src[0].u64[2] % src[1].u64[2] : UINT64_MAX; + dst->u64[3] = src[1].u64[3] ? src[0].u64[3] % src[1].u64[3] : UINT64_MAX; } static void micro_i64mod(union tgsi_double_channel *dst, const union tgsi_double_channel *src) { - dst->i64[0] = src[0].i64[0] % src[1].i64[0]; - dst->i64[1] = src[0].i64[1] % src[1].i64[1]; - dst->i64[2] = src[0].i64[2] % src[1].i64[2]; - dst->i64[3] = src[0].i64[3] % src[1].i64[3]; + dst->i64[0] = src[1].i64[0] ? src[0].i64[0] % src[1].i64[0] : INT64_MAX; + dst->i64[1] = src[1].i64[1] ? src[0].i64[1] % src[1].i64[1] : INT64_MAX; + dst->i64[2] = src[1].i64[2] ? src[0].i64[2] % src[1].i64[2] : INT64_MAX; + dst->i64[3] = src[1].i64[3] ? src[0].i64[3] % src[1].i64[3] : INT64_MAX; } static void @@ -4653,10 +4653,10 @@ micro_mod(union tgsi_exec_channel *dst, const union tgsi_exec_channel *src0, const union tgsi_exec_channel *src1) { - dst->i[0] = src0->i[0] % src1->i[0]; - dst->i[1] = src0->i[1] % src1->i[1]; - dst->i[2] = src0->i[2] % src1->i[2]; - dst->i[3] = src0->i[3] % src1->i[3]; + dst->i[0] = src1->i[0] ? src0->i[0] % src1->i[0] : INT_MAX; + dst->i[1] = src1->i[1] ? src0->i[1] % src1->i[1] : INT_MAX; + dst->i[2] = src1->i[2] ? src0->i[2] % src1->i[2] : INT_MAX; + dst->i[3] = src1->i[3] ? src0->i[3] % src1->i[3] : INT_MAX; } static void ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH] gallium: fixed modulo zero crashes in tgsi interpreter (v2)
softpipe throws integer division by zero exceptions on windows when using % with integers in a geometry shader. v2: Made error results consistent with existing div/mod zero handling in tgsi. 64 bit signed integer division by zero returns zero like in micro_idiv, unsigned returns ~0u like in micro_udiv. Modulo operations always set all result bits to one (like in micro_umod). --- src/gallium/auxiliary/tgsi/tgsi_exec.c | 40 +- 1 file changed, 20 insertions(+), 20 deletions(-) diff --git a/src/gallium/auxiliary/tgsi/tgsi_exec.c b/src/gallium/auxiliary/tgsi/tgsi_exec.c index c41954c..97c75e9 100644 --- a/src/gallium/auxiliary/tgsi/tgsi_exec.c +++ b/src/gallium/auxiliary/tgsi/tgsi_exec.c @@ -846,40 +846,40 @@ static void micro_u64div(union tgsi_double_channel *dst, const union tgsi_double_channel *src) { - dst->u64[0] = src[0].u64[0] / src[1].u64[0]; - dst->u64[1] = src[0].u64[1] / src[1].u64[1]; - dst->u64[2] = src[0].u64[2] / src[1].u64[2]; - dst->u64[3] = src[0].u64[3] / src[1].u64[3]; + dst->u64[0] = src[1].u64[0] ? src[0].u64[0] / src[1].u64[0] : ~0ull; + dst->u64[1] = src[1].u64[1] ? src[0].u64[1] / src[1].u64[1] : ~0ull; + dst->u64[2] = src[1].u64[2] ? src[0].u64[2] / src[1].u64[2] : ~0ull; + dst->u64[3] = src[1].u64[3] ? src[0].u64[3] / src[1].u64[3] : ~0ull; } static void micro_i64div(union tgsi_double_channel *dst, const union tgsi_double_channel *src) { - dst->i64[0] = src[0].i64[0] / src[1].i64[0]; - dst->i64[1] = src[0].i64[1] / src[1].i64[1]; - dst->i64[2] = src[0].i64[2] / src[1].i64[2]; - dst->i64[3] = src[0].i64[3] / src[1].i64[3]; + dst->i64[0] = src[1].i64[0] ? src[0].i64[0] / src[1].i64[0] : 0; + dst->i64[1] = src[1].i64[1] ? src[0].i64[1] / src[1].i64[1] : 0; + dst->i64[2] = src[1].i64[2] ? src[0].i64[2] / src[1].i64[2] : 0; + dst->i64[3] = src[1].i64[3] ? src[0].i64[3] / src[1].i64[3] : 0; } static void micro_u64mod(union tgsi_double_channel *dst, const union tgsi_double_channel *src) { - dst->u64[0] = src[0].u64[0] % src[1].u64[0]; - dst->u64[1] = src[0].u64[1] % src[1].u64[1]; - dst->u64[2] = src[0].u64[2] % src[1].u64[2]; - dst->u64[3] = src[0].u64[3] % src[1].u64[3]; + dst->u64[0] = src[1].u64[0] ? src[0].u64[0] % src[1].u64[0] : ~0ull; + dst->u64[1] = src[1].u64[1] ? src[0].u64[1] % src[1].u64[1] : ~0ull; + dst->u64[2] = src[1].u64[2] ? src[0].u64[2] % src[1].u64[2] : ~0ull; + dst->u64[3] = src[1].u64[3] ? src[0].u64[3] % src[1].u64[3] : ~0ull; } static void micro_i64mod(union tgsi_double_channel *dst, const union tgsi_double_channel *src) { - dst->i64[0] = src[0].i64[0] % src[1].i64[0]; - dst->i64[1] = src[0].i64[1] % src[1].i64[1]; - dst->i64[2] = src[0].i64[2] % src[1].i64[2]; - dst->i64[3] = src[0].i64[3] % src[1].i64[3]; + dst->i64[0] = src[1].i64[0] ? src[0].i64[0] % src[1].i64[0] : ~0ll; + dst->i64[1] = src[1].i64[1] ? src[0].i64[1] % src[1].i64[1] : ~0ll; + dst->i64[2] = src[1].i64[2] ? src[0].i64[2] % src[1].i64[2] : ~0ll; + dst->i64[3] = src[1].i64[3] ? src[0].i64[3] % src[1].i64[3] : ~0ll; } static void @@ -4653,10 +4653,10 @@ micro_mod(union tgsi_exec_channel *dst, const union tgsi_exec_channel *src0, const union tgsi_exec_channel *src1) { - dst->i[0] = src0->i[0] % src1->i[0]; - dst->i[1] = src0->i[1] % src1->i[1]; - dst->i[2] = src0->i[2] % src1->i[2]; - dst->i[3] = src0->i[3] % src1->i[3]; + dst->i[0] = src1->i[0] ? src0->i[0] % src1->i[0] : ~0; + dst->i[1] = src1->i[1] ? src0->i[1] % src1->i[1] : ~0; + dst->i[2] = src1->i[2] ? src0->i[2] % src1->i[2] : ~0; + dst->i[3] = src1->i[3] ? src0->i[3] % src1->i[3] : ~0; } static void -- 2.9.2.windows.1 ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev