Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

[windows] Support win_debug_x86 compilation target in engine #30417

Merged
merged 2 commits into from
Mar 16, 2022

Conversation

eggfly
Copy link
Member

@eggfly eggfly commented Dec 20, 2021

It added --windows param for gn, added x86 target_arch, and fixed windows win_debug_x86 32-bit debug target compilation errors in Flutter engine. That is, using this command can do a successful compilation for x86 target on windows x64 host:

$ tools\gn --no-goma --no-prebuilt-dart-sdk --windows --windows-cpu x86 # "--windows" is a must to generate "win" not "host“
$ ninja -C ..\out\win_debug_x86

(And windows jit_release_x86 (tools\gn --runtime-mode jit_release --windows --windows-cpu x86) is a WIP.)

Design doc:

Related issues:

In our Windows desktop scenarios and some customers like hospital, bank, embedded industrial control hardwares, the OS are not updated for years. So there are still many x86 users and devices.
Many cross platform frameworks, such as QT and electron, support 32-bit. I think Flutter can also support this CPU architecture.

So we want to contribute this PR. 😄

Details:

  1. About LPARAM change:
OS WPARAM LPARAM
32-bit Windows 32-bit unsigned 32-bit signed
64-bit Windows 64-bit unsigned 64-bit signed

Which cause compile error on 32-bit target:

[974/1574] CXX obj/flutter/shell/platform/windows/testing/flutter_windows_unittests.wm_builders.obj
FAILED: obj/flutter/shell/platform/windows/testing/flutter_windows_unittests.wm_builders.obj
ninja -t msvc -e environment.x86 -- ../../buildtools/windows-x64/clang/bin/clang-cl.exe -m32 /nologo /showIncludes @obj/flutter/shell/platform/windows/testing/flutter_windows_unittests.wm_builders.obj.rsp /c ../../flutter/shell/platform/windows/testing/wm_builders.cc /Foobj/flutter/shell/platform/windows/testing/flutter_windows_unittests.wm_builders.obj /Fdobj/flutter/shell/platform/windows/flutter_windows_unittests_cc.pdb
../../flutter/shell/platform/windows/testing/wm_builders.cc(28,17): error: non-constant-expression cannot be narrowed from type 'uint32_t' (aka 'unsigned int') to 'LPARAM' (aka 'long') in initializer list [-Wc++11-narrowing]
      .lParam = lParam,
                ^~~~~~
../../flutter/shell/platform/windows/testing/wm_builders.cc(28,17): note: insert an explicit cast to silence this issue
      .lParam = lParam,
                ^~~~~~
                static_cast<LPARAM>( )
1 error generated.
[987/1574] ACTION //third_party/dart/utils/analysis_server:analysis_server_dill(//build/toolchain/win:clang_x86)
ninja: build stopped: subcommand failed.

I think we can direcly use LPARAM type instead of uint32_t, and do some mod in tests to avoid this test failure:

[ RUN      ] KeyboardTest.AltGrTwice
../../flutter/shell/platform/windows/keyboard_win32_unittests.cc(288): error: Expected equality of these values:
  expectation.message.lParam
    Which is: -1071841279
  lParam & 0xffffffff
    Which is: 3223126017
../../flutter/shell/platform/windows/keyboard_win32_unittests.cc(288): error: Expected equality of these values:
  expectation.message.lParam
    Which is: -1071841279
  lParam & 0xffffffff
    Which is: 3223126017
[  FAILED  ] KeyboardTest.AltGrTwice (9 ms)

@dkwingsmt may I have your double check about this?

  1. The compilation error:
ninja: Entering directory `..\out\host_debug_unopt_x86'
[1/19] CXX obj/flutter/vulkan/vulkan.vulkan_debug_report.obj
FAILED: obj/flutter/vulkan/vulkan.vulkan_debug_report.obj
ninja -t msvc -e environment.x86 -- ../../buildtools/windows-x64/clang/bin/clang-cl.exe -m32 /nologo /showIncludes @obj/flutter/vulkan/vulkan.vulkan_debug_report.obj.rsp /c ../../flutter/vulkan/vulkan_debug_report.cc /Foobj/flutter/vulkan/vulkan.vulkan_debug_report.obj /Fdobj/flutter/vulkan/vulkan_cc.pdb
../../flutter/vulkan/vulkan_debug_report.cc(205,22): error: cannot initialize a member subobject of type 'PFN_vkDebugReportCallbackEXT' (aka 'unsigned int (*)(unsigned int, VkDebugReportObjectTypeEXT, unsigned long long, unsigned int, int, const char *, const char *, void *) __attribute__((stdcall))') with an rvalue of type 'VkBool32 (*)(VkDebugReportFlagsEXT, VkDebugReportObjectTypeEXT, uint64_t, size_t, int32_t, const char *, const char *, void *)' (aka 'unsigned int (*)(unsigned int, VkDebugReportObjectTypeEXT, unsigned long long, unsigned int, int, const char *, const char *, void *)')
      .pfnCallback = &vulkan::OnVulkanDebugReportCallback,
                     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
1 error generated.
ninja: build stopped: subcommand failed.

The fixing:

_WIN32 Defined as 1 when the compilation target is 32-bit ARM, 64-bit ARM, x86, or x64. Otherwise, undefined.
_WIN64 Defined as 1 when the compilation target is 64-bit ARM or x64. Otherwise, undefined.

Pre-launch Checklist

  • I read the [Contributor Guide] and followed the process outlined there for submitting PRs.
  • I read the [Tree Hygiene] wiki page, which explains my responsibilities.
  • I read and followed the [Flutter Style Guide] and the [C++, Objective-C, Java style guides].
  • I listed at least one issue that this PR fixes in the description above.
  • I added new tests to check the change I am making or feature I am adding, or Hixie said the PR is test-exempt. See [testing the engine] for instructions on
    writing and running engine tests.
  • I updated/added relevant documentation (doc comments with ///).
  • I signed the [CLA].
  • All existing and new tests are passing.

Sorry, something went wrong.

@flutter-dashboard
Copy link

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat.

If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix?

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

@eggfly eggfly changed the title Support windows-x86-jit 32-bit target WIP: Support windows-x86-jit 32-bit target Dec 20, 2021
@dnfield dnfield requested a review from cbracken December 21, 2021 03:40
@flutter-dashboard
Copy link

This pull request executed golden file tests, but it has not been updated in a while (20+ days). Test results from Gold expire after as many days, so this pull request will need to be updated with a fresh commit in order to get results from Gold.

@stuartmorgan-g stuartmorgan-g removed their request for review January 21, 2022 17:20
@eggfly eggfly force-pushed the windows_x86 branch 2 times, most recently from da069c8 to 30d3921 Compare March 9, 2022 07:42
@flutter-dashboard
Copy link

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat (don't just cc him here, he won't see it! He's on Discord!).

If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix?

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

@eggfly eggfly changed the title WIP: Support windows-x86-jit 32-bit target (windows) Support windows' host_debug_x86 compilation target in engine Mar 12, 2022
@eggfly eggfly changed the title (windows) Support windows' host_debug_x86 compilation target in engine (windows) Support windows host_debug_x86 compilation target in engine Mar 12, 2022
@eggfly eggfly requested a review from zanderso March 12, 2022 18:00
@eggfly eggfly changed the title (windows) Support windows host_debug_x86 compilation target in engine [windows] Support windows host_debug_x86 compilation target in engine Mar 12, 2022
Copy link
Member

@zanderso zanderso left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few things:

  • It looks like this PR is breaking unit tests, so at a minimum that will need to be addressed.
  • I'd like @cbracken to weigh in on this PR.
  • Even if the unit tests are fixed, my comments about the gn script are addressed, and @cbracken signs-off, this configuration will still be in Flutter's "Unsupported" support category, meaning that we may accidentally break it at any time. If this configuration is important to you, you'll be expected to keep it working on your own.

@eggfly eggfly changed the title [windows] Support windows host_debug_x86 compilation target in engine [windows] Support win_debug_x86 compilation target in engine Mar 13, 2022
@eggfly eggfly force-pushed the windows_x86 branch 2 times, most recently from 38c6098 to 6d0571b Compare March 13, 2022 09:17
@eggfly

This comment was marked as resolved.

@eggfly
Copy link
Member Author

eggfly commented Mar 14, 2022

A few things:

  • It looks like this PR is breaking unit tests, so at a minimum that will need to be addressed.
  • I'd like @cbracken to weigh in on this PR.
  • Even if the unit tests are fixed, my comments about the gn script are addressed, and @cbracken signs-off, this configuration will still be in Flutter's "Unsupported" support category, meaning that we may accidentally break it at any time. If this configuration is important to you, you'll be expected to keep it working on your own.

Thank you for your advise! : ) This change is ready for review now. @zanderso

  • I fixed the breaking unit test.
  • Whould you give me some advise about how to modify the "unsupported" state of windows x86 engine, and how to let the Flutter engine Infra check the x86 build, or even x86 unittests and engine artifacts, just like the x86/x86-jit-release for Android. I have some more PRs (including Flutter framework changes) about windows 32-bit. And it really important for us.

copybara-service bot pushed a commit to dart-lang/sdk that referenced this pull request Mar 15, 2022
dart_target_arch can be "x86", which is handled here:
https://github.com/dart-lang/sdk/blob/main/runtime/BUILD.gn#L113.
This CL also handles it in the remaining places.

This came up in flutter/engine#30417

TEST=N/A
Change-Id: Ib6dcd0a7236938026a0cbcb3955433960b396667
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/237120
Reviewed-by: Ryan Macnak <rmacnak@google.com>
Commit-Queue: Zach Anderson <zra@google.com>
@timsneath
Copy link
Contributor

Let's have the dialog about support in this issue flutter/flutter#37777, rather than in a pull request. Looks like this is ready for @cbracken to sign off as a prerequisite to merging.

Copy link
Member

@cbracken cbracken left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The change itself LGTM. We've historically said we wouldn't do anything to stand in the way of fixes that make life better for those who'd like to get Flutter working on 32-bit Windows so long as those fixes don't complicate support for 64-bit Windows, which is what we officially target. These fixes fall into that category, though I'm quite sure a fair bit more will be required.

Happy to have the discussion around support separately on flutter/flutter#37777. The short version is that while there are a few dedicated contributors who want to see 32-bit Windows support, there aren't currently enough contributors focusing on maintaining/improving our 32-bit Windows for it to successful "officially supported" target (nor do we have 32-bit Windows test infra, for that matter).

@eggfly eggfly added the waiting for tree to go green This PR is approved and tested, but waiting for the tree to be green to land. label Mar 16, 2022
@fluttergithubbot fluttergithubbot merged commit 9f13454 into flutter:main Mar 16, 2022
@eggfly eggfly deleted the windows_x86 branch March 16, 2022 03:10
zanderso pushed a commit to flutter/flutter that referenced this pull request Mar 17, 2022

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
* 9f13454 [windows] Support win_debug_x86 compilation target in engine (flutter/engine#30417)

* c5e958e Make tracing safe (flutter/engine#32042)

* cf875d4 [macOS] Forward key events to NSTextInputContext when there's composing text (flutter/engine#32051)

* ffd5c9c Roll Fuchsia Linux SDK from Ee9OX2o6P... to mVqiTwaVa... (flutter/engine#32055)

* b0018c3 Roll Skia from a9bc6c643791 to 8afe53fd76d3 (1 revision) (flutter/engine#32056)

* df12321 Roll Skia from 8afe53fd76d3 to 0d81bc7bb04e (2 revisions) (flutter/engine#32057)

* 398a274 Roll Fuchsia Mac SDK from jvlI1s78T... to vWlaMIVkM... (flutter/engine#32060)

* 3ffa9cf Roll Skia from 0d81bc7bb04e to e253cc3e55d3 (1 revision) (flutter/engine#32063)

* 275cd2b [web] Position spans absolutely within paragraph (flutter/engine#31907)

* 46c2cca Roll Skia from e253cc3e55d3 to 9301fe3779bb (1 revision) (flutter/engine#32064)
@GithubSiXun
Copy link

@eggfly Excuse me. Is it possible to run applications developed with Flutter on Windows 7 32-bit systems?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
needs tests platform-windows waiting for tree to go green This PR is approved and tested, but waiting for the tree to be green to land.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants