Re: tree ssa and type issues

Re: tree ssa and type issues

[Thomas R. Truscott]

> | >   dfs = (bitmap *) xmalloc (last_basic_block * sizeof (bitmap *));
> | >                                                        ^^^^^^^^

It would be nice if gcc gave a warning for suspicious cases like this.
I did a crude warning for this, and ironically it found a bug in valgrind:

http://websvn.kde.org/trunk/valgrind/coregrind/vg_signals.c?rev=396554&r1=396296&r2=396554

A generally usable warning would need to weed out false positives
by warning only when the types are suspiciously mismatched (as were these.)

Tom Truscott

Re: tree ssa and type issues

[Gabriel Dos Reis]

Daniel Berlin <dberlin@dberlin.org> writes:

| On Sat, 2005-05-21 at 18:55 +0200, Andreas Jaeger wrote:
| > Gabriel Dos Reis <gdr@integrable-solutions.net> writes:
| > 
| > > Hi again,
| > >
| > > I just hit this one from tree-ssa-into.c:rewrite_into_ssa()
| > >
| > >   /* Initialize dominance frontier.  */
| > >   dfs = (bitmap *) xmalloc (last_basic_block * sizeof (bitmap *));
| > >                                                        ^^^^^^^^
| > >                                                                 
| > > If the sizeof operand really supposed to be "bitmap*" and not "bitmap"?
| > 
| > This indeed looks wrong - and could lead to real memory corruption
| > :-(.  If this is wrong, he should be changed on the 4.0 branch as
| > well,
| 
| It's wrong, but it couldn't lead to actual memory corruption, since
| sizeof (bitmap) == sizeof (bitmap *), since both are pointers.

Similarly, we have this in cselib.c:

static struct elt_list **reg_values;

[...]
      reg_values = xcalloc (reg_values_size, sizeof (reg_values));
                                                     ^^^^^^^^^^

that should have been

      reg_values = xcalloc (reg_values_size, sizeof (*reg_values));

-- Gaby

Re: tree ssa and type issues

[Andreas Schwab]

Andreas Jaeger <aj@suse.de> writes:

> Gabriel Dos Reis <gdr@integrable-solutions.net> writes:
>
>> Hi again,
>>
>> I just hit this one from tree-ssa-into.c:rewrite_into_ssa()
>>
>>   /* Initialize dominance frontier.  */
>>   dfs = (bitmap *) xmalloc (last_basic_block * sizeof (bitmap *));
>>                                                        ^^^^^^^^
>>                                                                 
>> If the sizeof operand really supposed to be "bitmap*" and not "bitmap"?
>
> This indeed looks wrong - and could lead to real memory corruption
> :-(.

Since bitmap is also a pointer, sizeof (bitmap) == sizeof (bitmap *).

> If this is wrong, he should be changed on the 4.0 branch as well,

While it's wrong, it's harmless.

Andreas.

-- 
Andreas Schwab, SuSE Labs, schwab@suse.de
SuSE Linux Products GmbH, Maxfeldstraße 5, 90409 Nürnberg, Germany
Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: tree ssa and type issues

[Steven Bosscher]

On Saturday 21 May 2005 22:15, Daniel Berlin wrote:
> > where the storage is returned/reused as soon as done, as opposed to
> > waiting for the end of the compilation.
>
> Uh, nobody waits for the end of compilation to free their xmalloc'd
> things.

Except the alloc pools of et-forest :-)

Gr.
Steven

Re: tree ssa and type issues

[Daniel Berlin]

On Sat, 2005-05-21 at 22:59 +0200, Gabriel Dos Reis wrote:
> Daniel Berlin <dberlin@dberlin.org> writes:
> 
> | On Sat, 2005-05-21 at 22:00 +0200, Gabriel Dos Reis wrote:
> | > Daniel Berlin <dberlin@dberlin.org> writes:
> | > 
> | > [...]
> | > 
> | > | > Parenthetically, I was wondering who is freeing those extensive
> | > | > regions of storage xmalloc/xcalloc()ed here and there?
> | > | 
> | > | 1. The people who write the code to do the xmalloc'ing.
> | > | 2. Every couple of months, people run gcc through valgrind with leak
> | > | checking on to make sure we arne't leaking the xmalloc'd memory.
> | > 
> | > If all memory is realized at the end of program, then obviously there
> | > is no leak, but that is not what I was looking for.
> | 
> | Then what are you looking for?
> 
> [ A useful answer ;-) ]
> 
> where the storage is returned/reused as soon as done, as opposed to
> waiting for the end of the compilation.

Uh, nobody waits for the end of compilation to free their xmalloc'd
things.

> 
> [...]
> 
> | >   Do you know whether the have the resulst publically
> | > available somewhere?
> | No, i don't think anyone takes the extra effort to try to set up a web
> | page.
> 
> Thanks,
> 
> -- Gaby

Re: tree ssa and type issues

[Gabriel Dos Reis]

Daniel Berlin <dberlin@dberlin.org> writes:

| On Sat, 2005-05-21 at 22:00 +0200, Gabriel Dos Reis wrote:
| > Daniel Berlin <dberlin@dberlin.org> writes:
| > 
| > [...]
| > 
| > | > Parenthetically, I was wondering who is freeing those extensive
| > | > regions of storage xmalloc/xcalloc()ed here and there?
| > | 
| > | 1. The people who write the code to do the xmalloc'ing.
| > | 2. Every couple of months, people run gcc through valgrind with leak
| > | checking on to make sure we arne't leaking the xmalloc'd memory.
| > 
| > If all memory is realized at the end of program, then obviously there
| > is no leak, but that is not what I was looking for.
| 
| Then what are you looking for?

[ A useful answer ;-) ]

where the storage is returned/reused as soon as done, as opposed to
waiting for the end of the compilation.

[...]

| >   Do you know whether the have the resulst publically
| > available somewhere?
| No, i don't think anyone takes the extra effort to try to set up a web
| page.

Thanks,

-- Gaby

Re: tree ssa and type issues

[Daniel Berlin]

On Sat, 2005-05-21 at 22:00 +0200, Gabriel Dos Reis wrote:
> Daniel Berlin <dberlin@dberlin.org> writes:
> 
> [...]
> 
> | > Parenthetically, I was wondering who is freeing those extensive
> | > regions of storage xmalloc/xcalloc()ed here and there?
> | 
> | 1. The people who write the code to do the xmalloc'ing.
> | 2. Every couple of months, people run gcc through valgrind with leak
> | checking on to make sure we arne't leaking the xmalloc'd memory.
> 
> If all memory is realized at the end of program, then obviously there
> is no leak, but that is not what I was looking for.

Then what are you looking for?

Some of the xmalloc's are hidden behind things like pool_alloc and
pool_free, which internally use xmalloc/free, etc.

> 
> But, it is nice to know that people run GCC through valgrind every
> couple of months.

You can even use valgrind during bootstrap to verify even the gc'd
memory is being used properly.

--enable-checking=valgrind

It should work for both the zone and page collectors (I know Daniel J.
was using it with zone to verify the zone collector was working
properly)


>   Do you know whether the have the resulst publically
> available somewhere?
No, i don't think anyone takes the extra effort to try to set up a web
page.


> 
> -- Gaby

Re: tree ssa and type issues

[Gabriel Dos Reis]

Daniel Berlin <dberlin@dberlin.org> writes:

[...]

| > Parenthetically, I was wondering who is freeing those extensive
| > regions of storage xmalloc/xcalloc()ed here and there?
| 
| 1. The people who write the code to do the xmalloc'ing.
| 2. Every couple of months, people run gcc through valgrind with leak
| checking on to make sure we arne't leaking the xmalloc'd memory.

If all memory is realized at the end of program, then obviously there
is no leak, but that is not what I was looking for.

But, it is nice to know that people run GCC through valgrind every
couple of months.  Do you know whether the have the resulst publically
available somewhere?

-- Gaby

Re: tree ssa and type issues

[Gabriel Dos Reis]

Daniel Berlin <dberlin@dberlin.org> writes:

| On Sat, 2005-05-21 at 18:55 +0200, Andreas Jaeger wrote:
| > Gabriel Dos Reis <gdr@integrable-solutions.net> writes:
| > 
| > > Hi again,
| > >
| > > I just hit this one from tree-ssa-into.c:rewrite_into_ssa()
| > >
| > >   /* Initialize dominance frontier.  */
| > >   dfs = (bitmap *) xmalloc (last_basic_block * sizeof (bitmap *));
| > >                                                        ^^^^^^^^
| > >                                                                 
| > > If the sizeof operand really supposed to be "bitmap*" and not "bitmap"?
| > 
| > This indeed looks wrong - and could lead to real memory corruption
| > :-(.  If this is wrong, he should be changed on the 4.0 branch as
| > well,
| 
| It's wrong, but it couldn't lead to actual memory corruption, since
| sizeof (bitmap) == sizeof (bitmap *), since both are pointers.

Aha.  This the second time, I'm hitting something like this.  The
first one was with libiberty/argv.c:dupargv() a month ago.

-- Gaby

Re: tree ssa and type issues

[Gabriel Dos Reis]

Tom Tromey <tromey@redhat.com> writes:

| >>>>> "Gabriel" == Gabriel Dos Reis <gdr@integrable-solutions.net> writes:
| 
| Gabriel> I just hit this one from tree-ssa-into.c:rewrite_into_ssa()
| Gabriel>   /* Initialize dominance frontier.  */
| Gabriel>   dfs = (bitmap *) xmalloc (last_basic_block * sizeof (bitmap *));
| Gabriel>                                                        ^^^^^^^^
| 
| Maybe we should be using the XNEWVEC macro (libiberty.h) everywhere.
| That would avoid this problem.

Absolutely.

| 
| How did you catch this btw?  (If gcc or g++ warns about this, it is
| cool news... and if it doesn't, wouldn't that be nice?)

The way I got it was simple:  I just grepped for xmalloc/xcalloc in
tree-* (most of them rely on implicit void * -> T* which C++ does not
like) and was going to replace them with XNEWVEC/XCNEWVEC and my eyes
could not parse that line :-)

No, gcc/g++ -- unfortunately -- cannot catch such things.  
Yes, it would be nice (e.g. useful), if it could warn about them.  I
think it is a common mistake.  Of source if works only for syntactic
construct like

    (T) allocate (sizeof (U))

or

   (T) allocate (n * sizeof (U))

where T are not equivalent.  But, I guess that would already be an
improvement. 

-- Gaby

Re: tree ssa and type issues

[Daniel Berlin]

On Sat, 2005-05-21 at 20:33 +0200, Gabriel Dos Reis wrote:
> Andreas Jaeger <aj@suse.de> writes:
> 
> | Gabriel Dos Reis <gdr@integrable-solutions.net> writes:
> | 
> | > Hi again,
> | >
> | > I just hit this one from tree-ssa-into.c:rewrite_into_ssa()
> | >
> | >   /* Initialize dominance frontier.  */
> | >   dfs = (bitmap *) xmalloc (last_basic_block * sizeof (bitmap *));
> | >                                                        ^^^^^^^^
> | >                                                                 
> | > If the sizeof operand really supposed to be "bitmap*" and not "bitmap"?
> | 
> | This indeed looks wrong - and could lead to real memory corruption
> | :-(.  If this is wrong, he should be changed on the 4.0 branch as
> | well,
> 
> And if it is wrong, it would be interesting to know wheter we already
> have PRs related to that memory corruption -- but I guess it would be
> hard to know :-)

Again, it won't corrupt memory, because sizeof (bitmap *) == sizeof
(bitmap), though it is still wrong.


> 
> Parenthetically, I was wondering who is freeing those extensive
> regions of storage xmalloc/xcalloc()ed here and there?

1. The people who write the code to do the xmalloc'ing.
2. Every couple of months, people run gcc through valgrind with leak
checking on to make sure we arne't leaking the xmalloc'd memory.

Re: tree ssa and type issues

[Daniel Berlin]

On Sat, 2005-05-21 at 18:55 +0200, Andreas Jaeger wrote:
> Gabriel Dos Reis <gdr@integrable-solutions.net> writes:
> 
> > Hi again,
> >
> > I just hit this one from tree-ssa-into.c:rewrite_into_ssa()
> >
> >   /* Initialize dominance frontier.  */
> >   dfs = (bitmap *) xmalloc (last_basic_block * sizeof (bitmap *));
> >                                                        ^^^^^^^^
> >                                                                 
> > If the sizeof operand really supposed to be "bitmap*" and not "bitmap"?
> 
> This indeed looks wrong - and could lead to real memory corruption
> :-(.  If this is wrong, he should be changed on the 4.0 branch as
> well,

It's wrong, but it couldn't lead to actual memory corruption, since
sizeof (bitmap) == sizeof (bitmap *), since both are pointers.

Re: tree ssa and type issues

[Tom Tromey]

>>>>> "Gabriel" == Gabriel Dos Reis <gdr@integrable-solutions.net> writes:

Gabriel> I just hit this one from tree-ssa-into.c:rewrite_into_ssa()
Gabriel>   /* Initialize dominance frontier.  */
Gabriel>   dfs = (bitmap *) xmalloc (last_basic_block * sizeof (bitmap *));
Gabriel>                                                        ^^^^^^^^

Maybe we should be using the XNEWVEC macro (libiberty.h) everywhere.
That would avoid this problem.

How did you catch this btw?  (If gcc or g++ warns about this, it is
cool news... and if it doesn't, wouldn't that be nice?)

Tom

Re: tree ssa and type issues

[Andreas Jaeger]

[-- Attachment #1: Type: text/plain, Size: 1876 bytes --]

Gabriel Dos Reis <gdr@integrable-solutions.net> writes:

> Andreas Jaeger <aj@suse.de> writes:
>
> | Gabriel Dos Reis <gdr@integrable-solutions.net> writes:
> | 
> | > Hi again,
> | >
> | > I just hit this one from tree-ssa-into.c:rewrite_into_ssa()

Note update_ssa has the same problem.

> | >
> | >   /* Initialize dominance frontier.  */
> | >   dfs = (bitmap *) xmalloc (last_basic_block * sizeof (bitmap *));
> | >                                                        ^^^^^^^^
> | >                                                                 
> | > If the sizeof operand really supposed to be "bitmap*" and not "bitmap"?
> | 
> | This indeed looks wrong - and could lead to real memory corruption
> | :-(.  If this is wrong, he should be changed on the 4.0 branch as
> | well,
>
> And if it is wrong, it would be interesting to know wheter we already
> have PRs related to that memory corruption -- but I guess it would be
> hard to know :-)

Yeah.


> Parenthetically, I was wondering who is freeing those extensive
> regions of storage xmalloc/xcalloc()ed here and there?

I went through the file (btw. it's named tree-into-ssa.c) and it does
not look that bad.  I could not find a place which is obviously
wrong.  Looking at rewrite_into_ssa I see:

  interesting_blocks = sbitmap_alloc (last_basic_block);
  dfs = (bitmap *) xmalloc (last_basic_block * sizeof (bitmap *));
  FOR_EACH_BB (bb)
    dfs[bb->index] = BITMAP_ALLOC (NULL);
[...]
  /* Free allocated memory.  */
  FOR_EACH_BB (bb)
    BITMAP_FREE (dfs[bb->index]);
  free (dfs);
  sbitmap_free (interesting_blocks);

So, this does look fine,
Andreas
-- 
 Andreas Jaeger, aj@suse.de, http://www.suse.de/~aj
  SUSE Linux Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
   GPG fingerprint = 93A3 365E CE47 B889 DF7F  FED1 389A 563C C272 A126

[-- Attachment #2: Type: application/pgp-signature, Size: 188 bytes --]

Re: tree ssa and type issues

[Gabriel Dos Reis]

Andreas Jaeger <aj@suse.de> writes:

| Gabriel Dos Reis <gdr@integrable-solutions.net> writes:
| 
| > Hi again,
| >
| > I just hit this one from tree-ssa-into.c:rewrite_into_ssa()
| >
| >   /* Initialize dominance frontier.  */
| >   dfs = (bitmap *) xmalloc (last_basic_block * sizeof (bitmap *));
| >                                                        ^^^^^^^^
| >                                                                 
| > If the sizeof operand really supposed to be "bitmap*" and not "bitmap"?
| 
| This indeed looks wrong - and could lead to real memory corruption
| :-(.  If this is wrong, he should be changed on the 4.0 branch as
| well,

And if it is wrong, it would be interesting to know wheter we already
have PRs related to that memory corruption -- but I guess it would be
hard to know :-)

Parenthetically, I was wondering who is freeing those extensive
regions of storage xmalloc/xcalloc()ed here and there?

-- Gaby

Re: tree ssa and type issues

[Andreas Jaeger]

[-- Attachment #1: Type: text/plain, Size: 866 bytes --]

Gabriel Dos Reis <gdr@integrable-solutions.net> writes:

> Hi again,
>
> I just hit this one from tree-ssa-into.c:rewrite_into_ssa()
>
>   /* Initialize dominance frontier.  */
>   dfs = (bitmap *) xmalloc (last_basic_block * sizeof (bitmap *));
>                                                        ^^^^^^^^
>                                                                 
> If the sizeof operand really supposed to be "bitmap*" and not "bitmap"?

This indeed looks wrong - and could lead to real memory corruption
:-(.  If this is wrong, he should be changed on the 4.0 branch as
well,

Seems your C++ project is indeed usefull ;-)

Andreas
-- 
 Andreas Jaeger, aj@suse.de, http://www.suse.de/~aj
  SUSE Linux Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
   GPG fingerprint = 93A3 365E CE47 B889 DF7F  FED1 389A 563C C272 A126

[-- Attachment #2: Type: application/pgp-signature, Size: 188 bytes --]

tree ssa and type issues

[Gabriel Dos Reis]

Hi again,

I just hit this one from tree-ssa-into.c:rewrite_into_ssa()

  /* Initialize dominance frontier.  */
  dfs = (bitmap *) xmalloc (last_basic_block * sizeof (bitmap *));
                                                       ^^^^^^^^
                                                                
If the sizeof operand really supposed to be "bitmap*" and not "bitmap"?

-- Gaby