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

[Diego Novillo <dnovillo at google dot com>]

On Tue, Sep 29, 2009 at 18:52, Joseph S. Myers <joseph@codesourcery.com> wrote:
> 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.

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=41526

> 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

Fixed.

> 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.

Fixed.

> Regarding error handling, the code has a great many calls to gcc_assert
> and gcc_unreachable and comparatively few actual errors. ÂMy specific

I fixed all the most obvious ones.  Anything that resulted from an
unexpected value read from the stream is now an internal_error with an
appropriate message.

> 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.

Fixed.

I'm testing a combined patch with these fixes and those in other reviews.


Thanks.  Diego.