Re: [PATCH 0/4] Add ARM64 support for VS2017

["Gao, Liming" <liming.gao@intel.com>]

Pete:
  I push your patches into edk2 trunk. Please check them. 

>-----Original Message-----
>From: Pete Batard [mailto:pete@akeo.ie]
>Sent: Saturday, March 17, 2018 12:36 AM
>To: Gao, Liming <liming.gao@intel.com>; edk2-devel@lists.01.org
>Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>Subject: Re: [edk2] [PATCH 0/4] Add ARM64 support for VS2017
>
>Thanks Liming, much appreciated!
>
>I'll send the comment harmonization patch as soon as I see the
>VS2017/ARM64 changes in edk2 mainline.
>
>Regards,
>
>/Pete
>
>On 2018.03.16 16:31, Gao, Liming wrote:
>> Yes. This is a minor issue. So, I think the effort is small. If it is a big task to you,
>you can separate it into another patch.
>>
>> And, I don't expect this minor issue break your patches. I give my Reviewed-
>by: Liming Gao <liming.gao@intel.com>
>>
>> Thanks
>> Liming
>>> -----Original Message-----
>>> From: Pete Batard [mailto:pete@akeo.ie]
>>> Sent: Saturday, March 17, 2018 12:12 AM
>>> To: edk2-devel@lists.01.org; Gao, Liming <liming.gao@intel.com>
>>> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>>> Subject: Re: [edk2] [PATCH 0/4] Add ARM64 support for VS2017
>>>
>>> I understand where you're coming from, but that means I have to recreate
>>> this patch set, and then create a new patch for the .S (because it makes
>>> zero sense to require the same comment style on the .asm and not
>request
>>> a follow through for the .S).
>>>
>>> My time being limited, I'd rather only have to produce one new patch,
>>> that will harmonize the comments for both .S and .asm at the same time.
>>>
>>> The end result will be exactly the same, so I'm going to have to insist
>>> that we split the comment harmonization (which is a very minor issue and
>>> should hardly be seen as a showstopper for the patch series in the first
>>> place) into a subsequent patch.
>>>
>>> Thank you,
>>>
>>> /Pete
>>>
>>> On 2018.03.16 15:56, Gao, Liming wrote:
>>>> Pete:
>>>>     I understand the existing .S file has the inconsistent comment style. I
>also know new added ASM files are converted from .S files.
>>> But, my comment is for this patch that adds new ASM files. I expect new
>added ASM files have the same style. If you check ARM arch
>>> ASM files, you will find they all have the same style.
>>>>
>>>> Thanks
>>>> Liming
>>>>> -----Original Message-----
>>>>> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf
>Of Pete Batard
>>>>> Sent: Friday, March 16, 2018 7:04 PM
>>>>> To: edk2-devel@lists.01.org
>>>>> Cc: Gao, Liming <liming.gao@intel.com>; Ard Biesheuvel
><ard.biesheuvel@linaro.org>
>>>>> Subject: Re: [edk2] [PATCH 0/4] Add ARM64 support for VS2017
>>>>>
>>>>> On 2018.03.16 08:24, Gao, Liming wrote:
>>>>>> Pete:
>>>>>>       .S for GCC assembly, .asm for MSFT assembly. They can have the
>different comment style.
>>>>>
>>>>> Yes, but as I explained, the actual original issue is that our current
>>>>> .S files do *not* have the same comment styles in the first place.
>>>>>
>>>>> If you look at MdePkg/Library/BaseLib/AArch64/SwitchStack.S, you'll
>see
>>>>> that is uses '//' for comments, whereas other .S files, such as
>>>>> MdePkg/Library/BaseLib/AArch64/SetJumpLongJump.S, use '#'.
>>>>>
>>>>> So that is our actual issue here: Regardless of VS2017, the GCC assembly
>>>>> files for AARCH64 we currently have do not use the same comment style.
>>>>>
>>>>> Thus, the only reason why the .asm don't have the same comment style
>in
>>>>> our proposal is because the .S, which we derived the .asm from, don't.
>>>>> This means that either we should fix the .S too, or we shouldn't fix
>>>>> anything at all.
>>>>>
>>>>>>      Here, my comment is to make sure .asm files have the same
>comment style. I don't request to change .S file.
>>>>>
>>>>> And what I am saying is that it makes little sense to harmonize the
>>>>> comment style for the .asm files, if we're not going to do the same for
>>>>> the .S files as well. It just doesn't seem fair in my book to have the
>>>>> VS2017 assembly files held to a higher standard than the GCC ones. So
>>>>> either we need to fix both, or we fix none at all.
>>>>>
>>>>> But as I indicated in my last e-mail, I am planning to send an
>>>>> additional patch that does comment harmonization, for both .S and .asm,
>>>>> *after* this VS2017 series has been applied to mainline. So the change
>>>>> you request will happen. Just not as part of this patch series.
>>>>>
>>>>> And the reason I have insist on splitting these changes is because, if
>>>>> we have to alter the .S files to be consistent, then this comment
>>>>> harmonization request should logically be handled separately from the
>>>>> VS2017 effort.
>>>>>
>>>>> Please let me know if you still think having a future separate patch,
>>>>> that will do .S and .asm comment harmonization, does not make sense.
>>>>>
>>>>> Regards,
>>>>>
>>>>> /Pete
>>>>>
>>>>>>
>>>>>>> -----Original Message-----
>>>>>>> From: Pete Batard [mailto:pete@akeo.ie]
>>>>>>> Sent: Thursday, March 15, 2018 5:28 PM
>>>>>>> To: Gao, Liming <liming.gao@intel.com>; edk2-devel@lists.01.org
>>>>>>> Cc: ard.biesheuvel@linaro.org
>>>>>>> Subject: Re: [PATCH 0/4] Add ARM64 support for VS2017
>>>>>>>
>>>>>>> Hi Liming,
>>>>>>>
>>>>>>> Thanks for reviewing the patches.
>>>>>>>
>>>>>>> On 2018.03.15 06:15, Gao, Liming wrote:
>>>>>>>> Pete:
>>>>>>>>       For new added ASM file in BaseLib, could you use the same
>comment style
>>>>>>>> for them? ASM use ; for the comment. Most of new files uses ; as
>the
>>>>>>>> comment, but switchstack is not.
>>>>>>>
>>>>>>> This is because SwitchStack.asm is simply SwitchStack.S, with the GCC
>>>>>>> assembler specifics removed, and MSVC assembler specifics added.
>>>>>>>
>>>>>>> I did not change the comment style from the original files, so the real
>>>>>>> issue here is that our GCC assembly files for AARCH64 do not use the
>>>>>>> same comment style.
>>>>>>>
>>>>>>> I'm fine with trying to harmonize the comment styles, but seeing as
>this
>>>>>>> needs to be done for both the .S and .asm, I'd rather send a patch to
>do
>>>>>>> that *after* these VS2017 changes have been applied, as I don't
>consider
>>>>>>> this correction to in scope of this patch series (because logically, the
>>>>>>> introduction of VS2017 should not alter any of the .S files, unless we
>>>>>>> reuse them, which we don't).
>>>>>>>
>>>>>>> If you agree to apply this series, I'll make sure to send a non
>>>>>>> VS2017-specific additional patch, that does what you request for both
>>>>>>> the .S and .asm.
>>>>>>>
>>>>>>>> Besides, compared to Arm arch assembly
>>>>>>>> file, I don't find CpuPause.asm. Is it required?
>>>>>>>
>>>>>>> That file doesn't exist for GCC (as you will see there is no
>>>>>>> MdePkg/Library/BaseLib/AArch64/CpuPause.S), so we don't have
>one for
>>>>>>> VS2017 either.
>>>>>>>
>>>>>>> Regards,
>>>>>>>
>>>>>>> /Pete
>>>>>
>>>>> _______________________________________________
>>>>> edk2-devel mailing list
>>>>> edk2-devel@lists.01.org
>>>>> https://lists.01.org/mailman/listinfo/edk2-devel
>>