Patchwork initialized out.clauses in read_predicate

login
register
mail settings
Submitter Alexandre Oliva
Date May 30, 2011, 9:46 a.m.
Message ID <oroc2k34i5.fsf@livre.localdomain>
Download mbox | patch
Permalink /patch/97897/
State New
Headers show

Comments

Alexandre Oliva - May 30, 2011, 9:46 a.m.
In an -O3 bootstrap, out.clauses are reported as uninitialized.  GCC is
not smart enough to realize accesses to the uninitialized members will
never happen.  It shouldn't be too expensive to initialize them all, so
this is what this patch does.  Regstrapped on x86_64-linux-gnu and
i686-linux-gnu.  Ok to install?
Alexandre Oliva - June 6, 2011, 1:38 p.m.
On May 30, 2011, Alexandre Oliva <aoliva@redhat.com> wrote:

> In an -O3 bootstrap, out.clauses are reported as uninitialized.  GCC is
> not smart enough to realize accesses to the uninitialized members will
> never happen.  It shouldn't be too expensive to initialize them all, so
> this is what this patch does.  Regstrapped on x86_64-linux-gnu and
> i686-linux-gnu.  Ok to install?

Oops, I mistakenly checked this in along with a couple of other approved
patches, sorry.

Should I back it out, or may I leave it in?  (please :-)

> for  gcc/ChangeLog
> from  Alexandre Oliva  <aoliva@redhat.com>

> 	* ipa-inline-analysis.c (read_predicate): Initialize all clauses.

> Index: gcc/ipa-inline-analysis.c
> ===================================================================
> --- gcc/ipa-inline-analysis.c.orig	2011-05-18 03:29:08.295328903 -0300
> +++ gcc/ipa-inline-analysis.c	2011-05-18 03:29:38.187242177 -0300
> @@ -2289,6 +2289,11 @@ read_predicate (struct lto_input_block *
>        clause = out.clause[k++] = lto_input_uleb128 (ib);
>      }
>    while (clause);
> +
> +  /* Zero-initialize the remaining clauses in OUT.  */
> +  while (k <= MAX_CLAUSES)
> +    out.clause[k++] = 0;
> +
>    return out;
>  }
Jan Hubicka - June 6, 2011, 2:10 p.m.
> On May 30, 2011, Alexandre Oliva <aoliva@redhat.com> wrote:
> 
> > In an -O3 bootstrap, out.clauses are reported as uninitialized.  GCC is
> > not smart enough to realize accesses to the uninitialized members will
> > never happen.  It shouldn't be too expensive to initialize them all, so
> > this is what this patch does.  Regstrapped on x86_64-linux-gnu and
> > i686-linux-gnu.  Ok to install?
> 
> Oops, I mistakenly checked this in along with a couple of other approved
> patches, sorry.
> 
> Should I back it out, or may I leave it in?  (please :-)
> 
> > for  gcc/ChangeLog
> > from  Alexandre Oliva  <aoliva@redhat.com>
> 
> > 	* ipa-inline-analysis.c (read_predicate): Initialize all clauses.
> 
> > Index: gcc/ipa-inline-analysis.c
> > ===================================================================
> > --- gcc/ipa-inline-analysis.c.orig	2011-05-18 03:29:08.295328903 -0300
> > +++ gcc/ipa-inline-analysis.c	2011-05-18 03:29:38.187242177 -0300
> > @@ -2289,6 +2289,11 @@ read_predicate (struct lto_input_block *
> >        clause = out.clause[k++] = lto_input_uleb128 (ib);
> >      }
> >    while (clause);
> > +
> > +  /* Zero-initialize the remaining clauses in OUT.  */
> > +  while (k <= MAX_CLAUSES)
> > +    out.clause[k++] = 0;

Well, clauses is a typical zero terminated array, so I really do think the warning is bogus.
Do you at least know why we end up with that weird warning?

Honza
Jan Hubicka - June 6, 2011, 2:11 p.m.
> On May 30, 2011, Alexandre Oliva <aoliva@redhat.com> wrote:
> 
> > In an -O3 bootstrap, out.clauses are reported as uninitialized.  GCC is
> > not smart enough to realize accesses to the uninitialized members will
> > never happen.  It shouldn't be too expensive to initialize them all, so
> > this is what this patch does.  Regstrapped on x86_64-linux-gnu and
> > i686-linux-gnu.  Ok to install?
> 
> Oops, I mistakenly checked this in along with a couple of other approved
> patches, sorry.
> 
> Should I back it out, or may I leave it in?  (please :-)

But concerning backing out, I don't see why we should break bootstrap again.  it would
be however nice to get a testcase and track the bogus warning issue.

Thanks!
Honza
Alexandre Oliva - June 7, 2011, 6:19 a.m.
On Jun  6, 2011, Jan Hubicka <hubicka@ucw.cz> wrote:

>> On May 30, 2011, Alexandre Oliva <aoliva@redhat.com> wrote:
>> > Index: gcc/ipa-inline-analysis.c

> Well, clauses is a typical zero terminated array, so I really do think
> the warning is bogus.

Looking into it, I'm not sure it is.  Compiling with -O3 I get:

ipa-inline-analysis.c: In function ‘inline_read_summary’:
ipa-inline-analysis.c:531:22: warning: ‘out.clause[8]’ may be used uninitialized in this function [-Wmaybe-uninitialized]
ipa-inline-analysis.c:2294:20: note: ‘out.clause[8]’ was declared here
...

> Do you at least know why we end up with that weird warning?

531:      *es->predicate = *predicate;

This is clearly copying all fields and array members, even the
uninitialized ones.  However, like padding copied while copying structs,
I'm not sure that's a problem if the copied members are not accessed.
However, it is absolutely correct that we are copying uninitialized data
members.

Patch

for  gcc/ChangeLog
from  Alexandre Oliva  <aoliva@redhat.com>

	* ipa-inline-analysis.c (read_predicate): Initialize all clauses.

Index: gcc/ipa-inline-analysis.c
===================================================================
--- gcc/ipa-inline-analysis.c.orig	2011-05-18 03:29:08.295328903 -0300
+++ gcc/ipa-inline-analysis.c	2011-05-18 03:29:38.187242177 -0300
@@ -2289,6 +2289,11 @@  read_predicate (struct lto_input_block *
       clause = out.clause[k++] = lto_input_uleb128 (ib);
     }
   while (clause);
+
+  /* Zero-initialize the remaining clauses in OUT.  */
+  while (k <= MAX_CLAUSES)
+    out.clause[k++] = 0;
+
   return out;
 }