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

[Daniel P. Berrange]

On Mon, Jan 22, 2018 at 02:28:42PM +0100, Martin Kletzander wrote:
> 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?

As I said, I don't see any real point it in - we might as well just use
the -Wno-frame-larger-than flag.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|