From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: redhat.com, ip: 209.132.183.28, mailfrom: lersek@redhat.com) Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) by groups.io with SMTP; Mon, 30 Sep 2019 14:10:00 -0700 Received: from smtp.corp.redhat.com (int-mx03.intmail.prod.int.phx2.redhat.com [10.5.11.13]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id DF1D718C4275; Mon, 30 Sep 2019 21:09:59 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-121-111.rdu2.redhat.com [10.10.121.111]) by smtp.corp.redhat.com (Postfix) with ESMTP id 0FBCB60872; Mon, 30 Sep 2019 21:09:58 +0000 (UTC) Subject: Re: [edk2-devel] [Patch 12/12] OvmfPkg SecMain: Add build option "-fno-omit-frame-pointer" for CLANG9 X64 To: devel@edk2.groups.io, liming.gao@intel.com, "Jordan Justen (Intel address)" References: <1569570395-11240-1-git-send-email-liming.gao@intel.com> <1569570395-11240-13-git-send-email-liming.gao@intel.com> From: "Laszlo Ersek" Message-ID: <115e2a05-08b1-3ff6-018a-d7190a98f9d4@redhat.com> Date: Mon, 30 Sep 2019 23:09:58 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: <1569570395-11240-13-git-send-email-liming.gao@intel.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.13 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.6.2 (mx1.redhat.com [10.5.110.62]); Mon, 30 Sep 2019 21:10:00 +0000 (UTC) Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit + 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 > --- > 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 , 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 #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