From patchwork Wed Nov 23 18:26:36 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Martin Sebor X-Patchwork-Id: 83738 Delivered-To: patch@linaro.org Received: by 10.140.97.165 with SMTP id m34csp2785271qge; Wed, 23 Nov 2016 10:27:09 -0800 (PST) X-Received: by 10.99.8.133 with SMTP id 127mr7519150pgi.76.1479925629771; Wed, 23 Nov 2016 10:27:09 -0800 (PST) Return-Path: Received: from sourceware.org (server1.sourceware.org. [209.132.180.131]) by mx.google.com with ESMTPS id z3si6411668plb.85.2016.11.23.10.27.09 for (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Wed, 23 Nov 2016 10:27:09 -0800 (PST) Received-SPF: pass (google.com: domain of gcc-patches-return-442425-patch=linaro.org@gcc.gnu.org designates 209.132.180.131 as permitted sender) client-ip=209.132.180.131; Authentication-Results: mx.google.com; dkim=pass header.i=@gcc.gnu.org; spf=pass (google.com: domain of gcc-patches-return-442425-patch=linaro.org@gcc.gnu.org designates 209.132.180.131 as permitted sender) smtp.mailfrom=gcc-patches-return-442425-patch=linaro.org@gcc.gnu.org; dmarc=fail (p=NONE dis=NONE) header.from=gmail.com DomainKey-Signature: a=rsa-sha1; c=nofws; d=gcc.gnu.org; h=list-id :list-unsubscribe:list-archive:list-post:list-help:sender :subject:to:references:from:message-id:date:mime-version :in-reply-to:content-type; q=dns; s=default; b=FZ3O0/krpsugQls+7 EkZHQgujlFAQnoDH9WNHZ/tBsn+74kx6nyk71YEnyJN6EH52CjXyF62pqzZbauGK 4DTlIEd1PkryPm19twoqTGwMJQw9AUo/sh2qTkwaOKhSrcCqoMvlG30jHlvcNMJE CF8QwZhONGHJ2QI2wtxpI04tGw= 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 :subject:to:references:from:message-id:date:mime-version :in-reply-to:content-type; s=default; bh=t3Rx3Zm1CgtjABexsI1PPc5 RA/c=; b=tKlQuOow8FHn7f6gAijuIJNgBugqvHsmFv0grgY1B9XiWBcxmnrdXeC wJoYvKUMKIDirCQ5zyxDoMOu+gaCF0EYEfioOo4uWMCqxPuM3lJIiTrNakOxxvJs Lzxo7+UA3VzAz04KXcy6HzhWZmsno1uYhbQsR5in7+dpu71+wwdg= Received: (qmail 111860 invoked by alias); 23 Nov 2016 18:26:51 -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 111783 invoked by uid 89); 23 Nov 2016 18:26:50 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.0 required=5.0 tests=AWL, BAYES_00, FREEMAIL_FROM, RCVD_IN_DNSWL_LOW, RCVD_IN_SORBS_SPAM, SPF_PASS autolearn=ham version=3.3.2 spammy=collector, fits_to_tree_p, 7410, 5347 X-HELO: mail-qt0-f178.google.com Received: from mail-qt0-f178.google.com (HELO mail-qt0-f178.google.com) (209.85.216.178) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Wed, 23 Nov 2016 18:26:40 +0000 Received: by mail-qt0-f178.google.com with SMTP id w33so19493999qtc.3 for ; Wed, 23 Nov 2016 10:26:40 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:subject:to:references:from:message-id:date :user-agent:mime-version:in-reply-to; bh=f8oam9hmotdBcrAtZYiLiBaV+ZRT0ilOAKZd4iHnLSo=; b=KsNC0Ubi93006l4tbsavXSr099d/PfI/sjctcIfhC1jnfN3yJ1BdG48JECSlEFuJQx 6JXbb6gbwsBbK90chePdfbtpOIufHe17R+sdmznbSgSlIdLez3P9edI1CYBJcJz2hhRM lbbSpTLCBT6t0JcqcBqCfZLEXDo9FU3gCtYSCmw4RxoeRS9H+8KbWpx8QqdXv/7YzY4x rKcOiiF1yluqN0G6W67/GDYJo66msBXpLLvxUjIlbGM2HzsuezrmhV/BD/qfLhbWYFSU 6wCtfEWwyQtGnDTIx2dHmCgzkbL9gujVFM12rweuKG8BQnKoRtniAEQ79HAxkVCIuhSs 7cng== X-Gm-Message-State: AKaTC03qMREXlfXXTUl9yLFOE69Ql+YW8aTlKRZeSryNOmsSB8tvLGpenr8tZM5MF+IdzA== X-Received: by 10.200.44.184 with SMTP id 53mr3710163qtw.291.1479925598629; Wed, 23 Nov 2016 10:26:38 -0800 (PST) Received: from [192.168.0.26] (75-166-206-79.hlrn.qwest.net. [75.166.206.79]) by smtp.gmail.com with ESMTPSA id 29sm16882427qtu.21.2016.11.23.10.26.37 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Wed, 23 Nov 2016 10:26:38 -0800 (PST) Subject: Re: [PATCH] enable -Wformat-length for dynamically allocated buffers (pr 78245) To: Jeff Law , Gcc Patch List References: <8000f699-dee9-29be-d887-68b57ff0fafc@gmail.com> From: Martin Sebor Message-ID: <2fa72d08-0fac-28b3-4fcf-c70f83b18c8d@gmail.com> Date: Wed, 23 Nov 2016 11:26:36 -0700 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.3.0 MIME-Version: 1.0 In-Reply-To: X-IsSubscribed: yes > My only real concern here is that if we call compute_builtin_object_size > without having initialized the passes, then we initialize, compute, then > finalize. Subsequent calls will go through the same process -- the key > being each one re-computes the internal state which might get expensive. > > Wouldn't it just make more sense to pull up the init/fini calls, either > explicitly (which likely means externalizing the init/fini routines) or > by wrapping all this stuff in a class and instantiating a suitable object? > > I think the answer to your memory management question is that internal > state is likely not marked as a GC root and thus if you get a GC call > pieces of internal state are not seen as reachable, but you still can > reference them. ie, you end up with dangling pointers. > > Usually all you'd have to do is mark them so that gengtype will see > them. Bitmaps, trees, rtl, are all good examples. So marking the > bitmap would look like: > > static GTY (()) bitmap computed[4]; > > Or something like that. > > You might try --enable-checking=yes,extra,gc,gcac > > That will be slow, but is often helpful for tracking down cases where > someone has an object expected to be live across passes, but it isn't > reachable because someone failed to register a GC root. Thanks. Attached is an updated patch that avoids flushing the computed data until the current function changes, and tells the garbage collector not to collect it. I haven't finished bootstrapping and regtesting it yet but running it through Valgrind doesn't show any errors. Please let me know if this is what you had in mind. Thanks Martin PR middle-end/78245 - missing -Wformat-length on an overflow of a dynamically allocated buffer gcc/testsuite/ChangeLog: PR middle-end/78245 * gcc.dg/tree-ssa/builtin-sprintf-warn-3.c: Add tests. gcc/ChangeLog: PR middle-end/78245 * gimple-ssa-sprintf.c (object_sizes, computed): Declared GTY. (get_destination_size): Call compute_object_size. * tree-object-size.c (addr_object_size): Adjust. (pass_through_call): Adjust. (compute_object_size, internal_object_size): New functions. (compute_builtin_object_size): Call internal_object_size. (init_object_sizes): Initialize computed bitmap so the garbage collector knows about it. (fini_object_sizes): Clear the computed bitmap when non-null. (pass_object_sizes::execute): Adjust. * tree-object-size.h (compute_object_size): Declare. diff --git a/gcc/gimple-ssa-sprintf.c b/gcc/gimple-ssa-sprintf.c index ead8b0e..ea56570 100644 --- a/gcc/gimple-ssa-sprintf.c +++ b/gcc/gimple-ssa-sprintf.c @@ -2468,7 +2468,7 @@ get_destination_size (tree dest) object (the function fails without optimization in this type). */ int ost = optimize > 0; unsigned HOST_WIDE_INT size; - if (compute_builtin_object_size (dest, ost, &size)) + if (compute_object_size (dest, ost, &size)) return size; return HOST_WIDE_INT_M1U; diff --git a/gcc/testsuite/gcc.dg/tree-ssa/builtin-sprintf-warn-3.c b/gcc/testsuite/gcc.dg/tree-ssa/builtin-sprintf-warn-3.c index 8d97fa8..9874332 100644 --- a/gcc/testsuite/gcc.dg/tree-ssa/builtin-sprintf-warn-3.c +++ b/gcc/testsuite/gcc.dg/tree-ssa/builtin-sprintf-warn-3.c @@ -1,5 +1,10 @@ /* { dg-do compile } */ -/* { dg-options "-std=c99 -O2 -Wformat -Wformat-length=1 -ftrack-macro-expansion=0" } */ +/* { dg-options "-O2 -Wformat -Wformat-length=1 -ftrack-macro-expansion=0" } */ +/* Verify that all sprintf built-ins detect overflow involving directives + with non-constant arguments known to be constrained by some range of + values, and even when writing into dynamically allocated buffers. + -O2 (-ftree-vrp) is necessary for the tests involving ranges to pass, + otherwise -O1 is sufficient. */ #ifndef LINE # define LINE 0 @@ -7,18 +12,26 @@ #define bos(x) __builtin_object_size (x, 0) -#define T(bufsize, fmt, ...) \ - do { \ - if (!LINE || __LINE__ == LINE) \ - { \ - char *d = (char *)__builtin_malloc (bufsize); \ - __builtin___sprintf_chk (d, 0, bos (d), fmt, __VA_ARGS__); \ - sink (d); \ - } \ - } while (0) +/* Defined (and redefined) to the allocation function to use, either + malloc, or alloca, or a VLA. */ +#define ALLOC(p, n) (p) = __builtin_malloc (n) -void -sink (void*); +/* Defined (and redefined) to the sprintf function to exercise. */ +#define TEST_SPRINTF(d, maxsize, objsize, fmt, ...) \ + __builtin___sprintf_chk (d, 0, objsize, fmt, __VA_ARGS__) + +#define T(bufsize, fmt, ...) \ + do { \ + if (!LINE || __LINE__ == LINE) \ + { \ + char *d; \ + ALLOC (d, bufsize); \ + TEST_SPRINTF (d, 0, bos (d), fmt, __VA_ARGS__); \ + sink (d); \ + } \ + } while (0) + +void sink (void*); /* Identity function to verify that the checker figures out the value of the operand even when it's not constant (i.e., makes use of @@ -232,3 +245,88 @@ void test_sprintf_chk_range_sshort (signed short *a, signed short *b) T ( 4, "%i", Ra (998, 999)); T ( 4, "%i", Ra (999, 1000)); /* { dg-warning "may write a terminating nul past the end of the destination" } */ } + +/* Exercise ordinary sprintf with malloc. */ +#undef TEST_SPRINTF +#define TEST_SPRINTF(d, maxsize, objsize, fmt, ...) \ + __builtin_sprintf (d, fmt, __VA_ARGS__) + +void test_sprintf_malloc (const char *s, const char *t) +{ +#define x x () + + T (1, "%-s", x ? "" : "1"); /* { dg-warning "nul past the end" } */ + T (1, "%-s", x ? "1" : ""); /* { dg-warning "nul past the end" } */ + T (1, "%-s", x ? s : "1"); /* { dg-warning "nul past the end" } */ + T (1, "%-s", x ? "1" : s); /* { dg-warning "nul past the end" } */ + T (1, "%-s", x ? s : t); + + T (2, "%-s", x ? "" : "1"); + T (2, "%-s", x ? "" : s); + T (2, "%-s", x ? "1" : ""); + T (2, "%-s", x ? s : ""); + T (2, "%-s", x ? "1" : "2"); + T (2, "%-s", x ? "" : "12"); /* { dg-warning "nul past the end" } */ + T (2, "%-s", x ? "12" : ""); /* { dg-warning "nul past the end" } */ + + T (2, "%-s", x ? "" : "123"); /* { dg-warning "into a region" } */ + T (2, "%-s", x ? "123" : ""); /* { dg-warning "into a region" } */ + +#undef x +} + +/* Exercise ordinary sprintf with alloca. */ +#undef ALLOC +#define ALLOC(p, n) (p) = __builtin_alloca (n) + +void test_sprintf_alloca (const char *s, const char *t) +{ +#define x x () + + T (1, "%-s", x ? "" : "1"); /* { dg-warning "nul past the end" } */ + T (1, "%-s", x ? "1" : ""); /* { dg-warning "nul past the end" } */ + T (1, "%-s", x ? s : "1"); /* { dg-warning "nul past the end" } */ + T (1, "%-s", x ? "1" : s); /* { dg-warning "nul past the end" } */ + T (1, "%-s", x ? s : t); + + T (2, "%-s", x ? "" : "1"); + T (2, "%-s", x ? "" : s); + T (2, "%-s", x ? "1" : ""); + T (2, "%-s", x ? s : ""); + T (2, "%-s", x ? "1" : "2"); + T (2, "%-s", x ? "" : "12"); /* { dg-warning "nul past the end" } */ + T (2, "%-s", x ? "12" : ""); /* { dg-warning "nul past the end" } */ + + T (2, "%-s", x ? "" : "123"); /* { dg-warning "into a region" } */ + T (2, "%-s", x ? "123" : ""); /* { dg-warning "into a region" } */ + +#undef x +} + +/* Exercise ordinary sprintf with a VLA. */ +#undef ALLOC +#define ALLOC(p, n) char vla [i (n)]; (p) = vla + +void test_sprintf_vla (const char *s, const char *t) +{ +#define x x () + + T (1, "%-s", x ? "" : "1"); /* { dg-warning "nul past the end" } */ + T (1, "%-s", x ? "1" : ""); /* { dg-warning "nul past the end" } */ + T (1, "%-s", x ? s : "1"); /* { dg-warning "nul past the end" } */ + T (1, "%-s", x ? "1" : s); /* { dg-warning "nul past the end" } */ + T (1, "%-s", x ? s : t); + + T (2, "%-s", x ? "" : "1"); + T (2, "%-s", x ? "" : s); + T (2, "%-s", x ? "1" : ""); + T (2, "%-s", x ? s : ""); + T (2, "%-s", x ? "1" : "2"); + T (2, "%-s", x ? "" : "12"); /* { dg-warning "nul past the end" } */ + T (2, "%-s", x ? "12" : ""); /* { dg-warning "nul past the end" } */ + + T (2, "%-s", x ? "" : "123"); /* { dg-warning "into a region" } */ + T (2, "%-s", x ? "123" : ""); /* { dg-warning "into a region" } */ + +#undef x +} diff --git a/gcc/tree-object-size.c b/gcc/tree-object-size.c index 1317ad7..39c32e3 100644 --- a/gcc/tree-object-size.c +++ b/gcc/tree-object-size.c @@ -50,6 +50,8 @@ static const unsigned HOST_WIDE_INT unknown[4] = { 0 }; +static bool internal_object_size (tree, int, unsigned HOST_WIDE_INT *); +static void fini_object_sizes (void); static tree compute_object_offset (const_tree, const_tree); static bool addr_object_size (struct object_size_info *, const_tree, int, unsigned HOST_WIDE_INT *); @@ -72,10 +74,10 @@ static void check_for_plus_in_loops_1 (struct object_size_info *, tree, the subobject (innermost array or field with address taken). object_sizes[2] is lower bound for number of bytes till the end of the object and object_sizes[3] lower bound for subobject. */ -static vec object_sizes[4]; +static GTY (()) vec object_sizes[4]; /* Bitmaps what object sizes have been computed already. */ -static bitmap computed[4]; +static GTY (()) bitmap computed[4]; /* Maximum value of offset we consider to be addition. */ static unsigned HOST_WIDE_INT offset_limit; @@ -187,7 +189,7 @@ addr_object_size (struct object_size_info *osi, const_tree ptr, if (!osi || (object_size_type & 1) != 0 || TREE_CODE (TREE_OPERAND (pt_var, 0)) != SSA_NAME) { - compute_builtin_object_size (TREE_OPERAND (pt_var, 0), + internal_object_size (TREE_OPERAND (pt_var, 0), object_size_type & ~1, &sz); } else @@ -491,14 +493,14 @@ pass_through_call (const gcall *call) } -/* Compute __builtin_object_size value for PTR and set *PSIZE to - the resulting value. OBJECT_SIZE_TYPE is the second argument - to __builtin_object_size. Return true on success and false - when the object size could not be determined. */ +/* Compute the size of the object pointed to by PTR and set *PSIZE + to the resulting value. OBJECT_SIZE_TYPE is the second argument + to __builtin_object_size. Return true on success and false when + the object size could not be determined. */ bool -compute_builtin_object_size (tree ptr, int object_size_type, - unsigned HOST_WIDE_INT *psize) +internal_object_size (tree ptr, int object_size_type, + unsigned HOST_WIDE_INT *psize) { gcc_assert (object_size_type >= 0 && object_size_type <= 3); @@ -534,7 +536,7 @@ compute_builtin_object_size (tree ptr, int object_size_type, ptr = gimple_assign_rhs1 (def); if (cst_and_fits_in_hwi (offset) - && compute_builtin_object_size (ptr, object_size_type, psize)) + && internal_object_size (ptr, object_size_type, psize)) { /* Return zero when the offset is out of bounds. */ unsigned HOST_WIDE_INT off = tree_to_shwi (offset); @@ -664,6 +666,45 @@ compute_builtin_object_size (tree ptr, int object_size_type, return *psize != unknown[object_size_type]; } +/* Compute __builtin_object_size value for PTR and set *PSIZE to + the resulting value. OBJECT_SIZE_TYPE is the second argument + to __builtin_object_size. Return true on success and false + when the object size could not be determined. */ + +bool +compute_builtin_object_size (tree ptr, int object_size_type, + unsigned HOST_WIDE_INT *psize) +{ + return internal_object_size (ptr, object_size_type, psize); +} + + +/* Like compute_builtin_object_size but intended to be called + without a corresponding __builtin_object_size in the program. */ + +bool +compute_object_size (tree ptr, int object_size_type, + unsigned HOST_WIDE_INT *psize) +{ + static unsigned lastfunchash; + unsigned curfunchash + = IDENTIFIER_HASH_VALUE (DECL_NAME (current_function_decl)); + + /* Initialize the internal data structures for each new function + and keep the computed data around for any subsequent calls to + compute_object_size. */ + if (curfunchash != lastfunchash) + { + lastfunchash = curfunchash; + fini_object_sizes (); + init_object_sizes (); + } + + bool retval = internal_object_size (ptr, object_size_type, psize); + + return retval; +} + /* Compute object_sizes for PTR, defined to VALUE, which is not an SSA_NAME. */ static void @@ -1221,7 +1262,7 @@ init_object_sizes (void) for (object_size_type = 0; object_size_type <= 3; object_size_type++) { object_sizes[object_size_type].safe_grow (num_ssa_names); - computed[object_size_type] = BITMAP_ALLOC (NULL); + computed[object_size_type] = BITMAP_GGC_ALLOC (); } init_offset_limit (); @@ -1238,7 +1279,11 @@ fini_object_sizes (void) for (object_size_type = 0; object_size_type <= 3; object_size_type++) { object_sizes[object_size_type].release (); - BITMAP_FREE (computed[object_size_type]); + if (computed[object_size_type]) + { + bitmap_clear (computed[object_size_type]); + computed[object_size_type] = NULL; + } } } @@ -1325,7 +1370,7 @@ pass_object_sizes::execute (function *fun) { tree type = TREE_TYPE (lhs); unsigned HOST_WIDE_INT bytes; - if (compute_builtin_object_size (ptr, object_size_type, + if (internal_object_size (ptr, object_size_type, &bytes) && wi::fits_to_tree_p (bytes, type)) { diff --git a/gcc/tree-object-size.h b/gcc/tree-object-size.h index 38c3e07..60256e6 100644 --- a/gcc/tree-object-size.h +++ b/gcc/tree-object-size.h @@ -21,6 +21,7 @@ along with GCC; see the file COPYING3. If not see #define GCC_TREE_OBJECT_SIZE_H extern void init_object_sizes (void); +extern bool compute_object_size (tree, int, unsigned HOST_WIDE_INT *); extern bool compute_builtin_object_size (tree, int, unsigned HOST_WIDE_INT *); #endif // GCC_TREE_OBJECT_SIZE_H