Pepper IDL: Check for structs in callbacks.

ppapi/generators/idl_c_header.py

Index: ppapi/generators/idl_c_header.py
diff --git a/ppapi/generators/idl_c_header.py b/ppapi/generators/idl_c_header.py
index 2c3cf2cb3a2755a3d14e78ad4c7033449255de79..f97ebfe23adc75a171cdc6bde28156417a6b0867 100755
--- a/ppapi/generators/idl_c_header.py
+++ b/ppapi/generators/idl_c_header.py
@@ -139,11 +139,43 @@ def GenerateHeader(out, filenode, releases):
     out.Write(CommentLines(['*',' @}', '']) + '\n')
 
 
+def CheckTypedefs(filenode, releases):
+  """Checks that typedefs don't specify callbacks that take some structs.
+
+  See http://crbug.com/233439 for details.
+  """
+  typedefs = {}
+  cgen = CGen()

[Mark Seaborn, 2013/04/22 23:48:41]

cgen seems to be unused.

+  node_whitelist = ('PPB_Flash_BrowserOperations_GetSettingsCallback',

[Mark Seaborn, 2013/04/22 23:48:41]

Why does this interface fail the check?

From c/private/ppp_flash_browser_operations.h:

typedef void (*PPB_Flash_BrowserOperations_GetSettingsCallback)(
    void* user_data,
    PP_Bool success,
    PP_Flash_BrowserOperations_Permission default_permission,
    uint32_t site_count,
    const struct PP_Flash_BrowserOperations_SiteSetting sites[]);

PP_Flash_BrowserOperations_Permission is an enum so should be fine.

The arg "X sites[]" is equivalent to "X *sites" in C, so should be fine too.

+                    'PP_Ext_Alarms_OnAlarm_Func_Dev_0_1')

[noelallen1, 2013/04/22 21:48:26]

I think this is the current problem file for NaCl.  Are we not changing this
case, or are we planning to support PP_Var?
Mark?

[Mark Seaborn, 2013/04/22 23:48:41]

We'll have to change that interface if it's going to be usable from PNaCl, but
it can be done in a separate change since I imagine tests etc. will have to be
updated.  Justin, can you add a TODO to fix this case?

+  for node in filenode.GetListOf('Typedef'):
+    if node.GetName() in node_whitelist:
+      continue
+    build_list = node.GetUniqueReleases(releases)
+    callnode = node.GetOneOf('Callspec')
+    if not callnode:
+      typedefs[node.GetName()] = node.GetType(build_list[0])
+    else:
+      for param in callnode.GetListOf('Param'):
+        t = param.GetType(build_list[0])
+        if t.IsA('Struct'):
+          raise Exception('%s is a struct in callback %s. '

[noelallen1, 2013/04/22 21:48:26]

Isn't this only an issue of the param is [in]?  Shouldn't [out] or [inout] be
okay?

[Mark Seaborn, 2013/04/22 23:48:41]

Yes, I think so, on both counts.

+                          'See http://crbug.com/233439' %
+                          (t.GetName(), node.GetName()))
+        elif t.IsA('Typedef'):
+          if t.GetName() in typedefs:
+            # We can't handle typedefs that are defined in another file.
+            if typedefs[t.GetName()].IsA('Struct'):

[noelallen1, 2013/04/22 21:48:26]

I would expect GetType of the typedef to return the appropriate node.   All IDL
files are processes simultaneously, so I would expect all types to be resolved
at this point.  You shouldn't need to look in this file.

+              raise Exception('%s is a struct in callback %s. '
+                              'See http://crbug.com/233439' %
+                              (t.GetName(), node.GetName()))
+
 class HGen(GeneratorByFile):
   def __init__(self):
     Generator.__init__(self, 'C Header', 'cgen', 'Generate the C headers.')
 
   def GenerateFile(self, filenode, releases, options):
+    CheckTypedefs(filenode, releases)
     savename = GetHeaderFromNode(filenode, GetOption('dstroot'))
     my_min, my_max = filenode.GetMinMax(releases)
     if my_min > releases[-1] or my_max < releases[0]: