Re: [PATCH] MdeModulePkg: Remove dead code in .c and .h files

["Gao, Liming" <liming.gao@intel.com>]

Laszlo:
  We manually search the unused functions in the module source files, and also 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 which unused functions are removed in the driver. 

Thanks
Liming
> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Laszlo Ersek
> Sent: Monday, August 6, 2018 9:12 PM
> To: Zhang, Shenglei <shenglei.zhang@intel.com>; edk2-devel@lists.01.org
> Cc: Dong, Eric <eric.dong@intel.com>; Zeng, Star <star.zeng@intel.com>
> Subject: Re: [edk2] [PATCH] MdeModulePkg: Remove dead code in .c and .h files
> 
> 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 <star.zeng@intel.com>
> > Cc: Eric Dong <eric.dong@intel.com>
> > Contributed-under: TianoCore Contribution Agreement 1.1
> > Signed-off-by: shenglei <shenglei.zhang@intel.com>
> > ---
> >  .../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 <https://bugzilla.tianocore.org/show_bug.cgi?id=1062> 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
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel