Issue 9816003: GYP build for ARM untrusted runtime.

Issue 9816003: GYP build for ARM untrusted runtime. (Closed)

Created:
8 years, 9 months ago by Nikolay

Modified:
8 years, 8 months ago

Reviewers:
noelallen_use_chromium, pasko-google - do not use, sehr (please use chromium), halyavin, robertm, Mark Seaborn

CC:
native-client-reviews_googlegroups.com

Base URL:
svn://svn.chromium.org/native_client/trunk/src/native_client/

Visibility:
Public.

More Reviews

Description

GYP build for ARM untrusted runtime. BUG= http://code.google.com/p/nativeclient/issues/detail?id=2687 TEST=run standalone gyp with platform=arm Committed: https://src.chromium.org/viewvc/native_client?view=rev&revision=8123

Patch Set 8 : #

Created: 8 years, 8 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+584 lines, -418 lines)			Patch
M	build/all.gyp	View	1 2 3 4 5 6 7	1 chunk	+6 lines, -5 lines	0 comments	Download
M	build/build_nexe.py	View	1 2 3 4 5 6 7	8 chunks	+62 lines, -11 lines	0 comments	Download
M	build/untrusted.gypi	View	1 2 3 4 5 6 7	5 chunks	+141 lines, -13 lines	0 comments	Download
M	src/shared/gio/gio.gyp	View	1 2 3 4 5 6 7	1 chunk	+13 lines, -18 lines	0 comments	Download
M	src/shared/platform/platform.gyp	View	1 2 3 4 5 6 7	2 chunks	+24 lines, -29 lines	0 comments	Download
M	src/shared/srpc/srpc.gyp	View	1 2 3 4 5 6 7	1 chunk	+26 lines, -31 lines	0 comments	Download
M	src/untrusted/irt/irt.gyp	View	1 2 3 4 5 6 7	1 chunk	+60 lines, -47 lines	0 comments	Download
M	src/untrusted/irt_stub/irt_stub.gyp	View	1 2 3 4 5 6 7	1 chunk	+13 lines, -18 lines	0 comments	Download
M	src/untrusted/nacl/nacl.gyp	View	1 2 3 4 5 6 7	1 chunk	+85 lines, -81 lines	0 comments	Download
M	src/untrusted/nosys/nosys.gyp	View	1 2 3 4 5 6 7	1 chunk	+13 lines, -18 lines	0 comments	Download
M	src/untrusted/pthread/pthread.gyp	View	1 2 3 4 5 6 7	1 chunk	+34 lines, -39 lines	0 comments	Download
M	tools.gyp	View	1 2 3 4 5 6 7	7 chunks	+107 lines, -108 lines	0 comments	Download

Messages

Total messages: 17 (0 generated)

Expand Messages | Collapse Messages

halyavin

https://chromiumcodereview.appspot.com/9816003/diff/1/src/untrusted/irt/irt.gyp File src/untrusted/irt/irt.gyp (right): https://chromiumcodereview.appspot.com/9816003/diff/1/src/untrusted/irt/irt.gyp#newcode66 src/untrusted/irt/irt.gyp:66: ]}, Move } to next line? https://chromiumcodereview.appspot.com/9816003/diff/1/src/untrusted/irt/irt.gyp#newcode82 src/untrusted/irt/irt.gyp:82: ]} ...

8 years, 9 months ago (2012-03-21 10:34:53 UTC) #1

halyavin

https://chromiumcodereview.appspot.com/9816003/diff/4006/build/all.gyp File build/all.gyp (right): https://chromiumcodereview.appspot.com/9816003/diff/4006/build/all.gyp#newcode38 build/all.gyp:38: }], We need to remove space here then.

8 years, 9 months ago (2012-03-21 11:43:10 UTC) #2

Nikolay

https://chromiumcodereview.appspot.com/9816003/diff/1/src/untrusted/irt/irt.gyp File src/untrusted/irt/irt.gyp (right): https://chromiumcodereview.appspot.com/9816003/diff/1/src/untrusted/irt/irt.gyp#newcode66 src/untrusted/irt/irt.gyp:66: ]}, On 2012/03/21 10:34:53, halyavin wrote: > Move } ...

8 years, 9 months ago (2012-03-21 13:16:40 UTC) #3

Mark Seaborn

Please file an issue to track this, and add BUG= and TEST= fields to the ...

8 years, 9 months ago (2012-03-22 17:13:21 UTC) #5

Mark Seaborn

On 22 March 2012 10:13, <mseaborn@chromium.org> wrote: > Please file an issue to track this, ...

8 years, 9 months ago (2012-03-22 19:52:13 UTC) #6

Nikolay

Mark, thanks a lot for review. I created an issue, as you recommended, and updated ...

8 years, 9 months ago (2012-03-23 11:35:20 UTC) #7

Nikolay

https://chromiumcodereview.appspot.com/9816003/diff/27/build/untrusted.gypi File build/untrusted.gypi (right): https://chromiumcodereview.appspot.com/9816003/diff/27/build/untrusted.gypi#newcode295 build/untrusted.gypi:295: 'tool_name': 'newlib', On 2012/03/22 17:13:21, Mark Seaborn wrote: > ...

8 years, 9 months ago (2012-03-23 11:36:17 UTC) #8

robertm

Some more comments below. I am pretty sure that all changes to file in src/ ...

8 years, 9 months ago (2012-03-23 15:03:37 UTC) #9

Mark Seaborn

On 23 March 2012 04:35, <olonho@google.com> wrote: > Mark, thanks a lot for review. I ...

8 years, 9 months ago (2012-03-23 20:29:06 UTC) #10

On 23 March 2012 04:35, <olonho@google.com> wrote:

> Mark, thanks a lot for review. I created an issue, as you recommended, and
> updated BUG and TESTS fields.
>
>
>
>
https://chromiumcodereview.**appspot.com/9816003/diff/27/**build/all.gyp<http...
> File build/all.gyp (right):
>
> https://chromiumcodereview.**appspot.com/9816003/diff/27/**
>
build/all.gyp#newcode33<https://chromiumcodereview.appspot.com/9816003/diff/27/build/all.gyp#newcode33>
> build/all.gyp:33: # No tests on ARM yet.
> Two reasons why I did it:
>  - ARM tests have to be run with QEMU, which means change the way test
> are being run
>

Ah, of course, I forgot about that.


>  - this CL already pretty large, so I think enabling tests worth its own
> CL
>

Yes, fair enough, I agree.


>
> https://chromiumcodereview.appspot.com/9816003/diff/27/build/common.gypi
> File build/common.gypi (right):
>
> https://chromiumcodereview.**appspot.com/9816003/diff/27/**
>
build/common.gypi#newcode301<https://chromiumcodereview.appspot.com/9816003/diff/27/build/common.gypi#newcode301>
> build/common.gypi:301: '--sysroot=<(sysroot)',
> Trusted code builds on ARM for me otherwise.
>

Did you mean to say that it *doesn't* build on ARM?  If so, perhaps you can
do this as a separate changeset for clarity?  The trusted code does build
on the bots, so you might want to work out what is different in your setup.


> From what I've seen, when build system
> computes dependencies, it doesn't seem to look at cflags_cc,
> and so unable to find target system-wide headers, such as zlib.h. Also
> this way it looks more natural. Or there are hidden reasons why it has
> to be that way?
>
>
> https://chromiumcodereview.**appspot.com/9816003/diff/27/**
>
build/untrusted.gypi#newcode82<https://chromiumcodereview.appspot.com/9816003/diff/27/build/untrusted.gypi#newcode82>
> build/untrusted.gypi:82: 'extra_depsarm': [],
> Yep, sounds reasonable, but matching variables for x86 called
> extra_deps32 and extra_deps64. Do you still want me to rename?
>

Yes, please do rename.  The existing "deps32" and "deps64" names are
poorly-chosen because they are x86-specific, and "32" and "64" are not
architectures.  But "depsarm" is hard to visually parse in a way that
"deps32" isn't, because it's not clear that "deps" and "arm" are meant to
be separate tokens.  Ideally we should rename "deps32" to "deps_x86_32",
but that should be a separate change.


> https://chromiumcodereview.**appspot.com/9816003/diff/27/**
>
build/untrusted.gypi#**newcode297<https://chromiumcodereview.appspot.com/9816003/diff/27/build/untrusted.gypi#newcode297>
> build/untrusted.gypi:297: 'outarm%':
> '<(PRODUCT_DIR)/>(nexe_target)**_newlib_arm.nexe',
> Same as above? x86 version called out32 and out64, so is it OK to have
> inconsistent naming?
>

Yeah, let's at least use easy-to-read names for the new stuff.

Mark

-- 
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.

Mark Seaborn

https://chromiumcodereview.appspot.com/9816003/diff/10020/build/all.gyp File build/all.gyp (right): https://chromiumcodereview.appspot.com/9816003/diff/10020/build/all.gyp#newcode22 build/all.gyp:22: # TODO(robertm): Make sel_universal work without relying on the ...

8 years, 9 months ago (2012-03-23 20:33:36 UTC) #11

Nikolay

PTAL https://chromiumcodereview.appspot.com/9816003/diff/27/build/common.gypi File build/common.gypi (right): https://chromiumcodereview.appspot.com/9816003/diff/27/build/common.gypi#newcode301 build/common.gypi:301: '--sysroot=<(sysroot)', > Did you mean to say that ...

8 years, 9 months ago (2012-03-26 13:29:56 UTC) #12

PTAL

https://chromiumcodereview.appspot.com/9816003/diff/27/build/common.gypi
File build/common.gypi (right):

https://chromiumcodereview.appspot.com/9816003/diff/27/build/common.gypi#newc...
build/common.gypi:301: '--sysroot=<(sysroot)',
> Did you mean to say that it *doesn't* build on ARM?  If so, 
> perhaps you can do this as a separate changeset for clarity?  
> The trusted code does build on the bots, so you might want > to work out what
is different in your setup.
Yep, I meant it doesn't. Maybe on buildbots openssl-dev installed in
/usr/include is used by crosscompiler.
I see following error:

V=1 make
/home/olonho/x-tools/arm-unknown-linux-gnueabi.good/bin/arm-unknown-linux-gnueabi-gcc
'-DNACL_LINUX=1' '-DNACL_OSX=0' '-DNACL_WINDOWS=0' '-D_BSD_SOURCE=1'
'-D_POSIX_C_SOURCE=199506' '-D_XOPEN_SOURCE=600' '-D_GNU_SOURCE=1'
'-D__STDC_LIMIT_MACROS=1' '-D__STDC_FORMAT_MACROS' '-DNACL_STANDALONE=1'
'-DNACL_BUILD_ARCH=arm' '-DNACL_BUILD_SUBARCH=32' '-DNACL_TARGET_ARCH=arm'
'-DNACL_TARGET_SUBARCH=32' '-D_DEBUG' -Inative_client/src/third_party -I. 
-Werror -pthread -fno-exceptions -Wall -fPIC -Wextra -Wno-long-long
-Wswitch-enum -Wsign-compare -Wno-abi -march=armv7-a -mtune=cortex-a8 -mfpu=neon
-mfloat-abi=softfp -fno-exceptions -Wall -fPIC -Wno-long-long -O0 -g  -MMD -MF
out/Debug/.deps/out/Debug/obj.target/platform/native_client/src/shared/platform/nacl_global_secure_random.o.d.raw
 -c -o
out/Debug/obj.target/platform/native_client/src/shared/platform/nacl_global_secure_random.o
native_client/src/shared/platform/nacl_global_secure_random.c
In file included from
./native_client/src/shared/platform/nacl_secure_random.h:22,
                 from
./native_client/src/shared/platform/nacl_global_secure_random.h:14,
                 from
native_client/src/shared/platform/nacl_global_secure_random.c:11:
./native_client/src/shared/platform/linux/nacl_secure_random_types.h:32:26:
error: openssl/aes.h: No such file or directory
In file included from
./native_client/src/shared/platform/nacl_secure_random.h:22,
                 from
./native_client/src/shared/platform/nacl_global_secure_random.h:14,
                 from
native_client/src/shared/platform/nacl_global_secure_random.c:11:
./native_client/src/shared/platform/linux/nacl_secure_random_types.h:43: error:
expected specifier-qualifier-list before 'AES_KEY'

But to me this change looks pretty harmless, as just explicitly sets correct
sysroot for all calls of C compiler.

https://chromiumcodereview.appspot.com/9816003/diff/10020/build/all.gyp
File build/all.gyp (right):

https://chromiumcodereview.appspot.com/9816003/diff/10020/build/all.gyp#newco...
build/all.gyp:22: # TODO(robertm): Make sel_universal work without relying on
the old input
Good to know that bug is no longer relevant, but probably not for this CL.

On 2012/03/23 20:33:36, Mark Seaborn wrote:
> On 2012/03/23 15:03:38, robertm wrote:
> > I do not think this bug is relevant anymore - we certainly do not use
> > sel_universal for ppapi emulation like "events".
> 
> Why are you commenting on this line?  It's not relevant to the change in
hand...

https://chromiumcodereview.appspot.com/9816003/diff/10020/build/all.gyp#newco...
build/all.gyp:26: '../src/untrusted/irt/irt.gyp:irt_core_nexe',
My fix was just to make those deps unconditional. I wasn't questioning if it is
needed at all. Maybe if it's not, it shall be separate cleanup CL. But I think
they are needed, as it enables building untrusted runtime, it seems.

On 2012/03/23 15:03:38, robertm wrote:
> why were those needed for arm in the first place.
> What happens if they are deleted alltogether?
> (I am a little skeptical since that were not used in the non-arm case before)

https://chromiumcodereview.appspot.com/9816003/diff/10020/build/build_nexe.py
File build/build_nexe.py (right):

https://chromiumcodereview.appspot.com/9816003/diff/10020/build/build_nexe.py...
build/build_nexe.py:16: """
maybe, but I think one CL shall be focused on one thing, usually. 

On 2012/03/23 15:03:38, robertm wrote:
> not you fault - but an example invocation would make this 
> comment a lot better

https://chromiumcodereview.appspot.com/9816003/diff/10020/build/build_nexe.py...
build/build_nexe.py:112: if self.pnacl:
I'm with Mark here. IMO, Python is not intended to be so terse, although idea is
generally nice and "functional" in style :). 

On 2012/03/23 20:33:36, Mark Seaborn wrote:
> On 2012/03/23 15:03:38, robertm wrote:
> > it may help with readability to define:
> > 
> > J = os.path.join
> 
> I don't agree.  Such an abbreviation saves a few characters but is harder to
> read for anyone who knows Python.

https://chromiumcodereview.appspot.com/9816003/diff/10020/build/build_nexe.py...
build/build_nexe.py:114: self.toolchain =
self.GenNaClPath(os.path.join('toolchain',
Hmm, but aren't we generally try to separate improvements
and functionality into separate changesets. While I'm perfectly fine to do that,
I'm not sure if it is right.
So does it make sense to change it into local and rename?

On 2012/03/23 15:03:38, robertm wrote:
> self.toolchain does not seem to be used later on, maybe just call it
> toolchain_path

https://chromiumcodereview.appspot.com/9816003/diff/10020/build/untrusted.gypi
File build/untrusted.gypi (right):

https://chromiumcodereview.appspot.com/9816003/diff/10020/build/untrusted.gyp...
build/untrusted.gypi:71: # ARM case
I tried to follow style of the rest of this file.
And it separates definitions from rules. It makes sense to me, as we see later
how different binaries are built for different platforms. 

On 2012/03/23 15:03:38, robertm wrote:
> can this be better merged with the one above so that only what is different
for
> arm is in the arm section

https://chromiumcodereview.appspot.com/9816003/diff/10020/src/untrusted/irt/i...
File src/untrusted/irt/irt.gyp (right):

https://chromiumcodereview.appspot.com/9816003/diff/10020/src/untrusted/irt/i...
src/untrusted/irt/irt.gyp:92: 'target_name': 'irt_browser_lib',
All those lines are not really changed - I just removed if target != arm
condition and all formatting changes.
Yet another beauty of GYP :(. I have not written this code, so unsure if can
provide 100% comments on intentions in naming, but I think it's NaCl untrusted
runtime used in browser, vs. runtime used in standalone mode.

On 2012/03/23 15:03:38, robertm wrote:
> not sure what this is - maybe add a comment

https://chromiumcodereview.appspot.com/9816003/diff/10020/src/untrusted/irt/i...
src/untrusted/irt/irt.gyp:101: ['target_arch == "x64" or target_arch == "ia32"',
{
Same as above. This is not new or modified code.

On 2012/03/23 15:03:38, robertm wrote:
> this looks suspicious does this build for arm at all?
> also, you may want to change the condition to
> ['target_arch!="arm" 
> to be consistent

robertm

https://chromiumcodereview.appspot.com/9816003/diff/18001/build/untrusted.gypi File build/untrusted.gypi (right): https://chromiumcodereview.appspot.com/9816003/diff/18001/build/untrusted.gypi#newcode69 build/untrusted.gypi:69: }, There is a little too much duplication here ...

8 years, 9 months ago (2012-03-26 14:09:32 UTC) #13

Nikolay

PTAL. https://chromiumcodereview.appspot.com/9816003/diff/18001/build/untrusted.gypi File build/untrusted.gypi (right): https://chromiumcodereview.appspot.com/9816003/diff/18001/build/untrusted.gypi#newcode69 build/untrusted.gypi:69: }, Tried to address that. On 2012/03/26 14:09:33, ...

8 years, 9 months ago (2012-03-26 16:28:51 UTC) #14

Mark Seaborn

http://codereview.chromium.org/9816003/diff/18004/build/build_nexe.py File build/build_nexe.py (right): http://codereview.chromium.org/9816003/diff/18004/build/build_nexe.py#newcode180 build/build_nexe.py:180: return self.GetBinName("strip") Use ' instead of " http://codereview.chromium.org/9816003/diff/18004/build/common.gypi File ...

8 years, 8 months ago (2012-03-28 07:21:02 UTC) #16

Nikolay

8 years, 8 months ago (2012-03-28 13:24:23 UTC) #17

Cleaned up final comments and submitted. 
Thanks a lot everybody, especially Mark and Robert for great review. If there
will are future concerns - will fix them in upcoming CLs.

https://chromiumcodereview.appspot.com/9816003/diff/18004/build/build_nexe.py
File build/build_nexe.py (right):

https://chromiumcodereview.appspot.com/9816003/diff/18004/build/build_nexe.py...
build/build_nexe.py:180: return self.GetBinName("strip")
On 2012/03/28 07:21:02, Mark Seaborn wrote:
> Use ' instead of "

Done.

https://chromiumcodereview.appspot.com/9816003/diff/18004/build/common.gypi
File build/common.gypi (left):

https://chromiumcodereview.appspot.com/9816003/diff/18004/build/common.gypi#o...
build/common.gypi:302: 'cflags_cc': [
OK, will make it separate CL, see
https://chromiumcodereview.appspot.com/9875016/.
On 2012/03/28 07:21:02, Mark Seaborn wrote:
> I would still prefer if you did trusted-code Gyp changes in a separate
changeset
> for clarity.

https://chromiumcodereview.appspot.com/9816003/diff/23002/build/untrusted.gypi
File build/untrusted.gypi (right):

https://chromiumcodereview.appspot.com/9816003/diff/23002/build/untrusted.gyp...
build/untrusted.gypi:57: '<@(default_defines)',
If I used defines, instead of default_defines, gyp falls into infinite variable
expansion recursion for some reasons.
Same with compile flags, so I made this (indeed pretty artificial) split. If you
do have an idea how to fix that - would be perfect. Late expansion (>@(defines))
also doesn't help.

On 2012/03/28 07:21:02, Mark Seaborn wrote:
> I think you don't need the 'defines'/'default_defines' split.  I think if you
> write 'defines' in both cases, this part will just append to the list.

https://chromiumcodereview.appspot.com/9816003/diff/23002/build/untrusted.gyp...
build/untrusted.gypi:291: ['nexe_target!="" and build_newlib!=0', {
Thanks, it was indeed indent changes which confused diff. Fixed.

On 2012/03/28 07:21:02, Mark Seaborn wrote:
> It's a bit disconcerting that this chunk appears as a modification to an
> existing chunk.  Is that just an artefact of the ordering?  I think it's
because
> of whitespace changes.  See below...

https://chromiumcodereview.appspot.com/9816003/diff/23002/build/untrusted.gyp...
build/untrusted.gypi:381: 'tool_name': 'glibc',
On 2012/03/28 07:21:02, Mark Seaborn wrote:
> I think you've changed the indentation in this block (the x86-64 glibc nexe
> case), which is why it appears as an insertion.  Please change it back so that
> only the ARM cases appear as insertions.

Done.

https://chromiumcodereview.appspot.com/9816003/diff/23002/src/shared/platform...
File src/shared/platform/platform.gyp (right):

https://chromiumcodereview.appspot.com/9816003/diff/23002/src/shared/platform...
src/shared/platform/platform.gyp:195: ]
Thanks, seems my beloved editor misused python-mode instead of gyp-mode on Gyp
files.

On 2012/03/28 07:21:02, Mark Seaborn wrote:
> indent -2; same for other closing brackets below.

https://chromiumcodereview.appspot.com/9816003/diff/23002/tools.gyp
File tools.gyp (right):

https://chromiumcodereview.appspot.com/9816003/diff/23002/tools.gyp#newcode39
tools.gyp:39: 'src/untrusted/stubs/crt1.x',
On 2012/03/28 07:21:02, Mark Seaborn wrote:
> Fix the indentation here, please

Done.

Expand Messages | Collapse Messages