[Bug tree-optimization/78696] [7 Regression] -fprintf-return-value misoptimizes %.Ng where N is greater than 10
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=78696 Martin Sebor changed: What|Removed |Added Status|ASSIGNED|RESOLVED Resolution|--- |FIXED --- Comment #19 from Martin Sebor --- Patch committed in r244037.
[Bug tree-optimization/78696] [7 Regression] -fprintf-return-value misoptimizes %.Ng where N is greater than 10
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=78696 --- Comment #18 from Martin Sebor --- Author: msebor Date: Tue Jan 3 23:14:44 2017 New Revision: 244037 URL: https://gcc.gnu.org/viewcvs?rev=244037=gcc=rev Log: PR tree-optimization/78696 - [7 Regression] -fprintf-return-value misoptimizes %.Ng where N is greater than 10 gcc/ChangeLog: PR tree-optimization/78696 * gimple-ssa-sprintf.c (format_floating): Correct handling of precision. Use MPFR for %f for greater fidelity. Correct handling of %g. (pass_sprintf_length::compute_format_length): Set width and precision specified by asrerisk to void_node for vararg functions. (try_substitute_return_value): Adjust dump output. gcc/testsuite/ChangeLog: PR tree-optimization/78696 * gcc.dg/tree-ssa/builtin-sprintf-5.c: Remove incorrect test cases. * gcc.dg/tree-ssa/builtin-sprintf-warn-7.c: Correct off-by-1 errors. * gcc.dg/tree-ssa/builtin-sprintf-warn-9.c: New test. * gcc.dg/tree-ssa/builtin-sprintf.c: Add test cases. Added: trunk/gcc/testsuite/gcc.dg/tree-ssa/builtin-sprintf-warn-9.c Modified: trunk/gcc/ChangeLog trunk/gcc/gimple-ssa-sprintf.c trunk/gcc/testsuite/ChangeLog trunk/gcc/testsuite/gcc.dg/tree-ssa/builtin-sprintf-5.c trunk/gcc/testsuite/gcc.dg/tree-ssa/builtin-sprintf-warn-7.c trunk/gcc/testsuite/gcc.dg/tree-ssa/builtin-sprintf.c
[Bug tree-optimization/78696] [7 Regression] -fprintf-return-value misoptimizes %.Ng where N is greater than 10
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=78696 Martin Sebor changed: What|Removed |Added Keywords||patch --- Comment #17 from Martin Sebor --- Patch submitted for review: https://gcc.gnu.org/ml/gcc-patches/2016-12/msg00905.html
[Bug tree-optimization/78696] [7 Regression] -fprintf-return-value misoptimizes %.Ng where N is greater than 10
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=78696 --- Comment #16 from Martin Sebor --- (In reply to James Greenhalgh from comment #15) > (In reply to James Greenhalgh from comment #14) > > Did you accidentally commit it as part of r243419? I don't see the changes > > marked in your ChangeLog, nor did you tag that revision as a fix for this > > bug, but the code above has found its way in to trunk, and so the failures > > I've been seeing with our code base have been fixed. > > I take that back, it is the fix in comment #6 that I see in tree, not the > expanded fix from comment #13. I think that hunk was indeed accidental (though benign). I wondered where it came from myself just last night as I was rebasing my patch on top of the top of trunk.
[Bug tree-optimization/78696] [7 Regression] -fprintf-return-value misoptimizes %.Ng where N is greater than 10
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=78696 --- Comment #15 from James Greenhalgh --- (In reply to James Greenhalgh from comment #14) > Did you accidentally commit it as part of r243419? I don't see the changes > marked in your ChangeLog, nor did you tag that revision as a fix for this > bug, but the code above has found its way in to trunk, and so the failures > I've been seeing with our code base have been fixed. I take that back, it is the fix in comment #6 that I see in tree, not the expanded fix from comment #13.
[Bug tree-optimization/78696] [7 Regression] -fprintf-return-value misoptimizes %.Ng where N is greater than 10
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=78696 --- Comment #14 from James Greenhalgh --- (In reply to Martin Sebor from comment #13) > Created attachment 40272 [details] > Lightly tested patch. > > (In reply to Martin Sebor from comment #6) > > After some more testing, although the patch I showed happens to fix the > problem in the test case, it's not actually correct. I found a few other > subtle issues in the function that need to fixed. I'm testing a more > comprehensive patch that should fix it the right way. This patch also fixes the issue I've been seeing with the code base which is being miscompiled. Did you accidentally commit it as part of r243419? I don't see the changes marked in your ChangeLog, nor did you tag that revision as a fix for this bug, but the code above has found its way in to trunk, and so the failures I've been seeing with our code base have been fixed. I've got no opinion on whether the pass should be disabled by default - I do agree with Joseph's points that this seems like it has the potential to introduce or mask some very frightening security issues if a library is non-conformant.
[Bug tree-optimization/78696] [7 Regression] -fprintf-return-value misoptimizes %.Ng where N is greater than 10
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=78696 --- Comment #13 from Martin Sebor --- Created attachment 40272 --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=40272=edit Lightly tested patch. (In reply to Martin Sebor from comment #6) After some more testing, although the patch I showed happens to fix the problem in the test case, it's not actually correct. I found a few other subtle issues in the function that need to fixed. I'm testing a more comprehensive patch that should fix it the right way.
[Bug tree-optimization/78696] [7 Regression] -fprintf-return-value misoptimizes %.Ng where N is greater than 10
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=78696 Jeffrey A. Law changed: What|Removed |Added CC||law at redhat dot com --- Comment #12 from Jeffrey A. Law --- I'm starting to lean towards disabling the optimization by default for gcc-7. It's tickling a number of issues at this stage and I'm starting to worry there may be many more out there.
[Bug tree-optimization/78696] [7 Regression] -fprintf-return-value misoptimizes %.Ng where N is greater than 10
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=78696 --- Comment #11 from Jakub Jelinek --- (In reply to jos...@codesourcery.com from comment #9) > Implementation-specific can in practice include cases where the > implementation deviates from the standard (e.g. Windows 3-digit exponents, > though disabling the optimization for floating-point formats should hide > that). It's clearly not a good idea to convert a minor conformance bug in > the library into a buffer overrun by causing a program to allocate enough > memory for the standard output from a format but not enough for what the > library implementation actually outputs. > > (It wouldn't surprise me if there are printf implementations whose results > for some floating-point values are not one of the nearest decimals in each > direction and so could take more characters than either of those take in > some cases.) Yeah, that is what I don't like on -fprintf-return-value either. Warning if in certain cases the GCC assumption on what the library will actually do is not 100% matching the library implementation is one thing, but the optimization (which I'm not sure is really that beneficial in most cases) is just too dangerous, perhaps with the exception of the safest cases (no % characters, or just %s with known non-NULL strings, or perhaps some easy integral cases). There are many library implementations, and not all of them match the spec 100%, and the current GCC implementation of the optimization doesn't match it 100% either.
[Bug tree-optimization/78696] [7 Regression] -fprintf-return-value misoptimizes %.Ng where N is greater than 10
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=78696
Jakub Jelinek changed:
What|Removed |Added
CC||jakub at gcc dot gnu.org
--- Comment #10 from Jakub Jelinek ---
(In reply to Martin Sebor from comment #6)
> The bug behind the wrong output in comment #0 is in the format_floating
> function with an unknown argument failing to use the precision. The
> following simple patch fixes that.
>
> @@ -1261,9 +1277,9 @@ format_floating (const conversion_spec , int
> res.range.min = 2 + (prec < 0 ? 6 : prec);
>
> /* Compute the maximum just once. */
> - static const int f_max[] = {
> - format_floating_max (double_type_node, 'f'),
> - format_floating_max (long_double_type_node, 'f')
> + const int f_max[] = {
> + format_floating_max (double_type_node, 'f', prec),
> + format_floating_max (long_double_type_node, 'f', prec)
> };
> res.range.max = width == INT_MIN ? HOST_WIDE_INT_MAX : f_max [ldbl];
I'm happy you've removed the static, because it performs a runtime construction
of the array. But: 1) the comment is now wrong 2) it is really weird and
inefficient way of writing:
if (width == INT_MIN)
res.range.max = HOST_WIDE_INT_MAX;
else
res.range.max = format_floating_max (ldbl ? long_double_type_node
: double_type_node, 'f', prec);
> @@ -1279,9 +1295,9 @@ format_floating (const conversion_spec , int
> res.range.min = 2 + (prec < 0 ? 6 : prec);
>
> /* Compute the maximum just once. */
> - static const int g_max[] = {
> - format_floating_max (double_type_node, 'g'),
> - format_floating_max (long_double_type_node, 'g')
> + const int g_max[] = {
> + format_floating_max (double_type_node, 'g', prec),
> + format_floating_max (long_double_type_node, 'g', prec)
> };
> res.range.max = width == INT_MIN ? HOST_WIDE_INT_MAX : g_max [ldbl];
Similarly.
[Bug tree-optimization/78696] [7 Regression] -fprintf-return-value misoptimizes %.Ng where N is greater than 10
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=78696 --- Comment #9 from joseph at codesourcery dot com --- Implementation-specific can in practice include cases where the implementation deviates from the standard (e.g. Windows 3-digit exponents, though disabling the optimization for floating-point formats should hide that). It's clearly not a good idea to convert a minor conformance bug in the library into a buffer overrun by causing a program to allocate enough memory for the standard output from a format but not enough for what the library implementation actually outputs. (It wouldn't surprise me if there are printf implementations whose results for some floating-point values are not one of the nearest decimals in each direction and so could take more characters than either of those take in some cases.)
[Bug tree-optimization/78696] [7 Regression] -fprintf-return-value misoptimizes %.Ng where N is greater than 10
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=78696 Martin Sebor changed: What|Removed |Added See Also||https://gcc.gnu.org/bugzill ||a/show_bug.cgi?id=78703 --- Comment #8 from Martin Sebor --- (In reply to jos...@codesourcery.com from comment #5) Thanks for the pointer to 7.1.1#2. I wasn't aware of that either. With the removal of the %p handling the pass doesn't make any assumptions about implementation-specific behavior. I agree that in light of the locale-specific behavior the presence of floating point directives in the format string that emit a decimal point needs to disable the return value optimization. I've raised bug 78703 for that. I'll submit a separate patch for bug.
[Bug tree-optimization/78696] [7 Regression] -fprintf-return-value misoptimizes %.Ng where N is greater than 10
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=78696 --- Comment #7 from James Greenhalgh --- That fixes my miscompilation, and the miscompilation of the library I reduced the testcase from. Thanks.
[Bug tree-optimization/78696] [7 Regression] -fprintf-return-value misoptimizes %.Ng where N is greater than 10
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=78696
--- Comment #6 from Martin Sebor ---
The bug behind the wrong output in comment #0 is in the format_floating
function with an unknown argument failing to use the precision. The following
simple patch fixes that.
@@ -1261,9 +1277,9 @@ format_floating (const conversion_spec , int
res.range.min = 2 + (prec < 0 ? 6 : prec);
/* Compute the maximum just once. */
- static const int f_max[] = {
- format_floating_max (double_type_node, 'f'),
- format_floating_max (long_double_type_node, 'f')
+ const int f_max[] = {
+ format_floating_max (double_type_node, 'f', prec),
+ format_floating_max (long_double_type_node, 'f', prec)
};
res.range.max = width == INT_MIN ? HOST_WIDE_INT_MAX : f_max [ldbl];
@@ -1279,9 +1295,9 @@ format_floating (const conversion_spec , int
res.range.min = 2 + (prec < 0 ? 6 : prec);
/* Compute the maximum just once. */
- static const int g_max[] = {
- format_floating_max (double_type_node, 'g'),
- format_floating_max (long_double_type_node, 'g')
+ const int g_max[] = {
+ format_floating_max (double_type_node, 'g', prec),
+ format_floating_max (long_double_type_node, 'g', prec)
};
res.range.max = width == INT_MIN ? HOST_WIDE_INT_MAX : g_max [ldbl];
[Bug tree-optimization/78696] [7 Regression] -fprintf-return-value misoptimizes %.Ng where N is greater than 10
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=78696 --- Comment #5 from joseph at codesourcery dot com --- The radix character is not a POSIX extension. See the C11 7.1.1#2: "The decimal-point character is the character used by functions that convert floating-point numbers to or from character sequences to denote the beginning of the fractional part of such character sequences. 180) It is represented in the text and examples by a period, but may be changed by the setlocale function." (that wording dates back to C90). It's grouping, indicated by the ' flag, that's a POSIX extension. I tend to agree that this option is not yet ready to enable by default. It also risks introducing buffer overruns on systems where printf doesn't quite follow the standard interpretation followed by the implementation of the option. (E.g. some versions of Windows printf functions always use a 3-digit exponent even when standard C would require a 2-digit exponent. Do you allow for that?)
[Bug tree-optimization/78696] [7 Regression] -fprintf-return-value misoptimizes %.Ng where N is greater than 10
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=78696 Martin Sebor changed: What|Removed |Added Status|NEW |ASSIGNED Assignee|unassigned at gcc dot gnu.org |msebor at gcc dot gnu.org --- Comment #4 from Martin Sebor --- The optimization can be disabled for format strings containing floating point directives by using -frounding-math. The locale-specific radix character is a POSIX extension to C that I missed. I agree that it means that the optimization cannot be enabled when there are floating point directives in the format string that may include it.
[Bug tree-optimization/78696] [7 Regression] -fprintf-return-value misoptimizes %.Ng where N is greater than 10
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=78696
--- Comment #3 from Jakub Jelinek ---
The g/G handling looks indeed totally broken, not just for the > 10 precisions,
even for %.9g it assumes it will be in between 11 and 13 characters, which is
of course wrong.
But even generally, for all floating point printing, it seems the code doesn't
take into account that various parts of the floating point printing are derived
from locale and might not always have the same size.
Consider:
#include
#include
int __attribute__ ((noinline))
foo (char *x, size_t y, double z)
{
return __builtin_snprintf (x, y, "%.2f", z);
}
int
main ()
{
char x[100], y[100];
setlocale (LC_ALL, "");
int ret = foo (x, 100, 1.5);
int ret2 = foo (y, 100, -1.5);
__builtin_printf ("%d\t%s\n%d\t%s\n", ret, x, ret2, y);
return 0;
}
This prints (with -fno-printf-return-value) in LC_ALL=C as well as
LC_ALL=en_US.UTF-8:
4 1.50
5 -1.50
in LC_ALL=cs_CZ.UTF-8:
4 1,50
5 -1,50
(note, comma used instead of dot, but still the same size), but with
ps_AF.UTF-8 it prints:
5 1٫50
6 -1٫50
because the Pashto decimal point character is U+066B, which is 2 bytes in
UTF-8.
For floating point printing, there can be also thousands grouping etc., again
locale specific.
[Bug tree-optimization/78696] [7 Regression] -fprintf-return-value misoptimizes %.Ng where N is greater than 10
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=78696 Richard Biener changed: What|Removed |Added Version|unknown |7.0 Target Milestone|--- |7.0
[Bug tree-optimization/78696] [7 Regression] -fprintf-return-value misoptimizes %.Ng where N is greater than 10
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=78696 James Greenhalgh changed: What|Removed |Added Target|aarch64-none-linux-gnu |*-*-* --- Comment #2 from James Greenhalgh --- Not an AArch64-specific bug, this will affect everybody.
[Bug tree-optimization/78696] [7 Regression] -fprintf-return-value misoptimizes %.Ng where N is greater than 10
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=78696 Vidya Praveen changed: What|Removed |Added Target||aarch64-none-linux-gnu CC||vp at gcc dot gnu.org Severity|normal |major
[Bug tree-optimization/78696] [7 Regression] -fprintf-return-value misoptimizes %.Ng where N is greater than 10
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=78696 Markus Trippelsdorf changed: What|Removed |Added Keywords||wrong-code CC||trippels at gcc dot gnu.org --- Comment #1 from Markus Trippelsdorf --- I'm afraid that there might be many more issues of the same kind. I think -fprintf-return-value shouldn't be enabled by default.
[Bug tree-optimization/78696] [7 Regression] -fprintf-return-value misoptimizes %.Ng where N is greater than 10
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=78696 James Greenhalgh changed: What|Removed |Added Status|UNCONFIRMED |NEW Last reconfirmed||2016-12-06 CC||msebor at gcc dot gnu.org Ever confirmed|0 |1
