Re: [edk2] [PATCH] MdePkg: refine code for BaseOrderedCollectionRedBlackTreeLib

Subject: Re: [edk2] [PATCH] MdePkg: refine code for BaseOrderedCollectionRedBlackTreeLib

From: "Scott Duplichan" <scott@notabs.org>

To: <edk2-devel@lists.sourceforge.net>, "'Laszlo Ersek'" <lersek@redhat.com>

Date: 2014-08-19 21:20:01

Hello Mike,

I am surprised to hear about a source level debug problem
with static functions. The Microsoft build tools have an
optimization that can give the appearance of code stepping
through the incorrect function. Here is an example:

    static int f1 (void) {return 2;}
    static int f2 (void) {return 2;}
    static int f3 (void) {return 2;}
    int main (int argc, char *argv [])
        {
        if (argc ==1) return f1 ();
        if (argc ==2) return f2 ();
        if (argc ==3) return f3 ();
        return 0;
        }

With certain optimization settings, the code generation
actually replaces function f2() and f3() with f1() so
that f2 and f3 can be eliminated. Yet f1 remains a stand-alone
function that is not inlined:

main:
003B1010 55                   push        ebp  
003B1011 8B EC                mov         ebp,esp  
003B1013 83 7D 08 01          cmp         dword ptr [argc],1  
003B1017 75 07                jne         main+10h (3B1020h)  
003B1019 E8 E2 FF FF FF       call        f1 (3B1000h)  
003B101E EB 1C                jmp         main+2Ch (3B103Ch)  
003B1020 83 7D 08 02          cmp         dword ptr [argc],2  
003B1024 75 07                jne         main+1Dh (3B102Dh)  
003B1026 E8 D5 FF FF FF       call        f1 (3B1000h)  
003B102B EB 0F                jmp         main+2Ch (3B103Ch)  
003B102D 83 7D 08 03          cmp         dword ptr [argc],3  
003B1031 75 07                jne         main+2Ah (3B103Ah)  
003B1033 E8 C8 FF FF FF       call        f1 (3B1000h)  
003B1038 EB 02                jmp         main+2Ch (3B103Ch)  
003B103A 33 C0                xor         eax,eax  
003B103C 5D                   pop         ebp  
003B103D C3                   ret

In this case, even the Microsoft Visual Studio debugger gives
the appearance of stepping through the 'wrong' function. But
the linker /verbose output confirms the debugger is correct:

     Selected symbol:
         _f1 from 1.obj
     Replaced symbol(s):
         _f2 from 1.obj
         _f3 from 1.obj

Improved optimization from use of static functions is
unlikely with the Microsoft tools because link time code
generation (/LTCG) is enabled. But EDK2 does not yet enable
a similar option when gnu build tools are used. In some
cases GCC will duplicate the code of a local function that
is not static. One copy is in the form of a public function
for external calling, and other copies inlined to satisfy
local calls to the function. This results in unused code 
because the public function is never called. The code size
increase can affect boot time because of reading it from
the relatively slow flash chip.

Thanks,
Scott

-----Original Message-----
From: Kinney, Michael D [mailto:michael.d.kinney@intel.com] 
Sent: Monday, August 18, 2014 11:40 PM
To: edk2-devel@lists.sourceforge.net; 'Laszlo Ersek'
Subject: Re: [edk2] [PATCH] MdePkg: refine code for BaseOrderedCollectionRedBlackTreeLib

Scott,

We have seen issues with some source level debuggers when using static functions.  I some cases, it will show the wrong function
name when stepping through a static function.  Unless there is a really good reason, we prefer to not use static to improve
debugability.

Public functions are declared in the library class include file.  I agree that making it obvious that a function in a library
implementation is an internal worker function is very helpful when looking at the source code.

Can you provide an example where use of 'static' improves optimization?

Thanks,

Mike

-----Original Message-----
From: Scott Duplichan [mailto:scott@notabs.org] 
Sent: Monday, August 18, 2014 8:09 PM
To: edk2-devel@lists.sourceforge.net; 'Laszlo Ersek'
Subject: Re: [edk2] [PATCH] MdePkg: refine code for BaseOrderedCollectionRedBlackTreeLib

Dong, Eric [mailto:eric.dong@intel.com] wrote:

]Hi Laszlo,
]
]I do some changes for this library to follow EDKII coding style, please help to ]review it.
]
]Contributed-under: TianoCore Contribution Agreement 1.0
]Signed-off-by: Eric Dong 
]
]Thanks,
]Eric

Hello Eric,

A couple of questions:
1) Why convert static functions to global functions?
2) Where is this explained in an EDKII coding document?

I ask because I consider declaring a local helper
function as static a courtesy to the reader of the code.
It keeps the reader from having to search other files to
know if the function is called elsewhere. Declaring a
private function as static can also help the build tools
optimize in some cases.

Thanks,
Scott





------------------------------------------------------------------------------
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/edk2-devel

------------------------------------------------------------------------------
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/edk2-devel


------------------------------------------------------------------------------
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/edk2-devel