| Message ID | 20160118231114.GD3017@tucnak.redhat.com |
|---|---|
| State | New |
| Headers | show |
On Tue, 19 Jan 2016, Jakub Jelinek wrote: > Hi! > > Here is an attempt to fix ICE on statement expression in "m" asm input > operand. The problem is that gimplify_asm_expr attempts to mark it > addressable, but that can be just too late, a temporary the stmt-expression > gimplifies to might not be addressable and may be used already in the > gimplified code. Normally the C/C++ FEs attempt to mark the operand > addressable already, but in case of statement expression the temporaries > might not exist yet. > The patch turns also the PR29119 testcase into invalid test, but you've > already said in that PR it should be invalid and I agree with that. Hmm, but can't we detect this in the FE? > Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk? What happens if we just do _not_ mark the memory input addressable? Shouldn't IRA/LRA in the end satisfy the constraint by spilling a non-memory input and using the spill slot? Richard. > 2016-01-19 Jakub Jelinek <jakub@redhat.com> > > PR middle-end/67653 > * gimplify.c (gimplify_asm_expr): Error if it is too late to > attempt to mark memory input operand addressable. > > * c-c++-common/pr67653.c: New test. > * gcc.dg/torture/pr29119.c: Add dg-error. > > --- gcc/gimplify.c.jj 2016-01-15 20:37:30.000000000 +0100 > +++ gcc/gimplify.c 2016-01-18 16:05:21.125640974 +0100 > @@ -5305,6 +5305,27 @@ gimplify_asm_expr (tree *expr_p, gimple_ > TREE_VALUE (link) = error_mark_node; > tret = gimplify_expr (&TREE_VALUE (link), pre_p, post_p, > is_gimple_lvalue, fb_lvalue | fb_mayfail); > + if (tret != GS_ERROR) > + { > + /* Unlike output operands, memory inputs are not guaranteed > + to be lvalues by the FE, and while the expressions are > + marked addressable there, if it is e.g. a statement > + expression, temporaries in it might not end up being > + addressable. They might be already used in the IL and thus > + it is too late to make them addressable now though. */ > + tree x = TREE_VALUE (link); > + while (handled_component_p (x)) > + x = TREE_OPERAND (x, 0); > + if (TREE_CODE (x) == MEM_REF > + && TREE_CODE (TREE_OPERAND (x, 0)) == ADDR_EXPR) > + x = TREE_OPERAND (TREE_OPERAND (x, 0), 0); > + if ((TREE_CODE (x) == VAR_DECL > + || TREE_CODE (x) == PARM_DECL > + || TREE_CODE (x) == RESULT_DECL) > + && !TREE_ADDRESSABLE (x) > + && is_gimple_reg (x)) > + tret = GS_ERROR; > + } > mark_addressable (TREE_VALUE (link)); > if (tret == GS_ERROR) > { > --- gcc/testsuite/c-c++-common/pr67653.c.jj 2016-01-18 16:03:49.302899912 +0100 > +++ gcc/testsuite/c-c++-common/pr67653.c 2016-01-18 16:03:20.000000000 +0100 > @@ -0,0 +1,8 @@ > +/* PR middle-end/67653 */ > +/* { dg-do compile } */ > + > +void > +foo (void) > +{ > + __asm__ ("" : : "m" (({ static int a; a; }))); /* { dg-error "memory input 0 is not directly addressable" } */ > +} > --- gcc/testsuite/gcc.dg/torture/pr29119.c.jj 2014-09-25 15:02:28.000000000 +0200 > +++ gcc/testsuite/gcc.dg/torture/pr29119.c 2016-01-18 22:33:32.090515087 +0100 > @@ -2,6 +2,6 @@ > > void ldt_add_entry(void) > { > - __asm__ ("" :: "m"(({unsigned __v; __v;}))); > + __asm__ ("" :: "m"(({unsigned __v; __v;}))); /* { dg-error "memory input 0 is not directly addressable" } */ > } > > > Jakub > >
On Tue, Jan 19, 2016 at 10:00:00AM +0100, Richard Biener wrote: > On Tue, 19 Jan 2016, Jakub Jelinek wrote: > > Here is an attempt to fix ICE on statement expression in "m" asm input > > operand. The problem is that gimplify_asm_expr attempts to mark it > > addressable, but that can be just too late, a temporary the stmt-expression > > gimplifies to might not be addressable and may be used already in the > > gimplified code. Normally the C/C++ FEs attempt to mark the operand > > addressable already, but in case of statement expression the temporaries > > might not exist yet. > > The patch turns also the PR29119 testcase into invalid test, but you've > > already said in that PR it should be invalid and I agree with that. > > Hmm, but can't we detect this in the FE? We could diagnose a statement expression in "m", but not sure if that is all that can get wrong, or if all statement expressions are problematic. > > Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk? > > What happens if we just do _not_ mark the memory input addressable? > Shouldn't IRA/LRA in the end satisfy the constraint by spilling > a non-memory input and using the spill slot? Well, if you want to make broken testcases work, it is always possible to call say prepare_gimple_addressable, but I'd think it is preferrable to tell people that what they do is really going to do something different from what they expect (that the operand, while being a memory input, will be some temporary containing a copy of the value rather than than the variable itself. Jakub
On Tue, 19 Jan 2016, Jakub Jelinek wrote: > On Tue, Jan 19, 2016 at 10:00:00AM +0100, Richard Biener wrote: > > On Tue, 19 Jan 2016, Jakub Jelinek wrote: > > > Here is an attempt to fix ICE on statement expression in "m" asm input > > > operand. The problem is that gimplify_asm_expr attempts to mark it > > > addressable, but that can be just too late, a temporary the stmt-expression > > > gimplifies to might not be addressable and may be used already in the > > > gimplified code. Normally the C/C++ FEs attempt to mark the operand > > > addressable already, but in case of statement expression the temporaries > > > might not exist yet. > > > The patch turns also the PR29119 testcase into invalid test, but you've > > > already said in that PR it should be invalid and I agree with that. > > > > Hmm, but can't we detect this in the FE? > > We could diagnose a statement expression in "m", but not sure if that is all > that can get wrong, or if all statement expressions are problematic. I thought about either requiring an lvalue here or at least diagnosing that a non-lvalue might end up using a memory temporary. > > > Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk? > > > > What happens if we just do _not_ mark the memory input addressable? > > Shouldn't IRA/LRA in the end satisfy the constraint by spilling > > a non-memory input and using the spill slot? > > Well, if you want to make broken testcases work, it is always possible > to call say prepare_gimple_addressable, but I'd think it is preferrable > to tell people that what they do is really going to do something different > from what they expect (that the operand, while being a memory input, will > be some temporary containing a copy of the value rather than than the > variable itself. Sure, I'm just thinking that diagnosing sth at gimplification time feels wrong ... after all we can make it unexpected but valid GIMPLE. Erroring on a non-lvalue in the FE will likely break too much legacy code but I guess that might be a better choice than using a memory temporary (just in case we are faced with some fancy lock stuff). Richard.
On Wed, Jan 20, 2016 at 10:24:40AM +0100, Richard Biener wrote: > > We could diagnose a statement expression in "m", but not sure if that is all > > that can get wrong, or if all statement expressions are problematic. > > I thought about either requiring an lvalue here or at least diagnosing > that a non-lvalue might end up using a memory temporary. Requiring an lvalue sounds wrong, "m" input can be validly e.g. const object that can't be assigned to. Furthermore, I'm really afraid changing that would break too much stuff. We had until GCC 4.7 a warning: warning (0, "use of memory input without lvalue in " "asm operand %d is deprecated", i + noutputs); but 1) it wasn't anywhere near frontend, it was during expansion 2) it wasn't really checking lvalue, but whether the operand is a MEM or not > > > > > Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk? > > > > > > What happens if we just do _not_ mark the memory input addressable? > > > Shouldn't IRA/LRA in the end satisfy the constraint by spilling > > > a non-memory input and using the spill slot? > > > > Well, if you want to make broken testcases work, it is always possible > > to call say prepare_gimple_addressable, but I'd think it is preferrable > > to tell people that what they do is really going to do something different > > from what they expect (that the operand, while being a memory input, will > > be some temporary containing a copy of the value rather than than the > > variable itself. > > Sure, I'm just thinking that diagnosing sth at gimplification time > feels wrong ... after all we can make it unexpected but valid GIMPLE. We already do diagnose tons of other cases for inline asm at gimplification time. I can replace the error with a warning followed by copying it to addressable memory, that seems to be what the older gccs were doing during expansion after issuing above mentioned warning. Jakub
On Wed, 20 Jan 2016, Jakub Jelinek wrote: > On Wed, Jan 20, 2016 at 10:24:40AM +0100, Richard Biener wrote: > > > We could diagnose a statement expression in "m", but not sure if that is all > > > that can get wrong, or if all statement expressions are problematic. > > > > I thought about either requiring an lvalue here or at least diagnosing > > that a non-lvalue might end up using a memory temporary. > > Requiring an lvalue sounds wrong, "m" input can be validly e.g. const object that > can't be assigned to. Ok, so maybe the term "lvalue" is wrong but certainly an arbitrary rvalue is not "valid". > Furthermore, I'm really afraid changing that would break > too much stuff. > We had until GCC 4.7 a warning: > warning (0, "use of memory input without lvalue in " > "asm operand %d is deprecated", i + noutputs); > but 1) it wasn't anywhere near frontend, it was during expansion > 2) it wasn't really checking lvalue, but whether the operand is a MEM or not > > > > > > > Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk? > > > > > > > > What happens if we just do _not_ mark the memory input addressable? > > > > Shouldn't IRA/LRA in the end satisfy the constraint by spilling > > > > a non-memory input and using the spill slot? > > > > > > Well, if you want to make broken testcases work, it is always possible > > > to call say prepare_gimple_addressable, but I'd think it is preferrable > > > to tell people that what they do is really going to do something different > > > from what they expect (that the operand, while being a memory input, will > > > be some temporary containing a copy of the value rather than than the > > > variable itself. > > > > Sure, I'm just thinking that diagnosing sth at gimplification time > > feels wrong ... after all we can make it unexpected but valid GIMPLE. > > We already do diagnose tons of other cases for inline asm at > gimplification time. I can replace the error with a warning followed by > copying it to addressable memory, that seems to be what the older gccs were > doing during expansion after issuing above mentioned warning. That sounds better then though it would be nice to warn from the FE somehow. Richard.
--- gcc/gimplify.c.jj 2016-01-15 20:37:30.000000000 +0100 +++ gcc/gimplify.c 2016-01-18 16:05:21.125640974 +0100 @@ -5305,6 +5305,27 @@ gimplify_asm_expr (tree *expr_p, gimple_ TREE_VALUE (link) = error_mark_node; tret = gimplify_expr (&TREE_VALUE (link), pre_p, post_p, is_gimple_lvalue, fb_lvalue | fb_mayfail); + if (tret != GS_ERROR) + { + /* Unlike output operands, memory inputs are not guaranteed + to be lvalues by the FE, and while the expressions are + marked addressable there, if it is e.g. a statement + expression, temporaries in it might not end up being + addressable. They might be already used in the IL and thus + it is too late to make them addressable now though. */ + tree x = TREE_VALUE (link); + while (handled_component_p (x)) + x = TREE_OPERAND (x, 0); + if (TREE_CODE (x) == MEM_REF + && TREE_CODE (TREE_OPERAND (x, 0)) == ADDR_EXPR) + x = TREE_OPERAND (TREE_OPERAND (x, 0), 0); + if ((TREE_CODE (x) == VAR_DECL + || TREE_CODE (x) == PARM_DECL + || TREE_CODE (x) == RESULT_DECL) + && !TREE_ADDRESSABLE (x) + && is_gimple_reg (x)) + tret = GS_ERROR; + } mark_addressable (TREE_VALUE (link)); if (tret == GS_ERROR) { --- gcc/testsuite/c-c++-common/pr67653.c.jj 2016-01-18 16:03:49.302899912 +0100 +++ gcc/testsuite/c-c++-common/pr67653.c 2016-01-18 16:03:20.000000000 +0100 @@ -0,0 +1,8 @@ +/* PR middle-end/67653 */ +/* { dg-do compile } */ + +void +foo (void) +{ + __asm__ ("" : : "m" (({ static int a; a; }))); /* { dg-error "memory input 0 is not directly addressable" } */ +} --- gcc/testsuite/gcc.dg/torture/pr29119.c.jj 2014-09-25 15:02:28.000000000 +0200 +++ gcc/testsuite/gcc.dg/torture/pr29119.c 2016-01-18 22:33:32.090515087 +0100 @@ -2,6 +2,6 @@ void ldt_add_entry(void) { - __asm__ ("" :: "m"(({unsigned __v; __v;}))); + __asm__ ("" :: "m"(({unsigned __v; __v;}))); /* { dg-error "memory input 0 is not directly addressable" } */ }