Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
940993003:
Build the IRT with nacl-clang for x86 (Closed)
|
Created:
5 years, 10 months ago by Derek Schuff Modified:
5 years, 9 months ago CC:
native-client-reviews_googlegroups.com Base URL:
https://chromium.googlesource.com/native_client/src/native_client.git@master Target Ref:
refs/heads/master Project:
nacl Visibility:
Public. |
DescriptionBuild the IRT with nacl-clang for x86
This should result in faster builds, and in the future could
allow cleanups of the IRT-building gyp code and the PNaCl drivers.
* Use -mstackrealign and -mno-sse to be callable with under-aligned stacks.
* Use -integrated-as to ensure sandbox base address hiding on x86-64.
* Uses -fno-exceptions to reduce the size, and links with
PNaCl's unwind stubs to avoid including the unwinder (which
would otherwise be pulled in by libcxx).
* Remove -gline-tables-only which was a workaround for
slowness and excessive memory consumption in bitcode linking
* Also use nacl-clang for the SCons core IRT build.
BUG= https://code.google.com/p/nativeclient/issues/detail?id=4088
R=jvoung@chromium.org, mseaborn@chromium.org
Committed: https://chromium.googlesource.com/native_client/src/native_client/+/316f3e53bfcc34e4a137abbce02d890e3ea968ce
Patch Set 1 #Patch Set 2 : #Patch Set 3 : udpates #Patch Set 4 : #Patch Set 5 : rebase #Patch Set 6 : #Patch Set 7 : fix ARM #Patch Set 8 : #Patch Set 9 : <3 gyp #Patch Set 10 : fix sbtc build #
Total comments: 34
Patch Set 11 : review #Patch Set 12 : remove -lgcc_eh #
Total comments: 6
Patch Set 13 : review 2 #
Total comments: 25
Patch Set 14 : mseaborn review #
Total comments: 4
Patch Set 15 : rebase #Patch Set 16 : format nits #
Messages
Total messages: 22 (4 generated)
dschuff@chromium.org changed reviewers: + jvoung@chromium.org, mseaborn@chromium.org
https://codereview.chromium.org/940993003/diff/170001/SConstruct File SConstruct (right): https://codereview.chromium.org/940993003/diff/170001/SConstruct#newcode3482 SConstruct:3482: # base address hiding. It's also use on x86-32 for consistency. "It's also use on" -> "It's also used on" or something https://codereview.chromium.org/940993003/diff/170001/SConstruct#newcode3483 SConstruct:3483: nacl_irt_env.Append(CCFLAGS=['-integrated-as']) Might want to check with Nick about the GN build. I see a "nacl_toolchain("irt_x64")", but I don't know if it's using the integrated-as. Also not sure if it's doing the appropriate mstackrealign, etc. for nacl_toolchain("irt_x86") https://codereview.chromium.org/940993003/diff/170001/SConstruct#newcode3489 SConstruct:3489: nacl_irt_env.ClearBits('pnacl_generate_pexe') Maybe these last couple of ClearBits('...') should be moved up to be with the rest of the ClearBits. https://codereview.chromium.org/940993003/diff/170001/SConstruct#newcode3505 SConstruct:3505: if nacl_irt_env.Bit('build_x86_64'): this case isn't possible anymore? https://codereview.chromium.org/940993003/diff/170001/SConstruct#newcode3510 SConstruct:3510: elif nacl_irt_env.Bit('build_x86_32'): same https://codereview.chromium.org/940993003/diff/170001/SConstruct#newcode3533 SConstruct:3533: nacl_irt_env.Append(LINKFLAGS=['--pnacl-allow-native']) Could put this under the "3504 if nacl_irt_env.Bit('bitcode'):" to consolidate. https://codereview.chromium.org/940993003/diff/170001/build/untrusted.gypi File build/untrusted.gypi (right): https://codereview.chromium.org/940993003/diff/170001/build/untrusted.gypi#ne... build/untrusted.gypi:336: '<(DEPTH)/native_client/toolchain/<(TOOLCHAIN_OS)_x86/nacl_x86_newlib/nacl_x86_newlib.json', The nacl_x86_newlib.json should refert to nacl-clang's .json https://codereview.chromium.org/940993003/diff/170001/build/untrusted.gypi#ne... build/untrusted.gypi:348: '--compile_flags=^(pnacl_compile_flags) >(_pnacl_compile_flags) ^(native_irt_compile_flags) -integrated-as', Could add the same -fno-exceptions and -Os here that was in the nlib case for consistency. https://codereview.chromium.org/940993003/diff/170001/build/untrusted.gypi#ne... build/untrusted.gypi:380: '<(DEPTH)/native_client/toolchain/<(TOOLCHAIN_OS)_x86/nacl_x86_newlib/nacl_x86_newlib.json', same json thing https://codereview.chromium.org/940993003/diff/170001/build/untrusted.gypi#ne... build/untrusted.gypi:394: '--link_flags=-B>(tc_lib_dir_irt64) ^(link_flags) >(_link_flags) ^(native_irt_link_flags)', could make the order of link_flags vs native_irt_link_flags the same between the nlib and the nexe -- right now they are swapped https://codereview.chromium.org/940993003/diff/170001/build/untrusted.gypi#ne... build/untrusted.gypi:504: '<(DEPTH)/native_client/toolchain/<(TOOLCHAIN_OS)_x86/nacl_x86_newlib/nacl_x86_newlib.json', nacl_x86_newlib.json should refer to nacl-clang's .json https://codereview.chromium.org/940993003/diff/170001/build/untrusted.gypi#ne... build/untrusted.gypi:516: '--compile_flags=-m32 ^(compile_flags) >(_compile_flags) ^(pnacl_compile_flags) >(_pnacl_compile_flags) >(irt_flags_x86_32) ^(native_irt_compile_flags)', Same comment about -Os and -fno-exceptions. Might be able to just put that in native_irt_compile_flags? That could also affect ARM and not sure if that would have an effect. Since scons uses -integrated-as for x86-32 also, can this be made consistent with scons? https://codereview.chromium.org/940993003/diff/170001/build/untrusted.gypi#ne... build/untrusted.gypi:519: '--link_flags=-m32 -B>(tc_lib_dir_irt32) -L>(tc_lib_dir_irt32) ^(pnacl_irt_link_flags) ^(link_flags) >(_link_flags) ^(native_irt_link_flags) >(libcpp_irt_stdlibs)', ^(pnacl_irt_link_flags) is still here, and it has some flags like -Wt,-ffunction-sections -Wn,--gc-sections ordering of ^(link_flags) >(_link_flags) ^(native_irt_link_flags) is different from x86-64. https://codereview.chromium.org/940993003/diff/170001/build/untrusted.gypi#ne... build/untrusted.gypi:548: '<(DEPTH)/native_client/toolchain/<(TOOLCHAIN_OS)_x86/nacl_x86_newlib/nacl_x86_newlib.json', The .json should refer to nacl-clang's .json now? https://codereview.chromium.org/940993003/diff/170001/toolchain_build/pnacl_s... File toolchain_build/pnacl_sandboxed_translator.py (right): https://codereview.chromium.org/940993003/diff/170001/toolchain_build/pnacl_s... toolchain_build/pnacl_sandboxed_translator.py:29: # as a side effect of building the same targets in the le32 environment. I don't really understand this one... This is because we need the IRT headers and so on installed, and that's tied to the IRT environ? Or is it the non-IRT private libs? https://codereview.chromium.org/940993003/diff/170001/toolchain_build/pnacl_s... toolchain_build/pnacl_sandboxed_translator.py:57: # Copy the le32 bitcode libs le32 bitcode libs and nacl-clang newlib packages?
https://codereview.chromium.org/940993003/diff/170001/build/untrusted.gypi File build/untrusted.gypi (right): https://codereview.chromium.org/940993003/diff/170001/build/untrusted.gypi#ne... build/untrusted.gypi:1481: '-lgcc_eh', Also, does lgcc_eh need to be here, if the stubs are part of irt_browser?
https://codereview.chromium.org/940993003/diff/170001/SConstruct File SConstruct (right): https://codereview.chromium.org/940993003/diff/170001/SConstruct#newcode3482 SConstruct:3482: # base address hiding. It's also use on x86-32 for consistency. On 2015/03/19 17:36:25, jvoung wrote: > "It's also use on" -> "It's also used on" > or something Done. https://codereview.chromium.org/940993003/diff/170001/SConstruct#newcode3483 SConstruct:3483: nacl_irt_env.Append(CCFLAGS=['-integrated-as']) On 2015/03/19 17:36:25, jvoung wrote: > Might want to check with Nick about the GN build. > > I see a "nacl_toolchain("irt_x64")", but I don't know if it's using the > integrated-as. > > Also not sure if it's doing the appropriate mstackrealign, etc. for > nacl_toolchain("irt_x86") Yeah, my plan was to get this landed for gyp and ensure it works before circling back to GN, since they've always been doing their own thing anyway. https://codereview.chromium.org/940993003/diff/170001/SConstruct#newcode3489 SConstruct:3489: nacl_irt_env.ClearBits('pnacl_generate_pexe') On 2015/03/19 17:36:25, jvoung wrote: > Maybe these last couple of ClearBits('...') should be moved up to be with the > rest of the ClearBits. Done. https://codereview.chromium.org/940993003/diff/170001/SConstruct#newcode3505 SConstruct:3505: if nacl_irt_env.Bit('build_x86_64'): On 2015/03/19 17:36:25, jvoung wrote: > this case isn't possible anymore? Done. https://codereview.chromium.org/940993003/diff/170001/SConstruct#newcode3510 SConstruct:3510: elif nacl_irt_env.Bit('build_x86_32'): On 2015/03/19 17:36:25, jvoung wrote: > same Done. https://codereview.chromium.org/940993003/diff/170001/SConstruct#newcode3533 SConstruct:3533: nacl_irt_env.Append(LINKFLAGS=['--pnacl-allow-native']) On 2015/03/19 17:36:24, jvoung wrote: > Could put this under the "3504 if nacl_irt_env.Bit('bitcode'):" to consolidate. Done. https://codereview.chromium.org/940993003/diff/170001/build/untrusted.gypi File build/untrusted.gypi (right): https://codereview.chromium.org/940993003/diff/170001/build/untrusted.gypi#ne... build/untrusted.gypi:336: '<(DEPTH)/native_client/toolchain/<(TOOLCHAIN_OS)_x86/nacl_x86_newlib/nacl_x86_newlib.json', On 2015/03/19 17:36:25, jvoung wrote: > The nacl_x86_newlib.json should refert to nacl-clang's .json Done, and fixed a few other wrong ones too. https://codereview.chromium.org/940993003/diff/170001/build/untrusted.gypi#ne... build/untrusted.gypi:348: '--compile_flags=^(pnacl_compile_flags) >(_pnacl_compile_flags) ^(native_irt_compile_flags) -integrated-as', On 2015/03/19 17:36:25, jvoung wrote: > Could add the same -fno-exceptions and -Os here that was in the nlib case for > consistency. Done. Factored them into a variable. https://codereview.chromium.org/940993003/diff/170001/build/untrusted.gypi#ne... build/untrusted.gypi:380: '<(DEPTH)/native_client/toolchain/<(TOOLCHAIN_OS)_x86/nacl_x86_newlib/nacl_x86_newlib.json', On 2015/03/19 17:36:25, jvoung wrote: > same json thing Done. https://codereview.chromium.org/940993003/diff/170001/build/untrusted.gypi#ne... build/untrusted.gypi:394: '--link_flags=-B>(tc_lib_dir_irt64) ^(link_flags) >(_link_flags) ^(native_irt_link_flags)', On 2015/03/19 17:36:25, jvoung wrote: > could make the order of link_flags vs native_irt_link_flags the same between the > nlib and the nexe -- right now they are swapped yeah... there's general inconsistency there now because I waffled on whether I wanted to even include the compile flags in the link steps where they aren't even used (and vice versa). but i'll make them all the same, since they exist at all. https://codereview.chromium.org/940993003/diff/170001/build/untrusted.gypi#ne... build/untrusted.gypi:504: '<(DEPTH)/native_client/toolchain/<(TOOLCHAIN_OS)_x86/nacl_x86_newlib/nacl_x86_newlib.json', On 2015/03/19 17:36:25, jvoung wrote: > nacl_x86_newlib.json should refer to nacl-clang's .json Done. https://codereview.chromium.org/940993003/diff/170001/build/untrusted.gypi#ne... build/untrusted.gypi:516: '--compile_flags=-m32 ^(compile_flags) >(_compile_flags) ^(pnacl_compile_flags) >(_pnacl_compile_flags) >(irt_flags_x86_32) ^(native_irt_compile_flags)', On 2015/03/19 17:36:26, jvoung wrote: > Same comment about -Os and -fno-exceptions. Might be able to just put that in > native_irt_compile_flags? That could also affect ARM and not sure if that would > have an effect. > > Since scons uses -integrated-as for x86-32 also, can this be made consistent > with scons? Done; factored them into their own variable as mentioned above (left out for ARM with a TODO to merge them) https://codereview.chromium.org/940993003/diff/170001/build/untrusted.gypi#ne... build/untrusted.gypi:519: '--link_flags=-m32 -B>(tc_lib_dir_irt32) -L>(tc_lib_dir_irt32) ^(pnacl_irt_link_flags) ^(link_flags) >(_link_flags) ^(native_irt_link_flags) >(libcpp_irt_stdlibs)', On 2015/03/19 17:36:25, jvoung wrote: > ^(pnacl_irt_link_flags) is still here, and it has some flags like > > -Wt,-ffunction-sections > -Wn,--gc-sections > > > ordering of ^(link_flags) >(_link_flags) ^(native_irt_link_flags) is different > from x86-64. Done. https://codereview.chromium.org/940993003/diff/170001/build/untrusted.gypi#ne... build/untrusted.gypi:548: '<(DEPTH)/native_client/toolchain/<(TOOLCHAIN_OS)_x86/nacl_x86_newlib/nacl_x86_newlib.json', On 2015/03/19 17:36:25, jvoung wrote: > The .json should refer to nacl-clang's .json now? Done. https://codereview.chromium.org/940993003/diff/170001/toolchain_build/pnacl_s... File toolchain_build/pnacl_sandboxed_translator.py (right): https://codereview.chromium.org/940993003/diff/170001/toolchain_build/pnacl_s... toolchain_build/pnacl_sandboxed_translator.py:29: # as a side effect of building the same targets in the le32 environment. On 2015/03/19 17:36:26, jvoung wrote: > I don't really understand this one... > > This is because we need the IRT headers and so on installed, and that's tied to > the IRT environ? Or is it the non-IRT private libs? when you give a target of say libnacl_sys_private to SCons, it builds that target in all the environments where it's defined (i.e. both the IRT and pexe environments). The IRT one was failing because it didn't have the native newlib (even though it's actually the pexe one we want) https://codereview.chromium.org/940993003/diff/170001/toolchain_build/pnacl_s... toolchain_build/pnacl_sandboxed_translator.py:57: # Copy the le32 bitcode libs On 2015/03/19 17:36:26, jvoung wrote: > le32 bitcode libs and nacl-clang newlib packages? Done.
https://codereview.chromium.org/940993003/diff/170001/build/untrusted.gypi File build/untrusted.gypi (right): https://codereview.chromium.org/940993003/diff/170001/build/untrusted.gypi#ne... build/untrusted.gypi:1481: '-lgcc_eh', On 2015/03/19 17:51:53, jvoung wrote: > Also, does lgcc_eh need to be here, if the stubs are part of irt_browser? Ping about this question? If it links without this, then I'm more certain that the stubs are being used and none of the code is compiled by nacl-gcc instead. Otherwise, it's a bit hard to tell.
On 2015/03/19 19:21:06, jvoung wrote: > https://codereview.chromium.org/940993003/diff/170001/build/untrusted.gypi > File build/untrusted.gypi (right): > > https://codereview.chromium.org/940993003/diff/170001/build/untrusted.gypi#ne... > build/untrusted.gypi:1481: '-lgcc_eh', > On 2015/03/19 17:51:53, jvoung wrote: > > Also, does lgcc_eh need to be here, if the stubs are part of irt_browser? > > Ping about this question? > > If it links without this, then I'm more certain that the stubs are being used > and none of the code is compiled by nacl-gcc instead. Otherwise, it's a bit hard > to tell. Yeah i left a chrome build running without it over lunch to test it. looks like it's not necessary, will remove.
Cool -- faster compiles! LGTM https://codereview.chromium.org/940993003/diff/210001/src/untrusted/irt/frame... File src/untrusted/irt/frame_info_stubs.c (right): https://codereview.chromium.org/940993003/diff/210001/src/untrusted/irt/frame... src/untrusted/irt/frame_info_stubs.c:7: /* On x86-64 we build the IRT with sandbox base-address hiding. This means that nit: Usual NaCl style for multi-line C comments is more like: /* * xyz... */ https://codereview.chromium.org/940993003/diff/210001/src/untrusted/irt/irt.gyp File src/untrusted/irt/irt.gyp (right): https://codereview.chromium.org/940993003/diff/210001/src/untrusted/irt/irt.g... src/untrusted/irt/irt.gyp:52: # (which is build with exception handling enabled) and from crtbegin.c. "which is build with" -> "which is built with" https://codereview.chromium.org/940993003/diff/210001/toolchain_build/pnacl_s... File toolchain_build/pnacl_sandboxed_translator.py (right): https://codereview.chromium.org/940993003/diff/210001/toolchain_build/pnacl_s... toolchain_build/pnacl_sandboxed_translator.py:61: for p in ['target_lib_compiler'] + ( nit: pre-existing, but the one space indent of the "for p in " of the list comprehension seems a bit odd
faster but sadly, not as fast as you would hope. build_nexe.py is still terrible. https://codereview.chromium.org/940993003/diff/210001/src/untrusted/irt/frame... File src/untrusted/irt/frame_info_stubs.c (right): https://codereview.chromium.org/940993003/diff/210001/src/untrusted/irt/frame... src/untrusted/irt/frame_info_stubs.c:7: /* On x86-64 we build the IRT with sandbox base-address hiding. This means that On 2015/03/19 19:38:56, jvoung wrote: > nit: Usual NaCl style for multi-line C comments is more like: > > /* > * xyz... > */ Done. https://codereview.chromium.org/940993003/diff/210001/src/untrusted/irt/irt.gyp File src/untrusted/irt/irt.gyp (right): https://codereview.chromium.org/940993003/diff/210001/src/untrusted/irt/irt.g... src/untrusted/irt/irt.gyp:52: # (which is build with exception handling enabled) and from crtbegin.c. On 2015/03/19 19:38:56, jvoung wrote: > "which is build with" -> "which is built with" Done. https://codereview.chromium.org/940993003/diff/210001/toolchain_build/pnacl_s... File toolchain_build/pnacl_sandboxed_translator.py (right): https://codereview.chromium.org/940993003/diff/210001/toolchain_build/pnacl_s... toolchain_build/pnacl_sandboxed_translator.py:61: for p in ['target_lib_compiler'] + ( On 2015/03/19 19:38:56, jvoung wrote: > nit: pre-existing, but the one space indent of the "for p in " of the list > comprehension seems a bit odd fixed. also the parens are not required for line continuation since the whole thing is in brackets, so removed them.
still need mseaborn for src/untrusted/irt OWNERS
https://codereview.chromium.org/940993003/diff/230001/SConstruct File SConstruct (right): https://codereview.chromium.org/940993003/diff/230001/SConstruct#newcode3488 SConstruct:3488: # See https://code.google.com/p/nativeclient/issues/detail?id=3935 Nit: excess space after "See" https://codereview.chromium.org/940993003/diff/230001/SConstruct#newcode3489 SConstruct:3489: nacl_irt_env.Append(CCFLAGS=['-mstackrealign', '-mno-sse']) How are we applying these for the libraries in the toolchain? That's not clear from the issue linked here, https://code.google.com/p/nativeclient/issues/detail?id=3935. https://codereview.chromium.org/940993003/diff/230001/build/untrusted.gypi File build/untrusted.gypi (left): https://codereview.chromium.org/940993003/diff/230001/build/untrusted.gypi#ol... build/untrusted.gypi:348: '--compile_flags=--target=x86_64-unknown-nacl ^(compile_flags) >(_compile_flags) -gline-tables-only ^(pnacl_compile_flags) >(_pnacl_compile_flags)', What's the reason for taking out "-gline-tables-only"? Do we now use full debug info? Can you comment this in the commit message? https://codereview.chromium.org/940993003/diff/230001/build/untrusted.gypi File build/untrusted.gypi (right): https://codereview.chromium.org/940993003/diff/230001/build/untrusted.gypi#ne... build/untrusted.gypi:133: 'irt_flags_x86_32': '-mstackrealign -mno-sse', What was the reason for having both (adding '-mstackrealign')? https://codereview.chromium.org/940993003/diff/230001/build/untrusted.gypi#ne... build/untrusted.gypi:394: '--link_flags=-B>(tc_lib_dir_irt64) ^(link_flags) >(_link_flags) ^(native_irt_link_flags) >(libcpp_irt_stdlibs)', I don't think ">(libcpp_irt_stdlibs)" is needed for building libraries. It just contains "-lLIB" options. https://codereview.chromium.org/940993003/diff/230001/build/untrusted.gypi#ne... build/untrusted.gypi:596: '<(DEPTH)/native_client/toolchain/<(TOOLCHAIN_OS)_x86/pnacl_newlib/pnacl_newlib.json', Is this an unrelated fix? https://codereview.chromium.org/940993003/diff/230001/build/untrusted.gypi#ne... build/untrusted.gypi:1467: '-integrated-as', Add a comment to explain why this is needed for security hardening. Can you link to a reference explaining the base address hiding? https://codereview.chromium.org/940993003/diff/230001/src/untrusted/irt/frame... File src/untrusted/irt/frame_info_stubs.c (right): https://codereview.chromium.org/940993003/diff/230001/src/untrusted/irt/frame... src/untrusted/irt/frame_info_stubs.c:8: * On x86-64 we build the IRT with sandbox base-address hiding. This means that What's the plan for making sure this doesn't regress? If we stopped including this file in the build, would anything catch that? Shyam was going to implement some extra validator checks in parallel with your work on changing the IRT build, but he left. There are a lot of parts that must be right for the base-address-hiding to work. I'm concerned that an innocent-looking toolchain change could cause a regression without us noticing. Should we find someone to do the validator change, if only to catch a simple subset of cases, like disallowing CALL instructions? https://codereview.chromium.org/940993003/diff/230001/src/untrusted/irt/frame... src/untrusted/irt/frame_info_stubs.c:16: void __register_frame_info(void *begin, void *ob) {} When linking nexes with nacl-clang, do we always use the variant of crtbegin.o that pulls these in? Does nacl-clang always pull in these functions from libgcc_eh? https://codereview.chromium.org/940993003/diff/230001/src/untrusted/irt/irt.gyp File src/untrusted/irt/irt.gyp (right): https://codereview.chromium.org/940993003/diff/230001/src/untrusted/irt/irt.g... src/untrusted/irt/irt.gyp:46: # On x86-64 we build the IRT with sandbox base-address hiding. This means that Nit: indent comment https://codereview.chromium.org/940993003/diff/230001/src/untrusted/irt/irt.g... src/untrusted/irt/irt.gyp:103: 'sources': ['<@(irt_sources)','<@(stub_sources)'], Nit: add space after comma https://codereview.chromium.org/940993003/diff/230001/src/untrusted/irt/irt.g... src/untrusted/irt/irt.gyp:109: 'stub_sources' : [] Nit: would it be more idiomatic for the condition to add to "sources" rather than overriding "stub_sources"?
https://codereview.chromium.org/940993003/diff/230001/SConstruct File SConstruct (right): https://codereview.chromium.org/940993003/diff/230001/SConstruct#newcode3489 SConstruct:3489: nacl_irt_env.Append(CCFLAGS=['-mstackrealign', '-mno-sse']) On 2015/03/19 20:54:48, Mark Seaborn wrote: > How are we applying these for the libraries in the toolchain? That's not clear > from the issue linked here, > https://code.google.com/p/nativeclient/issues/detail?id=3935. The libraries do not need to be compiled without SSE because of the use of -mstackrealign here. Functions in the IRT realign the stack on entry so that any stack variables used by them and their callees are aligned. The -mno-sse ensures that the IRT functions do not use SSE to access their arguments (which can be misaligned and are not affected by -mstackrealign). I'll add this explanation to the bug. https://codereview.chromium.org/940993003/diff/230001/build/untrusted.gypi File build/untrusted.gypi (left): https://codereview.chromium.org/940993003/diff/230001/build/untrusted.gypi#ol... build/untrusted.gypi:348: '--compile_flags=--target=x86_64-unknown-nacl ^(compile_flags) >(_compile_flags) -gline-tables-only ^(pnacl_compile_flags) >(_pnacl_compile_flags)', On 2015/03/19 20:54:48, Mark Seaborn wrote: > What's the reason for taking out "-gline-tables-only"? Do we now use full debug > info? Can you comment this in the commit message? -gline-tables-only was basically just a workaround for slow bitcode linking. If you actually do want to debug with the IRT, full debug info is more useful. https://codereview.chromium.org/940993003/diff/230001/build/untrusted.gypi File build/untrusted.gypi (right): https://codereview.chromium.org/940993003/diff/230001/build/untrusted.gypi#ne... build/untrusted.gypi:133: 'irt_flags_x86_32': '-mstackrealign -mno-sse', On 2015/03/19 20:54:48, Mark Seaborn wrote: > What was the reason for having both (adding '-mstackrealign')? see reply in SConstruct (will post in the bug too) https://codereview.chromium.org/940993003/diff/230001/build/untrusted.gypi#ne... build/untrusted.gypi:394: '--link_flags=-B>(tc_lib_dir_irt64) ^(link_flags) >(_link_flags) ^(native_irt_link_flags) >(libcpp_irt_stdlibs)', On 2015/03/19 20:54:48, Mark Seaborn wrote: > I don't think ">(libcpp_irt_stdlibs)" is needed for building libraries. It just > contains "-lLIB" options. IIRC None of the link flags are even used at all when building libraries, and none of the compile flags are used at all when building nexes. IIRC There's no good reason for them to be there, but since they are they might as well be consistent. Fixing that would be another CL. https://codereview.chromium.org/940993003/diff/230001/build/untrusted.gypi#ne... build/untrusted.gypi:596: '<(DEPTH)/native_client/toolchain/<(TOOLCHAIN_OS)_x86/pnacl_newlib/pnacl_newlib.json', On 2015/03/19 20:54:48, Mark Seaborn wrote: > Is this an unrelated fix? yes. https://codereview.chromium.org/940993003/diff/230001/build/untrusted.gypi#ne... build/untrusted.gypi:1467: '-integrated-as', On 2015/03/19 20:54:48, Mark Seaborn wrote: > Add a comment to explain why this is needed for security hardening. Can you > link to a reference explaining the base address hiding? Done. https://codereview.chromium.org/940993003/diff/230001/src/untrusted/irt/frame... File src/untrusted/irt/frame_info_stubs.c (right): https://codereview.chromium.org/940993003/diff/230001/src/untrusted/irt/frame... src/untrusted/irt/frame_info_stubs.c:8: * On x86-64 we build the IRT with sandbox base-address hiding. This means that On 2015/03/19 20:54:48, Mark Seaborn wrote: > What's the plan for making sure this doesn't regress? If we stopped including > this file in the build, would anything catch that? > > Shyam was going to implement some extra validator checks in parallel with your > work on changing the IRT build, but he left. > > There are a lot of parts that must be right for the base-address-hiding to work. > I'm concerned that an innocent-looking toolchain change could cause a > regression without us noticing. > > Should we find someone to do the validator change, if only to catch a simple > subset of cases, like disallowing CALL instructions? That sounds reasonable. For simple things like catching call instructions, we could even implement something with objdump. We could even check for 'push rbp' that way. https://codereview.chromium.org/940993003/diff/230001/src/untrusted/irt/frame... src/untrusted/irt/frame_info_stubs.c:16: void __register_frame_info(void *begin, void *ob) {} On 2015/03/19 20:54:48, Mark Seaborn wrote: > When linking nexes with nacl-clang, do we always use the variant of crtbegin.o > that pulls these in? Does nacl-clang always pull in these functions from > libgcc_eh? Yes; there is currently only one variant, pnacl/support/clang_direct/crtbegin.c, which calls these from a global contstructor. The unwinder/libgcc_eh is the only implementation I know of. https://codereview.chromium.org/940993003/diff/230001/src/untrusted/irt/irt.gyp File src/untrusted/irt/irt.gyp (right): https://codereview.chromium.org/940993003/diff/230001/src/untrusted/irt/irt.g... src/untrusted/irt/irt.gyp:46: # On x86-64 we build the IRT with sandbox base-address hiding. This means that On 2015/03/19 20:54:48, Mark Seaborn wrote: > Nit: indent comment Done. https://codereview.chromium.org/940993003/diff/230001/src/untrusted/irt/irt.g... src/untrusted/irt/irt.gyp:103: 'sources': ['<@(irt_sources)','<@(stub_sources)'], On 2015/03/19 20:54:48, Mark Seaborn wrote: > Nit: add space after comma Done. https://codereview.chromium.org/940993003/diff/230001/src/untrusted/irt/irt.g... src/untrusted/irt/irt.gyp:109: 'stub_sources' : [] On 2015/03/19 20:54:48, Mark Seaborn wrote: > Nit: would it be more idiomatic for the condition to add to "sources" rather > than overriding "stub_sources"? I tried that, but maybe I didn't get the gyp syntax correct. What would be the way to do that?
https://codereview.chromium.org/940993003/diff/230001/src/untrusted/irt/frame... File src/untrusted/irt/frame_info_stubs.c (right): https://codereview.chromium.org/940993003/diff/230001/src/untrusted/irt/frame... src/untrusted/irt/frame_info_stubs.c:8: * On x86-64 we build the IRT with sandbox base-address hiding. This means that On 2015/03/19 21:28:30, Derek Schuff wrote: > On 2015/03/19 20:54:48, Mark Seaborn wrote: > > What's the plan for making sure this doesn't regress? If we stopped including > > this file in the build, would anything catch that? > > > > Shyam was going to implement some extra validator checks in parallel with your > > work on changing the IRT build, but he left. > > > > There are a lot of parts that must be right for the base-address-hiding to > work. > > I'm concerned that an innocent-looking toolchain change could cause a > > regression without us noticing. > > > > Should we find someone to do the validator change, if only to catch a simple > > subset of cases, like disallowing CALL instructions? > > That sounds reasonable. > For simple things like catching call instructions, we could even implement > something with objdump. We could even check for 'push rbp' that way. I've started a CL with a simple objdump-based lint check to keep this from regressing. It catches several bad constructs: https://codereview.chromium.org/1009533004 I don't think it needs to block this CL though; are there any other issues here?
dschuff@chromium.org changed reviewers: + mcgrathr@chromium.org
+mcgrathr Mark's on vacation, can you look for src/untrusted/irt OWNERS?
LGTM https://codereview.chromium.org/940993003/diff/230001/src/untrusted/irt/frame... File src/untrusted/irt/frame_info_stubs.c (right): https://codereview.chromium.org/940993003/diff/230001/src/untrusted/irt/frame... src/untrusted/irt/frame_info_stubs.c:8: * On x86-64 we build the IRT with sandbox base-address hiding. This means that On 2015/03/24 01:27:19, Derek Schuff wrote: > On 2015/03/19 21:28:30, Derek Schuff wrote: > > On 2015/03/19 20:54:48, Mark Seaborn wrote: > > > What's the plan for making sure this doesn't regress? If we stopped > including > > > this file in the build, would anything catch that? > > > > > > Shyam was going to implement some extra validator checks in parallel with > your > > > work on changing the IRT build, but he left. > > > > > > There are a lot of parts that must be right for the base-address-hiding to > > work. > > > I'm concerned that an innocent-looking toolchain change could cause a > > > regression without us noticing. > > > > > > Should we find someone to do the validator change, if only to catch a simple > > > subset of cases, like disallowing CALL instructions? > > > > That sounds reasonable. > > For simple things like catching call instructions, we could even implement > > something with objdump. We could even check for 'push rbp' that way. > > I've started a CL with a simple objdump-based lint check to keep this from > regressing. It catches several bad constructs: > https://codereview.chromium.org/1009533004 > I don't think it needs to block this CL though; are there any other issues here? Thanks, that would be useful. It's not as good as a validator change, which would give us some extra security hardening against bugs in the translator, but it's an improvement on the current lack of checks. https://codereview.chromium.org/940993003/diff/250001/build/untrusted.gypi File build/untrusted.gypi (right): https://codereview.chromium.org/940993003/diff/250001/build/untrusted.gypi#ne... build/untrusted.gypi:1463: # The IRT must be built using LLVM's assembler on x86-64 to preserve sandbox Nit: wrap to 80 chars https://codereview.chromium.org/940993003/diff/250001/toolchain_build/pnacl_s... File toolchain_build/pnacl_sandboxed_translator.py (right): https://codereview.chromium.org/940993003/diff/250001/toolchain_build/pnacl_s... toolchain_build/pnacl_sandboxed_translator.py:32: naclclang_packages = ['newlib_%s' % arch for arch in ['i686', 'x86_64','arm']] Nit: add space after ',' (but then wrap to 80 chars!)
https://codereview.chromium.org/940993003/diff/250001/build/untrusted.gypi File build/untrusted.gypi (right): https://codereview.chromium.org/940993003/diff/250001/build/untrusted.gypi#ne... build/untrusted.gypi:1463: # The IRT must be built using LLVM's assembler on x86-64 to preserve sandbox On 2015/03/25 08:37:00, Mark Seaborn wrote: > Nit: wrap to 80 chars Done. https://codereview.chromium.org/940993003/diff/250001/toolchain_build/pnacl_s... File toolchain_build/pnacl_sandboxed_translator.py (right): https://codereview.chromium.org/940993003/diff/250001/toolchain_build/pnacl_s... toolchain_build/pnacl_sandboxed_translator.py:32: naclclang_packages = ['newlib_%s' % arch for arch in ['i686', 'x86_64','arm']] On 2015/03/25 08:37:00, Mark Seaborn wrote: > Nit: add space after ',' (but then wrap to 80 chars!) (Busted!) Done.
The CQ bit was checked by dschuff@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jvoung@chromium.org, mseaborn@chromium.org Link to the patchset: https://codereview.chromium.org/940993003/#ps290001 (title: "format nits")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/940993003/290001
Message was sent while issue was closed.
Committed patchset #16 (id:290001) manually as 316f3e53bfcc34e4a137abbce02d890e3ea968ce (presubmit successful).
Message was sent while issue was closed.
A revert of this CL (patchset #16 id:290001) has been created in https://codereview.chromium.org/1036513005/ by dschuff@chromium.org. The reason for reverting is: F%*$^ windows XP. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
