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

codegen_llvm: check inline assembly constraints with LLVM #54568

Merged
merged 5 commits into from
Sep 28, 2018

Conversation

levex
Copy link
Contributor

@levex levex commented Sep 25, 2018

---%<---
Hey all,

As issue #54130 highlights, constraints are not checked and passing bad constraints to LLVM can crash it since a Verify() call is placed inside an assertion (see: src/llvm/lib/IR/InlineAsm.cpp:39).

As this is my first PR to the Rust compiler (woot! 🎉), there might be better ways of achieving this result. In particular, I am not too happy about generating an error in codegen; it would be much nicer if we did it earlier. However, @rkruppe noted on IRC that this should be fine for an unstable feature and a much better solution than the status quo, which is an ICE.

Thanks!
--->%---

LLVM provides a way of checking whether the constraints and the actual
inline assembly make sense. This commit introduces a check before
emitting code for the inline assembly. If LLVM rejects the inline
assembly (or its constraints), then the compiler emits an error E0668
("malformed inline assembly").

Fixes: #54130
Signed-off-by: Levente Kurusa <lkurusa@acm.org>

LLVM provides a way of checking whether the constraints and the actual
inline assembly make sense. This commit introduces a check before
emitting code for the inline assembly. If LLVM rejects the inline
assembly (or its constraints), then the compiler emits an error E0668
("malformed inline assembly").

Signed-off-by: Levente Kurusa <lkurusa@acm.org>
@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 @davidtwco (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 Sep 25, 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.

[00:04:48] travis_fold:start:tidy
travis_time:start:tidy
tidy check
[00:04:48] tidy error: /checkout/src/librustc_codegen_llvm/mir/statement.rs:91: line longer than 100 chars
[00:04:49] some tidy checks failed
[00:04:49] 
[00:04:49] 
[00:04:49] 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:49] 
[00:04:49] 
[00:04:49] failed to run: /checkout/obj/build/bootstrap/debug/bootstrap test src/tools/tidy
[00:04:49] Build completed unsuccessfully in 0:00:50
[00:04:49] Build completed unsuccessfully in 0:00:50
[00:04:49] make: *** [tidy] Error 1
[00:04:49] Makefile:79: recipe for target 'tidy' failed

The command "stamp sh -x -c "$RUN_SCRIPT"" exited with 2.
travis_time:start:09f56c40
$ date && (curl -fs --head https://google.com | grep ^Date: | sed 's/Date: //g' || true)
---
travis_time:end:08aecef1:start=1537902172301632479,finish=1537902172306055940,duration=4423461
travis_fold:end:after_failure.3
travis_fold:start:after_failure.4
travis_time:start:081fedb7
$ 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 --batch -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:019af790
travis_time:start:019af790
$ 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:08734abc
$ 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)

@davidtwco
Copy link
Member

r? @nagisa

@rust-highfive rust-highfive assigned nagisa and unassigned davidtwco Sep 25, 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.
[00:58:54] ....................................................................................................
[00:58:57] ................................................................i...................................
[00:59:00] ....................................................................................................
[00:59:03] ....................................................................................................
[00:59:06] .............iiiiiiiii..............................................................................
[00:59:12] ....................................................................................................
[00:59:16] ................................................................................................i...
[00:59:18] ....................................................................................................
[00:59:21] ........................................................i.i..ii.....................................
---
[01:36:02] ---- /checkout/obj/build/x86_64-unknown-linux-gnu/test/error-index.md - Rust_Compiler_Error_Index::E0668 (line 11046) stdout ----
[01:36:02] error[E0668]: malformed inline assembly
[01:36:02]  --> /checkout/obj/build/x86_64-unknown-linux-gnu/test/error-index.md:11052:9
[01:36:02]   |
[01:36:02] 7 |         asm!("" :"={rax"(rax));
[01:36:02] 
[01:36:02] thread '/checkout/obj/build/x86_64-unknown-linux-gnu/test/error-index.md - Rust_Compiler_Error_Index::E0668 (line 11046)' panicked at 'couldn't compile the test', librustdoc/test.rs:333:13
[01:36:02] note: Run with `RUST_BACKTRACE=1` for a backtrace.
[01:36:02] 
---
[01:36:02] 
[01:36:02] 
[01:36:02] failed to run: /checkout/obj/build/bootstrap/debug/bootstrap test
[01:36:02] Build completed unsuccessfully in 0:46:16
[01:36:02] make: *** [check] Error 1
[01:36:02] Makefile:58: recipe for target 'check' failed

The command "stamp sh -x -c "$RUN_SCRIPT"" exited with 2.
travis_time:start:1c2dd0b8
$ date && (curl -fs --head https://google.com | grep ^Date: | sed 's/Date: //g' || true)

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)

@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.
[00:55:30] ....................................................................................................
[00:55:33] ...............................................................i....................................
[00:55:36] ....................................................................................................
[00:55:39] ....................................................................................................
[00:55:42] ............iiiiiiiii...............................................................................
[00:55:47] ....................................................................................................
[00:55:51] ................................................................................................i...
[00:55:54] ....................................................................................................
[00:55:57] ........................................................i.i..ii.....................................
---
[01:31:04] 
[01:31:04] failures:
[01:31:04] 
[01:31:04] ---- /checkout/obj/build/x86_64-unknown-linux-gnu/test/error-index.md - Rust_Compiler_Error_Index::E0668 (line 11046) stdout ----
[01:31:04] thread '/checkout/obj/build/x86_64-unknown-linux-gnu/test/error-index.md - Rust_Compiler_Error_Index::E0668 (line 11046)' panicked at 'test compiled while it wasn't supposed to', librustdoc/test.rs:323:13
[01:31:04]gnu/stage1-rustc/x86_64-unknown-linux-gnu
143184 ./obj/build/x86_64-unknown-linux-gnu/stage1-rustc/x86_64-unknown-linux-gnu/release
137504 ./obj/build/x86_64-unknown-linux-gnu/stage1-rustc/x86_64-unknown-linux-gnu/release/deps
134216 ./obj/build/bootstrap/debug/incremental/bootstrap-2zc4gzhr0d54q
134216 ./obj/build/bootstrap/debug/incremental/bootstrap-2zc4gzhr0d54q
134212 ./obj/build/bootstrap/debug/incremental/bootstrap-2zc4gzhr0d54q/s-f55m0rjbp1-ybp2ey-abll1u66rzrn
133900 ./obj/build/x86_64-unknown-linux-gnu/stage0-bootstrap-tools/x86_64-unknown-linux-gnu/release
125916 ./obj/build/x86_64-unknown-linux-gnu/stage1-std
122628 ./obj/build/x86_64-unknown-linux-gnu/stage0-bootstrap-tools/x86_64-unknown-linux-gnu/release/deps
118932 ./obj/build/x86_64-unknown-linux-gnu/test/mir-opt

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)

@levex
Copy link
Contributor Author

levex commented Sep 26, 2018

Travis passed finally 🎉 !

@nagisa
Copy link
Member

nagisa commented Sep 26, 2018

@levex please also add a few tests which would specifically test the functionality introduced by this PR.

These tests would most likely end up going to src/test/ui/.

@levex
Copy link
Contributor Author

levex commented Sep 26, 2018

@levex please also add a few tests which would specifically test the functionality introduced by this PR.

These tests would most likely end up going to src/test/ui/.

Added three tests that right now (on master) cause ICEs, panics or LLVM errors. They all work with the PR merged.

@nagisa
Copy link
Member

nagisa commented Sep 26, 2018

@bors r+, thanks!

@nagisa
Copy link
Member

nagisa commented Sep 27, 2018

@bors r+

@bors
Copy link
Contributor

bors commented Sep 27, 2018

📌 Commit 0991d20 has been approved by nagisa

@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 27, 2018
@bors
Copy link
Contributor

bors commented Sep 28, 2018

⌛ Testing commit 0991d20 with merge d623ec6...

bors added a commit that referenced this pull request Sep 28, 2018
codegen_llvm: check inline assembly constraints with LLVM

---%<---
Hey all,

As issue #54130 highlights, constraints are not checked and passing bad constraints to LLVM can crash it since a `Verify()` call is placed inside an assertion (see: `src/llvm/lib/IR/InlineAsm.cpp:39`).

As this is my first PR to the Rust compiler (woot! 🎉), there might be better ways of achieving this result. In particular, I am not too happy about generating an error in codegen; it would be much nicer if we did it earlier. However, @rkruppe [noted on IRC](https://botbot.me/mozilla/rustc/2018-09-25/?msg=104791581&page=1) that this should be fine for an unstable feature and a much better solution than the _status quo_, which is an ICE.

Thanks!
--->%---

LLVM provides a way of checking whether the constraints and the actual
inline assembly make sense. This commit introduces a check before
emitting code for the inline assembly. If LLVM rejects the inline
assembly (or its constraints), then the compiler emits an error E0668
("malformed inline assembly").

Fixes: #54130
Signed-off-by: Levente Kurusa \<lkurusa@acm.org\>
@bors
Copy link
Contributor

bors commented Sep 28, 2018

☀️ Test successful - status-appveyor, status-travis
Approved by: nagisa
Pushing d623ec6 to master...

bors added a commit that referenced this pull request Oct 10, 2018
codegen_llvm: verify that inline assembly operands are scalars

Another set of inline assembly fixes. This time let's emit an error message when the operand value cannot be coerced into the operand constraint.

Two questions:

1) Should I reuse `E0668` which was introduced in #54568 or just use `E0669` as it stands because they do mean different things, but maybe that's not too user-friendly. Just a thought.
2) The `try_fold` returns the operand which failed to be converted into a scalar value, any suggestions on how to use that in the error message?

Thanks!
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.

5 participants