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

["Kinney, Michael D" <michael.d.kinney@intel.com>]

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 
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 <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