Re: [libvirt] [PATCH] Raise the frame limit for tests

On Mon, Jan 22, 2018 at 12:57:31PM +0000, Daniel P. Berrange wrote: > On Mon, Jan 22, 2018 at 01:54:28PM +0100, Martin Kletzander wrote: > > On Mon, Jan 22, 2018 at 01:47:24PM +0100, Michal Privoznik wrote: > > > On 01/22/2018 01:22 PM, Daniel P. Berrange wrote: > > > > On Mon, Jan 22, 2018 at 12:49:12PM +0100, Martin Kletzander wrote: > > > > > On Thu, Jan 18, 2018 at 10:16:55AM +0100, Ján Tomko wrote: > > > > > > After the latest CPU additions, the build fails with clang: > > > > > > cputest.c:905:1: error: stack frame size of 26136 bytes > > > > > > in function 'mymain' [-Werror,-Wframe-larger-than=] > > > > > > > > > > > > Raise the relaxed limit which is used for tests. > > > > > > --- > > > > > > m4/virt-compile-warnings.m4 | 2 +- > > > > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > > > > > > > Pushed as a build breaker fix > > > > > > > > > > > > diff --git a/m4/virt-compile-warnings.m4 b/m4/virt-compile-warnings.m4 > > > > > > index f18a08a8f..b9c974842 100644 > > > > > > --- a/m4/virt-compile-warnings.m4 > > > > > > +++ b/m4/virt-compile-warnings.m4 > > > > > > @@ -200,7 +200,7 @@ AC_DEFUN([LIBVIRT_COMPILE_WARNINGS],[ > > > > > > # but using 1024 bytes sized buffers (mostly for virStrerror) > > > > > > # stops us from going down further > > > > > > gl_WARN_ADD([-Wframe-larger-than=4096], [STRICT_FRAME_LIMIT_CFLAGS]) > > > > > > - gl_WARN_ADD([-Wframe-larger-than=25600], [RELAXED_FRAME_LIMIT_CFLAGS]) > > > > > > + gl_WARN_ADD([-Wframe-larger-than=32768], [RELAXED_FRAME_LIMIT_CFLAGS]) > > > > > > > > > > > > > > > > Remind me again why don't we do -Wno-frame-larger-than (or something to that > > > > > effect) for tests? Is it just because "We should fix it at some point"? I > > > > > can't really recall the reasoning behind that (and if it is still valid) even > > > > > though I already asked for it. > > > > > > > > I don't think there's a strong reason, given the way we currently write > > > > tests with huge amounts of stack variables. > > > > > > > > For -Wframe-larger-than to be useful, we'd need to move all the big data > > > > blobs to be static, global variables. > > > Or simply use compiler that honours variable lifetime. If a variable is > > > defined only in a block, compiler should be able to just reuse the > > > stack. I mean for the following case: > > > > > > do { > > > int x; > > > } while (0); > > > > > > do { > > > int y; > > > } while (0); > > > > > > I don't see any compelling reason for compiler to reserve two ints on > > > the stack. Or if it does, count it as one when comparing agains > > > -Wframe-larger-than. > > > > > > > We can do that ourselves, even though it's not really great thing to do. Just > > reset the one struct and reuse it. I added it (and future research) as an idea > > to GSoC ideas. Let's see if someone rewrites that. > > Is it really worth the effort though? It is important for the core library > because we have a unimaginable set of code paths that are hard to validate, > so keeping stack use low is key to minimize risk fo stack exhaustion. In the > test suite, however, we have basically 1-3 call frames and stack exhaustion > is a non-issue - the test would merely crash & not have any bad consequences. > There are two points for this. 1) It can drive someone to start contributing to libvirt by starting off easily, and 2) it can then help with assessing ways how we can make the library frame sizes smaller. So if there is no point in this for tests, as you said, we're back to my original question. Why to have this when we just randomly increase it?