From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from foss.arm.com (foss.arm.com [217.140.110.172]) by mx.groups.io with SMTP id smtpd.web10.34115.1591007522136982437 for ; Mon, 01 Jun 2020 03:32:02 -0700 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: arm.com, ip: 217.140.110.172, mailfrom: ard.biesheuvel@arm.com) Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id A975B1FB; Mon, 1 Jun 2020 03:32:01 -0700 (PDT) Received: from [192.168.1.69] (unknown [172.31.20.19]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 46D133F305; Mon, 1 Jun 2020 03:32:00 -0700 (PDT) Subject: Re: [edk2-devel] [PATCH] MdePkg/Include: AARCH64: disable outline atomics on GCC 10.2+ To: "Gao, Liming" , "devel@edk2.groups.io" , "lersek@redhat.com" , Leif Lindholm Cc: "philmd@redhat.com" , "mliska@suse.cz" , "Kinney, Michael D" , "afish@apple.com" References: <20200522101202.15016-1-ard.biesheuvel@arm.com> <36effdcd-91a7-da00-855d-570e64d650b8@redhat.com> <5587f0f0-9f30-62ce-9d07-5eed4ac6daa7@arm.com> <20200526143725.GM1923@vanye> <0f2f90b9-e2fe-5658-e507-299a387e0ce5@redhat.com> <20200528100515.GC1923@vanye> <6d6e3bb8-647c-42c4-5143-f64f5e6ba81b@redhat.com> <51509986-4950-30b9-73af-874bb991e355@arm.com> From: "Ard Biesheuvel" Message-ID: <8ab83dc3-45dd-4645-5531-824c13c8b82d@arm.com> Date: Mon, 1 Jun 2020 12:31:57 +0200 User-Agent: Mozilla/5.0 (X11; Linux aarch64; rv:68.0) Gecko/20100101 Thunderbird/68.8.1 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: quoted-printable On 5/30/20 5:10 PM, Gao, Liming wrote: > Ard: > Lefi requests to catch this change into 202005 stable tag. I also hig= hlight this request in hard feature freeze notice mail https://edk2.groups.= io/g/devel/message/60421. >=20 > If no objection before the middle of next week (2020-06-03), this pat= ch can be merged with the updated comments. >=20 If the intent is to allow things to stabilize a bit with this patch=20 applied, before creating the tag, then the patch should be pushed=20 earlier, no? Then, we give it a few days so that we can revert it again if anyone=20 finds any issues with it. Pushing it right before creating the tag does not sound to me like the=20 correct way to approach this. >> -----Original Message----- >> From: devel@edk2.groups.io On Behalf Of Ard Bies= heuvel >> Sent: Saturday, May 30, 2020 12:51 AM >> To: Gao, Liming ; devel@edk2.groups.io; lersek@re= dhat.com; Leif Lindholm >> Cc: philmd@redhat.com; mliska@suse.cz; Kinney, Michael D ; afish@apple.com >> Subject: Re: [edk2-devel] [PATCH] MdePkg/Include: AARCH64: disable outl= ine atomics on GCC 10.2+ >> >> On 5/29/20 4:29 PM, Gao, Liming wrote: >>> Ard: >>> >>>> -----Original Message----- >>>> From: devel@edk2.groups.io On Behalf Of Ard Bi= esheuvel >>>> Sent: Friday, May 29, 2020 1:47 PM >>>> To: devel@edk2.groups.io; Gao, Liming ; lersek@= redhat.com; Leif Lindholm >>>> Cc: philmd@redhat.com; mliska@suse.cz >>>> Subject: Re: [edk2-devel] [PATCH] MdePkg/Include: AARCH64: disable ou= tline atomics on GCC 10.2+ >>>> >>>> On 5/29/20 5:18 AM, Liming Gao via groups.io wrote: >>>>> Leif: >>>>> I get the point that the linux distribution default GCC version = may be 10 or above. Without this fix, those developers can=E2=80=99t pass >>>> build edk2-stable202005. So, you think this is a critical issue to ca= tch stable tag 202005. >>>>> >>>>> Ard: >>>>> For this patch, I have two minor comments. >>>>> 1) I suggest to remove Link: https://bugzilla.tianocore.org/show_bug= .cgi?id=3D2723 from comments, because this information has >>>> been in the commit message. >>>> >>>> I think it would be helpful to keep it but I won't insist. >>>> >>> >>> I agree this is useful. But, we record it in the commit message. I pre= fer to remove this link from source code. >>> With this change, Reviewed-by: Liming Gao >>> >> >> >> Works for me. >> >> I will send a v2 after the stable tag is released. >> >> >>>>> 2) Can we think __GNUC_MINOR__ is always defined? Do we need to chec= k its value after check whether it is defined or not? >>>>> >>>> >>>> Yes __GNUC_MINOR__ is always defined. >>>> >>>> >>>> >>>>> -----Original Message----- >>>>> From: devel@edk2.groups.io On Behalf Of Laszl= o Ersek >>>>> Sent: 2020=E5=B9=B45=E6=9C=8829=E6=97=A5 4:03 >>>>> To: Leif Lindholm >>>>> Cc: Ard Biesheuvel ; devel@edk2.groups.io; G= ao, Liming ; philmd@redhat.com; >>>> mliska@suse.cz >>>>> Subject: Re: [edk2-devel] [PATCH] MdePkg/Include: AARCH64: disable o= utline atomics on GCC 10.2+ >>>>> >>>>> On 05/28/20 12:05, Leif Lindholm wrote: >>>>>> On Wed, May 27, 2020 at 11:12:23 +0200, Laszlo Ersek wrote: >>>>>>>>>> Oh and I think both this patch and the assembly language >>>>>>>>>> implementation for the atomics should be delayed after the stab= le >>>>>>>>>> tag. gcc-10 is a new toolchain; so even if we don't introduce a >>>>>>>>>> new toolchain tag such as >>>>>>>>>> GCC10 for it, whatever we do in order to make it work, that's >>>>>>>>>> feature enablement in my book. >>>>>>>>> >>>>>>>>> Works for me. By the time the next stable tag comes around, earl= y >>>>>>>>> adopters that are now on GCC 10.1 will likely have moved to 10.2= by >>>>>>>>> that time, and so we may not need the assembly patch at all. >>>>>>>> >>>>>>>> I'm not ecstatic that we'll be releasing the first stable tag kno= wn >>>>>>>> to break with current toolchains. >>>>>>> >>>>>>> If this breakage affects "current toolchains", then why was >>>>>>> only repor= ted >>>>>>> on 2020-May-19, four days into the soft feature freeze? >>>>>> >>>>>> I agree the timing is crap. >>>>>> >>>>>>>> This isn't just affecting random crazies pulling latest toolchain= s >>>>>>>> down, but people using their distro defaults (native or cross). >>>>>>> >>>>>>> ... "people using their distro defaults" to *not* build upstream e= dk2 >>>>>>> until 2020-May-19, apparently. >>>>>> >>>>>> Or distro defaults changing in between. I mean, we could say "Arch = is >>>>>> the same as any other distro's unstable", but I wouldn't want to go >>>>>> down that route - I know people who use it for developing also for >>>>>> qemu and linux. >>>>>> >>>>>> Argh, I also just realised the error report I saw two days after Ar= d's >>>>>> intrinsics patch hit the list was not a public report. Yes, if this >>>>>> had affected only in-development/unstable distributions, I agree th= is >>>>>> isn't something we should try to deal with upstream. >>>>>> >>>>>>>> I don't recall if 10.1 ended up being default in F32, but it was >>>>>>>> definitely included. In Arch, it does appear default. >>>>>>>> >>>>>>>> Debian/Ubuntu are unaffected in their stable releases. >>>>>>>> >>>>>>>> I agree it's a transitional issue, but I would really prefer to h= ave >>>>>>>> the intrinsics included in the release. >>>>>>> >>>>>>> OK, let's delay the release then, by a few days. I agree the prese= nt >>>>>>> patch may qualify as a bugfix, but the other patch with the assemb= ly >>>>>>> language intrinsics doesn't. If it's really that important to have= in >>>>>>> the upcoming stable tag, then it's worth delaying the tag for. I'm >>>>>>> fine delaying the release for it; it wouldn't be without precedent= . >>>>>> >>>>>> I would argue it *is* a bugfix, since it only has an effect on buil= ds >>>>>> that would otherwise fail. >>>>> >>>>> OK. That's a good argument. From my POV, feel free to merge (both pa= tches). >>>>> >>>>> Thanks >>>>> Laszlo >>>>> >>>>>> But I also do think it is important enough to delay the release if = we >>>>>> feel that is necessary. >>>>>> >>>>>> / >>>>>> Leif >>>>>> >>>>>>> Also, I think Ard's assembly language patch needs a Tested-by from >>>>>>> Gary at the least (reporter of TianoCore#2723). Please reach out t= o >>>>>>> him in that thread. >>>>>>> >>>>>>> ... More precisely, please *ping* Gary for a Tested-by in that >>>>>>> thread, because Ard CC'd him from the start, and even credited Gar= y >>>>>>> in the commit message. >>>>>>> >>>>>>> Thanks, >>>>>>> Laszlo >>>>>>> >>>>>> >>>>> >>>>> >>>>> >>>>> >>>>> >>>>> >>>>> >>>> >>>> >>>> >>> >> >> >>=20 >=20