Re: [PATCH] Raise the frame limit for tests

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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?

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

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list

Attachment: signature.asc
Description: Digital signature

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list

[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]
  Powered by Linux