Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10826171:
Incorporate shimming into the irt (Closed)
|
Created:
8 years, 4 months ago by Robert Muth (chromium) Modified:
8 years, 3 months ago Reviewers:
bradn, jvoung (off chromium), Use chromium.org instead, sehr (please use chromium), Roland McGrath, Mark Seaborn, Nick Bray (chromium) CC:
native-client-reviews_googlegroups.com Visibility:
Public. |
DescriptionMerge shimming into the irt.
This CL moves the functionality from
src/untrusted/pnacl_irt_shim to
src/untrusted/irt
but does not delete the former yet.
The changes are kept minimal and no attempt was made
to simplify the shimming process.
In order to activate the irt based shimming nexe module calls to
GetInterface("nacl-irt-ppapihook-0.1") need to be changed
into GetInterface("nacl-irt-ppapihook-shimmed-0.1")
This can be done via another mini-shim which will added in
a subsequent CL.
BUG= http://code.google.com/p/nativeclient/issues/detail?id=2465
TEST= trybots
Patch Set 1 #Patch Set 2 : #Patch Set 3 : #Patch Set 4 : #Patch Set 5 : #Patch Set 6 : #Patch Set 7 : #Patch Set 8 : #Patch Set 9 : #Patch Set 10 : #
Total comments: 10
Patch Set 11 : #Patch Set 12 : #Patch Set 13 : #
Total comments: 4
Patch Set 14 : #Patch Set 15 : #Patch Set 16 : #Patch Set 17 : #Patch Set 18 : #Patch Set 19 : #Patch Set 20 : #
Total comments: 13
Patch Set 21 : #
Total comments: 52
Patch Set 22 : #Patch Set 23 : #Patch Set 24 : #
Total comments: 9
Patch Set 25 : #Patch Set 26 : #
Total comments: 14
Patch Set 27 : #Messages
Total messages: 21 (0 generated)
This is not quite done yet, e.g. the gyp changes from Jan's CL: http://codereview.chromium.org/8873035/ need to be added. Let me know if you are fine with the general direction.
http://codereview.chromium.org/10826171/diff/2005/src/untrusted/nacl/nacl_irt.c File src/untrusted/nacl/nacl_irt.c (right): http://codereview.chromium.org/10826171/diff/2005/src/untrusted/nacl/nacl_irt... src/untrusted/nacl/nacl_irt.c:104: /* __libnacl_mandatory_irt_query("@nacl-cc-shim-mode", NULL, 0); */ This is not an appropriate place to enable the shims because this file is part of user code (and it's also newlib-only). User PNaCl bitcode should not know about the shims. The shims should be enabled from PNaCl system code, presumably from some code that is inserted to run before the user code's _start entry point.
This also needs a clear plan for how the implementation in the new proxy will work. http://codereview.chromium.org/10826171/diff/2005/src/untrusted/irt/irt_inter... File src/untrusted/irt/irt_interfaces.c (right): http://codereview.chromium.org/10826171/diff/2005/src/untrusted/irt/irt_inter... src/untrusted/irt/irt_interfaces.c:56: if (interface_ident[0] == '@') { I don't see any defensible rationale for this kludgery. Just add a new interface that parallels nacl_irt_ppapihook. The speculated "other things" can be functions available in some new nacl_irt_control interface if they ever get added. http://codereview.chromium.org/10826171/diff/2005/src/untrusted/irt/irt_inter... File src/untrusted/irt/irt_interfaces.h (right): http://codereview.chromium.org/10826171/diff/2005/src/untrusted/irt/irt_inter... src/untrusted/irt/irt_interfaces.h:12: extern int g_use_cc_shim; Put this in irt_private.h instead. http://codereview.chromium.org/10826171/diff/2005/src/untrusted/irt/irt_ppapi.c File src/untrusted/irt/irt_ppapi.c (right): http://codereview.chromium.org/10826171/diff/2005/src/untrusted/irt/irt_ppapi... src/untrusted/irt/irt_ppapi.c:39: __set_real_Pnacl_PPBGetInterface(get_browser_intf); There's no reason to use __ names in any of this. http://codereview.chromium.org/10826171/diff/2005/src/untrusted/nacl/nacl_irt.c File src/untrusted/nacl/nacl_irt.c (right): http://codereview.chromium.org/10826171/diff/2005/src/untrusted/nacl/nacl_irt... src/untrusted/nacl/nacl_irt.c:101: #if defined(__pnacl__) This does not belong here. The shim only has to do with PPAPI interfaces, so it should be done in some PPAPI-specific code.
PTAL still working on the gyp integration http://codereview.chromium.org/10826171/diff/2005/src/untrusted/irt/irt_inter... File src/untrusted/irt/irt_interfaces.c (right): http://codereview.chromium.org/10826171/diff/2005/src/untrusted/irt/irt_inter... src/untrusted/irt/irt_interfaces.c:56: if (interface_ident[0] == '@') { On 2012/08/06 23:32:24, Roland McGrath wrote: > I don't see any defensible rationale for this kludgery. > Just add a new interface that parallels nacl_irt_ppapihook. > The speculated "other things" can be functions available in some new > nacl_irt_control interface if they ever get added. added a new interface but this will likely complicate the switch for pnacl from linktime shims to irt shims. http://codereview.chromium.org/10826171/diff/2005/src/untrusted/irt/irt_inter... File src/untrusted/irt/irt_interfaces.h (right): http://codereview.chromium.org/10826171/diff/2005/src/untrusted/irt/irt_inter... src/untrusted/irt/irt_interfaces.h:12: extern int g_use_cc_shim; On 2012/08/06 23:32:24, Roland McGrath wrote: > Put this in irt_private.h instead. Done. http://codereview.chromium.org/10826171/diff/2005/src/untrusted/irt/irt_ppapi.c File src/untrusted/irt/irt_ppapi.c (right): http://codereview.chromium.org/10826171/diff/2005/src/untrusted/irt/irt_ppapi... src/untrusted/irt/irt_ppapi.c:39: __set_real_Pnacl_PPBGetInterface(get_browser_intf); On 2012/08/06 23:32:24, Roland McGrath wrote: > There's no reason to use __ names in any of this. I agree but the "__" code is generated by python scripts in another repo so this change will have to be moved to another CL. http://codereview.chromium.org/10826171/diff/2005/src/untrusted/nacl/nacl_irt.c File src/untrusted/nacl/nacl_irt.c (right): http://codereview.chromium.org/10826171/diff/2005/src/untrusted/nacl/nacl_irt... src/untrusted/nacl/nacl_irt.c:101: #if defined(__pnacl__) I think it makes sense to keep it here. It is conceivable that the non-ppapi irt api could be affected by this. Do you have any suggestions where to move it instead? On 2012/08/06 23:32:24, Roland McGrath wrote: > This does not belong here. > The shim only has to do with PPAPI interfaces, so it should be done in some > PPAPI-specific code. http://codereview.chromium.org/10826171/diff/2005/src/untrusted/nacl/nacl_irt... src/untrusted/nacl/nacl_irt.c:104: /* __libnacl_mandatory_irt_query("@nacl-cc-shim-mode", NULL, 0); */ On 2012/08/06 23:30:29, Mark Seaborn wrote: > This is not an appropriate place to enable the shims because this file is part > of user code (and it's also newlib-only). User PNaCl bitcode should not know > about the shims. The shims should be enabled from PNaCl system code, presumably > from some code that is inserted to run before the user code's _start entry > point. Moving it elsewhere will likely be quite messy. As the IRT interface is not usable up to this point. Meaning I would have to replicated some of the code here elsewhere. We the new interface, I think it is ok for a pnacl pexe to advertise its nature to the irt.
http://codereview.chromium.org/10826171/diff/10031/src/untrusted/irt/irt_inte... File src/untrusted/irt/irt_interfaces.c (right): http://codereview.chromium.org/10826171/diff/10031/src/untrusted/irt/irt_inte... src/untrusted/irt/irt_interfaces.c:39: { NACL_IRT_CONTROL_v0_1, &nacl_irt_control, sizeof(nacl_irt_control) }, missing file for this? http://codereview.chromium.org/10826171/diff/10031/src/untrusted/irt/irt_ppapi.c File src/untrusted/irt/irt_ppapi.c (right): http://codereview.chromium.org/10826171/diff/10031/src/untrusted/irt/irt_ppap... src/untrusted/irt/irt_ppapi.c:36: static int32_t PPPInitializeModule_cc_shim(PP_Module module_id, add dash or take away dash from other PPP functions to be consistent. E.g., "PPP_InitializeModule_..." to be consistent with "PPP_ShutdownModule_..." http://codereview.chromium.org/10826171/diff/10031/src/untrusted/irt/irt_ppap... src/untrusted/irt/irt_ppapi.c:61: /* register entry points to untrusted pepper plugin */ extra space "entry points" http://codereview.chromium.org/10826171/diff/10031/src/untrusted/irt/irt_ppap... src/untrusted/irt/irt_ppapi.c:63: if (g_pancl_mode) { p-ankle -> pnacl
Add gyp support and addressed various concerns Gyp support was heavily borrowed from Jan's CLs e.g.: http://codereview.chromium.org/8873035/
ping On 2012/08/09 19:37:30, Robert Muth (chromium) wrote: > Add gyp support and addressed various concerns > > Gyp support was heavily borrowed from > Jan's CLs e.g.: > http://codereview.chromium.org/8873035/
We discussed this here a few weeks ago and agreed on what we think the implementation ought to be. In the IRT itself the only change will be the addition of a new query string that is an exact parallel to NACL_IRT_PPAPIHOOK_v0_1. The string can be something like "pnacl-shim-irt-ppapihook-0.1". The IRT will support an application calling the NACL_IRT_PPAPIHOOK_v0_1 ppapi_start function or the pnacl-shim-ppapihook ppapi_start function, but need not support more than one ppapi_start call being made in any given application. The pnacl_irt_shim will continue to exist as it does today, i.e. a small module linked into the translation output and providing its entry point as a wrapper around the _start symbol in the pexe. This will interpose on the IRT query function as it does today, but in a much simpler fashion. All the interposed query function needs to do is check for the NACL_IRT_PPAPIHOOK_v0_1 query string and then call the real IRT query function instead with "pnacl-shim-irt-ppapihook-0.1". This new query string will be in the IRT's de facto ABI, but not part of the published interface (i.e. I don't think it should go into irt.h). This work can and probably should be done in a few separate changes. The first step is the IRT piece, building the PPAPI shim code along with the rest of the proxy and adding the entry for the new query string to the irt_interfaces.c table (under #ifdef IRT_PPAPI). That can be implemented and rolled into Chrome without affecting existing PNaCl usage. Once that new IRT is deployed, the translator side can change to use the simple shim in place of the full PPAPI shim. -- You received this message because you are subscribed to the Google Groups "Native-Client-Reviews" group. To post to this group, send email to native-client-reviews@googlegroups.com. To unsubscribe from this group, send email to native-client-reviews+unsubscribe@googlegroups.com. For more options, visit this group at http://groups.google.com/group/native-client-reviews?hl=en.
PTAL
some nits -- perhaps Mark or Roland can comment about the earlier concerns http://codereview.chromium.org/10826171/diff/24003/build/gyp_glob.py File build/gyp_glob.py (right): http://codereview.chromium.org/10826171/diff/24003/build/gyp_glob.py#newcode14 build/gyp_glob.py:14: Perhaps this can be built into gyp instead. If you are asking BradN about the nacl-chrome split, can you ask him about this too? http://codereview.chromium.org/10826171/diff/24003/src/untrusted/irt/irt.gyp File src/untrusted/irt/irt.gyp (right): http://codereview.chromium.org/10826171/diff/24003/src/untrusted/irt/irt.gyp#... src/untrusted/irt/irt.gyp:55: 'target_name': 'irt_core_common', "irt_core_nexe" doesn't use a shim. How does "irt_core_common" relate to "irt_core_nexe"? http://codereview.chromium.org/10826171/diff/24003/src/untrusted/irt/irt.h File src/untrusted/irt/irt.h (right): http://codereview.chromium.org/10826171/diff/24003/src/untrusted/irt/irt.h#ne... src/untrusted/irt/irt.h:150: #define NACL_IRT_PPAPIHOOK_SHIMMED_v0_1 "nacl-irt-ppapihook-shimmed-0.1" I think Roland recommended to having this in irt.h, but I'm not sure where he was imagining this would sit in. Perhaps inlined into irt_interfaces.cpp? The other CL also seems to rely on these two having the same prefix, and have connected version numbers (if you bump one, you must bump the other). That seems worth commenting, if that's what we go for.
https://chromiumcodereview.appspot.com/10826171/diff/24003/src/untrusted/irt/... File src/untrusted/irt/irt.gyp (right): https://chromiumcodereview.appspot.com/10826171/diff/24003/src/untrusted/irt/... src/untrusted/irt/irt.gyp:55: 'target_name': 'irt_core_common', Instead of having the core irt depend on this. The gyp changes should land on the chrome side. And the ppapi irt should depend on the shim target. https://chromiumcodereview.appspot.com/10826171/diff/24003/src/untrusted/irt/... File src/untrusted/irt/irt_interfaces.c (right): https://chromiumcodereview.appspot.com/10826171/diff/24003/src/untrusted/irt/... src/untrusted/irt/irt_interfaces.c:36: { NACL_IRT_PPAPIHOOK_SHIMMED_v0_1, &nacl_irt_ppapihook_shimmed, This should should be guarded by a temporary #define (NACL_IRT_PPAPI_SHIMS or somesuch). https://chromiumcodereview.appspot.com/10826171/diff/24003/src/untrusted/irt/... File src/untrusted/irt/nacl.scons (right): https://chromiumcodereview.appspot.com/10826171/diff/24003/src/untrusted/irt/... src/untrusted/irt/nacl.scons:30: # Generate a 'pnacl_shim.c' So I realize this is from irt_stubs, but why can't you also delete that version? https://chromiumcodereview.appspot.com/10826171/diff/24003/src/untrusted/irt/... File src/untrusted/irt/shim_generator.gyp (right): https://chromiumcodereview.appspot.com/10826171/diff/24003/src/untrusted/irt/... src/untrusted/irt/shim_generator.gyp:30: # Potentially all python scripts in the generators directory. The globbing isn't thrilling, but we realize its identical to what the scons build does at the moment. Maybe ok to land chrome side to start, but should be made explicit there with a follow on CL. https://chromiumcodereview.appspot.com/10826171/diff/24003/src/untrusted/irt/... src/untrusted/irt/shim_generator.gyp:39: '../../../../ppapi/generators/generator.py', Note how everything here (including the generator script) is on the ppapi side. This suggests landing the change there.
As per a discussion with Nick Bray, removed all the gyp changes. The plan is to commit this. Roll the nacl deps on the chrome side and then add the gyp magic on the chrome side. https://chromiumcodereview.appspot.com/10826171/diff/24003/src/untrusted/irt/... File src/untrusted/irt/irt.h (right): https://chromiumcodereview.appspot.com/10826171/diff/24003/src/untrusted/irt/... src/untrusted/irt/irt.h:150: #define NACL_IRT_PPAPIHOOK_SHIMMED_v0_1 "nacl-irt-ppapihook-shimmed-0.1" On 2012/08/27 21:53:58, jvoung (chromium) wrote: > I think Roland recommended to having this in irt.h, but I'm not sure where he > was imagining this would sit in. Perhaps inlined into irt_interfaces.cpp? > > The other CL also seems to rely on these two having the same prefix, and have > connected version numbers (if you bump one, you must bump the other). That > seems worth commenting, if that's what we go for. Done. https://chromiumcodereview.appspot.com/10826171/diff/24003/src/untrusted/irt/... File src/untrusted/irt/irt_interfaces.c (right): https://chromiumcodereview.appspot.com/10826171/diff/24003/src/untrusted/irt/... src/untrusted/irt/irt_interfaces.c:36: { NACL_IRT_PPAPIHOOK_SHIMMED_v0_1, &nacl_irt_ppapihook_shimmed, On 2012/08/27 23:00:02, Nick Bray (chromium) wrote: > This should should be guarded by a temporary #define (NACL_IRT_PPAPI_SHIMS or > somesuch). Done. https://chromiumcodereview.appspot.com/10826171/diff/24003/src/untrusted/irt/... File src/untrusted/irt/nacl.scons (right): https://chromiumcodereview.appspot.com/10826171/diff/24003/src/untrusted/irt/... src/untrusted/irt/nacl.scons:30: # Generate a 'pnacl_shim.c' On 2012/08/27 23:00:02, Nick Bray (chromium) wrote: > So I realize this is from irt_stubs, but why can't you also delete that version? This has to wait until the new irt is fully functional. The CL for that change is aready out.
https://chromiumcodereview.appspot.com/10826171/diff/24003/src/untrusted/irt/... File src/untrusted/irt/irt_interfaces.c (right): https://chromiumcodereview.appspot.com/10826171/diff/24003/src/untrusted/irt/... src/untrusted/irt/irt_interfaces.c:36: { NACL_IRT_PPAPIHOOK_SHIMMED_v0_1, &nacl_irt_ppapihook_shimmed, On 2012/08/27 23:00:02, Nick Bray (chromium) wrote: > This should should be guarded by a temporary #define (NACL_IRT_PPAPI_SHIMS > or somesuch). @Nick/Brad: why did you say this? https://chromiumcodereview.appspot.com/10826171/diff/35001/src/untrusted/irt/... File src/untrusted/irt/irt.h (right): https://chromiumcodereview.appspot.com/10826171/diff/35001/src/untrusted/irt/... src/untrusted/irt/irt.h:149: /* these should be kept in sync, both are using the nacl_irt_ppapihook API */ If you are going to add comments, please make sure they are well-formatted: sentences should start with capital letters. The same applies to other comments in this change. https://chromiumcodereview.appspot.com/10826171/diff/35001/src/untrusted/irt/... src/untrusted/irt/irt.h:151: #define NACL_IRT_PPAPIHOOK_SHIMMED_v0_1 "nacl-irt-ppapihook-shimmed-0.1" So far irt.h only contains public interfaces. I think we should probably keep it that way, so can you find or create a different header to put this in? https://chromiumcodereview.appspot.com/10826171/diff/35001/src/untrusted/irt/... File src/untrusted/irt/irt_interfaces.c (right): https://chromiumcodereview.appspot.com/10826171/diff/35001/src/untrusted/irt/... src/untrusted/irt/irt_interfaces.c:36: #ifdef DISABLED_WHILE_IN_TRANSITION_TO_CHROME_TREE Why is this #ifdef here? No-one defines this symbol. Plus it looks like this is the wrong way around and should be an #ifndef. Please remove the #ifdef. https://chromiumcodereview.appspot.com/10826171/diff/35001/src/untrusted/irt/... src/untrusted/irt/irt_interfaces.c:48: void *table, The formatting change to this function is unnecessary, so please undo it. https://chromiumcodereview.appspot.com/10826171/diff/35001/src/untrusted/irt/... File src/untrusted/irt/irt_interfaces.h (right): https://chromiumcodereview.appspot.com/10826171/diff/35001/src/untrusted/irt/... src/untrusted/irt/irt_interfaces.h:2: * Copyright 2012 The Native Client Authors. All rights reserved. Why are you changing the copyright messages? The presubmit checks accept the "(c)" part so you don't need to remove it. Same for other files. https://chromiumcodereview.appspot.com/10826171/diff/35001/src/untrusted/irt/... File src/untrusted/irt/irt_ppapi.c (right): https://chromiumcodereview.appspot.com/10826171/diff/35001/src/untrusted/irt/... src/untrusted/irt/irt_ppapi.c:58: * pnacl has calling conventions which are slightly different from nacl-gcc. 'pnacl' -> 'PNaCl' https://chromiumcodereview.appspot.com/10826171/diff/35001/src/untrusted/irt/... src/untrusted/irt/irt_ppapi.c:59: * This affects structure passing on x86-64 which is only used by Ppapi 'Ppapi' -> 'PPAPI' https://chromiumcodereview.appspot.com/10826171/diff/35001/src/untrusted/irt/... src/untrusted/irt/irt_ppapi.c:63: * We interecept those functions, store the old version and then redirect "intercept" https://chromiumcodereview.appspot.com/10826171/diff/35001/src/untrusted/irt/... src/untrusted/irt/irt_ppapi.c:68: struct PP_StartFunctions g_pp_functions_non_shim; This should be static, because it's only used within one file. https://chromiumcodereview.appspot.com/10826171/diff/35001/src/untrusted/irt/... src/untrusted/irt/irt_ppapi.c:71: PPB_GetInterface get_browser_intf) { Please use the full name: "get_browser_interface", no need to abbreviate. https://chromiumcodereview.appspot.com/10826171/diff/35001/src/untrusted/irt/... src/untrusted/irt/irt_ppapi.c:76: module_id, &__Pnacl_PPBGetInterface); Nit: the usual style is to indent arguments by 4 spaces rather than 2 https://chromiumcodereview.appspot.com/10826171/diff/35001/src/untrusted/irt/... src/untrusted/irt/irt_ppapi.c:80: /* redirect to non-shimed version */ "shimmed" https://chromiumcodereview.appspot.com/10826171/diff/35001/src/untrusted/irt/... src/untrusted/irt/irt_ppapi.c:90: &PPP_InitializeModule_cc_shim, We don't use the redundant "&" for getting function pointers elsewhere in the file, so please don't use it here. https://chromiumcodereview.appspot.com/10826171/diff/35001/src/untrusted/irt/... src/untrusted/irt/irt_ppapi.c:97: * to chrome. 'chrome' -> 'Chrome' https://chromiumcodereview.appspot.com/10826171/diff/35001/src/untrusted/irt/... src/untrusted/irt/irt_ppapi.c:98: * We need to shim all these functions and also do some extra bookeeping Isn't this repeating what you said in the earlier comment? The earlier comment should be enough. https://chromiumcodereview.appspot.com/10826171/diff/35001/src/untrusted/irt/... src/untrusted/irt/irt_ppapi.c:108: return PpapiPluginMain(); Rather than duplicating code from irt_ppapi_start() above, please just call irt_ppapi_start() here. https://chromiumcodereview.appspot.com/10826171/diff/35001/src/untrusted/irt/... File src/untrusted/irt/nacl.scons (right): https://chromiumcodereview.appspot.com/10826171/diff/35001/src/untrusted/irt/... src/untrusted/irt/nacl.scons:26: def GetPNaClShimSource(env): Please put this lower in the file, just before LinkIrt(), because it's only an ancillary part of the IRT. https://chromiumcodereview.appspot.com/10826171/diff/35001/src/untrusted/irt/... src/untrusted/irt/nacl.scons:31: # api code Capitalise: "API code" https://chromiumcodereview.appspot.com/10826171/diff/35001/src/untrusted/irt/... src/untrusted/irt/nacl.scons:35: # python code Capitalise as "Python", please https://chromiumcodereview.appspot.com/10826171/diff/35001/src/untrusted/irt/... src/untrusted/irt/nacl.scons:38: 'pnacl_irt_shim.c', Nit: our Python style is that arguments are indented by 4 spaces, not 2 https://chromiumcodereview.appspot.com/10826171/diff/35001/src/untrusted/irt/... src/untrusted/irt/nacl.scons:45: irt_shim_obj = blob_env.ComponentObject(GetPNaClShimSource(blob_env)) I don't think you need ComponentObject() here, do you? I think you can just pass the .c file to ComponentProgram(). https://chromiumcodereview.appspot.com/10826171/diff/35001/src/untrusted/irt/... File src/untrusted/irt/shim_dummy.c (right): https://chromiumcodereview.appspot.com/10826171/diff/35001/src/untrusted/irt/... src/untrusted/irt/shim_dummy.c:1: /* Copyright 2012 The Chromium Authors. All rights reserved. If this is in the NaCl tree, this should say "The Native Client Authors". Also, this boilerplate should start with "/*" on a single line. https://chromiumcodereview.appspot.com/10826171/diff/35001/src/untrusted/irt/... src/untrusted/irt/shim_dummy.c:7: * This module satifies link dependencies in case we do not link "satisfies" Saying "in case we do not link in shimming support" is vague. Why wouldn't shimming support be linked in? Please elaborate. https://chromiumcodereview.appspot.com/10826171/diff/35001/src/untrusted/irt/... src/untrusted/irt/shim_dummy.c:9: * It also rough documents the API of the automatically generated "roughly"
PTAL https://chromiumcodereview.appspot.com/10826171/diff/24003/src/untrusted/irt/... File src/untrusted/irt/irt_interfaces.c (right): https://chromiumcodereview.appspot.com/10826171/diff/24003/src/untrusted/irt/... src/untrusted/irt/irt_interfaces.c:36: { NACL_IRT_PPAPIHOOK_SHIMMED_v0_1, &nacl_irt_ppapihook_shimmed, we will likely get link errors on the chrome side until the shimming magic has landed there On 2012/08/28 15:51:20, Mark Seaborn wrote: > On 2012/08/27 23:00:02, Nick Bray (chromium) wrote: > > This should should be guarded by a temporary #define (NACL_IRT_PPAPI_SHIMS > > or somesuch). > > @Nick/Brad: why did you say this? https://chromiumcodereview.appspot.com/10826171/diff/35001/src/untrusted/irt/... File src/untrusted/irt/irt.h (right): https://chromiumcodereview.appspot.com/10826171/diff/35001/src/untrusted/irt/... src/untrusted/irt/irt.h:149: /* these should be kept in sync, both are using the nacl_irt_ppapihook API */ On 2012/08/28 15:51:20, Mark Seaborn wrote: > If you are going to add comments, please make sure they are well-formatted: > sentences should start with capital letters. The same applies to other comments > in this change. Done. https://chromiumcodereview.appspot.com/10826171/diff/35001/src/untrusted/irt/... src/untrusted/irt/irt.h:151: #define NACL_IRT_PPAPIHOOK_SHIMMED_v0_1 "nacl-irt-ppapihook-shimmed-0.1" This is a public interface, if somebody chooses to use the pnacl toolchain for creating nexes this is what is being used. On 2012/08/28 15:51:20, Mark Seaborn wrote: > So far irt.h only contains public interfaces. I think we should probably keep > it that way, so can you find or create a different header to put this in? https://chromiumcodereview.appspot.com/10826171/diff/35001/src/untrusted/irt/... File src/untrusted/irt/irt_interfaces.c (right): https://chromiumcodereview.appspot.com/10826171/diff/35001/src/untrusted/irt/... src/untrusted/irt/irt_interfaces.c:36: #ifdef DISABLED_WHILE_IN_TRANSITION_TO_CHROME_TREE commented this out for now using "#if 0" and added a comment On 2012/08/28 15:51:20, Mark Seaborn wrote: > Why is this #ifdef here? No-one defines this symbol. Plus it looks like this > is the wrong way around and should be an #ifndef. Please remove the #ifdef. https://chromiumcodereview.appspot.com/10826171/diff/35001/src/untrusted/irt/... src/untrusted/irt/irt_interfaces.c:48: void *table, On 2012/08/28 15:51:20, Mark Seaborn wrote: > The formatting change to this function is unnecessary, so please undo it. Done. https://chromiumcodereview.appspot.com/10826171/diff/35001/src/untrusted/irt/... File src/untrusted/irt/irt_interfaces.h (right): https://chromiumcodereview.appspot.com/10826171/diff/35001/src/untrusted/irt/... src/untrusted/irt/irt_interfaces.h:2: * Copyright 2012 The Native Client Authors. All rights reserved. On 2012/08/28 15:51:20, Mark Seaborn wrote: > Why are you changing the copyright messages? The presubmit checks accept the > "(c)" part so you don't need to remove it. Same for other files. There was an email a couple of weeks ago "(c)"will be dropped from the copyright boiler plate undoing this for now https://chromiumcodereview.appspot.com/10826171/diff/35001/src/untrusted/irt/... File src/untrusted/irt/irt_ppapi.c (right): https://chromiumcodereview.appspot.com/10826171/diff/35001/src/untrusted/irt/... src/untrusted/irt/irt_ppapi.c:58: * pnacl has calling conventions which are slightly different from nacl-gcc. On 2012/08/28 15:51:20, Mark Seaborn wrote: > 'pnacl' -> 'PNaCl' Done. https://chromiumcodereview.appspot.com/10826171/diff/35001/src/untrusted/irt/... src/untrusted/irt/irt_ppapi.c:59: * This affects structure passing on x86-64 which is only used by Ppapi On 2012/08/28 15:51:20, Mark Seaborn wrote: > 'Ppapi' -> 'PPAPI' Done. https://chromiumcodereview.appspot.com/10826171/diff/35001/src/untrusted/irt/... src/untrusted/irt/irt_ppapi.c:63: * We interecept those functions, store the old version and then redirect On 2012/08/28 15:51:20, Mark Seaborn wrote: > "intercept" Done. https://chromiumcodereview.appspot.com/10826171/diff/35001/src/untrusted/irt/... src/untrusted/irt/irt_ppapi.c:68: struct PP_StartFunctions g_pp_functions_non_shim; On 2012/08/28 15:51:20, Mark Seaborn wrote: > This should be static, because it's only used within one file. Done. https://chromiumcodereview.appspot.com/10826171/diff/35001/src/untrusted/irt/... src/untrusted/irt/irt_ppapi.c:71: PPB_GetInterface get_browser_intf) { On 2012/08/28 15:51:20, Mark Seaborn wrote: > Please use the full name: "get_browser_interface", no need to abbreviate. Done. https://chromiumcodereview.appspot.com/10826171/diff/35001/src/untrusted/irt/... src/untrusted/irt/irt_ppapi.c:76: module_id, &__Pnacl_PPBGetInterface); On 2012/08/28 15:51:20, Mark Seaborn wrote: > Nit: the usual style is to indent arguments by 4 spaces rather than 2 Done. https://chromiumcodereview.appspot.com/10826171/diff/35001/src/untrusted/irt/... src/untrusted/irt/irt_ppapi.c:80: /* redirect to non-shimed version */ On 2012/08/28 15:51:20, Mark Seaborn wrote: > "shimmed" Done. https://chromiumcodereview.appspot.com/10826171/diff/35001/src/untrusted/irt/... src/untrusted/irt/irt_ppapi.c:90: &PPP_InitializeModule_cc_shim, On 2012/08/28 15:51:20, Mark Seaborn wrote: > We don't use the redundant "&" for getting function pointers elsewhere in the > file, so please don't use it here. Done. https://chromiumcodereview.appspot.com/10826171/diff/35001/src/untrusted/irt/... src/untrusted/irt/irt_ppapi.c:97: * to chrome. On 2012/08/28 15:51:20, Mark Seaborn wrote: > 'chrome' -> 'Chrome' Done. https://chromiumcodereview.appspot.com/10826171/diff/35001/src/untrusted/irt/... src/untrusted/irt/irt_ppapi.c:98: * We need to shim all these functions and also do some extra bookeeping emphasized the word *also* On 2012/08/28 15:51:20, Mark Seaborn wrote: > Isn't this repeating what you said in the earlier comment? The earlier comment > should be enough. https://chromiumcodereview.appspot.com/10826171/diff/35001/src/untrusted/irt/... src/untrusted/irt/irt_ppapi.c:108: return PpapiPluginMain(); On 2012/08/28 15:51:20, Mark Seaborn wrote: > Rather than duplicating code from irt_ppapi_start() above, please just call > irt_ppapi_start() here. Done. https://chromiumcodereview.appspot.com/10826171/diff/35001/src/untrusted/irt/... File src/untrusted/irt/nacl.scons (right): https://chromiumcodereview.appspot.com/10826171/diff/35001/src/untrusted/irt/... src/untrusted/irt/nacl.scons:26: def GetPNaClShimSource(env): On 2012/08/28 15:51:20, Mark Seaborn wrote: > Please put this lower in the file, just before LinkIrt(), because it's only an > ancillary part of the IRT. Done. https://chromiumcodereview.appspot.com/10826171/diff/35001/src/untrusted/irt/... src/untrusted/irt/nacl.scons:31: # api code On 2012/08/28 15:51:20, Mark Seaborn wrote: > Capitalise: "API code" Done. https://chromiumcodereview.appspot.com/10826171/diff/35001/src/untrusted/irt/... src/untrusted/irt/nacl.scons:35: # python code On 2012/08/28 15:51:20, Mark Seaborn wrote: > Capitalise as "Python", please Done. https://chromiumcodereview.appspot.com/10826171/diff/35001/src/untrusted/irt/... src/untrusted/irt/nacl.scons:38: 'pnacl_irt_shim.c', On 2012/08/28 15:51:20, Mark Seaborn wrote: > Nit: our Python style is that arguments are indented by 4 spaces, not 2 Done. https://chromiumcodereview.appspot.com/10826171/diff/35001/src/untrusted/irt/... src/untrusted/irt/nacl.scons:45: irt_shim_obj = blob_env.ComponentObject(GetPNaClShimSource(blob_env)) IIRC there was some strange python issue where auto-generated code was not picked up correctly unless we used ComponentObject. So I'd rather leave it this way to be on the safe side. On 2012/08/28 15:51:20, Mark Seaborn wrote: > I don't think you need ComponentObject() here, do you? I think you can just > pass the .c file to ComponentProgram(). https://chromiumcodereview.appspot.com/10826171/diff/35001/src/untrusted/irt/... File src/untrusted/irt/shim_dummy.c (right): https://chromiumcodereview.appspot.com/10826171/diff/35001/src/untrusted/irt/... src/untrusted/irt/shim_dummy.c:1: /* Copyright 2012 The Chromium Authors. All rights reserved. On 2012/08/28 15:51:20, Mark Seaborn wrote: > If this is in the NaCl tree, this should say "The Native Client Authors". Also, > this boilerplate should start with "/*" on a single line. Done. https://chromiumcodereview.appspot.com/10826171/diff/35001/src/untrusted/irt/... src/untrusted/irt/shim_dummy.c:7: * This module satifies link dependencies in case we do not link On 2012/08/28 15:51:20, Mark Seaborn wrote: > "satisfies" > > Saying "in case we do not link in shimming support" is vague. Why wouldn't > shimming support be linked in? Please elaborate. Done. https://chromiumcodereview.appspot.com/10826171/diff/35001/src/untrusted/irt/... src/untrusted/irt/shim_dummy.c:9: * It also rough documents the API of the automatically generated On 2012/08/28 15:51:20, Mark Seaborn wrote: > "roughly" Done.
https://chromiumcodereview.appspot.com/10826171/diff/35001/src/untrusted/irt/... File src/untrusted/irt/irt.h (right): https://chromiumcodereview.appspot.com/10826171/diff/35001/src/untrusted/irt/... src/untrusted/irt/irt.h:151: #define NACL_IRT_PPAPIHOOK_SHIMMED_v0_1 "nacl-irt-ppapihook-shimmed-0.1" On 2012/08/28 18:13:30, Robert Muth (chromium) wrote: > On 2012/08/28 15:51:20, Mark Seaborn wrote: > > So far irt.h only contains public interfaces. I think we should probably > > keep it that way, so can you find or create a different header to put > > this in? > > This is a public interface, if somebody chooses to use > the pnacl toolchain for creating nexes this is what is being used. That's not my and Roland's understanding. We are not committing to supporting more than one x86-64 calling convention ABI for NaCl. If someone wants to use the PNaCl toolchain to build an ABI-stable x86-64 nexe, they will have to link the shims into the nexe (the original approach), so that the nexe uses the original x86-64 ABI when interacting with the outside world. https://chromiumcodereview.appspot.com/10826171/diff/35001/src/untrusted/irt/... File src/untrusted/irt/irt_interfaces.c (right): https://chromiumcodereview.appspot.com/10826171/diff/35001/src/untrusted/irt/... src/untrusted/irt/irt_interfaces.c:36: #ifdef DISABLED_WHILE_IN_TRANSITION_TO_CHROME_TREE On 2012/08/28 18:13:30, Robert Muth (chromium) wrote: > On 2012/08/28 15:51:20, Mark Seaborn wrote: > > Why is this #ifdef here? No-one defines this symbol. Plus it looks > > like this is the wrong way around and should be an #ifndef. Please > > remove the #ifdef. > > commented this out for now using "#if 0" > and added a comment Doing "#if 0" rather defeats the purpose of the change. Brad N has since explained to me that he wanted an #ifdef so that this change doesn't break the Chromium build (so that we can commit the Chromium-side Gyp changes later). So please put an #ifdef in, but you will need to actually define the symbol the #ifdef references somewhere in the Scons build. https://chromiumcodereview.appspot.com/10826171/diff/40009/src/untrusted/irt/... File src/untrusted/irt/irt_interfaces.c (right): https://chromiumcodereview.appspot.com/10826171/diff/40009/src/untrusted/irt/... src/untrusted/irt/irt_interfaces.c:50: for (int i = 0; i < NACL_ARRAY_SIZE(irt_interfaces); ++i) { The formatting change to this function is unnecessary, so please undo it. https://chromiumcodereview.appspot.com/10826171/diff/40009/src/untrusted/irt/... File src/untrusted/irt/irt_ppapi.c (right): https://chromiumcodereview.appspot.com/10826171/diff/40009/src/untrusted/irt/... src/untrusted/irt/irt_ppapi.c:81: /* redirect to non-shimmed version */ My earlier comment that sentences should start with capital letters applies to all the comments. Please go through and fix them all! https://chromiumcodereview.appspot.com/10826171/diff/40009/src/untrusted/irt/... src/untrusted/irt/irt_ppapi.c:107: return irt_ppapi_start(g_pp_functions_cc_shim); This line has a compile failure https://chromiumcodereview.appspot.com/10826171/diff/40009/src/untrusted/irt/... File src/untrusted/irt/shim_dummy.c (right): https://chromiumcodereview.appspot.com/10826171/diff/40009/src/untrusted/irt/... src/untrusted/irt/shim_dummy.c:9: * the x86-64 specific auto-genenerated shimming code into the irt. "auto-generated". "irt" -> "IRT" https://chromiumcodereview.appspot.com/10826171/diff/40009/src/untrusted/irt/... src/untrusted/irt/shim_dummy.c:12: * into a any nexe (not just x86-64) the nexe should work with an "into any nexe" https://chromiumcodereview.appspot.com/10826171/diff/40009/src/untrusted/irt/... src/untrusted/irt/shim_dummy.c:13: * irt containing this code. "irt" -> "IRT"
PTAL https://chromiumcodereview.appspot.com/10826171/diff/35001/src/untrusted/irt/... File src/untrusted/irt/irt.h (right): https://chromiumcodereview.appspot.com/10826171/diff/35001/src/untrusted/irt/... src/untrusted/irt/irt.h:151: #define NACL_IRT_PPAPIHOOK_SHIMMED_v0_1 "nacl-irt-ppapihook-shimmed-0.1" On 2012/08/28 18:57:06, Mark Seaborn wrote: > On 2012/08/28 18:13:30, Robert Muth (chromium) wrote: > > On 2012/08/28 15:51:20, Mark Seaborn wrote: > > > So far irt.h only contains public interfaces. I think we should probably > > > keep it that way, so can you find or create a different header to put > > > this in? > > > > This is a public interface, if somebody chooses to use > > the pnacl toolchain for creating nexes this is what is being used. > > That's not my and Roland's understanding. We are not committing to supporting > more than one x86-64 calling convention ABI for NaCl. If someone wants to use > the PNaCl toolchain to build an ABI-stable x86-64 nexe, they will have to link > the shims into the nexe (the original approach), so that the nexe uses the > original x86-64 ABI when interacting with the outside world. Done. https://chromiumcodereview.appspot.com/10826171/diff/35001/src/untrusted/irt/... File src/untrusted/irt/irt_interfaces.c (right): https://chromiumcodereview.appspot.com/10826171/diff/35001/src/untrusted/irt/... src/untrusted/irt/irt_interfaces.c:36: #ifdef DISABLED_WHILE_IN_TRANSITION_TO_CHROME_TREE On 2012/08/28 18:57:06, Mark Seaborn wrote: > On 2012/08/28 18:13:30, Robert Muth (chromium) wrote: > > On 2012/08/28 15:51:20, Mark Seaborn wrote: > > > Why is this #ifdef here? No-one defines this symbol. Plus it looks > > > like this is the wrong way around and should be an #ifndef. Please > > > remove the #ifdef. > > > > commented this out for now using "#if 0" > > and added a comment > > Doing "#if 0" rather defeats the purpose of the change. Brad N has since > explained to me that he wanted an #ifdef so that this change doesn't break the > Chromium build (so that we can commit the Chromium-side Gyp changes later). > > So please put an #ifdef in, but you will need to actually define the symbol the > #ifdef references somewhere in the Scons build. Done. https://chromiumcodereview.appspot.com/10826171/diff/40009/src/untrusted/irt/... File src/untrusted/irt/irt_interfaces.c (right): https://chromiumcodereview.appspot.com/10826171/diff/40009/src/untrusted/irt/... src/untrusted/irt/irt_interfaces.c:50: for (int i = 0; i < NACL_ARRAY_SIZE(irt_interfaces); ++i) { On 2012/08/28 18:57:07, Mark Seaborn wrote: > The formatting change to this function is unnecessary, so please undo it. Done. https://chromiumcodereview.appspot.com/10826171/diff/40009/src/untrusted/irt/... File src/untrusted/irt/irt_ppapi.c (right): https://chromiumcodereview.appspot.com/10826171/diff/40009/src/untrusted/irt/... src/untrusted/irt/irt_ppapi.c:81: /* redirect to non-shimmed version */ On 2012/08/28 18:57:07, Mark Seaborn wrote: > My earlier comment that sentences should start with capital letters applies to > all the comments. Please go through and fix them all! Done. https://chromiumcodereview.appspot.com/10826171/diff/40009/src/untrusted/irt/... src/untrusted/irt/irt_ppapi.c:107: return irt_ppapi_start(g_pp_functions_cc_shim); On 2012/08/28 18:57:07, Mark Seaborn wrote: > This line has a compile failure Done.
https://chromiumcodereview.appspot.com/10826171/diff/35025/src/untrusted/irt/... File src/untrusted/irt/irt_interfaces.c (right): https://chromiumcodereview.appspot.com/10826171/diff/35025/src/untrusted/irt/... src/untrusted/irt/irt_interfaces.c:34: /* @IGNORE_LINES_FOR_CODE_HYGIENE[1] */ Please indent by 2 spaces to match the surrounding code https://chromiumcodereview.appspot.com/10826171/diff/35025/src/untrusted/irt/... src/untrusted/irt/irt_interfaces.c:37: /* This ifdef should be removed once the IRT build has migrated to the Chrome tree. */ Please add a TODO(robertm) in this comment. Also, this line is >80 chars long. Also, the comment would be more accurate if it said "once Chromium's Gyp build has been extended to build the shims". https://chromiumcodereview.appspot.com/10826171/diff/35025/src/untrusted/irt/... File src/untrusted/irt/irt_ppapi.c (right): https://chromiumcodereview.appspot.com/10826171/diff/35025/src/untrusted/irt/... src/untrusted/irt/irt_ppapi.c:17: /* register entry points to untrusted pepper plugin */ Please capitalise the first letter, or otherwise drop the comment. https://chromiumcodereview.appspot.com/10826171/diff/35025/src/untrusted/irt/... src/untrusted/irt/irt_ppapi.c:52: * PNaCl Shimming magic Please capitalise consistently, i.e. "PNaCl Shimming Magic" or "PNaCl shimming magic" https://chromiumcodereview.appspot.com/10826171/diff/35025/src/untrusted/irt/... src/untrusted/irt/irt_ppapi.c:71: PP_Module module_id, These arguments should be indented by 4 spaces not 2, so that they don't disappear into the function body. https://chromiumcodereview.appspot.com/10826171/diff/35025/src/untrusted/irt/... src/untrusted/irt/irt_ppapi.c:72: PPB_GetInterface get_browser_intface) { "intface" -> "interface" https://chromiumcodereview.appspot.com/10826171/diff/35025/src/untrusted/irt/... src/untrusted/irt/irt_ppapi.c:99: * We need to shim all these functions and *also* do some extra bookeeping "bookkeeping" https://chromiumcodereview.appspot.com/10826171/diff/35025/src/untrusted/irt/... File src/untrusted/irt/irt_shim.h (right): https://chromiumcodereview.appspot.com/10826171/diff/35025/src/untrusted/irt/... src/untrusted/irt/irt_shim.h:10: * This should be kept in sync with NACL_IRT_PPAPIHOOK_v0_1. There's 2 spaces after 'with'; just use 1. https://chromiumcodereview.appspot.com/10826171/diff/35025/src/untrusted/irt/... src/untrusted/irt/irt_shim.h:14: #define NACL_IRT_PPAPIHOOK_SHIMMED_v0_1 "nacl-irt-ppapihook-shimmed-0.1" Please add a comment saying this is a private interface for internal use in Chromium by code linked in at PNaCl translation time. https://chromiumcodereview.appspot.com/10826171/diff/35025/src/untrusted/irt/... File src/untrusted/irt/nacl.scons (right): https://chromiumcodereview.appspot.com/10826171/diff/35025/src/untrusted/irt/... src/untrusted/irt/nacl.scons:16: # This ifdef should be removed once Please add a TODO(robertm) in this comment. Also, "This ifdef" -> "This #define"? (Since there's no #ifdef in this file.) https://chromiumcodereview.appspot.com/10826171/diff/35025/src/untrusted/irt/... src/untrusted/irt/nacl.scons:18: CPPDEFINES = ['ENABLE_NACL_IRT_PPAPIHOOK_SHIMMED'] Nit: Python style is no spaces around '=' for keyword arguments. https://chromiumcodereview.appspot.com/10826171/diff/35025/src/untrusted/irt/... src/untrusted/irt/nacl.scons:19: ) Normal Python style is that the closing bracket can go on the previous line.
I escalated the NACL_IRT_PPAPIHOOK_SHIMMED_v0_1 to sehr, or PTAL On 2012/08/28 20:41:00, Mark Seaborn wrote: > https://chromiumcodereview.appspot.com/10826171/diff/35025/src/untrusted/irt/... > File src/untrusted/irt/irt_interfaces.c (right): > > https://chromiumcodereview.appspot.com/10826171/diff/35025/src/untrusted/irt/... > src/untrusted/irt/irt_interfaces.c:34: /* @IGNORE_LINES_FOR_CODE_HYGIENE[1] */ > Please indent by 2 spaces to match the surrounding code > > https://chromiumcodereview.appspot.com/10826171/diff/35025/src/untrusted/irt/... > src/untrusted/irt/irt_interfaces.c:37: /* This ifdef should be removed once the > IRT build has migrated to the Chrome tree. */ > Please add a TODO(robertm) in this comment. Also, this line is >80 chars long. > Also, the comment would be more accurate if it said "once Chromium's Gyp build > has been extended to build the shims". > > https://chromiumcodereview.appspot.com/10826171/diff/35025/src/untrusted/irt/... > File src/untrusted/irt/irt_ppapi.c (right): > > https://chromiumcodereview.appspot.com/10826171/diff/35025/src/untrusted/irt/... > src/untrusted/irt/irt_ppapi.c:17: /* register entry points to untrusted pepper > plugin */ > Please capitalise the first letter, or otherwise drop the comment. > > https://chromiumcodereview.appspot.com/10826171/diff/35025/src/untrusted/irt/... > src/untrusted/irt/irt_ppapi.c:52: * PNaCl Shimming magic > Please capitalise consistently, i.e. "PNaCl Shimming Magic" or "PNaCl shimming > magic" > > https://chromiumcodereview.appspot.com/10826171/diff/35025/src/untrusted/irt/... > src/untrusted/irt/irt_ppapi.c:71: PP_Module module_id, > These arguments should be indented by 4 spaces not 2, so that they don't > disappear into the function body. > > https://chromiumcodereview.appspot.com/10826171/diff/35025/src/untrusted/irt/... > src/untrusted/irt/irt_ppapi.c:72: PPB_GetInterface get_browser_intface) { > "intface" -> "interface" > > https://chromiumcodereview.appspot.com/10826171/diff/35025/src/untrusted/irt/... > src/untrusted/irt/irt_ppapi.c:99: * We need to shim all these functions and > *also* do some extra bookeeping > "bookkeeping" > > https://chromiumcodereview.appspot.com/10826171/diff/35025/src/untrusted/irt/... > File src/untrusted/irt/irt_shim.h (right): > > https://chromiumcodereview.appspot.com/10826171/diff/35025/src/untrusted/irt/... > src/untrusted/irt/irt_shim.h:10: * This should be kept in sync with > NACL_IRT_PPAPIHOOK_v0_1. > There's 2 spaces after 'with'; just use 1. > > https://chromiumcodereview.appspot.com/10826171/diff/35025/src/untrusted/irt/... > src/untrusted/irt/irt_shim.h:14: #define NACL_IRT_PPAPIHOOK_SHIMMED_v0_1 > "nacl-irt-ppapihook-shimmed-0.1" > Please add a comment saying this is a private interface for internal use in > Chromium by code linked in at PNaCl translation time. > > https://chromiumcodereview.appspot.com/10826171/diff/35025/src/untrusted/irt/... > File src/untrusted/irt/nacl.scons (right): > > https://chromiumcodereview.appspot.com/10826171/diff/35025/src/untrusted/irt/... > src/untrusted/irt/nacl.scons:16: # This ifdef should be removed once > Please add a TODO(robertm) in this comment. Also, "This ifdef" -> "This > #define"? (Since there's no #ifdef in this file.) > > https://chromiumcodereview.appspot.com/10826171/diff/35025/src/untrusted/irt/... > src/untrusted/irt/nacl.scons:18: CPPDEFINES = > ['ENABLE_NACL_IRT_PPAPIHOOK_SHIMMED'] > Nit: Python style is no spaces around '=' for keyword arguments. > > https://chromiumcodereview.appspot.com/10826171/diff/35025/src/untrusted/irt/... > src/untrusted/irt/nacl.scons:19: ) > Normal Python style is that the closing bracket can go on the previous line.
https://chromiumcodereview.appspot.com/10826171/diff/35025/src/untrusted/irt/... File src/untrusted/irt/irt_interfaces.c (right): https://chromiumcodereview.appspot.com/10826171/diff/35025/src/untrusted/irt/... src/untrusted/irt/irt_interfaces.c:34: /* @IGNORE_LINES_FOR_CODE_HYGIENE[1] */ On 2012/08/28 20:41:00, Mark Seaborn wrote: > Please indent by 2 spaces to match the surrounding code Ping? Please do this for the comment below too. https://chromiumcodereview.appspot.com/10826171/diff/35025/src/untrusted/irt/... File src/untrusted/irt/irt_shim.h (right): https://chromiumcodereview.appspot.com/10826171/diff/35025/src/untrusted/irt/... src/untrusted/irt/irt_shim.h:14: #define NACL_IRT_PPAPIHOOK_SHIMMED_v0_1 "nacl-irt-ppapihook-shimmed-0.1" On 2012/08/28 20:41:00, Mark Seaborn wrote: > Please add a comment saying this is a private interface for internal use > in Chromium by code linked in at PNaCl translation time. Ping -- once you do this, I can OK the change.
quick update: there is some controversy regarding the status of "nacl-irt-ppapihook-shimmed-0.1" and whether it is ok to use this interface from a regular nexe. This will hopefully be resolved offline soon.
We decided to simply migrate the exisiting shim build to the chrome tree and add a shim dummy to the pnacl toolchain build which later will get overwritten by the sdk creation process with the real shim. On 2012/08/28 23:16:01, Robert Muth (chromium) wrote: > quick update: > > there is some controversy regarding the > status of "nacl-irt-ppapihook-shimmed-0.1" > and whether it is ok to use this interface > from a regular nexe. > > This will hopefully be resolved offline soon. |
