diff mbox

Clean up pretty printers [18/n]

Message ID 521B1B4F.5090206@oracle.com
State New
Headers show

Commit Message

Paolo Carlini Aug. 26, 2013, 9:09 a.m. UTC
Hi Gaby,

bootstrap is currently broken:

/scratch/Gcc/svn-dirs/trunk/gcc/c/c-objc-common.c: In function ‘bool 
c_tree_printer(pretty_printer*, text_info*, const char*, int, bool, 
bool, bool)’:
/scratch/Gcc/svn-dirs/trunk/gcc/c/c-objc-common.c:123:31: error: 
‘pp_c_expression’ was not declared in this scope
make[2]: *** [c/c-objc-common.o] Error 1

If I don't hear from you I'm going to commit as obvious the below.

Thanks!
Paolo.

////////////////////////

Comments

Gabriel Dos Reis Aug. 26, 2013, 9:20 a.m. UTC | #1
Paolo Carlini <paolo.carlini@oracle.com> writes:

| Hi Gaby,
| 
| bootstrap is currently broken:
| 
| /scratch/Gcc/svn-dirs/trunk/gcc/c/c-objc-common.c: In function ‘bool
| c_tree_printer(pretty_printer*, text_info*, const char*, int, bool,
| bool, bool)’:
| /scratch/Gcc/svn-dirs/trunk/gcc/c/c-objc-common.c:123:31: error:
| ‘pp_c_expression’ was not declared in this scope
| make[2]: *** [c/c-objc-common.o] Error 1
| 
| If I don't hear from you I'm going to commit as obvious the below.

You need more than that.  The rest of patch is in trunk now.  
Sorry for the breakage -- as you probably guessed, I specified only
c-family directory on the commit command line.

-- Gaby
Paolo Carlini Aug. 26, 2013, 9:24 a.m. UTC | #2
On 08/26/2013 11:20 AM, Gabriel Dos Reis wrote:
> You need more than that.  The rest of patch is in trunk now.
> Sorry for the breakage -- as you probably guessed, I specified only
> c-family directory on the commit command line.
Thanks!

Paolo.
Jan Hubicka Aug. 26, 2013, 12:24 p.m. UTC | #3
Hi,
it seems to be couple weeks I am not able to compile big ltrans unit with -fdump-tree-all
because the compiler eventually runs out of memory.  I think it is one of your patches
introducing serious memory leak.  Can you, please, take a look on this? It makes my life
harder - it is not fun to wait for 15 minutes for a dump and then see compiler to ICE...

Thanks,
Honza
Gabriel Dos Reis Aug. 26, 2013, 1:55 p.m. UTC | #4
On Mon, Aug 26, 2013 at 7:24 AM, Jan Hubicka <hubicka@ucw.cz> wrote:
> Hi,
> it seems to be couple weeks I am not able to compile big ltrans unit with -fdump-tree-all
> because the compiler eventually runs out of memory.  I think it is one of your patches
> introducing serious memory leak.  Can you, please, take a look on this? It makes my life
> harder - it is not fun to wait for 15 minutes for a dump and then see compiler to ICE...
>
> Thanks,
> Honza


The existing pretty printers have always had memory leaks, and
part of the work is to reduce that in fact :-(   For example, instead
of printing directly to the output buffer, many of them construct
a string for a given formatter, then send that string to the stream,
and that string may or may not be reclaimed.  Please give me
actionable instructions so I can reproduce your situation.

-- Gaby
Jan Hubicka Aug. 26, 2013, 2 p.m. UTC | #5
> On Mon, Aug 26, 2013 at 7:24 AM, Jan Hubicka <hubicka@ucw.cz> wrote:
> > Hi,
> > it seems to be couple weeks I am not able to compile big ltrans unit with -fdump-tree-all
> > because the compiler eventually runs out of memory.  I think it is one of your patches
> > introducing serious memory leak.  Can you, please, take a look on this? It makes my life
> > harder - it is not fun to wait for 15 minutes for a dump and then see compiler to ICE...
> >
> > Thanks,
> > Honza
> 
> 
> The existing pretty printers have always had memory leaks, and
> part of the work is to reduce that in fact :-(   For example, instead
> of printing directly to the output buffer, many of them construct
> a string for a given formatter, then send that string to the stream,
> and that string may or may not be reclaimed.  Please give me
> actionable instructions so I can reproduce your situation.

I think it should be easiert to build with bootstrap-lto and then add -fdump-tree-all into
the final link command line options.  It seems to reproduce quite uniformlny and doesn't
seem to be specific to my firefox builds I am doing right now...

Thanks!
Honza
> 
> -- Gaby
Gabriel Dos Reis Aug. 26, 2013, 2:24 p.m. UTC | #6
On Mon, Aug 26, 2013 at 9:00 AM, Jan Hubicka <hubicka@ucw.cz> wrote:
>> On Mon, Aug 26, 2013 at 7:24 AM, Jan Hubicka <hubicka@ucw.cz> wrote:
>> > Hi,
>> > it seems to be couple weeks I am not able to compile big ltrans unit with -fdump-tree-all
>> > because the compiler eventually runs out of memory.  I think it is one of your patches
>> > introducing serious memory leak.  Can you, please, take a look on this? It makes my life
>> > harder - it is not fun to wait for 15 minutes for a dump and then see compiler to ICE...
>> >
>> > Thanks,
>> > Honza
>>
>>
>> The existing pretty printers have always had memory leaks, and
>> part of the work is to reduce that in fact :-(   For example, instead
>> of printing directly to the output buffer, many of them construct
>> a string for a given formatter, then send that string to the stream,
>> and that string may or may not be reclaimed.  Please give me
>> actionable instructions so I can reproduce your situation.
>
> I think it should be easiert to build with bootstrap-lto and then add -fdump-tree-all into
> the final link command line options.

modify ALL_LINKERFLAGS or BUILD_LINKERFLAGS?


> It seems to reproduce quite uniformlny and doesn't
> seem to be specific to my firefox builds I am doing right now…

I think I may have an idea about  which existing leak is making itself
noticed but I am looking for a (semi-)automatic way to regtest it.


>
> Thanks!
> Honza
>>
>> -- Gaby
Jan Hubicka Aug. 26, 2013, 2:32 p.m. UTC | #7
> On Mon, Aug 26, 2013 at 9:00 AM, Jan Hubicka <hubicka@ucw.cz> wrote:
> >> On Mon, Aug 26, 2013 at 7:24 AM, Jan Hubicka <hubicka@ucw.cz> wrote:
> >> > Hi,
> >> > it seems to be couple weeks I am not able to compile big ltrans unit with -fdump-tree-all
> >> > because the compiler eventually runs out of memory.  I think it is one of your patches
> >> > introducing serious memory leak.  Can you, please, take a look on this? It makes my life
> >> > harder - it is not fun to wait for 15 minutes for a dump and then see compiler to ICE...
> >> >
> >> > Thanks,
> >> > Honza
> >>
> >>
> >> The existing pretty printers have always had memory leaks, and
> >> part of the work is to reduce that in fact :-(   For example, instead
> >> of printing directly to the output buffer, many of them construct
> >> a string for a given formatter, then send that string to the stream,
> >> and that string may or may not be reclaimed.  Please give me
> >> actionable instructions so I can reproduce your situation.
> >
> > I think it should be easiert to build with bootstrap-lto and then add -fdump-tree-all into
> > the final link command line options.
> 
> modify ALL_LINKERFLAGS or BUILD_LINKERFLAGS?

configure --with-build-config=bootstrap-lto . You need plugin enabled buinutils for that.
I think there however should be a lot easier way to reproduce it by just dropping some really
large source file.  Here LTO only runs the backend on very many functions.

Honza
Gabriel Dos Reis Aug. 26, 2013, 2:48 p.m. UTC | #8
On Mon, Aug 26, 2013 at 9:32 AM, Jan Hubicka <hubicka@ucw.cz> wrote:
>> On Mon, Aug 26, 2013 at 9:00 AM, Jan Hubicka <hubicka@ucw.cz> wrote:
>> >> On Mon, Aug 26, 2013 at 7:24 AM, Jan Hubicka <hubicka@ucw.cz> wrote:
>> >> > Hi,
>> >> > it seems to be couple weeks I am not able to compile big ltrans unit with -fdump-tree-all
>> >> > because the compiler eventually runs out of memory.  I think it is one of your patches
>> >> > introducing serious memory leak.  Can you, please, take a look on this? It makes my life
>> >> > harder - it is not fun to wait for 15 minutes for a dump and then see compiler to ICE...
>> >> >
>> >> > Thanks,
>> >> > Honza
>> >>
>> >>
>> >> The existing pretty printers have always had memory leaks, and
>> >> part of the work is to reduce that in fact :-(   For example, instead
>> >> of printing directly to the output buffer, many of them construct
>> >> a string for a given formatter, then send that string to the stream,
>> >> and that string may or may not be reclaimed.  Please give me
>> >> actionable instructions so I can reproduce your situation.
>> >
>> > I think it should be easiert to build with bootstrap-lto and then add -fdump-tree-all into
>> > the final link command line options.
>>
>> modify ALL_LINKERFLAGS or BUILD_LINKERFLAGS?
>
> configure --with-build-config=bootstrap-lto .

I got this part.  The question I asked relates to the when you said
"then add -fdump-tree-all into the final link command line options."

> You need plugin enabled buinutils for that.

ah.

> I think there however should be a lot easier way to reproduce it by just dropping some really
> large source file.  Here LTO only runs the backend on very many functions.
>
> Honza

I think the problem is in tree gimple dumpers.

-- Gaby
Gabriel Dos Reis Aug. 26, 2013, 3:01 p.m. UTC | #9
On Mon, Aug 26, 2013 at 9:32 AM, Jan Hubicka <hubicka@ucw.cz> wrote:

>>
>> modify ALL_LINKERFLAGS or BUILD_LINKERFLAGS?
>
> configure --with-build-config=bootstrap-lto . You need plugin enabled buinutils for that.
> I think there however should be a lot easier way to reproduce it by just dropping some really
> large source file.  Here LTO only runs the backend on very many functions.
>
> Honza

by the way, we should be leaking less now that we release the memory
acquire in obstack for local pretty printers. and discharged pretty printers.
However, I think we still have problems with: (1) global pretty printers,
(2) when we do ggc_strdup on the formatted text.  One of the local patches
I have avoids the copy and GGC involvement.

Please let me know which of the above Make linker variables I should modify.

-- Gaby
Jan Hubicka Aug. 26, 2013, 3:14 p.m. UTC | #10
> On Mon, Aug 26, 2013 at 9:32 AM, Jan Hubicka <hubicka@ucw.cz> wrote:
> 
> >>
> >> modify ALL_LINKERFLAGS or BUILD_LINKERFLAGS?
> >
> > configure --with-build-config=bootstrap-lto . You need plugin enabled buinutils for that.
> > I think there however should be a lot easier way to reproduce it by just dropping some really
> > large source file.  Here LTO only runs the backend on very many functions.
> >
> > Honza
> 
> by the way, we should be leaking less now that we release the memory
> acquire in obstack for local pretty printers. and discharged pretty printers.
> However, I think we still have problems with: (1) global pretty printers,
> (2) when we do ggc_strdup on the formatted text.  One of the local patches
> I have avoids the copy and GGC involvement.
> 
> Please let me know which of the above Make linker variables I should modify.

My favorite way to do so is to wait for compilation to get into linking stage and then
simply cut&paste the command line and modify it myself.  Otherwise it is bit inpractical:
we link many times (every binary twice just for checksum :( ) and thus you would end
up with quite massive disk usage I would guess.

Honza

> 
> -- Gaby
diff mbox

Patch

Index: c-objc-common.c
===================================================================
--- c-objc-common.c	(revision 201987)
+++ c-objc-common.c	(working copy)
@@ -120,7 +120,7 @@  c_tree_printer (pretty_printer *pp, text_info *tex
 	  t = DECL_DEBUG_EXPR (t);
 	  if (!DECL_P (t))
 	    {
-	      pp_c_expression (cpp, t);
+	      pp_expression (cpp, t);
 	      return true;
 	    }
 	}