Message ID | 20191105073939.GP4650@tucnak |
---|---|
State | New |
Headers | show |
Series | Reject VLAs in inline asm operands that require registers (PR inline-asm/92352) | expand |
On Tue, 5 Nov 2019, Jakub Jelinek wrote: > Hi! > > On VLAs with register only constraints we ICE, because during gimplification > we try to create temporaries for them and force_constant_size aborts in that > case. > > The following patch diagnoses those early, like we diagnose already C++ > non-PODs. > > Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk? > > 2019-11-05 Jakub Jelinek <jakub@redhat.com> > > PR inline-asm/92352 > * gimplify.c (gimplify_asm_expr): Reject VLA in output or input > operands with non-memory constraints. > > * c-c++-common/pr92352.c: New test. > > --- gcc/gimplify.c.jj 2019-11-02 10:00:59.595253274 +0100 > +++ gcc/gimplify.c 2019-11-05 00:21:01.585958514 +0100 > @@ -6235,8 +6235,14 @@ gimplify_asm_expr (tree *expr_p, gimple_ > is_inout = false; > } > > - /* If we can't make copies, we can only accept memory. */ > - if (TREE_ADDRESSABLE (TREE_TYPE (TREE_VALUE (link)))) > + /* If we can't make copies, we can only accept memory. > + Similarly for VLAs. */ > + tree outtype = TREE_TYPE (TREE_VALUE (link)); > + if (outtype != error_mark_node so for error_mark_node we don't diagnose anything? > + && (TREE_ADDRESSABLE (outtype) > + || !COMPLETE_TYPE_P (outtype) > + || (!tree_fits_poly_uint64_p (TYPE_SIZE_UNIT (outtype)) > + && max_int_size_in_bytes (outtype)))) so max_int_size_in_bytes == 0 is OK? I suppose we have a testcase for this? Otherwise looks reasonable to me. Thanks, Richard. > { > if (allows_mem) > allows_reg = 0; > @@ -6392,7 +6398,12 @@ gimplify_asm_expr (tree *expr_p, gimple_ > oconstraints, &allows_mem, &allows_reg); > > /* If we can't make copies, we can only accept memory. */ > - if (TREE_ADDRESSABLE (TREE_TYPE (TREE_VALUE (link)))) > + tree intype = TREE_TYPE (TREE_VALUE (link)); > + if (intype != error_mark_node > + && (TREE_ADDRESSABLE (intype) > + || !COMPLETE_TYPE_P (intype) > + || (!tree_fits_poly_uint64_p (TYPE_SIZE_UNIT (intype)) > + && max_int_size_in_bytes (intype)))) > { > if (allows_mem) > allows_reg = 0; > --- gcc/testsuite/c-c++-common/pr92352.c.jj 2019-11-04 14:03:18.725275255 +0100 > +++ gcc/testsuite/c-c++-common/pr92352.c 2019-11-04 14:02:55.211629675 +0100 > @@ -0,0 +1,15 @@ > +/* PR inline-asm/92352 */ > + > +void > +foo (int x) > +{ > + int var[x]; > + asm volatile ("" : "+r" (var)); /* { dg-error "impossible constraint in 'asm'" } */ > +} /* { dg-error "non-memory output 0 must stay in memory" "" { target *-*-* } .-1 } */ > + > +void > +bar (int x) > +{ > + int var[x]; > + asm volatile ("" : "+m" (var)); > +} > > Jakub > >
On Tue, Nov 05, 2019 at 09:27:45AM +0100, Richard Biener wrote: > > --- gcc/gimplify.c.jj 2019-11-02 10:00:59.595253274 +0100 > > +++ gcc/gimplify.c 2019-11-05 00:21:01.585958514 +0100 > > @@ -6235,8 +6235,14 @@ gimplify_asm_expr (tree *expr_p, gimple_ > > is_inout = false; > > } > > > > - /* If we can't make copies, we can only accept memory. */ > > - if (TREE_ADDRESSABLE (TREE_TYPE (TREE_VALUE (link)))) > > + /* If we can't make copies, we can only accept memory. > > + Similarly for VLAs. */ > > + tree outtype = TREE_TYPE (TREE_VALUE (link)); > > + if (outtype != error_mark_node > > so for error_mark_node we don't diagnose anything? Yes, we should have diagnosed it already. The != error_mark_node I've added only after seeing tons of ICEs in the testsuite with earlier version of the patch. > > + && (TREE_ADDRESSABLE (outtype) > > + || !COMPLETE_TYPE_P (outtype) > > + || (!tree_fits_poly_uint64_p (TYPE_SIZE_UNIT (outtype)) > > + && max_int_size_in_bytes (outtype)))) > > so max_int_size_in_bytes == 0 is OK? I suppose we have a testcase > for this? Actually, I meant max_int_size_in_bytes (outtype) < 0, i.e. something on which force_constant_size ICE immediately, sorry for screwing it up in the end. All these VLAs with max_int_size_in_bytes >= 0 sizes are specific to Ada and I have no idea what is and isn't valid there, for C/C++ it should always return -1. > Otherwise looks reasonable to me. So, is the following ok if it passes bootstrap/regtest, or shall I just go for || !tree_fits_poly_uint64_p (TYPE_SIZE_UNIT (outtype))) ? 2019-11-05 Jakub Jelinek <jakub@redhat.com> PR inline-asm/92352 * gimplify.c (gimplify_asm_expr): Reject VLA in output or input operands with non-memory constraints. * c-c++-common/pr92352.c: New test. --- gcc/gimplify.c.jj 2019-11-02 10:00:59.595253274 +0100 +++ gcc/gimplify.c 2019-11-05 00:21:01.585958514 +0100 @@ -6235,8 +6235,14 @@ gimplify_asm_expr (tree *expr_p, gimple_ is_inout = false; } - /* If we can't make copies, we can only accept memory. */ - if (TREE_ADDRESSABLE (TREE_TYPE (TREE_VALUE (link)))) + /* If we can't make copies, we can only accept memory. + Similarly for VLAs. */ + tree outtype = TREE_TYPE (TREE_VALUE (link)); + if (outtype != error_mark_node + && (TREE_ADDRESSABLE (outtype) + || !COMPLETE_TYPE_P (outtype) + || (!tree_fits_poly_uint64_p (TYPE_SIZE_UNIT (outtype)) + && max_int_size_in_bytes (outtype) < 0))) { if (allows_mem) allows_reg = 0; @@ -6392,7 +6398,12 @@ gimplify_asm_expr (tree *expr_p, gimple_ oconstraints, &allows_mem, &allows_reg); /* If we can't make copies, we can only accept memory. */ - if (TREE_ADDRESSABLE (TREE_TYPE (TREE_VALUE (link)))) + tree intype = TREE_TYPE (TREE_VALUE (link)); + if (intype != error_mark_node + && (TREE_ADDRESSABLE (intype) + || !COMPLETE_TYPE_P (intype) + || (!tree_fits_poly_uint64_p (TYPE_SIZE_UNIT (intype)) + && max_int_size_in_bytes (intype) < 0))) { if (allows_mem) allows_reg = 0; --- gcc/testsuite/c-c++-common/pr92352.c.jj 2019-11-04 14:03:18.725275255 +0100 +++ gcc/testsuite/c-c++-common/pr92352.c 2019-11-04 14:02:55.211629675 +0100 @@ -0,0 +1,15 @@ +/* PR inline-asm/92352 */ + +void +foo (int x) +{ + int var[x]; + asm volatile ("" : "+r" (var)); /* { dg-error "impossible constraint in 'asm'" } */ +} /* { dg-error "non-memory output 0 must stay in memory" "" { target *-*-* } .-1 } */ + +void +bar (int x) +{ + int var[x]; + asm volatile ("" : "+m" (var)); +} Jakub
On Tue, 5 Nov 2019, Jakub Jelinek wrote: > On Tue, Nov 05, 2019 at 09:27:45AM +0100, Richard Biener wrote: > > > --- gcc/gimplify.c.jj 2019-11-02 10:00:59.595253274 +0100 > > > +++ gcc/gimplify.c 2019-11-05 00:21:01.585958514 +0100 > > > @@ -6235,8 +6235,14 @@ gimplify_asm_expr (tree *expr_p, gimple_ > > > is_inout = false; > > > } > > > > > > - /* If we can't make copies, we can only accept memory. */ > > > - if (TREE_ADDRESSABLE (TREE_TYPE (TREE_VALUE (link)))) > > > + /* If we can't make copies, we can only accept memory. > > > + Similarly for VLAs. */ > > > + tree outtype = TREE_TYPE (TREE_VALUE (link)); > > > + if (outtype != error_mark_node > > > > so for error_mark_node we don't diagnose anything? > > Yes, we should have diagnosed it already. The != error_mark_node > I've added only after seeing tons of ICEs in the testsuite with earlier > version of the patch. > > > > + && (TREE_ADDRESSABLE (outtype) > > > + || !COMPLETE_TYPE_P (outtype) > > > + || (!tree_fits_poly_uint64_p (TYPE_SIZE_UNIT (outtype)) > > > + && max_int_size_in_bytes (outtype)))) > > > > so max_int_size_in_bytes == 0 is OK? I suppose we have a testcase > > for this? > > Actually, I meant max_int_size_in_bytes (outtype) < 0, i.e. something > on which force_constant_size ICE immediately, sorry for screwing it up in > the end. > All these VLAs with max_int_size_in_bytes >= 0 sizes are specific to Ada > and I have no idea what is and isn't valid there, for C/C++ it should > always return -1. > > > Otherwise looks reasonable to me. > > So, is the following ok if it passes bootstrap/regtest, or shall > I just go for || !tree_fits_poly_uint64_p (TYPE_SIZE_UNIT (outtype))) ? Hmm, can we actually copy VLAs with max_int_size_in_bytes != -1? I suppose we can copy even unconstrained VLAs, we just miss a WITH_SIZE_EXPR here but then it's unlikely we ever fit that into a non-memory. That's true as well if we just know the max size, so allowing that seems suspicious at best. So I'd say OK with just !tree_fits_poly_uint64_p. Thanks, Richard. > 2019-11-05 Jakub Jelinek <jakub@redhat.com> > > PR inline-asm/92352 > * gimplify.c (gimplify_asm_expr): Reject VLA in output or input > operands with non-memory constraints. > > * c-c++-common/pr92352.c: New test. > > --- gcc/gimplify.c.jj 2019-11-02 10:00:59.595253274 +0100 > +++ gcc/gimplify.c 2019-11-05 00:21:01.585958514 +0100 > @@ -6235,8 +6235,14 @@ gimplify_asm_expr (tree *expr_p, gimple_ > is_inout = false; > } > > - /* If we can't make copies, we can only accept memory. */ > - if (TREE_ADDRESSABLE (TREE_TYPE (TREE_VALUE (link)))) > + /* If we can't make copies, we can only accept memory. > + Similarly for VLAs. */ > + tree outtype = TREE_TYPE (TREE_VALUE (link)); > + if (outtype != error_mark_node > + && (TREE_ADDRESSABLE (outtype) > + || !COMPLETE_TYPE_P (outtype) > + || (!tree_fits_poly_uint64_p (TYPE_SIZE_UNIT (outtype)) > + && max_int_size_in_bytes (outtype) < 0))) > { > if (allows_mem) > allows_reg = 0; > @@ -6392,7 +6398,12 @@ gimplify_asm_expr (tree *expr_p, gimple_ > oconstraints, &allows_mem, &allows_reg); > > /* If we can't make copies, we can only accept memory. */ > - if (TREE_ADDRESSABLE (TREE_TYPE (TREE_VALUE (link)))) > + tree intype = TREE_TYPE (TREE_VALUE (link)); > + if (intype != error_mark_node > + && (TREE_ADDRESSABLE (intype) > + || !COMPLETE_TYPE_P (intype) > + || (!tree_fits_poly_uint64_p (TYPE_SIZE_UNIT (intype)) > + && max_int_size_in_bytes (intype) < 0))) > { > if (allows_mem) > allows_reg = 0; > --- gcc/testsuite/c-c++-common/pr92352.c.jj 2019-11-04 14:03:18.725275255 +0100 > +++ gcc/testsuite/c-c++-common/pr92352.c 2019-11-04 14:02:55.211629675 +0100 > @@ -0,0 +1,15 @@ > +/* PR inline-asm/92352 */ > + > +void > +foo (int x) > +{ > + int var[x]; > + asm volatile ("" : "+r" (var)); /* { dg-error "impossible constraint in 'asm'" } */ > +} /* { dg-error "non-memory output 0 must stay in memory" "" { target *-*-* } .-1 } */ > + > +void > +bar (int x) > +{ > + int var[x]; > + asm volatile ("" : "+m" (var)); > +} > > > Jakub > >
--- gcc/gimplify.c.jj 2019-11-02 10:00:59.595253274 +0100 +++ gcc/gimplify.c 2019-11-05 00:21:01.585958514 +0100 @@ -6235,8 +6235,14 @@ gimplify_asm_expr (tree *expr_p, gimple_ is_inout = false; } - /* If we can't make copies, we can only accept memory. */ - if (TREE_ADDRESSABLE (TREE_TYPE (TREE_VALUE (link)))) + /* If we can't make copies, we can only accept memory. + Similarly for VLAs. */ + tree outtype = TREE_TYPE (TREE_VALUE (link)); + if (outtype != error_mark_node + && (TREE_ADDRESSABLE (outtype) + || !COMPLETE_TYPE_P (outtype) + || (!tree_fits_poly_uint64_p (TYPE_SIZE_UNIT (outtype)) + && max_int_size_in_bytes (outtype)))) { if (allows_mem) allows_reg = 0; @@ -6392,7 +6398,12 @@ gimplify_asm_expr (tree *expr_p, gimple_ oconstraints, &allows_mem, &allows_reg); /* If we can't make copies, we can only accept memory. */ - if (TREE_ADDRESSABLE (TREE_TYPE (TREE_VALUE (link)))) + tree intype = TREE_TYPE (TREE_VALUE (link)); + if (intype != error_mark_node + && (TREE_ADDRESSABLE (intype) + || !COMPLETE_TYPE_P (intype) + || (!tree_fits_poly_uint64_p (TYPE_SIZE_UNIT (intype)) + && max_int_size_in_bytes (intype)))) { if (allows_mem) allows_reg = 0; --- gcc/testsuite/c-c++-common/pr92352.c.jj 2019-11-04 14:03:18.725275255 +0100 +++ gcc/testsuite/c-c++-common/pr92352.c 2019-11-04 14:02:55.211629675 +0100 @@ -0,0 +1,15 @@ +/* PR inline-asm/92352 */ + +void +foo (int x) +{ + int var[x]; + asm volatile ("" : "+r" (var)); /* { dg-error "impossible constraint in 'asm'" } */ +} /* { dg-error "non-memory output 0 must stay in memory" "" { target *-*-* } .-1 } */ + +void +bar (int x) +{ + int var[x]; + asm volatile ("" : "+m" (var)); +}