diff mbox

Temporarily revert Steven's PCH changes for 4.8 (PR pch/54117)

Message ID 20130213234849.GB1215@tucnak.zalov.cz
State New
Headers show

Commit Message

Jakub Jelinek Feb. 13, 2013, 11:48 p.m. UTC
On Thu, Feb 14, 2013 at 12:41:30AM +0100, Steven Bosscher wrote:
> > I agree with David that it might be better to give up pch.
> 
> That'd be a really a good start.

You can do it with say following patch instead, PCH is just a compile time
optimization, so for -gstabs IMHO it is just fine if we just give up.



	Jakub

Comments

Steven Bosscher Feb. 14, 2013, 10:19 a.m. UTC | #1
On Thu, Feb 14, 2013 at 12:48 AM, Jakub Jelinek wrote:
> On Thu, Feb 14, 2013 at 12:41:30AM +0100, Steven Bosscher wrote:
>> > I agree with David that it might be better to give up pch.
>>
>> That'd be a really a good start.
>
> You can do it with say following patch instead, PCH is just a compile time
> optimization, so for -gstabs IMHO it is just fine if we just give up.

Does that also provent *writing* a PCH with -gstabs?

The point of making asm_out_file write-only and stop front ends from
writing to it, is to postpone opening asm_out_file to some point as
late as possible. For this to work, attempting to write a PCH must
also be disabled (because of the early stabs output to asm_out_file).

Ciao!
Steven
Jakub Jelinek Feb. 14, 2013, 10:52 a.m. UTC | #2
On Thu, Feb 14, 2013 at 11:19:22AM +0100, Steven Bosscher wrote:
> On Thu, Feb 14, 2013 at 12:48 AM, Jakub Jelinek wrote:
> > On Thu, Feb 14, 2013 at 12:41:30AM +0100, Steven Bosscher wrote:
> >> > I agree with David that it might be better to give up pch.
> >>
> >> That'd be a really a good start.
> >
> > You can do it with say following patch instead, PCH is just a compile time
> > optimization, so for -gstabs IMHO it is just fine if we just give up.
> 
> Does that also provent *writing* a PCH with -gstabs?

No, it prevents *reading* of PCH files with -gstabs, so whatever you write
into the PCH file is uninteresting.  The patch can surely be acompanied
by a patch to warn that writing a PCH file is useless in that case, as in
your patch, though the spot you chose for it is too early.  Consider
./xgcc -B ./ -gstabs -o aa.h.gch aa.h -g0
cc1: warning: the "stabs" debug format cannot be used with pre-compiled headers [-Wdeprecated]
which warns even when the header is actually precompiled without debug info.
If you try to use such PCH file with say
./xgcc -B ./ -gstabs aa.c
it will fail to load PCH because of option mismatch (similarly for -g
created PCH file and non-g user), but if you do
./xgcc -B ./ -gstabs aa.c -g0
there should be no reason why it wouldn't work right, so you shouldn't warn
earlier.
So, IMHO you want to warn at the spot which I've changed in my patch,
add
if (pch_file && (write_symbols != NO_DEBUG && write_symbols != DWARF2_DEBUG))
  warning...
there.
And, of course, pch.exp testsuite won't work if -gstabs etc. is in effect,
because it does those hacks which ensure that if PCH isn't loaded, tests
fail (the original header file isn't available when testing PCH load).
That is not a problem for normal PCH uses, where the headers are available,
but pch.exp would need to be tweaked for it somehow.
Supposedly not do any testing if -g (defaulting to non-dwarf) or -gstabs
etc. are present in the default options (try to pre-compile some short
header first, if that fails with the newly added warning, punt),
drop all torture options using -g in it if such test fails with explicit
-g added from the list of torture options for testing, and perhaps add some
effective target if needed (valid-1.c test?).

	Jakub
Jeff Law Feb. 14, 2013, 12:48 p.m. UTC | #3
On 02/14/13 03:52, Jakub Jelinek wrote:
>
> No, it prevents *reading* of PCH files with -gstabs, so whatever you write
> into the PCH file is uninteresting.  The patch can surely be acompanied
> by a patch to warn that writing a PCH file is useless in that case, as in
> your patch, though the spot you chose for it is too early.  Consider
> ./xgcc -B ./ -gstabs -o aa.h.gch aa.h -g0
[ ... ]
It's a fairly minor point, but I see what you're saying.




> Supposedly not do any testing if -g (defaulting to non-dwarf) or -gstabs
> etc. are present in the default options (try to pre-compile some short
> header first, if that fails with the newly added warning, punt),
> drop all torture options using -g in it if such test fails with explicit
> -g added from the list of torture options for testing, and perhaps add some
> effective target if needed (valid-1.c test?).
I think two tests should be sufficient.  First, compile a simple program 
with -g and verify it generates dwarf2 debug records.  Second verify 
that there aren't any -g<foo> options, unless <foo> is dwarf2.

I'm actually on PTO today/tomorrow, so I won't be able to look further 
at this until Monday.

If someone wants to run with it, my recommendation would be Steven's 
warning patch, moved to the location Jakub suggests so that users know 
stabs+PCH is going away plus a hack to the testsuite suite similar to 
what I outlined above plus something to either suppress creation of the 
pch or suppress reading the PCH in the presence of non-dwarf debug output.

--

Jakub -- presumably the implicit include of stdc-predef.h is what is 
causing the testing differences we were seeing.

Jeff
diff mbox

Patch

--- gcc/c-family/c-opts.c.jj	2013-02-13 23:48:14.000000000 +0100
+++ gcc/c-family/c-opts.c	2013-02-14 00:41:05.325625590 +0100
@@ -943,8 +943,11 @@  c_common_post_options (const char **pfil
 
       /* When writing a PCH file, avoid reading some other PCH file,
 	 because the default address space slot then can't be used
-	 for the output PCH file.  */
-      if (pch_file)
+	 for the output PCH file.  Also, avoid reading PCH files when
+	 debug info in format other than DWARF is requested.  */
+      if (pch_file
+	  || (write_symbols != NO_DEBUG
+	      && write_symbols != DWARF2_DEBUG))
 	c_common_no_more_pch ();
 
       /* Yuk.  WTF is this?  I do know ObjC relies on it somewhere.  */