Re: Should we add a compiler warning for large stack frames?

Hi,

On 2024-04-11 16:35:58 -0400, Tom Lane wrote:
> Andres Freund <andres(at)anarazel(dot)de> writes:
> > d8f5acbdb9b made me wonder if we should add a compiler option to warn when
> > stack frames are large. gcc compatible compilers have -Wstack-usage=limit, so
> > that's not hard.
>
> > Huge stack frames are somewhat dangerous, as they can defeat our stack-depth
> > checking logic. There are also some cases where large stack frames defeat
> > stack-protector logic by compilers/libc/os.
>
> Indeed. I recall reading, not long ago, some Linux kernel docs to the
> effect that automatic stack growth is triggered by a reference into
> the page just below what is currently mapped as your stack, and
> therefore allocating a stack frame greater than one page has the
> potential to cause SIGSEGV rather than the desired stack extension.
> (If you feel like digging in the archives, I think this was brought
> up in the last round of lets-add-some-more-check_stack_depth-calls.)

I think it's more than a single page, but I'm not entirely sure either. I
think some compilers inject artificial stack accesses when extending the stack
by a lot, but I don't remember the details.

There certainly was the issue that strict memory overcommit does not reliably
work with larger stack extensions.

Could be worth writing a test program for...

> > I don't really have an opinion about the concrete warning limit to use.
>
> Given the above, I'm tempted to say we should make it 8K.

Hm, why 8k? That's 2x the typical page size, isn't it?

> But I know we have a bunch of places that allocate page images as stack
> space, so maybe that'd be too painful.

8k does generate a fair number of warnings, 111 here. I think it might also
be hard to ensure that inlining doesn't end up creating bigger stack frames.

frame size warnings
4096 155
8192 111
16384 36
32768 14
65536 8

Suggests that starting somewhere around 16-32k might be reasonable?

One issue is of course that configuring a larger blocksize or wal_blocksize
will trigger more warnings. I guess we'd need to set the limit based on
Max(blocksize, wal_blocksize) * 2 or such.

Greetings,

Andres Freund