From: "Tim Lewis" <tim.lewis@insyde.com>
To: "'Yao, Jiewen'" <jiewen.yao@intel.com>,
<Marvin.Haeuser@outlook.com>, <edk2-devel@lists.01.org>
Subject: Re: [PATCH] MdePkg/BaseLib: Globally include ChkStk.c
Date: Tue, 8 May 2018 18:27:49 -0700 [thread overview]
Message-ID: <003701d3e734$f31a7770$d94f6650$@insyde.com> (raw)
In-Reply-To: <74D8A39837DF1E4DA445A8C0B3885C503AB983CA@shsmsx102.ccr.corp.intel.com>
I think that this is a fatal error in EDK2. It basically says, "we are out
of stack space." The alternative is: the system hangs in an unexpected way
since the stack overflows into other pages.
Tim
-----Original Message-----
From: edk2-devel <edk2-devel-bounces@lists.01.org> On Behalf Of Yao, Jiewen
Sent: Tuesday, May 8, 2018 5:25 PM
To: Marvin.Haeuser@outlook.com; edk2-devel@lists.01.org
Subject: Re: [edk2] [PATCH] MdePkg/BaseLib: Globally include ChkStk.c
We discussed internally in Intel.
The quick workaround is: use /Gs65536. :-)
At the same time, our recommendation is to revisit the code which triggers
this error. Why this function need such a big stack? And try to reduce the
local stack usage.
What is why we still use /Gs32768 as default, instead of /Gs65536.
Thank you
Yao Jiewen
> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of
> Marvin H?user
> Sent: Tuesday, May 8, 2018 5:21 PM
> To: edk2-devel@lists.01.org
> Cc: Yao, Jiewen <jiewen.yao@intel.com>
> Subject: Re: [edk2] [PATCH] MdePkg/BaseLib: Globally include ChkStk.c
>
> Oh, if you are prefering a good implementation, I will be all for
> that. This was just a 'quick workaround', same as currently done for GCC.
> I'm actually unsure whether a good implementation is possible with a
> flat memory model. It will likely be mere luck whether there is enough
> space free below the stack, except for maybe when it's located very
> high (preferably past the 32-bit space).
> Has there been any prior discussion on this topic? Would be interested
> to follow up if there was.
>
> Thanks,
> Marvin
>
> > -----Original Message-----
> > From: Yao, Jiewen <jiewen.yao@intel.com>
> > Sent: Wednesday, May 9, 2018 2:13 AM
> > To: Marvin.Haeuser@outlook.com; edk2-devel@lists.01.org
> > Cc: Gao, Liming <liming.gao@intel.com>
> > Subject: RE: [PATCH] MdePkg/BaseLib: Globally include ChkStk.c
> >
> > There are some open source implementation:
> > MSVC: https://github.com/ispc/ispc/issues/542
> > GCC:
> > https://android.googlesource.com/toolchain/gcc/+/b094d6c4bf572654a03
> > 1e cc4afe675154c886dc5/gcc-4.4.3/gcc/config/i386/chkstk.asm
> >
> > The compiler generated code assumes the stack is enlarged after we
> > can chkstk.
> >
> > I agree empty function can make build pass.
> > But more important, we need make it work if there is a need to
> > increase the stack.
> > The potential issue is that the later code (after chkstk) assumes
> > the stack is increased, but actually it is not.
> >
> > That is why I ask how this is validated.
> >
> > Thank you
> > Yao Jiewen
> >
> >
> > > -----Original Message-----
> > > From: Marvin Häuser [mailto:Marvin.Haeuser@outlook.com]
> > > Sent: Tuesday, May 8, 2018 4:58 PM
> > > To: edk2-devel@lists.01.org
> > > Cc: Yao, Jiewen <jiewen.yao@intel.com>; Gao, Liming
> > > <liming.gao@intel.com>
> > > Subject: RE: [PATCH] MdePkg/BaseLib: Globally include ChkStk.c
> > >
> > > Hey Yao,
> > >
> > > As far as I am aware, all __chkstk does is access the stack in 4
> > > KB intervals from the current location to the maximum requested
> > > location to eventually hit the Windows Guard Page, which then
> > > triggers the stack
> > increase.
> > > Because I do not know of any equivalent concept in edk2 and the
> > > intrinsic was already disabled for GCC, I supposed it was a good
> > > idea to do so globally. Are the potential issues I am not aware of?
> > >
> > > Thanks,
> > > Marvin.
> > >
> > > > -----Original Message-----
> > > > From: Yao, Jiewen <jiewen.yao@intel.com>
> > > > Sent: Wednesday, May 9, 2018 1:52 AM
> > > > To: Marvin.Haeuser@outlook.com; edk2-devel@lists.01.org
> > > > Cc: Gao, Liming <liming.gao@intel.com>
> > > > Subject: RE: [PATCH] MdePkg/BaseLib: Globally include ChkStk.c
> > > >
> > > > HI Marvin
> > > > Would you mind to share the information on how you test this update?
> > > >
> > > >
> > > > Per my experience, chkstk not only does the check but also does
> > > > the real work to allocate more stack.
> > > >
> > > > /Gs can be used to indicate the size (/Gs[num] control stack
> > > > checking calls)
> > > >
> > > > We use /Gs32768 by default, see tools_def.txt.
> > > >
> > > > Usually, we just increase this value to override the default
> > > > one, if we hit this issue.
> > > >
> > > > Thank you
> > > > Yao Jiewen
> > > >
> > > > > -----Original Message-----
> > > > > From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On
> > > > > Behalf Of Marvin H?user
> > > > > Sent: Tuesday, May 8, 2018 4:37 PM
> > > > > To: edk2-devel@lists.01.org
> > > > > Cc: Gao, Liming <liming.gao@intel.com>
> > > > > Subject: Re: [edk2] [PATCH] MdePkg/BaseLib: Globally include
> > > > > ChkStk.c
> > > > >
> > > > > Hey Liming,
> > > > >
> > > > > According to the MSDN documentation, the call will be inserted
> > > > > for a stack usage past a defined threshold - 4 KB for IA32, 8
> > > > > KB for X64
> > (source:
> > > > > https://msdn.microsoft.com/en-us/library/ms648426(v=vs.85).aspx).
> > > > >
> > > > > Thanks,
> > > > > Marvin
> > > > >
> > > > > > -----Ursprüngliche Nachricht-----
> > > > > > Von: Gao, Liming <liming.gao@intel.com>
> > > > > > Gesendet: Montag, 7. Mai 2018 14:16
> > > > > > An: Marvin.Haeuser@outlook.com; edk2-devel@lists.01.org
> > > > > > Cc: Kinney, Michael D <michael.d.kinney@intel.com>
> > > > > > Betreff: RE: [PATCH] MdePkg/BaseLib: Globally include
> > > > > > ChkStk.c
> > > > > >
> > > > > > Marvin:
> > > > > > In VS compiler, what case will trig this intrinsic function?
> > > > > >
> > > > > > Thanks
> > > > > > Liming
> > > > > > > -----Original Message-----
> > > > > > > From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org]
> > > > > > > On Behalf Of Marvin H?user
> > > > > > > Sent: Saturday, May 5, 2018 10:25 PM
> > > > > > > To: edk2-devel@lists.01.org
> > > > > > > Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Gao,
> > > > > > > Liming <liming.gao@intel.com>
> > > > > > > Subject: [edk2] [PATCH] MdePkg/BaseLib: Globally include
> > > > > > > ChkStk.c
> > > > > > >
> > > > > > > Initially added for GCC build support, this patch includes
> > > > > > > the function for all compilers and all architectures. This
> > > > > > > is done as huge variables on the stack may cause the
> > > > > > > generation of calls to this intrinsic function for
> > > > > > > Microsoft compilers, even for the IA32 architecture, too.
> > > > > > >
> > > > > > > Contributed-under: TianoCore Contribution Agreement 1.1
> > > > > > > Signed-off-by: Marvin Haeuser <Marvin.Haeuser@outlook.com>
> > > > > > > ---
> > > > > > > MdePkg/Library/BaseLib/{ChkStkGcc.c => ChkStk.c} | 48
> > > > > > > ++++++++++------
> > > > > > ----
> > > > > > > MdePkg/Library/BaseLib/BaseLib.inf | 4 +-
> > > > > > > 2 files changed, 26 insertions(+), 26 deletions(-)
> > > > > > >
> > > > > > > diff --git a/MdePkg/Library/BaseLib/ChkStkGcc.c
> > > > > > > b/MdePkg/Library/BaseLib/ChkStk.c similarity index 74%
> > > > > > > rename from MdePkg/Library/BaseLib/ChkStkGcc.c
> > > > > > > rename to MdePkg/Library/BaseLib/ChkStk.c index
> > > > > > > ecba3853b159..59e6d73f9331 100644
> > > > > > > --- a/MdePkg/Library/BaseLib/ChkStkGcc.c
> > > > > > > +++ b/MdePkg/Library/BaseLib/ChkStk.c
> > > > > > > @@ -1,24 +1,24 @@
> > > > > > > -/** @file
> > > > > > > - Provides hack function for passng GCC build.
> > > > > > > -
> > > > > > > - Copyright (c) 2006 - 2008, Intel Corporation. All
> > > > > > > rights reserved.<BR>
> > > > > > > - This program and the accompanying materials
> > > > > > > - are licensed and made available under the terms and
> > > > > > > conditions of the BSD License
> > > > > > > - which accompanies this distribution. The full text of
> > > > > > > the license may be found at
> > > > > > > - http://opensource.org/licenses/bsd-license.php.
> > > > > > > -
> > > > > > > - THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN
> > "AS
> > > > IS"
> > > > > > > BASIS,
> > > > > > > - WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND,
> > EITHER
> > > > > > EXPRESS OR IMPLIED.
> > > > > > > -
> > > > > > > -**/
> > > > > > > -
> > > > > > > -#include "BaseLibInternals.h"
> > > > > > > -
> > > > > > > -/**
> > > > > > > - Hack function for passing GCC build.
> > > > > > > -**/
> > > > > > > -VOID
> > > > > > > -__chkstk()
> > > > > > > -{
> > > > > > > -}
> > > > > > > -
> > > > > > > +/** @file
> > > > > > > + Provides hack function for passing build.
> > > > > > > +
> > > > > > > + Copyright (c) 2006 - 2018, Intel Corporation. All
> > > > > > > + rights reserved.<BR> This program and the accompanying
> > > > > > > + materials are licensed and made available under the
> > > > > > > + terms and conditions of the BSD License which
> > > > > > > + accompanies this distribution. The full text of the
> > > > > > > + license may be found at
> > > > > > > + http://opensource.org/licenses/bsd-
> > > > > > license.php.
> > > > > > > +
> > > > > > > + THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN
> > "AS
> > > > IS"
> > > > > > > + BASIS, WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY
> > KIND,
> > > > > > EITHER EXPRESS OR IMPLIED.
> > > > > > > +
> > > > > > > +**/
> > > > > > > +
> > > > > > > +#include "BaseLibInternals.h"
> > > > > > > +
> > > > > > > +/**
> > > > > > > + Hack function for passing build.
> > > > > > > +**/
> > > > > > > +VOID
> > > > > > > +__chkstk()
> > > > > > > +{
> > > > > > > +}
> > > > > > > +
> > > > > > > diff --git a/MdePkg/Library/BaseLib/BaseLib.inf
> > > > > > > b/MdePkg/Library/BaseLib/BaseLib.inf
> > > > > > > index 5fbbd02a94b6..d23a6db2581a 100644
> > > > > > > --- a/MdePkg/Library/BaseLib/BaseLib.inf
> > > > > > > +++ b/MdePkg/Library/BaseLib/BaseLib.inf
> > > > > > > @@ -1,7 +1,7 @@
> > > > > > > ## @file
> > > > > > > # Base Library implementation.
> > > > > > > #
> > > > > > > -# Copyright (c) 2007 - 2016, Intel Corporation. All
> > > > > > > rights reserved.<BR>
> > > > > > > +# Copyright (c) 2007 - 2018, Intel Corporation. All
> > > > > > > +rights reserved.<BR>
> > > > > > > # Portions copyright (c) 2008 - 2009, Apple Inc. All
> > > > > > > rights reserved.<BR> # Portions copyright (c) 2011 - 2013,
ARM Ltd.
> > > > > > > All rights reserved.<BR> # @@ -64,6 +64,7 @@ [Sources]
> > > > > > > SafeString.c
> > > > > > > String.c
> > > > > > > FilePaths.c
> > > > > > > + ChkStk.c
> > > > > > > BaseLibInternals.h
> > > > > > >
> > > > > > > [Sources.Ia32]
> > > > > > > @@ -781,7 +782,6 @@ [Sources.X64]
> > > > > > > X64/DisableCache.S | GCC
> > > > > > > X64/RdRand.nasm| GCC
> > > > > > > X64/RdRand.S | GCC
> > > > > > > - ChkStkGcc.c | GCC
> > > > > > >
> > > > > > > [Sources.IPF]
> > > > > > > Ipf/AccessGp.s
> > > > > > > --
> > > > > > > 2.17.0.windows.1
> > > > > > >
> > > > > > > _______________________________________________
> > > > > > > edk2-devel mailing list
> > > > > > > edk2-devel@lists.01.org
> > > > > > > https://lists.01.org/mailman/listinfo/edk2-devel
> > > > > _______________________________________________
> > > > > edk2-devel mailing list
> > > > > edk2-devel@lists.01.org
> > > > > https://lists.01.org/mailman/listinfo/edk2-devel
>
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
next prev parent reply other threads:[~2018-05-09 1:28 UTC|newest]
Thread overview: 11+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-05-08 23:36 [PATCH] MdePkg/BaseLib: Globally include ChkStk.c Marvin Häuser
2018-05-08 23:52 ` Yao, Jiewen
2018-05-08 23:57 ` Marvin Häuser
2018-05-09 0:13 ` Yao, Jiewen
2018-05-09 0:21 ` Marvin Häuser
2018-05-09 0:25 ` Yao, Jiewen
2018-05-09 1:27 ` Tim Lewis [this message]
2018-05-09 8:44 ` Marvin Häuser
2018-05-09 14:11 ` Tim Lewis
-- strict thread matches above, loose matches on Subject: below --
2018-05-05 14:24 Marvin Häuser
2018-05-07 12:16 ` Gao, Liming
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-list from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='003701d3e734$f31a7770$d94f6650$@insyde.com' \
--to=devel@edk2.groups.io \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox