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 <ler...@redhat.com>; devel@edk2.groups.io; Justen, Jordan L > <jordan.l.jus...@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 <ler...@redhat.com> > > Sent: Tuesday, October 1, 2019 5:10 AM > > To: devel@edk2.groups.io; Gao, Liming <liming....@intel.com>; Justen, > > Jordan L <jordan.l.jus...@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....@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 > > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#48730): https://edk2.groups.io/g/devel/message/48730 Mute This Topic: https://groups.io/mt/34309065/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
