From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=192.55.52.88; helo=mga01.intel.com; envelope-from=liming.gao@intel.com; receiver=edk2-devel@lists.01.org Received: from mga01.intel.com (mga01.intel.com [192.55.52.88]) (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 D583E21CAD998 for ; Mon, 6 Aug 2018 07:55:09 -0700 (PDT) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga008.jf.intel.com ([10.7.209.65]) by fmsmga101.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 06 Aug 2018 07:55:09 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.51,452,1526367600"; d="scan'208";a="62930325" Received: from fmsmsx105.amr.corp.intel.com ([10.18.124.203]) by orsmga008.jf.intel.com with ESMTP; 06 Aug 2018 07:54:32 -0700 Received: from FMSMSX109.amr.corp.intel.com (10.18.116.9) by FMSMSX105.amr.corp.intel.com (10.18.124.203) with Microsoft SMTP Server (TLS) id 14.3.319.2; Mon, 6 Aug 2018 07:54:32 -0700 Received: from shsmsx101.ccr.corp.intel.com (10.239.4.153) by fmsmsx109.amr.corp.intel.com (10.18.116.9) with Microsoft SMTP Server (TLS) id 14.3.319.2; Mon, 6 Aug 2018 07:54:32 -0700 Received: from shsmsx104.ccr.corp.intel.com ([169.254.5.81]) by SHSMSX101.ccr.corp.intel.com ([169.254.1.57]) with mapi id 14.03.0319.002; Mon, 6 Aug 2018 22:54:30 +0800 From: "Gao, Liming" To: Laszlo Ersek , "Zhang, Shenglei" , "edk2-devel@lists.01.org" CC: "Dong, Eric" , "Zeng, Star" Thread-Topic: [edk2] [PATCH] MdeModulePkg: Remove dead code in .c and .h files Thread-Index: AQHULVvHxvFrsHt0bEqQbjypqu3nzaSyLTiAgAChPNA= Date: Mon, 6 Aug 2018 14:54:29 +0000 Message-ID: <4A89E2EF3DFEDB4C8BFDE51014F606A14E2C6745@SHSMSX104.ccr.corp.intel.com> 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: x-ctpclassification: CTP_NT x-titus-metadata-40: eyJDYXRlZ29yeUxhYmVscyI6IiIsIk1ldGFkYXRhIjp7Im5zIjoiaHR0cDpcL1wvd3d3LnRpdHVzLmNvbVwvbnNcL0ludGVsMyIsImlkIjoiZDllZDU4YmEtYjBkMS00MWZkLWIzMjAtMWU4MDQ3MDcyOGFjIiwicHJvcHMiOlt7Im4iOiJDVFBDbGFzc2lmaWNhdGlvbiIsInZhbHMiOlt7InZhbHVlIjoiQ1RQX05UIn1dfV19LCJTdWJqZWN0TGFiZWxzIjpbXSwiVE1DVmVyc2lvbiI6IjE3LjEwLjE4MDQuNDkiLCJUcnVzdGVkTGFiZWxIYXNoIjoiU29PWFJKQnB5bFprem5nQTk1MjUyQzVLb0Z6SGg2Uk12VVIramIrZ2F2dHdsWnBwNGtWUE5UeUpKbFNGak1HVCJ9 dlp-product: dlpe-windows dlp-version: 11.0.400.15 dlp-reaction: no-action x-originating-ip: [10.239.127.40] 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 14:55:10 -0000 Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable Laszlo: We manually search the unused functions in the module source files, and a= lso verify the build to make sure the unused functions are real dead code. = I agree to separate this patch as the module level. So, we can document whi= ch unused functions are removed in the driver.=20 Thanks Liming > -----Original Message----- > From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of La= szlo Ersek > Sent: Monday, August 6, 2018 9:12 PM > 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 f= iles >=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 st= ates, >=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