Patchwork Clean up pretty printers [18/n]

login
register
mail settings
Submitter Paolo Carlini
Date Aug. 26, 2013, 9:09 a.m.
Message ID <521B1B4F.5090206@oracle.com>
Download mbox | patch
Permalink /patch/269832/
State New
Headers show

Comments

Paolo Carlini - Aug. 26, 2013, 9:09 a.m.
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.

////////////////////////
Gabriel Dos Reis - Aug. 26, 2013, 9:20 a.m.
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.
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.
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.
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.
> 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.
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.
> 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.
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.
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.
> 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

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;
 	    }
 	}