From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=66.187.233.73; helo=mx1.redhat.com; envelope-from=lersek@redhat.com; receiver=edk2-devel@lists.01.org Received: from mx1.redhat.com (mx3-rdu2.redhat.com [66.187.233.73]) (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 B0D7221CAD998 for ; Mon, 6 Aug 2018 06:11:47 -0700 (PDT) Received: from smtp.corp.redhat.com (int-mx05.intmail.prod.int.rdu2.redhat.com [10.11.54.5]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 0030640216E3; Mon, 6 Aug 2018 13:11:47 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-121-82.rdu2.redhat.com [10.10.121.82]) by smtp.corp.redhat.com (Postfix) with ESMTP id 1F84176F2; Mon, 6 Aug 2018 13:11:45 +0000 (UTC) To: shenglei , edk2-devel@lists.01.org Cc: Eric Dong , Star Zeng References: <20180806074958.22808-1-shenglei.zhang@intel.com> From: Laszlo Ersek Message-ID: <9cbf157b-6799-f7fd-16b5-71e121e8dc53@redhat.com> Date: Mon, 6 Aug 2018 15:11:45 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.7.0 MIME-Version: 1.0 In-Reply-To: <20180806074958.22808-1-shenglei.zhang@intel.com> X-Scanned-By: MIMEDefang 2.79 on 10.11.54.5 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.11.55.5]); Mon, 06 Aug 2018 13:11:47 +0000 (UTC) X-Greylist: inspected by milter-greylist-4.5.16 (mx1.redhat.com [10.11.55.5]); Mon, 06 Aug 2018 13:11:47 +0000 (UTC) for IP:'10.11.54.5' DOMAIN:'int-mx05.intmail.prod.int.rdu2.redhat.com' HELO:'smtp.corp.redhat.com' FROM:'lersek@redhat.com' RCPT:'' Subject: Re: [PATCH] MdeModulePkg: Remove dead code in .c and .h files X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.27 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 06 Aug 2018 13:11:48 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit On 08/06/18 09:49, shenglei wrote: > Some dead code has been removed in .c and .h files. > https://bugzilla.tianocore.org/show_bug.cgi?id=1062 > > Cc: Star Zeng > Cc: Eric Dong > Contributed-under: TianoCore Contribution Agreement 1.1 > Signed-off-by: shenglei > --- > .../Application/CapsuleApp/CapsuleDump.c | 31 --- > MdeModulePkg/Application/UiApp/FrontPage.c | 40 --- > MdeModulePkg/Application/UiApp/Ui.h | 30 -- > .../Bus/Ata/AtaAtapiPassThru/AhciMode.c | 104 ------- > .../Bus/Ata/AtaAtapiPassThru/IdeMode.c | 257 ------------------ > .../Bus/Isa/Ps2KeyboardDxe/Ps2KbdCtrller.c | 26 -- > .../Bus/Isa/Ps2KeyboardDxe/Ps2Keyboard.h | 12 - > MdeModulePkg/Bus/Pci/EhciDxe/EhciDebug.c | 27 -- > MdeModulePkg/Bus/Pci/EhciDxe/EhciDebug.h | 11 - > MdeModulePkg/Bus/Pci/EhciDxe/EhciReg.c | 44 --- > .../Bus/Pci/NvmExpressDxe/NvmExpressHci.c | 110 -------- > .../Bus/Pci/PciBusDxe/PciDeviceSupport.c | 80 ------ > .../Bus/Pci/PciBusDxe/PciDeviceSupport.h | 17 -- > MdeModulePkg/Bus/Pci/PciBusDxe/PciIo.c | 41 --- > MdeModulePkg/Bus/Pci/PciBusDxe/PciIo.h | 21 -- > MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdDevice.c | 94 ------- > MdeModulePkg/Bus/Pci/UhciPei/DmaMem.c | 22 -- > MdeModulePkg/Bus/Pci/UhciPei/UhcPeim.c | 125 --------- > MdeModulePkg/Bus/Pci/UhciPei/UhcPeim.h | 78 ------ > MdeModulePkg/Bus/Pci/XhciDxe/XhciReg.c | 66 ----- > MdeModulePkg/Bus/Pci/XhciDxe/XhciReg.h | 28 -- > MdeModulePkg/Bus/Pci/XhciPei/UsbHcMem.c | 24 -- > MdeModulePkg/Bus/Pci/XhciPei/XhcPeim.c | 22 -- > MdeModulePkg/Bus/Pci/XhciPei/XhciReg.h | 14 - > .../Bus/Sd/EmmcBlockIoPei/EmmcHcMem.c | 24 -- > MdeModulePkg/Bus/Sd/SdBlockIoPei/SdHcMem.c | 24 -- > .../Bus/Ufs/UfsBlockIoPei/UfsBlockIoPei.c | 101 ------- > MdeModulePkg/Bus/Ufs/UfsBlockIoPei/UfsHcMem.c | 24 -- > MdeModulePkg/Bus/Ufs/UfsBlockIoPei/UfsHci.c | 180 ------------ > .../Bus/Ufs/UfsPassThruDxe/UfsPassThruHci.c | 49 ---- > MdeModulePkg/Bus/Usb/UsbBotPei/PeiUsbLib.c | 190 ------------- > MdeModulePkg/Bus/Usb/UsbBotPei/PeiUsbLib.h | 98 ------- > MdeModulePkg/Bus/Usb/UsbBusDxe/UsbHub.c | 68 ----- > MdeModulePkg/Bus/Usb/UsbBusDxe/UsbUtility.c | 146 ---------- > MdeModulePkg/Bus/Usb/UsbBusDxe/UsbUtility.h | 114 -------- > MdeModulePkg/Bus/Usb/UsbBusPei/HubPeim.c | 39 --- > MdeModulePkg/Bus/Usb/UsbBusPei/HubPeim.h | 18 -- > MdeModulePkg/Bus/Usb/UsbBusPei/PeiUsbLib.c | 77 ------ > MdeModulePkg/Bus/Usb/UsbBusPei/PeiUsbLib.h | 35 --- > MdeModulePkg/Core/Dxe/DxeMain.h | 13 - > MdeModulePkg/Core/Dxe/DxeMain/DxeMain.c | 23 -- > MdeModulePkg/Core/Dxe/Mem/HeapGuard.c | 78 ------ > MdeModulePkg/Core/PiSmmCore/HeapGuard.c | 166 ----------- > .../Core/PiSmmCore/MemoryAttributesTable.c | 131 --------- > MdeModulePkg/Core/PiSmmCore/Page.c | 121 --------- > .../Universal/Console/TerminalDxe/Terminal.h | 12 - > .../Console/TerminalDxe/TerminalConIn.c | 25 -- > .../Universal/HiiDatabaseDxe/ConfigRouting.c | 47 ---- > .../Universal/Network/IScsiDxe/IScsiProto.c | 31 --- > .../Universal/Network/Ip4Dxe/Ip4Config2Impl.c | 16 -- > .../Universal/Network/Tcp4Dxe/SockImpl.c | 35 --- > .../Universal/Network/Tcp4Dxe/SockInterface.c | 41 --- > .../Universal/Network/Tcp4Dxe/Socket.h | 32 --- > .../Universal/Network/Tcp4Dxe/Tcp4Option.c | 28 -- > .../Universal/Network/Tcp4Dxe/Tcp4Option.h | 15 - > .../Universal/SetupBrowserDxe/IfrParse.c | 33 --- > 56 files changed, 3358 deletions(-) Please split this patch up to a series so that *at the least* each module is covered by a separate patch. For example, 1 MdeModulePkg/Application/CapsuleApp 2 MdeModulePkg/Application/UiApp 3 MdeModulePkg/Bus/Ata/AtaAtapiPassThru 4 MdeModulePkg/Bus/Isa/Ps2KeyboardDxe 5 MdeModulePkg/Bus/Pci/EhciDxe 6 MdeModulePkg/Bus/Pci/NvmExpressDxe 7 MdeModulePkg/Bus/Pci/PciBusDxe 8 MdeModulePkg/Bus/Pci/SdMmcPciHcDxe 9 MdeModulePkg/Bus/Pci/UhciPei 10 MdeModulePkg/Bus/Pci/XhciDxe 11 MdeModulePkg/Bus/Pci/XhciPei 12 MdeModulePkg/Bus/Sd/EmmcBlockIoPei 13 MdeModulePkg/Bus/Sd/SdBlockIoPei 14 MdeModulePkg/Bus/Ufs/UfsBlockIoPei 15 MdeModulePkg/Bus/Ufs/UfsPassThruDxe 16 MdeModulePkg/Bus/Usb/UsbBotPei 17 MdeModulePkg/Bus/Usb/UsbBusDxe 18 MdeModulePkg/Bus/Usb/UsbBusPei 19 MdeModulePkg/Core/Dxe 20 MdeModulePkg/Core/PiSmmCore 21 MdeModulePkg/Universal/Console/TerminalDxe 22 MdeModulePkg/Universal/HiiDatabaseDxe 23 MdeModulePkg/Universal/Network/IScsiDxe 24 MdeModulePkg/Universal/Network/Ip4Dxe 25 MdeModulePkg/Universal/Network/Tcp4Dxe 26 MdeModulePkg/Universal/SetupBrowserDxe Removing 3358 lines, under a blanket "dead code" comment, across 26 drivers, is terribly bad practice, in my opinion. It's unreviewable, and also unbisectable if something goes wrong. The bug report states, Uefi-aware compiler found many modules have dead code and redundant definitions in the MdeModulePkg. Please see the detail info in the attachment log. We need to clean them. Because the module number is big, I report them all together by package in this tracker and don't by modules separately. Please be aware that there might be false positives in the log. The Uefi-aware compiler currently only analyze the C code and has not supported the assemble code yet. So, if a function is only invoked by assemble code, the Uefi-aware compiler might miss it and report it as dead code. Please verify them carefully if the module have assemble code. The bug report itself states that there might be false positives in the log and that at least some cases have to be evaluated carefully. It's exactly that evaluation that should be captured in each commit message. The fact that the module number is big is *exactly* the reason why the patch series should be fine-grained. Many of these modules are consumed by most platforms in existence. If a platform owner would like to take a look at a small subset of the modules being modified here, they now have to dig around in a 3+ KLOC patch, and figure it out for themselves *why* some code is called "dead". Star, Eric, what are your takes on this? Do you agree with my request above, or do you prefer the patch in its current form? Thanks Laszlo