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

[Laszlo Ersek <lersek@redhat.com>]

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