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

[Pete Batard <pete@akeo.ie>]

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
>