Re: [PATCH] Removing nested functions, part one of lots

[Vladimir 'φ-coder/phcoder' Serbinenko]

Looks like nobody objects and I'm fine with this patch. Go ahead.

On 01.01.2013 15:42, Colin Watson wrote:

> (Part zero was a patch I already committed that dealt with some trivial
> cases.)
> 
> As I mentioned on #grub, and following up on
> https://lists.gnu.org/archive/html/grub-devel/2009-04/msg00406.html and
> thread, I'm working on a patch set to eliminate nested functions from
> GRUB.  I'd like to add a couple of extra reasons to those given by Pavel
> nearly four years ago:
> 
>  * The trampolines used when taking the address of a nested function are
>    a net cost in terms of code size.  With my full set of patches so
>    far, the i386-pc (compressed) kernel gets 52 bytes smaller and a
>    sample core image profile (biosdisk ext2 part_msdos lvm mdraid1x)
>    gets 39 bytes smaller.  That's admittedly not lots, but there are
>    some situations on i386-pc that are very close to the wire and where
>    every byte in the core image counts.
> 
>  * Even several years on, we have failed to solve all the build system
>    problems associated with nested functions.  See Savannah bugs #36669
>    and #37938.
> 
>  * It is revealing that if you search the web for "gcc nested functions"
>    or similar, you find several GCC bug reports from GRUB developers.
>    We appear to be in a small minority of free programs making use of
>    this facility, and I for one would rather concentrate on producing a
>    high-quality boot loader rather than fighting arcane compiler
>    features.
> 
> The vast majority of the nested functions we use - or at any rate the
> particularly problematic ones that involve trampolines - are iterator
> hook functions.  In the general case, un-nesting these involves passing
> a context structure as an additional argument to the hook function.  I
> have adopted a mostly formulaic approach to this, exemplified by the
> attached patch which converts grub_pci_iterate to the new scheme.  My
> approach, which I'll spell out for the sake of those maintaining
> patches, is as follows:
> 
>  * Whenever a function (usually *_iterate) takes a hook function pointer
>    "foo" as a parameter, add an additional "void *foo_data" parameter
>    immediately after it.
> 
>  * If there is not already a typedef for the hook function type, add
>    one.
> 
>  * Update all implementations of the hook type in question to take an
>    additional "void *data" parameter.
> 
>  * Move each hook that was previously a nested function to the top level
>    of its file, and mark it static.
> 
>  * If a hook requires no local variables from its parent function, mark
>    the data parameter __attribute__ ((unused)) and pass NULL when
>    calling it (either directly or via the relevant *_iterate function).
> 
>  * If a hook only requires a single local variable from its parent
>    function, pass a reference to that variable as its data parameter,
>    and have the hook declare an pointer variable at the top initialised
>    to data (e.g. "static int *found = data").
> 
>  * If a hook requires more than one local variable from its parent
>    function, declare "struct <name-of-parent>_ctx" with the necessary
>    variables, and convert both the hook and the parent to access the
>    variables in question via that structure.  I made use of GCC's
>    non-constant aggregate automatic initialisers extension when
>    initialising the context structure in the parent, which I found
>    clearest.
> 
>  * If a hook has a reasonably clear name already, leave it alone.
>    However, if it is called something excessively general such as
>    "hook", then rename it either to something describing its purpose or
>    else simply to "<name-of-parent>_iter".
> 
>  * Remove any NESTED_FUNC_ATTR from hook declarations, and from any
>    pre-existing typedef for the hook function type.
> 
> I have a number of patches mostly ready to go, but I'd prefer to make
> sure that this general approach is agreed before preparing and sending
> more than one of them.  I'd like to work one *_iterate function at a
> time (except where multiple iterators are entangled in a stack such that
> we need to change several at once, as is the case in parts of the
> disk/filesystem stacks), as that's roughly the minimum sensible unit and
> it makes it reasonably easy to grep for missing changes.
> 
> Please review.
> 
> Thanks,
> 
> 
> 
> 
> _______________________________________________
> Grub-devel mailing list
> address@hidden
> https://lists.gnu.org/mailman/listinfo/grub-devel



-- 
Regards
Vladimir 'φ-coder/phcoder' Serbinenko

signature.asc
Description: OpenPGP digital signature