Skip to content

Handle GCC's write-only inline asm constraint in liveness, borrowck and trans. #9850

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

Merged
merged 1 commit into from
Oct 18, 2013
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion src/librustc/middle/borrowck/check_loans.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
// Copyright 2012 The Rust Project Developers. See the COPYRIGHT
// Copyright 2012-2013 The Rust Project Developers. See the COPYRIGHT
// file at the top-level directory of this distribution and at
// http://rust-lang.org/COPYRIGHT.
//
Expand Down Expand Up @@ -839,6 +839,11 @@ fn check_loans_in_expr<'a>(this: &mut CheckLoanCtxt<'a>,
expr.span,
[]);
}
ast::ExprInlineAsm(ref ia) => {
for &(_, out) in ia.outputs.iter() {
this.check_assignment(out);
}
}
_ => { }
}
}
Expand Down
19 changes: 18 additions & 1 deletion src/librustc/middle/borrowck/gather_loans/mod.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
// Copyright 2012 The Rust Project Developers. See the COPYRIGHT
// Copyright 2012-2013 The Rust Project Developers. See the COPYRIGHT
// file at the top-level directory of this distribution and at
// http://rust-lang.org/COPYRIGHT.
//
Expand Down Expand Up @@ -309,6 +309,23 @@ fn gather_loans_in_expr(this: &mut GatherLoanCtxt,
visit::walk_expr(this, ex, ());
}

ast::ExprInlineAsm(ref ia) => {
for &(_, out) in ia.outputs.iter() {
let out_cmt = this.bccx.cat_expr(out);
match opt_loan_path(out_cmt) {
Some(out_lp) => {
gather_moves::gather_assignment(this.bccx, this.move_data,
ex.id, ex.span,
out_lp, out.id);
}
None => {
// See the comment for ExprAssign.
}
}
}
visit::walk_expr(this, ex, ());
}

_ => {
visit::walk_expr(this, ex, ());
}
Expand Down
16 changes: 7 additions & 9 deletions src/librustc/middle/liveness.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
// Copyright 2012 The Rust Project Developers. See the COPYRIGHT
// Copyright 2012-2013 The Rust Project Developers. See the COPYRIGHT
// file at the top-level directory of this distribution and at
// http://rust-lang.org/COPYRIGHT.
//
Expand Down Expand Up @@ -1227,12 +1227,15 @@ impl Liveness {
self.propagate_through_expr(e, succ)
}

ExprInlineAsm(ref ia) =>{
ExprInlineAsm(ref ia) => {
let succ = do ia.inputs.rev_iter().fold(succ) |succ, &(_, expr)| {
Copy link
Member

Choose a reason for hiding this comment

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

I'm not entirely sure about this, but ExprAssign at least calls this after the outputs have been handled. Could you add a few test cases with colorful usage of inputs and outputs to make sure that this tracks them correctly? One case off the top of my head that I can think of is a simultaneous use and assignment of an uninitialized variable should be disallowed.

self.propagate_through_expr(expr, succ)
};
do ia.outputs.rev_iter().fold(succ) |succ, &(_, expr)| {
self.propagate_through_expr(expr, succ)
// see comment on lvalues in
// propagate_through_lvalue_components()
let succ = self.write_lvalue(expr, succ, ACC_WRITE);
self.propagate_through_lvalue_components(expr, succ)
}
}

Expand Down Expand Up @@ -1478,12 +1481,7 @@ fn check_expr(this: &mut Liveness, expr: @Expr) {

// Output operands must be lvalues
for &(_, out) in ia.outputs.iter() {
match out.node {
ExprAddrOf(_, inner) => {
this.check_lvalue(inner);
}
_ => {}
}
this.check_lvalue(out);
this.visit_expr(out, ());
}

Expand Down
47 changes: 14 additions & 33 deletions src/librustc/middle/trans/asm.rs
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,8 @@ use lib;
use middle::trans::build::*;
use middle::trans::callee;
use middle::trans::common::*;
use middle::trans::expr::*;
use middle::trans::type_of::*;
use middle::ty;

use middle::trans::type_::Type;
Expand All @@ -30,34 +32,15 @@ pub fn trans_inline_asm(bcx: @mut Block, ia: &ast::inline_asm) -> @mut Block {
let mut bcx = bcx;
let mut constraints = ~[];
let mut cleanups = ~[];
let mut aoutputs = ~[];
let mut output_types = ~[];

// Prepare the output operands
let outputs = do ia.outputs.map |&(c, out)| {
constraints.push(c);

aoutputs.push(unpack_result!(bcx, {
callee::trans_arg_expr(bcx,
expr_ty(bcx, out),
ty::ByCopy,
out,
&mut cleanups,
callee::DontAutorefArg)
}));

let e = match out.node {
ast::ExprAddrOf(_, e) => e,
_ => fail2!("Expression must be addr of")
};

unpack_result!(bcx, {
callee::trans_arg_expr(bcx,
expr_ty(bcx, e),
ty::ByCopy,
e,
&mut cleanups,
callee::DontAutorefArg)
})
let out_datum = unpack_datum!(bcx, trans_to_datum(bcx, out));
output_types.push(type_of(bcx.ccx(), out_datum.ty));
out_datum.val

};

Expand Down Expand Up @@ -92,7 +75,7 @@ pub fn trans_inline_asm(bcx: @mut Block, ia: &ast::inline_asm) -> @mut Block {
clobbers = format!("{},{}", ia.clobbers, clobbers);
} else {
clobbers.push_str(ia.clobbers);
};
}

// Add the clobbers to our constraints list
if clobbers.len() != 0 && constraints.len() != 0 {
Expand All @@ -107,12 +90,12 @@ pub fn trans_inline_asm(bcx: @mut Block, ia: &ast::inline_asm) -> @mut Block {
let numOutputs = outputs.len();

// Depending on how many outputs we have, the return type is different
let output = if numOutputs == 0 {
let output_type = if numOutputs == 0 {
Type::void()
} else if numOutputs == 1 {
val_ty(outputs[0])
output_types[0]
} else {
Type::struct_(outputs.map(|o| val_ty(*o)), false)
Type::struct_(output_types, false)
};

let dialect = match ia.dialect {
Expand All @@ -122,19 +105,17 @@ pub fn trans_inline_asm(bcx: @mut Block, ia: &ast::inline_asm) -> @mut Block {

let r = do ia.asm.with_c_str |a| {
do constraints.with_c_str |c| {
InlineAsmCall(bcx, a, c, inputs, output, ia.volatile, ia.alignstack, dialect)
InlineAsmCall(bcx, a, c, inputs, output_type, ia.volatile, ia.alignstack, dialect)
}
};

// Again, based on how many outputs we have
if numOutputs == 1 {
let op = PointerCast(bcx, aoutputs[0], val_ty(outputs[0]).ptr_to());
Store(bcx, r, op);
Store(bcx, r, outputs[0]);
} else {
for (i, o) in aoutputs.iter().enumerate() {
for (i, o) in outputs.iter().enumerate() {
let v = ExtractValue(bcx, r, i);
let op = PointerCast(bcx, *o, val_ty(outputs[i]).ptr_to());
Store(bcx, v, op);
Store(bcx, v, *o);
}
}

Expand Down
21 changes: 15 additions & 6 deletions src/libsyntax/ext/asm.rs
Original file line number Diff line number Diff line change
Expand Up @@ -75,16 +75,18 @@ pub fn expand_asm(cx: @ExtCtxt, sp: Span, tts: &[ast::token_tree])
}

let (constraint, _str_style) = p.parse_str();

if constraint.starts_with("+") {
cx.span_unimpl(*p.last_span,
"'+' (read+write) output operand constraint modifier");
} else if !constraint.starts_with("=") {
cx.span_err(*p.last_span, "output operand constraint lacks '='");
}

p.expect(&token::LPAREN);
let out = p.parse_expr();
p.expect(&token::RPAREN);

let out = @ast::Expr {
id: ast::DUMMY_NODE_ID,
span: out.span,
node: ast::ExprAddrOf(ast::MutMutable, out)
};

outputs.push((constraint, out));
}
}
Expand All @@ -98,6 +100,13 @@ pub fn expand_asm(cx: @ExtCtxt, sp: Span, tts: &[ast::token_tree])
}

let (constraint, _str_style) = p.parse_str();

if constraint.starts_with("=") {
cx.span_err(*p.last_span, "input operand constraint contains '='");
} else if constraint.starts_with("+") {
cx.span_err(*p.last_span, "input operand constraint contains '+'");
}

p.expect(&token::LPAREN);
let input = p.parse_expr();
p.expect(&token::RPAREN);
Expand Down
27 changes: 27 additions & 0 deletions src/test/compile-fail/asm-in-bad-modifier.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
// Copyright 2012-2013 The Rust Project Developers. See the COPYRIGHT
// file at the top-level directory of this distribution and at
// http://rust-lang.org/COPYRIGHT.
//
// Licensed under the Apache License, Version 2.0 <LICENSE-APACHE or
// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license
// <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your
// option. This file may not be copied, modified, or distributed
// except according to those terms.

fn foo(x: int) { info2!("{}", x); }

#[cfg(target_arch = "x86")]
#[cfg(target_arch = "x86_64")]
pub fn main() {
let x: int;
let y: int;
unsafe {
asm!("mov $1, $0" : "=r"(x) : "=r"(5u)); //~ ERROR input operand constraint contains '='
asm!("mov $1, $0" : "=r"(y) : "+r"(5u)); //~ ERROR input operand constraint contains '+'
}
foo(x);
foo(y);
}

#[cfg(not(target_arch = "x86"), not(target_arch = "x86_64"))]
pub fn main() {}
26 changes: 26 additions & 0 deletions src/test/compile-fail/asm-out-assign-imm.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
// Copyright 2012-2013 The Rust Project Developers. See the COPYRIGHT
// file at the top-level directory of this distribution and at
// http://rust-lang.org/COPYRIGHT.
//
// Licensed under the Apache License, Version 2.0 <LICENSE-APACHE or
// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license
// <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your
// option. This file may not be copied, modified, or distributed
// except according to those terms.

fn foo(x: int) { info2!("{}", x); }

#[cfg(target_arch = "x86")]
#[cfg(target_arch = "x86_64")]
pub fn main() {
let x: int;
x = 1; //~ NOTE prior assignment occurs here
foo(x);
unsafe {
asm!("mov $1, $0" : "=r"(x) : "r"(5u)); //~ ERROR re-assignment of immutable variable `x`
Copy link
Member

Choose a reason for hiding this comment

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

All of these tests will probably only work on x86 and x64 architectures. I don't think that we have a // xfail-arm or a directive like that, but could you put these in a #[cfg(target_arch = ...)] function which is defined for all other architectures as well? It wouldn't do anything useful on arm, but it would at least still compile on arm.

Copy link
Member

Choose a reason for hiding this comment

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

I feel like it might work since arm does have a mov instruction and there doesn't seem to be any x86 specific stuff. But yea, for anything more complicated definitely using target_arch is the way to go.

Copy link
Member

Choose a reason for hiding this comment

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

asm! is a very platform-specific macro, and even if it "happens to work", I'd rather not rely on it.

Copy link
Member

Choose a reason for hiding this comment

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

The best way to do this IMO is to have another function that main calls that has the per-arch tests in it. That way if we port to a new arch, this test won't silently do nothing.

Copy link
Member Author

Choose a reason for hiding this comment

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

While grepping for target_arch, I noticed MIPS. I didn't think of that, and it might differ from x86 even more than ARM.

}
foo(x);
}

#[cfg(not(target_arch = "x86"), not(target_arch = "x86_64"))]
pub fn main() {}
24 changes: 24 additions & 0 deletions src/test/compile-fail/asm-out-no-modifier.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
// Copyright 2012-2013 The Rust Project Developers. See the COPYRIGHT
// file at the top-level directory of this distribution and at
// http://rust-lang.org/COPYRIGHT.
//
// Licensed under the Apache License, Version 2.0 <LICENSE-APACHE or
// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license
// <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your
// option. This file may not be copied, modified, or distributed
// except according to those terms.

fn foo(x: int) { info2!("{}", x); }

#[cfg(target_arch = "x86")]
#[cfg(target_arch = "x86_64")]
pub fn main() {
let x: int;
unsafe {
asm!("mov $1, $0" : "r"(x) : "r"(5u)); //~ ERROR output operand constraint lacks '='
}
foo(x);
}

#[cfg(not(target_arch = "x86"), not(target_arch = "x86_64"))]
pub fn main() {}
24 changes: 24 additions & 0 deletions src/test/compile-fail/asm-out-read-uninit.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
// Copyright 2012-2013 The Rust Project Developers. See the COPYRIGHT
// file at the top-level directory of this distribution and at
// http://rust-lang.org/COPYRIGHT.
//
// Licensed under the Apache License, Version 2.0 <LICENSE-APACHE or
// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license
// <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your
// option. This file may not be copied, modified, or distributed
// except according to those terms.

fn foo(x: int) { info2!("{}", x); }

#[cfg(target_arch = "x86")]
#[cfg(target_arch = "x86_64")]
pub fn main() {
let x: int;
unsafe {
asm!("mov $1, $0" : "=r"(x) : "r"(x)); //~ ERROR use of possibly uninitialized value: `x`
}
foo(x);
}

#[cfg(not(target_arch = "x86"), not(target_arch = "x86_64"))]
pub fn main() {}
32 changes: 32 additions & 0 deletions src/test/run-pass/asm-out-assign.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
// Copyright 2012-2013 The Rust Project Developers. See the COPYRIGHT
// file at the top-level directory of this distribution and at
// http://rust-lang.org/COPYRIGHT.
//
// Licensed under the Apache License, Version 2.0 <LICENSE-APACHE or
// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license
// <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your
// option. This file may not be copied, modified, or distributed
// except according to those terms.

#[cfg(target_arch = "x86")]
#[cfg(target_arch = "x86_64")]
pub fn main() {
let x: int;
unsafe {
// Treat the output as initialization.
asm!("mov $1, $0" : "=r"(x) : "r"(5u));
}
assert_eq!(x, 5);

let mut x = x + 1;
assert_eq!(x, 6);

unsafe {
// Assignment to mutable.
asm!("mov $1, $0" : "=r"(x) : "r"(x + 7));
}
assert_eq!(x, 13);
}

#[cfg(not(target_arch = "x86"), not(target_arch = "x86_64"))]
pub fn main() {}