This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [LTO merge][8/15] GIMPLE streamer


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


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]