[PATCH] Removing nested functions, part one of lots

[Colin Watson]

(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
Description: Text Data