| Message ID | 20160921150954.GC7282@tucnak.redhat.com |
|---|---|
| State | New |
| Headers | show |
On 09/21/2016 09:09 AM, Jakub Jelinek wrote: > 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. Thanks for correcting the uses of the gimple APIs! I appreciate your fixing the various typos as well, but I see no value in changing the order of operands in inequality expressions or in moving code around for no apparent reason. However, I won't object if you feel strongly about committing those changes(*). What I would be even more grateful for is a review of the error prone parts like those that caused the bootstrap failure. I.e., any lingering assumptions about integer sizes between the host and the target, and the code that controls the return value range optimization. (Similar to what you just did for the PR77676 patch -- that was very helpful, thanks again.) I'll look into the outstanding issues you noted below next. Thanks Martin PS [*] I think it's unproductive to dwell on these details when there are much bigger problems to worry about. IME, insisting on perfect formatting or on a particular order of operands only takes time and energy away from focusing on the parts that could actually cause serious issues. > > 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 <jakub@redhat.com> > > * 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 >
On Wed, Sep 21, 2016 at 06:38:54PM -0600, Martin Sebor wrote: > On 09/21/2016 09:09 AM, Jakub Jelinek wrote: > >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. > > Thanks for correcting the uses of the gimple APIs! I appreciate > your fixing the various typos as well, but I see no value in > changing the order of operands in inequality expressions or in > moving code around for no apparent reason. However, I won't The moving of code around is in just one spot, and it has a reason - consistency. After the move, each non-_chk builtin is followed by its _chk counterpart, before that the order has been random. Another possible ordering that makes sense is putting all the non-_chk builtins first and then in the same order all their _chk counterparts. The reason why I wrote the patch has been that when skimming the code I've noticed the missing is_* calls, then when looking for that issue discovered something different etc. The var > 0 vs. 0 < var is just something that caught my eye when looking around, I don't feel too strongly about it, it just looked weird and unexpected. There are > 50 optimize > 0 preexisting checks elsewhere, and even far more just optimize, but none 0 < optimize. > What I would be even more grateful for is a review of the error > prone parts like those that caused the bootstrap failure. I.e., > any lingering assumptions about integer sizes between the host I must say I'm surprised you do all your computations in HOST_WIDE_INT, rather than say in wide_int, then you could just compare against TYPE_{MIN,MAX}_VALUE (TYPE_DOMAIN (integer_type_node)) instead of inventing a function for that. As for the warn_* vs. flag_* stuff, it just looked that the warn_* is tested in lots of functions invoked even for the return length folding, so it is much harder to prove whether it works properly or not. Jakub
On Thu, Sep 22, 2016 at 09:24:11AM +0200, Jakub Jelinek wrote: > On Wed, Sep 21, 2016 at 06:38:54PM -0600, Martin Sebor wrote: > > On 09/21/2016 09:09 AM, Jakub Jelinek wrote: > > >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. > > > > Thanks for correcting the uses of the gimple APIs! I appreciate > > your fixing the various typos as well, but I see no value in > > changing the order of operands in inequality expressions or in > > moving code around for no apparent reason. However, I won't > > The moving of code around is in just one spot, and it has a reason - > consistency. After the move, each non-_chk builtin is followed by its _chk > counterpart, before that the order has been random. > Another possible ordering that makes sense is putting all the non-_chk > builtins first and then in the same order all their _chk counterparts. > > The reason why I wrote the patch has been that when skimming the code I've > noticed the missing is_* calls, then when looking for that issue discovered > something different etc. The var > 0 vs. 0 < var is just something that > caught my eye when looking around, I don't feel too strongly about it, it > just looked weird and unexpected. There are > 50 optimize > 0 preexisting > checks elsewhere, and even far more just optimize, but none 0 < optimize. I find those 0 < var confusing and hard to read. While I know that some people prefer 0 == var (0 is not an lvalue so it catches mistakes like var = 0 instead of var == 0), I don't see why 0 < optimized would ever be preferred. Marek
>> What I would be even more grateful for is a review of the error >> prone parts like those that caused the bootstrap failure. I.e., >> any lingering assumptions about integer sizes between the host > > I must say I'm surprised you do all your computations in HOST_WIDE_INT, > rather than say in wide_int, then you could just compare against > TYPE_{MIN,MAX}_VALUE (TYPE_DOMAIN (integer_type_node)) instead of inventing > a function for that. As for the warn_* vs. flag_* stuff, it just looked > that the warn_* is tested in lots of functions invoked even for the return > length folding, so it is much harder to prove whether it works properly or > not. Well, chalk that up to my ignorance of a better/more appropriate API. I'll be happy to switch to using it if it simplifies things. Thanks for the tip! Martin
On 09/22/2016 01:46 AM, Marek Polacek wrote: > On Thu, Sep 22, 2016 at 09:24:11AM +0200, Jakub Jelinek wrote: >> On Wed, Sep 21, 2016 at 06:38:54PM -0600, Martin Sebor wrote: >>> On 09/21/2016 09:09 AM, Jakub Jelinek wrote: >>>> 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. >>> >>> Thanks for correcting the uses of the gimple APIs! I appreciate >>> your fixing the various typos as well, but I see no value in >>> changing the order of operands in inequality expressions or in >>> moving code around for no apparent reason. However, I won't >> >> The moving of code around is in just one spot, and it has a reason - >> consistency. After the move, each non-_chk builtin is followed by its _chk >> counterpart, before that the order has been random. >> Another possible ordering that makes sense is putting all the non-_chk >> builtins first and then in the same order all their _chk counterparts. >> >> The reason why I wrote the patch has been that when skimming the code I've >> noticed the missing is_* calls, then when looking for that issue discovered >> something different etc. The var > 0 vs. 0 < var is just something that >> caught my eye when looking around, I don't feel too strongly about it, it >> just looked weird and unexpected. There are > 50 optimize > 0 preexisting >> checks elsewhere, and even far more just optimize, but none 0 < optimize. > > I find those 0 < var confusing and hard to read. While I know that some > people prefer 0 == var (0 is not an lvalue so it catches mistakes like > var = 0 instead of var == 0), I don't see why 0 < optimized would ever be > preferred. I don't have a preference for one or the other. It's just how I wrote the code. Over the years of working on the C++ standard library I trained myself to use the less than expression in preference to any of the others because C++ algorithms had to (or we wanted them to, I don't remember which anymore) work with user-defined types that only defined that one relational operator and not any of the other forms. Martin
Hi Martin, I'm afraid there may be further fallout from your patch. GCC now bootstraps fine on my tester, but building Wine I started to see internal compiler error: in format_floating, at gimple-ssa-sprintf.c:1165 four or so days ago. https://gcc.gnu.org/bugzilla/show_bug.cgi?id=77740 has the invocation and a preprocessed input file. Gerald
On 09/26/2016 02:30 AM, Gerald Pfeifer wrote: > Hi Martin, > > I'm afraid there may be further fallout from your patch. GCC > now bootstraps fine on my tester, but building Wine I started > to see > > internal compiler error: in format_floating, at gimple-ssa-sprintf.c:1165 > > four or so days ago. > > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=77740 has the invocation > and a preprocessed input file. Thanks. (As I just commented in pr77740, so just to close to loop here) a patch for the problem awaits code review: https://gcc.gnu.org/ml/gcc-patches/2016-09/msg01470.html Martin
On 09/27/2016 11:29 AM, Martin Sebor wrote: > On 09/26/2016 02:30 AM, Gerald Pfeifer wrote: >> Hi Martin, >> >> I'm afraid there may be further fallout from your patch. GCC >> now bootstraps fine on my tester, but building Wine I started >> to see >> >> internal compiler error: in format_floating, at >> gimple-ssa-sprintf.c:1165 >> >> four or so days ago. >> >> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=77740 has the invocation >> and a preprocessed input file. > > Thanks. (As I just commented in pr77740, so just to close to loop > here) a patch for the problem awaits code review: > > https://gcc.gnu.org/ml/gcc-patches/2016-09/msg01470.html I should perhaps mention that since the patch is straightforward I'll go ahead and commit it as obvious unless there are objections. Martin
On 09/27/2016 11:29 AM, Martin Sebor wrote: > On 09/26/2016 02:30 AM, Gerald Pfeifer wrote: >> Hi Martin, >> >> I'm afraid there may be further fallout from your patch. GCC >> now bootstraps fine on my tester, but building Wine I started >> to see >> >> internal compiler error: in format_floating, at >> gimple-ssa-sprintf.c:1165 >> >> four or so days ago. >> >> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=77740 has the invocation >> and a preprocessed input file. > > Thanks. (As I just commented in pr77740, so just to close to loop > here) a patch for the problem awaits code review: > > https://gcc.gnu.org/ml/gcc-patches/2016-09/msg01470.html I didn't see this in my folder of things to review, so sorry it lingered. It's OK for the trunk. THanks, Jeff
--- 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); } }
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 <jakub@redhat.com> * 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. Jakub