Pnacl ppapi shim generator (from IDL), based on Noel's first cut.

Pnacl ppapi shim generator (from IDL), based on Noel's first cut.

BUG= http://code.google.com/p/nativeclient/issues/detail?id=2413
TEST= python idl_gen_pnacl.py --test --wnone

Also
** ./generator.py doesn't change
** ./generator.py --wnone --pnacl --pnaclshim=pnacl_shim.c
generates a shim that compiles and works in the NaCl repo.

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=112244

[jvoung - send to chromium..., 2011-11-15 22:33:10]

draft of refactoring the shim generator if you want to take a look (with sample
output.c and output.s files)

[sehr (please use chromium), 2011-11-16 00:44:01]

lgtm

http://codereview.chromium.org/8568025/diff/1/ppapi/generators/idl_gen_pnacl.py
File ppapi/generators/idl_gen_pnacl.py (right):

http://codereview.chromium.org/8568025/diff/1/ppapi/generators/idl_gen_pnacl....
ppapi/generators/idl_gen_pnacl.py:21: Option('pnaclglue', 'Name of the pnacl
glue file.',
We should use either 'glue' or 'shim' consistently.  I don't really care which.

[jvoung - send to chromium..., 2011-11-16 01:45:22]

http://codereview.chromium.org/8568025/diff/1/ppapi/generators/idl_c_proto.py
File ppapi/generators/idl_c_proto.py (right):

http://codereview.chromium.org/8568025/diff/1/ppapi/generators/idl_c_proto.py...
ppapi/generators/idl_c_proto.py:375: func_attributes, is_cast):
changed func_attributes to ptr_prefix
changed is_cast to include_name (and changed callsites so that it is possible to
get an empty "name" for casts)

http://codereview.chromium.org/8568025/diff/1/ppapi/generators/idl_c_proto.py...
ppapi/generators/idl_c_proto.py:457: def GetStructName(self, node, release,
is_latest_release=False):
changed parameter to "include_version" to be more descriptive.

http://codereview.chromium.org/8568025/diff/1/ppapi/generators/idl_gen_pnacl.py
File ppapi/generators/idl_gen_pnacl.py (right):

http://codereview.chromium.org/8568025/diff/1/ppapi/generators/idl_gen_pnacl....
ppapi/generators/idl_gen_pnacl.py:21: Option('pnaclglue', 'Name of the pnacl
glue file.',
On 2011/11/16 00:44:01, sehr wrote:
> We should use either 'glue' or 'shim' consistently.  I don't really care
which.

Changed to shim.

[jvoung - send to chromium..., 2011-11-16 21:43:37]

Added some bits to skip wrapping of an entire interface if none of the methods
need wrapping.

See comment like " /* Not generating wrapper... " in the sample output .c file
if you want to compare, robert.

The option "disable_pnacl_opt" will let us disable that for debugging.

I'm not yet sure how to skip individual methods, but this still helps cut about
half of the interfaces out.

[robertm, 2011-11-16 22:17:12]

initial comments

http://codereview.chromium.org/8568025/diff/8001/ppapi/generators/out4.c
File ppapi/generators/out4.c (right):

http://codereview.chromium.org/8568025/diff/8001/ppapi/generators/out4.c#newc...
ppapi/generators/out4.c:964: static  __attribute__((pnaclcall))  int32_t
PNACL_M14_PPB_Transport_Dev_Connect(PP_Resource transport, struct
PP_CompletionCallback cb) {
maybe add a lint break after
static  __attribute__((pnaclcall))

http://codereview.chromium.org/8568025/diff/8001/ppapi/generators/out4.c#newc...
ppapi/generators/out4.c:1106: /* Not generating wrappers for PPB_Widget_Dev */
maybe an empty line after each "not generating" comment

http://codereview.chromium.org/8568025/diff/8001/ppapi/generators/out4.c#newc...
ppapi/generators/out4.c:1370: static const void *s_wrapPtrs[PNACL_IFACE_COUNT +
1] = {
Not sure about the comments - you may need to emit NULL instead.

it might actually be slightly cleaner to generate an array of structs here which
could look like:

struct {
   char *if_name;
   void *real;   // NULL initially but filled in as you go
   void * wrapped;    
   char needs_wrapping; 
}


PnaclShimIfaceID() would simply iterate over this array and return 
the index of the first matching if_name


BTW: do you handle the case where we have a wrapper but for some reason 
the real_PPBGetInterface returns NULL;

[jvoung - send to chromium..., 2011-11-16 22:51:42]

http://codereview.chromium.org/8568025/diff/8001/ppapi/generators/out4.c
File ppapi/generators/out4.c (right):

http://codereview.chromium.org/8568025/diff/8001/ppapi/generators/out4.c#newc...
ppapi/generators/out4.c:964: static  __attribute__((pnaclcall))  int32_t
PNACL_M14_PPB_Transport_Dev_Connect(PP_Resource transport, struct
PP_CompletionCallback cb) {
On 2011/11/16 22:17:12, robertm wrote:
> maybe add a lint break after
> static  __attribute__((pnaclcall))

Do you mean line break? If so, done.

http://codereview.chromium.org/8568025/diff/8001/ppapi/generators/out4.c#newc...
ppapi/generators/out4.c:1106: /* Not generating wrappers for PPB_Widget_Dev */
On 2011/11/16 22:17:12, robertm wrote:
> maybe an empty line after each "not generating" comment

Done.

http://codereview.chromium.org/8568025/diff/8001/ppapi/generators/out4.c#newc...
ppapi/generators/out4.c:1370: static const void *s_wrapPtrs[PNACL_IFACE_COUNT +
1] = {
On 2011/11/16 22:17:12, robertm wrote:
> Not sure about the comments - you may need to emit NULL instead.
> 

oops, good catch

> it might actually be slightly cleaner to generate an array of structs here
which
> could look like:
> 
> struct {
>    char *if_name;
>    void *real;   // NULL initially but filled in as you go
>    void * wrapped;    
>    char needs_wrapping; 
> }
> 
> 
> PnaclShimIfaceID() would simply iterate over this array and return 
> the index of the first matching if_name
> 

Perhaps put that off to a second iteration? I'm not yet sure how much
restructuring that would involve, but could certainly make PnaclShimIfaceID
smaller.

Another enhancement I was thinking of would be to separate the PPP interfaces
from PPB, since those are looked up by different top-level functions.

> 
> BTW: do you handle the case where we have a wrapper but for some reason 
> the real_PPBGetInterface returns NULL;
> 

It's handled in the Pnacl_PPBGetInterface() function, in that if the real
pointer is NULL, we also return NULL (we shouldn't return the wrapper, since
that will ultimately look for the real pointer).

[jvoung - send to chromium..., 2011-11-17 01:01:54]

Noel: Question, is there a flag to enable generation of private interfaces from
IDL? I ran into a test that used a private interface but wasn't shimmed.

Also, are all interfaces expected to be IDL'ized eventually? I also ran into
some tests that didn't have IDL.

[noelallen1, 2011-11-20 00:32:56]

The default behavior of the generator is to search '.', './dev','./trusted' for
*.idl files.  You can override this behavior by just using:

../api/*.idl ../api/dev/*.idl ../api/trusted/*.idl ../api/private/*.idl

However, I'm not sure you care about private interfaces.  I was under the
impression that those are not availible to untrusted code.

[jvoung - send to chromium..., 2011-11-22 01:14:27]

On 2011/11/20 00:32:56, noelallen1 wrote:
> The default behavior of the generator is to search '.', './dev','./trusted'
for
> *.idl files.  You can override this behavior by just using:
> 
> ../api/*.idl ../api/dev/*.idl ../api/trusted/*.idl ../api/private/*.idl
> 
> However, I'm not sure you care about private interfaces.  I was under the
> impression that those are not availible to untrusted code.


In practice we will probably not care about the private interface. There was one
pdf test in NaCl that tripped up without the glue code -- it may be okay for
pnacl to skip that for now, or I could include that one pdf IDL file when I
generate the glue code, or something...

[jvoung - send to chromium..., 2011-11-22 01:17:02]

Noel: I added a unittest. Are they usually invoked the way I listed in the CL
description? Should I get it to hook into the bots or the PRESUBMIT in some way?

Robert, I restructured the generated code as you suggested and added a
"mystrcmp" to work around the libc dependency. If there are no more objections,
I'll use Sehr's lgtm and submit this initial version.

[jvoung - send to chromium..., 2011-11-22 19:29:20]

On 2011/11/22 01:17:02, jvoung wrote:
> Noel: I added a unittest. Are they usually invoked the way I listed in the CL
> description? Should I get it to hook into the bots or the PRESUBMIT in some
way?

Added hook to PRESUBMIT.py to run the unittest whenever 'ppapi/generators/X'
changes.

[noelallen1, 2011-11-23 18:06:11]

A few style nits, and a question about how your test against the new IDL file
works.  

Finally, while I'm okay with not testing against '--cgen' in your presubmit
update, you should be running ./generator.py as a test before you submit anyway
so....

http://codereview.chromium.org/8568025/diff/30004/ppapi/PRESUBMIT.py
File ppapi/PRESUBMIT.py (right):

http://codereview.chromium.org/8568025/diff/30004/ppapi/PRESUBMIT.py#newcode37
ppapi/PRESUBMIT.py:37: output_api))
Should add the regular generator to this, but that can be a new CL.

http://codereview.chromium.org/8568025/diff/30004/ppapi/generators/idl_gen_pn...
File ppapi/generators/idl_gen_pnacl.py (right):

http://codereview.chromium.org/8568025/diff/30004/ppapi/generators/idl_gen_pn...
ppapi/generators/idl_gen_pnacl.py:33: class PnaclGen(WrapperGen):
Python style is:
"""One line description.

Followed by multi-line 
details.
Here.
"""

http://codereview.chromium.org/8568025/diff/30004/ppapi/generators/idl_gen_wr...
File ppapi/generators/idl_gen_wrapper.py (right):

http://codereview.chromium.org/8568025/diff/30004/ppapi/generators/idl_gen_wr...
ppapi/generators/idl_gen_wrapper.py:54: """WrapperGen - A subclass of Generator
which takes the IDL sources and
"""One Line

Multi
Line
"""

[jvoung - send to chromium..., 2011-11-23 22:39:28]

Not sure which question you had about how the test works, but here is some
clarification just in case:

The test does a sort of string compare (w/ some canonicalization as you did in
your tests) against the wrapper method format as listed in the comments in the
test .idl file.

This is mostly to check that only some of the methods were wrapped (ones with
byval structs).  However, it also ends up checking the exact format of the pnacl
wrappers. E.g.,
- Did it get the interface struct name correct: "struct Foo_1_0", vs "struct
Foo", etc.?
- Does the function signature look the same?

http://codereview.chromium.org/8568025/diff/30004/ppapi/PRESUBMIT.py
File ppapi/PRESUBMIT.py (right):

http://codereview.chromium.org/8568025/diff/30004/ppapi/PRESUBMIT.py#newcode37
ppapi/PRESUBMIT.py:37: output_api))
On 2011/11/23 18:06:11, noelallen1 wrote:
> Should add the regular generator to this, but that can be a new CL.

Do you mean to just run "generator.py" ? I'm not sure how that differs from the
stuff at line 83-88 (except that those are triggered on changes to .h and .idl
while this is triggered on changes to the generator scripts).

http://codereview.chromium.org/8568025/diff/30004/ppapi/generators/idl_gen_pn...
File ppapi/generators/idl_gen_pnacl.py (right):

http://codereview.chromium.org/8568025/diff/30004/ppapi/generators/idl_gen_pn...
ppapi/generators/idl_gen_pnacl.py:33: class PnaclGen(WrapperGen):
On 2011/11/23 18:06:11, noelallen1 wrote:
> Python style is:
> """One line description.
> 
> Followed by multi-line 
> details.
> Here.
> """

Done.

[jvoung - send to chromium..., 2011-11-28 18:57:33]

ptal

[robertm, 2011-11-28 19:44:04]

added a few more comments but I do not really understand well
enough to do it justice.

One thing I would like to see (in a future CL) is the elimination of then enter
and exit messages in favor
of a decorator, e.g.
 http://wordaligned.org/articles/echo

http://codereview.chromium.org/8568025/diff/37002/ppapi/generators/generator.py
File ppapi/generators/generator.py (right):

http://codereview.chromium.org/8568025/diff/37002/ppapi/generators/generator....
ppapi/generators/generator.py:10: from idl_generator import Generator
are all these needed?

http://codereview.chromium.org/8568025/diff/37002/ppapi/generators/idl_gen_pn...
File ppapi/generators/idl_gen_pnacl.py (right):

http://codereview.chromium.org/8568025/diff/37002/ppapi/generators/idl_gen_pn...
ppapi/generators/idl_gen_pnacl.py:15: 
are these all needed?

[jvoung - send to chromium..., 2011-11-28 20:14:25]

http://codereview.chromium.org/8568025/diff/37002/ppapi/generators/generator.py
File ppapi/generators/generator.py (right):

http://codereview.chromium.org/8568025/diff/37002/ppapi/generators/generator....
ppapi/generators/generator.py:10: from idl_generator import Generator
On 2011/11/28 19:44:04, robertm wrote:
> are all these needed?

importing some of these appears to have a side-effect of registering commandline
options, etc.

I've removed the ones that appear to not have this side-effect... hopefully I
didn't remove too many.

http://codereview.chromium.org/8568025/diff/37002/ppapi/generators/idl_gen_pn...
File ppapi/generators/idl_gen_pnacl.py (right):

http://codereview.chromium.org/8568025/diff/37002/ppapi/generators/idl_gen_pn...
ppapi/generators/idl_gen_pnacl.py:15: 
On 2011/11/28 19:44:04, robertm wrote:
> are these all needed?

Removed a few more.

[robertm, 2011-11-28 20:23:06]

can you add comment as this is really unexpected behavior

[jvoung - send to chromium..., 2011-11-28 20:36:40]

On 2011/11/28 20:23:06, robertm wrote:
> can you add comment as this is really unexpected behavior

Done

[robertm, 2011-11-29 00:10:50]

LGTM

assuming there will be a few more rounds to cleanup some of the style
guide violations

[jvoung - send to chromium..., 2011-11-29 21:13:48]

Noel, any other issues?

Robert, sorry one more change on the pnacl side if you want to take a look.
Previously "pnacl_shim.h" was generated too. After trying to integrate it in the
nacl tree I realized it was a pain to get both the non-generated code
(src/untrusted/pnacl_irt_shim/shim_ppapi.c) and the generated code to include
the same "pnacl_shim.h" (mostly, which directory would the generated
"pnacl_shim.h" sit?). So the header is now checked in instead, and both files
can just '#include "ppapi/generators/pnacl_shim.h"'.

+dmichael for OWNERS (for ppapi/PRESUMIT.py)

http://codereview.chromium.org/8568025/diff/30004/ppapi/PRESUBMIT.py
File ppapi/PRESUBMIT.py (right):

http://codereview.chromium.org/8568025/diff/30004/ppapi/PRESUBMIT.py#newcode37
ppapi/PRESUBMIT.py:37: output_api))
On 2011/11/23 22:39:28, jvoung wrote:
> On 2011/11/23 18:06:11, noelallen1 wrote:
> > Should add the regular generator to this, but that can be a new CL.
> 
> Do you mean to just run "generator.py" ? I'm not sure how that differs from
the
> stuff at line 83-88 (except that those are triggered on changes to .h and .idl
> while this is triggered on changes to the generator scripts).

Noel, in a second CL, I can have it run something like line 83-88, but without
limiting the scope to specific .h and .idl files if the generator code changed.

[dmichael (off chromium), 2011-11-30 04:53:15]

Came up with some little nits before I realized you only needed me to look at 1
file.

ppapi/PRESUBMIT.py LGTM

http://codereview.chromium.org/8568025/diff/36004/ppapi/generators/idl_c_prot...
File ppapi/generators/idl_c_proto.py (right):

http://codereview.chromium.org/8568025/diff/36004/ppapi/generators/idl_c_prot...
ppapi/generators/idl_c_proto.py:385: name = '%s%s%s' % (prefix, name, arrayspec)
nit: One of our many Python style guides says not to use % for just basic
concatenation.
http://google-styleguide.googlecode.com/svn/trunk/pyguide.html#Strings

(I've picked on Noel for this, so it's only fair I pick on you, too...  sorry)

http://codereview.chromium.org/8568025/diff/36004/ppapi/generators/idl_c_prot...
ppapi/generators/idl_c_proto.py:467: def GetStructName(self, node, release,
include_version=False):
Maybe should be GetTypeName, since it applies to more than structs? (This may
eventually come up for enums too)

http://codereview.chromium.org/8568025/diff/36004/ppapi/generators/pnacl_shim.h
File ppapi/generators/pnacl_shim.h (right):

http://codereview.chromium.org/8568025/diff/36004/ppapi/generators/pnacl_shim...
ppapi/generators/pnacl_shim.h:13: /* There is not a typedef for
PPP_GetInterface_type, but it is currently
type->Type?

[noelallen1, 2011-11-30 06:58:25]

LGTM

One last note is generation location.  I'll be cleaning up the regular generator
to use <(SHARED_INTERMEDIATE) one we have everything in IDL.  I suspect this is
the correct location for your header as well.  But that's just a heads up.

[jvoung - send to chromium..., 2011-11-30 17:48:03]

Thanks everyone! Committing...

http://codereview.chromium.org/8568025/diff/36004/ppapi/generators/idl_c_prot...
File ppapi/generators/idl_c_proto.py (right):

http://codereview.chromium.org/8568025/diff/36004/ppapi/generators/idl_c_prot...
ppapi/generators/idl_c_proto.py:385: name = '%s%s%s' % (prefix, name, arrayspec)
On 2011/11/30 04:53:15, dmichael wrote:
> nit: One of our many Python style guides says not to use % for just basic
> concatenation.
> http://google-styleguide.googlecode.com/svn/trunk/pyguide.html#Strings
> 
> (I've picked on Noel for this, so it's only fair I pick on you, too...  sorry)

Done.

http://codereview.chromium.org/8568025/diff/36004/ppapi/generators/idl_c_prot...
ppapi/generators/idl_c_proto.py:467: def GetStructName(self, node, release,
include_version=False):
On 2011/11/30 04:53:15, dmichael wrote:
> Maybe should be GetTypeName, since it applies to more than structs? (This may
> eventually come up for enums too)

Yes, this includes unions too, but I'm thinking of leaving this alone for now,
since it is called from DefineStructInternals() which is called from
DefineStruct(). Perhaps we can rename the all three when it comes time.

http://codereview.chromium.org/8568025/diff/36004/ppapi/generators/pnacl_shim.h
File ppapi/generators/pnacl_shim.h (right):

http://codereview.chromium.org/8568025/diff/36004/ppapi/generators/pnacl_shim...
ppapi/generators/pnacl_shim.h:13: /* There is not a typedef for
PPP_GetInterface_type, but it is currently
On 2011/11/30 04:53:15, dmichael wrote:
> type->Type?

Done.