On 12/10/17 17:51, Brian Paul wrote:
On 10/12/2017 08:04 AM, Jose Fonseca wrote:
The intent here was not so much to match the piglti MSVC build, but apps
build by MSVC in general.
After all, nothing ever prevented us from setting a huge stack size on
both MinGW and MSVC alike, as both toolchains allow to congifure the
stack size to whatever we want.
The key issue here is that OpenGL driver don't get to pick the apps they
are loaded, and real OpenGL applications will be likely built with MSVC
instead of Mingw, and therefore will likely only have the MSVC default
stack size. And we should err on the side of caution when testing.
Regardless of the compiler used, if we bump the stack size in piglit,
one is just increasing the chance that wasteful stack allocations go
undetected on piglit and blow up on real applications.
Therefore I suggest we continue to keep 1MB default, and try to fix Mesa
to be less stack hungry. If that's not pratical here,
The ir_expression::constant_expression_value() function's local vars are
pretty minimal. The two that stand out:
ir_constant *op[ARRAY_SIZE(this->operands)] = { NULL, };
ir_constant_data data;
are only 32 and 128 bytes, respectively (on a 32-bit build). I'm not
sure what else accounts for the approx 2KB of the activation record.
I don't see an obvious way to fix the problem. Even if we could reduce
per-call stack memory, someone could write an evil shader that adds a
thousand or more terms and we'd overflow the stack again.
I'm not following...
If the application is malicous, it can overflow the stack without OpenGL
driver help. We have to trust the application. After all, the opengl
driver is in the same process.
If the application is a browser, who needs to handle untrusted shaders,
then it's the application responsibility to ensure it has enough stack
to cope with big shaders.
And for the sake of argument, if increasing stack size is the solution
for malicious shaders, where would one stop? If increasing stack size
is not a solution to malicous shaders, then why is it relevant to the
discussion?
then we should
try to bump the stack size of specific piglit tests (like those that
stress the compiler to the extreme, as Cmake allows to set these options
per executable), and only those tests. Or just mark the affected tests
as expected fail/skip.
The op-selection-bool-bvec4-bvec4.frag test is run with
shader_runner.exe. I think most of the large/complex shaders in Piglit
are run with shader_runner so I don't think it makes much difference if
only shader_runner or all piglit executables are build with a 2MB stack.
In any case, I've got a patch which only sets shader_runner's stack size
which I'll post next.
I guess another option is to change the
op-selection-bool-bvec4-bvec4.frag test to be simpler, but that's just
sweeping the root problem under the rug. Someone could always craft a
complex shader which blows out the stack.
That's fine. I'm not worried about uber shaders at all. Malicous or not.
What I'm worried is that once you set the stack to 2MB, some appearently
innocuous code somewhere in Mesa source tree (probably completely
unrelated to GLSL for example) wastefully uses up 1MB of stack, and
piglit does not catch the issue because all tests have 2MB of stack.
And this is not hypothetical: we had several functions in Mesa related
to writing/read/converting pixels to/from textures, eating up wasteful
amounts of stack. And more than once real apps hit stack overflow
because of it. I'd hate to see us increasing the stack sizes of all our
tests, just for the same issues with real applications creeping back in.
The only way I see to avoid this sort of regressions is to continue
using the default stack, and use big stacks only when necessary, IMO.
Jose
_______________________________________________
Piglit mailing list
Piglit@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/piglit
