Re: [PATCH] preprocessor/58580 - preprocessor goes OOM with warning for zero literals

[Jakub Jelinek <jakub@redhat.com>]

On Mon, Nov 04, 2013 at 12:46:10PM +0100, Dodji Seketeli wrote:
> --- a/gcc/diagnostic.c
> +++ b/gcc/diagnostic.c
> @@ -259,12 +259,13 @@ diagnostic_build_prefix (diagnostic_context *context,
>     MAX_WIDTH by some margin, then adjust the start of the line such
>     that the COLUMN is smaller than MAX_WIDTH minus the margin.  The
>     margin is either 10 characters or the difference between the column
> -   and the length of the line, whatever is smaller.  */
> +   and the length of the line, whatever is smaller.  The length of
> +   LINE is given by LINE_WIDTH.  */
>  static const char *
> -adjust_line (const char *line, int max_width, int *column_p)
> +adjust_line (const char *line, int line_width,
> +	     int max_width, int *column_p)

Eventually, I think using int for sizes is just a ticking bomb, what if
somebody uses > 2GB long lines?  Surely, on 32-bit hosts we are unlikely to
handle it, but why couldn't 64-bit host handle it?  Column info maybe bogus
in there, sure, but at least we should not crash or overflow buffers on it
;).  Anyway, not something needed to be fixed right now, but in the future
it would be nicer to use size_t and/or ssize_t here.

>  {
>    int right_margin = 10;
> -  int line_width = strlen (line);
>    int column = *column_p;
>  
>    right_margin = MIN (line_width - column, right_margin);
> @@ -284,6 +285,7 @@ diagnostic_show_locus (diagnostic_context * context,
>  		       const diagnostic_info *diagnostic)
>  {
>    const char *line;
> +  int line_width;
>    char *buffer;
>    expanded_location s;
>    int max_width;
> @@ -297,22 +299,25 @@ diagnostic_show_locus (diagnostic_context * context,
>  
>    context->last_location = diagnostic->location;
>    s = expand_location_to_spelling_point (diagnostic->location);
> -  line = location_get_source_line (s);
> +  line = location_get_source_line (s, line_width);

I think richi didn't like C++ reference arguments to be used that way (and
perhaps guidelines don't either), because it isn't immediately obvious
that line_width is modified by the call.  Can you change it to a pointer
argument instead and pass &line_width?
> +      *lineptr = XNEWVEC (char, *n);
> +      if (*lineptr == NULL)
> +	return -1;

XNEWVEC or XRESIZEVEC will never return NULL though, so it doesn't have
to be tested.  Though, the question is if that is what we want, caret
diagnostics should be optional, if we can't print it, we just won't.
So perhaps using malloc/realloc here would be better?

>  
>  const char *
> -location_get_source_line (expanded_location xloc)
> +location_get_source_line (expanded_location xloc,
> +			  int& line_len)

Ditto.

Otherwise, LGTM.

	Jakub