Re: [edk2-devel] [PATCH] MdePkg/Include: AARCH64: disable outline atomics on GCC 10.2+

["Ard Biesheuvel" <ard.biesheuvel@arm.com>]

On 5/30/20 5:10 PM, Gao, Liming wrote:
> Ard:
>    Lefi requests to catch this change into 202005 stable tag. I also highlight this request in hard feature freeze notice mail https://edk2.groups.io/g/devel/message/60421.
> 
>    If no objection before the middle of next week (2020-06-03), this patch can be merged with the updated comments.
> 

If the intent is to allow things to stabilize a bit with this patch 
applied, before creating the tag, then the patch should be pushed 
earlier, no?

Then, we give it a few days so that we can revert it again if anyone 
finds any issues with it.

Pushing it right before creating the tag does not sound to me like the 
correct way to approach this.



>> -----Original Message-----
>> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Ard Biesheuvel
>> Sent: Saturday, May 30, 2020 12:51 AM
>> To: Gao, Liming <liming.gao@intel.com>; devel@edk2.groups.io; lersek@redhat.com; Leif Lindholm <leif@nuviainc.com>
>> Cc: philmd@redhat.com; mliska@suse.cz; Kinney, Michael D <michael.d.kinney@intel.com>; afish@apple.com
>> Subject: Re: [edk2-devel] [PATCH] MdePkg/Include: AARCH64: disable outline atomics on GCC 10.2+
>>
>> On 5/29/20 4:29 PM, Gao, Liming wrote:
>>> Ard:
>>>
>>>> -----Original Message-----
>>>> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Ard Biesheuvel
>>>> Sent: Friday, May 29, 2020 1:47 PM
>>>> To: devel@edk2.groups.io; Gao, Liming <liming.gao@intel.com>; lersek@redhat.com; Leif Lindholm <leif@nuviainc.com>
>>>> Cc: philmd@redhat.com; mliska@suse.cz
>>>> Subject: Re: [edk2-devel] [PATCH] MdePkg/Include: AARCH64: disable outline 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’t pass
>>>> build edk2-stable202005. So, you think this is a critical issue to catch 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=2723 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 prefer to remove this link from source code.
>>> With this change, Reviewed-by: Liming Gao <liming.gao@intel.com>
>>>
>>
>>
>> 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 check its value after check whether it is defined or not?
>>>>>
>>>>
>>>> Yes __GNUC_MINOR__ is always defined.
>>>>
>>>>
>>>>
>>>>> -----Original Message-----
>>>>> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Laszlo Ersek
>>>>> Sent: 2020年5月29日 4:03
>>>>> To: Leif Lindholm <leif@nuviainc.com>
>>>>> Cc: Ard Biesheuvel <ard.biesheuvel@arm.com>; devel@edk2.groups.io; Gao, Liming <liming.gao@intel.com>; philmd@redhat.com;
>>>> mliska@suse.cz
>>>>> Subject: Re: [edk2-devel] [PATCH] MdePkg/Include: AARCH64: disable outline 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 stable
>>>>>>>>>> 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, early
>>>>>>>>> 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 known
>>>>>>>> to break with current toolchains.
>>>>>>>
>>>>>>> If this breakage affects "current toolchains", then why was
>>>>>>> <https://bugzilla.tianocore.org/show_bug.cgi?id=2723> only reported
>>>>>>> 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 toolchains
>>>>>>>> down, but people using their distro defaults (native or cross).
>>>>>>>
>>>>>>> ... "people using their distro defaults" to *not* build upstream edk2
>>>>>>> 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 Ard's
>>>>>> intrinsics patch hit the list was not a public report. Yes, if this
>>>>>> had affected only in-development/unstable distributions, I agree this
>>>>>> 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 have
>>>>>>>> the intrinsics included in the release.
>>>>>>>
>>>>>>> OK, let's delay the release then, by a few days. I agree the present
>>>>>>> patch may qualify as a bugfix, but the other patch with the assembly
>>>>>>> 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 builds
>>>>>> that would otherwise fail.
>>>>>
>>>>> OK. That's a good argument. From my POV, feel free to merge (both patches).
>>>>>
>>>>> 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 to
>>>>>>> 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 Gary
>>>>>>> in the commit message.
>>>>>>>
>>>>>>> Thanks,
>>>>>>> Laszlo
>>>>>>>
>>>>>>
>>>>>
>>>>>
>>>>>
>>>>>
>>>>>
>>>>>
>>>>>
>>>>
>>>>
>>>>
>>>
>>
>>
>> 
>