Mark inout asm! operands as used in liveness pass #77976

oliviacrain · 2020-10-15T14:11:02Z

Variables used in inout operands in inline assembly (that is, they're used as both input and output to some arbitrary assembly instruction) are being marked as read and written, but are not marked as being used in the RWU table during the liveness pass. This can result in such expressions triggering an unused variable lint warning. This is incorrect behavior- reads without uses are currently only used for compound assignments. We conservatively assume that an inout operand is being read and used in the context of the assembly instruction.

Closes #77915

rust-highfive · 2020-10-15T14:11:06Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @matthewjasper (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.

compiler/rustc_passes/src/liveness.rs

nagisa · 2020-10-17T22:22:57Z

src/test/ui/liveness/liveness-issue-77915.rs

+#![deny(unused_variables)]
+
+// Tests the single variable inout case
+unsafe fn rep_movsb(mut dest: *mut u8, mut src: *const u8, mut n: usize) -> *mut u8 {


Can you also add a test that ensures projections work correctly as well?

Added a couple of test cases for field projection, thanks!

nagisa · 2020-10-17T22:27:15Z

src/test/ui/liveness/liveness-issue-77915.rs

@@ -0,0 +1,36 @@
+// Ensure inout asm! operands are marked as used by the liveness pass
+
+// only-x86_64


This test could also be made architecture independent by doing something like this:

unsafe fn rep_movsb(mut src: *const u8) { asm!("/*{0}*/", inout(reg) src); }

It would allow testing this by people who use something else than x86_64 as a host.

Fixed in the latest changes, thanks!

nagisa · 2020-10-17T22:30:19Z

compiler/rustc_passes/src/liveness.rs

@@ -1199,7 +1199,7 @@ impl<'a, 'tcx> Liveness<'a, 'tcx> {
                            }
                        }
                        hir::InlineAsmOperand::InOut { expr, .. } => {
-                            succ = self.propagate_through_place_components(expr, succ);
+                            succ = self.propagate_through_expr(expr, succ);


I didn't dig too deep into this, but looking at Assign code as an example, I suspect this may need both propagate_through_place_components and propagate_through_expr.

It looks like it was determined that propagate_through_place_components would be the necessary call here in order to avoid improperly setting the inout expr as its own successor/predecessory.

matthewjasper · 2020-10-19T20:06:49Z

@bors r+

bors · 2020-10-19T20:06:50Z

📌 Commit 8f0bced has been approved by matthewjasper

…hewjasper Mark inout asm! operands as used in liveness pass Variables used in `inout` operands in inline assembly (that is, they're used as both input and output to some arbitrary assembly instruction) are being marked as read and written, but are not marked as being used in the RWU table during the liveness pass. This can result in such expressions triggering an unused variable lint warning. This is incorrect behavior- reads without uses are currently only used for compound assignments. We conservatively assume that an `inout` operand is being read and used in the context of the assembly instruction. Closes rust-lang#77915

JohnTitor · 2020-10-21T04:53:45Z

Failed in https://github.com/rust-lang-ci/rust/runs/1284758758:

failures:

---- [ui] ui/liveness/liveness-asm.rs stdout ----

error: test compilation failed although it shouldn't!
status: exit code: 1
command: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/bin/rustc" "/checkout/src/test/ui/liveness/liveness-asm.rs" "-Zthreads=1" "--target=wasm32-unknown-unknown" "--error-format" "json" "-Zui-testing" "-Zdeduplicate-diagnostics=no" "--emit" "metadata" "--out-dir" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/ui/liveness/liveness-asm" "-A" "unused" "-Crpath" "-O" "-Cdebuginfo=0" "-Zunstable-options" "-Lnative=/checkout/obj/build/wasm32-unknown-unknown/native/rust-test-helpers" "-L" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/ui/liveness/liveness-asm/auxiliary"
stdout:
------------------------------------------

------------------------------------------
stderr:
------------------------------------------
error[E0472]: asm! is unsupported on this target
  --> /checkout/src/test/ui/liveness/liveness-asm.rs:12:5
   |
LL |     asm!("/*{0}*/", inout(reg) src); //~ WARN value assigned to `src` is never read
   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^


error: aborting due to 6 previous errors


------------------------------------------



failures:
    [ui] ui/liveness/liveness-asm.rs

test result: FAILED. 10397 passed; 1 failed; 564 ignored; 0 measured; 0 filtered out

The test needs a tweak.
@bors r-

oliviacrain · 2020-10-21T05:37:49Z

@JohnTitor Sorry to break the rollup- changed it back to only target x86_64, like other inline asm tests in ui. Confirmed test is properly ignored on other platforms.

JohnTitor · 2020-10-21T05:56:11Z

@oliviacrain No worries! You should also bless the output as the line number is changed.

oliviacrain · 2020-10-21T05:57:56Z

@JohnTitor Thanks for catching that! Output blessed.

JohnTitor · 2020-10-21T06:32:07Z

Thanks!
@bors r=matthewjasper

bors · 2020-10-21T06:32:08Z

📌 Commit dc29c7a has been approved by matthewjasper

Rollup of 10 pull requests Successful merges: - rust-lang#77420 (Unify const-checking structured errors for `&mut` and `&raw mut`) - rust-lang#77554 (Support signed integers and `char` in v0 mangling) - rust-lang#77976 (Mark inout asm! operands as used in liveness pass) - rust-lang#78009 (Haiku: explicitly set CMAKE_SYSTEM_NAME when cross-compiling) - rust-lang#78084 (Greatly improve display for small mobile devices screens) - rust-lang#78155 (Fix two small issues in compiler/rustc_lint/src/types.rs) - rust-lang#78156 (Fixed build failure of `rustfmt`) - rust-lang#78172 (Add test case for rust-lang#77062) - rust-lang#78188 (Add tracking issue number for pin_static_ref) - rust-lang#78200 (Add `ControlFlow::is_{break,continue}` methods) Failed merges: r? `@ghost`

rust-highfive assigned matthewjasper Oct 15, 2020

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 15, 2020

oliviacrain commented Oct 15, 2020

View reviewed changes

compiler/rustc_passes/src/liveness.rs Show resolved Hide resolved

camelid added A-inline-assembly Area: Inline assembly (`asm!(…)`) F-asm `#![feature(asm)]` (not `llvm_asm`) labels Oct 17, 2020

oliviacrain added 2 commits October 17, 2020 16:37

Mark inout asm! operands as used in liveness pass

cc0b718

Treat InOut variables like other input variables

fd193f2

nagisa reviewed Oct 17, 2020

View reviewed changes

oliviacrain added 2 commits October 18, 2020 23:51

Mark InOut operands as used in RWU table with write_place

17c6c59

Refactor liveness-issue-77915 to liveness-asm and improve tests

8f0bced

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 Oct 19, 2020

JohnTitor mentioned this pull request Oct 21, 2020

Rollup of 8 pull requests #78176

Closed

bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Oct 21, 2020

Limit liveness-asm tests to x86_64

2720b2d

Bless liveness-asm output

dc29c7a

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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Oct 21, 2020

jonas-schievink mentioned this pull request Oct 21, 2020

Rollup of 10 pull requests #78197

Closed

JohnTitor mentioned this pull request Oct 22, 2020

Rollup of 10 pull requests #78212

Merged

bors merged commit ae95005 into rust-lang:master Oct 22, 2020

rustbot added this to the 1.49.0 milestone Oct 22, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Mark inout asm! operands as used in liveness pass #77976

Mark inout asm! operands as used in liveness pass #77976

oliviacrain commented Oct 15, 2020

rust-highfive commented Oct 15, 2020

nagisa Oct 17, 2020

oliviacrain Oct 19, 2020

nagisa Oct 17, 2020

oliviacrain Oct 19, 2020

nagisa Oct 17, 2020 •

edited

Loading

oliviacrain Oct 19, 2020

matthewjasper commented Oct 19, 2020

bors commented Oct 19, 2020

JohnTitor commented Oct 21, 2020

oliviacrain commented Oct 21, 2020

JohnTitor commented Oct 21, 2020

oliviacrain commented Oct 21, 2020

JohnTitor commented Oct 21, 2020

bors commented Oct 21, 2020

		@@ -0,0 +1,36 @@
		// Ensure inout asm! operands are marked as used by the liveness pass

		// only-x86_64

Mark inout asm! operands as used in liveness pass #77976

Mark inout asm! operands as used in liveness pass #77976

Conversation

oliviacrain commented Oct 15, 2020

rust-highfive commented Oct 15, 2020

nagisa Oct 17, 2020

Choose a reason for hiding this comment

oliviacrain Oct 19, 2020

Choose a reason for hiding this comment

nagisa Oct 17, 2020

Choose a reason for hiding this comment

oliviacrain Oct 19, 2020

Choose a reason for hiding this comment

nagisa Oct 17, 2020 • edited Loading

Choose a reason for hiding this comment

oliviacrain Oct 19, 2020

Choose a reason for hiding this comment

matthewjasper commented Oct 19, 2020

bors commented Oct 19, 2020

JohnTitor commented Oct 21, 2020

oliviacrain commented Oct 21, 2020

JohnTitor commented Oct 21, 2020

oliviacrain commented Oct 21, 2020

JohnTitor commented Oct 21, 2020

bors commented Oct 21, 2020

nagisa Oct 17, 2020 •

edited

Loading