From patchwork Mon Nov 4 15:40:38 2013 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Dodji Seketeli X-Patchwork-Id: 288197 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from sourceware.org (server1.sourceware.org [209.132.180.131]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (Client did not present a certificate) by ozlabs.org (Postfix) with ESMTPS id A582D2C00AA for ; Tue, 5 Nov 2013 02:42:00 +1100 (EST) DomainKey-Signature: a=rsa-sha1; c=nofws; d=gcc.gnu.org; h=list-id :list-unsubscribe:list-archive:list-post:list-help:sender:from :to:cc:subject:references:date:in-reply-to:message-id :mime-version:content-type; q=dns; s=default; b=noMnsERrtW/dps+l sAod+GMHb7zCqdIftAXQhREk10yTk55dul6Uu45bk9Bg/ljIengUbQ49GfreHEjf Su90nE7ENEXT5Nu3ajcj2pk9w5xUo+LVRXIej1ypRU9w7rUSwQZMvs6hcAsPl8jr EvXiRBeTzuKbPA89mz4sm0gcSGg= DKIM-Signature: v=1; a=rsa-sha1; c=relaxed; d=gcc.gnu.org; h=list-id :list-unsubscribe:list-archive:list-post:list-help:sender:from :to:cc:subject:references:date:in-reply-to:message-id :mime-version:content-type; s=default; bh=AXkvCD+boXAuDDjcVtgM/W UEt9c=; b=Lo1U+jIJvR6XNuiG7FdbuV86YfjmUV7B6BkLgiNt855XsTQRGW1nLo xCYKZ9uyDhme6GZd9iVMsFYq+MIY7BxaCCzHGrDH814Tg8+FIAJBLAovfk0baaGJ MMFRr/l4L8N4EMA39bee6OBl4NIyuuOdarE7wl/3g5SRj965Zv+Ys= Received: (qmail 16834 invoked by alias); 4 Nov 2013 15:40:54 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Delivered-To: mailing list gcc-patches@gcc.gnu.org Received: (qmail 16643 invoked by uid 89); 4 Nov 2013 15:40:51 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.2 required=5.0 tests=AWL, BAYES_50, RDNS_NONE, SPF_HELO_PASS, SPF_PASS autolearn=no version=3.3.2 X-HELO: mx1.redhat.com Received: from Unknown (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Mon, 04 Nov 2013 15:40:50 +0000 Received: from int-mx09.intmail.prod.int.phx2.redhat.com (int-mx09.intmail.prod.int.phx2.redhat.com [10.5.11.22]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id rA4Fefjm030058 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK); Mon, 4 Nov 2013 10:40:42 -0500 Received: from localhost (ovpn-116-76.ams2.redhat.com [10.36.116.76]) by int-mx09.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id rA4FedHL029361 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO); Mon, 4 Nov 2013 10:40:40 -0500 Received: by localhost (Postfix, from userid 1000) id 4B55416503C; Mon, 4 Nov 2013 16:40:38 +0100 (CET) From: Dodji Seketeli To: Jakub Jelinek Cc: Bernd Edlinger , "gcc-patches\@gcc.gnu.org" Subject: Re: [PATCH] preprocessor/58580 - preprocessor goes OOM with warning for zero literals References: <20131031144309.GR27813@tucnak.zalov.cz> <87y559xz7y.fsf@redhat.com> <20131031173649.GW27813@tucnak.zalov.cz> <87r4awtmnx.fsf@redhat.com> <20131104115748.GO27813@tucnak.zalov.cz> X-URL: http://www.redhat.com Date: Mon, 04 Nov 2013 16:40:38 +0100 In-Reply-To: <20131104115748.GO27813@tucnak.zalov.cz> (Jakub Jelinek's message of "Mon, 4 Nov 2013 12:57:48 +0100") Message-ID: <87zjpkrx8p.fsf@redhat.com> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/24.2 (gnu/linux) MIME-Version: 1.0 Jakub Jelinek writes: [...] > 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. Yes. I initially tried to use size_t but found that I'd need to modify several other places to shutdown many warning because these places where using int :-(. So I felt that would be a battle for later. But I am adding this to my TODO. I'll send a patch later that changes this to size_t then, and adjusts the other places that need it as well. [...] >> 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? Sure. I have done the change in the patch below. Sorry for this reflex. I tend to use pointers like these only in places where we can allow them to be NULL. > 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. Hmmh. This particular bug was noticed because of the explicit OOM message displayed by XNEWVEC/XRESIZEVEC; otherwise, I bet this could have just felt through the crack for a little longer. So I'd say let's just use XNEWVEC/XRESIZEVEC and remove the test, as you first suggested. The caret diagnostics functionality as a whole can be disabled with -fno-diagnostic-show-caret. [...] > Otherwise, LGTM. Thanks. So here is the patch that bootstraps. gcc/ChangeLog: * input.h (location_get_source_line): Take an additional line_size parameter by reference. * input.c (get_line): New static function definition. (read_line): Take an additional line_length output parameter to be set to the size of the line. Use the new get_line function to compute the size of the line returned by fgets, rather than using strlen. Ensure that the buffer is initially zeroed; ensure that when growing the buffer too. (location_get_source_line): Take an additional output line_len parameter. Update the use of read_line to pass it the line_len parameter. * diagnostic.c (adjust_line): Take an additional input parameter for the length of the line, rather than calculating it with strlen. (diagnostic_show_locus): Adjust the use of location_get_source_line and adjust_line with respect to their new signature. While displaying a line now, do not stop at the first null byte. Rather, display the zero byte as a space and keep going until we reach the size of the line. gcc/testsuite/ChangeLog: * c-c++-common/cpp/warning-zero-in-literals-1.c: New test file. --- gcc/diagnostic.c | 17 ++-- gcc/input.c | 100 ++++++++++++++++++--- gcc/input.h | 3 +- .../c-c++-common/cpp/warning-zero-in-literals-1.c | Bin 0 -> 240 bytes 4 files changed, 99 insertions(+), 21 deletions(-) create mode 100644 gcc/testsuite/c-c++-common/cpp/warning-zero-in-literals-1.c diff --git a/gcc/testsuite/c-c++-common/cpp/warning-zero-in-literals-1.c b/gcc/testsuite/c-c++-common/cpp/warning-zero-in-literals-1.c new file mode 100644 index 0000000000000000000000000000000000000000..ff2ed962ac96e47ae05b0b040f4e10b8e09637e2 GIT binary patch literal 240 zcmdPbSEyDlast_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); if (line == NULL) return; max_width = context->caret_max_width; - line = adjust_line (line, max_width, &(s.column)); + line = adjust_line (line, line_width, max_width, &(s.column)); pp_newline (context->printer); saved_prefix = pp_get_prefix (context->printer); pp_set_prefix (context->printer, NULL); pp_space (context->printer); - while (max_width > 0 && *line != '\0') + while (max_width > 0 && line_width > 0) { char c = *line == '\t' ? ' ' : *line; + if (c == '\0') + c = ' '; pp_character (context->printer, c); max_width--; + line_width--; line++; } pp_newline (context->printer); diff --git a/gcc/input.c b/gcc/input.c index a141a92..2ee7882 100644 --- a/gcc/input.c +++ b/gcc/input.c @@ -87,44 +87,116 @@ expand_location_1 (source_location loc, return xloc; } -/* Reads one line from file into a static buffer. */ +/* This function reads a line that might contain bytes whose value is + zero. It returns the number of bytes read. Note that this + function has been adapted from getline() and _IO_getdelim() GNU C + library functions. It's been duplicated here because the getline() + function is not necessarily present on all platforms. + + LINEPTR points to a buffer that is to contain the line read. + + N points to the size of the the LINEPTR buffer. + + FP points to the file to consider. */ + +static ssize_t +get_line (char **lineptr, size_t *n, FILE *fp) +{ + ssize_t cur_len = 0, len; + char buf[16384]; + + if (lineptr == NULL || n == NULL) + return -1; + + if (*lineptr == NULL || *n == 0) + { + *n = 120; + *lineptr = XNEWVEC (char, *n); + } + + len = fread (buf, 1, sizeof buf, fp); + if (ferror (fp)) + return -1; + + for (;;) + { + size_t needed; + char *t = (char*) memchr (buf, '\n', len); + if (t != NULL) len = (t - buf) + 1; + if (__builtin_expect (len >= SSIZE_MAX - cur_len, 0)) + return -1; + needed = cur_len + len + 1; + if (needed > *n) + { + char *new_lineptr; + if (needed < 2 * *n) + needed = 2 * *n; + new_lineptr = XRESIZEVEC (char, *lineptr, needed); + *lineptr = new_lineptr; + *n = needed; + } + memcpy (*lineptr + cur_len, buf, len); + cur_len += len; + if (t != NULL) + break; + len = fread (buf, 1, sizeof buf, fp); + if (ferror (fp)) + return -1; + if (len == 0) + break; + } + (*lineptr)[cur_len] = '\0'; + return cur_len; +} + +/* Reads one line from FILE into a static buffer. LINE_LENGTH is set + by this function to the length of the returned line. Note that the + returned line can contain several zero bytes. */ static const char * -read_line (FILE *file) +read_line (FILE *file, int *line_length) { static char *string; - static size_t string_len; + static size_t string_len, cur_len; size_t pos = 0; char *ptr; if (!string_len) { string_len = 200; - string = XNEWVEC (char, string_len); + string = XCNEWVEC (char, string_len); } + else + memset (string, 0, string_len); - while ((ptr = fgets (string + pos, string_len - pos, file))) + ptr = string; + cur_len = string_len; + while (size_t len = get_line (&ptr, &cur_len, file)) { - size_t len = strlen (string + pos); - - if (string[pos + len - 1] == '\n') + if (ptr[len - 1] == '\n') { - string[pos + len - 1] = 0; + ptr[len - 1] = 0; + *line_length = len; return string; } pos += len; string = XRESIZEVEC (char, string, string_len * 2); string_len *= 2; - } - + ptr = string + pos; + cur_len = string_len - pos; + } + + *line_length = pos ? string_len : 0; return pos ? string : NULL; } /* Return the physical source line that corresponds to xloc in a buffer that is statically allocated. The newline is replaced by - the null character. */ + the null character. Note that the line can contain several null + characters, so LINE_LEN contains the actual length of the line. */ const char * -location_get_source_line (expanded_location xloc) +location_get_source_line (expanded_location xloc, + int& line_len) { const char *buffer; int lines = 1; @@ -132,7 +204,7 @@ location_get_source_line (expanded_location xloc) if (!stream) return NULL; - while ((buffer = read_line (stream)) && lines < xloc.line) + while ((buffer = read_line (stream, &line_len)) && lines < xloc.line) lines++; fclose (stream); diff --git a/gcc/input.h b/gcc/input.h index 8fdc7b2..79b3a10 100644 --- a/gcc/input.h +++ b/gcc/input.h @@ -37,7 +37,8 @@ extern char builtins_location_check[(BUILTINS_LOCATION < RESERVED_LOCATION_COUNT) ? 1 : -1]; extern expanded_location expand_location (source_location); -extern const char *location_get_source_line (expanded_location xloc); +extern const char *location_get_source_line (expanded_location xloc, + int& line_size); extern expanded_location expand_location_to_spelling_point (source_location); extern source_location expansion_point_location_if_in_system_header (source_location);