This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: [LTO merge][8/15] GIMPLE streamer
- From: "Joseph S. Myers" <joseph at codesourcery dot com>
- To: Diego Novillo <dnovillo at google dot com>
- Cc: gcc-patches at gcc dot gnu dot org
- Date: Tue, 29 Sep 2009 22:52:33 +0000 (UTC)
- Subject: Re: [LTO merge][8/15] GIMPLE streamer
- References: <20090929012105.GA13650@google.com>
On Mon, 28 Sep 2009, Diego Novillo wrote:
> This patch adds the gimple streamer.
My comments on this patch relate to the portability of output (to
different hosts for the same target), diagnostics and error handling.
I can see some issues here with output portability that should be
addressed for objects with LTO information to be portable between hosts.
1. There are some structures lto_*header that include 16-bit or 32-bit
integer fields (plus an enum). It appears that the byte sequences for
these headers get written out verbatim eithout regard for endianness
issues, and reading in a file written on a host with the other endianness
will result in an assertion failure. Regarding the appropriateness of an
assertion failure here see my comments on error handling, but it should
not be hard to write out each field explicitly with defined endianness
that does not depend on the endianness of the host. There are other
places writing out individual integers like this (at least
write_global_references, lto_output_decl_state_refs, write_symbol_vec)
that will also need fixing.
2. The storing of values in bitpacks may depend on the number of bits in
various types. I haven't looked at whether how the integers of size
BITS_PER_BITPACK_WORD = HOST_BITS_PER_WIDE_INT are then written out to see
whether the host endianness is also relevant there; if it is, that should
be addressed.
(a) The cases where HOST_BITS_PER_WIDE_INT bits are stored in a bitpack
would most simply be addressed by making LTO imply 64-bit (exactly)
HOST_WIDE_INT for all hosts and targets. (I'd be happy for
HOST_WIDE_INT to be made 64-bit everywhere whether or not LTO is
enabled, in the interests of avoiding host-dependency in code
generation, but making it so for LTO may be less controversial.)
(b) We can reasonably assume HOST_BITS_PER_INT to be 32, and
HOST_BITS_PER_SHORT to be 16, for all hosts, but a static assertion that
these are so if building LTO would ensure no problems arise with it
building but producing bad objects on unusual hosts. Likewise for the
case using sizeof (unsigned) * 8.
(c) REAL_CSTs, writing out HOST_BITS_PER_LONG bits, may take more care.
The obvious way is to encode them in target format before writing out,
then decode when reading in.
Regarding diagnostics, you have
> + error ("Tree code %qs is not supported in gimple streams",
> + tree_code_name[code]);
> + gcc_unreachable ();
where "Tree" should not be capitalized, and this combination should just
be internal_error (). You also have some diagnostic functions in
lto-wrapper.c whose operand names (cmsgid) imply that the error message is
passed to gettext for translation - but these function do not carry out
that translation. You need to call gcc_init_libintl early in
lto-wrapper's main (), and pass the messages to _() for translation.
Where you have
> + fatal ("%s terminated with signal %d [%s]%s",
> + prog, sig, strsignal(sig),
> + WCOREDUMP(status) ? ", core dumped" : "");
this is not i18n-friendly, and you should have two separate calls to fatal
with distinct error messages, one with the "core dumped" and one without.
Regarding error handling, the code has a great many calls to gcc_assert
and gcc_unreachable and comparatively few actual errors. My specific
concern here is about ICEs when *reading* LTO information from a previous
compilation; invalid or corrupt information in a .o file provided by the
user should result in a normal error, not an ICE, and it looks rather like
it would tend to result in assertion failures. I don't think there's
anything here needing fixing for the merge and it's certainly less
important than the host portability issues above, but if there isn't
already there should be a PR to distinguish properly between invalid user
input and compiler bugs, unless you're confident the code does make that
distinction correctly.
Using assertions that zlib's deflate/inflate functions do not return
errors is especially dubious; they can quite reasonably return Z_MEM_ERROR
if out of memory, and that should not be an ICE, quite apart from errors
from inflating corrupted data.
--
Joseph S. Myers
joseph@codesourcery.com