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

[Pete Batard <pete@akeo.ie>]

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