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

Update rustc analyses to reflect asm! order-of-eval: outputs, then inputs #14966

Conversation

pnkfelix
Copy link
Member

Fix #14962.

Update rustc analyses to reflect asm! order-of-eval: outputs, then inputs.

Also reordered the fields in libsyntax to reflect the order that the
expressions occur in an instance of asm!, as an attempt to remind
others of this ordering.

Finally, added a couple notes about cases that may need to be further
revised when/if #14936 is addressed.

@alexcrichton
Copy link
Member

Does this fix #14963, or does this just need to cc #14936? (just wanted to clarify)

@alexcrichton
Copy link
Member

Ah it seems you may have intended #14962. r=me with the links updated.

…puts.

Fix rust-lang#14962.

Also reordered the fields in libsyntax to reflect the order that the
expressions occur in an instance of `asm!`, as an attempt to remind
others of this ordering.

Finally, added a couple notes about cases that may need to be further
revised when/if rust-lang#14936 is addressed.
@pnkfelix
Copy link
Member Author

Okay the PR that I posted here is not quite right.

See this example (adapted from compile-fail/asm-misplaced-option.rs, but made to actually run):

#![feature(asm)]
#![allow(dead_code)]
#[cfg(target_arch = "x86")]
#[cfg(target_arch = "x86_64")]
pub fn main() {
    // assignment not dead
    let mut x: int = 17;
    unsafe {
        asm!("mov $1, $0"
             : "=r"(x)
             : "r"(5u),
               "0"({println!("eval input-2, x: {}", x); x}) : "cc");
    }
    assert_eq!(x, 5);
}

which prints this on my machine:

% ./x86_64-apple-darwin/stage1/bin/rustc  /tmp/asm-play.rs && ./asm-play 
/tmp/asm-play.rs:21:9: 21:14 warning: value assigned to `x` is never read, #[warn(dead_assignment)] on by default
/tmp/asm-play.rs:21     let mut x: int = 17;
                            ^~~~~
eval input-2, x: 17
%

i.e.: the changes in this PR are making rustc think that the output expressions are evaluated first, to the point where they actually overwrite their defined contents. But that is not what they do: the side-effects of the output expressions take place first, but the actual overwrite specified by the capture of a variable's address only happens within the asm! block itself, which will run after all of the input expressions have been evaluated.

I am going to need to do a more careful modification of the code involved here; it is unfortunately not as simple as just reordering the traversals to visit inputs before outputs, unfortunately.

(Part of me wonders if it would be simpler to just change trans to generate the output expressions after the inputs. But then we will lose the left-to-right evaluation ordering of the arguments to asm!.)

Closing this PR for now; I will revive it later after I come up with a better fix.

@pnkfelix pnkfelix closed this Jun 17, 2014
bors added a commit to rust-lang-ci/rust that referenced this pull request Jun 5, 2023
…r, r=HKalbasi

fix: consider outer binders when folding captured items' type

Fixes rust-lang#14966

Basically, the crash is caused by us producing a broken type and passing it to chalk: `&dyn for<type> [for<> Implemented(^1.0: A<^0.0>)]` (notice the innermost bound var `^0.0` has no corresponding binder). It's created in `CapturedItemWithoutTy::with_ty()`, which didn't consider outer binders when folding types to replace placeholders with bound variables.

The fix is one-liner, but I've also refactored the surrounding code a little.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

rustc's analyses have different order of eval for asm! than what trans emits.
3 participants