| Message ID | 20110627162138.346051DA195@topo.tor.corp.google.com |
|---|---|
| State | New |
| Headers | show |
Couldn't we have headers look for their corresponding .pph file by
default when -fpph-map is on? (especially since pph.map is only
temporary for the implementation phase)
I would see it like this:
if -fpph-map is not NULL:
look for mapping in file
if not found:
look for default mapping (i.e. replace .h by .pph and look for file)
if not found:
use actual #include behaviour
Could that also be why the small test I tried to introduce last week
to test the ordering of the bindings coming from the pph would pass
(i.e. it wouldn't use it's pph as I didn't add it to pph.map?).
On Mon, Jun 27, 2011 at 9:21 AM, Diego Novillo <dnovillo@google.com> wrote:
>
> When I added these tests, I forgot to add them to pph.map, so the header
> files were being generated into an image, but not used in the second
> compilation.
>
> Most of the failures are in the form of different asm output. There
> is one new ICE, though.
>
> Committed to branch.
>
>
> Diego.
>
> * g++.dg/pph/pph.map: Add entries for c120060625-1.cc,
> c1attr-warn-unused-result.cc, c1builtin-integral-1.cc,
> c1builtin-object-size-2.cc, c1eabi1.cc, c1eabi1.h,
> c1limits-externalid.cc, c1meteor-contest.cc, c1pr36533.cc,
> c1pr44948-1a.cc, c1return-5.cc, and x1template.cc.
> * g++.dg/pph/c120060625-1.cc: Adjust XFAIL patterns.
> * g++.dg/pph/c1attr-warn-unused-result.cc: Adjust XFAIL
> patterns.
> * g++.dg/pph/c1builtin-integral-1.cc: Adjust XFAIL patterns.
> * g++.dg/pph/c1builtin-object-size-2.cc: Adjust XFAIL
> patterns.
> * g++.dg/pph/c1eabi1.cc: Adjust XFAIL patterns.
> * g++.dg/pph/c1eabi1.h: Adjust XFAIL patterns.
> * g++.dg/pph/c1limits-externalid.cc: Adjust XFAIL patterns.
> * g++.dg/pph/c1meteor-contest.cc: Adjust XFAIL patterns.
> * g++.dg/pph/c1pr36533.cc: Adjust XFAIL patterns.
> * g++.dg/pph/c1pr44948-1a.cc: Adjust XFAIL patterns.
> * g++.dg/pph/c1return-5.cc: Adjust XFAIL patterns.
> * g++.dg/pph/x1template.cc: Adjust XFAIL patterns.
>
> diff --git a/gcc/testsuite/g++.dg/pph/c120060625-1.cc b/gcc/testsuite/g++.dg/pph/c120060625-1.cc
> index 05c7929..d09be39 100644
> --- a/gcc/testsuite/g++.dg/pph/c120060625-1.cc
> +++ b/gcc/testsuite/g++.dg/pph/c120060625-1.cc
> @@ -1 +1,2 @@
> +// pph asm xdiff
> #include "c120060625-1.h"
> diff --git a/gcc/testsuite/g++.dg/pph/c1attr-warn-unused-result.cc b/gcc/testsuite/g++.dg/pph/c1attr-warn-unused-result.cc
> index 921d294..da75561 100644
> --- a/gcc/testsuite/g++.dg/pph/c1attr-warn-unused-result.cc
> +++ b/gcc/testsuite/g++.dg/pph/c1attr-warn-unused-result.cc
> @@ -1,2 +1,3 @@
> /* { dg-options "-w" } */
> +// pph asm xdiff
> #include "c1attr-warn-unused-result.h"
> diff --git a/gcc/testsuite/g++.dg/pph/c1builtin-integral-1.cc b/gcc/testsuite/g++.dg/pph/c1builtin-integral-1.cc
> index bf53219..962086c 100644
> --- a/gcc/testsuite/g++.dg/pph/c1builtin-integral-1.cc
> +++ b/gcc/testsuite/g++.dg/pph/c1builtin-integral-1.cc
> @@ -1 +1,2 @@
> +// pph asm xdiff
> #include "c1builtin-integral-1.h"
> diff --git a/gcc/testsuite/g++.dg/pph/c1builtin-object-size-2.cc b/gcc/testsuite/g++.dg/pph/c1builtin-object-size-2.cc
> index 615e7da..17fe707 100644
> --- a/gcc/testsuite/g++.dg/pph/c1builtin-object-size-2.cc
> +++ b/gcc/testsuite/g++.dg/pph/c1builtin-object-size-2.cc
> @@ -1,2 +1,3 @@
> /* { dg-options "-O2 -w -fpermissive" } */
> +// pph asm xdiff
> #include "c1builtin-object-size-2.h"
> diff --git a/gcc/testsuite/g++.dg/pph/c1eabi1.cc b/gcc/testsuite/g++.dg/pph/c1eabi1.cc
> index b2e9b11..07a3a8b 100644
> --- a/gcc/testsuite/g++.dg/pph/c1eabi1.cc
> +++ b/gcc/testsuite/g++.dg/pph/c1eabi1.cc
> @@ -1,3 +1,4 @@
> +// { dg-timeout 2 { target *-*-* } }
> // { dg-options "-w -fpermissive" }
> // pph asm xdiff
>
> diff --git a/gcc/testsuite/g++.dg/pph/c1eabi1.h b/gcc/testsuite/g++.dg/pph/c1eabi1.h
> index 5f5b593..383b752 100644
> --- a/gcc/testsuite/g++.dg/pph/c1eabi1.h
> +++ b/gcc/testsuite/g++.dg/pph/c1eabi1.h
> @@ -1,4 +1,6 @@
> // { dg-options "-w -fpermissive" }
> +// FIXME pph - Enabling PPH for this file causes memory problems in cc1plus.
> +// c1eabi1.h c1eabi1.pph
>
> #ifndef __PPH_GUARD_H
> #define __PPH_GUARD_H
> diff --git a/gcc/testsuite/g++.dg/pph/c1limits-externalid.cc b/gcc/testsuite/g++.dg/pph/c1limits-externalid.cc
> index 8b5039c..8d2da40 100644
> --- a/gcc/testsuite/g++.dg/pph/c1limits-externalid.cc
> +++ b/gcc/testsuite/g++.dg/pph/c1limits-externalid.cc
> @@ -1 +1,2 @@
> +// pph asm xdiff
> #include "c1limits-externalid.h"
> diff --git a/gcc/testsuite/g++.dg/pph/c1meteor-contest.cc b/gcc/testsuite/g++.dg/pph/c1meteor-contest.cc
> index bb097ac..58d2c89 100644
> --- a/gcc/testsuite/g++.dg/pph/c1meteor-contest.cc
> +++ b/gcc/testsuite/g++.dg/pph/c1meteor-contest.cc
> @@ -1,2 +1,4 @@
> +/* { dg-timeout 5 { target *-*-* } } */
> +// { dg-xfail-if "INFINITE" { "*-*-*" } { "-fpph-map=pph.map" } }
> /* { dg-options "-w" } */
> #include "c1meteor-contest.h"
> diff --git a/gcc/testsuite/g++.dg/pph/c1pr36533.cc b/gcc/testsuite/g++.dg/pph/c1pr36533.cc
> index b44e8c9..d8d6d8c 100644
> --- a/gcc/testsuite/g++.dg/pph/c1pr36533.cc
> +++ b/gcc/testsuite/g++.dg/pph/c1pr36533.cc
> @@ -1,2 +1,3 @@
> /* { dg-options "-w -fpermissive" } */
> +// pph asm xdiff
> #include "c1pr36533.h"
> diff --git a/gcc/testsuite/g++.dg/pph/c1pr44948-1a.cc b/gcc/testsuite/g++.dg/pph/c1pr44948-1a.cc
> index f3f0427..d2ebd27 100644
> --- a/gcc/testsuite/g++.dg/pph/c1pr44948-1a.cc
> +++ b/gcc/testsuite/g++.dg/pph/c1pr44948-1a.cc
> @@ -1 +1,3 @@
> +// { dg-xfail-if "INFINITE" { "*-*-*" } { "-fpph-map=pph.map" } }
> +// { dg-bogus "internal compiler error: in lto_streamer_cache_get, at lto-streamer.c" "" { xfail *-*-* } 0 }
> #include "c1pr44948-1a.h"
> diff --git a/gcc/testsuite/g++.dg/pph/c1return-5.cc b/gcc/testsuite/g++.dg/pph/c1return-5.cc
> index a29c8a9..aa7dfe4 100644
> --- a/gcc/testsuite/g++.dg/pph/c1return-5.cc
> +++ b/gcc/testsuite/g++.dg/pph/c1return-5.cc
> @@ -1,4 +1,5 @@
> // { dg-options "-mpreferred-stack-boundary=4" }
> // { dg-final { scan-assembler-not "and\[lq\]?\[^\\n\]*-64,\[^\\n\]*sp" } }
> +// pph asm xdiff
>
> #include "c1return-5.h"
> diff --git a/gcc/testsuite/g++.dg/pph/pph.map b/gcc/testsuite/g++.dg/pph/pph.map
> index f0c7abd..2735af8 100644
> --- a/gcc/testsuite/g++.dg/pph/pph.map
> +++ b/gcc/testsuite/g++.dg/pph/pph.map
> @@ -1,5 +1,9 @@
> +c120060625-1.h c120060625-1.pph
> c1altinc1.h c1altinc1.pph
> c1altinc2.h c1altinc2.pph
> +c1attr-warn-unused-result.h c1attr-warn-unused-result.pph
> +c1builtin-integral-1.h c1builtin-integral-1.pph
> +c1builtin-object-size-2.h c1builtin-object-size-2.pph
> c1chained1.h c1chained1.pph
> c1chained2.h c1chained2.pph
> c1empty.h c1empty.pph
> @@ -9,8 +13,13 @@ c1functions.h c1functions.pph
> c1guarded1.h c1guarded1.pph
> c1guarded2.h c1guarded2.pph
> c1guarded3.h c1guarded3.pph
> +c1limits-externalid.h c1limits-externalid.pph
> +c1meteor-contest.h c1meteor-contest.pph
> c1multinc1.h c1multinc1.pph
> c1multinc2.h c1multinc2.pph
> +c1pr36533.h c1pr36533.pph
> +c1pr44948-1a.h c1pr44948-1a.pph
> +c1return-5.h c1return-5.pph
> c1simple1.h c1simple1.pph
> c1simple2.h c1simple2.pph
> c1struct.h c1struct.pph
> diff --git a/gcc/testsuite/g++.dg/pph/x1template.cc b/gcc/testsuite/g++.dg/pph/x1template.cc
> index cecefd7..95ad779 100644
> --- a/gcc/testsuite/g++.dg/pph/x1template.cc
> +++ b/gcc/testsuite/g++.dg/pph/x1template.cc
> @@ -1,5 +1,5 @@
> // { dg-xfail-if "ICE" { "*-*-*" } { "-fpph-map=pph.map" } }
> -// { dg-bogus "x1template.h:18:13: internal compiler error: in resume_scope, at cp/name-lookup.c:1568" "" { xfail *-*-* } 0 }
> +// { dg-bogus "x1template.h:18:13: internal compiler error: in resume_scope" "" { xfail *-*-* } 0 }
> // { dg-prune-output "In file included from " }
>
> #include "x1template.h"
>
> --
> This patch is available for review at http://codereview.appspot.com/4639073
On Mon, Jun 27, 2011 at 13:01, Gabriel Charette <gchare@google.com> wrote: > Couldn't we have headers look for their corresponding .pph file by > default when -fpph-map is on? (especially since pph.map is only > temporary for the implementation phase) The problem is with headers that include other headers. We want to limit the generation of images to specific headers. Given that we are in this initial implementation phase, the simplest approach is to remember to update pph.map. > Could that also be why the small test I tried to introduce last week > to test the ordering of the bindings coming from the pph would pass > (i.e. it wouldn't use it's pph as I didn't add it to pph.map?). Yes, that's likely. Diego.
On Mon, Jun 27, 2011 at 10:06 AM, Diego Novillo <dnovillo@google.com> wrote: > On Mon, Jun 27, 2011 at 13:01, Gabriel Charette <gchare@google.com> wrote: >> Couldn't we have headers look for their corresponding .pph file by >> default when -fpph-map is on? (especially since pph.map is only >> temporary for the implementation phase) > > The problem is with headers that include other headers. We want to > limit the generation of images to specific headers. Given that we are > in this initial implementation phase, the simplest approach is to > remember to update pph.map. Well in what I'm proposing we only use the pph file if it was actually generated before (we don't generate it if it's not there). Are there any situations where we have a corresponding pph file, but don't actually want to use it when the fpph-map flag is on? Seems unlikely as we put all the pph files in the pph map for now anyways... > >> Could that also be why the small test I tried to introduce last week >> to test the ordering of the bindings coming from the pph would pass >> (i.e. it wouldn't use it's pph as I didn't add it to pph.map?). > > Yes, that's likely. > > > Diego. >
On Mon, Jun 27, 2011 at 13:13, Gabriel Charette <gchare@google.com> wrote: > On Mon, Jun 27, 2011 at 10:06 AM, Diego Novillo <dnovillo@google.com> wrote: >> On Mon, Jun 27, 2011 at 13:01, Gabriel Charette <gchare@google.com> wrote: >>> Couldn't we have headers look for their corresponding .pph file by >>> default when -fpph-map is on? (especially since pph.map is only >>> temporary for the implementation phase) >> >> The problem is with headers that include other headers. We want to >> limit the generation of images to specific headers. Given that we are >> in this initial implementation phase, the simplest approach is to >> remember to update pph.map. > > Well in what I'm proposing we only use the pph file if it was actually > generated before (we don't generate it if it's not there). Are there > any situations where we have a corresponding pph file, but don't > actually want to use it when the fpph-map flag is on? Seems unlikely > as we put all the pph files in the pph map for now anyways... Headers include other headers. We are not generating PPH for all headers. The mapping file also allows us to control what happens when generating a PPH file that emits #include directives itself. Some of those we want to have referenced as PPH, but not all. It's really easier for us to remember to update the mapping file rather than adding some automatic detection that would have to be removed later. Diego.
On Mon, Jun 27, 2011 at 10:18 AM, Diego Novillo <dnovillo@google.com> wrote: > On Mon, Jun 27, 2011 at 13:13, Gabriel Charette <gchare@google.com> wrote: >> On Mon, Jun 27, 2011 at 10:06 AM, Diego Novillo <dnovillo@google.com> wrote: >>> On Mon, Jun 27, 2011 at 13:01, Gabriel Charette <gchare@google.com> wrote: >>>> Couldn't we have headers look for their corresponding .pph file by >>>> default when -fpph-map is on? (especially since pph.map is only >>>> temporary for the implementation phase) >>> >>> The problem is with headers that include other headers. We want to >>> limit the generation of images to specific headers. Given that we are >>> in this initial implementation phase, the simplest approach is to >>> remember to update pph.map. >> >> Well in what I'm proposing we only use the pph file if it was actually >> generated before (we don't generate it if it's not there). Are there >> any situations where we have a corresponding pph file, but don't >> actually want to use it when the fpph-map flag is on? Seems unlikely >> as we put all the pph files in the pph map for now anyways... > > Headers include other headers. We are not generating PPH for all headers. > > The mapping file also allows us to control what happens when > generating a PPH file that emits #include directives itself. Some of > those we want to have referenced as PPH, but not all. > > It's really easier for us to remember to update the mapping file > rather than adding some automatic detection that would have to be > removed later. Ok I agree, temporary flags and map files are better then temporary code for the implementation phase. LGTM, Gab > > Diego. >
On 6/27/11, Gabriel Charette <gchare@google.com> wrote: > On Jun 27, 2011 Diego Novillo <dnovillo@google.com> wrote: > > On Jun 27, 2011 Gabriel Charette <gchare@google.com> wrote: > > > Couldn't we have headers look for their corresponding .pph > > > file by default when -fpph-map is on? (especially since > > > pph.map is only temporary for the implementation phase) > > > > The problem is with headers that include other headers. > > We want to limit the generation of images to specific headers. > > Given that we are in this initial implementation phase, the > > simplest approach is to remember to update pph.map. > > Well in what I'm proposing we only use the pph file if it was > actually generated before (we don't generate it if it's not > there). Are there any situations where we have a corresponding > pph file, but don't actually want to use it when the fpph-map > flag is on? Seems unlikely as we put all the pph files in the > pph map for now anyways... For a shortcut, you can add the option -fpph-hdr=<base-name> A mapping from <base-name>.h to <base-name>.pph In general, the pph files won't live in the same directory as the header file. This will cause problems when two headers have the same name. I would rather not get into complicated search rules just yet.
diff --git a/gcc/testsuite/g++.dg/pph/c120060625-1.cc b/gcc/testsuite/g++.dg/pph/c120060625-1.cc index 05c7929..d09be39 100644 --- a/gcc/testsuite/g++.dg/pph/c120060625-1.cc +++ b/gcc/testsuite/g++.dg/pph/c120060625-1.cc @@ -1 +1,2 @@ +// pph asm xdiff #include "c120060625-1.h" diff --git a/gcc/testsuite/g++.dg/pph/c1attr-warn-unused-result.cc b/gcc/testsuite/g++.dg/pph/c1attr-warn-unused-result.cc index 921d294..da75561 100644 --- a/gcc/testsuite/g++.dg/pph/c1attr-warn-unused-result.cc +++ b/gcc/testsuite/g++.dg/pph/c1attr-warn-unused-result.cc @@ -1,2 +1,3 @@ /* { dg-options "-w" } */ +// pph asm xdiff #include "c1attr-warn-unused-result.h" diff --git a/gcc/testsuite/g++.dg/pph/c1builtin-integral-1.cc b/gcc/testsuite/g++.dg/pph/c1builtin-integral-1.cc index bf53219..962086c 100644 --- a/gcc/testsuite/g++.dg/pph/c1builtin-integral-1.cc +++ b/gcc/testsuite/g++.dg/pph/c1builtin-integral-1.cc @@ -1 +1,2 @@ +// pph asm xdiff #include "c1builtin-integral-1.h" diff --git a/gcc/testsuite/g++.dg/pph/c1builtin-object-size-2.cc b/gcc/testsuite/g++.dg/pph/c1builtin-object-size-2.cc index 615e7da..17fe707 100644 --- a/gcc/testsuite/g++.dg/pph/c1builtin-object-size-2.cc +++ b/gcc/testsuite/g++.dg/pph/c1builtin-object-size-2.cc @@ -1,2 +1,3 @@ /* { dg-options "-O2 -w -fpermissive" } */ +// pph asm xdiff #include "c1builtin-object-size-2.h" diff --git a/gcc/testsuite/g++.dg/pph/c1eabi1.cc b/gcc/testsuite/g++.dg/pph/c1eabi1.cc index b2e9b11..07a3a8b 100644 --- a/gcc/testsuite/g++.dg/pph/c1eabi1.cc +++ b/gcc/testsuite/g++.dg/pph/c1eabi1.cc @@ -1,3 +1,4 @@ +// { dg-timeout 2 { target *-*-* } } // { dg-options "-w -fpermissive" } // pph asm xdiff diff --git a/gcc/testsuite/g++.dg/pph/c1eabi1.h b/gcc/testsuite/g++.dg/pph/c1eabi1.h index 5f5b593..383b752 100644 --- a/gcc/testsuite/g++.dg/pph/c1eabi1.h +++ b/gcc/testsuite/g++.dg/pph/c1eabi1.h @@ -1,4 +1,6 @@ // { dg-options "-w -fpermissive" } +// FIXME pph - Enabling PPH for this file causes memory problems in cc1plus. +// c1eabi1.h c1eabi1.pph #ifndef __PPH_GUARD_H #define __PPH_GUARD_H diff --git a/gcc/testsuite/g++.dg/pph/c1limits-externalid.cc b/gcc/testsuite/g++.dg/pph/c1limits-externalid.cc index 8b5039c..8d2da40 100644 --- a/gcc/testsuite/g++.dg/pph/c1limits-externalid.cc +++ b/gcc/testsuite/g++.dg/pph/c1limits-externalid.cc @@ -1 +1,2 @@ +// pph asm xdiff #include "c1limits-externalid.h" diff --git a/gcc/testsuite/g++.dg/pph/c1meteor-contest.cc b/gcc/testsuite/g++.dg/pph/c1meteor-contest.cc index bb097ac..58d2c89 100644 --- a/gcc/testsuite/g++.dg/pph/c1meteor-contest.cc +++ b/gcc/testsuite/g++.dg/pph/c1meteor-contest.cc @@ -1,2 +1,4 @@ +/* { dg-timeout 5 { target *-*-* } } */ +// { dg-xfail-if "INFINITE" { "*-*-*" } { "-fpph-map=pph.map" } } /* { dg-options "-w" } */ #include "c1meteor-contest.h" diff --git a/gcc/testsuite/g++.dg/pph/c1pr36533.cc b/gcc/testsuite/g++.dg/pph/c1pr36533.cc index b44e8c9..d8d6d8c 100644 --- a/gcc/testsuite/g++.dg/pph/c1pr36533.cc +++ b/gcc/testsuite/g++.dg/pph/c1pr36533.cc @@ -1,2 +1,3 @@ /* { dg-options "-w -fpermissive" } */ +// pph asm xdiff #include "c1pr36533.h" diff --git a/gcc/testsuite/g++.dg/pph/c1pr44948-1a.cc b/gcc/testsuite/g++.dg/pph/c1pr44948-1a.cc index f3f0427..d2ebd27 100644 --- a/gcc/testsuite/g++.dg/pph/c1pr44948-1a.cc +++ b/gcc/testsuite/g++.dg/pph/c1pr44948-1a.cc @@ -1 +1,3 @@ +// { dg-xfail-if "INFINITE" { "*-*-*" } { "-fpph-map=pph.map" } } +// { dg-bogus "internal compiler error: in lto_streamer_cache_get, at lto-streamer.c" "" { xfail *-*-* } 0 } #include "c1pr44948-1a.h" diff --git a/gcc/testsuite/g++.dg/pph/c1return-5.cc b/gcc/testsuite/g++.dg/pph/c1return-5.cc index a29c8a9..aa7dfe4 100644 --- a/gcc/testsuite/g++.dg/pph/c1return-5.cc +++ b/gcc/testsuite/g++.dg/pph/c1return-5.cc @@ -1,4 +1,5 @@ // { dg-options "-mpreferred-stack-boundary=4" } // { dg-final { scan-assembler-not "and\[lq\]?\[^\\n\]*-64,\[^\\n\]*sp" } } +// pph asm xdiff #include "c1return-5.h" diff --git a/gcc/testsuite/g++.dg/pph/pph.map b/gcc/testsuite/g++.dg/pph/pph.map index f0c7abd..2735af8 100644 --- a/gcc/testsuite/g++.dg/pph/pph.map +++ b/gcc/testsuite/g++.dg/pph/pph.map @@ -1,5 +1,9 @@ +c120060625-1.h c120060625-1.pph c1altinc1.h c1altinc1.pph c1altinc2.h c1altinc2.pph +c1attr-warn-unused-result.h c1attr-warn-unused-result.pph +c1builtin-integral-1.h c1builtin-integral-1.pph +c1builtin-object-size-2.h c1builtin-object-size-2.pph c1chained1.h c1chained1.pph c1chained2.h c1chained2.pph c1empty.h c1empty.pph @@ -9,8 +13,13 @@ c1functions.h c1functions.pph c1guarded1.h c1guarded1.pph c1guarded2.h c1guarded2.pph c1guarded3.h c1guarded3.pph +c1limits-externalid.h c1limits-externalid.pph +c1meteor-contest.h c1meteor-contest.pph c1multinc1.h c1multinc1.pph c1multinc2.h c1multinc2.pph +c1pr36533.h c1pr36533.pph +c1pr44948-1a.h c1pr44948-1a.pph +c1return-5.h c1return-5.pph c1simple1.h c1simple1.pph c1simple2.h c1simple2.pph c1struct.h c1struct.pph diff --git a/gcc/testsuite/g++.dg/pph/x1template.cc b/gcc/testsuite/g++.dg/pph/x1template.cc index cecefd7..95ad779 100644 --- a/gcc/testsuite/g++.dg/pph/x1template.cc +++ b/gcc/testsuite/g++.dg/pph/x1template.cc @@ -1,5 +1,5 @@ // { dg-xfail-if "ICE" { "*-*-*" } { "-fpph-map=pph.map" } } -// { dg-bogus "x1template.h:18:13: internal compiler error: in resume_scope, at cp/name-lookup.c:1568" "" { xfail *-*-* } 0 } +// { dg-bogus "x1template.h:18:13: internal compiler error: in resume_scope" "" { xfail *-*-* } 0 } // { dg-prune-output "In file included from " } #include "x1template.h"