Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add target thumbv7a-pc-windows-msvc #53621

Merged
merged 4 commits into from
Sep 13, 2018
Merged

Add target thumbv7a-pc-windows-msvc #53621

merged 4 commits into from
Sep 13, 2018

Conversation

jordanrh1
Copy link
Contributor

This is an early draft of support for Windows/ARM. To test it,

  1. Install Visual Studio 2017 and Windows SDK version 17134.
  2. Obtain Update vcxproj file for VS2017 alexcrichton/xz2-rs#35, Support windows/arm target compiler-builtins#256, and the fix for LLVM Bug 38620.
  3. Open a command prompt and run
set CC_thumbv7a-pc-windows-msvc=C:\Program Files (x86)\Microsoft Visual Studio\2017\Enterprise\VC\Tools\MSVC\14.11.25503\bin\HostX64\arm\CL.exe
set CFLAGS_thumbv7a-pc-windows-msvc=/D_ARM_WINAPI_PARTITION_DESKTOP_SDK_AVAILABLE=1 /nologo
c:\python27\python.exe x.py build --host x86_64-pc-windows-msvc --build x86_64-pc-windows-msvc --target thumbv7a-pc-windows-msvc

It will build the stage 2 compiler, but fail building stage 2 test. To build an executable targeting windows/arm,

  1. Copy build\x86_64-pc-windows-msvc\stage0\bin\cargo.exe to build\x86_64-pc-windows-msvc\stage2\bin
  2. Open a command prompt and run
"C:\Program Files (x86)\Microsoft Visual Studio\2017\Enterprise\VC\Auxiliary\Build\vcvars64.bat"
set PATH=build\x86_64-pc-windows-msvc\stage2\bin;%PATH%
cargo new hello
cd hello
cargo build --target thumbv7a-pc-windows-msvc –release

Copy target\thumbv7a-pc-windows-msvc\release\hello.exe to your platform and run.

There are a number of open issues that I'm hoping to get help with:

  • Error when compiling the test crate: error: cannot link together two panic runtimes: panic_abort and panic_unwind
  • Warnings when building the compiler_builtins crate: warning: cl : Command line warning D9002 : ignoring unknown option '-fvisibility=hidden'. It looks like the build system is passing GCC-style flags to MSVC.
  • How to specify the LIBPATH entries for ARM. Right now they are hardcoded as absolute paths in the target spec.

This pull request depends on

This PR updates #52659

@rust-highfive
Copy link
Collaborator

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @withoutboats (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 22, 2018
@rust-highfive
Copy link
Collaborator

The job x86_64-gnu-llvm-5.0 of your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.
/usr/local/lib/python2.7/dist-packages/pip/_vendor/requests/packages/urllib3/util/ssl_.py:122: InsecurePlatformWarning: A true SSLContext object is not available. This prevents urllib3 from configuring SSL appropriately and may cause certain SSL connections to fail. You can upgrade to a newer version of Python to solve this. For more information, see https://urllib3.readthedocs.io/en/latest/security.html#insecureplatformwarning.
  InsecurePlatformWarning
/usr/local/lib/python2.7/dist-packages/pip/_vendor/requests/packages/urllib3/util/ssl_.py:122: InsecurePlatformWarning: A true SSLContext object is not available. This prevents urllib3 from configuring SSL appropriately and may cause certain SSL connections to fail. You can upgrade to a newer version of Python to solve this. For more information, see https://urllib3.readthedocs.io/en/latest/security.html#insecureplatformwarning.
  InsecurePlatformWarning
  Downloading https://files.pythonhosted.org/packages/4e/cf/0c313db4b8e3b231447d3807657db4f5e7fad26d5eaeb294b3cfa1388a6c/awscli-1.15.84-py2.py3-none-any.whl (1.3MB)
    0% |▎                               | 10kB 10.4MB/s eta 0:00:01
    1% |▌                               | 20kB 1.9MB/s eta 0:00:01
    2% |▊                               | 30kB 2.2MB/s eta 0:00:01
    3% |█                               | 40kB 2.0MB/s eta 0:00:01
---

[00:04:40] travis_fold:start:tidy
travis_time:start:tidy
tidy check
[00:04:40] tidy error: /checkout/src/librustc_target/spec/thumbv7a_pc_windows_msvc.rs:17: line longer than 100 chars
[00:04:40] tidy error: /checkout/src/librustc_target/spec/thumbv7a_pc_windows_msvc.rs:20: line longer than 100 chars
[00:04:40] tidy error: /checkout/src/librustc_target/spec/thumbv7a_pc_windows_msvc.rs:23: line longer than 100 chars
[00:04:40] tidy error: /checkout/src/libpanic_unwind/seh.rs:122: TODO is deprecated; use FIXME
[00:04:40] tidy error: /checkout/src/libstd/sys/windows/c.rs:836: line longer than 100 chars
[00:04:41] some tidy checks failed
[00:04:41] 
[00:04:41] 
[00:04:41] command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-tools-bin/tidy" "/checkout/src" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "--no-vendor" "--quiet"
[00:04:41] 
[00:04:41] 
[00:04:41] failed to run: /checkout/obj/build/bootstrap/debug/bootstrap test src/tools/tidy
[00:04:41] Build completed unsuccessfully in 0:00:50
[00:04:41] Build completed unsuccessfully in 0:00:50
[00:04:41] Makefile:79: recipe for target 'tidy' failed
[00:04:41] make: *** [tidy] Error 1

The command "stamp sh -x -c "$RUN_SCRIPT"" exited with 2.
travis_time:start:0775b603
$ date && (curl -fs --head https://google.com | grep ^Date: | sed 's/Date: //g' || true)
---
travis_time:end:0cc3f686:start=1534980842492236391,finish=1534980842499233517,duration=6997126
travis_fold:end:after_failure.3
travis_fold:start:after_failure.4
travis_time:start:05105ced
$ ln -s . checkout && for CORE in obj/cores/core.*; do EXE=$(echo $CORE | sed 's|obj/cores/core\.[0-9]*\.!checkout!\(.*\)|\1|;y|!|/|'); if [ -f "$EXE" ]; then printf travis_fold":start:crashlog\n\033[31;1m%s\033[0m\n" "$CORE"; gdb -q -c "$CORE" "$EXE" -iex 'set auto-load off' -iex 'dir src/' -iex 'set sysroot .' -ex bt -ex q; echo travis_fold":"end:crashlog; fi; done || true
travis_fold:end:after_failure.4
travis_fold:start:after_failure.5
travis_time:start:055f48bf
travis_time:start:055f48bf
$ cat ./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers || true
cat: ./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers: No such file or directory
travis_fold:end:after_failure.5
travis_fold:start:after_failure.6
travis_time:start:003d29e4
$ dmesg | grep -i kill

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

@@ -0,0 +1,57 @@
// Copyright 2016 The Rust Project Developers. See the COPYRIGHT
Copy link
Contributor

Choose a reason for hiding this comment

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

2016 or 2018?

let mut base = super::windows_msvc_base::opts();

base.pre_link_args.get_mut(&LinkerFlavor::Msvc).unwrap().push(
"/LIBPATH:C:\\Program Files (x86)\\Microsoft Visual Studio\\2017\\Enterprise\\VC\\Tools\\MSVC\\14.11.25503\\lib\\arm".to_string());
Copy link
Contributor

Choose a reason for hiding this comment

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

Why this hard-coded path?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was hoping someone might have some suggestions on how to solve this.

The problem is that you must build rust (x.py build) in a Visual Studio 2017 developer command prompt for the host environment so that it can build code for the host machine. When it tries to build code for the target, it uses the CC_${target} and CFLAGS_${target} environment variables, which are used to specify a cross compiler and flags for the cross compiler.

However, when it comes time to link, it invokes link.exe which ends up invoking the linker for the host machine. I couldn't find anything like LD_${target} or LDFLAGS_${target} to let me specify the cross linker via environment variables, so the quickest fix was to use pre_link_args in the target spec.

Possible solutions include

  • Adding LD_${target} and LDFLAGS_${target} environment variables
  • Writing code to detect the linker paths based on target architecture
  • Somehow leveraging vcvars.bat

Open to suggestions.

Copy link
Member

Choose a reason for hiding this comment

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

The problem is that you must build rust (x.py build) in a Visual Studio 2017 developer command prompt for the host environment so that it can build code for the host machine.

Just don't run in a VS developer command prompt. Most things should be able to find VS themselves, so if something fails to do that we can just fix that and then cross compiling should work.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the tip. The following changes allowed me to remove the hardcoded paths:

linker_flavor: LinkerFlavor::Msvc,

options: TargetOptions {
features: "+v7,+thumb-mode,+vfp3,+d16,+thumb2,+neon".to_string(),
Copy link
Contributor

Choose a reason for hiding this comment

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

+thumb-mode is superfluous when used with +thumb2, I guess.

Copy link
Contributor

Choose a reason for hiding this comment

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

It's the thumb part in 'llvm_target' that makes it superfluous, thumb2 doesn't actually specify the state, it just says what's available when using the Thumb state (but it's implied by +v7 anyway, which is implied by v7 in the llvm_target also).

+d16 should be removed becuase +neon requires 32 registers.

@bors
Copy link
Contributor

bors commented Sep 1, 2018

☔ The latest upstream changes (presumably #53884) made this pull request unmergeable. Please resolve the merge conflicts.

@parched
Copy link
Contributor

parched commented Sep 7, 2018

Hi @jordanrh1, this looks good.

Bug 38620 - ARM: Incorrect COFF relocation type for thumb bl instruction

This is actually the correct behaviour I believe, BLX is what you want on Armv7, what error were you seeing?

@jordanrh1
Copy link
Contributor Author

The behavior I was seeing was that a BL instruction in the executable was being rewritten to a BLX instruction by the Windows loader, because the relocation tables told it to. A thumb2 BLX instruction to relative offset necessarily changes the CPU to ARM state. When you're branching from thumb code to thumb code, this is not what you want. The effect at runtime is an immediate crash. It executes the branch and changes to ARM mode, then interprets a thumb instruction as ARM which clearly leads to bad things.

More precisely, an instruction with the T1 encoding was being written to an instruction with the T2 encoding. The branch target is thumb code. As you can see, the T2 encoding changes the CPU to ARM mode, which is incorrect because the target is thumb code.

image

@parched
Copy link
Contributor

parched commented Sep 7, 2018

Ah right yes, relative offset, relocations aren't really my speciality. Still, I believe the relocation is correct, the IMAGE_REL_THUMB_BRANCH24 is for a branch without link. If I'm not mistaken, IMAGE_REL_THUMB_BLX23 is for both BL and BLX (it's just called R_ARM_THM_CALL in ELF) and it's up to the linker to select the correct one based on the current and destination state.

@jordanrh1
Copy link
Contributor Author

I can see why you might think IMAGE_REL_ARM_BLX23T is correct by reading the documentation, but the runtime behavior, which I have tested and observed, tells the real story. IMAGE_REL_THUMB_BRANCH24 is for both B and BL instructions. The comments from ntimage.h are consistent with this observation.

#define IMAGE_REL_ARM_BRANCH20T         0x0012  // Thumb: 32-bit conditional B
#define IMAGE_REL_THUMB_BRANCH20        0x0012  // Thumb: 32-bit conditional B (deprecated)
#define IMAGE_REL_ARM_BRANCH24T         0x0014  // Thumb: 32-bit B or BL
#define IMAGE_REL_THUMB_BRANCH24        0x0014  // Thumb: 32-bit B or BL (deprecated)
#define IMAGE_REL_ARM_BLX23T            0x0015  // Thumb: BLX immediate
#define IMAGE_REL_THUMB_BLX23           0x0015  // Thumb: BLX immediate (deprecated)

The real issue is that the documentation is misleading.

This discussion really belongs on the LLVM bug report.

@parched
Copy link
Contributor

parched commented Sep 8, 2018

The real issue is that the documentation is misleading.

Yes, so it seems you are correct then. Perhaps you should submit your patch to https://reviews.llvm.org/ then someone can review and commit it for you.

@jordanrh1
Copy link
Contributor Author

@alexcrichton Please let me know if you think this PR is ready for merge. With the above mentioned fixes to cmake-rs and cc-rs, a VS command prompt is no longer required. The remaining issues are:

  • test crate: error: cannot link together two panic runtimes: panic_abort and panic_unwind
  • GCC-style flags get passed to MSVC ARM cross compiler, causing non-fatal warnings to be printed
  • You must set CFLAGS_thumbv7a-pc-windows-msvc=/D_ARM_WINAPI_PARTITION_DESKTOP_SDK_AVAILABLE=1 /nologo before building

Copy link
Member

@alexcrichton alexcrichton left a comment

Choose a reason for hiding this comment

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

Looking good to me, thanks for this!

base.pre_link_args.get_mut(&LinkerFlavor::Msvc).unwrap().push(
"/OPT:NOLBR".to_string());

base.panic_strategy = PanicStrategy::Abort;
Copy link
Member

Choose a reason for hiding this comment

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

Could a FIXME be left here to eventually remove this line?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@alexcrichton
Copy link
Member

r? @alexcrichton

@alexcrichton
Copy link
Member

@bors: r+

@bors
Copy link
Contributor

bors commented Sep 12, 2018

📌 Commit fd41c39 has been approved by alexcrichton

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 12, 2018
@bors
Copy link
Contributor

bors commented Sep 12, 2018

⌛ Testing commit fd41c39 with merge 2c827146c5f433b3fc75c0bdb08d16d053199663...

@bors
Copy link
Contributor

bors commented Sep 12, 2018

💔 Test failed - status-travis

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Sep 12, 2018
@rust-highfive
Copy link
Collaborator

Your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

@alexcrichton
Copy link
Member

@bors: retry

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 13, 2018
@bors
Copy link
Contributor

bors commented Sep 13, 2018

⌛ Testing commit fd41c39 with merge 89ca3c6ef6ff8e6b5f541a7b8ab7c6c2972d558f...

@bors
Copy link
Contributor

bors commented Sep 13, 2018

💔 Test failed - status-travis

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Sep 13, 2018
@rust-highfive
Copy link
Collaborator

The job dist-i686-linux of your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.
travis_fold:end:step_upload_script
travis_fold:start:worker_info
Worker information
hostname: 2326c7b0-867d-4c51-b87b-4bc1cf0ed948@1.production-4-worker-org-a-1-gce
version: v4.1.0-17-g600200d https://github.com/travis-ci/worker/tree/600200deddbb9114004580ec8d6dd7b4722840d9
startup: 7.221001643s
travis_fold:end:worker_info
travis_fold:start:system_info
Build system information
---
[00:48:31] [RUSTC-TIMING] rustc_borrowck test:false 33.961
[00:48:37] [RUSTC-TIMING] rustc_lint test:false 37.630
[00:48:46] [RUSTC-TIMING] rustc_plugin test:false 23.349
[00:49:11] [RUSTC-TIMING] rustc_resolve test:false 47.965
The job exceeded the maximum time limit for jobs, and has been terminated.

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

@kennytm
Copy link
Member

kennytm commented Sep 13, 2018

@bors retry

50 minutes timeout

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 13, 2018
@bors
Copy link
Contributor

bors commented Sep 13, 2018

⌛ Testing commit fd41c39 with merge 90d36fb...

bors added a commit that referenced this pull request Sep 13, 2018
Add target thumbv7a-pc-windows-msvc

This is an early draft of support for Windows/ARM. To test it,

1. Install Visual Studio 2017 and Windows SDK version 17134.
1. Obtain alexcrichton/xz2-rs#35, rust-lang/compiler-builtins#256, and the fix for [LLVM Bug 38620](https://bugs.llvm.org/show_bug.cgi?id=38620).
2. Open a command prompt and run
```
set CC_thumbv7a-pc-windows-msvc=C:\Program Files (x86)\Microsoft Visual Studio\2017\Enterprise\VC\Tools\MSVC\14.11.25503\bin\HostX64\arm\CL.exe
set CFLAGS_thumbv7a-pc-windows-msvc=/D_ARM_WINAPI_PARTITION_DESKTOP_SDK_AVAILABLE=1 /nologo
c:\python27\python.exe x.py build --host x86_64-pc-windows-msvc --build x86_64-pc-windows-msvc --target thumbv7a-pc-windows-msvc
```

It will build the stage 2 compiler, but fail building stage 2 test. To build an executable targeting windows/arm,
1. Copy `build\x86_64-pc-windows-msvc\stage0\bin\cargo.exe` to `build\x86_64-pc-windows-msvc\stage2\bin`
2. Open a command prompt and run
```
"C:\Program Files (x86)\Microsoft Visual Studio\2017\Enterprise\VC\Auxiliary\Build\vcvars64.bat"
set PATH=build\x86_64-pc-windows-msvc\stage2\bin;%PATH%
cargo new hello
cd hello
cargo build --target thumbv7a-pc-windows-msvc –release
```

Copy target\thumbv7a-pc-windows-msvc\release\hello.exe to your platform and run.

There are a number of open issues that I'm hoping to get help with:

 - Error when compiling the `test` crate: `error: cannot link together two panic runtimes: panic_abort and panic_unwind`
 - Warnings when building the compiler_builtins crate: `warning: cl : Command line warning D9002 : ignoring unknown option '-fvisibility=hidden'`. It looks like the build system is passing GCC-style flags to MSVC.
 - How to specify the LIBPATH entries for ARM. Right now they are hardcoded as absolute paths in the target spec.

This pull request depends on
 - alexcrichton/xz2-rs#35 - update vcxproj to Visual Studio 2017
 - rust-lang/compiler-builtins#256 - fix compile errors when building for windows/arm
 - [Bug 38620 - ARM: Incorrect COFF relocation type for thumb bl instruction](https://bugs.llvm.org/show_bug.cgi?id=38620)

This PR updates #52659
@bors
Copy link
Contributor

bors commented Sep 13, 2018

☀️ Test successful - status-appveyor, status-travis
Approved by: alexcrichton
Pushing 90d36fb to master...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants