On May 28, 2020, Andrew Stubbs <a...@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
