Message ID | YmJYTyEtsqlTwhKM@tucnak |
---|---|
State | New |
Headers | show |
Series | i386: Fix up ix86_gimplify_va_arg [PR105331] | expand |
Hi! I'd like to ping this patch. I know it isn't a full week yet, but we are almost out of P1s and GCC 12 branching is any time now. Thanks: On Fri, Apr 22, 2022 at 09:25:04AM +0200, Jakub Jelinek via Gcc-patches wrote: > On the following testcase we emit a bogus > 'va_arg_tmp.5' may be used uninitialized > warning. The reason is that when gimplifying the addr = &temp; > statement, the va_arg_tmp temporary var for which we emit ADDR_EXPR > is not TREE_ADDRESSABLE, prepare_gimple_addressable emits some extra > code to initialize the newly addressable var from its previous value, > but it is a new variable which hasn't been initialized yet and will > be later, so we end up initializing it with uninitialized SSA_NAME: > va_arg_tmp.6 = va_arg_tmp.5_14(D); > addr.2_16 = &va_arg_tmp.6; > _17 = MEM[(double *)sse_addr.4_13]; > MEM[(double * {ref-all})addr.2_16] = _17; > and with -O1 we actually don't DSE it before the warning is emitted. > If we make the temp TREE_ADDRESSABLE before the gimplification, then > this prepare_gimple_addressable path isn't taken and we effectively > omit the first statement above and so the bogus warning is gone. > > I went through other backends and didn't find another instance of this > problem. > > Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk? > > 2022-04-22 Jakub Jelinek <jakub@redhat.com> > > PR target/105331 > * config/i386/i386.cc (ix86_gimplify_va_arg): Mark va_arg_tmp > temporary TREE_ADDRESSABLE before trying to gimplify ADDR_EXPR > of it. > > * gcc.dg/pr105331.c: New test. Jakub
On Thu, Apr 28, 2022 at 10:31 AM Jakub Jelinek <jakub@redhat.com> wrote: > > Hi! > > I'd like to ping this patch. I know it isn't a full week yet, but we are > almost out of P1s and GCC 12 branching is any time now. > > Thanks: > On Fri, Apr 22, 2022 at 09:25:04AM +0200, Jakub Jelinek via Gcc-patches wrote: > > On the following testcase we emit a bogus > > 'va_arg_tmp.5' may be used uninitialized > > warning. The reason is that when gimplifying the addr = &temp; > > statement, the va_arg_tmp temporary var for which we emit ADDR_EXPR > > is not TREE_ADDRESSABLE, prepare_gimple_addressable emits some extra > > code to initialize the newly addressable var from its previous value, > > but it is a new variable which hasn't been initialized yet and will > > be later, so we end up initializing it with uninitialized SSA_NAME: > > va_arg_tmp.6 = va_arg_tmp.5_14(D); > > addr.2_16 = &va_arg_tmp.6; > > _17 = MEM[(double *)sse_addr.4_13]; > > MEM[(double * {ref-all})addr.2_16] = _17; > > and with -O1 we actually don't DSE it before the warning is emitted. > > If we make the temp TREE_ADDRESSABLE before the gimplification, then > > this prepare_gimple_addressable path isn't taken and we effectively > > omit the first statement above and so the bogus warning is gone. > > > > I went through other backends and didn't find another instance of this > > problem. > > > > Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk? > > > > 2022-04-22 Jakub Jelinek <jakub@redhat.com> > > > > PR target/105331 > > * config/i386/i386.cc (ix86_gimplify_va_arg): Mark va_arg_tmp > > temporary TREE_ADDRESSABLE before trying to gimplify ADDR_EXPR > > of it. > > > > * gcc.dg/pr105331.c: New test. Sorry, I have no idea if this patch is correct or not. Uros.
On Thu, Apr 28, 2022 at 10:39:39AM +0200, Uros Bizjak wrote: > > > Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk? > > > > > > 2022-04-22 Jakub Jelinek <jakub@redhat.com> > > > > > > PR target/105331 > > > * config/i386/i386.cc (ix86_gimplify_va_arg): Mark va_arg_tmp > > > temporary TREE_ADDRESSABLE before trying to gimplify ADDR_EXPR > > > of it. > > > > > > * gcc.dg/pr105331.c: New test. > > Sorry, I have no idea if this patch is correct or not. Richard or Jeff, could you please review it instead? Thanks. Jakub
On Thu, 28 Apr 2022, Jakub Jelinek wrote: > On Thu, Apr 28, 2022 at 10:39:39AM +0200, Uros Bizjak wrote: > > > > Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk? > > > > > > > > 2022-04-22 Jakub Jelinek <jakub@redhat.com> > > > > > > > > PR target/105331 > > > > * config/i386/i386.cc (ix86_gimplify_va_arg): Mark va_arg_tmp > > > > temporary TREE_ADDRESSABLE before trying to gimplify ADDR_EXPR > > > > of it. > > > > > > > > * gcc.dg/pr105331.c: New test. > > > > Sorry, I have no idea if this patch is correct or not. > > Richard or Jeff, could you please review it instead? OK. Thanks, Richard.
On 4/28/2022 4:31 AM, Richard Biener wrote: > On Thu, 28 Apr 2022, Jakub Jelinek wrote: > >> On Thu, Apr 28, 2022 at 10:39:39AM +0200, Uros Bizjak wrote: >>>>> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk? >>>>> >>>>> 2022-04-22 Jakub Jelinek <jakub@redhat.com> >>>>> >>>>> PR target/105331 >>>>> * config/i386/i386.cc (ix86_gimplify_va_arg): Mark va_arg_tmp >>>>> temporary TREE_ADDRESSABLE before trying to gimplify ADDR_EXPR >>>>> of it. >>>>> >>>>> * gcc.dg/pr105331.c: New test. >>> Sorry, I have no idea if this patch is correct or not. >> Richard or Jeff, could you please review it instead? > OK. Agreed. In fact I'd fixed something very similar in our port a while ago. Jeff
--- gcc/config/i386/i386.cc.jj 2022-04-12 09:20:07.566662842 +0200 +++ gcc/config/i386/i386.cc 2022-04-21 12:03:32.201951522 +0200 @@ -4891,6 +4891,7 @@ ix86_gimplify_va_arg (tree valist, tree { int i, prev_size = 0; tree temp = create_tmp_var (type, "va_arg_tmp"); + TREE_ADDRESSABLE (temp) = 1; /* addr = &temp; */ t = build1 (ADDR_EXPR, build_pointer_type (type), temp); --- gcc/testsuite/gcc.dg/pr105331.c.jj 2022-04-21 12:09:34.398906718 +0200 +++ gcc/testsuite/gcc.dg/pr105331.c 2022-04-21 12:09:07.304283903 +0200 @@ -0,0 +1,11 @@ +/* PR target/105331 */ +/* { dg-do compile } */ +/* { dg-options "-O -Wuninitialized" } */ + +#include <stdarg.h> + +int +foo (va_list *va) +{ + return va_arg (*va, double _Complex); /* { dg-bogus "may be used uninitialized" } */ +}