PCH test errors

PCH test errors

[Andrew Stubbs]

I'm testing amdgcn-amdhsa, and I get lot of PCH test failures with 
errors like this:

gcc.dg/pch/common-1.c:1:22: error: one or more PCH files were found, but 
they were invalid
gcc.dg/pch/common-1.c:1:22: error: use -Winvalid-pch for more information
gcc.dg/pch/common-1.c:1:10: fatal error: common-1.h: No such file or 
directory

It may affect other targets, but I've not tested those. Has anybody else 
seen this issue?

I've done a bisect, and found it first starts with this:

    1dedc12d1: revamp dump and aux output names

I've also seen others complaining about other issues with this commit, 
so this may be a duplicate issue, somehow.

Andrew

Re: PCH test errors

[Andrew Stubbs]

On 27/05/2020 15:46, Andrew Stubbs wrote:
> I'm testing amdgcn-amdhsa, and I get lot of PCH test failures with 
> errors like this:
> 
> gcc.dg/pch/common-1.c:1:22: error: one or more PCH files were found, but 
> they were invalid
> gcc.dg/pch/common-1.c:1:22: error: use -Winvalid-pch for more information
> gcc.dg/pch/common-1.c:1:10: fatal error: common-1.h: No such file or 
> directory

Hi Alexandre,

I've created a test toolchain for you on the GCC compute farm: 
gcc14.fsffrance.org

Please see /home/ams_cs/gccobj and /home/amd_cs/install.

I ran "make check RUNTESTFLAGS=pch.exp" already, so you should be able 
to see the errors in gcc/testsuite/gcc/gcc.log.

The sources and objects are set world writeable, so you should be able 
to experiment (please set "umask 0" so I can clean up after).

Andrew

Re: PCH test errors

[Alexandre Oliva]

On May 28, 2020, Andrew Stubbs <ams@codesourcery.com> wrote:

> On 27/05/2020 15:46, Andrew Stubbs wrote:
>> I'm testing amdgcn-amdhsa, and I get lot of PCH test failures with
>> errors like this:
>> 
>> gcc.dg/pch/common-1.c:1:22: error: one or more PCH files were found,
>> but they were invalid
>> gcc.dg/pch/common-1.c:1:22: error: use -Winvalid-pch for more information
>> gcc.dg/pch/common-1.c:1:10: fatal error: common-1.h: No such file or
>> directory

> I've created a test toolchain for you on the GCC compute farm:
> gcc14.fsffrance.org

Thanks!

I understand the problem, and I'm tempted to say it was a latent
preexisting problem.

gcn-hsa.h defines -mlocal-symbol-id=%b in CC1_SPEC.

This is a target option not marked as pch_ignore, so
option_affects_pch_p returns true for it, and default_pch_valid_p in
targhooks.c compares the saved option in the PCH file with the active
option in the current compilation.

In our PCH tests, because of the design of the PCH testsuite, the
basename of the header file matches the basename of the corresponding
test file, so the compare used to pass.

After the aux/dump revamp, %b changes in some link commands, and
pch-creation commands look like link commands becuase they don't have
-[cSE].  The link commands in which they don't change are those with a
single input, when the output name is the basename of the input plus an
optional executable extension.

Since 'gcc foo.h -o foo.h.gch' is not such a command, %b becomes
foo.h.gch-foo.  That's what we'd use as basenames for dump and aux
outputs, and that's what gets saved in the PCH file as local-symbol-id.


The change to dumpbase in PCH commands was as desired.  Though this is
not a link command proper, one of the goals of the revamp was to arrange
for separate compilations, with or without linking, to get different
dump and aux outputs, even when compiling the same inputs.

So, even if we were to recognize PCH as compilation commands, the
presence of an explicit -o would (or should) derive from the output the
basename that identifies the translation unit.


Looking back at the PCH tests, there was no reason for the .h and the .c
files of each test to share the same basename, it was just an
implementation detail of the testsuite.  Using a different name for the
header would have caused the PCH tests to fail even before the revamp.
I conclude from this that PCH was already mostly useless on this target,
because of -mlocal-symbol-id.

Marking -mlocal-symbol-id as PchIgnore in gcn.opt, as in the patchlet
below, might be a proper fix for the problem, but I don't know enough
about the target to tell whether doing so would be safe or correct.
Andrew, do you?

diff --git a/gcc/config/gcn/gcn.opt b/gcc/config/gcn/gcn.opt
index 04c73d6..51e3878 100644
--- a/gcc/config/gcn/gcn.opt
+++ b/gcc/config/gcn/gcn.opt
@@ -71,7 +71,7 @@ Target Report RejectNegative Joined UInteger Var(stack_size_opt) Init(-1)
 -mstack-size=<number>	Set the private segment size per wave-front, in bytes.
 
 mlocal-symbol-id=
-Target RejectNegative Report JoinedOrMissing Var(local_symbol_id) Init(0)
+Target RejectNegative Report JoinedOrMissing IgnorePch Var(local_symbol_id) Init(0)
 
 Wopenacc-dims
 Target Var(warn_openacc_dims) Warning


Another workaround, mainly for the testsuite, would be to recognize
.h.gch as an executable extension in gcc.c, as in the patchlet below.
Something conceptually like this, but further elaborated so as to
recognize all cases of PCH compilation, and checking for the %i.gch
pattern, would make the testsuite work for amdgcn-amdhsa even without
ignoring -mlocal-symbol-id changes between PCH and compilations using
it, and would avoid longer dump names in PCH compilations that used the
default gch output name.

PCH still wouldn't work on this target for other ways to name .gch files
that GCC recognizes, such as those used by libstdc++-v3, and preserving
%b unchanged for any of these possibilities just for -mlocal-symbol-id
to work would defeat the purpose of the revamp, because then, building
multiple such variants would get their dumps to overwrite each other.


diff --git a/gcc/gcc.c b/gcc/gcc.c
index e2362175f4..0a7410c 100644
--- a/gcc/gcc.c
+++ b/gcc/gcc.c
@@ -4973,6 +4973,7 @@ process_command (unsigned int decoded_options_count,
 	      : ((temp = strrchr (obase + 1, '.'))
 		 && (xlen = strlen (temp))
 		 && (strcmp (temp, ".exe") == 0
+		     || strcmp (temp, ".h.gch") == 0
 #if defined(HAVE_TARGET_EXECUTABLE_SUFFIX)
 		     || strcmp (temp, TARGET_EXECUTABLE_SUFFIX) == 0
 #endif


So I see mainly the following courses of action:

- if -mlocal-symbol-id can vary between PCH and PCH-using source, mark
it as IgnorePch and the problem is fixed.

- otherwise, change tests for PCH support so that different basenames
are used, so it won't seem to work for this target.


- regardless, it might make sense to implement the elaborate version of
the gcc.c change above, to get shorter dump names in pch compilation
from B.X to B.X.gch, though the present behavior is quite defensible; we
might prefer to just document it.


Thoughts?

-- 
Alexandre Oliva, freedom fighter    he/him    https://FSFLA.org/blogs/lxo/
Free Software Evangelist              Stallman was right, but he's left :(
GNU Toolchain Engineer           Live long and free, and prosper ethically

Re: PCH test errors

[Andrew Stubbs]

On 29/05/2020 01:00, Alexandre Oliva wrote:
> I understand the problem, and I'm tempted to say it was a latent
> preexisting problem.
> 
> gcn-hsa.h defines -mlocal-symbol-id=%b in CC1_SPEC.
> 
> This is a target option not marked as pch_ignore, so
> option_affects_pch_p returns true for it, and default_pch_valid_p in
> targhooks.c compares the saved option in the PCH file with the active
> option in the current compilation.

Thank you for the careful analysis. I would have struggled to get there 
myself.

That option is the vestige of a horrible, ugly workaround for an ELF 
loader bug in the GPU drivers, in which it would refuse to load a binary 
that had the same local symbol defined multiple times (e.g. from linking 
together .o files each containing the same local name).

That bug has been fixed in the driver, and I don't think the name 
mangling was ever committed to the upstream toolchain, but the (now 
inactive) option remains. I don't recall why; it may have been for 
backward compatibility, or it may have been an unintentional omission.

Either way, I don't think we need it in GCC 11, so I can just rip it out.

Thanks again

Andrew