Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10828060:
Use the ARM assembly code in mpi_arm.c for iOS. (Closed)
|
Created:
8 years, 4 months ago by wtc Modified:
7 years, 10 months ago CC:
chromium-reviews Visibility:
Public. |
DescriptionUse the ARM assembly code in mpi_arm.c for iOS.
For Mac OS X and iOS, use a forced include file
(mozilla/security/nss/lib/freebl/build_config_mac.h) to define the
processor architecture specific macros used by NSS's lib/freebl
code.
R=droger@chromium.org,mark@chromium.org,rsleevi@chromium.org
BUG=139432
TEST=no build error or net_unittests failure. Visit https://www.google.com/
on iOS.
Patch Set 1 #
Total comments: 2
Patch Set 2 : Make changes suggested by rsleevi and stuartmorgan #
Total comments: 4
Patch Set 3 : Add mpi_mac.c for including the appropriate assembly code file for Mac/iOS #
Total comments: 11
Patch Set 4 : Sync with tip of tree and use -include <forced_include_header> #
Total comments: 6
Patch Set 5 : Rename mpi_mac.c to mpi_arm_mac.c. Add comments. #
Total comments: 14
Patch Set 6 : Improve nss.gyp changes. Describe new files in README.chromium. #
Total comments: 7
Patch Set 7 : Define the forced_include_file variable in the nss_static target. Exclude mpi_arm.c from sources un… #Patch Set 8 : List build_config_mac.h in the sources block #
Messages
Total messages: 35 (0 generated)
droger: please review and test this patch for iOS device and Simulator builds. mark: please review. https://chromiumcodereview.appspot.com/10828060/diff/1/nss/nss.gyp File nss/nss.gyp (right): https://chromiumcodereview.appspot.com/10828060/diff/1/nss/nss.gyp#newcode1089 nss/nss.gyp:1089: 'GCC_PREPROCESSOR_DEFINITIONS[arch=armv7]': [ Is this the right way to test for a build that targets an iOS device (as opposed to the Simulator)?
Stuart, can you answer Wan-Teh’s question about targeting iOS devices only, and not the simulator?
drive by comment. The changes to GCC_PREPROCESSOR_DEFINITIONS looks correct to me - For simulator builds, arch should be i386 and TARGET_IPHONE_SIMULATOR should be defined. arch should only be arm7 for 'real' builds. http://codereview.chromium.org/10828060/diff/1/nss/nss.gyp File nss/nss.gyp (right): http://codereview.chromium.org/10828060/diff/1/nss/nss.gyp#newcode1053 nss/nss.gyp:1053: 'mozilla/security/nss/lib/freebl/mpi/mpi_arm.c', nit: It would seem that either line 1050 should be moved to an explicit sources!, or the sources! should exclude _arm files, since there is only one platform-specific file for each (mpi_arm.c/mpi_amd64.c)
On 2012/07/27 23:31:58, wtc wrote: > https://chromiumcodereview.appspot.com/10828060/diff/1/nss/nss.gyp#newcode1089 > nss/nss.gyp:1089: 'GCC_PREPROCESSOR_DEFINITIONS[arch=armv7]': [ > > Is this the right way to test for a build that targets > an iOS device (as opposed to the Simulator)? I think the generalized way is sdk=iphoneos*, but you don't actually want a generalized device since it's ARM assembly. Perhaps arch=arm* would be more future-proof though (assuming that works; I'm guessing it does since sdk allows wildcards)
Please review and test patch set 2. arch=arm* works. Thank you for the suggestion. https://chromiumcodereview.appspot.com/10828060/diff/5001/nss/nss.gyp File nss/nss.gyp (right): https://chromiumcodereview.appspot.com/10828060/diff/5001/nss/nss.gyp#newcode... nss/nss.gyp:1085: ], I need help with GYP here. I want to exclude the source file 'mozilla/security/nss/lib/freebl/mpi/mpi_arm.c' when we are building for arch=i386 or arch=x86_64 on "mac" and "ios", but I don't know how to do that. https://chromiumcodereview.appspot.com/10828060/diff/5001/nss/nss.gyp#newcode... nss/nss.gyp:1107: ], mpi_arm.c contains GCC inline assembly code, so it is correct to simply exclude this file if OS=="win".
https://chromiumcodereview.appspot.com/10828060/diff/5001/nss/nss.gyp File nss/nss.gyp (right): https://chromiumcodereview.appspot.com/10828060/diff/5001/nss/nss.gyp#newcode... nss/nss.gyp:1085: ], On 2012/07/30 23:20:53, wtc wrote: > > I need help with GYP here. > > I want to exclude the source file > 'mozilla/security/nss/lib/freebl/mpi/mpi_arm.c' > when we are building for arch=i386 or arch=x86_64 on "mac" > and "ios", but I don't know how to do that. GYP (by way of XCode limitations) doesn't really support configuration-specific source files. We have this same problem elsewhere with zlib optimizations. One possible solution would be to define a foo.c file that conditionally #included mpi/mpi_arm.c if TARGET_CPU_ARM (based on TargetConditionals.h) The downside with such an approach would be that it wouldn't be fully dependency tracked (Ninja/Make would track it, XCode wouldn't)
https://chromiumcodereview.appspot.com/10828060/diff/5001/nss/nss.gyp File nss/nss.gyp (right): https://chromiumcodereview.appspot.com/10828060/diff/5001/nss/nss.gyp#newcode... nss/nss.gyp:1085: ], As Ryan points out, the only option is to always compile a .c/.cc/.m/.mm file and have #ifdefs turn specific code you want on and off. A single file that just has #ifdefs and #includes is fine.
rsleevi,mark: thank you for the suggestion. Please review patch set 3. justincohen: could you test this patch on an iOS device? Please visit a few https:// sites to exercise the assembly code. Thanks. https://chromiumcodereview.appspot.com/10828060/diff/11001/nss/mozilla/securi... File nss/mozilla/security/nss/lib/freebl/mpi/mpi_mac.c (right): https://chromiumcodereview.appspot.com/10828060/diff/11001/nss/mozilla/securi... nss/mozilla/security/nss/lib/freebl/mpi/mpi_mac.c:41: #endif Thinking ahead, I realized that the assembly code files for the other two processor architectures are .s files. I don't think this file (a .c file) can include .s files. So we'll need to add another .s file for Mac, and I believe that .s file cannot be named mpi_mac.s.
https://chromiumcodereview.appspot.com/10828060/diff/11001/nss/nss.gyp File nss/nss.gyp (right): https://chromiumcodereview.appspot.com/10828060/diff/11001/nss/nss.gyp#newcod... nss/nss.gyp:1070: 'xcode_settings': { I just discovered that this entire xcode_settings section is ignored by Ninja. Can you suggest a solution? Thanks.
On 2012/08/02 02:47:08, wtc wrote: > I just discovered that this entire xcode_settings section > is ignored by Ninja. Can you suggest a solution? Thanks. iOS builds don't support ninja.
https://chromiumcodereview.appspot.com/10828060/diff/11001/nss/mozilla/securi... File nss/mozilla/security/nss/lib/freebl/mpi/mpi_arm.c (right): https://chromiumcodereview.appspot.com/10828060/diff/11001/nss/mozilla/securi... nss/mozilla/security/nss/lib/freebl/mpi/mpi_arm.c:43: /* 16-bit thumb doesn't work inlined assember version */ This doesn’t quite seem like a sentence. https://chromiumcodereview.appspot.com/10828060/diff/11001/nss/mozilla/securi... nss/mozilla/security/nss/lib/freebl/mpi/mpi_arm.c:111: if (!a_len) Just curious why you’re handling this in C code here and in s_mpv_sqr_add_prop. https://chromiumcodereview.appspot.com/10828060/diff/11001/nss/mozilla/securi... File nss/mozilla/security/nss/lib/freebl/mpi/mpi_mac.c (right): https://chromiumcodereview.appspot.com/10828060/diff/11001/nss/mozilla/securi... nss/mozilla/security/nss/lib/freebl/mpi/mpi_mac.c:41: #endif wtc wrote: > Thinking ahead, I realized that the assembly code files for > the other two processor architectures are .s files. I don't > think this file (a .c file) can include .s files. So we'll > need to add another .s file for Mac, and I believe that .s > file cannot be named mpi_mac.s. If you eliminate the two bits of C code in the other file, you can easily make it an .s file too. You don’t give up the ability to use preprocessor directives in an .s file on the Mac. On other platforms, you do, but if you name it with a capital .S, you can continue to use preprocessor directives. .S also works on the Mac. https://chromiumcodereview.appspot.com/10828060/diff/11001/nss/nss.gyp File nss/nss.gyp (right): https://chromiumcodereview.appspot.com/10828060/diff/11001/nss/nss.gyp#newcod... nss/nss.gyp:1070: 'xcode_settings': { wtc wrote: > I just discovered that this entire xcode_settings section > is ignored by Ninja. Can you suggest a solution? Thanks. I’m don’t think Ninja supports the [arch=] or [sdk=] construct at all yet. Nico or Robert would know for sure.
https://chromiumcodereview.appspot.com/10828060/diff/11001/nss/nss.gyp File nss/nss.gyp (right): https://chromiumcodereview.appspot.com/10828060/diff/11001/nss/nss.gyp#newcod... nss/nss.gyp:1070: 'xcode_settings': { On 2012/08/02 14:50:34, Mark Mentovai wrote: > wtc wrote: > > I just discovered that this entire xcode_settings section > > is ignored by Ninja. Can you suggest a solution? Thanks. > > I’m don’t think Ninja supports the [arch=] or [sdk=] construct at all yet. Nico > or Robert would know for sure. Correct, that's not implemented for ninja (http://crbug.com/122592).
I compiled and ran the patch on a device (iPhone 4S). I did not see any nss unittests. I ran the crypto and net unittest, and browsed to gmail. Are there any other thing that I could test for you? Any performance test?
https://codereview.chromium.org/10828060/diff/11001/nss/mozilla/security/nss/... File nss/mozilla/security/nss/lib/freebl/mpi/mpi_arm.c (right): https://codereview.chromium.org/10828060/diff/11001/nss/mozilla/security/nss/... nss/mozilla/security/nss/lib/freebl/mpi/mpi_arm.c:92: "adc r4, #0\n" When we tried building this on trunk we had a syntax error here (and the same error for all the uses of the 'adc' instruction). We solved it by changing it to: "adc r4, r4, #0\n" (additional r4 parameter)
wtc: Ping on this?
https://codereview.chromium.org/10828060/diff/11001/nss/mozilla/security/nss/... File nss/mozilla/security/nss/lib/freebl/mpi/mpi_arm.c (right): https://codereview.chromium.org/10828060/diff/11001/nss/mozilla/security/nss/... nss/mozilla/security/nss/lib/freebl/mpi/mpi_arm.c:43: /* 16-bit thumb doesn't work inlined assember version */ On 2012/08/02 14:50:34, Mark Mentovai wrote: > This doesn’t quite seem like a sentence. This file was contributed by someone else. I'll ask him what he meant here. https://codereview.chromium.org/10828060/diff/11001/nss/nss.gyp File nss/nss.gyp (right): https://codereview.chromium.org/10828060/diff/11001/nss/nss.gyp#newcode1090 nss/nss.gyp:1090: 'GCC_PREPROCESSOR_DEFINITIONS[arch=arm*]': [ I remember this CL is blocked by my inability to support Ninja builds. Ninja doesn't support GCC_PREPROCESSOR_DEFINITIONS (https://code.google.com/p/chromium/issues/detail?id=122592). I could be wrong though. I may have run into the Ninja limitation for a different reason (I also want to use x86 SSE2 assembly code on Mac OS X).
This is correct. ninja doesn’t support GCC_PREPROCESSOR_DEFINITIONS or the bracketed [arch=] syntax. I recently did a 64-bit build of Chrome on the Mac, and when I tried it with ninja, the only use of GCC_PREPROCESSOR_DEFINITIONS or [arch=] that caused a build break was in third_party/nss/nss.gyp. Would it be possible to make NSS pick up these definitions from a common header file? We could make that header use #if logic along with compiler-predefined macros to set the NSS_* macros up as needed. That would be best and it would require minimal changes to ninja. We’d then deprecate GCC_PREPROCESSOR_DEFINITIONS and [arch=] syntax and make GYP produce an error upon encountering either.
https://codereview.chromium.org/10828060/diff/11001/nss/nss.gyp File nss/nss.gyp (right): https://codereview.chromium.org/10828060/diff/11001/nss/nss.gyp#newcode1090 nss/nss.gyp:1090: 'GCC_PREPROCESSOR_DEFINITIONS[arch=arm*]': [ On 2012/11/26 22:22:44, wtc wrote: > > I remember this CL is blocked by my inability to support > Ninja builds. Ninja doesn't support > GCC_PREPROCESSOR_DEFINITIONS > (https://code.google.com/p/chromium/issues/detail?id=122592). > > I could be wrong though. I may have run into the Ninja > limitation for a different reason (I also want to use > x86 SSE2 assembly code on Mac OS X). For iOS, we should be able to map this into an OS=="ios" definition. iOS builds do not support Ninja, so it does not seem critical (for now) that this is XCode only. This is different than the x86 SSE2 code, which we would want to properly support on both XCode and Ninja.
Please review patch set 4. Sorry about the delay. I just got my Mac workstation back. I've reduced the reviewers to just droger, mark, and rsleevi. There is a Ninja GYP generator question that thakis can answer, but mark or rsleevi should be able to answer that question, too. https://chromiumcodereview.appspot.com/10828060/diff/20001/nss/nss.gyp File nss/nss.gyp (right): https://chromiumcodereview.appspot.com/10828060/diff/20001/nss/nss.gyp#newcod... nss/nss.gyp:1065: 'mozilla/security/nss/lib/freebl/mpi/mpi_amd64.c', This is just a cleanup. I want to exclude amd64 files using narrower rules. https://chromiumcodereview.appspot.com/10828060/diff/20001/nss/nss.gyp#newcod... nss/nss.gyp:1100: ], This is intentional broken code. I discovered that cflags blocks are ignored by Ninja, and Ninja actually honors the xcode_settings WARNING_CFLAGS and OTHER_CFLAGS blocks. What should I do? Just omit the cflags block?
https://codereview.chromium.org/10828060/diff/20001/nss/nss.gyp File nss/nss.gyp (right): https://codereview.chromium.org/10828060/diff/20001/nss/nss.gyp#newcode16 nss/nss.gyp:16: 'forced_include_file%': '<(DEPTH)/third_party/nss/mozilla/security/nss/lib/freebl/build_config_mac.h', You should not need to use <(DEPTH) here. Instead, you can use the path relative to the GYP file being used here. mozilla/security/nss/lib/freebl/build_config_mac.h and rely on the fact that it will be relativized (by virtue of the _file suffix) to the place where the compile is executing. https://codereview.chromium.org/10828060/diff/20001/nss/nss.gyp#newcode1100 nss/nss.gyp:1100: ], On 2013/01/29 04:37:54, wtc wrote: > > This is intentional broken code. I discovered that cflags > blocks are ignored by Ninja, and Ninja actually honors the > xcode_settings WARNING_CFLAGS and OTHER_CFLAGS blocks. > > What should I do? Just omit the cflags block? I believe this is because ninja is matching the XCode behaviour, which is to only respect OTHER_CFLAGS.
Please review patch set 5. I tested the patch on: - Windows, with msvs - Mac OS X, with ninja and make - iOS, with xcode To my surprise, my experiments showed that xcode, ninja, and make all recompile NSS when I touch the forced include file. I don't know how they figured out that dependency just from the -include flag in the xcode_settings OTHER_CFLAGS block. Amazing! https://codereview.chromium.org/10828060/diff/20001/nss/nss.gyp File nss/nss.gyp (right): https://codereview.chromium.org/10828060/diff/20001/nss/nss.gyp#newcode16 nss/nss.gyp:16: 'forced_include_file%': '<(DEPTH)/third_party/nss/mozilla/security/nss/lib/freebl/build_config_mac.h', On 2013/01/29 19:02:47, Ryan Sleevi wrote: > You should not need to use <(DEPTH) here. Instead, you can use the path relative > to the GYP file being used here. > > mozilla/security/nss/lib/freebl/build_config_mac.h > > and rely on the fact that it will be relativized (by virtue of the _file suffix) > to the place where the compile is executing. This works with Xcode, but not with make and ninja. With make, I get: In file included from <built-in>:156: <command line>:52:10: fatal error: 'mozilla/security/nss/lib/freebl/build_config_mac.h' file not found With ninja, I get: [894/1805] CC obj/third_party/nss/mozill...security/nss/lib/base/nss_static.error.o FAILED: ../../third_party/llvm-build/Release+Asserts/bin/clang -MMD -MF obj/third_party/nss/mozilla/security/nss/lib/base/nss_static.error.o.d -DCHROMIUM_BUILD -DUSE_LIBJPEG_TURBO=1 -DENABLE_ONE_CLICK_SIGNIN -DENABLE_REMOTING=1 -DENABLE_WEBRTC=1 -DENABLE_PEPPER_THREADING -DENABLE_CONFIGURATION_POLICY -DENABLE_INPUT_SPEECH -DENABLE_NOTIFICATIONS -DENABLE_HIDPI=1 -DENABLE_GPU=1 -DENABLE_EGLIMAGE=1 -DUSE_SKIA=1 -DENABLE_TASK_MANAGER=1 -DENABLE_WEB_INTENTS=1 -DENABLE_EXTENSIONS=1 -DENABLE_PLUGIN_INSTALLATION=1 -DENABLE_PLUGINS=1 -DENABLE_SESSION_SERVICE=1 -DENABLE_THEMES=1 -DENABLE_BACKGROUND=1 -DENABLE_AUTOMATION=1 -DENABLE_GOOGLE_NOW=1 -DENABLE_LANGUAGE_DETECTION=1 -DENABLE_PRINTING=1 -DENABLE_CAPTIVE_PORTAL_DETECTION=1 -DENABLE_APP_LIST=1 -DENABLE_MANAGED_USERS=1 -DMP_API_COMPATIBLE -DNSS_DISABLE_DBM -DNSS_ENABLE_ECC -DNSS_STATIC -DNSS_USE_STATIC_LIBS -DRIJNDAEL_INCLUDE_TABLES '-DSHLIB_VERSION="3"' '-DSOFTOKEN_SHLIB_VERSION="3"' -DUSE_UTIL_DIRECTLY -DNSS_DISABLE_ROOT_CERTS -DNSS_DISABLE_LIBPKIX -DXP_UNIX -DDARWIN -DHAVE_STRERROR -DHAVE_BSD_FLOCK '-DSHLIB_SUFFIX="dylib"' '-DSHLIB_PREFIX="lib"' '-DSOFTOKEN_LIB_NAME="libsoftokn3.dylib"' -DNSPR_STATIC -DDYNAMIC_ANNOTATIONS_ENABLED=1 -DWTF_USE_DYNAMIC_ANNOTATIONS=1 -DDEBUG -D_DEBUG -I../../third_party/nss/mozilla/security/nss/lib/base -I../../third_party/nss/mozilla/security/nss/lib/certdb -I../../third_party/nss/mozilla/security/nss/lib/certhigh -I../../third_party/nss/mozilla/security/nss/lib/cryptohi -I../../third_party/nss/mozilla/security/nss/lib/dev -I../../third_party/nss/mozilla/security/nss/lib/freebl -I../../third_party/nss/mozilla/security/nss/lib/freebl/ecl -I../../third_party/nss/mozilla/security/nss/lib/freebl/mpi -I../../third_party/nss/mozilla/security/nss/lib/nss -I../../third_party/nss/mozilla/security/nss/lib/pk11wrap -I../../third_party/nss/mozilla/security/nss/lib/pkcs7 -I../../third_party/nss/mozilla/security/nss/lib/pki -I../../third_party/nss/mozilla/security/nss/lib/smime -I../../third_party/nss/mozilla/security/nss/lib/softoken -I../../third_party/nss/mozilla/security/nss/lib/ssl -I../../third_party/nss/mozilla/security/nss/lib/util -I../../third_party/nss/mozilla/nsprpub/pr/include -I../../third_party/nss/mozilla/nsprpub/lib/ds -I../../third_party/nss/mozilla/nsprpub/lib/libc/include -I../../third_party/sqlite -I../.. -isysroot /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.7.sdk -O0 -gdwarf-2 -fvisibility=hidden -Werror -Wnewline-eof -mmacosx-version-min=10.6 -arch i386 -Wendif-labels -Wno-unused-parameter -Wno-missing-field-initializers -Wheader-hygiene -Wno-c++11-narrowing -Wno-reserved-user-defined-literal -Wno-char-subscripts -Wno-unused-function -Wno-covered-switch-default -Wno-conversion -Wno-incompatible-pointer-types -Wno-logical-op-parentheses -Wno-switch -Wno-tautological-compare -std=c99 -Xclang -load -Xclang /Users/wtc/chrome5/src/tools/clang/scripts/../../../third_party/llvm-build/Release+Asserts/lib/libFindBadConstructs.dylib -Xclang -add-plugin -Xclang find-bad-constructs -Xclang -plugin-arg-find-bad-constructs -Xclang skip-virtuals-in-implementations -fcolor-diagnostics -fno-strict-aliasing -include mozilla/security/nss/lib/freebl/build_config_mac.h -fstack-protector-all -c ../../third_party/nss/mozilla/security/nss/lib/base/error.c -o obj/third_party/nss/mozilla/security/nss/lib/base/nss_static.error.o In file included from <built-in>:156: <command line>:52:10: fatal error: 'mozilla/security/nss/lib/freebl/build_config_mac.h' file not found #include "mozilla/security/nss/lib/freebl/build_config_mac.h" ^ 1 error generated. https://codereview.chromium.org/10828060/diff/20001/nss/nss.gyp#newcode1100 nss/nss.gyp:1100: ], Thanks. My experiments showed that this cflags block is ignored by the xcode, ninja, and make GYP generators. So I removed the cflags block. https://codereview.chromium.org/10828060/diff/26001/nss/mozilla/security/nss/... File nss/mozilla/security/nss/lib/freebl/mpi/mpi_arm_mac.c (right): https://codereview.chromium.org/10828060/diff/26001/nss/mozilla/security/nss/... nss/mozilla/security/nss/lib/freebl/mpi/mpi_arm_mac.c:6: * A wrapper file for mpi_arm.c on iOS and Mac OS X. I renamed this file mpi_arm_mac.c to suggest the fact that this is a wrapper file for mpi_arm.c. Strictly speaking only OS="ios" needs this file, but it is a simpler change to nss.gyp if I also use this file for Mac OS X.
https://codereview.chromium.org/10828060/diff/26001/nss/mozilla/security/nss/... File nss/mozilla/security/nss/lib/freebl/build_config_mac.h (right): https://codereview.chromium.org/10828060/diff/26001/nss/mozilla/security/nss/... nss/mozilla/security/nss/lib/freebl/build_config_mac.h:30: #define SHA_NO_LONG_LONG 1 These four macro definitions come from this upstream NSS makefile: http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/nss/lib/freebl/M... https://codereview.chromium.org/10828060/diff/26001/nss/scripts/nss-checkout.sh File nss/scripts/nss-checkout.sh (right): https://codereview.chromium.org/10828060/diff/26001/nss/scripts/nss-checkout.... nss/scripts/nss-checkout.sh:79: ! -name mpi_amd64.c ! -name mpi_arm.c ! -name mpi_x86_asm.c \ I added mpi_arm.c to the list of files we want to keep.
https://chromiumcodereview.appspot.com/10828060/diff/26001/nss/mozilla/securi... File nss/mozilla/security/nss/lib/freebl/build_config_mac.h (right): https://chromiumcodereview.appspot.com/10828060/diff/26001/nss/mozilla/securi... nss/mozilla/security/nss/lib/freebl/build_config_mac.h:1: /* This Source Code Form is subject to the terms of the Mozilla Public Is this part of upstream NSS now? If not, a README somewhere probably needs to be updated to reflect the local addition. https://chromiumcodereview.appspot.com/10828060/diff/26001/nss/mozilla/securi... nss/mozilla/security/nss/lib/freebl/build_config_mac.h:17: #define i386 1 This should be predefined this way by the compiler driver. https://chromiumcodereview.appspot.com/10828060/diff/26001/nss/nss.gyp File nss/nss.gyp (right): https://chromiumcodereview.appspot.com/10828060/diff/26001/nss/nss.gyp#newcode16 nss/nss.gyp:16: 'forced_include_file%': '<(DEPTH)/third_party/nss/mozilla/security/nss/lib/freebl/build_config_mac.h', You shouldn’t need a <(DEPTH)-rooted path. You should be able to specify this ias mozilla/security/…, relative to the directory of the .gyp file. https://chromiumcodereview.appspot.com/10828060/diff/26001/nss/nss.gyp#newcod... nss/nss.gyp:1081: 'mozilla/security/nss/lib/freebl/mpi/mpi_arm.c', You said that mpi_arm_mac.c existed to avoid a more complicated change in the .gyp file, but now that you’re excluding mpi_arm.c (to avoid compiling it twice, once on its own and once as part of mpi_arm_mac.c), you’ve got a freaky thing going on in the .gyp file. I think it would be better to get rid of mpi_arm_mac.cc and add the necessary GYP bits to build mpi_arm.cc only on iOS (or other ARM platforms?) https://chromiumcodereview.appspot.com/10828060/diff/26001/nss/nss.gyp#newcod... nss/nss.gyp:1109: # mpi_arm.c contains GCC inline assembly code. Don’t you really want to do this whenever OS!="mac" or OS!="ios", not just when OS=="win"? In conjunction with the other suggestion, this will probably all change.
Please review patch set 6. I made the changes that mark suggested. Please let me know if there is a better solution for compiling mpi_arm.c only for iOS and only for the device (arm) target. Thanks. https://chromiumcodereview.appspot.com/10828060/diff/26001/nss/mozilla/securi... File nss/mozilla/security/nss/lib/freebl/build_config_mac.h (right): https://chromiumcodereview.appspot.com/10828060/diff/26001/nss/mozilla/securi... nss/mozilla/security/nss/lib/freebl/build_config_mac.h:1: /* This Source Code Form is subject to the terms of the Mozilla Public On 2013/01/30 20:37:54, Mark Mentovai wrote: > Is this part of upstream NSS now? If not, a README somewhere probably needs to > be updated to reflect the local addition. Ah, I forgot about this. Thanks for the reminder. https://chromiumcodereview.appspot.com/10828060/diff/26001/nss/mozilla/securi... nss/mozilla/security/nss/lib/freebl/build_config_mac.h:17: #define i386 1 On 2013/01/30 20:37:54, Mark Mentovai wrote: > This should be predefined this way by the compiler driver. This is a faithful emulation of the upstream NSS makefile: http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/coreconf/Darwin.... I have been wanting to remove the -Di386 and -Dppc from upstream NSS, but never got around to it. https://chromiumcodereview.appspot.com/10828060/diff/26001/nss/nss.gyp File nss/nss.gyp (right): https://chromiumcodereview.appspot.com/10828060/diff/26001/nss/nss.gyp#newcode16 nss/nss.gyp:16: 'forced_include_file%': '<(DEPTH)/third_party/nss/mozilla/security/nss/lib/freebl/build_config_mac.h', On 2013/01/30 20:37:54, Mark Mentovai wrote: > You shouldn’t need a <(DEPTH)-rooted path. You should be able to specify this > ias mozilla/security/…, relative to the directory of the .gyp file. Both you and Ryan suggested this, but my experiments showed this only works with the xcode GYP generator, but not with the ninja and make GYP generators. Please se the ninja and make compile error messages in my comment at https://codereview.chromium.org/10828060/diff/20001/nss/nss.gyp#newcode16 I wonder if I made a mistake. Could it be that I define this variable outside the target that uses the variable? https://chromiumcodereview.appspot.com/10828060/diff/26001/nss/nss.gyp#newcod... nss/nss.gyp:1081: 'mozilla/security/nss/lib/freebl/mpi/mpi_arm.c', On 2013/01/30 20:37:54, Mark Mentovai wrote: > You said that mpi_arm_mac.c existed to avoid a more complicated change in the > .gyp file, ... 1. A wrapper file for mpi_arm.c is still necessary for iOS because only the device (arm) target (but not the simulator target) can use mpi_arm.c. I assume it is still not possible to specify arch specific source files in Xcode. 2. Strictly speaking I just need to use the wrapper file for OS=="ios". But it is convenient to treat mac and ios in the same way in this GYP file. If I name the wrapper file mpi_arm_ios.c and only use it for iOS, the changes to this GYP file will be larger. That's what I wanted to avoid.
We should figure out if this ninja working directory thing is in fact what’s causing the bug, if ninja is doing it intentionally, and if anything will break if we fix it. https://chromiumcodereview.appspot.com/10828060/diff/26001/nss/nss.gyp File nss/nss.gyp (right): https://chromiumcodereview.appspot.com/10828060/diff/26001/nss/nss.gyp#newcode16 nss/nss.gyp:16: 'forced_include_file%': '<(DEPTH)/third_party/nss/mozilla/security/nss/lib/freebl/build_config_mac.h', Hmm. Looks like maybe ninja isn’t changing to the right directory before starting the compiler. https://chromiumcodereview.appspot.com/10828060/diff/31001/nss/mozilla/securi... File nss/mozilla/security/nss/lib/freebl/build_config_mac.h (right): https://chromiumcodereview.appspot.com/10828060/diff/31001/nss/mozilla/securi... nss/mozilla/security/nss/lib/freebl/build_config_mac.h:1: /* This Source Code Form is subject to the terms of the Mozilla Public Is this really an MPL file if it’s our own local addition? https://chromiumcodereview.appspot.com/10828060/diff/31001/nss/mozilla/securi... File nss/mozilla/security/nss/lib/freebl/mpi/mpi_arm_mac.c (right): https://chromiumcodereview.appspot.com/10828060/diff/31001/nss/mozilla/securi... nss/mozilla/security/nss/lib/freebl/mpi/mpi_arm_mac.c:1: /* This Source Code Form is subject to the terms of the Mozilla Public Same.
https://codereview.chromium.org/10828060/diff/31001/nss/nss.gyp File nss/nss.gyp (right): https://codereview.chromium.org/10828060/diff/31001/nss/nss.gyp#newcode16 nss/nss.gyp:16: 'forced_include_file%': '<(DEPTH)/third_party/nss/mozilla/security/nss/lib/freebl/build_config_mac.h', You don't need to use the "%" suffix here. That is used to indicate an optional variable that may be set by another layer (eg: build/common.gypi). This variable is entirely local to the file. You should also be able to remove this variable into the nss target, where it lives. There's no need to place it at the top like this. Line 1082 should suffice. https://codereview.chromium.org/10828060/diff/31001/nss/nss.gyp#newcode1081 nss/nss.gyp:1081: 'mozilla/security/nss/lib/freebl/mpi/mpi_arm.c', Is there ever a situation where we actually want to include mpi_arm.c? It seems like this should just be an unconditional 'sources!' (the benefit to listing in 'sources' to begin with is so that it still shows up in IDEs, even if its not directly compiled). Perhaps ~ line 1040?
mark,rsleevi: thanks for the comments. I will wait for your answers to my questions before uploading a new patch set. Thanks. https://chromiumcodereview.appspot.com/10828060/diff/31001/nss/mozilla/securi... File nss/mozilla/security/nss/lib/freebl/build_config_mac.h (right): https://chromiumcodereview.appspot.com/10828060/diff/31001/nss/mozilla/securi... nss/mozilla/security/nss/lib/freebl/build_config_mac.h:1: /* This Source Code Form is subject to the terms of the Mozilla Public On 2013/01/30 23:20:32, Mark Mentovai wrote: > Is this really an MPL file if it’s our own local addition? This is our own local addition. I put MPL in it because it would allow us to build a universal binary of upstream NSS in a single pass. https://chromiumcodereview.appspot.com/10828060/diff/31001/nss/mozilla/securi... File nss/mozilla/security/nss/lib/freebl/mpi/mpi_arm_mac.c (right): https://chromiumcodereview.appspot.com/10828060/diff/31001/nss/mozilla/securi... nss/mozilla/security/nss/lib/freebl/mpi/mpi_arm_mac.c:1: /* This Source Code Form is subject to the terms of the Mozilla Public This is our own addition. This file doesn't seem useful to the upstream NSS, which uses makefiles and has no problem with target arch specific source files. But it doesn't seem bad to put MPL in such a trivial file. https://chromiumcodereview.appspot.com/10828060/diff/31001/nss/nss.gyp File nss/nss.gyp (right): https://chromiumcodereview.appspot.com/10828060/diff/31001/nss/nss.gyp#newcod... nss/nss.gyp:1081: 'mozilla/security/nss/lib/freebl/mpi/mpi_arm.c', On 2013/01/30 23:23:35, Ryan Sleevi wrote: > Is there ever a situation where we actually want to include mpi_arm.c? It seems > like this should just be an unconditional 'sources!' (the benefit to listing in > 'sources' to begin with is so that it still shows up in IDEs, even if its not > directly compiled). Perhaps ~ line 1040? mpi_arm.c could be used directly for Linux/arm. So I am preparing for the future when we may want to build NSS on Linux. Should I deal with that when we cross that bridge and just do an unconditional 'sources!' now?
https://chromiumcodereview.appspot.com/10828060/diff/26001/nss/nss.gyp File nss/nss.gyp (right): https://chromiumcodereview.appspot.com/10828060/diff/26001/nss/nss.gyp#newcode16 nss/nss.gyp:16: 'forced_include_file%': '<(DEPTH)/third_party/nss/mozilla/security/nss/lib/freebl/build_config_mac.h', On 2013/01/30 23:20:32, Mark Mentovai wrote: > Hmm. Looks like maybe ninja isn’t changing to the right directory before > starting the compiler. I run ninja inside the src directory, with this command: ninja -C out/Debug nss_static The clang command line issued by ninja shows that ninja is inside the out/Debug directory. ninja refers to the source file using a path like ../../third_party/nss/mozilla/security/nss/lib/certhigh/ocsp.c and specifies the output .o file using a path like -o obj/third_party/nss/mozilla/security/nss/lib/certhigh/nss_static.ocsp.o Therefore, if the current directory is out/Debug, -include mozilla/security/nss/lib/freebl/build_config_mac.h won't work.
On 2013/01/31 00:00:15, wtc wrote: > https://chromiumcodereview.appspot.com/10828060/diff/26001/nss/nss.gyp > File nss/nss.gyp (right): > > https://chromiumcodereview.appspot.com/10828060/diff/26001/nss/nss.gyp#newcode16 > nss/nss.gyp:16: 'forced_include_file%': > '<(DEPTH)/third_party/nss/mozilla/security/nss/lib/freebl/build_config_mac.h', > > On 2013/01/30 23:20:32, Mark Mentovai wrote: > > Hmm. Looks like maybe ninja isn’t changing to the right directory before > > starting the compiler. > > I run ninja inside the src directory, with this command: > > ninja -C out/Debug nss_static > > The clang command line issued by ninja shows that ninja > is inside the out/Debug directory. ninja refers to the source > file using a path like > ../../third_party/nss/mozilla/security/nss/lib/certhigh/ocsp.c > and specifies the output .o file using a path like > -o obj/third_party/nss/mozilla/security/nss/lib/certhigh/nss_static.ocsp.o > > Therefore, if the current directory is out/Debug, > -include mozilla/security/nss/lib/freebl/build_config_mac.h > won't work. Right, the generators are supposed to normalize the _file path to be relative to the path where the build system executes (eg: it's supposed to canonicalize the file as ../../mozilla/security/nss/lib/freebl/build_config_mac.h ). That's odd that it's not...
The _file path is supposed to be normalized to be relative to the .gyp file, and then the compilation is supposed to happen in that directory too. But I have a hazy memory of make not changing to the right directory before invoking the compiler, and ninja then inheriting that behavior.
Please review patch set 7. 1. I made the two changes to nss.gyp that rsleevi suggested. 2. I didn't change the MPL 2 header in the two _mac files I added.
LGTM
wtc asked me by IM: > Hi. Should I list that forced include header in the 'sources' block? Yes, you can make that change and this will still be LGTM.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/wtc@chromium.org/10828060/24006
Message was sent while issue was closed.
Change committed as 179928 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
