Bug 190407 – CURRENT_ARCH should not be used in Run Script phase.

• Component: WebKit Misc.
• Status: RESOLVED FIXED

[Keith Rollin]

CURRENT_ARCH is used in a number of Xcode Run Script phases. However, CURRENT_ARCH is not well-defined during this phase (and may even have the value "undefined") since this phase is run just once per build rather than once per supported architecture. Migrate away from CURRENT_ARCH in favor of ARCHS, either by iterating over ARCHS and performing an operation for each value, or by picking the first entry in ARCHS and using that as a representative value.

[Radar WebKit Bug Importer]

<rdar://problem/45133556>

[Keith Rollin]

Created attachment 351901 [details]
Patch

[Keith Rollin]

Created attachment 351906 [details]
Fix post-last-build, pre-upload typo.

[Mark Lam]

Comment on attachment 351906 [details]
Fix post-last-build, pre-upload typo.

View in context: https://bugs.webkit.org/attachment.cgi?id=351906&action=review

> Source/JavaScriptCore/JavaScriptCore.xcodeproj/project.pbxproj:9975
> +			shellScript = "set -e\n\nmkdir -p \"${BUILT_PRODUCTS_DIR}/LLIntOffsets/${ARCHS}\"\n\n/usr/bin/env ruby \"${SRCROOT}/offlineasm/generate_offset_extractor.rb\" \"-I${BUILT_PRODUCTS_DIR}/DerivedSources/JavaScriptCore\" \"${SRCROOT}/llint/LowLevelInterpreter.asm\" \"${BUILT_PRODUCTS_DIR}/LLIntOffsets/${ARCHS}/LLIntDesiredOffsets.h\" \"${ARCHS} C_LOOP\"\n";

Is this correct?  What happens when we build for more than 1 ARCH e.g. ARCHS="arm64 arm64e"?  I think this will fail or do the wrong thing.

[Keith Rollin]

> What happens when we build for more than 1 ARCH e.g. ARCHS="arm64 arm64e"?

Then the directory path incorporates "arm64 arm64e" into itself. This is what I'm expecting, and it seems to work. The LLIntDesiredOffsets.h file contains versions of itself for each architecture and C_LOOP, so it's kind of universal for the given architectures. What failure do you imagine?

[Mark Lam]

(In reply to Keith Rollin from comment #5)
> > What happens when we build for more than 1 ARCH e.g. ARCHS="arm64 arm64e"?
> 
> Then the directory path incorporates "arm64 arm64e" into itself. This is
> what I'm expecting, and it seems to work. The LLIntDesiredOffsets.h file
> contains versions of itself for each architecture and C_LOOP, so it's kind
> of universal for the given architectures. What failure do you imagine?

You are correct.  I was mistaken.  Ignore me.

[Keith Rollin]

This patch is ready for review.

[Alexey Proskuryakov]

Comment on attachment 351906 [details]
Fix post-last-build, pre-upload typo.

View in context: https://bugs.webkit.org/attachment.cgi?id=351906&action=review

> Source/WebKitLegacy/ChangeLog:19
> +        * WebKitLegacy.xcodeproj/project.pbxproj: When generating
> +        WebKitLegacy.*.exp, generate both 32- and 64-bit versions for
> +        non-cocoa platforms.

Can you name those platforms here?

> Source/WebCore/DerivedSources.make:1047
> -	TARGET_TRIPLE_FLAGS=-target $(CURRENT_ARCH)-$(LLVM_TARGET_TRIPLE_VENDOR)-$(LLVM_TARGET_TRIPLE_OS_VERSION)$(LLVM_TARGET_TRIPLE_SUFFIX)
> +	WK_CURRENT_ARCH=$(word 1, $(ARCHS))
> +	TARGET_TRIPLE_FLAGS=-target $(WK_CURRENT_ARCH)-$(LLVM_TARGET_TRIPLE_VENDOR)-$(LLVM_TARGET_TRIPLE_OS_VERSION)$(LLVM_TARGET_TRIPLE_SUFFIX)

Why is this right? Do we even need architecture here, if it's not a faithful representation of what we are building for?

> Source/WebKitLegacy/WebKitLegacy.xcodeproj/project.pbxproj:-3288
> -				"$(SRCROOT)/mac/WebKit.exp",
> -				"$(SRCROOT)/ios/WebKit.iOS.exp",
> -				"$(SRCROOT)/mac/WebKit.mac.exp",
> -				"$(BUILT_PRODUCTS_DIR)/DerivedSources/WebKitLegacy/ReexportedWebCoreSymbols_$(CURRENT_ARCH).exp",

Without dependencies specified, I don't see how dependency tracing would work correctly. Also, I believe that these are necessary for the modern Xcode build system.

> Source/WebKitLegacy/mac/migrate-headers.sh:39
> +    for WK_CURRENT_ARCH in ${ARCHS}; do

How do we end up with ARCHS properly defined in this script, but not in others?

[Keith Rollin]

(In reply to Alexey Proskuryakov from comment #8)
> Can you name those platforms here?

My bad. I meant to say non-cocoatouch platforms. I was working from an xcconfig file that defined EXPORTED_SYMBOLS_FILE_cocoatouch if WK_COCOA_TOUCH was true. So that's why I used that term.

However, the build script I'm updating distinguishes between where WK_PLATFORM_NAME is "macosx" or not. Given that, I could change "32- and 64-bit versions for non-cocoa platforms" to 32- and 64-bit versions for macosx platform"

> > Source/WebCore/DerivedSources.make:1047
> > -	TARGET_TRIPLE_FLAGS=-target $(CURRENT_ARCH)-$(LLVM_TARGET_TRIPLE_VENDOR)-$(LLVM_TARGET_TRIPLE_OS_VERSION)$(LLVM_TARGET_TRIPLE_SUFFIX)
> > +	WK_CURRENT_ARCH=$(word 1, $(ARCHS))
> > +	TARGET_TRIPLE_FLAGS=-target $(WK_CURRENT_ARCH)-$(LLVM_TARGET_TRIPLE_VENDOR)-$(LLVM_TARGET_TRIPLE_OS_VERSION)$(LLVM_TARGET_TRIPLE_SUFFIX)
> 
> Why is this right? Do we even need architecture here, if it's not a faithful
> representation of what we are building for?

This triple is used when invoking $CC's preprocessor to determine the values of WTF_PLATFORM_IOS, WTF_PLATFORM_MAC, USE_APPLE_INTERNAL_SDK, and ENABLE_ORIENTATION_EVENTS. I don't think any of those are sensitive to architecture. At worst, we'd need an architecture that is representative or "close enough". That is, either one of x86_64 or i386 should be sufficient if both appeared in ARCHS. So picking the first one with which to invoke $CC should be OK.

At one point, on the theory that architecture was completely ignored, I tried passing in "undefined", but the compiler did not accept it.


> > Source/WebKitLegacy/WebKitLegacy.xcodeproj/project.pbxproj:-3288
> > -				"$(SRCROOT)/mac/WebKit.exp",
> > -				"$(SRCROOT)/ios/WebKit.iOS.exp",
> > -				"$(SRCROOT)/mac/WebKit.mac.exp",
> > -				"$(BUILT_PRODUCTS_DIR)/DerivedSources/WebKitLegacy/ReexportedWebCoreSymbols_$(CURRENT_ARCH).exp",
> 
> Without dependencies specified, I don't see how dependency tracing would
> work correctly. Also, I believe that these are necessary for the modern
> Xcode build system.

I've consulted with the XCBuild engineers on this. Currently, there is no way to specify dynamic sets of input or output files. This includes files whose names cannot be determined during the dependency generation stage at the start of the build (as is the case above with $CURRENT_ARCH), or directories whose contents need to be discovered.

Instead, they say to remove the input/output information. This causes XCBuild to assume the worst and essentially serialize that part of the build, giving up on any parallelization opportunities at that point.


> > Source/WebKitLegacy/mac/migrate-headers.sh:39
> > +    for WK_CURRENT_ARCH in ${ARCHS}; do
> 
> How do we end up with ARCHS properly defined in this script, but not in
> others?

ARCHS is fine. CURRENT_ARCH is the problematic variable.

[Keith Rollin]

Created attachment 352054 [details]
Made indicated ChangeLog wording change.

[WebKit Commit Bot]

Comment on attachment 352054 [details]
Made indicated ChangeLog wording change.

Clearing flags on attachment: 352054

Committed r237047: <https://trac.webkit.org/changeset/237047>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.