Pepper IDL: Check for structs in callbacks.

ppapi/generators/idl_c_header.py

 #!/usr/bin/env python
 # Copyright (c) 2012 The Chromium Authors. All rights reserved.
 # Use of this source code is governed by a BSD-style license that can be
 # found in the LICENSE file.
 
 """ Generator for C style prototypes and definitions """
 
 import glob
 import os
 import re
@@ skipping 121 matching lines @@
           form = 'PP_COMPILE_ASSERT_SIZE_IN_BYTES(%s, %s);\n'
       else:
         form = 'PP_COMPILE_ASSERT_SIZE_IN_BYTES(%s, %s);\n'
       item += form % (name, asize[0])
 
     if item: out.Write(item)
   if last_group:
     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]:
       if os.path.isfile(savename):
         print "Removing stale %s for this range." % filenode.GetName()
         os.remove(os.path.realpath(savename))
       return False
 
     out = IDLOutFile(savename)
     self.GenerateHead(out, filenode, releases, options)
@@ skipping 133 matching lines @@
     print "Golden file for M13-M15 failed."
     failed =1
   else:
     print "Golden file for M13-M15 passed."
 
   return failed
 
 if __name__ == '__main__':
   sys.exit(main(sys.argv[1:]))