Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
9838005:
Support for ARM NaCl untrusted runtime build. (Closed)
|
Created:
8 years, 9 months ago by Nikolay Modified:
8 years, 8 months ago Reviewers:
noelallen_use_chromium, dmichael (off chromium), robertm, jvoung - send to chromium..., Roland McGrath, Mark Seaborn, davidjames CC:
native-client-reviews_googlegroups.com Base URL:
http://git.chromium.org/chromium/src.git@master Visibility:
Public. |
DescriptionSupport for ARM NaCl untrusted runtime build.
This one depends on https://chromiumcodereview.appspot.com/9816003/
on NaCl side and indeed produces working untrusted runtime.
BUG= http://code.google.com/p/nativeclient/issues/detail?id=2687
TEST=Compile for ARM with "GYP_DEFINES="target_arch=arm sysroot=~/rootfs/" and run DOSBox with produced untrusted runtime.
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=130386
Patch Set 1 #
Total comments: 5
Patch Set 2 : Added toolchain download. #Patch Set 3 : Rename #
Total comments: 6
Patch Set 4 : Added comment. #
Total comments: 4
Patch Set 5 : Review feedback. #
Total comments: 3
Patch Set 6 : Typo fixed #Patch Set 7 : Remove excessive check for CrOs #
Total comments: 13
Patch Set 8 : Review feedback. #
Total comments: 3
Patch Set 9 : Remove pthread deps. #
Total comments: 4
Patch Set 10 : Removed hack. #Patch Set 11 : Hopefully better missed deps fix. #
Total comments: 2
Patch Set 12 : Split on PPAPI and Chromium part. #Patch Set 13 : Back to Chrome #
Messages
Total messages: 30 (0 generated)
This change is for the ppapi side, and is complimentary to https://chromiumcodereview.appspot.com/9816003/, so send to same reviewers set.
https://chromiumcodereview.appspot.com/9838005/diff/1/ppapi/native_client/nat... File ppapi/native_client/native_client.gyp (right): https://chromiumcodereview.appspot.com/9838005/diff/1/ppapi/native_client/nat... ppapi/native_client/native_client.gyp:90: 'extra_deps64': [ it would be nice if those could be factored. but that may not be possible in which case it would be nice to add some comments. E.g. it maybe the case that we need to build extra_deps32 and extra_deps64 during one and the same chrome build so we cannot just call them "extra_deps" https://chromiumcodereview.appspot.com/9838005/diff/1/ppapi/ppapi_untrusted.gyp File ppapi/ppapi_untrusted.gyp (right): https://chromiumcodereview.appspot.com/9838005/diff/1/ppapi/ppapi_untrusted.g... ppapi/ppapi_untrusted.gyp:53: '<(SHARED_INTERMEDIATE_DIR)/tc_newlib/lib64/libppapi_cpp.a', it might be nice to factor the libXXX part somehow
Variables renamed, fooarm -> foo_arm. PTAL. https://chromiumcodereview.appspot.com/9838005/diff/1/ppapi/ppapi_untrusted.gyp File ppapi/ppapi_untrusted.gyp (right): https://chromiumcodereview.appspot.com/9838005/diff/1/ppapi/ppapi_untrusted.g... ppapi/ppapi_untrusted.gyp:53: '<(SHARED_INTERMEDIATE_DIR)/tc_newlib/lib64/libppapi_cpp.a', Agree, but unsure how to do that in .gyp :(. Do you know?
Adding noel to get some feedback on the pnacl/arm TC download and jvoung about the -mtls-use-call stuff https://chromiumcodereview.appspot.com/9838005/diff/1/ppapi/ppapi_untrusted.gyp File ppapi/ppapi_untrusted.gyp (right): https://chromiumcodereview.appspot.com/9838005/diff/1/ppapi/ppapi_untrusted.g... ppapi/ppapi_untrusted.gyp:1: # Copyright (c) 2011 The Chromium Authors. All rights reserved. date https://chromiumcodereview.appspot.com/9838005/diff/1/ppapi/ppapi_untrusted.g... ppapi/ppapi_untrusted.gyp:53: '<(SHARED_INTERMEDIATE_DIR)/tc_newlib/lib64/libppapi_cpp.a', you could declare a new variable 'archlib' and set it to lib64, lib32, libarm accordingly but this makes the code more magical so lets not do this for now https://chromiumcodereview.appspot.com/9838005/diff/6001/build/download_nacl_... File build/download_nacl_toolchains.py (right): https://chromiumcodereview.appspot.com/9838005/diff/6001/build/download_nacl_... build/download_nacl_toolchains.py:38: use_pnacl = False I am not sure whether we should hook into the optional-pnacl hook. I believe this was meant for the SDK bots only. I'll add noealallan to the reviewer list. In any case, the comment should probably be augmented to reflect the extra magic. https://chromiumcodereview.appspot.com/9838005/diff/6001/ppapi/native_client/... File ppapi/native_client/native_client.gyp (right): https://chromiumcodereview.appspot.com/9838005/diff/6001/ppapi/native_client/... ppapi/native_client/native_client.gyp:75: '-Wl,-Ttext=<(NACL_IRT_TEXT_START)', it would be nice to see whether arm would would work with -Wl,-Ttext-segment=<(NACL_IRT_TEXT_START)' so that the x86 and arm version are more similar https://chromiumcodereview.appspot.com/9838005/diff/6001/ppapi/native_client/... ppapi/native_client/native_client.gyp:78: '-Wt,-mtls-use-call', Can you double check the -Wt,-mtls-use-call setting I think I saw the opposite in another CL where we were uisng '-Wt,-mtls-use-call' for x86 but not for arm. I will also ping jvoung about this
PTAL. https://chromiumcodereview.appspot.com/9838005/diff/6001/build/download_nacl_... File build/download_nacl_toolchains.py (right): https://chromiumcodereview.appspot.com/9838005/diff/6001/build/download_nacl_... build/download_nacl_toolchains.py:38: use_pnacl = False Well, from what I see regular gclient runhooks use "--optional-pnacl" switch. I just wanted minimal changes, but can move it up, if you believe it makes sense. Comment added. On 2012/03/26 14:30:44, robertm wrote: > I am not sure whether we should hook into the optional-pnacl > hook. I believe this was meant for the SDK bots only. > > I'll add noealallan to the reviewer list. > > In any case, the comment should probably be augmented to > reflect the extra magic. https://chromiumcodereview.appspot.com/9838005/diff/6001/ppapi/native_client/... File ppapi/native_client/native_client.gyp (right): https://chromiumcodereview.appspot.com/9838005/diff/6001/ppapi/native_client/... ppapi/native_client/native_client.gyp:75: '-Wl,-Ttext=<(NACL_IRT_TEXT_START)', See comment in native_client/src/untrusted/irt/nacl.scons regarding this switch. It's ugly, and indeed better be unified. On 2012/03/26 14:30:44, robertm wrote: > it would be nice to see whether arm would would work with > -Wl,-Ttext-segment=<(NACL_IRT_TEXT_START)' > > so that the x86 and arm version are more similar https://chromiumcodereview.appspot.com/9838005/diff/6001/ppapi/native_client/... ppapi/native_client/native_client.gyp:78: '-Wt,-mtls-use-call', Hmm, I tried to model after scons On 2012/03/26 14:30:44, robertm wrote: > Can you double check the -Wt,-mtls-use-call setting > I think I saw the opposite in another CL where we were uisng > '-Wt,-mtls-use-call' for x86 but not for arm. > > I will also ping jvoung about this
LGTM On 2012/03/26 14:59:55, olonho wrote: > PTAL. > > https://chromiumcodereview.appspot.com/9838005/diff/6001/build/download_nacl_... > File build/download_nacl_toolchains.py (right): > > https://chromiumcodereview.appspot.com/9838005/diff/6001/build/download_nacl_... > build/download_nacl_toolchains.py:38: use_pnacl = False > Well, from what I see regular gclient runhooks use > "--optional-pnacl" switch. I just wanted minimal changes, > but can move it up, if you believe it makes sense. > Comment added. > > On 2012/03/26 14:30:44, robertm wrote: > > I am not sure whether we should hook into the optional-pnacl > > hook. I believe this was meant for the SDK bots only. > > > > I'll add noealallan to the reviewer list. > > > > In any case, the comment should probably be augmented to > > reflect the extra magic. > > https://chromiumcodereview.appspot.com/9838005/diff/6001/ppapi/native_client/... > File ppapi/native_client/native_client.gyp (right): > > https://chromiumcodereview.appspot.com/9838005/diff/6001/ppapi/native_client/... > ppapi/native_client/native_client.gyp:75: '-Wl,-Ttext=<(NACL_IRT_TEXT_START)', > See comment in native_client/src/untrusted/irt/nacl.scons regarding this switch. > It's ugly, and indeed better be unified. > > On 2012/03/26 14:30:44, robertm wrote: > > it would be nice to see whether arm would would work with > > -Wl,-Ttext-segment=<(NACL_IRT_TEXT_START)' > > > > so that the x86 and arm version are more similar > > https://chromiumcodereview.appspot.com/9838005/diff/6001/ppapi/native_client/... > ppapi/native_client/native_client.gyp:78: '-Wt,-mtls-use-call', > Hmm, I tried to model after scons > > On 2012/03/26 14:30:44, robertm wrote: > > Can you double check the -Wt,-mtls-use-call setting > > I think I saw the opposite in another CL where we were uisng > > '-Wt,-mtls-use-call' for x86 but not for arm. > > > > I will also ping jvoung about this
https://chromiumcodereview.appspot.com/9838005/diff/7005/ppapi/native_client/... File ppapi/native_client/native_client.gyp (right): https://chromiumcodereview.appspot.com/9838005/diff/7005/ppapi/native_client/... ppapi/native_client/native_client.gyp:69: '-Wl,--section-start,.rodata=<(NACL_IRT_DATA_START)', can you copy the comments for the spcecial casing from the scons file. I'll try to fix the text-segment vs text problem on the nacl side. You can also reference this bug here: http://code.google.com/p/nativeclient/issues/detail?id=2691 https://chromiumcodereview.appspot.com/9838005/diff/7005/ppapi/native_client/... ppapi/native_client/native_client.gyp:113: '<(SHARED_INTERMEDIATE_DIR)/tc_newlib/libarm/libpthread.a', if there is an explanation somewhere why arm need libpthread.a please add a comment ow just add a todo for a comment
Uploaded final version. https://chromiumcodereview.appspot.com/9838005/diff/7005/ppapi/native_client/... File ppapi/native_client/native_client.gyp (right): https://chromiumcodereview.appspot.com/9838005/diff/7005/ppapi/native_client/... ppapi/native_client/native_client.gyp:69: '-Wl,--section-start,.rodata=<(NACL_IRT_DATA_START)', On 2012/03/26 15:15:34, robertm wrote: > can you copy the comments for the spcecial casing from the scons file. > > I'll try to fix the text-segment vs text problem on the nacl side. > > You can also reference this bug here: > http://code.google.com/p/nativeclient/issues/detail?id=2691 Done. https://chromiumcodereview.appspot.com/9838005/diff/7005/ppapi/native_client/... ppapi/native_client/native_client.gyp:113: '<(SHARED_INTERMEDIATE_DIR)/tc_newlib/libarm/libpthread.a', Hmm, rechecked and it seems not really needed. So will remove this change. Thanks! On 2012/03/26 15:15:34, robertm wrote: > if there is an explanation somewhere why arm need libpthread.a > please add a comment ow just add a todo for a comment
Use of -Wt,-mtls-use-call is good. If you want to double check, you can objdump
the generated IRT and grep for a lack of direct references to "r9".
One important typo below ("use_pncal") and other typos.
https://chromiumcodereview.appspot.com/9838005/diff/8003/build/download_nacl_...
File build/download_nacl_toolchains.py (right):
https://chromiumcodereview.appspot.com/9838005/diff/8003/build/download_nacl_...
build/download_nacl_toolchains.py:38: # By default we don't use pNaCl toolchain
yet, unless on ARM, where
s/pNaCl/PNaCl/
https://chromiumcodereview.appspot.com/9838005/diff/8003/build/download_nacl_...
build/download_nacl_toolchains.py:40: # So analyze is we're building for ARM, or
on SDK buildbot.
s/analyze is/analyze if/
https://chromiumcodereview.appspot.com/9838005/diff/8003/build/download_nacl_...
build/download_nacl_toolchains.py:46: use_pncal = True
s/use_pncal/use_pnacl/
Could you PTAL, as I updated common.gypi (for CrOs case). Also included David James to reviewers list.
https://chromiumcodereview.appspot.com/9838005/diff/14001/build/common.gypi File build/common.gypi (right): https://chromiumcodereview.appspot.com/9838005/diff/14001/build/common.gypi#n... build/common.gypi:727: # Native Client is enabled by default. Are the chrome-os bots explicitly setting disable_nacl=1 ? I think it would be safer to turn this on in a separate CL since it is higher risk. This way the other changes have some time to "bake".
https://chromiumcodereview.appspot.com/9838005/diff/14001/build/common.gypi File build/common.gypi (right): https://chromiumcodereview.appspot.com/9838005/diff/14001/build/common.gypi#n... build/common.gypi:727: # Native Client is enabled by default. Yes, they do disable nacl in .ebuild files (see use_nacl in third_party/chromiumos-overlay/chromeos-base/chromeos-chrome/chromeos-chrome-9999.ebuild). This change only modifies CrOs build behavior, but I agree that we'd better make it separate CL, to minimize risks. On 2012/03/28 14:22:14, robertm wrote: > Are the chrome-os bots explicitly setting > disable_nacl=1 ? > > I think it would be safer to turn this on in a separate > CL since it is higher risk. This way the other changes > have some time to "bake".
https://chromiumcodereview.appspot.com/9838005/diff/14001/build/download_nacl... File build/download_nacl_toolchains.py (right): https://chromiumcodereview.appspot.com/9838005/diff/14001/build/download_nacl... build/download_nacl_toolchains.py:42: if 'target_arch=arm' in os.environ.get('GYP_DEFINES', ''): Hmm, this is a horrible hack that doesn't work if one wants to supply gyp defines via another means. What would it mean to do this properly? Please put a TODO in at least. Is there a Chromium trybot that covers this? I don't see any trybot results for your change? (To be fair, you were probably waiting on the NaCl side change before you can do try runs.) https://chromiumcodereview.appspot.com/9838005/diff/14001/ppapi/native_client... File ppapi/native_client/native_client.gyp (right): https://chromiumcodereview.appspot.com/9838005/diff/14001/ppapi/native_client... ppapi/native_client/native_client.gyp:67: # option. However, with the linker for x86, the "-Ttext" option does Line too long: please rewrap. https://chromiumcodereview.appspot.com/9838005/diff/14001/ppapi/native_client... ppapi/native_client/native_client.gyp:81: '-arch', 'arm', This is duplicating similar stuff on the NaCl side. At least put in a TODO to do this properly without duplication. build_nexe.py should be supplying "-arch arm". https://chromiumcodereview.appspot.com/9838005/diff/14001/ppapi/native_client... ppapi/native_client/native_client.gyp:125: '../../native_client/src/untrusted/pthread/pthread.gyp:pthread_lib', Why is this added? The IRT does not use the full libpthread. By doing this there might be a danger that you pull in code that doesn't work inside the IRT. Please run a try job.
PTAL. https://chromiumcodereview.appspot.com/9838005/diff/14001/build/download_nacl... File build/download_nacl_toolchains.py (right): https://chromiumcodereview.appspot.com/9838005/diff/14001/build/download_nacl... build/download_nacl_toolchains.py:42: if 'target_arch=arm' in os.environ.get('GYP_DEFINES', ''): Agree, but fortunately this hack also matches the way CrOs build system works, so for most important ARM/NaCl case and for standard way developers configure their build we're OK. For cross-compilation case, it's rather hard to figure out intention of developer, the only other (somewhat) reasonable approach is to perform smth like $CC --version | grep arm. Will add TODO. Regarding trybots - CrOs bots will build NaCl by default, once this whole effort will be over. On 2012/03/28 14:59:24, Mark Seaborn wrote: > Hmm, this is a horrible hack that doesn't work if one wants to supply gyp > defines via another means. What would it mean to do this properly? Please put > a TODO in at least. > > Is there a Chromium trybot that covers this? I don't see any trybot results for > your change? (To be fair, you were probably waiting on the NaCl side change > before you can do try runs.) https://chromiumcodereview.appspot.com/9838005/diff/14001/ppapi/native_client... File ppapi/native_client/native_client.gyp (right): https://chromiumcodereview.appspot.com/9838005/diff/14001/ppapi/native_client... ppapi/native_client/native_client.gyp:67: # option. However, with the linker for x86, the "-Ttext" option does On 2012/03/28 14:59:24, Mark Seaborn wrote: > Line too long: please rewrap. Done. https://chromiumcodereview.appspot.com/9838005/diff/14001/ppapi/native_client... ppapi/native_client/native_client.gyp:81: '-arch', 'arm', From what I understand, here we use trusted linker to link untrusted binaries. So build_nexe.py not used. Not sure if it's good or bad, but this is how gyp files are written. On 2012/03/28 14:59:24, Mark Seaborn wrote: > This is duplicating similar stuff on the NaCl side. At least put in a TODO to > do this properly without duplication. build_nexe.py should be supplying "-arch > arm". https://chromiumcodereview.appspot.com/9838005/diff/14001/ppapi/native_client... ppapi/native_client/native_client.gyp:125: '../../native_client/src/untrusted/pthread/pthread.gyp:pthread_lib', If not do this, following happens: /build/chromium/src/out/Release/obj/gen/tc_newlib/libarm/libplatform.a: error: undefined reference to 'pthread_cond_destroy' /build/chromium/src/out/Release/obj/gen/tc_newlib/libarm/libplatform.a: error: undefined reference to 'pthread_cond_timedwait_abs' /build/chromium/src/out/Release/obj/gen/tc_newlib/libarm/libplatform.a: error: undefined reference to 'pthread_cond_signal' /build/chromium/src/out/Release/obj/gen/tc_newlib/libarm/libplatform.a: error: undefined reference to 'pthread_cond_broadcast' /build/chromium/src/out/Release/obj/gen/tc_newlib/libarm/libplatform.a: error: undefined reference to 'pthread_cond_init' /build/chromium/src/out/Release/obj/gen/tc_newlib/libarm/libplatform.a: error: undefined reference to 'pthread_cond_wait' so I think it is indeed needed. On 2012/03/28 14:59:24, Mark Seaborn wrote: > Why is this added? The IRT does not use the full libpthread. By doing this > there might be a danger that you pull in code that doesn't work inside the IRT. > Please run a try job.
LGTM On 2012/03/29 14:36:06, olonho wrote: > PTAL. > > https://chromiumcodereview.appspot.com/9838005/diff/14001/build/download_nacl... > File build/download_nacl_toolchains.py (right): > > https://chromiumcodereview.appspot.com/9838005/diff/14001/build/download_nacl... > build/download_nacl_toolchains.py:42: if 'target_arch=arm' in > os.environ.get('GYP_DEFINES', ''): > Agree, but fortunately this hack also matches the way CrOs build system works, > so for most important ARM/NaCl case and for standard way developers configure > their build we're OK. > For cross-compilation case, it's rather hard to figure out intention of > developer, the only other (somewhat) reasonable approach is to perform smth like > $CC --version | grep arm. > Will add TODO. > > Regarding trybots - CrOs bots will build NaCl by default, once this whole effort > will be over. > > On 2012/03/28 14:59:24, Mark Seaborn wrote: > > Hmm, this is a horrible hack that doesn't work if one wants to supply gyp > > defines via another means. What would it mean to do this properly? Please > put > > a TODO in at least. > > > > Is there a Chromium trybot that covers this? I don't see any trybot results > for > > your change? (To be fair, you were probably waiting on the NaCl side change > > before you can do try runs.) > > https://chromiumcodereview.appspot.com/9838005/diff/14001/ppapi/native_client... > File ppapi/native_client/native_client.gyp (right): > > https://chromiumcodereview.appspot.com/9838005/diff/14001/ppapi/native_client... > ppapi/native_client/native_client.gyp:67: # option. However, with the linker > for x86, the "-Ttext" option does > On 2012/03/28 14:59:24, Mark Seaborn wrote: > > Line too long: please rewrap. > > Done. > > https://chromiumcodereview.appspot.com/9838005/diff/14001/ppapi/native_client... > ppapi/native_client/native_client.gyp:81: '-arch', 'arm', > From what I understand, here we use trusted linker to link untrusted binaries. > So build_nexe.py not used. Not sure if it's good or bad, but this is how gyp > files are written. > > On 2012/03/28 14:59:24, Mark Seaborn wrote: > > This is duplicating similar stuff on the NaCl side. At least put in a TODO to > > do this properly without duplication. build_nexe.py should be supplying > "-arch > > arm". > > https://chromiumcodereview.appspot.com/9838005/diff/14001/ppapi/native_client... > ppapi/native_client/native_client.gyp:125: > '../../native_client/src/untrusted/pthread/pthread.gyp:pthread_lib', > If not do this, following happens: > /build/chromium/src/out/Release/obj/gen/tc_newlib/libarm/libplatform.a: error: > undefined reference to 'pthread_cond_destroy' > /build/chromium/src/out/Release/obj/gen/tc_newlib/libarm/libplatform.a: error: > undefined reference to 'pthread_cond_timedwait_abs' > /build/chromium/src/out/Release/obj/gen/tc_newlib/libarm/libplatform.a: error: > undefined reference to 'pthread_cond_signal' > /build/chromium/src/out/Release/obj/gen/tc_newlib/libarm/libplatform.a: error: > undefined reference to 'pthread_cond_broadcast' > /build/chromium/src/out/Release/obj/gen/tc_newlib/libarm/libplatform.a: error: > undefined reference to 'pthread_cond_init' > /build/chromium/src/out/Release/obj/gen/tc_newlib/libarm/libplatform.a: error: > undefined reference to 'pthread_cond_wait' > so I think it is indeed needed. > > On 2012/03/28 14:59:24, Mark Seaborn wrote: > > Why is this added? The IRT does not use the full libpthread. By doing this > > there might be a danger that you pull in code that doesn't work inside the > IRT. > > Please run a try job.
https://chromiumcodereview.appspot.com/9838005/diff/21001/ppapi/native_client... File ppapi/native_client/native_client.gyp (right): https://chromiumcodereview.appspot.com/9838005/diff/21001/ppapi/native_client... ppapi/native_client/native_client.gyp:63: '-lpthread', -lpthread should not be here. The IRT compiles its own versions of the pthread routines it uses. If something needs more pthread support than those in irt_private_pthread.c plus the privately-compiled versions of nc_{mutex,condvar,token}.c, then that should be dealt with explicitly rather than by adding -lpthread.
https://chromiumcodereview.appspot.com/9838005/diff/14001/build/download_nacl... File build/download_nacl_toolchains.py (right): https://chromiumcodereview.appspot.com/9838005/diff/14001/build/download_nacl... build/download_nacl_toolchains.py:42: if 'target_arch=arm' in os.environ.get('GYP_DEFINES', ''): On 2012/03/29 14:36:07, olonho wrote: > Agree, but fortunately this hack also matches the way CrOs build system works, > so for most important ARM/NaCl case and for standard way developers configure > their build we're OK. > For cross-compilation case, it's rather hard to figure out intention of > developer, the only other (somewhat) reasonable approach is to perform smth like > $CC --version | grep arm. > Will add TODO. > > Regarding trybots - CrOs bots will build NaCl by default, once this whole effort > will be over. > > On 2012/03/28 14:59:24, Mark Seaborn wrote: > > Hmm, this is a horrible hack that doesn't work if one wants to supply gyp > > defines via another means. What would it mean to do this properly? Please > put > > a TODO in at least. > > > > Is there a Chromium trybot that covers this? I don't see any trybot results > for > > your change? (To be fair, you were probably waiting on the NaCl side change > > before you can do try runs.) What changes need to be made to the CrOS ebuild to make the gyp files clean? Please make those changes and upload them. You don't need to be afraid of patching the chrome ebuild as it is easy :)
No code updates, only comments. https://chromiumcodereview.appspot.com/9838005/diff/14001/build/download_nacl... File build/download_nacl_toolchains.py (right): https://chromiumcodereview.appspot.com/9838005/diff/14001/build/download_nacl... build/download_nacl_toolchains.py:42: if 'target_arch=arm' in os.environ.get('GYP_DEFINES', ''): See https://chromiumcodereview.appspot.com/9923012/ On 2012/03/29 17:30:24, davidjames wrote: > On 2012/03/29 14:36:07, olonho wrote: > > Agree, but fortunately this hack also matches the way CrOs build system works, > > so for most important ARM/NaCl case and for standard way developers configure > > their build we're OK. > > For cross-compilation case, it's rather hard to figure out intention of > > developer, the only other (somewhat) reasonable approach is to perform smth > like > > $CC --version | grep arm. > > Will add TODO. > > > > Regarding trybots - CrOs bots will build NaCl by default, once this whole > effort > > will be over. > > > > On 2012/03/28 14:59:24, Mark Seaborn wrote: > > > Hmm, this is a horrible hack that doesn't work if one wants to supply gyp > > > defines via another means. What would it mean to do this properly? Please > > put > > > a TODO in at least. > > > > > > Is there a Chromium trybot that covers this? I don't see any trybot results > > for > > > your change? (To be fair, you were probably waiting on the NaCl side change > > > before you can do try runs.) > > What changes need to be made to the CrOS ebuild to make the gyp files clean? > Please make those changes and upload them. You don't need to be afraid of > patching the chrome ebuild as it is easy :) https://chromiumcodereview.appspot.com/9838005/diff/21001/ppapi/native_client... File ppapi/native_client/native_client.gyp (right): https://chromiumcodereview.appspot.com/9838005/diff/21001/ppapi/native_client... ppapi/native_client/native_client.gyp:63: '-lpthread', Hmm, I think this issue appears from the fact, that libplatform.a is compiled with trusted toolchain to native code, while libirt_browser.a (which contains pthread functions) contains bitcode, produced by untrusted toolchain. So generally, -lpthread here satisfies dependencies of trusted code only in libplatform.a. What are alternatives you suggest? On 2012/03/29 16:47:15, Roland McGrath wrote: > -lpthread should not be here. > The IRT compiles its own versions of the pthread routines it uses. If something > needs more pthread support than those in irt_private_pthread.c plus the > privately-compiled versions of nc_{mutex,condvar,token}.c, then that should be > dealt with explicitly rather than by adding -lpthread.
https://chromiumcodereview.appspot.com/9838005/diff/21001/ppapi/native_client... File ppapi/native_client/native_client.gyp (right): https://chromiumcodereview.appspot.com/9838005/diff/21001/ppapi/native_client... ppapi/native_client/native_client.gyp:63: '-lpthread', One more thing Robert figured out. There are two libplatform.a: one with trusted code, another with bitcode. And it seems in my build they were swapped, what resulted in error mentioned earlier. After clean rebuild, error disappeared, even if I'll remove -lpthread, so updated CL accordingly. On 2012/03/30 14:02:11, olonho wrote: > Hmm, I think this issue appears from the fact, that libplatform.a is compiled > with trusted toolchain to native code, while libirt_browser.a (which contains > pthread functions) contains bitcode, produced by untrusted toolchain. > > So generally, -lpthread here satisfies dependencies of trusted code only in > libplatform.a. > > What are alternatives you suggest? > > On 2012/03/29 16:47:15, Roland McGrath wrote: > > -lpthread should not be here. > > The IRT compiles its own versions of the pthread routines it uses. If > something > > needs more pthread support than those in irt_private_pthread.c plus the > > privately-compiled versions of nc_{mutex,condvar,token}.c, then that should be > > dealt with explicitly rather than by adding -lpthread. >
LGTM On 2012/03/30 15:45:03, olonho wrote: > https://chromiumcodereview.appspot.com/9838005/diff/21001/ppapi/native_client... > File ppapi/native_client/native_client.gyp (right): > > https://chromiumcodereview.appspot.com/9838005/diff/21001/ppapi/native_client... > ppapi/native_client/native_client.gyp:63: '-lpthread', > One more thing Robert figured out. There are two libplatform.a: one with trusted > code, another with bitcode. > And it seems in my build they were swapped, what resulted in error mentioned > earlier. After clean rebuild, error disappeared, even if I'll remove -lpthread, > so updated CL accordingly. > > On 2012/03/30 14:02:11, olonho wrote: > > Hmm, I think this issue appears from the fact, that libplatform.a is compiled > > with trusted toolchain to native code, while libirt_browser.a (which contains > > pthread functions) contains bitcode, produced by untrusted toolchain. > > > > So generally, -lpthread here satisfies dependencies of trusted code only in > > libplatform.a. > > > > What are alternatives you suggest? > > > > On 2012/03/29 16:47:15, Roland McGrath wrote: > > > -lpthread should not be here. > > > The IRT compiles its own versions of the pthread routines it uses. If > > something > > > needs more pthread support than those in irt_private_pthread.c plus the > > > privately-compiled versions of nc_{mutex,condvar,token}.c, then that should > be > > > dealt with explicitly rather than by adding -lpthread. > >
LGTM with some nits https://chromiumcodereview.appspot.com/9838005/diff/26001/ppapi/native_client... File ppapi/native_client/native_client.gyp (right): https://chromiumcodereview.appspot.com/9838005/diff/26001/ppapi/native_client... ppapi/native_client/native_client.gyp:54: # Link offsets taken from native_client/build/untrusted.gypi I think this comment is about NACL_IRT_DATA_START/NACL_IRT_TEXT_START. Since you moved those references to further away you should move/remove the comment. I'd just remove the comment since one can grep for the definitions. https://chromiumcodereview.appspot.com/9838005/diff/26001/ppapi/native_client... ppapi/native_client/native_client.gyp:84: # TODO(olonho): rethink Rethink what? This comment is not descriptive enough. Why is there a -L argument here for ARM and not for x86?
https://chromiumcodereview.appspot.com/9838005/diff/14001/build/download_nacl... File build/download_nacl_toolchains.py (right): https://chromiumcodereview.appspot.com/9838005/diff/14001/build/download_nacl... build/download_nacl_toolchains.py:42: if 'target_arch=arm' in os.environ.get('GYP_DEFINES', ''): Was told that need to use gerrit here, so please see https://gerrit.chromium.org/gerrit/#change,19355 On 2012/03/30 14:02:11, olonho wrote: > See https://chromiumcodereview.appspot.com/9923012/ > > On 2012/03/29 17:30:24, davidjames wrote: > > On 2012/03/29 14:36:07, olonho wrote: > > > Agree, but fortunately this hack also matches the way CrOs build system > works, > > > so for most important ARM/NaCl case and for standard way developers > configure > > > their build we're OK. > > > For cross-compilation case, it's rather hard to figure out intention of > > > developer, the only other (somewhat) reasonable approach is to perform smth > > like > > > $CC --version | grep arm. > > > Will add TODO. > > > > > > Regarding trybots - CrOs bots will build NaCl by default, once this whole > > effort > > > will be over. > > > > > > On 2012/03/28 14:59:24, Mark Seaborn wrote: > > > > Hmm, this is a horrible hack that doesn't work if one wants to supply gyp > > > > defines via another means. What would it mean to do this properly? > Please > > > put > > > > a TODO in at least. > > > > > > > > Is there a Chromium trybot that covers this? I don't see any trybot > results > > > for > > > > your change? (To be fair, you were probably waiting on the NaCl side > change > > > > before you can do try runs.) > > > > What changes need to be made to the CrOS ebuild to make the gyp files clean? > > Please make those changes and upload them. You don't need to be afraid of > > patching the chrome ebuild as it is easy :) >
Please, look on this issue again. We have lengthy discussion with Egor on reasons, why linker complains on pthread deps, and came to following conclusions: - libplatform.a/libppruntime.a indeed contain files referring pthread_create/pthread_join, which are not in files linked into IRT - however, due to GNU linker smartness, it overrides symbols referring to those missing ones, and happily links final executable - seems LLVM linker perform symbol override somehow different ()or not so lazy), and as result tries to fetch missing symbols - If I'll add -Wl,--whole-archive for browser runtime static library, override seems to work fine. Not that I'm very happy with this fix, but it seems to be the only solution I see. Maybe I shall make it ARM-conditional, although. https://chromiumcodereview.appspot.com/9838005/diff/26001/ppapi/native_client... File ppapi/native_client/native_client.gyp (right): https://chromiumcodereview.appspot.com/9838005/diff/26001/ppapi/native_client... ppapi/native_client/native_client.gyp:54: # Link offsets taken from native_client/build/untrusted.gypi On 2012/03/30 16:42:46, Mark Seaborn wrote: > I think this comment is about NACL_IRT_DATA_START/NACL_IRT_TEXT_START. Since > you moved those references to further away you should move/remove the comment. > I'd just remove the comment since one can grep for the definitions. Done. https://chromiumcodereview.appspot.com/9838005/diff/26001/ppapi/native_client... ppapi/native_client/native_client.gyp:84: # TODO(olonho): rethink That's exactly what I meant in this comment. If I'll omit this -L, linker doesn't find irt_browser: pnacl-ld: Cannot find '-lirt_browser' Turned out to be a typo, fixed in https://chromiumcodereview.appspot.com/9959060/ On 2012/03/30 16:42:46, Mark Seaborn wrote: > Rethink what? This comment is not descriptive enough. Why is there a -L > argument here for ARM and not for x86?
Just for reference: nacl_threads.o in libplatform.a refers to missing symbols U pthread_create U pthread_exit U pthread_join scons build isn't affected, as (see native_client/src/untrusted/irt/nacl.scons) we pass browser IRT sources directly to compiler, not in form of .a file, so we're semantically close to scons behavior with suggested fix.
https://chromiumcodereview.appspot.com/9838005/diff/30001/ppapi/native_client... File ppapi/native_client/native_client.gyp (right): https://chromiumcodereview.appspot.com/9838005/diff/30001/ppapi/native_client... ppapi/native_client/native_client.gyp:55: '-Wl,--whole-archive', --whole-archive is a pretty big hammer. If -start-group/-end-group or a library re-ordering can achieve the same affect that would be preferable. But we stick with --whole-archive please comment in the code why the special casing is necessary
Split CL in two parts, so Chrome change now is very minimal. https://chromiumcodereview.appspot.com/9838005/diff/30001/ppapi/native_client... File ppapi/native_client/native_client.gyp (right): https://chromiumcodereview.appspot.com/9838005/diff/30001/ppapi/native_client... ppapi/native_client/native_client.gyp:55: '-Wl,--whole-archive', Good idea about linker group. It also fixes the linking issue, so I'll use that. BTW, do I really commit ppapi changes to Chromium tree, or it shall be committed to Pepper tree and fetched to Chromium tree? Seems second, so created https://chromiumcodereview.appspot.com/9958108/ On 2012/04/02 15:32:33, robertm wrote: > --whole-archive is a pretty big hammer. > If -start-group/-end-group or a library re-ordering can achieve the same affect > that would be preferable. > But we stick with --whole-archive please comment in the code why the special > casing is necessary
Egor explained that ppapi changes indeed belongs to Chrome, so moved back. Sorry for confusion.
lgtm
lgtm
ppapi/* lgtm |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
