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

[Seth Goldberg]

Yay!! Does this change the minimum GCC version needed to build?

 Thanks!!
  -S

On Jan 1, 2013, at 6:42 AM, Colin Watson <address@hidden> 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,
> 
> -- 
> Colin Watson                                       address@hidden
> <unnest-pci-iterate.patch>
> _______________________________________________
> Grub-devel mailing list
> address@hidden
> https://lists.gnu.org/mailman/listinfo/grub-devel