From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: None (no SPF record) identity=mailfrom; client-ip=2a00:1450:400c:c09::234; helo=mail-wm0-x234.google.com; envelope-from=pete@akeo.ie; receiver=edk2-devel@lists.01.org Received: from mail-wm0-x234.google.com (mail-wm0-x234.google.com [IPv6:2a00:1450:400c:c09::234]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id A770222571B4E for ; Fri, 16 Mar 2018 09:05:08 -0700 (PDT) Received: by mail-wm0-x234.google.com with SMTP id h21so3992792wmd.1 for ; Fri, 16 Mar 2018 09:11:33 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=akeo-ie.20150623.gappssmtp.com; s=20150623; h=from:subject:to:references:cc:message-id:date:user-agent :mime-version:in-reply-to:content-language:content-transfer-encoding; bh=e20XLa5pm03H+9/pPUndgMoGhSH1RVRgqsQuXg6VJmQ=; b=o7ymqAheM/M+8QOY/61hkPLqTueCEgklCmV17QvEbemli4CsyiAxwHYsMsfW82yRny 0j5+ociBxpWZLWfT5MEhdE4umQ452dVn3pO8WkC6Sf6c+lSa0DtQXUCOpyBb459L1q4m D+MicnO6QUC2Up6zaQA8/2Ov80gVM2QksYrxqqYzZiBAcTtYs75EiDChin3iPwBj2daX rFybY3BP+ZOGgww1Na9x5TIc7IF437N3WzgVR+p30xYz72gizOVW24vS1OedM0vHG3ep 22k4SDQLt06NeWL/I8tdtdGPAemnCmIjBgjeLhmW8uRQ307wvybxcflPfQu5Hl/SFLs5 ldJg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:from:subject:to:references:cc:message-id:date :user-agent:mime-version:in-reply-to:content-language :content-transfer-encoding; bh=e20XLa5pm03H+9/pPUndgMoGhSH1RVRgqsQuXg6VJmQ=; b=ngFzfCGyM0axP56ig7Wp+FILSvo5MIKFM80brWumB6Aqq5XysQRBaRxzaKUjENR3q6 rYlJzPVvbZK2HU08Y6x3/xbw07yL7T5U5PcDo9rHp1/HGeK9kxY3Y+/N1+gSbeaCMtmR N6WbqLRCf6K0VaodOSP1MTeWogVXAGj8+4KrMUkXfkosbYQXYFjdBzhsCEgyzYQHV1H3 n4yVYV6IgAnPSWvwmDRy/omK2D/zl9QkIrYpNYwbUNSuGJu77AzYwbp9eDexmarBiLSm qLqZ5fmb3K67zfc7YTvGKgE3Nt9t78p0pkyktMvDe5cDB91fwglrnH6D16Ej7iJDLKVM qXdg== X-Gm-Message-State: AElRT7G79vlycFy8PoZt6g/juW1lTvDQbiyl6hzxN5xTb2A4VHbqzJk8 +uLpRgzQhhl67UbPqR445t22Fg== X-Google-Smtp-Source: AG47ELusU3FpZU2vU2g9Hg02DU54xRyDsxfv2B+sGeeoLB8ZG+lnHcg/8F5M+27pJYIIsQGqPv9Dlg== X-Received: by 10.80.141.77 with SMTP id t13mr3136404edt.238.1521216691735; Fri, 16 Mar 2018 09:11:31 -0700 (PDT) Received: from [10.0.0.101] ([84.203.73.68]) by smtp.googlemail.com with ESMTPSA id p26sm3554346edm.41.2018.03.16.09.11.30 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Fri, 16 Mar 2018 09:11:31 -0700 (PDT) From: Pete Batard To: "edk2-devel@lists.01.org" , "Gao, Liming" References: <20180223095003.6012-1-pete@akeo.ie> <4A89E2EF3DFEDB4C8BFDE51014F606A14E1E7104@SHSMSX104.ccr.corp.intel.com> <71d6b134-d875-2c24-6687-f3b01d0ff9ea@akeo.ie> <4A89E2EF3DFEDB4C8BFDE51014F606A14E1E7EEB@SHSMSX104.ccr.corp.intel.com> <4A89E2EF3DFEDB4C8BFDE51014F606A14E1E8137@SHSMSX104.ccr.corp.intel.com> Cc: Ard Biesheuvel Message-ID: Date: Fri, 16 Mar 2018 16:11:30 +0000 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:52.0) Gecko/20100101 Thunderbird/52.6.0 MIME-Version: 1.0 In-Reply-To: <4A89E2EF3DFEDB4C8BFDE51014F606A14E1E8137@SHSMSX104.ccr.corp.intel.com> Subject: Re: [PATCH 0/4] Add ARM64 support for VS2017 X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 16 Mar 2018 16:05:09 -0000 Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-GB Content-Transfer-Encoding: 7bit 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 ; Ard Biesheuvel >> 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 ; 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