Fix double evaluation of read+write operands

pczarn · pczarn · commit 4155643428f1 · 2014-08-19T20:39:26.000+01:00
Stop read+write expressions from expanding into two occurences in the AST. Add a bool to indicate whether an operand in output position if read+write or not. Fixes #14936
diff --git a/src/librustc/middle/cfg/construct.rs b/src/librustc/middle/cfg/construct.rs
@@ -473,14 +473,15 @@ impl<'a> CFGBuilder<'a> {
             ast::ExprInlineAsm(ref inline_asm) => {
                 let inputs = inline_asm.inputs.iter();
                 let outputs = inline_asm.outputs.iter();
-                fn extract_expr<A>(&(_, expr): &(A, Gc<ast::Expr>)) -> Gc<ast::Expr> { expr }
                 let post_inputs = self.exprs(inputs.map(|a| {
                     debug!("cfg::construct InlineAsm id:{} input:{:?}", expr.id, a);
-                    extract_expr(a)
+                    let &(_, expr) = a;
+                    expr
                 }), pred);
                 let post_outputs = self.exprs(outputs.map(|a| {
                     debug!("cfg::construct InlineAsm id:{} output:{:?}", expr.id, a);
-                    extract_expr(a)
+                    let &(_, expr, _) = a;
+                    expr
                 }), post_inputs);
                 self.add_node(expr.id, [post_outputs])
             }
diff --git a/src/librustc/middle/expr_use_visitor.rs b/src/librustc/middle/expr_use_visitor.rs
@@ -380,8 +380,9 @@ impl<'d,'t,TYPER:mc::Typer> ExprUseVisitor<'d,'t,TYPER> {
                     self.consume_expr(&**input);
                 }
 
-                for &(_, ref output) in ia.outputs.iter() {
-                    self.mutate_expr(expr, &**output, JustWrite);
+                for &(_, ref output, is_rw) in ia.outputs.iter() {
+                    self.mutate_expr(expr, &**output,
+                                           if is_rw { WriteAndRead } else { JustWrite });
                 }
             }
 
diff --git a/src/librustc/middle/liveness.rs b/src/librustc/middle/liveness.rs
@@ -1192,9 +1192,10 @@ impl<'a> Liveness<'a> {
           }
 
           ExprInlineAsm(ref ia) => {
-            let succ = ia.outputs.iter().rev().fold(succ, |succ, &(_, ref expr)| {
-                // see comment on lvalues in
-                // propagate_through_lvalue_components()
+
+            let succ = ia.outputs.iter().rev().fold(succ, |succ, &(_, ref expr, _)| {
+                // 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)
             });
@@ -1437,7 +1438,7 @@ fn check_expr(this: &mut Liveness, expr: &Expr) {
         }
 
         // Output operands must be lvalues
-        for &(_, ref out) in ia.outputs.iter() {
+        for &(_, ref out, _) in ia.outputs.iter() {
           this.check_lvalue(&**out);
           this.visit_expr(&**out, ());
         }
diff --git a/src/librustc/middle/trans/asm.rs b/src/librustc/middle/trans/asm.rs
@@ -36,13 +36,27 @@ pub fn trans_inline_asm<'a>(bcx: &'a Block<'a>, ia: &ast::InlineAsm)
 
     let temp_scope = fcx.push_custom_cleanup_scope();
 
+    let mut ext_inputs = Vec::new();
+    let mut ext_constraints = Vec::new();
+
     // Prepare the output operands
-    let outputs = ia.outputs.iter().map(|&(ref c, ref out)| {
+    let outputs = ia.outputs.iter().enumerate().map(|(i, &(ref c, ref out, is_rw))| {
         constraints.push((*c).clone());
 
         let out_datum = unpack_datum!(bcx, expr::trans(bcx, &**out));
         output_types.push(type_of::type_of(bcx.ccx(), out_datum.ty));
-        out_datum.val
+        let val = out_datum.val;
+        if is_rw {
+            ext_inputs.push(unpack_result!(bcx, {
+                callee::trans_arg_datum(bcx,
+                                       expr_ty(bcx, &**out),
+                                       out_datum,
+                                       cleanup::CustomScope(temp_scope),
+                                       callee::DontAutorefArg)
+            }));
+            ext_constraints.push(i.to_string());
+        }
+        val
 
     }).collect::<Vec<_>>();
 
@@ -58,14 +72,15 @@ pub fn trans_inline_asm<'a>(bcx: &'a Block<'a>, ia: &ast::InlineAsm)
                                    cleanup::CustomScope(temp_scope),
                                    callee::DontAutorefArg)
         })
-    }).collect::<Vec<_>>();
+    }).collect::<Vec<_>>().append(ext_inputs.as_slice());
 
     // no failure occurred preparing operands, no need to cleanup
     fcx.pop_custom_cleanup_scope(temp_scope);
 
     let mut constraints =
         String::from_str(constraints.iter()
                                     .map(|s| s.get().to_string())
+                                    .chain(ext_constraints.move_iter())
                                     .collect::<Vec<String>>()
                                     .connect(",")
                                     .as_slice());
diff --git a/src/librustc/middle/trans/debuginfo.rs b/src/librustc/middle/trans/debuginfo.rs
@@ -3729,7 +3729,7 @@ fn populate_scope_map(cx: &CrateContext,
                     walk_expr(cx, &**exp, scope_stack, scope_map);
                 }
 
-                for &(_, ref exp) in outputs.iter() {
+                for &(_, ref exp, _) in outputs.iter() {
                     walk_expr(cx, &**exp, scope_stack, scope_map);
                 }
             }
diff --git a/src/librustc/middle/typeck/check/mod.rs b/src/librustc/middle/typeck/check/mod.rs
@@ -3332,7 +3332,7 @@ fn check_expr_with_unifier(fcx: &FnCtxt,
           for &(_, ref input) in ia.inputs.iter() {
               check_expr(fcx, &**input);
           }
-          for &(_, ref out) in ia.outputs.iter() {
+          for &(_, ref out, _) in ia.outputs.iter() {
               check_expr(fcx, &**out);
           }
           fcx.write_nil(id);
diff --git a/src/libsyntax/ast.rs b/src/libsyntax/ast.rs
@@ -958,9 +958,9 @@ pub enum AsmDialect {
 pub struct InlineAsm {
     pub asm: InternedString,
     pub asm_str_style: StrStyle,
-    pub clobbers: InternedString,
+    pub outputs: Vec<(InternedString, Gc<Expr>, bool)>,
     pub inputs: Vec<(InternedString, Gc<Expr>)>,
-    pub outputs: Vec<(InternedString, Gc<Expr>)>,
+    pub clobbers: InternedString,
     pub volatile: bool,
     pub alignstack: bool,
     pub dialect: AsmDialect
diff --git a/src/libsyntax/ext/asm.rs b/src/libsyntax/ext/asm.rs
@@ -59,8 +59,6 @@ pub fn expand_asm(cx: &mut ExtCtxt, sp: Span, tts: &[ast::TokenTree])
 
     let mut state = Asm;
 
-    let mut read_write_operands = Vec::new();
-
     'statement: loop {
         match state {
             Asm => {
@@ -100,8 +98,6 @@ pub fn expand_asm(cx: &mut ExtCtxt, sp: Span, tts: &[ast::TokenTree])
                     let output = match constraint.get().slice_shift_char() {
                         (Some('='), _) => None,
                         (Some('+'), operand) => {
-                            // Save a reference to the output
-                            read_write_operands.push((outputs.len(), out));
                             Some(token::intern_and_get_ident(format!(
                                         "={}",
                                         operand).as_slice()))
@@ -112,7 +108,8 @@ pub fn expand_asm(cx: &mut ExtCtxt, sp: Span, tts: &[ast::TokenTree])
                         }
                     };
 
-                    outputs.push((output.unwrap_or(constraint), out));
+                    let is_rw = output.is_some();
+                    outputs.push((output.unwrap_or(constraint), out, is_rw));
                 }
             }
             Inputs => {
@@ -202,21 +199,14 @@ pub fn expand_asm(cx: &mut ExtCtxt, sp: Span, tts: &[ast::TokenTree])
         }
     }
 
-    // Append an input operand, with the form of ("0", expr)
-    // that links to an output operand.
-    for &(i, out) in read_write_operands.iter() {
-        inputs.push((token::intern_and_get_ident(i.to_string().as_slice()),
-                                                 out));
-    }
-
     MacExpr::new(box(GC) ast::Expr {
         id: ast::DUMMY_NODE_ID,
         node: ast::ExprInlineAsm(ast::InlineAsm {
             asm: token::intern_and_get_ident(asm.get()),
             asm_str_style: asm_str_style.unwrap(),
-            clobbers: token::intern_and_get_ident(cons.as_slice()),
-            inputs: inputs,
             outputs: outputs,
+            inputs: inputs,
+            clobbers: token::intern_and_get_ident(cons.as_slice()),
             volatile: volatile,
             alignstack: alignstack,
             dialect: dialect
diff --git a/src/libsyntax/fold.rs b/src/libsyntax/fold.rs
@@ -1189,8 +1189,8 @@ pub fn noop_fold_expr<T: Folder>(e: Gc<Expr>, folder: &mut T) -> Gc<Expr> {
                 inputs: a.inputs.iter().map(|&(ref c, input)| {
                     ((*c).clone(), folder.fold_expr(input))
                 }).collect(),
-                outputs: a.outputs.iter().map(|&(ref c, out)| {
-                    ((*c).clone(), folder.fold_expr(out))
+                outputs: a.outputs.iter().map(|&(ref c, out, is_rw)| {
+                    ((*c).clone(), folder.fold_expr(out), is_rw)
                 }).collect(),
                 .. (*a).clone()
             })
diff --git a/src/libsyntax/print/pprust.rs b/src/libsyntax/print/pprust.rs
@@ -1671,8 +1671,14 @@ impl<'a> State<'a> {
                 try!(self.word_space(":"));
 
                 try!(self.commasep(Inconsistent, a.outputs.as_slice(),
-                                   |s, &(ref co, ref o)| {
-                    try!(s.print_string(co.get(), ast::CookedStr));
+                                   |s, &(ref co, ref o, is_rw)| {
+                    match co.get().slice_shift_char() {
+                        (Some('='), operand) if is_rw => {
+                            try!(s.print_string(format!("+{}", operand).as_slice(),
+                                                ast::CookedStr))
+                        }
+                        _ => try!(s.print_string(co.get(), ast::CookedStr))
+                    }
                     try!(s.popen());
                     try!(s.print_expr(&**o));
                     try!(s.pclose());
diff --git a/src/libsyntax/visit.rs b/src/libsyntax/visit.rs
@@ -854,11 +854,11 @@ pub fn walk_expr<E: Clone, V: Visitor<E>>(visitor: &mut V, expression: &Expr, en
         ExprParen(ref subexpression) => {
             visitor.visit_expr(&**subexpression, env.clone())
         }
-        ExprInlineAsm(ref assembler) => {
-            for &(_, ref input) in assembler.inputs.iter() {
+        ExprInlineAsm(ref ia) => {
+            for &(_, ref input) in ia.inputs.iter() {
                 visitor.visit_expr(&**input, env.clone())
             }
-            for &(_, ref output) in assembler.outputs.iter() {
+            for &(_, ref output, _) in ia.outputs.iter() {
                 visitor.visit_expr(&**output, env.clone())
             }
         }
diff --git a/src/test/run-pass/issue-14936.rs b/src/test/run-pass/issue-14936.rs
@@ -0,0 +1,54 @@
+// Copyright 2012-2014 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.
+
+#![feature(asm, macro_rules)]
+
+type History = Vec<&'static str>;
+
+fn wrap<A>(x:A, which: &'static str, history: &mut History) -> A {
+    history.push(which);
+    x
+}
+
+macro_rules! demo {
+    ( $output_constraint:tt ) => {
+        {
+            let mut x: int = 0;
+            let y: int = 1;
+
+            let mut history: History = vec!();
+            unsafe {
+                asm!("mov ($1), $0"
+                     : $output_constraint (*wrap(&mut x, "out", &mut history))
+                     : "r"(&wrap(y, "in", &mut history)));
+            }
+            assert_eq!((x,y), (1,1));
+            assert_eq!(history.as_slice(), &["out", "in"]);
+        }
+    }
+}
+
+#[cfg(target_arch = "x86")]
+#[cfg(target_arch = "x86_64")]
+fn main() {
+    fn out_write_only_expr_then_in_expr() {
+        demo!("=r")
+    }
+
+    fn out_read_write_expr_then_in_expr() {
+        demo!("+r")
+    }
+
+    out_write_only_expr_then_in_expr();
+    out_read_write_expr_then_in_expr();
+}
+
+#[cfg(not(target_arch = "x86"), not(target_arch = "x86_64"))]
+pub fn main() {}

Original file line number	Diff line number	Diff line change
`@@ -380,8 +380,9 @@ impl<'d,'t,TYPER:mc::Typer> ExprUseVisitor<'d,'t,TYPER> {`
`380`	`380`	`self.consume_expr(&**input);`
`381`	`381`	`}`
`382`	`382`
`383`		`- for &(_, ref output) in ia.outputs.iter() {`
`384`		`- self.mutate_expr(expr, &**output, JustWrite);`
	`383`	`+ for &(_, ref output, is_rw) in ia.outputs.iter() {`
	`384`	`+ self.mutate_expr(expr, &**output,`
	`385`	`+ if is_rw { WriteAndRead } else { JustWrite });`
`385`	`386`	`}`
`386`	`387`	`}`
`387`	`388`
Original file line number	Diff line number	Diff line change
`@@ -3729,7 +3729,7 @@ fn populate_scope_map(cx: &CrateContext,`
`3729`	`3729`	`walk_expr(cx, &**exp, scope_stack, scope_map);`
`3730`	`3730`	`}`
`3731`	`3731`
`3732`		`- for &(_, ref exp) in outputs.iter() {`
	`3732`	`+ for &(_, ref exp, _) in outputs.iter() {`
`3733`	`3733`	`walk_expr(cx, &**exp, scope_stack, scope_map);`
`3734`	`3734`	`}`
`3735`	`3735`	`}`
Original file line number	Diff line number	Diff line change
`@@ -3332,7 +3332,7 @@ fn check_expr_with_unifier(fcx: &FnCtxt,`
`3332`	`3332`	`for &(_, ref input) in ia.inputs.iter() {`
`3333`	`3333`	`check_expr(fcx, &**input);`
`3334`	`3334`	`}`
`3335`		`- for &(_, ref out) in ia.outputs.iter() {`
	`3335`	`+ for &(_, ref out, _) in ia.outputs.iter() {`
`3336`	`3336`	`check_expr(fcx, &**out);`
`3337`	`3337`	`}`
`3338`	`3338`	`fcx.write_nil(id);`