Eliminating nested functions

Eliminating nested functions

[Pavel Roskin]

Hello!

While I have just applied a patch adding NESTED_FUNC_ATTR to several
functions as an emergency fix for a major breakage in ata, ohci, uhci
and lspci modules, I would prefer a more radical solution.

I suggest that we eliminate all nested functions.  The reasons are:

1) They make the code less readable, as they make the parent functions
longer.

2) They have problems with some popular compilers, as recent as gcc-4.0
when regparm(3) is used.

3) We failed to implement a reliable test for such problems.  We are
using regparm(1) for all compilers.

4) The existing test is one of the obstacles making it impossible to
compile without having libc for the target (x86_64->i386 would be really
nice), as we need to run the compiled test executable.

5) Non-i386 architectures define NESTED_FUNC_ATTR as an empty symbol, so
developers on such architectures don't see if they use it correctly.

6) NESTED_FUNC_ATTR tends to proliferate to the file scope functions, as
it happened with grub_pci_iterate().  It only takes one caller using a
nested function to force NESTED_FUNC_ATTR on all functions used as an
argument to the same function.

We can give all formerly nested functions better, more descriptive names
starting like other functions in the file and ending with "_iter".

-- 
Regards,
Pavel Roskin

Re: Eliminating nested functions

[Bean]

Hi,

One of the advantage of nested function is to use local variables.
Without it, we would need to pass them as global variable, or add
custom data pointer in many of the iterate function, which would make
the code a lot uglier IMO.

On Fri, Apr 17, 2009 at 11:54 PM, Pavel Roskin <proski@gnu.org> wrote:
> Hello!
>
> While I have just applied a patch adding NESTED_FUNC_ATTR to several
> functions as an emergency fix for a major breakage in ata, ohci, uhci
> and lspci modules, I would prefer a more radical solution.
>
> I suggest that we eliminate all nested functions.  The reasons are:
>
> 1) They make the code less readable, as they make the parent functions
> longer.
>
> 2) They have problems with some popular compilers, as recent as gcc-4.0
> when regparm(3) is used.
>
> 3) We failed to implement a reliable test for such problems.  We are
> using regparm(1) for all compilers.
>
> 4) The existing test is one of the obstacles making it impossible to
> compile without having libc for the target (x86_64->i386 would be really
> nice), as we need to run the compiled test executable.
>
> 5) Non-i386 architectures define NESTED_FUNC_ATTR as an empty symbol, so
> developers on such architectures don't see if they use it correctly.
>
> 6) NESTED_FUNC_ATTR tends to proliferate to the file scope functions, as
> it happened with grub_pci_iterate().  It only takes one caller using a
> nested function to force NESTED_FUNC_ATTR on all functions used as an
> argument to the same function.
>
> We can give all formerly nested functions better, more descriptive names
> starting like other functions in the file and ending with "_iter".
>
> --
> Regards,
> Pavel Roskin
>
>
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> http://lists.gnu.org/mailman/listinfo/grub-devel
>



-- 
Bean

Re: Eliminating nested functions

[Vladimir Serbinenko]

[-- Attachment #1: Type: text/plain, Size: 2298 bytes --]

On Fri, Apr 17, 2009 at 9:33 PM, Bean <bean123ch@gmail.com> wrote:

> Hi,
>
> One of the advantage of nested function is to use local variables.
> Without it, we would need to pass them as global variable, or add
> custom data pointer in many of the iterate function, which would make
> the code a lot uglier IMO.
>
Do you have information in which direction it will influence the size of
kernel? (trampoline vs global vs custom pointer)


>
> On Fri, Apr 17, 2009 at 11:54 PM, Pavel Roskin <proski@gnu.org> wrote:
> > Hello!
> >
> > While I have just applied a patch adding NESTED_FUNC_ATTR to several
> > functions as an emergency fix for a major breakage in ata, ohci, uhci
> > and lspci modules, I would prefer a more radical solution.
> >
> > I suggest that we eliminate all nested functions.  The reasons are:
> >
> > 1) They make the code less readable, as they make the parent functions
> > longer.
> >
> > 2) They have problems with some popular compilers, as recent as gcc-4.0
> > when regparm(3) is used.
> >
> > 3) We failed to implement a reliable test for such problems.  We are
> > using regparm(1) for all compilers.
> >
> > 4) The existing test is one of the obstacles making it impossible to
> > compile without having libc for the target (x86_64->i386 would be really
> > nice), as we need to run the compiled test executable.
> >
> > 5) Non-i386 architectures define NESTED_FUNC_ATTR as an empty symbol, so
> > developers on such architectures don't see if they use it correctly.
> >
> > 6) NESTED_FUNC_ATTR tends to proliferate to the file scope functions, as
> > it happened with grub_pci_iterate().  It only takes one caller using a
> > nested function to force NESTED_FUNC_ATTR on all functions used as an
> > argument to the same function.
> >
> > We can give all formerly nested functions better, more descriptive names
> > starting like other functions in the file and ending with "_iter".
> >
> > --
> > Regards,
> > Pavel Roskin
> >
> >
> > _______________________________________________
> > Grub-devel mailing list
> > Grub-devel@gnu.org
> > http://lists.gnu.org/mailman/listinfo/grub-devel
> >
>
>
>
> --
> Bean
>
>
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> http://lists.gnu.org/mailman/listinfo/grub-devel
>

[-- Attachment #2: Type: text/html, Size: 3475 bytes --]

Re: Eliminating nested functions

[Pavel Roskin]

On Sat, 2009-04-18 at 03:33 +0800, Bean wrote: 
> Hi,
> 
> One of the advantage of nested function is to use local variables.
> Without it, we would need to pass them as global variable, or add
> custom data pointer in many of the iterate function, which would make
> the code a lot uglier IMO.

Indeed, I tried to get rid on nested functions in fs/ext2.c, and it
requires more changes than I expected.

However, my impression is that everything can be written nicely with
more effort.  Some arguments could be put into the structures we need to
pass, so the number of arguments doesn't increase.

We should probably try to avoid adding new nested functions in the
meantime.

-- 
Regards,
Pavel Roskin

Re: Eliminating nested functions

[Christian Franke]

Pavel Roskin wrote:
> I suggest that we eliminate all nested functions.  The reasons are:
>
> 1) They make the code less readable, as they make the parent functions
> longer.
>
> 2) They have problems with some popular compilers, as recent as gcc-4.0
> when regparm(3) is used.
>
> 3) We failed to implement a reliable test for such problems.  We are
> using regparm(1) for all compilers.
>
> 4) The existing test is one of the obstacles making it impossible to
> compile without having libc for the target (x86_64->i386 would be really
> nice), as we need to run the compiled test executable.
>
> 5) Non-i386 architectures define NESTED_FUNC_ATTR as an empty symbol, so
> developers on such architectures don't see if they use it correctly.
>
> 6) NESTED_FUNC_ATTR tends to proliferate to the file scope functions, as
> it happened with grub_pci_iterate().  It only takes one caller using a
> nested function to force NESTED_FUNC_ATTR on all functions used as an
> argument to the same function.
>
>   

7) The grub utils may not run on some 'security hardened' platform which 
does not allow to enable stack execution.


-- 
Christian Franke

Re: Eliminating nested functions

[David Miller]

From: Pavel Roskin <proski@gnu.org>
Date: Fri, 17 Apr 2009 11:54:57 -0400

> I suggest that we eliminate all nested functions.

I support this completely.

Re: Eliminating nested functions

[Colin D Bennett]

David Miller wrote on Friday 17 April 2009:
> From: Pavel Roskin <proski@gnu.org>
> Date: Fri, 17 Apr 2009 11:54:57 -0400
>
> > I suggest that we eliminate all nested functions.
>
> I support this completely.

Me too.

While I like the idea of nested functions, since they are like closures and 
make a lot of common operations (such as iterating over a collection) a little 
more concise in the source code, you can certainly implement anything without 
nested functions that you can with them.  Probably passing a pointer to a 
local structure is the easiest way to do it in most cases if the iteration 
function needs to access some state, right?

Regards,
Colin

Re: Eliminating nested functions

[David Miller]

From: Colin D Bennett <colin@gibibit.com>
Date: Sat, 18 Apr 2009 08:57:33 -0700

> Probably passing a pointer to a local structure is the easiest way
> to do it in most cases if the iteration function needs to access
> some state, right?

Right.

The biggest surprise for me, easily, when reading the grub2 sources
for the first time was seeing all of this masterbation with nested
functions.

Re: Eliminating nested functions

[Bean]

Hi,

Yeah, I agree with you. The conversion will take some effort, but it
could payoff in the long run. Perhaps we can achieve this in two
steps:

1, Change nested function definition to accept only one parameter. For
function with multiple parameters, place them in a structure and pass
the pointer. This would eliminate NESTED_FUNC_ATTR, as the regparm
issue won't occur in function with only one parameter.

2. Eliminate nested function. This would be easier after step 1. As we
now pass parameters in a structure, we can append extra variables at
the end, and cast it to the required type. Inside the callback
function, we cast it back to use the extra fields.

On Sat, Apr 18, 2009 at 5:38 AM, Pavel Roskin <proski@gnu.org> wrote:
> On Sat, 2009-04-18 at 03:33 +0800, Bean wrote:
>> Hi,
>>
>> One of the advantage of nested function is to use local variables.
>> Without it, we would need to pass them as global variable, or add
>> custom data pointer in many of the iterate function, which would make
>> the code a lot uglier IMO.
>
> Indeed, I tried to get rid on nested functions in fs/ext2.c, and it
> requires more changes than I expected.
>
> However, my impression is that everything can be written nicely with
> more effort.  Some arguments could be put into the structures we need to
> pass, so the number of arguments doesn't increase.
>
> We should probably try to avoid adding new nested functions in the
> meantime.
>
> --
> Regards,
> Pavel Roskin
>
>
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> http://lists.gnu.org/mailman/listinfo/grub-devel
>



-- 
Bean

Re: Eliminating nested functions

[Pavel Roskin]

Quoting Bean <bean123ch@gmail.com>:

> Yeah, I agree with you. The conversion will take some effort, but it
> could payoff in the long run. Perhaps we can achieve this in two
> steps:
>
> 1, Change nested function definition to accept only one parameter. For
> function with multiple parameters, place them in a structure and pass
> the pointer. This would eliminate NESTED_FUNC_ATTR, as the regparm
> issue won't occur in function with only one parameter.
>
> 2. Eliminate nested function. This would be easier after step 1. As we
> now pass parameters in a structure, we can append extra variables at
> the end, and cast it to the required type. Inside the callback
> function, we cast it back to use the extra fields.

That's an excellent plan!  Thank you!

Considering the amount of changes, it may be a good idea to use the  
git mirror with stgit, so that several patches can be made and  
everything is well tested together before applying.

-- 
Regards,
Pavel Roskin

Re: Eliminating nested functions

[Vladimir Serbinenko]

[-- Attachment #1: Type: text/plain, Size: 1555 bytes --]

In my opinion a far better way would be to add an additional void * argument
which is passed unchanged to the hook. Like:

struct local
{
   ...
};

main_func ()
{
struct local locvars;

grub_*_iterate (arg1,arg2,arg3, &locvars);
}

hook (arg1, arg2, arg3, voidparg)
{
struct local *locvars = (struct local *) voidparg;

}

On Sun, Apr 19, 2009 at 4:29 PM, Pavel Roskin <proski@gnu.org> wrote:

> Quoting Bean <bean123ch@gmail.com>:
>
>  Yeah, I agree with you. The conversion will take some effort, but it
>> could payoff in the long run. Perhaps we can achieve this in two
>> steps:
>>
>> 1, Change nested function definition to accept only one parameter. For
>> function with multiple parameters, place them in a structure and pass
>> the pointer. This would eliminate NESTED_FUNC_ATTR, as the regparm
>> issue won't occur in function with only one parameter.
>>
>> 2. Eliminate nested function. This would be easier after step 1. As we
>> now pass parameters in a structure, we can append extra variables at
>> the end, and cast it to the required type. Inside the callback
>> function, we cast it back to use the extra fields.
>>
>
> That's an excellent plan!  Thank you!
>
> Considering the amount of changes, it may be a good idea to use the git
> mirror with stgit, so that several patches can be made and everything is
> well tested together before applying.
>
> --
> Regards,
> Pavel Roskin
>
>
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> http://lists.gnu.org/mailman/listinfo/grub-devel
>

[-- Attachment #2: Type: text/html, Size: 2395 bytes --]

Re: Eliminating nested functions

[Bean]

Hi,

Right, we can wrap the local variables in void* and pass them in a
custom pointer, but I still think it's advantageous to put them in a
structure rather than use multiple parameters:

1, We can still use nested function. Sometimes it may seem tedious to
use external function for simple callback function. IMO, nested
function is not that bad, we just have to use it safely.

2. It's extensible. For example, we may need to add new information
for the hook function (such as adding modification time for dir hook).
With the structure pointer, we just need to add a new field, old code
that doesn' t use this information doesn't need to change. With
parameters, we need to change the definition of all callback function,
and the numerous __attribute__ ((unused)) is not pretty either.

On Sun, Apr 19, 2009 at 10:51 PM, Vladimir Serbinenko <phcoder@gmail.com> wrote:
> In my opinion a far better way would be to add an additional void * argument
> which is passed unchanged to the hook. Like:
>
> struct local
> {
>    ...
> };
>
> main_func ()
> {
> struct local locvars;
>
> grub_*_iterate (arg1,arg2,arg3, &locvars);
> }
>
> hook (arg1, arg2, arg3, voidparg)
> {
> struct local *locvars = (struct local *) voidparg;
>
> }
>
> On Sun, Apr 19, 2009 at 4:29 PM, Pavel Roskin <proski@gnu.org> wrote:
>>
>> Quoting Bean <bean123ch@gmail.com>:
>>
>>> Yeah, I agree with you. The conversion will take some effort, but it
>>> could payoff in the long run. Perhaps we can achieve this in two
>>> steps:
>>>
>>> 1, Change nested function definition to accept only one parameter. For
>>> function with multiple parameters, place them in a structure and pass
>>> the pointer. This would eliminate NESTED_FUNC_ATTR, as the regparm
>>> issue won't occur in function with only one parameter.
>>>
>>> 2. Eliminate nested function. This would be easier after step 1. As we
>>> now pass parameters in a structure, we can append extra variables at
>>> the end, and cast it to the required type. Inside the callback
>>> function, we cast it back to use the extra fields.
>>
>> That's an excellent plan!  Thank you!
>>
>> Considering the amount of changes, it may be a good idea to use the git
>> mirror with stgit, so that several patches can be made and everything is
>> well tested together before applying.
>>
>> --
>> Regards,
>> Pavel Roskin
>>
>>
>> _______________________________________________
>> Grub-devel mailing list
>> Grub-devel@gnu.org
>> http://lists.gnu.org/mailman/listinfo/grub-devel
>
>
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> http://lists.gnu.org/mailman/listinfo/grub-devel
>
>



-- 
Bean