[PATCH] Removing nested functions, part one of lots

[PATCH] Removing nested functions, part one of lots

[Colin Watson]

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

(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                                       [cjwatson@ubuntu.com]

[-- Attachment #2: unnest-pci-iterate.patch --]
[-- Type: text/x-diff, Size: 33983 bytes --]

=== modified file 'grub-core/bus/cs5536.c'
--- grub-core/bus/cs5536.c	2011-12-13 14:13:51 +0000
+++ grub-core/bus/cs5536.c	2013-01-01 14:25:26 +0000
@@ -29,28 +29,39 @@
 
 GRUB_MOD_LICENSE ("GPLv3+");
 
+/* Context for grub_cs5536_find.  */
+struct grub_cs5536_find_ctx
+{
+  grub_pci_device_t *devp;
+  int found;
+};
+
+/* Helper for grub_cs5536_find.  */
+static int
+grub_cs5536_find_iter (grub_pci_device_t dev, grub_pci_id_t pciid, void *data)
+{
+  struct grub_cs5536_find_ctx *ctx = data;
+
+  if (pciid == GRUB_CS5536_PCIID)
+    {
+      *ctx->devp = dev;
+      ctx->found = 1;
+      return 1;
+    }
+  return 0;
+}
+
 int
 grub_cs5536_find (grub_pci_device_t *devp)
 {
-  int found = 0;
-  auto int NESTED_FUNC_ATTR hook (grub_pci_device_t dev,
-				  grub_pci_id_t pciid);
-
-  int NESTED_FUNC_ATTR hook (grub_pci_device_t dev,
-			     grub_pci_id_t pciid)
-  {
-    if (pciid == GRUB_CS5536_PCIID)
-      {
-	*devp = dev;
-	found = 1;
-	return 1;
-      }
-    return 0;
-  }
+  struct grub_cs5536_find_ctx ctx = {
+    .devp = devp,
+    .found = 0
+  };
 
-  grub_pci_iterate (hook);
+  grub_pci_iterate (grub_cs5536_find_iter, &ctx);
 
-  return found;
+  return ctx.found;
 }
 
 grub_uint64_t

=== modified file 'grub-core/bus/emu/pci.c'
--- grub-core/bus/emu/pci.c	2010-05-06 07:25:47 +0000
+++ grub-core/bus/emu/pci.c	2013-01-01 12:50:14 +0000
@@ -32,7 +32,7 @@ grub_pci_make_address (grub_pci_device_t
 }
 
 void
-grub_pci_iterate (grub_pci_iteratefunc_t hook)
+grub_pci_iterate (grub_pci_iteratefunc_t hook, void *hook_data)
 {
   struct pci_device_iterator *iter;
   struct pci_slot_match slot;
@@ -43,7 +43,7 @@ grub_pci_iterate (grub_pci_iteratefunc_t
   slot.func = PCI_MATCH_ANY;
   iter = pci_slot_match_iterator_create (&slot);
   while ((dev = pci_device_next (iter)))
-    hook (dev, dev->vendor_id | (dev->device_id << 16));
+    hook (dev, dev->vendor_id | (dev->device_id << 16), hook_data);
   pci_iterator_destroy (iter);
 }
 

=== modified file 'grub-core/bus/pci.c'
--- grub-core/bus/pci.c	2012-05-03 15:26:55 +0000
+++ grub-core/bus/pci.c	2013-01-01 12:50:14 +0000
@@ -98,7 +98,7 @@ grub_pci_make_address (grub_pci_device_t
 }
 
 void
-grub_pci_iterate (grub_pci_iteratefunc_t hook)
+grub_pci_iterate (grub_pci_iteratefunc_t hook, void *hook_data)
 {
   grub_pci_device_t dev;
   grub_pci_address_t addr;
@@ -125,7 +125,7 @@ grub_pci_iterate (grub_pci_iteratefunc_t
 		    continue;
 		}
 
-	      if (hook (dev, id))
+	      if (hook (dev, id, hook_data))
 		return;
 
 	      /* Probe only func = 0 if the device if not multifunction */

=== modified file 'grub-core/bus/usb/ehci.c'
--- grub-core/bus/usb/ehci.c	2012-12-30 09:57:58 +0000
+++ grub-core/bus/usb/ehci.c	2013-01-01 12:50:14 +0000
@@ -454,8 +454,9 @@ grub_ehci_reset (struct grub_ehci *e)
 }
 
 /* PCI iteration function... */
-static int NESTED_FUNC_ATTR
-grub_ehci_pci_iter (grub_pci_device_t dev, grub_pci_id_t pciid)
+static int
+grub_ehci_pci_iter (grub_pci_device_t dev, grub_pci_id_t pciid,
+		    void *data __attribute__ ((unused)))
 {
   grub_uint8_t release;
   grub_uint32_t class_code;
@@ -1814,7 +1815,7 @@ grub_ehci_detect_dev (grub_usb_controlle
 static void
 grub_ehci_inithw (void)
 {
-  grub_pci_iterate (grub_ehci_pci_iter);
+  grub_pci_iterate (grub_ehci_pci_iter, NULL);
 }
 
 static grub_err_t

=== modified file 'grub-core/bus/usb/ohci.c'
--- grub-core/bus/usb/ohci.c	2012-07-22 19:09:30 +0000
+++ grub-core/bus/usb/ohci.c	2013-01-01 12:50:14 +0000
@@ -213,9 +213,9 @@ grub_ohci_writereg32 (struct grub_ohci *
 
 /* Iterate over all PCI devices.  Determine if a device is an OHCI
    controller.  If this is the case, initialize it.  */
-static int NESTED_FUNC_ATTR
-grub_ohci_pci_iter (grub_pci_device_t dev,
-		    grub_pci_id_t pciid)
+static int
+grub_ohci_pci_iter (grub_pci_device_t dev, grub_pci_id_t pciid,
+		    void *data __attribute__ ((unused)))
 {
   grub_uint32_t interf;
   grub_uint32_t base;
@@ -477,7 +477,7 @@ grub_ohci_pci_iter (grub_pci_device_t de
 static void
 grub_ohci_inithw (void)
 {
-  grub_pci_iterate (grub_ohci_pci_iter);
+  grub_pci_iterate (grub_ohci_pci_iter, NULL);
 }
 
 \f

=== modified file 'grub-core/bus/usb/uhci.c'
--- grub-core/bus/usb/uhci.c	2012-02-09 14:00:05 +0000
+++ grub-core/bus/usb/uhci.c	2013-01-01 12:50:14 +0000
@@ -185,9 +185,10 @@ grub_uhci_portstatus (grub_usb_controlle
 
 /* Iterate over all PCI devices.  Determine if a device is an UHCI
    controller.  If this is the case, initialize it.  */
-static int NESTED_FUNC_ATTR
+static int
 grub_uhci_pci_iter (grub_pci_device_t dev,
-		    grub_pci_id_t pciid __attribute__((unused)))
+		    grub_pci_id_t pciid __attribute__((unused)),
+		    void *data __attribute__ ((unused)))
 {
   grub_uint32_t class_code;
   grub_uint32_t class;
@@ -351,7 +352,7 @@ grub_uhci_pci_iter (grub_pci_device_t de
 static void
 grub_uhci_inithw (void)
 {
-  grub_pci_iterate (grub_uhci_pci_iter);
+  grub_pci_iterate (grub_uhci_pci_iter, NULL);
 }
 
 static grub_uhci_td_t

=== modified file 'grub-core/commands/efi/fixvideo.c'
--- grub-core/commands/efi/fixvideo.c	2012-02-27 13:13:24 +0000
+++ grub-core/commands/efi/fixvideo.c	2013-01-01 13:03:29 +0000
@@ -23,6 +23,7 @@
 #include <grub/pci.h>
 #include <grub/command.h>
 #include <grub/i18n.h>
+#include <grub/mm.h>
 
 GRUB_MOD_LICENSE ("GPLv3+");
 
@@ -40,8 +41,9 @@ static struct grub_video_patch
     {0, 0, 0, 0, 0}
   };
 
-static int NESTED_FUNC_ATTR
-scan_card (grub_pci_device_t dev, grub_pci_id_t pciid)
+static int
+scan_card (grub_pci_device_t dev, grub_pci_id_t pciid,
+	   void *data __attribute__ ((unused)))
 {
   grub_pci_address_t addr;
 
@@ -93,7 +95,7 @@ grub_cmd_fixvideo (grub_command_t cmd __
 		   int argc __attribute__ ((unused)),
 		   char *argv[] __attribute__ ((unused)))
 {
-  grub_pci_iterate (scan_card);
+  grub_pci_iterate (scan_card, NULL);
   return 0;
 }
 

=== modified file 'grub-core/commands/lspci.c'
--- grub-core/commands/lspci.c	2011-11-30 15:20:13 +0000
+++ grub-core/commands/lspci.c	2013-01-01 12:50:14 +0000
@@ -22,6 +22,7 @@
 #include <grub/misc.h>
 #include <grub/extcmd.h>
 #include <grub/i18n.h>
+#include <grub/mm.h>
 
 GRUB_MOD_LICENSE ("GPLv3+");
 
@@ -126,8 +127,9 @@ static const struct grub_arg_option opti
 
 static int iospace;
 
-static int NESTED_FUNC_ATTR
-grub_lspci_iter (grub_pci_device_t dev, grub_pci_id_t pciid)
+static int
+grub_lspci_iter (grub_pci_device_t dev, grub_pci_id_t pciid,
+		 void *data __attribute__ ((unused)))
 {
   grub_uint32_t class;
   const char *sclass;
@@ -218,7 +220,7 @@ grub_cmd_lspci (grub_extcmd_context_t ct
 		char **args __attribute__ ((unused)))
 {
   iospace = ctxt->state[0].set;
-  grub_pci_iterate (grub_lspci_iter);
+  grub_pci_iterate (grub_lspci_iter, NULL);
   return GRUB_ERR_NONE;
 }
 

=== modified file 'grub-core/commands/setpci.c'
--- grub-core/commands/setpci.c	2012-02-08 18:26:01 +0000
+++ grub-core/commands/setpci.c	2013-01-01 12:50:14 +0000
@@ -83,8 +83,9 @@ static int regsize;
 static grub_uint16_t regaddr;
 static const char *varname;
 
-static int NESTED_FUNC_ATTR
-grub_setpci_iter (grub_pci_device_t dev, grub_pci_id_t pciid)
+static int
+grub_setpci_iter (grub_pci_device_t dev, grub_pci_id_t pciid,
+		  void *data __attribute__ ((unused)))
 {
   grub_uint32_t regval = 0;
   grub_pci_address_t addr;
@@ -320,7 +321,7 @@ grub_cmd_setpci (grub_extcmd_context_t c
     return grub_error (GRUB_ERR_BAD_ARGUMENT,
 		       "option -v isn't valid for writes");
 
-  grub_pci_iterate (grub_setpci_iter);
+  grub_pci_iterate (grub_setpci_iter, NULL);
   return GRUB_ERR_NONE;
 }
 

=== modified file 'grub-core/disk/ahci.c'
--- grub-core/disk/ahci.c	2012-02-08 18:26:01 +0000
+++ grub-core/disk/ahci.c	2013-01-01 12:50:14 +0000
@@ -254,9 +254,10 @@ init_port (struct grub_ahci_device *dev)
   return 1;
 }
 
-static int NESTED_FUNC_ATTR
+static int
 grub_ahci_pciinit (grub_pci_device_t dev,
-		   grub_pci_id_t pciid __attribute__ ((unused)))
+		   grub_pci_id_t pciid __attribute__ ((unused)),
+		   void *data __attribute__ ((unused)))
 {
   grub_pci_address_t addr;
   grub_uint32_t class;
@@ -394,7 +395,7 @@ grub_ahci_pciinit (grub_pci_device_t dev
 static grub_err_t
 grub_ahci_initialize (void)
 {
-  grub_pci_iterate (grub_ahci_pciinit);
+  grub_pci_iterate (grub_ahci_pciinit, NULL);
   return grub_errno;
 }
 

=== modified file 'grub-core/disk/pata.c'
--- grub-core/disk/pata.c	2012-06-06 10:38:49 +0000
+++ grub-core/disk/pata.c	2013-01-01 12:50:14 +0000
@@ -338,9 +338,10 @@ grub_pata_device_initialize (int port, i
 }
 
 #ifndef GRUB_MACHINE_MIPS_QEMU_MIPS
-static int NESTED_FUNC_ATTR
+static int
 grub_pata_pciinit (grub_pci_device_t dev,
-		   grub_pci_id_t pciid)
+		   grub_pci_id_t pciid,
+		   void *data __attribute__ ((unused)))
 {
   static int compat_use[2] = { 0 };
   grub_pci_address_t addr;
@@ -446,7 +447,7 @@ grub_pata_pciinit (grub_pci_device_t dev
 static grub_err_t
 grub_pata_initialize (void)
 {
-  grub_pci_iterate (grub_pata_pciinit);
+  grub_pci_iterate (grub_pata_pciinit, NULL);
   return 0;
 }
 #else

=== modified file 'grub-core/kern/efi/mm.c'
--- grub-core/kern/efi/mm.c	2012-05-03 15:26:55 +0000
+++ grub-core/kern/efi/mm.c	2013-01-01 14:26:12 +0000
@@ -109,37 +109,36 @@ grub_efi_free_pages (grub_efi_physical_a
 
 #if defined (__i386__) || defined (__x86_64__)
 
+/* Helper for stop_broadcom.  */
+static int
+find_card (grub_pci_device_t dev, grub_pci_id_t pciid,
+	   void *data __attribute__ ((unused)))
+{
+  grub_pci_address_t addr;
+  grub_uint8_t cap;
+  grub_uint16_t pm_state;
+
+  if ((pciid & 0xffff) != GRUB_PCI_VENDOR_BROADCOM)
+    return 0;
+
+  addr = grub_pci_make_address (dev, GRUB_PCI_REG_CLASS);
+  if (grub_pci_read (addr) >> 24 != GRUB_PCI_CLASS_NETWORK)
+    return 0;
+  cap = grub_pci_find_capability (dev, GRUB_PCI_CAP_POWER_MANAGEMENT);
+  if (!cap)
+    return 0;
+  addr = grub_pci_make_address (dev, cap + 4);
+  pm_state = grub_pci_read_word (addr);
+  pm_state = pm_state | 0x03;
+  grub_pci_write_word (addr, pm_state);
+  grub_pci_read_word (addr);
+  return 0;
+}
+
 static void
 stop_broadcom (void)
 {
-  auto int NESTED_FUNC_ATTR find_card (grub_pci_device_t dev,
-				       grub_pci_id_t pciid);
-
-  int NESTED_FUNC_ATTR find_card (grub_pci_device_t dev,
-				  grub_pci_id_t pciid)
-    {
-      grub_pci_address_t addr;
-      grub_uint8_t cap;
-      grub_uint16_t pm_state;
-
-      if ((pciid & 0xffff) != GRUB_PCI_VENDOR_BROADCOM)
-	return 0;
-
-      addr = grub_pci_make_address (dev, GRUB_PCI_REG_CLASS);
-      if (grub_pci_read (addr) >> 24 != GRUB_PCI_CLASS_NETWORK)
-	return 0;
-      cap = grub_pci_find_capability (dev, GRUB_PCI_CAP_POWER_MANAGEMENT);
-      if (!cap)
-	return 0;
-      addr = grub_pci_make_address (dev, cap + 4);
-      pm_state = grub_pci_read_word (addr);
-      pm_state = pm_state | 0x03;
-      grub_pci_write_word (addr, pm_state);
-      grub_pci_read_word (addr);
-      return 0;
-    }
-
-  grub_pci_iterate (find_card);
+  grub_pci_iterate (find_card, NULL);
 }
 
 #endif

=== modified file 'grub-core/kern/mips/loongson/init.c'
--- grub-core/kern/mips/loongson/init.c	2012-06-11 18:44:38 +0000
+++ grub-core/kern/mips/loongson/init.c	2013-01-01 12:50:14 +0000
@@ -49,45 +49,47 @@ grub_machine_mmap_iterate (grub_memory_h
   return GRUB_ERR_NONE;
 }
 
+/* Helper for init_pci.  */
+static int
+set_card (grub_pci_device_t dev, grub_pci_id_t pciid,
+	  void *data __attribute__ ((unused)))
+{
+  grub_pci_address_t addr;
+  /* FIXME: autoscan for BARs and devices.  */
+  switch (pciid)
+    {
+    case GRUB_LOONGSON_OHCI_PCIID:
+      addr = grub_pci_make_address (dev, GRUB_PCI_REG_ADDRESS_REG0);
+      grub_pci_write (addr, 0x5025000);
+      addr = grub_pci_make_address (dev, GRUB_PCI_REG_COMMAND);
+      grub_pci_write_word (addr, GRUB_PCI_COMMAND_SERR_ENABLE
+			   | GRUB_PCI_COMMAND_PARITY_ERROR
+			   | GRUB_PCI_COMMAND_BUS_MASTER
+			   | GRUB_PCI_COMMAND_MEM_ENABLED);
+
+      addr = grub_pci_make_address (dev, GRUB_PCI_REG_STATUS);
+      grub_pci_write_word (addr, 0x0200 | GRUB_PCI_STATUS_CAPABILITIES);
+      break;
+    case GRUB_LOONGSON_EHCI_PCIID:
+      addr = grub_pci_make_address (dev, GRUB_PCI_REG_ADDRESS_REG0);
+      grub_pci_write (addr, 0x5026000);
+      addr = grub_pci_make_address (dev, GRUB_PCI_REG_COMMAND);
+      grub_pci_write_word (addr, GRUB_PCI_COMMAND_SERR_ENABLE
+			   | GRUB_PCI_COMMAND_PARITY_ERROR
+			   | GRUB_PCI_COMMAND_BUS_MASTER
+			   | GRUB_PCI_COMMAND_MEM_ENABLED);
+
+      addr = grub_pci_make_address (dev, GRUB_PCI_REG_STATUS);
+      grub_pci_write_word (addr, (1 << GRUB_PCI_STATUS_DEVSEL_TIMING_SHIFT)
+			   | GRUB_PCI_STATUS_CAPABILITIES);
+      break;
+    }
+  return 0;
+}
+
 static void
 init_pci (void)
 {
-  auto int NESTED_FUNC_ATTR set_card (grub_pci_device_t dev, grub_pci_id_t pciid);
-  int NESTED_FUNC_ATTR set_card (grub_pci_device_t dev, grub_pci_id_t pciid)
-  {
-    grub_pci_address_t addr;
-    /* FIXME: autoscan for BARs and devices.  */
-    switch (pciid)
-      {
-      case GRUB_LOONGSON_OHCI_PCIID:
-	addr = grub_pci_make_address (dev, GRUB_PCI_REG_ADDRESS_REG0);
-	grub_pci_write (addr, 0x5025000);
-	addr = grub_pci_make_address (dev, GRUB_PCI_REG_COMMAND);
-	grub_pci_write_word (addr, GRUB_PCI_COMMAND_SERR_ENABLE
-			     | GRUB_PCI_COMMAND_PARITY_ERROR
-			     | GRUB_PCI_COMMAND_BUS_MASTER
-			     | GRUB_PCI_COMMAND_MEM_ENABLED);
-
-	addr = grub_pci_make_address (dev, GRUB_PCI_REG_STATUS);
-	grub_pci_write_word (addr, 0x0200 | GRUB_PCI_STATUS_CAPABILITIES);
-	break;
-      case GRUB_LOONGSON_EHCI_PCIID:
-	addr = grub_pci_make_address (dev, GRUB_PCI_REG_ADDRESS_REG0);
-	grub_pci_write (addr, 0x5026000);
-	addr = grub_pci_make_address (dev, GRUB_PCI_REG_COMMAND);
-	grub_pci_write_word (addr, GRUB_PCI_COMMAND_SERR_ENABLE
-			     | GRUB_PCI_COMMAND_PARITY_ERROR
-			     | GRUB_PCI_COMMAND_BUS_MASTER
-			     | GRUB_PCI_COMMAND_MEM_ENABLED);
-
-	addr = grub_pci_make_address (dev, GRUB_PCI_REG_STATUS);
-	grub_pci_write_word (addr, (1 << GRUB_PCI_STATUS_DEVSEL_TIMING_SHIFT)
-			     | GRUB_PCI_STATUS_CAPABILITIES);
-	break;
-      }
-    return 0;
-  }
-
   *((volatile grub_uint32_t *) GRUB_CPU_LOONGSON_PCI_HIT1_SEL_LO) = 0x8000000c;
   *((volatile grub_uint32_t *) GRUB_CPU_LOONGSON_PCI_HIT1_SEL_HI) = 0xffffffff;
 
@@ -110,7 +112,7 @@ init_pci (void)
   *((volatile grub_uint32_t *) (GRUB_MACHINE_PCI_CONTROLLER_HEADER 
 				+ GRUB_PCI_REG_ADDRESS_REG1)) = 0;
 
-  grub_pci_iterate (set_card);
+  grub_pci_iterate (set_card, NULL);
 }
 
 void

=== modified file 'grub-core/kern/vga_init.c'
--- grub-core/kern/vga_init.c	2011-07-05 21:46:15 +0000
+++ grub-core/kern/vga_init.c	2013-01-01 14:39:55 +0000
@@ -18,6 +18,7 @@
 
 #ifndef __mips__
 #include <grub/pci.h>
+#include <grub/mm.h>
 #endif
 #include <grub/machine/kernel.h>
 #include <grub/misc.h>
@@ -87,38 +88,42 @@ load_palette (void)
     grub_vga_palette_write (i, colors[i].r, colors[i].g, colors[i].b);
 }
 
+#ifndef __mips__
+/* Helper for grub_qemu_init_cirrus.  */
+static int
+find_card (grub_pci_device_t dev, grub_pci_id_t pciid __attribute__ ((unused)),
+	   void *data __attribute__ ((unused)))
+{
+  grub_pci_address_t addr;
+  grub_uint32_t class;
+
+  addr = grub_pci_make_address (dev, GRUB_PCI_REG_CLASS);
+  class = grub_pci_read (addr);
+
+  if (((class >> 16) & 0xffff) != GRUB_PCI_CLASS_SUBCLASS_VGA)
+    return 0;
+  
+  /* FIXME: chooose addresses dynamically.  */
+  addr = grub_pci_make_address (dev, GRUB_PCI_REG_ADDRESS_REG0);
+  grub_pci_write (addr, 0xf0000000 | GRUB_PCI_ADDR_MEM_PREFETCH
+		  | GRUB_PCI_ADDR_SPACE_MEMORY | GRUB_PCI_ADDR_MEM_TYPE_32);
+  addr = grub_pci_make_address (dev, GRUB_PCI_REG_ADDRESS_REG1);
+  grub_pci_write (addr, 0xf2000000
+		  | GRUB_PCI_ADDR_SPACE_MEMORY | GRUB_PCI_ADDR_MEM_TYPE_32);
+
+  addr = grub_pci_make_address (dev, GRUB_PCI_REG_COMMAND);
+  grub_pci_write (addr, GRUB_PCI_COMMAND_MEM_ENABLED
+		  | GRUB_PCI_COMMAND_IO_ENABLED);
+  
+  return 1;
+}
+#endif
+
 void
 grub_qemu_init_cirrus (void)
 {
 #ifndef __mips__
-  auto int NESTED_FUNC_ATTR find_card (grub_pci_device_t dev, grub_pci_id_t pciid);
-  int NESTED_FUNC_ATTR find_card (grub_pci_device_t dev, grub_pci_id_t pciid __attribute__ ((unused)))
-    {
-      grub_pci_address_t addr;
-      grub_uint32_t class;
-
-      addr = grub_pci_make_address (dev, GRUB_PCI_REG_CLASS);
-      class = grub_pci_read (addr);
-
-      if (((class >> 16) & 0xffff) != GRUB_PCI_CLASS_SUBCLASS_VGA)
-	return 0;
-      
-      /* FIXME: chooose addresses dynamically.  */
-      addr = grub_pci_make_address (dev, GRUB_PCI_REG_ADDRESS_REG0);
-      grub_pci_write (addr, 0xf0000000 | GRUB_PCI_ADDR_MEM_PREFETCH
-		      | GRUB_PCI_ADDR_SPACE_MEMORY | GRUB_PCI_ADDR_MEM_TYPE_32);
-      addr = grub_pci_make_address (dev, GRUB_PCI_REG_ADDRESS_REG1);
-      grub_pci_write (addr, 0xf2000000
-		      | GRUB_PCI_ADDR_SPACE_MEMORY | GRUB_PCI_ADDR_MEM_TYPE_32);
- 
-      addr = grub_pci_make_address (dev, GRUB_PCI_REG_COMMAND);
-      grub_pci_write (addr, GRUB_PCI_COMMAND_MEM_ENABLED
-		      | GRUB_PCI_COMMAND_IO_ENABLED);
-      
-      return 1;
-    }
-
-  grub_pci_iterate (find_card);
+  grub_pci_iterate (find_card, NULL);
 #endif
 
   grub_outb (GRUB_VGA_IO_MISC_COLOR,

=== modified file 'grub-core/video/bochs.c'
--- grub-core/video/bochs.c	2012-12-30 09:57:58 +0000
+++ grub-core/video/bochs.c	2013-01-01 12:50:14 +0000
@@ -199,6 +199,29 @@ grub_video_bochs_set_palette (unsigned i
   return grub_video_fb_set_palette (start, count, palette_data);
 }
 
+/* Helper for grub_video_bochs_setup.  */
+static int
+find_card (grub_pci_device_t dev, grub_pci_id_t pciid, void *data)
+{
+  int *found = data;
+  grub_pci_address_t addr;
+  grub_uint32_t class;
+
+  addr = grub_pci_make_address (dev, GRUB_PCI_REG_CLASS);
+  class = grub_pci_read (addr);
+
+  if (((class >> 16) & 0xffff) != 0x0300 || pciid != 0x11111234)
+    return 0;
+  
+  *found = 1;
+
+  addr = grub_pci_make_address (dev, GRUB_PCI_REG_ADDRESS_REG0);
+  framebuffer.base = grub_pci_read (addr) & GRUB_PCI_ADDR_MEM_MASK;
+  framebuffer.dev = dev;
+
+  return 1;
+}
+
 static grub_err_t
 grub_video_bochs_setup (unsigned int width, unsigned int height,
 			grub_video_mode_type_t mode_type,
@@ -210,27 +233,6 @@ grub_video_bochs_setup (unsigned int wid
   int pitch, bytes_per_pixel;
   grub_size_t page_size;        /* The size of a page in bytes.  */
 
-  auto int NESTED_FUNC_ATTR find_card (grub_pci_device_t dev, grub_pci_id_t pciid);
-  int NESTED_FUNC_ATTR find_card (grub_pci_device_t dev, grub_pci_id_t pciid)
-    {
-      grub_pci_address_t addr;
-      grub_uint32_t class;
-
-      addr = grub_pci_make_address (dev, GRUB_PCI_REG_CLASS);
-      class = grub_pci_read (addr);
-
-      if (((class >> 16) & 0xffff) != 0x0300 || pciid != 0x11111234)
-	return 0;
-      
-      found = 1;
-
-      addr = grub_pci_make_address (dev, GRUB_PCI_REG_ADDRESS_REG0);
-      framebuffer.base = grub_pci_read (addr) & GRUB_PCI_ADDR_MEM_MASK;
-      framebuffer.dev = dev;
-
-      return 1;
-    }
-
   /* Decode depth from mode_type.  If it is zero, then autodetect.  */
   depth = (mode_type & GRUB_VIDEO_MODE_TYPE_DEPTH_MASK)
           >> GRUB_VIDEO_MODE_TYPE_DEPTH_POS;
@@ -280,7 +282,7 @@ grub_video_bochs_setup (unsigned int wid
   if (page_size > BOCHS_APERTURE_SIZE)
     return grub_error (GRUB_ERR_IO, "Not enough video memory for this mode");
 
-  grub_pci_iterate (find_card);
+  grub_pci_iterate (find_card, &found);
   if (!found)
     return grub_error (GRUB_ERR_IO, "Couldn't find graphics card");
 

=== modified file 'grub-core/video/cirrus.c'
--- grub-core/video/cirrus.c	2012-12-30 09:57:58 +0000
+++ grub-core/video/cirrus.c	2013-01-01 12:50:14 +0000
@@ -235,6 +235,29 @@ grub_video_cirrus_set_palette (unsigned
   return grub_video_fb_set_palette (start, count, palette_data);
 }
 
+/* Helper for grub_video_cirrus_setup.  */
+static int
+find_card (grub_pci_device_t dev, grub_pci_id_t pciid, void *data)
+{
+  int *found = data;
+  grub_pci_address_t addr;
+  grub_uint32_t class;
+
+  addr = grub_pci_make_address (dev, GRUB_PCI_REG_CLASS);
+  class = grub_pci_read (addr);
+
+  if (((class >> 16) & 0xffff) != 0x0300 || pciid != 0x00b81013)
+    return 0;
+  
+  *found = 1;
+
+  addr = grub_pci_make_address (dev, GRUB_PCI_REG_ADDRESS_REG0);
+  framebuffer.base = grub_pci_read (addr) & GRUB_PCI_ADDR_MEM_MASK;
+  framebuffer.dev = dev;
+
+  return 1;
+}
+
 static grub_err_t
 grub_video_cirrus_setup (unsigned int width, unsigned int height,
 			 grub_video_mode_type_t mode_type,
@@ -245,27 +268,6 @@ grub_video_cirrus_setup (unsigned int wi
   int found = 0;
   int pitch, bytes_per_pixel;
 
-  auto int NESTED_FUNC_ATTR find_card (grub_pci_device_t dev, grub_pci_id_t pciid);
-  int NESTED_FUNC_ATTR find_card (grub_pci_device_t dev, grub_pci_id_t pciid)
-    {
-      grub_pci_address_t addr;
-      grub_uint32_t class;
-
-      addr = grub_pci_make_address (dev, GRUB_PCI_REG_CLASS);
-      class = grub_pci_read (addr);
-
-      if (((class >> 16) & 0xffff) != 0x0300 || pciid != 0x00b81013)
-	return 0;
-      
-      found = 1;
-
-      addr = grub_pci_make_address (dev, GRUB_PCI_REG_ADDRESS_REG0);
-      framebuffer.base = grub_pci_read (addr) & GRUB_PCI_ADDR_MEM_MASK;
-      framebuffer.dev = dev;
-
-      return 1;
-    }
-
   /* Decode depth from mode_type.  If it is zero, then autodetect.  */
   depth = (mode_type & GRUB_VIDEO_MODE_TYPE_DEPTH_MASK)
           >> GRUB_VIDEO_MODE_TYPE_DEPTH_POS;
@@ -314,7 +316,7 @@ grub_video_cirrus_setup (unsigned int wi
   if (framebuffer.page_size > CIRRUS_APERTURE_SIZE)
     return grub_error (GRUB_ERR_IO, "Not enough video memory for this mode");
 
-  grub_pci_iterate (find_card);
+  grub_pci_iterate (find_card, &found);
   if (!found)
     return grub_error (GRUB_ERR_IO, "Couldn't find graphics card");
 

=== modified file 'grub-core/video/efi_uga.c'
--- grub-core/video/efi_uga.c	2012-02-27 13:13:24 +0000
+++ grub-core/video/efi_uga.c	2013-01-01 13:05:55 +0000
@@ -81,77 +81,88 @@ find_line_len (grub_uint32_t *fb_base, g
   return 0;
 }
 
-static int
-find_framebuf (grub_uint32_t *fb_base, grub_uint32_t *line_len)
+/* Context for find_framebuf.  */
+struct find_framebuf_ctx
 {
-  int found = 0;
+  grub_uint32_t *fb_base;
+  grub_uint32_t *line_len;
+  int found;
+};
 
-  auto int NESTED_FUNC_ATTR find_card (grub_pci_device_t dev,
-				       grub_pci_id_t pciid);
+/* Helper for find_framebuf.  */
+static int
+find_card (grub_pci_device_t dev, grub_pci_id_t pciid, void *data)
+{
+  struct find_framebuf_ctx *ctx = data;
+  grub_pci_address_t addr;
 
-  int NESTED_FUNC_ATTR find_card (grub_pci_device_t dev,
-				  grub_pci_id_t pciid)
+  addr = grub_pci_make_address (dev, GRUB_PCI_REG_CLASS);
+  if (grub_pci_read (addr) >> 24 == 0x3)
     {
-      grub_pci_address_t addr;
+      int i;
 
-      addr = grub_pci_make_address (dev, GRUB_PCI_REG_CLASS);
-      if (grub_pci_read (addr) >> 24 == 0x3)
+      grub_dprintf ("fb", "Display controller: %d:%d.%d\nDevice id: %x\n",
+		    grub_pci_get_bus (dev), grub_pci_get_device (dev),
+		    grub_pci_get_function (dev), pciid);
+      addr += 8;
+      for (i = 0; i < 6; i++, addr += 4)
 	{
-	  int i;
+	  grub_uint32_t old_bar1, old_bar2, type;
+	  grub_uint64_t base64;
+
+	  old_bar1 = grub_pci_read (addr);
+	  if ((! old_bar1) || (old_bar1 & GRUB_PCI_ADDR_SPACE_IO))
+	    continue;
 
-	  grub_dprintf ("fb", "Display controller: %d:%d.%d\nDevice id: %x\n",
-			grub_pci_get_bus (dev), grub_pci_get_device (dev),
-			grub_pci_get_function (dev), pciid);
-	  addr += 8;
-	  for (i = 0; i < 6; i++, addr += 4)
+	  type = old_bar1 & GRUB_PCI_ADDR_MEM_TYPE_MASK;
+	  if (type == GRUB_PCI_ADDR_MEM_TYPE_64)
 	    {
-	      grub_uint32_t old_bar1, old_bar2, type;
-	      grub_uint64_t base64;
+	      if (i == 5)
+		break;
 
-	      old_bar1 = grub_pci_read (addr);
-	      if ((! old_bar1) || (old_bar1 & GRUB_PCI_ADDR_SPACE_IO))
-		continue;
-
-	      type = old_bar1 & GRUB_PCI_ADDR_MEM_TYPE_MASK;
-	      if (type == GRUB_PCI_ADDR_MEM_TYPE_64)
-		{
-		  if (i == 5)
-		    break;
-
-		  old_bar2 = grub_pci_read (addr + 4);
-		}
-	      else
-		old_bar2 = 0;
-
-	      base64 = old_bar2;
-	      base64 <<= 32;
-	      base64 |= (old_bar1 & GRUB_PCI_ADDR_MEM_MASK);
-
-	      grub_dprintf ("fb", "%s(%d): 0x%llx\n",
-			    ((old_bar1 & GRUB_PCI_ADDR_MEM_PREFETCH) ?
-			    "VMEM" : "MMIO"), i,
-			   (unsigned long long) base64);
-
-	      if ((old_bar1 & GRUB_PCI_ADDR_MEM_PREFETCH) && (! found))
-		{
-		  *fb_base = base64;
-		  if (find_line_len (fb_base, line_len))
-		    found++;
-		}
-
-	      if (type == GRUB_PCI_ADDR_MEM_TYPE_64)
-		{
-		  i++;
-		  addr += 4;
-		}
+	      old_bar2 = grub_pci_read (addr + 4);
+	    }
+	  else
+	    old_bar2 = 0;
+
+	  base64 = old_bar2;
+	  base64 <<= 32;
+	  base64 |= (old_bar1 & GRUB_PCI_ADDR_MEM_MASK);
+
+	  grub_dprintf ("fb", "%s(%d): 0x%llx\n",
+			((old_bar1 & GRUB_PCI_ADDR_MEM_PREFETCH) ?
+			"VMEM" : "MMIO"), i,
+		       (unsigned long long) base64);
+
+	  if ((old_bar1 & GRUB_PCI_ADDR_MEM_PREFETCH) && (! ctx->found))
+	    {
+	      *ctx->fb_base = base64;
+	      if (find_line_len (ctx->fb_base, ctx->line_len))
+		ctx->found++;
 	    }
-	}
 
-      return found;
+	  if (type == GRUB_PCI_ADDR_MEM_TYPE_64)
+	    {
+	      i++;
+	      addr += 4;
+	    }
+	}
     }
 
-  grub_pci_iterate (find_card);
-  return found;
+  return ctx->found;
+}
+
+static int
+find_framebuf (grub_uint32_t *fb_base, grub_uint32_t *line_len)
+{
+  struct find_framebuf_ctx ctx = {
+    .fb_base = fb_base,
+    .line_len = line_len,
+    .found = 0
+  };
+
+  grub_pci_iterate (find_card, &ctx);
+  return ctx.found;
 }
 
 static int

=== modified file 'grub-core/video/radeon_fuloong2e.c'
--- grub-core/video/radeon_fuloong2e.c	2012-12-30 09:57:58 +0000
+++ grub-core/video/radeon_fuloong2e.c	2013-01-01 12:50:14 +0000
@@ -60,6 +60,32 @@ grub_video_radeon_fuloong2e_video_fini (
   return grub_video_fb_fini ();
 }
 
+#ifndef TEST
+/* Helper for grub_video_radeon_fuloong2e_setup.  */
+static int
+find_card (grub_pci_device_t dev, grub_pci_id_t pciid, void *data)
+{
+  int *found = data;
+  grub_pci_address_t addr;
+  grub_uint32_t class;
+
+  addr = grub_pci_make_address (dev, GRUB_PCI_REG_CLASS);
+  class = grub_pci_read (addr);
+
+  if (((class >> 16) & 0xffff) != GRUB_PCI_CLASS_SUBCLASS_VGA
+      || pciid != 0x515a1002)
+    return 0;
+  
+  *found = 1;
+
+  addr = grub_pci_make_address (dev, GRUB_PCI_REG_ADDRESS_REG0);
+  framebuffer.base = grub_pci_read (addr);
+  framebuffer.dev = dev;
+
+  return 1;
+}
+#endif
+
 static grub_err_t
 grub_video_radeon_fuloong2e_setup (unsigned int width, unsigned int height,
 			unsigned int mode_type, unsigned int mode_mask __attribute__ ((unused)))
@@ -69,28 +95,6 @@ grub_video_radeon_fuloong2e_setup (unsig
   int found = 0;
 
 #ifndef TEST
-  auto int NESTED_FUNC_ATTR find_card (grub_pci_device_t dev, grub_pci_id_t pciid);
-  int NESTED_FUNC_ATTR find_card (grub_pci_device_t dev, grub_pci_id_t pciid)
-    {
-      grub_pci_address_t addr;
-      grub_uint32_t class;
-
-      addr = grub_pci_make_address (dev, GRUB_PCI_REG_CLASS);
-      class = grub_pci_read (addr);
-
-      if (((class >> 16) & 0xffff) != GRUB_PCI_CLASS_SUBCLASS_VGA
-	  || pciid != 0x515a1002)
-	return 0;
-      
-      found = 1;
-
-      addr = grub_pci_make_address (dev, GRUB_PCI_REG_ADDRESS_REG0);
-      framebuffer.base = grub_pci_read (addr);
-      framebuffer.dev = dev;
-
-      return 1;
-    }
-
   /* Decode depth from mode_type.  If it is zero, then autodetect.  */
   depth = (mode_type & GRUB_VIDEO_MODE_TYPE_DEPTH_MASK)
           >> GRUB_VIDEO_MODE_TYPE_DEPTH_POS;
@@ -100,7 +104,7 @@ grub_video_radeon_fuloong2e_setup (unsig
     return grub_error (GRUB_ERR_NOT_IMPLEMENTED_YET,
 		       "Only 640x480x16 is supported");
 
-  grub_pci_iterate (find_card);
+  grub_pci_iterate (find_card, &found);
   if (!found)
     return grub_error (GRUB_ERR_IO, "Couldn't find graphics card");
 #endif

=== modified file 'grub-core/video/sis315pro.c'
--- grub-core/video/sis315pro.c	2012-12-30 09:57:58 +0000
+++ grub-core/video/sis315pro.c	2013-01-01 12:50:14 +0000
@@ -88,6 +88,37 @@ grub_video_sis315pro_video_fini (void)
 
 #include "sis315_init.c"
 
+#ifndef TEST
+/* Helper for grub_video_sis315pro_setup.  */
+static int
+find_card (grub_pci_device_t dev, grub_pci_id_t pciid, void *data)
+{
+  int *found = data;
+  grub_pci_address_t addr;
+  grub_uint32_t class;
+
+  addr = grub_pci_make_address (dev, GRUB_PCI_REG_CLASS);
+  class = grub_pci_read (addr);
+
+  if (((class >> 16) & 0xffff) != GRUB_PCI_CLASS_SUBCLASS_VGA
+      || pciid != GRUB_SIS315PRO_PCIID)
+    return 0;
+  
+  *found = 1;
+
+  addr = grub_pci_make_address (dev, GRUB_PCI_REG_ADDRESS_REG0);
+  framebuffer.base = grub_pci_read (addr) & GRUB_PCI_ADDR_MEM_MASK;
+  addr = grub_pci_make_address (dev, GRUB_PCI_REG_ADDRESS_REG1);
+  framebuffer.mmiobase = grub_pci_read (addr) & GRUB_PCI_ADDR_MEM_MASK;
+  addr = grub_pci_make_address (dev, GRUB_PCI_REG_ADDRESS_REG2);
+  framebuffer.io = (grub_pci_read (addr) & GRUB_PCI_ADDR_IO_MASK)
+    + GRUB_MACHINE_PCI_IO_BASE;
+  framebuffer.dev = dev;
+
+  return 1;
+}
+#endif
+
 static grub_err_t
 grub_video_sis315pro_setup (unsigned int width, unsigned int height,
 			    unsigned int mode_type,
@@ -99,33 +130,6 @@ grub_video_sis315pro_setup (unsigned int
   unsigned i;
 
 #ifndef TEST
-  auto int NESTED_FUNC_ATTR find_card (grub_pci_device_t dev, grub_pci_id_t pciid);
-  int NESTED_FUNC_ATTR find_card (grub_pci_device_t dev, grub_pci_id_t pciid)
-    {
-      grub_pci_address_t addr;
-      grub_uint32_t class;
-
-      addr = grub_pci_make_address (dev, GRUB_PCI_REG_CLASS);
-      class = grub_pci_read (addr);
-
-      if (((class >> 16) & 0xffff) != GRUB_PCI_CLASS_SUBCLASS_VGA
-	  || pciid != GRUB_SIS315PRO_PCIID)
-	return 0;
-      
-      found = 1;
-
-      addr = grub_pci_make_address (dev, GRUB_PCI_REG_ADDRESS_REG0);
-      framebuffer.base = grub_pci_read (addr) & GRUB_PCI_ADDR_MEM_MASK;
-      addr = grub_pci_make_address (dev, GRUB_PCI_REG_ADDRESS_REG1);
-      framebuffer.mmiobase = grub_pci_read (addr) & GRUB_PCI_ADDR_MEM_MASK;
-      addr = grub_pci_make_address (dev, GRUB_PCI_REG_ADDRESS_REG2);
-      framebuffer.io = (grub_pci_read (addr) & GRUB_PCI_ADDR_IO_MASK)
-	+ GRUB_MACHINE_PCI_IO_BASE;
-      framebuffer.dev = dev;
-
-      return 1;
-    }
-
   /* Decode depth from mode_type.  If it is zero, then autodetect.  */
   depth = (mode_type & GRUB_VIDEO_MODE_TYPE_DEPTH_MASK)
           >> GRUB_VIDEO_MODE_TYPE_DEPTH_POS;
@@ -135,7 +139,7 @@ grub_video_sis315pro_setup (unsigned int
     return grub_error (GRUB_ERR_NOT_IMPLEMENTED_YET,
 		       "Only 640x480x8 is supported");
 
-  grub_pci_iterate (find_card);
+  grub_pci_iterate (find_card, &found);
   if (!found)
     return grub_error (GRUB_ERR_IO, "Couldn't find graphics card");
 #endif

=== modified file 'grub-core/video/sm712.c'
--- grub-core/video/sm712.c	2012-12-30 09:57:58 +0000
+++ grub-core/video/sm712.c	2013-01-01 12:50:14 +0000
@@ -360,6 +360,32 @@ grub_sm712_write_dda_lookup (int idx, gr
 		       GRUB_SM712_CR_DDA_LOOKUP_REG1_START + idx);
 }
 
+#if !defined (TEST) && !defined(GENINIT)
+/* Helper for grub_video_sm712_setup.  */
+static int
+find_card (grub_pci_device_t dev, grub_pci_id_t pciid, void *data)
+{
+  int *found = data;
+  grub_pci_address_t addr;
+  grub_uint32_t class;
+
+  addr = grub_pci_make_address (dev, GRUB_PCI_REG_CLASS);
+  class = grub_pci_read (addr);
+
+  if (((class >> 16) & 0xffff) != GRUB_PCI_CLASS_SUBCLASS_VGA
+      || pciid != GRUB_SM712_PCIID)
+    return 0;
+  
+  *found = 1;
+
+  addr = grub_pci_make_address (dev, GRUB_PCI_REG_ADDRESS_REG0);
+  framebuffer.base = grub_pci_read (addr);
+  framebuffer.dev = dev;
+
+  return 1;
+}
+#endif
+
 static grub_err_t
 grub_video_sm712_setup (unsigned int width, unsigned int height,
 			unsigned int mode_type, unsigned int mode_mask __attribute__ ((unused)))
@@ -370,28 +396,6 @@ grub_video_sm712_setup (unsigned int wid
   grub_err_t err;
   int found = 0;
 
-  auto int NESTED_FUNC_ATTR find_card (grub_pci_device_t dev, grub_pci_id_t pciid);
-  int NESTED_FUNC_ATTR find_card (grub_pci_device_t dev, grub_pci_id_t pciid)
-    {
-      grub_pci_address_t addr;
-      grub_uint32_t class;
-
-      addr = grub_pci_make_address (dev, GRUB_PCI_REG_CLASS);
-      class = grub_pci_read (addr);
-
-      if (((class >> 16) & 0xffff) != GRUB_PCI_CLASS_SUBCLASS_VGA
-	  || pciid != GRUB_SM712_PCIID)
-	return 0;
-      
-      found = 1;
-
-      addr = grub_pci_make_address (dev, GRUB_PCI_REG_ADDRESS_REG0);
-      framebuffer.base = grub_pci_read (addr);
-      framebuffer.dev = dev;
-
-      return 1;
-    }
-
   /* Decode depth from mode_type.  If it is zero, then autodetect.  */
   depth = (mode_type & GRUB_VIDEO_MODE_TYPE_DEPTH_MASK)
           >> GRUB_VIDEO_MODE_TYPE_DEPTH_POS;
@@ -401,7 +405,7 @@ grub_video_sm712_setup (unsigned int wid
     return grub_error (GRUB_ERR_NOT_IMPLEMENTED_YET,
 		       "Only 1024x600x16 is supported");
 
-  grub_pci_iterate (find_card);
+  grub_pci_iterate (find_card, &found);
   if (!found)
     return grub_error (GRUB_ERR_IO, "Couldn't find graphics card");
   /* Fill mode info details.  */

=== modified file 'include/grub/pci.h'
--- include/grub/pci.h	2012-05-04 08:54:38 +0000
+++ include/grub/pci.h	2013-01-01 12:50:14 +0000
@@ -132,13 +132,14 @@ grub_pci_get_function (grub_pci_device_t
 #include <grub/cpu/pci.h>
 #endif
 
-typedef int NESTED_FUNC_ATTR (*grub_pci_iteratefunc_t)
-     (grub_pci_device_t dev, grub_pci_id_t pciid);
+typedef int (*grub_pci_iteratefunc_t)
+     (grub_pci_device_t dev, grub_pci_id_t pciid, void *data);
 
 grub_pci_address_t EXPORT_FUNC(grub_pci_make_address) (grub_pci_device_t dev,
 						       int reg);
 
-void EXPORT_FUNC(grub_pci_iterate) (grub_pci_iteratefunc_t hook);
+void EXPORT_FUNC(grub_pci_iterate) (grub_pci_iteratefunc_t hook,
+				    void *hook_data);
 
 struct grub_pci_dma_chunk;
 

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 <cjwatson@ubuntu.com> 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                                       [cjwatson@ubuntu.com]
> <unnest-pci-iterate.patch>
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel

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

[Vladimir 'φ-coder/phcoder' Serbinenko]

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

On 01.01.2013 19:31, Seth Goldberg wrote:

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

We don't support anything older than 4.2. That's unlikely to change.

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 294 bytes --]

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

[Colin Watson]

On Tue, Jan 01, 2013 at 10:31:52AM -0800, Seth Goldberg wrote:
> Yay!! Does this change the minimum GCC version needed to build?

As Vladimir said, it probably isn't worth the effort at this point for
us to wind back the version we recommend, as I suspect not enough of us
can conveniently test with older versions on a regular basis.  That
said, since I think the increased GCC requirement was due to a bug
related to nested functions, it may be that this improves the situation
for those who need to use older versions despite our documented
requirement.

-- 
Colin Watson                                       [cjwatson@ubuntu.com]

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

[Vladimir 'φ-coder/phcoder' Serbinenko]

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

On 01.01.2013 22:14, Colin Watson wrote:

> On Tue, Jan 01, 2013 at 10:31:52AM -0800, Seth Goldberg wrote:
>> Yay!! Does this change the minimum GCC version needed to build?
> 
> As Vladimir said, it probably isn't worth the effort at this point for
> us to wind back the version we recommend, as I suspect not enough of us
> can conveniently test with older versions on a regular basis.  That
> said, since I think the increased GCC requirement was due to a bug
> related to nested functions, it may be that this improves the situation
> for those who need to use older versions despite our documented
> requirement.
>

Older than 4.2 versions of GCC can compile GRUB 2.00. It's just not
supported. As for 3.x, I don't think it's worth even going there

 



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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 294 bytes --]

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

[Andrey Borzenkov]

В Tue, 1 Jan 2013 14:42:04 +0000
Colin Watson <cjwatson@ubuntu.com> пишет:

> 
>  * 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.

Personally I find "ctx" part a bit confusing. It is not really execution
context in usual sense, it is just collection of random variables. I
would rather go with "struct <name-of-parent>_data" here.

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

[richardvoigt]

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

On Tue, Jan 1, 2013 at 4:37 PM, Andrey Borzenkov <arvidjaar@gmail.com>wrote:

> В Tue, 1 Jan 2013 14:42:04 +0000
> Colin Watson <cjwatson@ubuntu.com> пишет:
>
> >
> >  * 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.
>
> Personally I find "ctx" part a bit confusing. It is not really execution
> context in usual sense, it is just collection of random variables. I
> would rather go with "struct <name-of-parent>_data" here.
>

It's acting as a closure.  That might be a more exact name.


>
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel
>

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

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

[Colin Watson]

On Wed, Jan 02, 2013 at 01:37:38AM +0400, Andrey Borzenkov wrote:
> В Tue, 1 Jan 2013 14:42:04 +0000
> Colin Watson <cjwatson@ubuntu.com> пишет:
> >  * 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.
> 
> Personally I find "ctx" part a bit confusing. It is not really execution
> context in usual sense, it is just collection of random variables. I
> would rather go with "struct <name-of-parent>_data" here.

I'm fine with that (and this is exactly why I posted this for a bit of a
bikeshedding opportunity :-) ).  Vladimir, any opinions on the naming?

-- 
Colin Watson                                       [cjwatson@ubuntu.com]

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

[Colin Watson]

On Tue, Jan 01, 2013 at 05:24:00PM -0500, richardvoigt@gmail.com wrote:
> On Tue, Jan 1, 2013 at 4:37 PM, Andrey Borzenkov <arvidjaar@gmail.com>wrote:
> > В Tue, 1 Jan 2013 14:42:04 +0000
> > Colin Watson <cjwatson@ubuntu.com> пишет:
> > >  * 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.
> >
> > Personally I find "ctx" part a bit confusing. It is not really execution
> > context in usual sense, it is just collection of random variables. I
> > would rather go with "struct <name-of-parent>_data" here.
> 
> It's acting as a closure.  That might be a more exact name.

"Closure" has all sorts of baggage for different people that I would
rather not invoke.  In particular the context/data/whatever structure
does not contain any reference to the hook function, so in my book
calling it a closure would be confusing; it's really only part of a
closure, namely the set of associated free variables.

-- 
Colin Watson                                       [cjwatson@ubuntu.com]

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

[Colin Watson]

On Wed, Jan 02, 2013 at 12:05:04AM +0000, Colin Watson wrote:
> On Wed, Jan 02, 2013 at 01:37:38AM +0400, Andrey Borzenkov wrote:
> > В Tue, 1 Jan 2013 14:42:04 +0000
> > Colin Watson <cjwatson@ubuntu.com> пишет:
> > >  * 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.
> > 
> > Personally I find "ctx" part a bit confusing. It is not really execution
> > context in usual sense, it is just collection of random variables. I
> > would rather go with "struct <name-of-parent>_data" here.
> 
> I'm fine with that (and this is exactly why I posted this for a bit of a
> bikeshedding opportunity :-) ).  Vladimir, any opinions on the naming?

Actually, "*_data" is suboptimal because (particularly in filesystem
code) there are many other variables and types called "data".  How about
"*_vars"?  Then I can use "struct foo_vars *vars = data;" or similar as
well and it should work out reasonably well.

-- 
Colin Watson                                       [cjwatson@ubuntu.com]

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

[Vladimir 'φ-coder/phcoder' Serbinenko]

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

On 02.01.2013 03:02, Colin Watson wrote:

> On Wed, Jan 02, 2013 at 12:05:04AM +0000, Colin Watson wrote:
>> On Wed, Jan 02, 2013 at 01:37:38AM +0400, Andrey Borzenkov wrote:
>>> В Tue, 1 Jan 2013 14:42:04 +0000
>>> Colin Watson <cjwatson@ubuntu.com> пишет:
>>>>  * 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.
>>>
>>> Personally I find "ctx" part a bit confusing. It is not really execution
>>> context in usual sense, it is just collection of random variables. I
>>> would rather go with "struct <name-of-parent>_data" here.
>>
>> I'm fine with that (and this is exactly why I posted this for a bit of a
>> bikeshedding opportunity :-) ).  Vladimir, any opinions on the naming?
> 
> Actually, "*_data" is suboptimal because (particularly in filesystem
> code) there are many other variables and types called "data".  How about
> "*_vars"?  Then I can use "struct foo_vars *vars = data;" or similar as
> well and it should work out reasonably well.
> 

I feel like "_ctx" is a good one. It is the executional context, it's
just trimmed to what we really need.

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 294 bytes --]

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

[Vladimir 'φ-coder/phcoder' Serbinenko]

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

On 01.01.2013 15:42, Colin Watson wrote:

> 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.

There is also another approach of using iterators of the kind:

struct pci_iterator;
void pci_init (struct pci_iterator *pci);
int pci_iter (struct pci_iterator *pci);
#define FOR_PCI_DEVICE(x) for (pci_init (&x); pci_iter (&x);)

I have done it for PCI but seem to have bzr problem:
bzr: ERROR: Revision {phcoder@gmail.com-20110421142803-h6jncda1j0lf8msf}
not present in
"Graph(StackedParentsProvider(bzrlib.repository._LazyListJoin(([CachingParentsProvider(CallableToParentsProviderAdapter(<bound
method CHKInventoryRepository._get_parent_map_no_fallbacks of
CHKInventoryRepository('file:///home/phcoder/grub2/bzr/.bzr/repository/')>))],
[]))))".

Admittingly for disk/filesystems it's not viable and even for PCI it was
somewhat messy but may be viable in some cases like where we already
switch to FOR_* macros.

> 
> Please review.
> 
> Thanks,
> 
> 
> 
> 
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel



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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 294 bytes --]

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

[Vladimir 'φ-coder/phcoder' Serbinenko]

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

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
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel



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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 294 bytes --]