Bug 190407 - CURRENT_ARCH should not be used in Run Script phase.
Summary: CURRENT_ARCH should not be used in Run Script phase.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Keith Rollin
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-10-09 11:44 PDT by Keith Rollin
Modified: 2018-10-11 14:22 PDT (History)
13 users (show)

See Also:


Attachments
Patch (19.39 KB, patch)
2018-10-09 12:11 PDT, Keith Rollin
no flags Details | Formatted Diff | Diff
Fix post-last-build, pre-upload typo. (19.39 KB, patch)
2018-10-09 13:36 PDT, Keith Rollin
no flags Details | Formatted Diff | Diff
Made indicated ChangeLog wording change. (19.41 KB, patch)
2018-10-11 10:06 PDT, Keith Rollin
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Keith Rollin 2018-10-09 11:44:11 PDT
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.
Comment 1 Radar WebKit Bug Importer 2018-10-09 11:44:25 PDT
<rdar://problem/45133556>
Comment 2 Keith Rollin 2018-10-09 12:11:39 PDT
Created attachment 351901 [details]
Patch
Comment 3 Keith Rollin 2018-10-09 13:36:57 PDT
Created attachment 351906 [details]
Fix post-last-build, pre-upload typo.
Comment 4 Mark Lam 2018-10-09 13:40:16 PDT
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.
Comment 5 Keith Rollin 2018-10-09 13:44:27 PDT
> 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?
Comment 6 Mark Lam 2018-10-09 17:02:50 PDT
(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.
Comment 7 Keith Rollin 2018-10-10 10:47:57 PDT
This patch is ready for review.
Comment 8 Alexey Proskuryakov 2018-10-10 12:58:56 PDT
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?
Comment 9 Keith Rollin 2018-10-10 14:04:51 PDT
(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.
Comment 10 Keith Rollin 2018-10-11 10:06:32 PDT
Created attachment 352054 [details]
Made indicated ChangeLog wording change.
Comment 11 WebKit Commit Bot 2018-10-11 14:22:48 PDT
Comment on attachment 352054 [details]
Made indicated ChangeLog wording change.

Clearing flags on attachment: 352054

Committed r237047: <https://trac.webkit.org/changeset/237047>
Comment 12 WebKit Commit Bot 2018-10-11 14:22:50 PDT
All reviewed patches have been landed.  Closing bug.