Re: [edk2-devel] [Patch 12/12] OvmfPkg SecMain: Add build option "-fno-omit-frame-pointer" for CLANG9 X64

["Liming Gao" <liming.gao@intel.com>]

Laszlo:
  Option (a) works. Jordan patch can fix this issue. 
  Option (b) doesn't work. Even if disable optimization, CLANG doesn't generate the code with push rbp & pop rbp.
  
  So, Jordan patch becomes only option. We can discuss this topic again. But, I don't think this is the block issue
  to add new CLANG9 tool chain. I will try to add CLANG9 tool chain patch without this change, and use BZ 2024 to track this issue. 

Thanks
Liming
> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Liming Gao
> Sent: Tuesday, October 8, 2019 11:10 PM
> To: Laszlo Ersek <lersek@redhat.com>; devel@edk2.groups.io; Justen, Jordan L <jordan.l.justen@intel.com>
> Subject: Re: [edk2-devel] [Patch 12/12] OvmfPkg SecMain: Add build option "-fno-omit-frame-pointer" for CLANG9 X64
> 
> Laszlo:
>   I will verify your option (a) and (b). The problem is described in https://bugzilla.tianocore.org/show_bug.cgi?id=2024.
>   I know there are some discussion in Jordan patch for PeiCore change. Before the conclusion for Jordan change is made,
>   I expect to find the alternate option to resolve CLANG9 tool chain issue first.
> 
> Thanks
> Liming
> > -----Original Message-----
> > From: Laszlo Ersek <lersek@redhat.com>
> > Sent: Tuesday, October 1, 2019 5:10 AM
> > To: devel@edk2.groups.io; Gao, Liming <liming.gao@intel.com>; Justen, Jordan L <jordan.l.justen@intel.com>
> > Subject: Re: [edk2-devel] [Patch 12/12] OvmfPkg SecMain: Add build option "-fno-omit-frame-pointer" for CLANG9 X64
> >
> > + Jordan
> >
> > On 09/27/19 09:46, Liming Gao wrote:
> > > BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=2024
> > >
> > > Signed-off-by: Liming Gao <liming.gao@intel.com>
> > > ---
> > >  OvmfPkg/Sec/SecMain.inf | 4 ++++
> > >  1 file changed, 4 insertions(+)
> > >
> > > diff --git a/OvmfPkg/Sec/SecMain.inf b/OvmfPkg/Sec/SecMain.inf
> > > index 63ba4cb555..cd765cac25 100644
> > > --- a/OvmfPkg/Sec/SecMain.inf
> > > +++ b/OvmfPkg/Sec/SecMain.inf
> > > @@ -69,3 +69,7 @@
> > >
> > >  [FeaturePcd]
> > >    gUefiOvmfPkgTokenSpaceGuid.PcdSmmSmramRequire
> > > +
> > > +[BuildOptions]
> > > +  # CLANG9 X64 requires it.
> > > +  GCC:*_CLANG9_X64_CC_FLAGS = -fno-omit-frame-pointer
> > >
> >
> > I disagree with this patch for two reasons.
> >
> > First, the comment "CLANG9 X64 requires it" is quite lacking. It does
> > nothing to explain the issue; it doesn't even provide a pointer in the
> > code comment (only in the code message).
> >
> > Second, Jordan suggested an alternative approach at the end of
> > <https://bugzilla.tianocore.org/show_bug.cgi?id=2024#c3>, which I prefer
> > to the one seen above. Can we please try that with CLANG9 too?
> >
> >
> > ... Oh wait I see there are new comments in BZ#2024.
> >
> > Apparently,
> >
> >   #pragma GCC optimize ("no-omit-frame-pointer")
> >
> > does not work with CLANG9.
> >
> > That's not a problem: we still have two options that are superior to the
> > present patch, and should be tested.
> >
> > (a) Does Jordan's series linked below fix the problem with CLANG9?
> >
> >   [edk2-devel] [PATCH v2 0/6] Fix PEI Core issue during TemporaryRamMigration
> >   http://mid.mail-archive.com/20190410084000.19660-1-jordan.l.justen@intel.com
> >   https://edk2.groups.io/g/devel/message/38785
> >
> > (b) Jordan's full #pragma suggestion was:
> >
> > #ifdef __GNUC__
> > #pragma GCC push_options
> > #pragma GCC optimize ("no-omit-frame-pointer")
> > #else
> > #pragma optimize ("y", off)
> > #endif
> >
> > If the '#pragme GCC' branch doesn't help, can we customize the *other*
> > branch for CLANG9?
> >
> > For example, in patch#4, we rely on defined(__clang__). Therefore, can
> > we try the following:
> >
> > #ifdef __GNUC__
> > #pragma GCC push_options
> > #pragma GCC optimize ("no-omit-frame-pointer")
> > #elif defined(__clang__)
> > #pragma clang optimize off  <----- NOTE THIS <http://clang.llvm.org/docs/LanguageExtensions.html>
> > #else
> > #pragma optimize ("y", off)
> > #endif
> >
> > In summary, the fact that CLANG9 breaks is just a symptom; it shows that
> > the PEI Core issue fixed by Jordan is real. We should go for the real
> > fix (a).
> >
> > Alternatively, use a clang-specific optimization override, as tightly as
> > possible; (b) might work.
> >
> > -*-
> >
> > In fact I think this is the perfect time to fix the PEI Core issue: we
> > now have a feature request that depends on fixing the bug in the PEI
> > Core. We should fix technical debt in edk2 at least *sometimes*. If we
> > are not willing to fix technical debt in edk2 for the sake of new
> > features, we will *never* fix technical debt.
> >
> > (Well, to be technically correct, we also fix technical debt when it
> > turns into security issues. Yay!)
> >
> > Please test this series with the present patch removed, and Jordan's v2
> > series (linked above) applied.
> >
> > Thanks
> > Laszlo
> 
>