From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id AD9091A1DEF for ; Thu, 27 Oct 2016 00:52:21 -0700 (PDT) Received: from int-mx13.intmail.prod.int.phx2.redhat.com (int-mx13.intmail.prod.int.phx2.redhat.com [10.5.11.26]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id DB6B5804EF; Thu, 27 Oct 2016 07:52:20 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-116-24.phx2.redhat.com [10.3.116.24]) by int-mx13.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id u9R7qF8T026244; Thu, 27 Oct 2016 03:52:16 -0400 To: Gary Lin References: <20161026190504.9888-1-lersek@redhat.com> <20161027044323.e4yvyxtvga3e3u7h@GaryWorkstation> Cc: edk2-devel-01 , Ruiyu Ni , Tim He , Feng Tian , Michael D Kinney , Eric Dong , Cecil Sheng , Ard Biesheuvel , Jordan Justen , Liming Gao , Dandan Bi , Jiaxin Wu , Star Zeng , Daryl McDaniel , Jaben Carsey , Siyuan Fu , Jeff Fan , Chao Zhang , David Wei From: Laszlo Ersek Message-ID: <7ecc4ffa-6021-8364-e6a9-484cfa6e204e@redhat.com> Date: Thu, 27 Oct 2016 09:52:15 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.4.0 MIME-Version: 1.0 In-Reply-To: <20161027044323.e4yvyxtvga3e3u7h@GaryWorkstation> X-Scanned-By: MIMEDefang 2.68 on 10.5.11.26 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.27]); Thu, 27 Oct 2016 07:52:21 +0000 (UTC) Subject: Re: [PATCH 00/47] edk2: centralize and adopt ARRAY_SIZE() X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 27 Oct 2016 07:52:21 -0000 Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit On 10/27/16 06:43, Gary Lin wrote: > On Wed, Oct 26, 2016 at 09:04:17PM +0200, Laszlo Ersek wrote: >> I'm sure most of us have wished at some point that ARRAY_SIZE() >> had existed in a central header file, like it does in many other >> open source projects. This series implements that. >> >> Patches 01-04 round up the modules that #define ARRAY_SIZE() just >> for themselves, and wrap their definitions in #ifndef ARRAY_SIZE. >> >> Patch 05 defines ARRAY_SIZE() in "MdePkg/Include/Base.h". This >> flips the previously modified modules to MdePkg's macro definition. >> >> Patches 06-09 modify the same set of modules as patches 01-04 do; this >> time the (now unused) module-local ARRAY_SIZE() macro definitions are >> eliminated. >> >> The rest of the patches (10-47) migrate the edk2 tree to ARRAY_SIZE(), >> covering the most obvious open-coded ARRAY_SIZE() expressions. These >> expressions were located and converted with the following shell >> pipeline: >> >> git ls-files \ >> | xargs --no-run-if-empty -- \ >> sed --regexp-extended --in-place \ >> --expression='s,sizeof \(([a-zA-Z0-9_]+)\) / sizeof \((\*\1|\1\[0\])\),ARRAY_SIZE (\1),g' \ >> -- > Hi Laszlo, > > I found several missing cases in OvmfPkg: > > Library/PciHostBridgeLib/PciHostBridgeLib.c: > > 421: ASSERT (Descriptor->ResType < > 422: (sizeof (mPciHostBridgeLibAcpiAddressSpaceTypeStr) / > 423: sizeof (mPciHostBridgeLibAcpiAddressSpaceTypeStr[0]) > 424: ) > 425: ); Well, this is a multi-line one; it's no surprise the SED regexp didn't match. I only meant the SED command to convert the most obvious, clear-cut cases. You would be very much welcome to submit patches that manually convert (some of) the remaining instances, after I commit this series :) > > -- > VirtioGpuDxe/Gop.c: > > 207: if (ModeNumber >= sizeof mGopResolutions / sizeof mGopResolutions[0]) { > > > 242: if (ModeNumber >= sizeof mGopResolutions / sizeof mGopResolutions[0]) { Yes, these didn't match because I only use parentheses in the operand of the sizeof operator when it is required by the C language standard. (I.e., for type names.) When writing the SED regexp, I focused on the pattern (with the technically superfluous parens) that pretty much every other developer prefers. > 261: VgpuGop->GopMode.MaxMode = (UINT32)(sizeof mGopResolutions / > 262: sizeof mGopResolutions[0]); > Multi-line :) > -- > Library/LoadLinuxLib/Linux.c: > > 360: if (E820EntryCount >= (sizeof (Bp->e820_map) / sizeof (Bp->e820_map[0]))) { > This didn't match because the parenthesized operand is not a simple identifier, it uses the -> operator inside. > -- > Csm/CsmSupportLib/LegacyRegion.c: > > 133: for (Index = 0; Index < (sizeof(mSectionArray) / sizeof (mSectionArray[0])); Index++) { > > 139: ASSERT (Index < (sizeof(mSectionArray) / sizeof (mSectionArray[0]))); > > 144: for (Index = StartIndex; Index < (sizeof(mSectionArray) / sizeof (mSectionArray[0])); Index++) { > > 180: ASSERT (Index < (sizeof(mSectionArray) / sizeof (mSectionArray[0]))); These didn't match because there is no space character between the first "sizeof" operator and its operand. Converting these would be very valuable, but I didn't want to un-focus my series with manual conversions. Can you please submit a patch for these instances (one per module) after I commit this series? Thank you! Laszlo > > > Cheers, > > Gary Lin > >> >> This pipeline lists all tracked files with "git". The pathnames are then >> composed with "xargs" into maximal length "sed" command lines, and those >> "sed" commands are run. >> >> Here the "sed" stream editor filters and modifies all the listed files >> in place. The extended regular expression replaces the following two >> patterns: >> >> sizeof (ArrayName) / sizeof (*ArrayName) >> sizeof (ArrayName) / sizeof (ArrayName[0]) >> >> with >> >> ARRAY_SIZE (ArrayName) >> >> The regexp uses a backreference in the match, ensuring that the sizeof >> operator in the dividend and the sizeof operator in the divisor both >> refer to the same ArrayName. If there are multiple occurrences on the >> same line, the sed command replaces all of them. >> >> I collected the changes into patches manually, and I reviewed them all. >> >> I test-built all of the modified packages, repeating the command >> >> build -a X64 -t GCC48 -b DEBUG -p FooPkg/FooPkg.dsc >> >> as many times as necessary. Two packages were impossible to build like >> this: Nt32Pkg and Vlv2TbltDevicePkg, so I couldn't build-test them. >> >> I did some light-weight testing with OVMF: boot with Secure Boot >> enabled, S3 suspend/resume with and without SMM. I also booted >> ArmVirtQemu once. >> >> Repository: https://github.com/lersek/edk2.git >> Branch: array_size >> >> Cc: Ard Biesheuvel >> Cc: Cecil Sheng >> Cc: Chao Zhang >> Cc: Dandan Bi >> Cc: Daryl McDaniel >> Cc: David Wei >> Cc: Eric Dong >> Cc: Feng Tian >> Cc: Gary Lin >> Cc: Jaben Carsey >> Cc: Jeff Fan >> Cc: Jiaxin Wu >> Cc: Jordan Justen >> Cc: Liming Gao >> Cc: Michael D Kinney >> Cc: Ruiyu Ni >> Cc: Siyuan Fu >> Cc: Star Zeng >> Cc: Tim He >> >> Thanks >> Laszlo >> >> Laszlo Ersek (47): >> MdeModulePkg/RegularExpressionDxe: guard the definition of ARRAY_SIZE >> NetworkPkg/IpsecConfig: guard the definition of ARRAY_SIZE >> OvmfPkg/PlatformBootManagerLib: guard the definition of ARRAY_SIZE >> OvmfPkg/XenBusDxe: guard the definition of ARRAY_SIZE >> MdePkg/Include/Base.h: introduce the ARRAY_SIZE() function-like macro >> MdeModulePkg/RegularExpressionDxe: remove module-local ARRAY_SIZE >> macro >> NetworkPkg/IpsecConfig: remove module-local ARRAY_SIZE macro >> OvmfPkg/PlatformBootManagerLib: remove module-local ARRAY_SIZE macro >> OvmfPkg/XenBusDxe: remove module-local ARRAY_SIZE macro >> ArmVirtPkg/NorFlashQemuLib: rebase to ARRAY_SIZE() >> DuetPkg/DuetBdsLib: rebase to ARRAY_SIZE() >> EdkCompatibilityPkg/FrameworkHiiOnUefiHiiThunk: rebase to ARRAY_SIZE() >> EdkCompatibilityPkg/Sample/Tools: rebase to ARRAY_SIZE() >> FatPkg/EnhancedFatDxe: rebase to ARRAY_SIZE() >> IntelFrameworkModulePkg/LegacyBootManagerLib: rebase to ARRAY_SIZE() >> IntelFrameworkModulePkg/BdsDxe: rebase to ARRAY_SIZE() >> MdeModulePkg/MemoryProfileInfo: rebase to ARRAY_SIZE() >> MdeModulePkg/PciBusDxe: rebase to ARRAY_SIZE() >> MdeModulePkg/PciHostBridgeDxe: rebase to ARRAY_SIZE() >> MdeModulePkg/UsbBusDxe: rebase to ARRAY_SIZE() >> MdeModulePkg/Core/Dxe: rebase to ARRAY_SIZE() >> MdeModulePkg/Core/PiSmmCore: rebase to ARRAY_SIZE() >> MdeModulePkg/BootMaintenanceManagerUiLib: rebase to ARRAY_SIZE() >> MdeModulePkg/BootManagerUiLib: rebase to ARRAY_SIZE() >> MdeModulePkg/UefiBootManagerLib: rebase to ARRAY_SIZE() >> MdeModulePkg/VarCheckHiiLib: rebase to ARRAY_SIZE() >> MdeModulePkg/Logo: rebase to ARRAY_SIZE() >> MdeModulePkg/BdsDxe: rebase to ARRAY_SIZE() >> MdeModulePkg/DisplayEngineDxe: rebase to ARRAY_SIZE() >> MdeModulePkg/EbcDxe: rebase to ARRAY_SIZE() >> MdeModulePkg/Tcp4Dxe: rebase to ARRAY_SIZE() >> MdeModulePkg/Variable/RuntimeDxe: rebase to ARRAY_SIZE() >> NetworkPkg/IScsiDxe: rebase to ARRAY_SIZE() >> NetworkPkg/TcpDxe: rebase to ARRAY_SIZE() >> Nt32Pkg/WinNtSerialIoDxe: rebase to ARRAY_SIZE() >> OptionRomPkg/AtapiPassThruDxe: rebase to ARRAY_SIZE() >> OvmfPkg/QemuBootOrderLib: rebase to ARRAY_SIZE() >> OvmfPkg/QemuVideoDxe: rebase to ARRAY_SIZE() >> PerformancePkg/Dp_App: rebase to ARRAY_SIZE() >> SecurityPkg/AuthVariableLib: rebase to ARRAY_SIZE() >> ShellPkg/UefiDpLib: rebase to ARRAY_SIZE() >> ShellPkg/UefiShellLevel2CommandsLib: rebase to ARRAY_SIZE() >> Vlv2TbltDevicePkg/FirmwareUpdate: rebase to ARRAY_SIZE() >> Vlv2TbltDevicePkg/PlatformInitPei: rebase to ARRAY_SIZE() >> Vlv2TbltDevicePkg/PlatformPei: rebase to ARRAY_SIZE() >> Vlv2TbltDevicePkg/PlatformSetupDxe: rebase to ARRAY_SIZE() >> Vlv2TbltDevicePkg/SmBiosMiscDxe: rebase to ARRAY_SIZE() >> >> ArmVirtPkg/Library/NorFlashQemuLib/NorFlashQemuLib.c | 2 +- >> DuetPkg/Library/DuetBdsLib/BdsPlatform.c | 2 +- >> EdkCompatibilityPkg/Compatibility/FrameworkHiiOnUefiHiiThunk/OpcodeCreation.c | 2 +- >> EdkCompatibilityPkg/Sample/Tools/Source/EfiRom/EfiRom.c | 4 ++-- >> EdkCompatibilityPkg/Sample/Tools/Source/HiiPack/IfrParse.c | 2 +- >> FatPkg/EnhancedFatDxe/DirectoryManage.c | 4 ++-- >> FatPkg/EnhancedFatDxe/FileName.c | 4 ++-- >> FatPkg/EnhancedFatDxe/Hash.c | 4 ++-- >> IntelFrameworkModulePkg/Library/LegacyBootManagerLib/LegacyBm.c | 2 +- >> IntelFrameworkModulePkg/Universal/BdsDxe/BdsEntry.c | 2 +- >> IntelFrameworkModulePkg/Universal/BdsDxe/BootMaint/BBSsupport.c | 4 ++-- >> IntelFrameworkModulePkg/Universal/BdsDxe/BootMaint/BootMaint.c | 8 ++++---- >> IntelFrameworkModulePkg/Universal/BdsDxe/BootMaint/BootOption.c | 2 +- >> IntelFrameworkModulePkg/Universal/BdsDxe/BootMaint/ConsoleOption.c | 6 +++--- >> IntelFrameworkModulePkg/Universal/BdsDxe/BootMaint/UpdatePage.c | 10 +++++----- >> IntelFrameworkModulePkg/Universal/BdsDxe/BootMaint/Variable.c | 2 +- >> IntelFrameworkModulePkg/Universal/BdsDxe/BootMngr/BootManager.c | 2 +- >> MdeModulePkg/Application/MemoryProfileInfo/MemoryProfileInfo.c | 6 +++--- >> MdeModulePkg/Bus/Pci/PciBusDxe/PciLib.c | 2 +- >> MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c | 2 +- >> MdeModulePkg/Bus/Usb/UsbBusDxe/UsbHub.c | 4 ++-- >> MdeModulePkg/Core/Dxe/Mem/Pool.c | 2 +- >> MdeModulePkg/Core/PiSmmCore/Pool.c | 2 +- >> MdeModulePkg/Core/PiSmmCore/SmramProfileRecord.c | 6 +++--- >> MdeModulePkg/Library/BootMaintenanceManagerUiLib/BootMaintenance.c | 8 ++++---- >> MdeModulePkg/Library/BootMaintenanceManagerUiLib/ConsoleOption.c | 8 ++++---- >> MdeModulePkg/Library/BootMaintenanceManagerUiLib/UpdatePage.c | 12 ++++++------ >> MdeModulePkg/Library/BootMaintenanceManagerUiLib/Variable.c | 2 +- >> MdeModulePkg/Library/BootManagerUiLib/BootManager.c | 6 +++--- >> MdeModulePkg/Library/UefiBootManagerLib/BmBootDescription.c | 2 +- >> MdeModulePkg/Library/UefiBootManagerLib/BmConsole.c | 2 +- >> MdeModulePkg/Library/UefiBootManagerLib/BmHotkey.c | 2 +- >> MdeModulePkg/Library/UefiBootManagerLib/BmLoadOption.c | 4 ++-- >> MdeModulePkg/Library/VarCheckHiiLib/VarCheckHiiGen.c | 4 ++-- >> MdeModulePkg/Library/VarCheckHiiLib/VarCheckHiiLibNullClass.c | 2 +- >> MdeModulePkg/Logo/Logo.c | 2 +- >> MdeModulePkg/Universal/BdsDxe/BdsEntry.c | 2 +- >> MdeModulePkg/Universal/DisplayEngineDxe/FormDisplay.c | 4 ++-- >> MdeModulePkg/Universal/DisplayEngineDxe/InputHandler.c | 6 +++--- >> MdeModulePkg/Universal/EbcDxe/EbcExecute.c | 2 +- >> MdeModulePkg/Universal/Network/Tcp4Dxe/Tcp4Output.c | 2 +- >> MdeModulePkg/Universal/RegularExpressionDxe/RegularExpressionDxe.h | 2 -- >> MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c | 2 +- >> MdePkg/Include/Base.h | 13 +++++++++++++ >> NetworkPkg/Application/IpsecConfig/IpSecConfig.h | 2 -- >> NetworkPkg/IScsiDxe/IScsiConfig.c | 4 ++-- >> NetworkPkg/IScsiDxe/IScsiDriver.c | 2 +- >> NetworkPkg/IScsiDxe/IScsiMisc.c | 6 +++--- >> NetworkPkg/TcpDxe/TcpOutput.c | 2 +- >> Nt32Pkg/WinNtSerialIoDxe/WinNtSerialIo.c | 2 +- >> OptionRomPkg/AtapiPassThruDxe/AtapiPassThru.c | 4 ++-- >> OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.c | 5 ----- >> OvmfPkg/Library/QemuBootOrderLib/QemuBootOrderLib.c | 12 ++++++------ >> OvmfPkg/QemuVideoDxe/Initialize.c | 4 ++-- >> OvmfPkg/XenBusDxe/XenStore.c | 1 - >> PerformancePkg/Dp_App/Dp.c | 2 +- >> PerformancePkg/Dp_App/DpTrace.c | 10 +++++----- >> SecurityPkg/Library/AuthVariableLib/AuthVariableLib.c | 4 ++-- >> ShellPkg/Library/UefiDpLib/DpTrace.c | 10 +++++----- >> ShellPkg/Library/UefiShellLevel2CommandsLib/TimeDate.c | 2 +- >> Vlv2TbltDevicePkg/Application/FirmwareUpdate/FirmwareUpdate.c | 2 +- >> Vlv2TbltDevicePkg/PlatformInitPei/FlashMap.c | 2 +- >> Vlv2TbltDevicePkg/PlatformPei/BootMode.c | 6 +++--- >> Vlv2TbltDevicePkg/PlatformSetupDxe/SetupInfoRecords.c | 2 +- >> Vlv2TbltDevicePkg/SmBiosMiscDxe/MiscOemType0x94Function.c | 2 +- >> 65 files changed, 131 insertions(+), 128 deletions(-) >> >> -- >> 2.9.2 >> >> _______________________________________________ >> edk2-devel mailing list >> edk2-devel@lists.01.org >> https://lists.01.org/mailman/listinfo/edk2-devel >>