Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
13973011:
Pepper IDL: Check for structs in callbacks. (Closed)
|
Created:
7 years, 8 months ago by teravest Modified:
7 years, 8 months ago CC:
chromium-reviews, Mark Seaborn Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionPepper IDL: Check for structs in callbacks.
It's problematic for PNaCl when callbacks take structs that are passed by
value; there's not enough type information for matching GCC's calling
conventions on the target architecture.
This change scans for typedefs that define functions, and scans the arguments
to see if any are a struct (or a typedef of a struct).
This change whitelists one existing problematic callback.
Tested:
Verified that exceptions are raised for the one type in the whitelist when
the whitelist was empty. Verified that no exceptions are raised with this
change as-is.
BUG=233439
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=196162
Patch Set 1 #
Total comments: 7
Patch Set 2 : Fixes for noelallen #Patch Set 3 : Fixes for mseaborn #
Total comments: 3
Patch Set 4 : Fix mseaborn comments, round 2 #Messages
Total messages: 15 (0 generated)
https://codereview.chromium.org/13973011/diff/1/ppapi/generators/idl_c_header.py File ppapi/generators/idl_c_header.py (right): https://codereview.chromium.org/13973011/diff/1/ppapi/generators/idl_c_header... ppapi/generators/idl_c_header.py:150: 'PP_Ext_Alarms_OnAlarm_Func_Dev_0_1') 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? https://codereview.chromium.org/13973011/diff/1/ppapi/generators/idl_c_header... ppapi/generators/idl_c_header.py:162: raise Exception('%s is a struct in callback %s. ' Isn't this only an issue of the param is [in]? Shouldn't [out] or [inout] be okay? https://codereview.chromium.org/13973011/diff/1/ppapi/generators/idl_c_header... ppapi/generators/idl_c_header.py:168: if typedefs[t.GetName()].IsA('Struct'): 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.
Thanks for looking into this. https://codereview.chromium.org/13973011/diff/1/ppapi/generators/idl_c_header.py File ppapi/generators/idl_c_header.py (right): https://codereview.chromium.org/13973011/diff/1/ppapi/generators/idl_c_header... ppapi/generators/idl_c_header.py:148: cgen = CGen() cgen seems to be unused. https://codereview.chromium.org/13973011/diff/1/ppapi/generators/idl_c_header... ppapi/generators/idl_c_header.py:149: node_whitelist = ('PPB_Flash_BrowserOperations_GetSettingsCallback', 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. https://codereview.chromium.org/13973011/diff/1/ppapi/generators/idl_c_header... ppapi/generators/idl_c_header.py:150: 'PP_Ext_Alarms_OnAlarm_Func_Dev_0_1') On 2013/04/22 21:48:26, noelallen1 wrote: > 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? 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? https://codereview.chromium.org/13973011/diff/1/ppapi/generators/idl_c_header... ppapi/generators/idl_c_header.py:162: raise Exception('%s is a struct in callback %s. ' On 2013/04/22 21:48:26, noelallen1 wrote: > Isn't this only an issue of the param is [in]? Shouldn't [out] or [inout] be > okay? Yes, I think so, on both counts.
On Mon, Apr 22, 2013 at 3:48 PM, <noelallen@chromium.org> wrote: > > https://codereview.chromium.org/13973011/diff/1/ppapi/generators/idl_c_header.py > File ppapi/generators/idl_c_header.py (right): > > https://codereview.chromium.org/13973011/diff/1/ppapi/generators/idl_c_header... > ppapi/generators/idl_c_header.py:150: > 'PP_Ext_Alarms_OnAlarm_Func_Dev_0_1') > 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? > > https://codereview.chromium.org/13973011/diff/1/ppapi/generators/idl_c_header... > ppapi/generators/idl_c_header.py:162: raise Exception('%s is a struct in > callback %s. ' > Isn't this only an issue of the param is [in]? Shouldn't [out] or > [inout] be okay? You're right. I added a filter so we won't warn on [in] parameters or ones that are an array. > > https://codereview.chromium.org/13973011/diff/1/ppapi/generators/idl_c_header... > ppapi/generators/idl_c_header.py:168: if > > typedefs[t.GetName()].IsA('Struct'): > 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. Yes, that's way better. Thanks. > > https://codereview.chromium.org/13973011/
On Mon, Apr 22, 2013 at 5:48 PM, <mseaborn@chromium.org> wrote: > Thanks for looking into this. > > > > https://codereview.chromium.org/13973011/diff/1/ppapi/generators/idl_c_header.py > File ppapi/generators/idl_c_header.py (right): > > https://codereview.chromium.org/13973011/diff/1/ppapi/generators/idl_c_header... > ppapi/generators/idl_c_header.py:148: cgen = CGen() > cgen seems to be unused. You're right, but I'm using it now to filter parameters. > > https://codereview.chromium.org/13973011/diff/1/ppapi/generators/idl_c_header... > ppapi/generators/idl_c_header.py:149: node_whitelist = > ('PPB_Flash_BrowserOperations_GetSettingsCallback', > 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. Fixed. > > > https://codereview.chromium.org/13973011/diff/1/ppapi/generators/idl_c_header... > ppapi/generators/idl_c_header.py:150: > 'PP_Ext_Alarms_OnAlarm_Func_Dev_0_1') > On 2013/04/22 21:48:26, noelallen1 wrote: >> >> 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? > > > 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? Done. > > > https://codereview.chromium.org/13973011/diff/1/ppapi/generators/idl_c_header... > ppapi/generators/idl_c_header.py:162: raise Exception('%s is a struct in > callback %s. ' > On 2013/04/22 21:48:26, noelallen1 wrote: >> >> Isn't this only an issue of the param is [in]? Shouldn't [out] or > > [inout] be >> >> okay? > > > Yes, I think so, on both counts. Fixed. > > https://codereview.chromium.org/13973011/
You need to update this part of the commit message: "This change whitelists the two examples that already exist. There's one limitation to the change: If an argument to a callback function is a typedef which is defined in another IDL file, we won't be able to raise an exception." Then LGTM. https://codereview.chromium.org/13973011/diff/9001/ppapi/generators/idl_c_hea... File ppapi/generators/idl_c_header.py (right): https://codereview.chromium.org/13973011/diff/9001/ppapi/generators/idl_c_hea... ppapi/generators/idl_c_header.py:148: # TODO(teravest): Allow PP_Var structs to be passed as callback parameters. We don't want to allow PP_Var to be passed by value in typedef'd callbacks. Maybe write "Fix the following callback to pass PP_Var by pointer rather than by value". https://codereview.chromium.org/13973011/diff/9001/ppapi/generators/idl_c_hea... ppapi/generators/idl_c_header.py:149: node_whitelist = ('PP_Ext_Alarms_OnAlarm_Func_Dev_0_1',) Nit: '[]' lists are a little neater than '(,)' tuples for variable-length lists since there's no odd trailing comma in the singleton case. https://codereview.chromium.org/13973011/diff/9001/ppapi/generators/idl_c_hea... ppapi/generators/idl_c_header.py:162: if t.IsA('Struct'): How about: while t.IsA('Typedef'): t = t.GetType(build_list[0]) I don't know if you need to handle multiple levels of typedefs, but this at least means you don't have to duplicate the struct check + error message.
On Tue, Apr 23, 2013 at 9:08 AM, <mseaborn@chromium.org> wrote: > You need to update this part of the commit message: > > > "This change whitelists the two examples that already exist. > > There's one limitation to the change: If an argument to a callback function > is > a typedef which is defined in another IDL file, we won't be able to raise an > exception." > > Then LGTM. Done. > > > https://codereview.chromium.org/13973011/diff/9001/ppapi/generators/idl_c_hea... > File ppapi/generators/idl_c_header.py (right): > > https://codereview.chromium.org/13973011/diff/9001/ppapi/generators/idl_c_hea... > ppapi/generators/idl_c_header.py:148: # TODO(teravest): Allow PP_Var > structs to be passed as callback parameters. > We don't want to allow PP_Var to be passed by value in typedef'd > callbacks. Maybe write "Fix the following callback to pass PP_Var by > pointer rather than by value". Done. > > https://codereview.chromium.org/13973011/diff/9001/ppapi/generators/idl_c_hea... > ppapi/generators/idl_c_header.py:149: node_whitelist = > ('PP_Ext_Alarms_OnAlarm_Func_Dev_0_1',) > Nit: '[]' lists are a little neater than '(,)' tuples for > variable-length lists since there's no odd trailing comma in the > singleton case. Done. > > https://codereview.chromium.org/13973011/diff/9001/ppapi/generators/idl_c_hea... > ppapi/generators/idl_c_header.py:162: if t.IsA('Struct'): > How about: > > while t.IsA('Typedef'): > t = t.GetType(build_list[0]) > > I don't know if you need to handle multiple levels of typedefs, but this > at least means you don't have to duplicate the struct check + error > message. I like that better. Thanks. > > https://codereview.chromium.org/13973011/
LGTM. I think this is a positive move forward, but something to consider if we run into this again: The whitelist could be annoying to find and/or maintain. A possible TODO would be to look for a whitelist notation in the IDL instead. Basically, a type should have something like: [passByValue, paramOk=True] Now we can scan ALL params instead of just callback functions. And we put the whitelist in the source instead of code. But that can be a different CL if it makes sense or we run into this again.
On Tue, Apr 23, 2013 at 11:29 AM, <noelallen@chromium.org> wrote: > LGTM. I think this is a positive move forward, but something to consider if > we > run into this again: > > The whitelist could be annoying to find and/or maintain. > A possible TODO would be to look for a whitelist notation in the IDL > instead. > > Basically, a type should have something like: > [passByValue, paramOk=True] > > Now we can scan ALL params instead of just callback functions. And we put > the > whitelist in the source instead of code. I'd considered doing this, but I was afraid that people would copy that annotation from another example without thinking about the problems it would cause with pnacl. I agree that it is more annoying to update the whitelist inside the python code, though. > > But that can be a different CL if it makes sense or we run into this again. > > https://codereview.chromium.org/13973011/
On 23 April 2013 10:49, Justin TerAvest <teravest@chromium.org> wrote: > On Tue, Apr 23, 2013 at 11:29 AM, <noelallen@chromium.org> wrote: > > LGTM. I think this is a positive move forward, but something to > consider if > > we > > run into this again: > > > > The whitelist could be annoying to find and/or maintain. > > A possible TODO would be to look for a whitelist notation in the IDL > > instead. > > > > Basically, a type should have something like: > > [passByValue, paramOk=True] > > > > Now we can scan ALL params instead of just callback functions. And we > put > > the whitelist in the source instead of code. > > I'd considered doing this, but I was afraid that people would copy > that annotation from another example without thinking about the > problems it would cause with pnacl. I agree that it is more annoying > to update the whitelist inside the python code, though. > Right. I think it's better to have the whitelist in a central place in the Python code along with a big comment saying "Do not add to this list". BTW, I'm assuming that in a later change we can add a whitelist to prevent new structs from being declared as passByValue. Cheers, Mark
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/teravest@chromium.org/13973011/13001
Retried try job too often on win7_aura for step(s) interactive_ui_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win7_aura&...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/teravest@chromium.org/13973011/13001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/teravest@chromium.org/13973011/13001
Message was sent while issue was closed.
Change committed as 196162 |
