Re: [pph] Add initial support for including nested pph images (issue4847044)
On Tue, Aug 9, 2011 at 20:52, Gabriel Charette wrote: > An ICE in lto_streamer_cache_get is not very intuitive to point out > the problem to the user in this case imo Right. > I'm thinking we should at least warn when some flags like this one > differ and when that difference has an impact on the internal > representation and can produce ICEs. Yes, we can use the header of the PPH file to add anything we need to do compatibility tests: original flags, version numbers, etc. When we open the file we can do these checks to avoid reading files incompatible with the current compilation context. Diego.
Re: [pph] Add initial support for including nested pph images (issue4847044)
>>> Gah. I was going to commit this separately but forgot. Sorry about that. >>> >> K, why does this fix the test? seems weird that we had an infinite >> loop before and changing the flags gets rid of it? > > It wasn't an infinite loop but an ICE in the tree caches. The > different options were producing different ASTs, so the tree cache had > different trees on write-out and read-in. > Ah ok I see, this is actually the test I was aiming at fixing next, nice. I remember we were discussing it was the build system's job to ensure flags matched. In this case however it's much worse then the asm diff we were getting before in the c1builtin-integral test when using different flags, which was fine as the assembly was still valid and was just adapting to the behavior wanted by the user. An ICE in lto_streamer_cache_get is not very intuitive to point out the problem to the user in this case imo I'm thinking we should at least warn when some flags like this one differ and when that difference has an impact on the internal representation and can produce ICEs. Gab
Re: [pph] Add initial support for including nested pph images (issue4847044)
On Fri, Aug 5, 2011 at 14:10, Gabriel Charette wrote: > On Fri, Aug 5, 2011 at 9:42 AM, Diego Novillo wrote: >> On Thu, Aug 4, 2011 at 17:47, wrote: >>> So if I understand correctly, the reference system doesn't work yet, you >>> just put out the design in this patch and you'll fill up functionality >>> in an upcoming patch so that's it's clearer what you're adding? >> >> Right, the 'if (0)' in pph_in_includes() disables support for it. >> >>> http://codereview.appspot.com/4847044/diff/1/gcc/cp/pph-streamer-in.c >>> File gcc/cp/pph-streamer-in.c (right): >>> >>> http://codereview.appspot.com/4847044/diff/1/gcc/cp/pph-streamer-in.c#newcode1491 >>> gcc/cp/pph-streamer-in.c:1491: pph_add_include (stream); >>> does this mean the included pph has the include information for all of >>> the headers in it's transitive closure? i.e. we don't need to load child >>> information of a child? if so, isn't this bad from a dependency point of >>> view? >> >> Hmm? Each header knows what other images it includes, so before >> reading its own content it reads the contents from the others. The >> dependencies don't change. The parent will not have the physical >> representation of the ASTs from the children, just references to them. >> > > What I mean is if you have the following include structure A.c -> B.h > -> C.h -> D.h > > Would B.h hold references to both C.h and D.h or only to C which in > turn knows about D? Ah, no. B.pph only knows about C.pph. When we load C.pph, it then proceeds to load D.pph. So, we shouldn't be loading D.pph twice when including B.pph. I think this now doesn't work and we will try to load D twice (which is a bug). >> Gah. I was going to commit this separately but forgot. Sorry about that. >> > K, why does this fix the test? seems weird that we had an infinite > loop before and changing the flags gets rid of it? It wasn't an infinite loop but an ICE in the tree caches. The different options were producing different ASTs, so the tree cache had different trees on write-out and read-in. Diego.
Re: [pph] Add initial support for including nested pph images (issue4847044)
On Fri, Aug 5, 2011 at 9:42 AM, Diego Novillo wrote:
> On Thu, Aug 4, 2011 at 17:47, wrote:
>> So if I understand correctly, the reference system doesn't work yet, you
>> just put out the design in this patch and you'll fill up functionality
>> in an upcoming patch so that's it's clearer what you're adding?
>
> Right, the 'if (0)' in pph_in_includes() disables support for it.
>
>> http://codereview.appspot.com/4847044/diff/1/gcc/cp/pph-streamer-in.c
>> File gcc/cp/pph-streamer-in.c (right):
>>
>> http://codereview.appspot.com/4847044/diff/1/gcc/cp/pph-streamer-in.c#newcode1491
>> gcc/cp/pph-streamer-in.c:1491: pph_add_include (stream);
>> does this mean the included pph has the include information for all of
>> the headers in it's transitive closure? i.e. we don't need to load child
>> information of a child? if so, isn't this bad from a dependency point of
>> view?
>
> Hmm? Each header knows what other images it includes, so before
> reading its own content it reads the contents from the others. The
> dependencies don't change. The parent will not have the physical
> representation of the ASTs from the children, just references to them.
>
What I mean is if you have the following include structure A.c -> B.h
-> C.h -> D.h
Would B.h hold references to both C.h and D.h or only to C which in
turn knows about D?
From what I understood it seemed like all of the children in the
transitive closure of B will be referenced in B and that B will not
need to look at references in C when we read it (which is great from
an I/O standpoint, but maybe not from a dependency point of view? just
trying to understand the design)
>> http://codereview.appspot.com/4847044/diff/1/gcc/cp/pph.c
>> File gcc/cp/pph.c (right):
>>
>> http://codereview.appspot.com/4847044/diff/1/gcc/cp/pph.c#newcode164
>> gcc/cp/pph.c:164: if (pph_out_file != NULL)
>> shouldn't you use your new pph_enabled_p function here?
>
> No. I need to know whether we are *creating* a PPH image. Perhaps I
> should add a different predicate.
>
Ah ok I see, ya perhaps another inline function could clarify..
>> http://codereview.appspot.com/4847044/diff/1/gcc/testsuite/g++.dg/pph/c1pr44948-1a.cc
>> File gcc/testsuite/g++.dg/pph/c1pr44948-1a.cc (right):
>>
>> http://codereview.appspot.com/4847044/diff/1/gcc/testsuite/g++.dg/pph/c1pr44948-1a.cc#newcode1
>> gcc/testsuite/g++.dg/pph/c1pr44948-1a.cc:1: /* { dg-options "-O
>> -Wno-psabi -mtune=generic" } */
>> Is this related to the rest of the changes in this patch? I don't see it
>> mentioned in your patch comments
>
> Gah. I was going to commit this separately but forgot. Sorry about that.
>
K, why does this fix the test? seems weird that we had an infinite
loop before and changing the flags gets rid of it?
Thanks,
Gab
Re: [pph] Add initial support for including nested pph images (issue4847044)
On Thu, Aug 4, 2011 at 17:47, wrote:
> So if I understand correctly, the reference system doesn't work yet, you
> just put out the design in this patch and you'll fill up functionality
> in an upcoming patch so that's it's clearer what you're adding?
Right, the 'if (0)' in pph_in_includes() disables support for it.
> http://codereview.appspot.com/4847044/diff/1/gcc/cp/pph-streamer-in.c
> File gcc/cp/pph-streamer-in.c (right):
>
> http://codereview.appspot.com/4847044/diff/1/gcc/cp/pph-streamer-in.c#newcode1491
> gcc/cp/pph-streamer-in.c:1491: pph_add_include (stream);
> does this mean the included pph has the include information for all of
> the headers in it's transitive closure? i.e. we don't need to load child
> information of a child? if so, isn't this bad from a dependency point of
> view?
Hmm? Each header knows what other images it includes, so before
reading its own content it reads the contents from the others. The
dependencies don't change. The parent will not have the physical
representation of the ASTs from the children, just references to them.
> http://codereview.appspot.com/4847044/diff/1/gcc/cp/pph.c
> File gcc/cp/pph.c (right):
>
> http://codereview.appspot.com/4847044/diff/1/gcc/cp/pph.c#newcode164
> gcc/cp/pph.c:164: if (pph_out_file != NULL)
> shouldn't you use your new pph_enabled_p function here?
No. I need to know whether we are *creating* a PPH image. Perhaps I
should add a different predicate.
> http://codereview.appspot.com/4847044/diff/1/gcc/testsuite/g++.dg/pph/c1pr44948-1a.cc
> File gcc/testsuite/g++.dg/pph/c1pr44948-1a.cc (right):
>
> http://codereview.appspot.com/4847044/diff/1/gcc/testsuite/g++.dg/pph/c1pr44948-1a.cc#newcode1
> gcc/testsuite/g++.dg/pph/c1pr44948-1a.cc:1: /* { dg-options "-O
> -Wno-psabi -mtune=generic" } */
> Is this related to the rest of the changes in this patch? I don't see it
> mentioned in your patch comments
Gah. I was going to commit this separately but forgot. Sorry about that.
Diego.
Re: [pph] Add initial support for including nested pph images (issue4847044)
So if I understand correctly, the reference system doesn't work yet, you
just put out the design in this patch and you'll fill up functionality
in an upcoming patch so that's it's clearer what you're adding?
Cheers,
Gab
http://codereview.appspot.com/4847044/diff/1/gcc/cp/pph-streamer-in.c
File gcc/cp/pph-streamer-in.c (right):
http://codereview.appspot.com/4847044/diff/1/gcc/cp/pph-streamer-in.c#newcode1491
gcc/cp/pph-streamer-in.c:1491: pph_add_include (stream);
does this mean the included pph has the include information for all of
the headers in it's transitive closure? i.e. we don't need to load child
information of a child? if so, isn't this bad from a dependency point of
view?
http://codereview.appspot.com/4847044/diff/1/gcc/cp/pph.c
File gcc/cp/pph.c (right):
http://codereview.appspot.com/4847044/diff/1/gcc/cp/pph.c#newcode164
gcc/cp/pph.c:164: if (pph_out_file != NULL)
shouldn't you use your new pph_enabled_p function here?
http://codereview.appspot.com/4847044/diff/1/gcc/testsuite/g++.dg/pph/c1pr44948-1a.cc
File gcc/testsuite/g++.dg/pph/c1pr44948-1a.cc (right):
http://codereview.appspot.com/4847044/diff/1/gcc/testsuite/g++.dg/pph/c1pr44948-1a.cc#newcode1
gcc/testsuite/g++.dg/pph/c1pr44948-1a.cc:1: /* { dg-options "-O
-Wno-psabi -mtune=generic" } */
Is this related to the rest of the changes in this patch? I don't see it
mentioned in your patch comments
http://codereview.appspot.com/4847044/
[pph] Add initial support for including nested pph images (issue4847044)
This patch adds initial support for PPH images that include other PPH
images. Currently, we support this but it is inefficient. When a
parent image includes a child image, the parent embeds all the
contents of the child in its image. This is a waste of space.
There is one other missing piece for this to work, however. We need
to change the way references work. Currently, they are simply an
offset into the streamer cache, which is created for every file we
open. When we have images including other images, the references to
symbols in another file's symbol table should be 2-dimensional.
I am currently adding that support, but this will be more intrusive,
so I'm flushing this out first.
The main changes:
- Add a symbol table in every stream (field symtab in pph_stream).
This supports efficient searches using a pointer set and has the
symbols arranged in declaration order.
- Initialize PPH support before the parser starts. This is needed to
allow the parser to register symbols into the PPH symbol table for
the image that we are currently generating.
- New stream flags to determine whether the stream is currently open,
nested images and a table of included images.
There is a pretty big potential change in the order in which files are
included. Suppose we have file parent.h:
parent.h --
int X;
class A { ... };
#include "child.pph"
TypeFromChild L;
When parent and child are converted to pph images, the image child.pph
will be read *before* any of the contents of parent.pph. We already
require child.pph to not depend on any content from the outside, but
child.pph could alter macros defined and used by parent.h before the
original inclusion. The plan for now is to disallow this.
Tested on x86_64.
Diego.
cp/ChangeLog.pph
* parser.c (cp_lexer_new_main): Move call to pph_init ...
(c_parse_file): ... here.
Call pph_finish only if pph_enabled_p returns true.
* pph-streamer-in.c (pph_in_includes): New.
(pph_read_file_contents): Call it.
(pph_read_file): Call pph_add_include.
* pph-streamer-out.c (current_pph_file): Remove. Update all users.
(decls_to_register_t) Likewise.
(decls_to_register): Likewise.
(pph_out_stream): Declare.
(pph_out_symtab_marker): Assert that the MARKER fits in an
unsigned char.
(pph_out_includes): New.
(pph_write_file): Make static. Assume that STREAM is opened and
closed by the caller.
(pph_add_include): New.
(pph_writer_init): Open pph_out_file into pph_out_stream.
(pph_writer_finish): Close pph_out_stream. Call pph_write_file
if no errors were found.
(pph_add_decl_to_symtab): Rename from pph_add_decl_to_register.
Update all users.
* pph-streamer.c (pph_stream_open): Tidy.
Initialize stream->symtab.
(pph_stream_close_1): Factor out of ...
(pph_stream_close): ... here.
Only call pph_stream_close_1 if STREAM is not nested in another
PPH file.
* pph-streamer.h (struct pph_symtab): Declare.
(pph_stream_ptr): Declare.
(struct pph_stream): Add fields open_p, nested_p, symtab
and includes.
(pph_add_decl_to_symtab): Declare.
(pph_add_include): Declare.
(pph_writer_init): Declare.
(pph_writer_finish): Declare.
* pph.c (pph_init): Call pph_writer_init if pph_out_file is set.
(pph_finish): Call pph_writer_finish if pph_out_file is set.
* pph.h (pph_enabled_p): New.
testsuite/ChangeLog.pph:
* g++.dg/pph/c1pr44948-1a.cc: Use the same flags as the header file.
Mark fixed.
diff --git a/gcc/cp/decl.c b/gcc/cp/decl.c
index 5896688..3b312d3 100644
--- a/gcc/cp/decl.c
+++ b/gcc/cp/decl.c
@@ -5879,10 +5879,9 @@ cp_rest_of_decl_compilation (tree decl, int top_level,
int at_end)
{
rest_of_decl_compilation (decl, top_level, at_end);
- /* If we are generating a PPH image, add DECL to the list of
- declarations that need to be registered when this image is read. */
+ /* If we are generating a PPH image, add DECL to its symbol table. */
if (pph_out_file)
-pph_add_decl_to_register (decl);
+pph_add_decl_to_symtab (decl);
}
@@ -12815,10 +12814,9 @@ start_preparsed_function (tree decl1, tree attrs, int
flags)
store_parm_decls (current_function_parms);
- /* If we are generating a PPH image, add DECL1 to the list of
- declarations that need to be registered when restoring the image. */
+ /* If we are generating a PPH image, add DECL1 to its symbol table. */
if (pph_out_file)
-pph_add_decl_to_register (decl1);
+pph_add_decl_to_symtab (decl1);
}
diff --git a/gcc/cp/parser.c b/gcc/cp/parser.c
index c48807d..e069441 100644
--- a/gcc/cp/parser.c
+++ b/gcc/cp/parser.c
@@ -604,20 +604,19 @@ cp_lexer_new_main (void)
cp_lexer *lexer;
cp_token token;
- if (pph_out_file != NULL || qu
