From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 171F621A04813 for ; Wed, 5 Apr 2017 01:03:54 -0700 (PDT) Received: from smtp.corp.redhat.com (int-mx01.intmail.prod.int.phx2.redhat.com [10.5.11.11]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 556D97E9C9; Wed, 5 Apr 2017 08:03:53 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com 556D97E9C9 Authentication-Results: ext-mx02.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx02.extmail.prod.ext.phx2.redhat.com; spf=pass smtp.mailfrom=lersek@redhat.com DKIM-Filter: OpenDKIM Filter v2.11.0 mx1.redhat.com 556D97E9C9 Received: from lacos-laptop-7.usersys.redhat.com (ovpn-116-13.phx2.redhat.com [10.3.116.13]) by smtp.corp.redhat.com (Postfix) with ESMTP id 0C4A6189FE; Wed, 5 Apr 2017 08:03:51 +0000 (UTC) To: "Gao, Liming" , Ard Biesheuvel References: <559D2DF22BC9A3468B4FA1AA547F0EF102551554@shsmsx102.ccr.corp.intel.com> <1eca3726-e92f-7fbb-d61f-f8026c0b4762@redhat.com> <7fe99ab9-78c8-94d5-fa00-94cd342768be@redhat.com> <4A89E2EF3DFEDB4C8BFDE51014F606A14D718512@shsmsx102.ccr.corp.intel.com> Cc: "Song, BinX" , "edk2-devel@lists.01.org" From: Laszlo Ersek Message-ID: Date: Wed, 5 Apr 2017 10:03:51 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.8.0 MIME-Version: 1.0 In-Reply-To: <4A89E2EF3DFEDB4C8BFDE51014F606A14D718512@shsmsx102.ccr.corp.intel.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.11 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.26]); Wed, 05 Apr 2017 08:03:53 +0000 (UTC) Subject: Re: [PATCH] MdeModulePkg: Fix GCC48/GCC49 build error X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.22 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 05 Apr 2017 08:03:54 -0000 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit On 04/05/17 06:52, Gao, Liming wrote: > I agree to move this option to common GCC option. > > And, now GCC5 has this option. That means if the platform pass GCC5 > build, it should not be impacted in GCC48/GCC49 by this change. > Right? I think so, yes. Laszlo > > Thanks > Liming >> -----Original Message----- >> From: Laszlo Ersek [mailto:lersek@redhat.com] >> Sent: Tuesday, April 4, 2017 12:34 AM >> To: Ard Biesheuvel >> Cc: Song, BinX ; edk2-devel@lists.01.org; Gao, Liming >> Subject: Re: [edk2] [PATCH] MdeModulePkg: Fix GCC48/GCC49 build error >> >> On 04/03/17 18:21, Ard Biesheuvel wrote: >>> On 3 April 2017 at 17:16, Laszlo Ersek wrote: >>>> adding Ard >>>> >>>> On 04/01/17 10:38, Song, BinX wrote: >>>>> - Fix GCC48/GCC49 build error >>>>> >>>>> Cc: Liming Gao >>>>> Contributed-under: TianoCore Contribution Agreement 1.0 >>>>> Signed-off-by: Bell Song >>>>> --- >>>>> .../Library/BrotliCustomDecompressLib/BrotliCustomDecompressLib.inf | 3 +++ >>>>> 1 file changed, 3 insertions(+) >>>>> >>>>> diff --git a/MdeModulePkg/Library/BrotliCustomDecompressLib/BrotliCustomDecompressLib.inf >> b/MdeModulePkg/Library/BrotliCustomDecompressLib/BrotliCustomDecompressLib.inf >>>>> index 578f97f..4c9aff5 100644 >>>>> --- a/MdeModulePkg/Library/BrotliCustomDecompressLib/BrotliCustomDecompressLib.inf >>>>> +++ b/MdeModulePkg/Library/BrotliCustomDecompressLib/BrotliCustomDecompressLib.inf >>>>> @@ -54,3 +54,6 @@ >>>>> DebugLib >>>>> BaseMemoryLib >>>>> ExtractGuidedSectionLib >>>>> + >>>>> +[BuildOptions] >>>>> + GCC:*_*_*_CC_FLAGS = -fno-builtin >>>>> >>>> >>>> In "BaseTools/Conf/tools_def.template", we currently have: >>>> >>>> DEFINE GCC_ARM_CC_FLAGS = DEF(GCC_ALL_CC_FLAGS) [...] -fno-builtin [...] >>>> DEFINE GCC_AARCH64_CC_FLAGS = DEF(GCC_ALL_CC_FLAGS) [...] -fno-builtin [...] >>>> >>>> DEFINE GCC5_IA32_CC_FLAGS = DEF(GCC49_IA32_CC_FLAGS) -fno-builtin >>>> DEFINE GCC5_X64_CC_FLAGS = DEF(GCC49_X64_CC_FLAGS) -fno-builtin >>>> >>>> Now, GCC5_IA32_CC_FLAGS goes back to GCC44_ALL_CC_FLAGS, via: >>>> - GCC49_IA32_CC_FLAGS >>>> - GCC48_IA32_CC_FLAGS >>>> - GCC47_IA32_CC_FLAGS >>>> - GCC46_IA32_CC_FLAGS >>>> - GCC45_IA32_CC_FLAGS >>>> - GCC44_IA32_CC_FLAGS >>>> - GCC44_ALL_CC_FLAGS >>>> >>>> (similarly for GCC5_X64_CC_FLAGS.) >>>> >>>> So, instead of this patch for BrotliCustomDecompressLib, how about: >>>> >>>> - moving "-fno-builtin" from >>>> GCC_ARM_CC_FLAGS and >>>> GCC_AARCH64_CC_FLAGS >>>> to >>>> GCC_ALL_CC_FLAGS, and >>>> >>>> - moving "-fno-builtin" from >>>> GCC5_IA32_CC_FLAGS and >>>> GCC5_X64_CC_FLAGS >>>> to >>>> GCC44_ALL_CC_FLAGS? >>>> >>>> Do we have any reason for permitting builtins at all? >>>> >>> >>> Well, one thing I noticed the other day is that GCC does not >>> 'recognize' memcpy() and memset() when -fno-builtin is defined, which >>> means trivial memcpys and memsets will not be inlined. >> >> But memcpy() and memset(), as written, cannot be called in edk2 anyway >> >> - explicitly, because we don't allow that, >> >> - implicitly (via struct assignment or initialization, for example), >> because we forbid that as well -- source code is supposed to use >> CopyMem(), CopyGuid(), and the like. >> >> So, yes, memcpy() and memset() would not be inlined with my suggestion, >> but edk2 code shouldn't exist in the first place that leads to the >> generation of memcpy() and memset() calls. >> >>> I guess that >>> argues for not permitting it at all, but it also means adding it >>> unconditionally may affect how code is currently generated for some >>> platforms. >> >> If said code doesn't conform to the above requirement, then yes, it >> could happen. >> >> Thanks! >> Laszlo >