From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=134.134.136.126; helo=mga18.intel.com; envelope-from=michael.d.kinney@intel.com; receiver=edk2-devel@lists.01.org Received: from mga18.intel.com (mga18.intel.com [134.134.136.126]) (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 B57E721CAD998 for ; Mon, 6 Aug 2018 10:37:40 -0700 (PDT) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga001.jf.intel.com ([10.7.209.18]) by orsmga106.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 06 Aug 2018 10:37:39 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.51,452,1526367600"; d="scan'208";a="79355400" Received: from orsmsx106.amr.corp.intel.com ([10.22.225.133]) by orsmga001.jf.intel.com with ESMTP; 06 Aug 2018 10:36:59 -0700 Received: from orsmsx113.amr.corp.intel.com ([169.254.9.96]) by ORSMSX106.amr.corp.intel.com ([169.254.1.69]) with mapi id 14.03.0319.002; Mon, 6 Aug 2018 10:36:59 -0700 From: "Kinney, Michael D" To: Laszlo Ersek , "Zhang, Shenglei" , "edk2-devel@lists.01.org" , "Kinney, Michael D" CC: "Dong, Eric" , "Zeng, Star" Thread-Topic: [edk2] [PATCH] MdeModulePkg: Remove dead code in .c and .h files Thread-Index: AQHULVvGVV44lvT+T0yNIpPnwOQFZqSzKK2A///Hj9A= Date: Mon, 6 Aug 2018 17:36:58 +0000 Message-ID: References: <20180806074958.22808-1-shenglei.zhang@intel.com> <9cbf157b-6799-f7fd-16b5-71e121e8dc53@redhat.com> In-Reply-To: <9cbf157b-6799-f7fd-16b5-71e121e8dc53@redhat.com> Accept-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: dlp-product: dlpe-windows dlp-version: 11.0.400.15 dlp-reaction: no-action x-originating-ip: [10.22.254.139] MIME-Version: 1.0 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 17:37:40 -0000 Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable Laszlo, I agree for a patch like this that one patch per module makes more sense. We also need to make sure that this series is tested with all supported compilers and CPU architectures to make sure code is not being=20 removed that is required for a combination that was missed. Thanks, Mike > -----Original Message----- > From: edk2-devel [mailto:edk2-devel- > bounces@lists.01.org] On Behalf Of Laszlo Ersek > Sent: Monday, August 6, 2018 6:12 AM > To: Zhang, Shenglei ; edk2- > devel@lists.01.org > Cc: Dong, Eric ; Zeng, Star > > Subject: Re: [edk2] [PATCH] MdeModulePkg: Remove dead > code in .c and .h files >=20 > 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=3D1062 > > > > 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(-) >=20 > Please split this patch up to a series so that *at the > least* each > module is covered by a separate patch. For example, >=20 > 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 >=20 > 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. >=20 > The bug report > > states, >=20 > 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. >=20 > Because the module number is big, I report them all > together by > package in this tracker and don't by modules > separately. >=20 > 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. >=20 > 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. >=20 > 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". >=20 > 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? >=20 > Thanks > Laszlo > _______________________________________________ > edk2-devel mailing list > edk2-devel@lists.01.org > https://lists.01.org/mailman/listinfo/edk2-devel