Bug 237329 – [XCBuild] Emit a discovered dependency file from offlineasm

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Elliott Williams]

offlineasm does not track which files influence its compilation, so XCBuild cannot know when to schedule recompilations. By changing offlineasm emitting a discovered dependency files ("depfile"), Xcode can track included file dependencies in the same way that it does for clang-based sources.

[Elliott Williams]

rdar://89054989

[Elliott Williams]

Created attachment 453505 [details]
Integrate offlineasm using build rules and emit a depfile

[Elliott Williams]

Created attachment 453524 [details]
Integrate offlineasm using build rules and emit a depfile r2

Fix build errors

[Elliott Williams]

Created attachment 453552 [details]
Integrate offlineasm using build rules and emit a depfile r3

Fix building with multiple ARCHS. Prevent offlineasm from short-circuiting when the depfile is missing (this could happen if the depfile is deleted, but the generated header is not).

[Elliott Williams]

Created attachment 453553 [details]
Integrate offlineasm using build rules and emit a depfile r4

Typo

[Keith Miller]

Comment on attachment 453524 [details]
Integrate offlineasm using build rules and emit a depfile r2

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

I'm gonna assume the JavaScriptCore.xcodeproj/project.pbxproj changes are correct since I don't know how to read that directly but everything else looks good.

> Source/JavaScriptCore/offlineasm/asm.rb:419
> +        sources = ast.flatten.reduce(Set.new) { |files, node| files.add(node.codeOrigin.fileName) }

Can you put a FIXME here to add files to a set when executing the import step and link to a bugzilla. I think that would be more efficient since there may be tens of thousands of nodes per file and offlineasm is somewhat slow already.

> Source/JavaScriptCore/offlineasm/generate_offset_extractor.rb:90
> +
> +

Can you remove this extra whitespace?

> Source/JavaScriptCore/offlineasm/generate_offset_extractor.rb:95
> +    sources = ast.flatten.reduce(Set.new) { |files, node| files.add(node.codeOrigin.fileName) }

Ditto on FIXME.

> Source/JavaScriptCore/offlineasm/generate_settings_extractor.rb:75
> +    sources = prunedAST.flatten.reduce(Set.new) { |files, node| files.add(node.codeOrigin.fileName) }

Ditto on FIXME.

[Elliott Williams]

Created attachment 453560 [details]
Integrate offlineasm using build rules and emit a depfile r5

Collect source file names without an AST pass

--
Keith, lmk if this works or if you'd prefer the set of included source files to be part of each AST node. I opted against the latter since it felt like it'd lead to a lot of unnecessary copying.

[Keith Miller]

Comment on attachment 453560 [details]
Integrate offlineasm using build rules and emit a depfile r5

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

> Source/JavaScriptCore/offlineasm/asm.rb:391
> +if FileTest.exist?(outputFlnm) and not $options[:depfile] or FileTest.exist?($options[:depfile])

Can we put parens here that way it's trivial to remember if this is: `FileTest.exist?(outputFlnm) and (not $options[:depfile] or FileTest.exist?($options[:depfile]))` vs `(FileTest.exist?(outputFlnm) and not $options[:depfile]) or FileTest.exist?($options[:depfile])`. I also think it's wrong without the parens anyway (IIRC, this is the latter).

> Source/JavaScriptCore/offlineasm/generate_offset_extractor.rb:78
> +if FileTest.exist?(outputFlnm) and not $options[:depfile] or FileTest.exist?($options[:depfile])

Ditto.

> Source/JavaScriptCore/offlineasm/generate_settings_extractor.rb:60
> +if FileTest.exist?(outputFlnm) and not $options[:depfile] or FileTest.exist?($options[:depfile])

Ditto.

[Keith Miller]

(In reply to Elliott Williams from comment #7)
> Created attachment 453560 [details]
> Integrate offlineasm using build rules and emit a depfile r5
> 
> Collect source file names without an AST pass
> 
> --
> Keith, lmk if this works or if you'd prefer the set of included source files
> to be part of each AST node. I opted against the latter since it felt like
> it'd lead to a lot of unnecessary copying.

Yeah, this is exactly what I was thinking for the FIXME, thanks!

[Keith Miller]

Comment on attachment 453560 [details]
Integrate offlineasm using build rules and emit a depfile r5

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

> Source/JavaScriptCore/offlineasm/parser.rb:846
>                  raise "File not found: #{fileName}" if not fileExists and not isOptional

Do we want to add this file to the `.deps` if it doesn't exist? Does that work? What happens if someone adds that file later? e.g. someone adds the `Internal` directory after doing an OS build.

[Alexey Proskuryakov]

Comment on attachment 453560 [details]
Integrate offlineasm using build rules and emit a depfile r5

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

> Source/JavaScriptCore/JavaScriptCore.xcodeproj/project.pbxproj:2166
> +			dependencyFile = "$(DERIVED_FILES_DIR)/$(CURRENT_ARCH)/$(INPUT_FILE_BASE).d";

For unrelated reasons, DerivedSources unfortunately end up in build artifacts. Can these .d files be somewhere else where they don't show up unnecessarily?

> Source/JavaScriptCore/JavaScriptCore.xcodeproj/project.pbxproj:2175
> +				"$(DERIVED_FILES_DIR)/$(CURRENT_ARCH)/LLIntAssembly.h",

On my machine, this file is in WebKitBuild/Debug/DerivedSources/JavaScriptCore/LLIntAssembly.h. Is this basically a drive-by fix to move it to $(CURRENT_ARCH)? But I don't see where it's changed for the legacy build system.

> Source/JavaScriptCore/JavaScriptCore.xcodeproj/project.pbxproj:2183
> +			filePatterns = "*.asm";

There are three rules with the same filePatterns value. I'm curious how this works. Which rules process which inputs?

> Source/JavaScriptCore/JavaScriptCore.xcodeproj/project.pbxproj:12004
> +			shellScript = "[ \"${WK_USE_NEW_BUILD_SYSTEM}\" = \"YES\" ] && exit 0\n\nif [[ \"${ACTION}\" == \"installhdrs\" ]]; then\n    exit 0\nfi\n\nOFFLINEASM_ARGS=\"\"\nif [[ \"${DEPLOYMENT_LOCATION}\" == \"YES\" ]]; then\n    OFFLINEASM_ARGS=\"${OFFLINEASM_ARGS} --webkit-additions-path=${WK_WEBKITADDITIONS_HEADERS_FOLDER_PATH}\"\nfi\n\ncd \"${BUILT_PRODUCTS_DIR}/DerivedSources/JavaScriptCore\"\n\n/usr/bin/env ruby JavaScriptCore/offlineasm/asm.rb \"-I${BUILT_PRODUCTS_DIR}/DerivedSources/JavaScriptCore\" JavaScriptCore/llint/LowLevelInterpreter.asm \"${BUILT_PRODUCTS_DIR}/JSCLLIntOffsetsExtractor\" LLIntAssembly.h \"${BUILD_VARIANTS}\" ${OFFLINEASM_ARGS} || exit 1\n";

It seems like the check for installhdrs is not there for the modern build system. Is that right and OK?

[Elliott Williams]

(In reply to Keith Miller from comment #10)
> Comment on attachment 453560 [details]
> Integrate offlineasm using build rules and emit a depfile r5
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=453560&action=review
> 
> > Source/JavaScriptCore/offlineasm/parser.rb:846
> >                  raise "File not found: #{fileName}" if not fileExists and not isOptional
> 
> Do we want to add this file to the `.deps` if it doesn't exist? Does that
> work? What happens if someone adds that file later? e.g. someone adds the
> `Internal` directory after doing an OS build.

This is a good idea! AFAICT, having missing discovered dependencies does not affect incremental builds (i.e. you can still null build) but does trigger a rerun when they become available, which seem like the exact semantics we'd want.


(In reply to Alexey Proskuryakov from comment #11)
> Comment on attachment 453560 [details]
> Integrate offlineasm using build rules and emit a depfile r5
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=453560&action=review
> 
> > Source/JavaScriptCore/JavaScriptCore.xcodeproj/project.pbxproj:2166
> > +			dependencyFile = "$(DERIVED_FILES_DIR)/$(CURRENT_ARCH)/$(INPUT_FILE_BASE).d";
> 
> For unrelated reasons, DerivedSources unfortunately end up in build
> artifacts. Can these .d files be somewhere else where they don't show up
> unnecessarily?

This is a different directory from the DerivedSources in build products that many of our script write to. On my machine, DERIVED_FILES_DIR is WebKitBuild/JavaScriptCore.build/Debug/JavaScriptCore.build/DerivedSources. This directory shouldn't show up in build artifacts -- it contains a bunch of machine-specific intermediate files.

> On my machine, this file is in WebKitBuild/Debug/DerivedSources/JavaScriptCore/LLIntAssembly.h. Is this basically a drive-by fix to move it to $(CURRENT_ARCH)? But I don't see where it's changed for the legacy build system.

This patch runs offlineasm once per architecture, which is why I had it generate separate LLIntAssembly.h's (otherwise, XCBuild will fail because multiple tasks are creating the same outputs). But it seems like the legacy build system does not do this, so I think I misunderstood how offlineasm handles multi-arch builds.

As we discussed IRL, I need to go back and make sure both build systems write to this path, so that the file reference in the project is reliable.

> > Source/JavaScriptCore/JavaScriptCore.xcodeproj/project.pbxproj:2175
> > +				"$(DERIVED_FILES_DIR)/$(CURRENT_ARCH)/LLIntAssembly.h",
> 
> On my machine, this file is in
> WebKitBuild/Debug/DerivedSources/JavaScriptCore/LLIntAssembly.h. Is this
> basically a drive-by fix to move it to $(CURRENT_ARCH)? But I don't see
> where it's changed for the legacy build system.
> 
> > Source/JavaScriptCore/JavaScriptCore.xcodeproj/project.pbxproj:2183
> > +			filePatterns = "*.asm";
> 
> There are three rules with the same filePatterns value. I'm curious how this
> works. Which rules process which inputs?

Build rules are per-target. In the order they appear in the pbxproj, they correspond with JavaScriptCore, JSCLLIntOffsetsExtractor, JSCLLIntSettingsExtractor respectively.
 
> > Source/JavaScriptCore/JavaScriptCore.xcodeproj/project.pbxproj:12004
> > +			shellScript = "[ \"${WK_USE_NEW_BUILD_SYSTEM}\" = \"YES\" ] && exit 0\n\nif [[ \"${ACTION}\" == \"installhdrs\" ]]; then\n    exit 0\nfi\n\nOFFLINEASM_ARGS=\"\"\nif [[ \"${DEPLOYMENT_LOCATION}\" == \"YES\" ]]; then\n    OFFLINEASM_ARGS=\"${OFFLINEASM_ARGS} --webkit-additions-path=${WK_WEBKITADDITIONS_HEADERS_FOLDER_PATH}\"\nfi\n\ncd \"${BUILT_PRODUCTS_DIR}/DerivedSources/JavaScriptCore\"\n\n/usr/bin/env ruby JavaScriptCore/offlineasm/asm.rb \"-I${BUILT_PRODUCTS_DIR}/DerivedSources/JavaScriptCore\" JavaScriptCore/llint/LowLevelInterpreter.asm \"${BUILT_PRODUCTS_DIR}/JSCLLIntOffsetsExtractor\" LLIntAssembly.h \"${BUILD_VARIANTS}\" ${OFFLINEASM_ARGS} || exit 1\n";
> 
> It seems like the check for installhdrs is not there for the modern build
> system. Is that right and OK?

Yes! These rules are now invoked as part of the "Compile Sources" phase, which is build and install-only, so we get the desired behavior without needing a check.

[Elliott Williams]

(In reply to Keith Miller from comment #10)
> What happens if someone adds that file later? e.g. someone adds the
> `Internal` directory after doing an OS build.

To answer this question more directly: We support making internal and OSS builds using the same build directory. At the very least, it's not a workflow I've seen tested.

There is a problem where, for instance, at build time an included file might come from the SDK ($(SDKROOT)/usr/local/WebKitAdditions/…), but later an Internal directory is updated so that now it is produced locally ($(BUILT_PRODUCTS_DIR)/usr/local/WebKitAdditions/…). Under this patch, offlineasm would not have put the relative-to-build-products path in its .d and thus would not re-run. 

This seems bad, but doesn't appear to cause problems in practice: clang has the same theoretical issue, since its header depfiles only recorded what it included, and not every header it searched for and did not find.

Fundamentally, I don't think discovered dependencies are a good fit for tracking "dependencies which *might* exist later but don't exist right now". And I'm not sure it's worth our time to make offlineasm better than clang at responding to filesystem changes from outside the project directory.

With that in mind, I've convinced myself not to try and emit dependencies for missing files -- wdyt?

[Elliott Williams]

Created attachment 453776 [details]
Integrate offlineasm using build rules and emit a depfile r6

Clarify boolean expression, fix multi-architecture builds, and keep generated file outputs in their old locations

This fixes the bug Alexey noticed, where LLIntAssembly.h is no longer in the build directory and its file reference in the Xcode project goes missing. It also reduces the surface area of this change, since the pbxbuild and xcbuild versions of this script always write to the same locations.

I mistakingly thought that offlineasm had to be run once per architecture -- in reality, it wants to run once and generate assembly for all active platforms at once. I have unchecked "Run once per architecture" in the build rules.

[Alexey Proskuryakov]

Comment on attachment 453776 [details]
Integrate offlineasm using build rules and emit a depfile r6

Looks good to me. Keith?

[Keith Miller]

Comment on attachment 453776 [details]
Integrate offlineasm using build rules and emit a depfile r6

r=me too

[EWS]

Committed r290975 (248153@main): <https://commits.webkit.org/248153@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 453776 [details].

[Chris Dumez]

This broke the build for me:

Input files for custom build rules are not supported by the legacy build system.
Discovered dependency files in custom build rules are not supported by the legacy build system.
Disabling run-once-per-architecture for custom build rules is not supported by the legacy build system.

[Chris Dumez]

Reverted r290975 for reason:

Broke the build for some configurations

Committed r290990 (248168@trunk): <https://commits.webkit.org/248168@trunk>

[Elliott Williams]

Created attachment 454431 [details]
Patch for relanding

The build failures were due to an unrelated internal bug, so this patch can be landed again safely.

Updated to declare Alexey's approval.

[EWS]

Committed r291142 (248302@main): <https://commits.webkit.org/248302@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 454431 [details].