From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 55485 invoked by alias); 21 Sep 2016 15:10:03 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Received: (qmail 55265 invoked by uid 89); 21 Sep 2016 15:10:02 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-5.0 required=5.0 tests=BAYES_00,RP_MATCHES_RCVD,SPF_HELO_PASS autolearn=ham version=3.3.2 spammy=promoting, sentences, hundred, substitute X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Wed, 21 Sep 2016 15:10:01 +0000 Received: from int-mx09.intmail.prod.int.phx2.redhat.com (int-mx09.intmail.prod.int.phx2.redhat.com [10.5.11.22]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id D3E588E688; Wed, 21 Sep 2016 15:09:59 +0000 (UTC) Received: from tucnak.zalov.cz (ovpn-204-43.brq.redhat.com [10.40.204.43]) by int-mx09.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id u8LF9w8D004076 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=NO); Wed, 21 Sep 2016 11:09:59 -0400 Received: from tucnak.zalov.cz (localhost [127.0.0.1]) by tucnak.zalov.cz (8.15.2/8.15.2) with ESMTP id u8LF9ulB002401; Wed, 21 Sep 2016 17:09:57 +0200 Received: (from jakub@localhost) by tucnak.zalov.cz (8.15.2/8.15.2/Submit) id u8LF9srZ002400; Wed, 21 Sep 2016 17:09:54 +0200 Date: Wed, 21 Sep 2016 15:19:00 -0000 From: Jakub Jelinek To: Jeff Law , Martin Sebor Cc: gcc-patches@gcc.gnu.org Subject: [PATCH] Fix various minor gimple-ssa-sprintf.c issues Message-ID: <20160921150954.GC7282@tucnak.redhat.com> Reply-To: Jakub Jelinek MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline User-Agent: Mutt/1.5.24 (2015-08-30) X-IsSubscribed: yes X-SW-Source: 2016-09/txt/msg01438.txt.bz2 Hi! When looking at PR77676, I've noticed various small formatting etc. issues, like not using is_gimple_* APIs where we have them, not using gimple_call_builtin_p/gimple_call_fndecl (this one actually can show up, if e.g. uses the builtin with incorrect arguments (fewer, different types etc.)), one pasto, 2 spaces in comments instead of 1 in the middle of sentences. And, lastly 0 < var is very unusual ordering of the comparison operands, while we have a couple of such cases in the sources, usually it is when using 0 < var && var <= someotherconst, while var > 0 is used hundred times more often. I see various other issues, which I'll happily defer to Martin, e.g. /* These are available as macros in the C and C++ front ends but, sadly, not here. */ static tree intmax_type_node; static tree uintmax_type_node; /* Initialize the intmax nodes above the first time through here. */ if (!intmax_type_node) build_intmax_type_nodes (&intmax_type_node, &uintmax_type_node); This is a big no no, the trees aren't registered with GC this way, so they could be garbage collected (you'd need to move those to file-scope to be able to GC register them). Or /* Compute the maximum just once. */ static const int a_max[] = { format_floating_max (double_type_node, 'a'), format_floating_max (long_double_type_node, 'a') }; etc. (used several time), do we really need to mess up with the (thread-safe) local static locking/guards etc.? Can't you initialize the cache say to -1 and compute first if you need it and it is still -1? PR77676 shows various other issues in the pass. Bootstrapped/regtested on x86_64-linux, ok for trunk? 2016-09-21 Jakub Jelinek * gimple-ssa-sprintf.c: Fix comment formatting. (pass_sprintf_length::gate): Use x > 0 instead of 0 < x. (format_integer): Use is_gimple_assign. (format_floating, format_string, format_directive, get_destination_size): Use x > 0 instead of 0 < x. (pass_sprintf_length::handle_gimple_call): Likewise. Use gimple_call_builtin_p and gimple_call_fndecl. Reorder case BUILT_IN_SPRINTF_CHK. Fix up BUILT_IN_SNPRINTF_CHK comment. Replace "to to" with "to" in comment. (pass_sprintf_length::execute): Use is_gimple_call. --- gcc/gimple-ssa-sprintf.c.jj 2016-09-21 08:54:15.000000000 +0200 +++ gcc/gimple-ssa-sprintf.c 2016-09-21 15:09:02.209261013 +0200 @@ -37,7 +37,7 @@ along with GCC; see the file COPYING3. The pass handles all forms standard sprintf format directives, including character, integer, floating point, pointer, and strings, - with the standard C flags, widths, and precisions. For integers + with the standard C flags, widths, and precisions. For integers and strings it computes the length of output itself. For floating point it uses MPFR to fornmat known constants with up and down rounding and uses the resulting range of output lengths. For @@ -130,8 +130,8 @@ pass_sprintf_length::gate (function *) not optimizing and the pass is being invoked early, or when optimizing and the pass is being invoked during optimization (i.e., "late"). */ - return ((0 < warn_format_length || flag_printf_return_value) - && (0 < optimize) == fold_return_value); + return ((warn_format_length > 0 || flag_printf_return_value) + && (optimize > 0) == fold_return_value); } /* The result of a call to a formatted function. */ @@ -464,7 +464,7 @@ struct conversion_spec /* Format conversion function that given a conversion specification and an argument returns the formatting result. */ - fmtresult (*fmtfunc) (const conversion_spec &, tree); + fmtresult (*fmtfunc) (const conversion_spec &, tree); /* Return True when a the format flag CHR has been used. */ bool get_flag (char chr) const @@ -1032,10 +1032,10 @@ format_integer (const conversion_spec &s { /* The argument here may be the result of promoting the actual argument to int. Try to determine the type of the actual - argument before promotion and narrow down its range that + argument before promotion and narrow down its range that way. */ gimple *def = SSA_NAME_DEF_STMT (arg); - if (gimple_code (def) == GIMPLE_ASSIGN) + if (is_gimple_assign (def)) { tree_code code = gimple_assign_rhs_code (def); if (code == NOP_EXPR) @@ -1188,7 +1188,7 @@ format_floating (const conversion_spec & case 'a': { /* The minimum output is "0x.p+0". */ - res.range.min = 6 + (0 < prec ? prec : 0); + res.range.min = 6 + (prec > 0 ? prec : 0); /* Compute the maximum just once. */ static const int a_max[] = { @@ -1249,7 +1249,7 @@ format_floating (const conversion_spec & gcc_unreachable (); } - if (0 < width) + if (width > 0) { if (res.range.min < (unsigned)width) res.range.min = width; @@ -1440,7 +1440,7 @@ get_string_length (tree str) static fmtresult format_string (const conversion_spec &spec, tree arg) { - unsigned width = spec.have_width && 0 < spec.width ? spec.width : 0; + unsigned width = spec.have_width && spec.width > 0 ? spec.width : 0; int prec = spec.have_precision ? spec.precision : -1; if (spec.star_width) @@ -1756,7 +1756,7 @@ format_directive (const pass_sprintf_len } else { - if (!res->warned && 0 < fmtres.range.min && navail < fmtres.range.min) + if (!res->warned && fmtres.range.min > 0 && navail < fmtres.range.min) { const char* fmtstr = (info.bounded @@ -2332,7 +2332,7 @@ get_destination_size (tree dest) a member array as opposed to the whole enclosing object), otherwise use type-zero object size to determine the size of the enclosing object (the function fails without optimization in this type). */ - int ost = 0 < optimize; + int ost = optimize > 0; unsigned HOST_WIDE_INT size; if (compute_builtin_object_size (dest, ost, &size)) return size; @@ -2449,18 +2449,10 @@ pass_sprintf_length::handle_gimple_call call_info info = call_info (); info.callstmt = gsi_stmt (gsi); - info.func = gimple_call_fn (info.callstmt); - if (!info.func) - return; - - if (TREE_CODE (info.func) == ADDR_EXPR) - info.func = TREE_OPERAND (info.func, 0); - - if (TREE_CODE (info.func) != FUNCTION_DECL - || !DECL_BUILT_IN(info.func) - || DECL_BUILT_IN_CLASS (info.func) != BUILT_IN_NORMAL) + if (!gimple_call_builtin_p (info.callstmt, BUILT_IN_NORMAL)) return; + info.func = gimple_call_fndecl (info.callstmt); info.fncode = DECL_FUNCTION_CODE (info.func); /* The size of the destination as in snprintf(dest, size, ...). */ @@ -2487,6 +2479,14 @@ pass_sprintf_length::handle_gimple_call info.argidx = 2; break; + case BUILT_IN_SPRINTF_CHK: + // Signature: + // __builtin___sprintf_chk (dst, ost, objsize, format, ...) + idx_objsize = 2; + idx_format = 3; + info.argidx = 4; + break; + case BUILT_IN_SNPRINTF: // Signature: // __builtin_snprintf (dst, size, format, ...) @@ -2498,7 +2498,7 @@ pass_sprintf_length::handle_gimple_call case BUILT_IN_SNPRINTF_CHK: // Signature: - // __builtin___sprintf_chk (dst, size, ost, objsize, format, ...) + // __builtin___snprintf_chk (dst, size, ost, objsize, format, ...) idx_dstsize = 1; idx_objsize = 3; idx_format = 4; @@ -2506,14 +2506,6 @@ pass_sprintf_length::handle_gimple_call info.bounded = true; break; - case BUILT_IN_SPRINTF_CHK: - // Signature: - // __builtin___sprintf_chk (dst, ost, objsize, format, ...) - idx_objsize = 2; - idx_format = 3; - info.argidx = 4; - break; - case BUILT_IN_VSNPRINTF: // Signature: // __builtin_vsprintf (dst, size, format, va) @@ -2556,7 +2548,7 @@ pass_sprintf_length::handle_gimple_call if (idx_dstsize == HOST_WIDE_INT_M1U) { - // For non-bounded functions like sprintf, to to determine + // For non-bounded functions like sprintf, to determine // the size of the destination from the object or pointer // passed to it as the first argument. dstsize = get_destination_size (gimple_call_arg (info.callstmt, 0)); @@ -2648,7 +2640,8 @@ pass_sprintf_length::handle_gimple_call attempt to substitute the computed result for the return value of the call. Avoid this optimization when -frounding-math is in effect and the format string contains a floating point directive. */ - if (0 < optimize && flag_printf_return_value + if (optimize > 0 + && flag_printf_return_value && (!flag_rounding_math || !res.floating)) try_substitute_return_value (gsi, info, res); } @@ -2667,7 +2660,7 @@ pass_sprintf_length::execute (function * /* Iterate over statements, looking for function calls. */ gimple *stmt = gsi_stmt (si); - if (gimple_code (stmt) == GIMPLE_CALL) + if (is_gimple_call (stmt)) handle_gimple_call (si); } } Jakub