[Piglit] [PATCH] mingw: use default 2MB stack size instead of 1MB

[Brian Paul]

On 10/12/2017 12:11 PM, Jose Fonseca wrote:
> 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?

I'm just saying that if we found a way to "fix Mesa to be less stack 
hungry" someone could generate a new "evil" Piglit test that'd still 
breaks things.  Any construct which generates a really deep IR tree and 
is evaluated recursively could run out of stack space.

I wasn't thinking about it, but now that you mention malicious shaders, 
we should probably consider the case of the GLSL compiler running in a 
driver loaded by the X server for indirect rendering.  But that's a 
whole other topic.


>
>>> 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.

So, is increasing the stack size for shader_runner alone to 2MB 
acceptable to you?

Otherwise, the question is: can we change the default stack size at 
shader_runner runtime for particular shader tests?  I guess 
setrlimit(RLIMIT_STACK) is an option on Linux.  Not sure about Windows.

-Brian