* [PATCH] use zero as the lower bound for a signed-unsigned range (PR 79327) @ 2017-02-01 23:52 Martin Sebor 2017-02-02 7:37 ` Jakub Jelinek 0 siblings, 1 reply; 36+ messages in thread From: Martin Sebor @ 2017-02-01 23:52 UTC (permalink / raw) To: Gcc Patch List [-- Attachment #1: Type: text/plain, Size: 617 bytes --] PR tree-optimization/79327 exposes a bug in the gimple-ssa-sprintf pass in the computation of the expected size of output of an integer format directive whose argument is in a signed-unsigned range (such as [-12, 345]). The current algorithm on trunk uses the bounds of the range to compute the range of output when it should be using zero to obtain the minimum output and whichever of the two bounds results in more output to compute the maximum. This can lead to both wrong code (as pointed out in the referenced bug) to and to the wrong range printed in the warnings. The attached patch fixes that bug. Martin [-- Attachment #2: gcc-79327.diff --] [-- Type: text/x-patch, Size: 15940 bytes --] PR tree-optimization/79327 - wrong code at -O2 and -fprintf-return-value gcc/ChangeLog: PR tree-optimization/79327 * gimple-ssa-sprintf.c (format_integer): Replace with zero the lower bound of an argument in a signed-unsigned range. gcc/testsuite/ChangeLog: PR tree-optimization/79327 * gcc.dg/tree-ssa/builtin-sprintf-warn-12.c: New test. * gcc.dg/tree-ssa/pr79327.c: New test. diff --git a/gcc/gimple-ssa-sprintf.c b/gcc/gimple-ssa-sprintf.c index 11f4174..4f0670e 100644 --- a/gcc/gimple-ssa-sprintf.c +++ b/gcc/gimple-ssa-sprintf.c @@ -1382,13 +1382,26 @@ format_integer (const directive &dir, tree arg) would reflect the largest possible precision (i.e., INT_MAX). */ res.range.min = format_integer (dir, argmax).range.min; res.range.max = format_integer (dir, argmin).range.max; - } - if (res.range.max < res.range.min) - { - unsigned HOST_WIDE_INT tmp = res.range.max; - res.range.max = res.range.min; - res.range.min = tmp; + if (res.range.max < res.range.min) + { + unsigned HOST_WIDE_INT tmp = res.range.max; + res.range.max = res.range.min; + res.range.min = tmp; + } + + if (!TYPE_UNSIGNED (argtype) + && tree_int_cst_lt (integer_zero_node, argmax) + && tree_int_cst_lt (argmin, integer_zero_node)) + { + /* The minimum output for a signed argument in a negative-positive + range is that of zero. */ + unsigned HOST_WIDE_INT + nzero = format_integer (dir, integer_zero_node).range.min; + + if (nzero < res.range.min) + res.range.min = nzero; + } } res.range.likely = res.knownrange ? res.range.max : res.range.min; diff --git a/gcc/testsuite/gcc.dg/tree-ssa/builtin-sprintf-warn-12.c b/gcc/testsuite/gcc.dg/tree-ssa/builtin-sprintf-warn-12.c new file mode 100644 index 0000000..35db400 --- /dev/null +++ b/gcc/testsuite/gcc.dg/tree-ssa/builtin-sprintf-warn-12.c @@ -0,0 +1,228 @@ +/* PR tree-optimization79/327 - wrong code at -O2 and -fprintf-return-value + { dg-do compile } + { dg-options "-O2 -Wall -Wformat-overflow=1 -ftrack-macro-expansion=0" } + { dg-require-effective-target int32plus } */ + +typedef __SIZE_TYPE__ size_t; +typedef __WCHAR_TYPE__ wchar_t; + +#define INT_MAX __INT_MAX__ +#define INT_MIN (-INT_MAX - 1) + +/* When debugging, define LINE to the line number of the test case to exercise + and avoid exercising any of the others. The buffer and objsize macros + below make use of LINE to avoid warnings for other lines. */ +#ifndef LINE +# define LINE 0 +#endif + +void sink (char*, char*); + +int dummy_sprintf (char*, const char*, ...); + +char buffer [256]; +extern char *ptr; + +int int_range (int min, int max) +{ + extern int int_value (void); + int n = int_value (); + return n < min || max < n ? min : n; +} + +unsigned uint_range (unsigned min, unsigned max) +{ + extern unsigned uint_value (void); + unsigned n = uint_value (); + return n < min || max < n ? min : n; +} + +/* Evaluate to an array of SIZE characters when non-negative, or to + a pointer to an unknown object otherwise. */ +#define buffer(size) \ + ((0 <= size) ? buffer + sizeof buffer - (size) : ptr) + +/* Helper to expand function to either __builtin_f or dummy_f to + make debugging GCC easy. */ +#define FUNC(f) \ + ((!LINE || LINE == __LINE__) ? __builtin_ ## f : dummy_ ## f) + +/* Macro to verify that calls to __builtin_sprintf (i.e., with no size + argument) issue diagnostics by correctly determining the size of + the destination buffer. */ +#define T(size, ...) \ + (FUNC (sprintf) (buffer (size), __VA_ARGS__), \ + sink (buffer, ptr)) + +/* Return a signed integer in the range [MIN, MAX]. */ +#define R(min, max) int_range (min, max) + +/* Return a unsigned integer in the range [MIN, MAX]. */ +#define U(min, max) uint_range (min, max) + +/* Exercise the hh length modifier with ranges. */ +void test_hh (void) +{ + T (0, "%hhi", R ( -1, 0)); /* { dg-warning "between 1 and 2 bytes" } */ + T (0, "%hhi", R ( -1, 1)); /* { dg-warning "between 1 and 2 bytes" } */ + T (0, "%hhi", R ( -1, 12)); /* { dg-warning "between 1 and 2 bytes" } */ + T (0, "%hhi", R ( -1, 123)); /* { dg-warning "between 1 and 3 bytes" } */ + T (0, "%hhi", R ( -1, 128)); /* { dg-warning "between 1 and 4 bytes" } */ + T (0, "%hhi", R ( -1, 257)); /* { dg-warning "between 1 and 4 bytes" } */ + T (0, "%hhi", R ( -1, 1234)); /* { dg-warning "between 1 and 4 bytes" } */ + T (0, "%hhi", R ( -12, -11)); /* { dg-warning "writing 3 bytes" } */ + T (0, "%hhi", R ( -12, -1)); /* { dg-warning "between 2 and 3 bytes" } */ + T (0, "%hhi", R ( -12, 0)); /* { dg-warning "between 1 and 3 bytes" } */ + T (0, "%hhi", R ( -12, 1)); /* { dg-warning "between 1 and 3 bytes" } */ + T (0, "%hhi", R ( -12, 12)); /* { dg-warning "between 1 and 3 bytes" } */ + T (0, "%hhi", R ( -12, 123)); /* { dg-warning "between 1 and 3 bytes" } */ + T (0, "%hhi", R ( -12, 128)); /* { dg-warning "between 1 and 4 bytes" } */ + T (0, "%hhi", R ( -12, 257)); /* { dg-warning "between 1 and 4 bytes" } */ + T (0, "%hhi", R ( -12, 1234)); /* { dg-warning "between 1 and 4 bytes" } */ + T (0, "%hhi", R ( -99, -10)); /* { dg-warning "writing 3 bytes" } */ + T (0, "%hhi", R (-123, -1)); /* { dg-warning "between 2 and 4 bytes" } */ + T (0, "%hhi", R (-123, 0)); /* { dg-warning "between 1 and 4 bytes" } */ + T (0, "%hhi", R (-123, 1)); /* { dg-warning "between 1 and 4 bytes" } */ + T (0, "%hhi", R (-123, 12)); /* { dg-warning "between 1 and 4 bytes" } */ + T (0, "%hhi", R (-123, 123)); /* { dg-warning "between 1 and 4 bytes" } */ + T (0, "%hhi", R (-123, 257)); /* { dg-warning "between 1 and 4 bytes" } */ + T (0, "%hhi", R (-123, 1234)); /* { dg-warning "between 1 and 4 bytes" } */ + T (0, "%hhi", R (-129, 1)); /* { dg-warning "between 1 and 4 bytes" } */ + T (0, "%hhi", R (-130, -129)); /* { dg-warning "writing 3 bytes" } */ + + T (0, "%hhi", U ( 0, 127)); /* { dg-warning "between 1 and 3 bytes" } */ + + /* The following results in either "127" and "-128" so the ideal result + should be "between 3 and 4 bytes" but because of the overflow from + 128 to -128 in the %hhi directive the input range is reset to that + of char, or [CHAR_MIN, CHAR_MAX], and the warning reflects that. */ + T (0, "%hhi", U ( 127, 128)); /* { dg-warning "between \[13\] and 4 bytes" } */ + /* The following results in either "-128" or "-127". */ + T (0, "%hhi", U ( 128, 129)); /* { dg-warning "writing 4 bytes" } */ + /* The following results in between "-128" and "-99". */ + T (0, "%hhi", U ( 128, 157)); /* { dg-warning "writing between 3 and 4 bytes" } */ + /* Between "-128" and "-1". */ + T (0, "%hhi", U ( 128, 255)); /* { dg-warning "writing between 2 and 4 bytes" } */ + /* Between "-128" and "0". */ + T (0, "%hhi", U ( 128, 256)); /* { dg-warning "writing between 1 and 4 bytes" } */ + /* Between "-128" and "" (zero formats as nothing with zero precision). */ + T (0, "%.0hhi", U ( 128, 256)); /* { dg-warning "writing up to 4 bytes" } */ + /* Same as above but with a range of precisions including zero. */ + T (0, "%.*hhi", /* { dg-warning "writing up to 4 bytes" } */ + R (0, 1), U ( 128, 256)); + /* Same as above but with a positive range of precisions. */ + T (0, "%.*hhi", /* { dg-warning "between 1 and 4 bytes" } */ + R (1, 2), U ( 128, 256)); + /* Precision range includes zero but width is non-zero so output cannot + be empty. */ + T (0, "%1.*hhi", /* { dg-warning "between 1 and 4 bytes" } */ + R (0, 2), U ( 128, 256)); + /* Same as above but with a width range. */ + T (0, "%*.*hhi", /* { dg-warning "between 1 and 4 bytes" } */ + R (1, 2), R (0, 2), U ( 128, 256)); + /* Same as above but this time width range does include zero. */ + T (0, "%*.*hhi", /* { dg-warning "up to 4 bytes" } */ + R (0, 2), R (0, 2), U ( 128, 256)); + + /* Range of precisions in excess of the number of digits and sign. */ + T (0, "%.*hhi", /* { dg-warning "between 5 and 8 bytes" } */ + R (5, 7), U ( 128, 256)); + + /* Between "-128" and "+0". */ + T (0, "%+hhi", U ( 128, 256)); /* { dg-warning "between 2 and 4 bytes" } */ + /* Between "-128" and " 0". */ + T (0, "% hhi", U ( 128, 256)); /* { dg-warning "between 2 and 4 bytes" } */ + + T (0, "%hhu", R ( -1, 1)); /* { dg-warning "between 1 and 3 bytes" } */ + T (0, "%hhu", R ( -1, 12)); /* { dg-warning "between 1 and 3 bytes" } */ + T (0, "%hhu", R ( -1, 123)); /* { dg-warning "between 1 and 3 bytes" } */ + T (0, "%hhu", R ( -1, 128)); /* { dg-warning "between 1 and 3 bytes" } */ + T (0, "%hhu", R ( -1, 257)); /* { dg-warning "between 1 and 3 bytes" } */ + T (0, "%hhu", R ( -1, 1234)); /* { dg-warning "between 1 and 3 bytes" } */ + T (0, "%hhu", R ( -12, 1)); /* { dg-warning "between 1 and 3 bytes" } */ + T (0, "%hhu", R ( -12, 12)); /* { dg-warning "between 1 and 3 bytes" } */ + T (0, "%hhu", R ( -12, 123)); /* { dg-warning "between 1 and 3 bytes" } */ + T (0, "%hhu", R ( -12, 128)); /* { dg-warning "between 1 and 3 bytes" } */ + T (0, "%hhu", R ( -12, 257)); /* { dg-warning "between 1 and 3 bytes" } */ + T (0, "%hhu", R ( -12, 1234)); /* { dg-warning "between 1 and 3 bytes" } */ + T (0, "%hhu", R (-123, 1)); /* { dg-warning "between 1 and 3 bytes" } */ + T (0, "%hhu", R (-123, 12)); /* { dg-warning "between 1 and 3 bytes" } */ + T (0, "%hhu", R (-123, 123)); /* { dg-warning "between 1 and 3 bytes" } */ + T (0, "%hhu", R (-123, 257)); /* { dg-warning "between 1 and 3 bytes" } */ + T (0, "%hhu", R (-123, 1234)); /* { dg-warning "between 1 and 3 bytes" } */ + T (0, "%hhu", R (-129, 1)); /* { dg-warning "between 1 and 3 bytes" } */ + T (0, "%hhu", R (-199, -159)); /* { dg-warning "writing 2 bytes" } */ + T (0, "%hhu", R (-255, -250)); /* { dg-warning "writing 1 byte" } */ +} + +/* Exercise the h length modifier. */ +void test_h (void) +{ + T (0, "%hi", R ( -1, 0)); /* { dg-warning "between 1 and 2 bytes" } */ + T (0, "%hi", R ( 0, 1)); /* { dg-warning "writing 1 byte" } */ + T (0, "%hi", R ( -1, 1)); /* { dg-warning "between 1 and 2 bytes" } */ + T (0, "%hi", R ( -1, 12)); /* { dg-warning "between 1 and 2 bytes" } */ + T (0, "%hi", R ( -12, 1)); /* { dg-warning "between 1 and 3 bytes" } */ + T (0, "%hi", R ( -99, -10)); /* { dg-warning "writing 3 bytes" } */ + T (0, "%hi", R ( -123, 4)); /* { dg-warning "between 1 and 4 bytes" } */ + T (0, "%hi", R ( -1234, 56)); /* { dg-warning "between 1 and 5 bytes" } */ + T (0, "%hi", R ( -1234, 567)); /* { dg-warning "between 1 and 5 bytes" } */ + T (0, "%hi", R ( -1234, 5678)); /* { dg-warning "between 1 and 5 bytes" } */ + T (0, "%ho", R (-32768,-32767)); /* { dg-warning "writing 6 bytes" } */ + + T (0, "%ho", R ( -1, 0)); /* { dg-warning "between 1 and 6 bytes" } */ + T (0, "%ho", R ( 0, 1)); /* { dg-warning "writing 1 byte" } */ + T (0, "%ho", R ( -1, 1)); /* { dg-warning "between 1 and 6 bytes" } */ + T (0, "%ho", R ( -1, 12)); /* { dg-warning "between 1 and 6 bytes" } */ + T (0, "%ho", R ( -12, 1)); /* { dg-warning "between 1 and 6 bytes" } */ + T (0, "%ho", R ( -123, 4)); /* { dg-warning "between 1 and 6 bytes" } */ + T (0, "%ho", R ( -1234, 56)); /* { dg-warning "between 1 and 6 bytes" } */ + T (0, "%ho", R ( -1234, 567)); /* { dg-warning "between 1 and 6 bytes" } */ + T (0, "%ho", R ( -1234, 5678)); /* { dg-warning "between 1 and 6 bytes" } */ + T (0, "%ho", R (-32768,-32767)); /* { dg-warning "writing 6 bytes" } */ + + T (0, "%hu", R ( -1, 0)); /* { dg-warning "between 1 and 5 bytes" } */ + T (0, "%hu", R ( 0, 1)); /* { dg-warning "writing 1 byte" } */ + T (0, "%hu", R ( -1, 1)); /* { dg-warning "between 1 and 5 bytes" } */ + T (0, "%hu", R ( -1, 12)); /* { dg-warning "between 1 and 5 bytes" } */ + T (0, "%hu", R ( -12, 1)); /* { dg-warning "between 1 and 5 bytes" } */ + T (0, "%hu", R ( -123, 4)); /* { dg-warning "between 1 and 5 bytes" } */ + T (0, "%hu", R ( -1234, 56)); /* { dg-warning "between 1 and 5 bytes" } */ + T (0, "%hu", R ( -1234, 567)); /* { dg-warning "between 1 and 5 bytes" } */ + T (0, "%hu", R ( -1234, 5678)); /* { dg-warning "between 1 and 5 bytes" } */ + T (0, "%hu", R (-32768,-32767)); /* { dg-warning "writing 5 bytes" } */ + + T (0, "%hx", R (-32768,-32767)); /* { dg-warning "writing 4 bytes" } */ +} + +/* Exercise integer directives with no length modifier. */ +void test_diou (void) +{ + T (0, "%d", R ( -1, 0)); /* { dg-warning "between 1 and 2 bytes" } */ + T (0, "%i", R ( 0, 1)); /* { dg-warning "writing 1 byte" } */ + T (0, "%d", R ( -1, 1)); /* { dg-warning "between 1 and 2 bytes" } */ + T (0, "%i", R ( -1, 12)); /* { dg-warning "between 1 and 2 bytes" } */ + T (0, "%d", R ( -12, 1)); /* { dg-warning "between 1 and 3 bytes" } */ + T (0, "%i", R ( -123, 4)); /* { dg-warning "between 1 and 4 bytes" } */ + T (0, "%d", R ( -1234, 56)); /* { dg-warning "between 1 and 5 bytes" } */ + T (0, "%i", R ( -1234, 567)); /* { dg-warning "between 1 and 5 bytes" } */ + T (0, "%d", R ( -1234, 5678)); /* { dg-warning "between 1 and 5 bytes" } */ + + T (0, "%u", R ( -1, 0)); /* { dg-warning "between 1 and 10 bytes" } */ + T (0, "%u", R ( 0, 1)); /* { dg-warning "writing 1 byte" } */ + T (0, "%u", R ( -1, 1)); /* { dg-warning "between 1 and 10 bytes" } */ + T (0, "%u", R ( -1, 12)); /* { dg-warning "between 1 and 10 bytes" } */ + T (0, "%u", R ( -12, 1)); /* { dg-warning "between 1 and 10 bytes" } */ + T (0, "%u", R ( -123, 4)); /* { dg-warning "between 1 and 10 bytes" } */ + T (0, "%u", R ( -1234, 56)); /* { dg-warning "between 1 and 10 bytes" } */ + T (0, "%u", R ( -1234, 567)); /* { dg-warning "between 1 and 10 bytes" } */ + T (0, "%u", R ( -1234, 5678)); /* { dg-warning "between 1 and 10 bytes" } */ + + T (0, "%o", R ( -1, 0)); /* { dg-warning "between 1 and 11 bytes" } */ + T (0, "%o", R ( -2, 1)); /* { dg-warning "between 1 and 11 bytes" } */ + T (0, "%o", R ( 0, 1)); /* { dg-warning "writing 1 byte" } */ + + T (0, "%x", R ( -1, 0)); /* { dg-warning "between 1 and 8 bytes" } */ + T (0, "%x", R ( -2, 1)); /* { dg-warning "between 1 and 8 bytes" } */ + T (0, "%x", R ( 0, 1)); /* { dg-warning "writing 1 byte" } */ +} diff --git a/gcc/testsuite/gcc.dg/tree-ssa/pr79327.c b/gcc/testsuite/gcc.dg/tree-ssa/pr79327.c new file mode 100644 index 0000000..8337523 --- /dev/null +++ b/gcc/testsuite/gcc.dg/tree-ssa/pr79327.c @@ -0,0 +1,31 @@ +/* PR tree-optimization/79327 - wrong code at -O2 and -fprintf-return-value + { dg-do run } + { dg-options "-O2 -Wall" } */ + +volatile int a, b = -1; +char buf[64]; + +#define FMT "%+03d%02d" +const char* fmt = FMT; + +int main () +{ + int c = a; + int d = b; + if (c >= -35791395 && c < 35791394 && d >= -1 && d < __INT_MAX__) + { + /* In the following the range of return values can be computed + by GCC. */ + int n1 = __builtin_sprintf (buf, FMT, c + 1, d + 1); + if (n1 > 7) + __builtin_abort (); + + /* Here GCC can't see the format string so the return value + must be computed by a libc call. */ + int n2 = __builtin_sprintf (buf, fmt, c + 1, d + 1); + + if (n1 != n2) + __builtin_abort (); + } + return 0; +} ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH] use zero as the lower bound for a signed-unsigned range (PR 79327) 2017-02-01 23:52 [PATCH] use zero as the lower bound for a signed-unsigned range (PR 79327) Martin Sebor @ 2017-02-02 7:37 ` Jakub Jelinek 2017-02-02 9:52 ` Jakub Jelinek 0 siblings, 1 reply; 36+ messages in thread From: Jakub Jelinek @ 2017-02-02 7:37 UTC (permalink / raw) To: Martin Sebor; +Cc: Gcc Patch List On Wed, Feb 01, 2017 at 04:52:35PM -0700, Martin Sebor wrote: > --- a/gcc/gimple-ssa-sprintf.c > +++ b/gcc/gimple-ssa-sprintf.c > @@ -1382,13 +1382,26 @@ format_integer (const directive &dir, tree arg) > would reflect the largest possible precision (i.e., INT_MAX). */ > res.range.min = format_integer (dir, argmax).range.min; > res.range.max = format_integer (dir, argmin).range.max; > - } > > - if (res.range.max < res.range.min) > - { > - unsigned HOST_WIDE_INT tmp = res.range.max; > - res.range.max = res.range.min; > - res.range.min = tmp; > + if (res.range.max < res.range.min) > + { > + unsigned HOST_WIDE_INT tmp = res.range.max; > + res.range.max = res.range.min; > + res.range.min = tmp; These 3 lines are std::swap (res.range.max, res.range.min); also note the spaces instead of tabs and indentation by 7/9 spaces instead of 1 tab and 1 tab + 2 spaces. > + } > + > + if (!TYPE_UNSIGNED (argtype) > + && tree_int_cst_lt (integer_zero_node, argmax) > + && tree_int_cst_lt (argmin, integer_zero_node)) And these 2 lines && tree_int_cst_sgn (argmax) > 0 && tree_int_cst_sgn (argmin) < 0 > + { > + /* The minimum output for a signed argument in a negative-positive > + range is that of zero. */ > + unsigned HOST_WIDE_INT > + nzero = format_integer (dir, integer_zero_node).range.min; > + > + if (nzero < res.range.min) > + res.range.min = nzero; > + } > } > > res.range.likely = res.knownrange ? res.range.max : res.range.min; > --- /dev/null > +++ b/gcc/testsuite/gcc.dg/tree-ssa/pr79327.c > @@ -0,0 +1,31 @@ > +/* PR tree-optimization/79327 - wrong code at -O2 and -fprintf-return-value > + { dg-do run } > + { dg-options "-O2 -Wall" } */ > + > +volatile int a, b = -1; > +char buf[64]; > + > +#define FMT "%+03d%02d" > +const char* fmt = FMT; Perhaps make it even const char *volatile fmt = FMT; to make sure we don't see through it even with -fwhole-program or LTO? Don't you need -Wno-format-security or is that not on in -Wall? > + > +int main () > +{ > + int c = a; > + int d = b; > + if (c >= -35791395 && c < 35791394 && d >= -1 && d < __INT_MAX__) > + { > + /* In the following the range of return values can be computed > + by GCC. */ > + int n1 = __builtin_sprintf (buf, FMT, c + 1, d + 1); > + if (n1 > 7) > + __builtin_abort (); > + > + /* Here GCC can't see the format string so the return value > + must be computed by a libc call. */ > + int n2 = __builtin_sprintf (buf, fmt, c + 1, d + 1); > + > + if (n1 != n2) > + __builtin_abort (); > + } > + return 0; > +} Jakub ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH] use zero as the lower bound for a signed-unsigned range (PR 79327) 2017-02-02 7:37 ` Jakub Jelinek @ 2017-02-02 9:52 ` Jakub Jelinek 2017-02-02 14:12 ` Jakub Jelinek 2017-02-02 16:58 ` Martin Sebor 0 siblings, 2 replies; 36+ messages in thread From: Jakub Jelinek @ 2017-02-02 9:52 UTC (permalink / raw) To: Martin Sebor, Jeff Law; +Cc: Gcc Patch List On Thu, Feb 02, 2017 at 08:37:07AM +0100, Jakub Jelinek wrote: > > + if (res.range.max < res.range.min) > > + { > > + unsigned HOST_WIDE_INT tmp = res.range.max; > > + res.range.max = res.range.min; > > + res.range.min = tmp; > > These 3 lines are > std::swap (res.range.max, res.range.min); > also note the spaces instead of tabs and indentation by 7/9 spaces > instead of 1 tab and 1 tab + 2 spaces. > > > + } > > + > > + if (!TYPE_UNSIGNED (argtype) > > + && tree_int_cst_lt (integer_zero_node, argmax) > > + && tree_int_cst_lt (argmin, integer_zero_node)) > > And these 2 lines > && tree_int_cst_sgn (argmax) > 0 > && tree_int_cst_sgn (argmin) < 0 That said, as I've said in the PR and earlier in December on gcc-patches, the way format_integer is structured is not really maintainable, has very different code paths for arguments with known ranges and without them intermixed together and I still ran into wrong-code issues or wrong warnings with your patch. See below. Thus I'd like to propose to just very much simplify the code: gimple-ssa-sprintf.c | 108 ++++++++++++--------------------------------------- 1 file changed, 27 insertions(+), 81 deletions(-) So far I haven't done full bootstrap/regtest on this, just make check-gcc -j16 -k RUNTESTFLAGS=tree-ssa.exp which revealed the need for gcc.dg/tree-ssa/builtin-sprintf-warn-1.c hunk below. Here it is turned into a short wrong-code without the patch below: volatile int a; int main (void) { int i; char buf[64]; if (__builtin_sprintf (buf, "%#hho", a) > 1) __builtin_abort (); if (__builtin_sprintf (buf, "%#hhx", a) > 1) __builtin_abort (); return 0; } Current trunk as well as trunk + your patch computes ranges [2, 4] and [3, 4], but the correct ranges are [1, 4] and [1, 4], because for a 0 (or say -131072 etc.) it sets buf to "0" in both cases. Otherwise all the tests succeeded, though looking e.g. at the diff between builtin-sprintf-warn-1.c diagnostics with your patch and with the patch below instead, there are also differences like: -builtin-sprintf-warn-1.c:1119:3: note: using the range [1, -128] for directive argument +builtin-sprintf-warn-1.c:1119:3: note: using the range [-128, 127] for directive argument where [1, -128] looks clearly wrong, that isn't a valid range, and this test is -O0 anyway, plus a doesn't have a known range: T (0, "%hhd", a); /* { dg-warning ".%hhd. directive writing between 1 and . bytes into a region of size 0" } */ (so to some extent even the note with the patch isn't accurate, the printed value range is the range of the value after an implicit conversion from int to signed char). Or -builtin-sprintf-warn-1.c:1122:3: note: using the range [1, 255] for directive argument +builtin-sprintf-warn-1.c:1122:3: note: using the range [0, 255] for directive argument T (0, "%hhu", a); /* { dg-warning "into a region" } */ similarly, a has [INT_MIN, INT_MAX] range, but after implicit conversion to unsigned char it is [0, 255], certainly not [1, 255]. Plus there are certain notes removed: -builtin-sprintf-warn-1.c:1311:3: note: using the range [1, -2147483648] for directive argument T (0, "%d", a); /* { dg-warning "into a region" } */ without replacement, here a has [-2147483648, 2147483647] range, but as that is equal to the type's range, I bet it omits it. 2017-02-02 Jakub Jelinek <jakub@redhat.com> Martin Sebor <msebor@redhat.com> PR tree-optimization/79327 * gimple-ssa-sprintf.c (adjust_range_for_overflow): If returning true, always set *argmin and *argmax to TYPE_{MIN,MAX}_VALUE of dirtype. (format_integer): Use wide_int_to_tree instead of build_int_cst + to_?hwi. If argmin is NULL, just set argmin and argmax to TYPE_{MIN,MAX}_VALUE of argtype. Simplify and fix computation of shortest and longest sequence. * gcc.dg/tree-ssa/builtin-sprintf-warn-12.c: New test. * gcc.dg/tree-ssa/pr79327.c: New test. * gcc.dg/tree-ssa/builtin-sprintf-warn-1.c (test_sprintf_chk_hh_nonconst): Don't expect 2 bogus warnings. --- gcc/gimple-ssa-sprintf.c.jj 2017-01-31 09:26:00.692873226 +0100 +++ gcc/gimple-ssa-sprintf.c 2017-02-02 10:20:06.965884495 +0100 @@ -1014,8 +1014,8 @@ get_int_range (tree arg, tree type, HOST determined by checking for the actual argument being in the range of the type of the directive. If it isn't it must be assumed to take on the full range of the directive's type. - Return true when the range has been adjusted to the full unsigned - range of DIRTYPE, or [0, DIRTYPE_MAX], and false otherwise. */ + Return true when the range has been adjusted to the full range + of DIRTYPE, and false otherwise. */ static bool adjust_range_for_overflow (tree dirtype, tree *argmin, tree *argmax) @@ -1051,20 +1051,8 @@ adjust_range_for_overflow (tree dirtype, return false; } - tree dirmin = TYPE_MIN_VALUE (dirtype); - tree dirmax = TYPE_MAX_VALUE (dirtype); - - if (TYPE_UNSIGNED (dirtype)) - { - *argmin = dirmin; - *argmax = dirmax; - } - else - { - *argmin = integer_zero_node; - *argmax = dirmin; - } - + *argmin = TYPE_MIN_VALUE (dirtype); + *argmax = TYPE_MAX_VALUE (dirtype); return true; } @@ -1260,10 +1248,8 @@ format_integer (const directive &dir, tr enum value_range_type range_type = get_range_info (arg, &min, &max); if (range_type == VR_RANGE) { - argmin = build_int_cst (argtype, wi::fits_uhwi_p (min) - ? min.to_uhwi () : min.to_shwi ()); - argmax = build_int_cst (argtype, wi::fits_uhwi_p (max) - ? max.to_uhwi () : max.to_shwi ()); + argmin = wide_int_to_tree (argtype, min); + argmax = wide_int_to_tree (argtype, max); /* Set KNOWNRANGE if the argument is in a known subrange of the directive's type (KNOWNRANGE may be reset below). */ @@ -1307,47 +1293,8 @@ format_integer (const directive &dir, tr if (!argmin) { - /* For an unknown argument (e.g., one passed to a vararg function) - or one whose value range cannot be determined, create a T_MIN - constant if the argument's type is signed and T_MAX otherwise, - and use those to compute the range of bytes that the directive - can output. When precision may be zero, use zero as the minimum - since it results in no bytes on output (unless width is specified - to be greater than 0). */ - bool zero = dir.prec[0] <= 0 && dir.prec[1] >= 0; - argmin = build_int_cst (argtype, !zero); - - int typeprec = TYPE_PRECISION (dirtype); - int argprec = TYPE_PRECISION (argtype); - - if (argprec < typeprec) - { - if (POINTER_TYPE_P (argtype)) - argmax = build_all_ones_cst (argtype); - else if (TYPE_UNSIGNED (argtype)) - argmax = TYPE_MAX_VALUE (argtype); - else - argmax = TYPE_MIN_VALUE (argtype); - } - else - { - if (POINTER_TYPE_P (dirtype)) - argmax = build_all_ones_cst (dirtype); - else if (TYPE_UNSIGNED (dirtype)) - argmax = TYPE_MAX_VALUE (dirtype); - else - argmax = TYPE_MIN_VALUE (dirtype); - } - - res.argmin = argmin; - res.argmax = argmax; - } - - if (tree_int_cst_lt (argmax, argmin)) - { - tree tmp = argmax; - argmax = argmin; - argmin = tmp; + argmin = TYPE_MIN_VALUE (argtype); + argmax = TYPE_MAX_VALUE (argtype); } /* Clear KNOWNRANGE if the range has been adjusted to the maximum @@ -1361,34 +1308,33 @@ format_integer (const directive &dir, tr res.argmax = argmax; } - /* Recursively compute the minimum and maximum from the known range, - taking care to swap them if the lower bound results in longer - output than the upper bound (e.g., in the range [-1, 0]. */ - - if (TYPE_UNSIGNED (dirtype)) - { - /* For unsigned conversions/directives, use the minimum (i.e., 0 - or 1) and maximum to compute the shortest and longest output, - respectively. */ + /* Recursively compute the minimum and maximum from the known range. */ + if (TYPE_UNSIGNED (dirtype) || tree_int_cst_sgn (argmin) >= 0) + { + /* For unsigned conversions/directives or signed when + the minimum is positive, use the minimum and maximum to compute + the shortest and longest output, respectively. */ res.range.min = format_integer (dir, argmin).range.min; res.range.max = format_integer (dir, argmax).range.max; } - else + else if (tree_int_cst_sgn (argmax) < 0) { - /* For signed conversions/directives, use the maximum (i.e., 0) - to compute the shortest output and the minimum (i.e., TYPE_MIN) - to compute the longest output. This is important when precision - is specified but unknown because otherwise both output lengths - would reflect the largest possible precision (i.e., INT_MAX). */ + /* For signed conversions/directives if maximum is negative, + use the minimum as the longest output and maximum as the + shortest output. */ res.range.min = format_integer (dir, argmax).range.min; res.range.max = format_integer (dir, argmin).range.max; } - - if (res.range.max < res.range.min) + else { - unsigned HOST_WIDE_INT tmp = res.range.max; - res.range.max = res.range.min; - res.range.min = tmp; + /* Otherwise, 0 is inside of the range and minimum negative. Use 0 + as the shortest output and for the longest output compute the + length of the output of both minimum and maximum and pick the + longer. */ + unsigned HOST_WIDE_INT max1 = format_integer (dir, argmin).range.max; + unsigned HOST_WIDE_INT max2 = format_integer (dir, argmax).range.max; + res.range.min = format_integer (dir, integer_zero_node).range.min; + res.range.max = MAX (max1, max2); } res.range.likely = res.knownrange ? res.range.max : res.range.min; --- gcc/testsuite/gcc.dg/tree-ssa/pr79327.c.jj 2017-02-02 08:56:42.278163030 +0100 +++ gcc/testsuite/gcc.dg/tree-ssa/pr79327.c 2017-02-02 08:56:42.278163030 +0100 @@ -0,0 +1,31 @@ +/* PR tree-optimization/79327 - wrong code at -O2 and -fprintf-return-value + { dg-do run } + { dg-options "-O2 -Wall" } */ + +volatile int a, b = -1; +char buf[64]; + +#define FMT "%+03d%02d" +const char* fmt = FMT; + +int main () +{ + int c = a; + int d = b; + if (c >= -35791395 && c < 35791394 && d >= -1 && d < __INT_MAX__) + { + /* In the following the range of return values can be computed + by GCC. */ + int n1 = __builtin_sprintf (buf, FMT, c + 1, d + 1); + if (n1 > 7) + __builtin_abort (); + + /* Here GCC can't see the format string so the return value + must be computed by a libc call. */ + int n2 = __builtin_sprintf (buf, fmt, c + 1, d + 1); + + if (n1 != n2) + __builtin_abort (); + } + return 0; +} --- gcc/testsuite/gcc.dg/tree-ssa/builtin-sprintf-warn-12.c.jj 2017-02-02 08:56:42.278163030 +0100 +++ gcc/testsuite/gcc.dg/tree-ssa/builtin-sprintf-warn-12.c 2017-02-02 08:56:42.278163030 +0100 @@ -0,0 +1,228 @@ +/* PR tree-optimization79/327 - wrong code at -O2 and -fprintf-return-value + { dg-do compile } + { dg-options "-O2 -Wall -Wformat-overflow=1 -ftrack-macro-expansion=0" } + { dg-require-effective-target int32plus } */ + +typedef __SIZE_TYPE__ size_t; +typedef __WCHAR_TYPE__ wchar_t; + +#define INT_MAX __INT_MAX__ +#define INT_MIN (-INT_MAX - 1) + +/* When debugging, define LINE to the line number of the test case to exercise + and avoid exercising any of the others. The buffer and objsize macros + below make use of LINE to avoid warnings for other lines. */ +#ifndef LINE +# define LINE 0 +#endif + +void sink (char*, char*); + +int dummy_sprintf (char*, const char*, ...); + +char buffer [256]; +extern char *ptr; + +int int_range (int min, int max) +{ + extern int int_value (void); + int n = int_value (); + return n < min || max < n ? min : n; +} + +unsigned uint_range (unsigned min, unsigned max) +{ + extern unsigned uint_value (void); + unsigned n = uint_value (); + return n < min || max < n ? min : n; +} + +/* Evaluate to an array of SIZE characters when non-negative, or to + a pointer to an unknown object otherwise. */ +#define buffer(size) \ + ((0 <= size) ? buffer + sizeof buffer - (size) : ptr) + +/* Helper to expand function to either __builtin_f or dummy_f to + make debugging GCC easy. */ +#define FUNC(f) \ + ((!LINE || LINE == __LINE__) ? __builtin_ ## f : dummy_ ## f) + +/* Macro to verify that calls to __builtin_sprintf (i.e., with no size + argument) issue diagnostics by correctly determining the size of + the destination buffer. */ +#define T(size, ...) \ + (FUNC (sprintf) (buffer (size), __VA_ARGS__), \ + sink (buffer, ptr)) + +/* Return a signed integer in the range [MIN, MAX]. */ +#define R(min, max) int_range (min, max) + +/* Return a unsigned integer in the range [MIN, MAX]. */ +#define U(min, max) uint_range (min, max) + +/* Exercise the hh length modifier with ranges. */ +void test_hh (void) +{ + T (0, "%hhi", R ( -1, 0)); /* { dg-warning "between 1 and 2 bytes" } */ + T (0, "%hhi", R ( -1, 1)); /* { dg-warning "between 1 and 2 bytes" } */ + T (0, "%hhi", R ( -1, 12)); /* { dg-warning "between 1 and 2 bytes" } */ + T (0, "%hhi", R ( -1, 123)); /* { dg-warning "between 1 and 3 bytes" } */ + T (0, "%hhi", R ( -1, 128)); /* { dg-warning "between 1 and 4 bytes" } */ + T (0, "%hhi", R ( -1, 257)); /* { dg-warning "between 1 and 4 bytes" } */ + T (0, "%hhi", R ( -1, 1234)); /* { dg-warning "between 1 and 4 bytes" } */ + T (0, "%hhi", R ( -12, -11)); /* { dg-warning "writing 3 bytes" } */ + T (0, "%hhi", R ( -12, -1)); /* { dg-warning "between 2 and 3 bytes" } */ + T (0, "%hhi", R ( -12, 0)); /* { dg-warning "between 1 and 3 bytes" } */ + T (0, "%hhi", R ( -12, 1)); /* { dg-warning "between 1 and 3 bytes" } */ + T (0, "%hhi", R ( -12, 12)); /* { dg-warning "between 1 and 3 bytes" } */ + T (0, "%hhi", R ( -12, 123)); /* { dg-warning "between 1 and 3 bytes" } */ + T (0, "%hhi", R ( -12, 128)); /* { dg-warning "between 1 and 4 bytes" } */ + T (0, "%hhi", R ( -12, 257)); /* { dg-warning "between 1 and 4 bytes" } */ + T (0, "%hhi", R ( -12, 1234)); /* { dg-warning "between 1 and 4 bytes" } */ + T (0, "%hhi", R ( -99, -10)); /* { dg-warning "writing 3 bytes" } */ + T (0, "%hhi", R (-123, -1)); /* { dg-warning "between 2 and 4 bytes" } */ + T (0, "%hhi", R (-123, 0)); /* { dg-warning "between 1 and 4 bytes" } */ + T (0, "%hhi", R (-123, 1)); /* { dg-warning "between 1 and 4 bytes" } */ + T (0, "%hhi", R (-123, 12)); /* { dg-warning "between 1 and 4 bytes" } */ + T (0, "%hhi", R (-123, 123)); /* { dg-warning "between 1 and 4 bytes" } */ + T (0, "%hhi", R (-123, 257)); /* { dg-warning "between 1 and 4 bytes" } */ + T (0, "%hhi", R (-123, 1234)); /* { dg-warning "between 1 and 4 bytes" } */ + T (0, "%hhi", R (-129, 1)); /* { dg-warning "between 1 and 4 bytes" } */ + T (0, "%hhi", R (-130, -129)); /* { dg-warning "writing 3 bytes" } */ + + T (0, "%hhi", U ( 0, 127)); /* { dg-warning "between 1 and 3 bytes" } */ + + /* The following results in either "127" and "-128" so the ideal result + should be "between 3 and 4 bytes" but because of the overflow from + 128 to -128 in the %hhi directive the input range is reset to that + of char, or [CHAR_MIN, CHAR_MAX], and the warning reflects that. */ + T (0, "%hhi", U ( 127, 128)); /* { dg-warning "between \[13\] and 4 bytes" } */ + /* The following results in either "-128" or "-127". */ + T (0, "%hhi", U ( 128, 129)); /* { dg-warning "writing 4 bytes" } */ + /* The following results in between "-128" and "-99". */ + T (0, "%hhi", U ( 128, 157)); /* { dg-warning "writing between 3 and 4 bytes" } */ + /* Between "-128" and "-1". */ + T (0, "%hhi", U ( 128, 255)); /* { dg-warning "writing between 2 and 4 bytes" } */ + /* Between "-128" and "0". */ + T (0, "%hhi", U ( 128, 256)); /* { dg-warning "writing between 1 and 4 bytes" } */ + /* Between "-128" and "" (zero formats as nothing with zero precision). */ + T (0, "%.0hhi", U ( 128, 256)); /* { dg-warning "writing up to 4 bytes" } */ + /* Same as above but with a range of precisions including zero. */ + T (0, "%.*hhi", /* { dg-warning "writing up to 4 bytes" } */ + R (0, 1), U ( 128, 256)); + /* Same as above but with a positive range of precisions. */ + T (0, "%.*hhi", /* { dg-warning "between 1 and 4 bytes" } */ + R (1, 2), U ( 128, 256)); + /* Precision range includes zero but width is non-zero so output cannot + be empty. */ + T (0, "%1.*hhi", /* { dg-warning "between 1 and 4 bytes" } */ + R (0, 2), U ( 128, 256)); + /* Same as above but with a width range. */ + T (0, "%*.*hhi", /* { dg-warning "between 1 and 4 bytes" } */ + R (1, 2), R (0, 2), U ( 128, 256)); + /* Same as above but this time width range does include zero. */ + T (0, "%*.*hhi", /* { dg-warning "up to 4 bytes" } */ + R (0, 2), R (0, 2), U ( 128, 256)); + + /* Range of precisions in excess of the number of digits and sign. */ + T (0, "%.*hhi", /* { dg-warning "between 5 and 8 bytes" } */ + R (5, 7), U ( 128, 256)); + + /* Between "-128" and "+0". */ + T (0, "%+hhi", U ( 128, 256)); /* { dg-warning "between 2 and 4 bytes" } */ + /* Between "-128" and " 0". */ + T (0, "% hhi", U ( 128, 256)); /* { dg-warning "between 2 and 4 bytes" } */ + + T (0, "%hhu", R ( -1, 1)); /* { dg-warning "between 1 and 3 bytes" } */ + T (0, "%hhu", R ( -1, 12)); /* { dg-warning "between 1 and 3 bytes" } */ + T (0, "%hhu", R ( -1, 123)); /* { dg-warning "between 1 and 3 bytes" } */ + T (0, "%hhu", R ( -1, 128)); /* { dg-warning "between 1 and 3 bytes" } */ + T (0, "%hhu", R ( -1, 257)); /* { dg-warning "between 1 and 3 bytes" } */ + T (0, "%hhu", R ( -1, 1234)); /* { dg-warning "between 1 and 3 bytes" } */ + T (0, "%hhu", R ( -12, 1)); /* { dg-warning "between 1 and 3 bytes" } */ + T (0, "%hhu", R ( -12, 12)); /* { dg-warning "between 1 and 3 bytes" } */ + T (0, "%hhu", R ( -12, 123)); /* { dg-warning "between 1 and 3 bytes" } */ + T (0, "%hhu", R ( -12, 128)); /* { dg-warning "between 1 and 3 bytes" } */ + T (0, "%hhu", R ( -12, 257)); /* { dg-warning "between 1 and 3 bytes" } */ + T (0, "%hhu", R ( -12, 1234)); /* { dg-warning "between 1 and 3 bytes" } */ + T (0, "%hhu", R (-123, 1)); /* { dg-warning "between 1 and 3 bytes" } */ + T (0, "%hhu", R (-123, 12)); /* { dg-warning "between 1 and 3 bytes" } */ + T (0, "%hhu", R (-123, 123)); /* { dg-warning "between 1 and 3 bytes" } */ + T (0, "%hhu", R (-123, 257)); /* { dg-warning "between 1 and 3 bytes" } */ + T (0, "%hhu", R (-123, 1234)); /* { dg-warning "between 1 and 3 bytes" } */ + T (0, "%hhu", R (-129, 1)); /* { dg-warning "between 1 and 3 bytes" } */ + T (0, "%hhu", R (-199, -159)); /* { dg-warning "writing 2 bytes" } */ + T (0, "%hhu", R (-255, -250)); /* { dg-warning "writing 1 byte" } */ +} + +/* Exercise the h length modifier. */ +void test_h (void) +{ + T (0, "%hi", R ( -1, 0)); /* { dg-warning "between 1 and 2 bytes" } */ + T (0, "%hi", R ( 0, 1)); /* { dg-warning "writing 1 byte" } */ + T (0, "%hi", R ( -1, 1)); /* { dg-warning "between 1 and 2 bytes" } */ + T (0, "%hi", R ( -1, 12)); /* { dg-warning "between 1 and 2 bytes" } */ + T (0, "%hi", R ( -12, 1)); /* { dg-warning "between 1 and 3 bytes" } */ + T (0, "%hi", R ( -99, -10)); /* { dg-warning "writing 3 bytes" } */ + T (0, "%hi", R ( -123, 4)); /* { dg-warning "between 1 and 4 bytes" } */ + T (0, "%hi", R ( -1234, 56)); /* { dg-warning "between 1 and 5 bytes" } */ + T (0, "%hi", R ( -1234, 567)); /* { dg-warning "between 1 and 5 bytes" } */ + T (0, "%hi", R ( -1234, 5678)); /* { dg-warning "between 1 and 5 bytes" } */ + T (0, "%ho", R (-32768,-32767)); /* { dg-warning "writing 6 bytes" } */ + + T (0, "%ho", R ( -1, 0)); /* { dg-warning "between 1 and 6 bytes" } */ + T (0, "%ho", R ( 0, 1)); /* { dg-warning "writing 1 byte" } */ + T (0, "%ho", R ( -1, 1)); /* { dg-warning "between 1 and 6 bytes" } */ + T (0, "%ho", R ( -1, 12)); /* { dg-warning "between 1 and 6 bytes" } */ + T (0, "%ho", R ( -12, 1)); /* { dg-warning "between 1 and 6 bytes" } */ + T (0, "%ho", R ( -123, 4)); /* { dg-warning "between 1 and 6 bytes" } */ + T (0, "%ho", R ( -1234, 56)); /* { dg-warning "between 1 and 6 bytes" } */ + T (0, "%ho", R ( -1234, 567)); /* { dg-warning "between 1 and 6 bytes" } */ + T (0, "%ho", R ( -1234, 5678)); /* { dg-warning "between 1 and 6 bytes" } */ + T (0, "%ho", R (-32768,-32767)); /* { dg-warning "writing 6 bytes" } */ + + T (0, "%hu", R ( -1, 0)); /* { dg-warning "between 1 and 5 bytes" } */ + T (0, "%hu", R ( 0, 1)); /* { dg-warning "writing 1 byte" } */ + T (0, "%hu", R ( -1, 1)); /* { dg-warning "between 1 and 5 bytes" } */ + T (0, "%hu", R ( -1, 12)); /* { dg-warning "between 1 and 5 bytes" } */ + T (0, "%hu", R ( -12, 1)); /* { dg-warning "between 1 and 5 bytes" } */ + T (0, "%hu", R ( -123, 4)); /* { dg-warning "between 1 and 5 bytes" } */ + T (0, "%hu", R ( -1234, 56)); /* { dg-warning "between 1 and 5 bytes" } */ + T (0, "%hu", R ( -1234, 567)); /* { dg-warning "between 1 and 5 bytes" } */ + T (0, "%hu", R ( -1234, 5678)); /* { dg-warning "between 1 and 5 bytes" } */ + T (0, "%hu", R (-32768,-32767)); /* { dg-warning "writing 5 bytes" } */ + + T (0, "%hx", R (-32768,-32767)); /* { dg-warning "writing 4 bytes" } */ +} + +/* Exercise integer directives with no length modifier. */ +void test_diou (void) +{ + T (0, "%d", R ( -1, 0)); /* { dg-warning "between 1 and 2 bytes" } */ + T (0, "%i", R ( 0, 1)); /* { dg-warning "writing 1 byte" } */ + T (0, "%d", R ( -1, 1)); /* { dg-warning "between 1 and 2 bytes" } */ + T (0, "%i", R ( -1, 12)); /* { dg-warning "between 1 and 2 bytes" } */ + T (0, "%d", R ( -12, 1)); /* { dg-warning "between 1 and 3 bytes" } */ + T (0, "%i", R ( -123, 4)); /* { dg-warning "between 1 and 4 bytes" } */ + T (0, "%d", R ( -1234, 56)); /* { dg-warning "between 1 and 5 bytes" } */ + T (0, "%i", R ( -1234, 567)); /* { dg-warning "between 1 and 5 bytes" } */ + T (0, "%d", R ( -1234, 5678)); /* { dg-warning "between 1 and 5 bytes" } */ + + T (0, "%u", R ( -1, 0)); /* { dg-warning "between 1 and 10 bytes" } */ + T (0, "%u", R ( 0, 1)); /* { dg-warning "writing 1 byte" } */ + T (0, "%u", R ( -1, 1)); /* { dg-warning "between 1 and 10 bytes" } */ + T (0, "%u", R ( -1, 12)); /* { dg-warning "between 1 and 10 bytes" } */ + T (0, "%u", R ( -12, 1)); /* { dg-warning "between 1 and 10 bytes" } */ + T (0, "%u", R ( -123, 4)); /* { dg-warning "between 1 and 10 bytes" } */ + T (0, "%u", R ( -1234, 56)); /* { dg-warning "between 1 and 10 bytes" } */ + T (0, "%u", R ( -1234, 567)); /* { dg-warning "between 1 and 10 bytes" } */ + T (0, "%u", R ( -1234, 5678)); /* { dg-warning "between 1 and 10 bytes" } */ + + T (0, "%o", R ( -1, 0)); /* { dg-warning "between 1 and 11 bytes" } */ + T (0, "%o", R ( -2, 1)); /* { dg-warning "between 1 and 11 bytes" } */ + T (0, "%o", R ( 0, 1)); /* { dg-warning "writing 1 byte" } */ + + T (0, "%x", R ( -1, 0)); /* { dg-warning "between 1 and 8 bytes" } */ + T (0, "%x", R ( -2, 1)); /* { dg-warning "between 1 and 8 bytes" } */ + T (0, "%x", R ( 0, 1)); /* { dg-warning "writing 1 byte" } */ +} --- gcc/testsuite/gcc.dg/tree-ssa/builtin-sprintf-warn-1.c.jj 2017-01-30 09:31:45.000000000 +0100 +++ gcc/testsuite/gcc.dg/tree-ssa/builtin-sprintf-warn-1.c 2017-02-02 10:21:22.014902923 +0100 @@ -1151,8 +1151,8 @@ void test_sprintf_chk_hh_nonconst (int w T (2, "% hhu", a); /* { dg-warning ". . flag used with .%u." } */ T (2, "% hhx", a); /* { dg-warning ". . flag used with .%x." } */ - T (2, "%#hho", a); /* { dg-warning "nul past the end" } */ - T (2, "%#hhx", a); /* { dg-warning ".%#hhx. directive writing between 3 and . bytes into a region of size 2" } */ + T (2, "%#hho", a); + T (2, "%#hhx", a); T (3, "%0hhd", a); T (3, "%1hhd", a); Jakub ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH] use zero as the lower bound for a signed-unsigned range (PR 79327) 2017-02-02 9:52 ` Jakub Jelinek @ 2017-02-02 14:12 ` Jakub Jelinek 2017-02-03 18:44 ` Jeff Law 2017-02-02 16:58 ` Martin Sebor 1 sibling, 1 reply; 36+ messages in thread From: Jakub Jelinek @ 2017-02-02 14:12 UTC (permalink / raw) To: Martin Sebor, Jeff Law; +Cc: Gcc Patch List On Thu, Feb 02, 2017 at 10:52:18AM +0100, Jakub Jelinek wrote: > That said, as I've said in the PR and earlier in December on gcc-patches, > the way format_integer is structured is not really maintainable, has > very different code paths for arguments with known ranges and without them > intermixed together and I still ran into wrong-code issues or wrong warnings > with your patch. See below. Thus I'd like to propose to just very much > simplify the code: > > gimple-ssa-sprintf.c | 108 ++++++++++++--------------------------------------- > 1 file changed, 27 insertions(+), 81 deletions(-) > > So far I haven't done full bootstrap/regtest on this, just > make check-gcc -j16 -k RUNTESTFLAGS=tree-ssa.exp > which revealed the need for gcc.dg/tree-ssa/builtin-sprintf-warn-1.c hunk > below. Here it is turned into a short wrong-code without the patch below: Testing revealed a bug in my patch (POINTER_TYPE args really need special handling, restored), and one further testcase glitch, plus I've added another testcase for the other wrong-code. Going to bootstrap/regtest this again: 2017-02-02 Jakub Jelinek <jakub@redhat.com> Martin Sebor <msebor@redhat.com> PR tree-optimization/79327 * gimple-ssa-sprintf.c (adjust_range_for_overflow): If returning true, always set *argmin and *argmax to TYPE_{MIN,MAX}_VALUE of dirtype. (format_integer): Use wide_int_to_tree instead of build_int_cst + to_?hwi. If argmin is NULL, just set argmin and argmax to TYPE_{MIN,MAX}_VALUE of argtype. Simplify and fix computation of shortest and longest sequence. * gcc.dg/tree-ssa/pr79327.c: New test. * gcc.dg/tree-ssa/builtin-sprintf-warn-1.c (test_sprintf_chk_hh_nonconst): Don't expect 2 bogus warnings. * gcc.dg/tree-ssa/builtin-sprintf-warn-3.c (test_sprintf_chk_range_schar): Adjust dg-message. * gcc.dg/tree-ssa/builtin-sprintf-warn-12.c: New test. * gcc.c-torture/execute/pr79327.c: New test. --- gcc/gimple-ssa-sprintf.c.jj 2017-02-02 11:03:46.536526801 +0100 +++ gcc/gimple-ssa-sprintf.c 2017-02-02 14:53:54.657592450 +0100 @@ -1014,8 +1014,8 @@ get_int_range (tree arg, tree type, HOST determined by checking for the actual argument being in the range of the type of the directive. If it isn't it must be assumed to take on the full range of the directive's type. - Return true when the range has been adjusted to the full unsigned - range of DIRTYPE, or [0, DIRTYPE_MAX], and false otherwise. */ + Return true when the range has been adjusted to the full range + of DIRTYPE, and false otherwise. */ static bool adjust_range_for_overflow (tree dirtype, tree *argmin, tree *argmax) @@ -1051,20 +1051,8 @@ adjust_range_for_overflow (tree dirtype, return false; } - tree dirmin = TYPE_MIN_VALUE (dirtype); - tree dirmax = TYPE_MAX_VALUE (dirtype); - - if (TYPE_UNSIGNED (dirtype)) - { - *argmin = dirmin; - *argmax = dirmax; - } - else - { - *argmin = integer_zero_node; - *argmax = dirmin; - } - + *argmin = TYPE_MIN_VALUE (dirtype); + *argmax = TYPE_MAX_VALUE (dirtype); return true; } @@ -1260,10 +1248,8 @@ format_integer (const directive &dir, tr enum value_range_type range_type = get_range_info (arg, &min, &max); if (range_type == VR_RANGE) { - argmin = build_int_cst (argtype, wi::fits_uhwi_p (min) - ? min.to_uhwi () : min.to_shwi ()); - argmax = build_int_cst (argtype, wi::fits_uhwi_p (max) - ? max.to_uhwi () : max.to_shwi ()); + argmin = wide_int_to_tree (argtype, min); + argmax = wide_int_to_tree (argtype, max); /* Set KNOWNRANGE if the argument is in a known subrange of the directive's type (KNOWNRANGE may be reset below). */ @@ -1307,47 +1293,16 @@ format_integer (const directive &dir, tr if (!argmin) { - /* For an unknown argument (e.g., one passed to a vararg function) - or one whose value range cannot be determined, create a T_MIN - constant if the argument's type is signed and T_MAX otherwise, - and use those to compute the range of bytes that the directive - can output. When precision may be zero, use zero as the minimum - since it results in no bytes on output (unless width is specified - to be greater than 0). */ - bool zero = dir.prec[0] <= 0 && dir.prec[1] >= 0; - argmin = build_int_cst (argtype, !zero); - - int typeprec = TYPE_PRECISION (dirtype); - int argprec = TYPE_PRECISION (argtype); - - if (argprec < typeprec) + if (TREE_CODE (argtype) == POINTER_TYPE) { - if (POINTER_TYPE_P (argtype)) - argmax = build_all_ones_cst (argtype); - else if (TYPE_UNSIGNED (argtype)) - argmax = TYPE_MAX_VALUE (argtype); - else - argmax = TYPE_MIN_VALUE (argtype); + argmin = build_int_cst (pointer_sized_int_node, 0); + argmax = build_all_ones_cst (pointer_sized_int_node); } else { - if (POINTER_TYPE_P (dirtype)) - argmax = build_all_ones_cst (dirtype); - else if (TYPE_UNSIGNED (dirtype)) - argmax = TYPE_MAX_VALUE (dirtype); - else - argmax = TYPE_MIN_VALUE (dirtype); + argmin = TYPE_MIN_VALUE (argtype); + argmax = TYPE_MAX_VALUE (argtype); } - - res.argmin = argmin; - res.argmax = argmax; - } - - if (tree_int_cst_lt (argmax, argmin)) - { - tree tmp = argmax; - argmax = argmin; - argmin = tmp; } /* Clear KNOWNRANGE if the range has been adjusted to the maximum @@ -1361,34 +1316,33 @@ format_integer (const directive &dir, tr res.argmax = argmax; } - /* Recursively compute the minimum and maximum from the known range, - taking care to swap them if the lower bound results in longer - output than the upper bound (e.g., in the range [-1, 0]. */ - - if (TYPE_UNSIGNED (dirtype)) - { - /* For unsigned conversions/directives, use the minimum (i.e., 0 - or 1) and maximum to compute the shortest and longest output, - respectively. */ + /* Recursively compute the minimum and maximum from the known range. */ + if (TYPE_UNSIGNED (dirtype) || tree_int_cst_sgn (argmin) >= 0) + { + /* For unsigned conversions/directives or signed when + the minimum is positive, use the minimum and maximum to compute + the shortest and longest output, respectively. */ res.range.min = format_integer (dir, argmin).range.min; res.range.max = format_integer (dir, argmax).range.max; } - else + else if (tree_int_cst_sgn (argmax) < 0) { - /* For signed conversions/directives, use the maximum (i.e., 0) - to compute the shortest output and the minimum (i.e., TYPE_MIN) - to compute the longest output. This is important when precision - is specified but unknown because otherwise both output lengths - would reflect the largest possible precision (i.e., INT_MAX). */ + /* For signed conversions/directives if maximum is negative, + use the minimum as the longest output and maximum as the + shortest output. */ res.range.min = format_integer (dir, argmax).range.min; res.range.max = format_integer (dir, argmin).range.max; } - - if (res.range.max < res.range.min) + else { - unsigned HOST_WIDE_INT tmp = res.range.max; - res.range.max = res.range.min; - res.range.min = tmp; + /* Otherwise, 0 is inside of the range and minimum negative. Use 0 + as the shortest output and for the longest output compute the + length of the output of both minimum and maximum and pick the + longer. */ + unsigned HOST_WIDE_INT max1 = format_integer (dir, argmin).range.max; + unsigned HOST_WIDE_INT max2 = format_integer (dir, argmax).range.max; + res.range.min = format_integer (dir, integer_zero_node).range.min; + res.range.max = MAX (max1, max2); } res.range.likely = res.knownrange ? res.range.max : res.range.min; --- gcc/testsuite/gcc.dg/tree-ssa/pr79327.c.jj 2017-02-02 14:17:58.683661237 +0100 +++ gcc/testsuite/gcc.dg/tree-ssa/pr79327.c 2017-02-02 14:17:58.683661237 +0100 @@ -0,0 +1,31 @@ +/* PR tree-optimization/79327 - wrong code at -O2 and -fprintf-return-value + { dg-do run } + { dg-options "-O2 -Wall" } */ + +volatile int a, b = -1; +char buf[64]; + +#define FMT "%+03d%02d" +const char *volatile fmt = FMT; + +int main () +{ + int c = a; + int d = b; + if (c >= -35791395 && c < 35791394 && d >= -1 && d < __INT_MAX__) + { + /* In the following the range of return values can be computed + by GCC. */ + int n1 = __builtin_sprintf (buf, FMT, c + 1, d + 1); + if (n1 > 7) + __builtin_abort (); + + /* Here GCC can't see the format string so the return value + must be computed by a libc call. */ + int n2 = __builtin_sprintf (buf, fmt, c + 1, d + 1); + + if (n1 != n2) + __builtin_abort (); + } + return 0; +} --- gcc/testsuite/gcc.dg/tree-ssa/builtin-sprintf-warn-1.c.jj 2017-02-02 11:03:47.040520177 +0100 +++ gcc/testsuite/gcc.dg/tree-ssa/builtin-sprintf-warn-1.c 2017-02-02 14:17:58.704660965 +0100 @@ -1151,8 +1151,8 @@ void test_sprintf_chk_hh_nonconst (int w T (2, "% hhu", a); /* { dg-warning ". . flag used with .%u." } */ T (2, "% hhx", a); /* { dg-warning ". . flag used with .%x." } */ - T (2, "%#hho", a); /* { dg-warning "nul past the end" } */ - T (2, "%#hhx", a); /* { dg-warning ".%#hhx. directive writing between 3 and . bytes into a region of size 2" } */ + T (2, "%#hho", a); + T (2, "%#hhx", a); T (3, "%0hhd", a); T (3, "%1hhd", a); --- gcc/testsuite/gcc.dg/tree-ssa/builtin-sprintf-warn-3.c.jj 2017-01-26 22:43:12.000000000 +0100 +++ gcc/testsuite/gcc.dg/tree-ssa/builtin-sprintf-warn-3.c 2017-02-02 14:58:55.000000000 +0100 @@ -198,7 +198,7 @@ void test_sprintf_chk_range_schar (void) /* { dg-message "directive argument in the range \\\[1024, 1034\\\]" "note" { target *-*-* } .-1 } */ T ( 0, "%hhi", R (1024, 2035)); /* { dg-warning ".%hhi. directive writing between 1 and 4 bytes into a region of size 0" } */ - /* { dg-message "using the range \\\[0, -128\\\] for directive argument" "note" { target *-*-* } .-1 } */ + /* { dg-message "using the range \\\[-128, 127\\\] for directive argument" "note" { target *-*-* } .-1 } */ #undef R #define R(min, max) range_schar (min, max) --- gcc/testsuite/gcc.dg/tree-ssa/builtin-sprintf-warn-12.c.jj 2017-02-02 14:17:58.684661224 +0100 +++ gcc/testsuite/gcc.dg/tree-ssa/builtin-sprintf-warn-12.c 2017-02-02 14:17:58.684661224 +0100 @@ -0,0 +1,228 @@ +/* PR tree-optimization/79327 - wrong code at -O2 and -fprintf-return-value + { dg-do compile } + { dg-options "-O2 -Wall -Wformat-overflow=1 -ftrack-macro-expansion=0" } + { dg-require-effective-target int32plus } */ + +typedef __SIZE_TYPE__ size_t; +typedef __WCHAR_TYPE__ wchar_t; + +#define INT_MAX __INT_MAX__ +#define INT_MIN (-INT_MAX - 1) + +/* When debugging, define LINE to the line number of the test case to exercise + and avoid exercising any of the others. The buffer and objsize macros + below make use of LINE to avoid warnings for other lines. */ +#ifndef LINE +# define LINE 0 +#endif + +void sink (char*, char*); + +int dummy_sprintf (char*, const char*, ...); + +char buffer [256]; +extern char *ptr; + +int int_range (int min, int max) +{ + extern int int_value (void); + int n = int_value (); + return n < min || max < n ? min : n; +} + +unsigned uint_range (unsigned min, unsigned max) +{ + extern unsigned uint_value (void); + unsigned n = uint_value (); + return n < min || max < n ? min : n; +} + +/* Evaluate to an array of SIZE characters when non-negative, or to + a pointer to an unknown object otherwise. */ +#define buffer(size) \ + ((0 <= size) ? buffer + sizeof buffer - (size) : ptr) + +/* Helper to expand function to either __builtin_f or dummy_f to + make debugging GCC easy. */ +#define FUNC(f) \ + ((!LINE || LINE == __LINE__) ? __builtin_ ## f : dummy_ ## f) + +/* Macro to verify that calls to __builtin_sprintf (i.e., with no size + argument) issue diagnostics by correctly determining the size of + the destination buffer. */ +#define T(size, ...) \ + (FUNC (sprintf) (buffer (size), __VA_ARGS__), \ + sink (buffer, ptr)) + +/* Return a signed integer in the range [MIN, MAX]. */ +#define R(min, max) int_range (min, max) + +/* Return a unsigned integer in the range [MIN, MAX]. */ +#define U(min, max) uint_range (min, max) + +/* Exercise the hh length modifier with ranges. */ +void test_hh (void) +{ + T (0, "%hhi", R ( -1, 0)); /* { dg-warning "between 1 and 2 bytes" } */ + T (0, "%hhi", R ( -1, 1)); /* { dg-warning "between 1 and 2 bytes" } */ + T (0, "%hhi", R ( -1, 12)); /* { dg-warning "between 1 and 2 bytes" } */ + T (0, "%hhi", R ( -1, 123)); /* { dg-warning "between 1 and 3 bytes" } */ + T (0, "%hhi", R ( -1, 128)); /* { dg-warning "between 1 and 4 bytes" } */ + T (0, "%hhi", R ( -1, 257)); /* { dg-warning "between 1 and 4 bytes" } */ + T (0, "%hhi", R ( -1, 1234)); /* { dg-warning "between 1 and 4 bytes" } */ + T (0, "%hhi", R ( -12, -11)); /* { dg-warning "writing 3 bytes" } */ + T (0, "%hhi", R ( -12, -1)); /* { dg-warning "between 2 and 3 bytes" } */ + T (0, "%hhi", R ( -12, 0)); /* { dg-warning "between 1 and 3 bytes" } */ + T (0, "%hhi", R ( -12, 1)); /* { dg-warning "between 1 and 3 bytes" } */ + T (0, "%hhi", R ( -12, 12)); /* { dg-warning "between 1 and 3 bytes" } */ + T (0, "%hhi", R ( -12, 123)); /* { dg-warning "between 1 and 3 bytes" } */ + T (0, "%hhi", R ( -12, 128)); /* { dg-warning "between 1 and 4 bytes" } */ + T (0, "%hhi", R ( -12, 257)); /* { dg-warning "between 1 and 4 bytes" } */ + T (0, "%hhi", R ( -12, 1234)); /* { dg-warning "between 1 and 4 bytes" } */ + T (0, "%hhi", R ( -99, -10)); /* { dg-warning "writing 3 bytes" } */ + T (0, "%hhi", R (-123, -1)); /* { dg-warning "between 2 and 4 bytes" } */ + T (0, "%hhi", R (-123, 0)); /* { dg-warning "between 1 and 4 bytes" } */ + T (0, "%hhi", R (-123, 1)); /* { dg-warning "between 1 and 4 bytes" } */ + T (0, "%hhi", R (-123, 12)); /* { dg-warning "between 1 and 4 bytes" } */ + T (0, "%hhi", R (-123, 123)); /* { dg-warning "between 1 and 4 bytes" } */ + T (0, "%hhi", R (-123, 257)); /* { dg-warning "between 1 and 4 bytes" } */ + T (0, "%hhi", R (-123, 1234)); /* { dg-warning "between 1 and 4 bytes" } */ + T (0, "%hhi", R (-129, 1)); /* { dg-warning "between 1 and 4 bytes" } */ + T (0, "%hhi", R (-130, -129)); /* { dg-warning "writing 3 bytes" } */ + + T (0, "%hhi", U ( 0, 127)); /* { dg-warning "between 1 and 3 bytes" } */ + + /* The following results in either "127" and "-128" so the ideal result + should be "between 3 and 4 bytes" but because of the overflow from + 128 to -128 in the %hhi directive the input range is reset to that + of char, or [CHAR_MIN, CHAR_MAX], and the warning reflects that. */ + T (0, "%hhi", U ( 127, 128)); /* { dg-warning "between \[13\] and 4 bytes" } */ + /* The following results in either "-128" or "-127". */ + T (0, "%hhi", U ( 128, 129)); /* { dg-warning "writing 4 bytes" } */ + /* The following results in between "-128" and "-99". */ + T (0, "%hhi", U ( 128, 157)); /* { dg-warning "writing between 3 and 4 bytes" } */ + /* Between "-128" and "-1". */ + T (0, "%hhi", U ( 128, 255)); /* { dg-warning "writing between 2 and 4 bytes" } */ + /* Between "-128" and "0". */ + T (0, "%hhi", U ( 128, 256)); /* { dg-warning "writing between 1 and 4 bytes" } */ + /* Between "-128" and "" (zero formats as nothing with zero precision). */ + T (0, "%.0hhi", U ( 128, 256)); /* { dg-warning "writing up to 4 bytes" } */ + /* Same as above but with a range of precisions including zero. */ + T (0, "%.*hhi", /* { dg-warning "writing up to 4 bytes" } */ + R (0, 1), U ( 128, 256)); + /* Same as above but with a positive range of precisions. */ + T (0, "%.*hhi", /* { dg-warning "between 1 and 4 bytes" } */ + R (1, 2), U ( 128, 256)); + /* Precision range includes zero but width is non-zero so output cannot + be empty. */ + T (0, "%1.*hhi", /* { dg-warning "between 1 and 4 bytes" } */ + R (0, 2), U ( 128, 256)); + /* Same as above but with a width range. */ + T (0, "%*.*hhi", /* { dg-warning "between 1 and 4 bytes" } */ + R (1, 2), R (0, 2), U ( 128, 256)); + /* Same as above but this time width range does include zero. */ + T (0, "%*.*hhi", /* { dg-warning "up to 4 bytes" } */ + R (0, 2), R (0, 2), U ( 128, 256)); + + /* Range of precisions in excess of the number of digits and sign. */ + T (0, "%.*hhi", /* { dg-warning "between 5 and 8 bytes" } */ + R (5, 7), U ( 128, 256)); + + /* Between "-128" and "+0". */ + T (0, "%+hhi", U ( 128, 256)); /* { dg-warning "between 2 and 4 bytes" } */ + /* Between "-128" and " 0". */ + T (0, "% hhi", U ( 128, 256)); /* { dg-warning "between 2 and 4 bytes" } */ + + T (0, "%hhu", R ( -1, 1)); /* { dg-warning "between 1 and 3 bytes" } */ + T (0, "%hhu", R ( -1, 12)); /* { dg-warning "between 1 and 3 bytes" } */ + T (0, "%hhu", R ( -1, 123)); /* { dg-warning "between 1 and 3 bytes" } */ + T (0, "%hhu", R ( -1, 128)); /* { dg-warning "between 1 and 3 bytes" } */ + T (0, "%hhu", R ( -1, 257)); /* { dg-warning "between 1 and 3 bytes" } */ + T (0, "%hhu", R ( -1, 1234)); /* { dg-warning "between 1 and 3 bytes" } */ + T (0, "%hhu", R ( -12, 1)); /* { dg-warning "between 1 and 3 bytes" } */ + T (0, "%hhu", R ( -12, 12)); /* { dg-warning "between 1 and 3 bytes" } */ + T (0, "%hhu", R ( -12, 123)); /* { dg-warning "between 1 and 3 bytes" } */ + T (0, "%hhu", R ( -12, 128)); /* { dg-warning "between 1 and 3 bytes" } */ + T (0, "%hhu", R ( -12, 257)); /* { dg-warning "between 1 and 3 bytes" } */ + T (0, "%hhu", R ( -12, 1234)); /* { dg-warning "between 1 and 3 bytes" } */ + T (0, "%hhu", R (-123, 1)); /* { dg-warning "between 1 and 3 bytes" } */ + T (0, "%hhu", R (-123, 12)); /* { dg-warning "between 1 and 3 bytes" } */ + T (0, "%hhu", R (-123, 123)); /* { dg-warning "between 1 and 3 bytes" } */ + T (0, "%hhu", R (-123, 257)); /* { dg-warning "between 1 and 3 bytes" } */ + T (0, "%hhu", R (-123, 1234)); /* { dg-warning "between 1 and 3 bytes" } */ + T (0, "%hhu", R (-129, 1)); /* { dg-warning "between 1 and 3 bytes" } */ + T (0, "%hhu", R (-199, -159)); /* { dg-warning "writing 2 bytes" } */ + T (0, "%hhu", R (-255, -250)); /* { dg-warning "writing 1 byte" } */ +} + +/* Exercise the h length modifier. */ +void test_h (void) +{ + T (0, "%hi", R ( -1, 0)); /* { dg-warning "between 1 and 2 bytes" } */ + T (0, "%hi", R ( 0, 1)); /* { dg-warning "writing 1 byte" } */ + T (0, "%hi", R ( -1, 1)); /* { dg-warning "between 1 and 2 bytes" } */ + T (0, "%hi", R ( -1, 12)); /* { dg-warning "between 1 and 2 bytes" } */ + T (0, "%hi", R ( -12, 1)); /* { dg-warning "between 1 and 3 bytes" } */ + T (0, "%hi", R ( -99, -10)); /* { dg-warning "writing 3 bytes" } */ + T (0, "%hi", R ( -123, 4)); /* { dg-warning "between 1 and 4 bytes" } */ + T (0, "%hi", R ( -1234, 56)); /* { dg-warning "between 1 and 5 bytes" } */ + T (0, "%hi", R ( -1234, 567)); /* { dg-warning "between 1 and 5 bytes" } */ + T (0, "%hi", R ( -1234, 5678)); /* { dg-warning "between 1 and 5 bytes" } */ + T (0, "%ho", R (-32768,-32767)); /* { dg-warning "writing 6 bytes" } */ + + T (0, "%ho", R ( -1, 0)); /* { dg-warning "between 1 and 6 bytes" } */ + T (0, "%ho", R ( 0, 1)); /* { dg-warning "writing 1 byte" } */ + T (0, "%ho", R ( -1, 1)); /* { dg-warning "between 1 and 6 bytes" } */ + T (0, "%ho", R ( -1, 12)); /* { dg-warning "between 1 and 6 bytes" } */ + T (0, "%ho", R ( -12, 1)); /* { dg-warning "between 1 and 6 bytes" } */ + T (0, "%ho", R ( -123, 4)); /* { dg-warning "between 1 and 6 bytes" } */ + T (0, "%ho", R ( -1234, 56)); /* { dg-warning "between 1 and 6 bytes" } */ + T (0, "%ho", R ( -1234, 567)); /* { dg-warning "between 1 and 6 bytes" } */ + T (0, "%ho", R ( -1234, 5678)); /* { dg-warning "between 1 and 6 bytes" } */ + T (0, "%ho", R (-32768,-32767)); /* { dg-warning "writing 6 bytes" } */ + + T (0, "%hu", R ( -1, 0)); /* { dg-warning "between 1 and 5 bytes" } */ + T (0, "%hu", R ( 0, 1)); /* { dg-warning "writing 1 byte" } */ + T (0, "%hu", R ( -1, 1)); /* { dg-warning "between 1 and 5 bytes" } */ + T (0, "%hu", R ( -1, 12)); /* { dg-warning "between 1 and 5 bytes" } */ + T (0, "%hu", R ( -12, 1)); /* { dg-warning "between 1 and 5 bytes" } */ + T (0, "%hu", R ( -123, 4)); /* { dg-warning "between 1 and 5 bytes" } */ + T (0, "%hu", R ( -1234, 56)); /* { dg-warning "between 1 and 5 bytes" } */ + T (0, "%hu", R ( -1234, 567)); /* { dg-warning "between 1 and 5 bytes" } */ + T (0, "%hu", R ( -1234, 5678)); /* { dg-warning "between 1 and 5 bytes" } */ + T (0, "%hu", R (-32768,-32767)); /* { dg-warning "writing 5 bytes" } */ + + T (0, "%hx", R (-32768,-32767)); /* { dg-warning "writing 4 bytes" } */ +} + +/* Exercise integer directives with no length modifier. */ +void test_diou (void) +{ + T (0, "%d", R ( -1, 0)); /* { dg-warning "between 1 and 2 bytes" } */ + T (0, "%i", R ( 0, 1)); /* { dg-warning "writing 1 byte" } */ + T (0, "%d", R ( -1, 1)); /* { dg-warning "between 1 and 2 bytes" } */ + T (0, "%i", R ( -1, 12)); /* { dg-warning "between 1 and 2 bytes" } */ + T (0, "%d", R ( -12, 1)); /* { dg-warning "between 1 and 3 bytes" } */ + T (0, "%i", R ( -123, 4)); /* { dg-warning "between 1 and 4 bytes" } */ + T (0, "%d", R ( -1234, 56)); /* { dg-warning "between 1 and 5 bytes" } */ + T (0, "%i", R ( -1234, 567)); /* { dg-warning "between 1 and 5 bytes" } */ + T (0, "%d", R ( -1234, 5678)); /* { dg-warning "between 1 and 5 bytes" } */ + + T (0, "%u", R ( -1, 0)); /* { dg-warning "between 1 and 10 bytes" } */ + T (0, "%u", R ( 0, 1)); /* { dg-warning "writing 1 byte" } */ + T (0, "%u", R ( -1, 1)); /* { dg-warning "between 1 and 10 bytes" } */ + T (0, "%u", R ( -1, 12)); /* { dg-warning "between 1 and 10 bytes" } */ + T (0, "%u", R ( -12, 1)); /* { dg-warning "between 1 and 10 bytes" } */ + T (0, "%u", R ( -123, 4)); /* { dg-warning "between 1 and 10 bytes" } */ + T (0, "%u", R ( -1234, 56)); /* { dg-warning "between 1 and 10 bytes" } */ + T (0, "%u", R ( -1234, 567)); /* { dg-warning "between 1 and 10 bytes" } */ + T (0, "%u", R ( -1234, 5678)); /* { dg-warning "between 1 and 10 bytes" } */ + + T (0, "%o", R ( -1, 0)); /* { dg-warning "between 1 and 11 bytes" } */ + T (0, "%o", R ( -2, 1)); /* { dg-warning "between 1 and 11 bytes" } */ + T (0, "%o", R ( 0, 1)); /* { dg-warning "writing 1 byte" } */ + + T (0, "%x", R ( -1, 0)); /* { dg-warning "between 1 and 8 bytes" } */ + T (0, "%x", R ( -2, 1)); /* { dg-warning "between 1 and 8 bytes" } */ + T (0, "%x", R ( 0, 1)); /* { dg-warning "writing 1 byte" } */ +} --- gcc/testsuite/gcc.c-torture/execute/pr79327.c.jj 2017-02-02 15:04:34.318642413 +0100 +++ gcc/testsuite/gcc.c-torture/execute/pr79327.c 2017-02-02 15:04:25.427754168 +0100 @@ -0,0 +1,26 @@ +/* PR tree-optimization/79327 */ +/* { dg-require-effective-target c99_runtime } */ + +volatile int a; + +int +main (void) +{ + int i; + char buf[64]; + if (__builtin_sprintf (buf, "%#hho", a) != 1) + __builtin_abort (); + if (__builtin_sprintf (buf, "%#hhx", a) != 1) + __builtin_abort (); + a = 1; + if (__builtin_sprintf (buf, "%#hho", a) != 2) + __builtin_abort (); + if (__builtin_sprintf (buf, "%#hhx", a) != 3) + __builtin_abort (); + a = 127; + if (__builtin_sprintf (buf, "%#hho", a) != 4) + __builtin_abort (); + if (__builtin_sprintf (buf, "%#hhx", a) != 4) + __builtin_abort (); + return 0; +} Jakub ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH] use zero as the lower bound for a signed-unsigned range (PR 79327) 2017-02-02 14:12 ` Jakub Jelinek @ 2017-02-03 18:44 ` Jeff Law 2017-02-03 19:02 ` Jakub Jelinek 0 siblings, 1 reply; 36+ messages in thread From: Jeff Law @ 2017-02-03 18:44 UTC (permalink / raw) To: Jakub Jelinek, Martin Sebor; +Cc: Gcc Patch List On 02/02/2017 07:12 AM, Jakub Jelinek wrote: > On Thu, Feb 02, 2017 at 10:52:18AM +0100, Jakub Jelinek wrote: >> That said, as I've said in the PR and earlier in December on gcc-patches, >> the way format_integer is structured is not really maintainable, has >> very different code paths for arguments with known ranges and without them >> intermixed together and I still ran into wrong-code issues or wrong warnings >> with your patch. See below. Thus I'd like to propose to just very much >> simplify the code: >> >> gimple-ssa-sprintf.c | 108 ++++++++++++--------------------------------------- >> 1 file changed, 27 insertions(+), 81 deletions(-) >> >> So far I haven't done full bootstrap/regtest on this, just >> make check-gcc -j16 -k RUNTESTFLAGS=tree-ssa.exp >> which revealed the need for gcc.dg/tree-ssa/builtin-sprintf-warn-1.c hunk >> below. Here it is turned into a short wrong-code without the patch below: > > Testing revealed a bug in my patch (POINTER_TYPE args really need special > handling, restored), and one further testcase glitch, plus I've added > another testcase for the other wrong-code. > > Going to bootstrap/regtest this again: > > 2017-02-02 Jakub Jelinek <jakub@redhat.com> > Martin Sebor <msebor@redhat.com> > > PR tree-optimization/79327 > * gimple-ssa-sprintf.c (adjust_range_for_overflow): If returning > true, always set *argmin and *argmax to TYPE_{MIN,MAX}_VALUE of > dirtype. > (format_integer): Use wide_int_to_tree instead of build_int_cst > + to_?hwi. If argmin is NULL, just set argmin and argmax to > TYPE_{MIN,MAX}_VALUE of argtype. Simplify and fix computation > of shortest and longest sequence. > > * gcc.dg/tree-ssa/pr79327.c: New test. > * gcc.dg/tree-ssa/builtin-sprintf-warn-1.c > (test_sprintf_chk_hh_nonconst): Don't expect 2 bogus warnings. > * gcc.dg/tree-ssa/builtin-sprintf-warn-3.c > (test_sprintf_chk_range_schar): Adjust dg-message. > * gcc.dg/tree-ssa/builtin-sprintf-warn-12.c: New test. > * gcc.c-torture/execute/pr79327.c: New test. After a lot of thinking about the situation, I think we're better off doing this bit of cleanup now. Jakub and I both expect we're going to be in this code again during the gcc-7 cycle, so the cleanups have immediate benefit. I don't see them as inherently destabilizing and they're a smaller than I had anticipated. They may (or may not) need tweaks to adjust for the vla/flexible array support that went in last night. I don't see an obvious conflict, but if there is use your best judgment about whether or not you need to go through another review/approval cycle. > > --- gcc/gimple-ssa-sprintf.c.jj 2017-02-02 11:03:46.536526801 +0100 > +++ gcc/gimple-ssa-sprintf.c 2017-02-02 14:53:54.657592450 +0100 > @@ -1260,10 +1248,8 @@ format_integer (const directive &dir, tr > @@ -1307,47 +1293,16 @@ format_integer (const directive &dir, tr > > if (!argmin) > { > - /* For an unknown argument (e.g., one passed to a vararg function) > - or one whose value range cannot be determined, create a T_MIN > - constant if the argument's type is signed and T_MAX otherwise, > - and use those to compute the range of bytes that the directive > - can output. When precision may be zero, use zero as the minimum > - since it results in no bytes on output (unless width is specified > - to be greater than 0). */ > - bool zero = dir.prec[0] <= 0 && dir.prec[1] >= 0; > - argmin = build_int_cst (argtype, !zero); > - > - int typeprec = TYPE_PRECISION (dirtype); > - int argprec = TYPE_PRECISION (argtype); > - > - if (argprec < typeprec) > + if (TREE_CODE (argtype) == POINTER_TYPE) > { > - if (POINTER_TYPE_P (argtype)) > - argmax = build_all_ones_cst (argtype); > - else if (TYPE_UNSIGNED (argtype)) > - argmax = TYPE_MAX_VALUE (argtype); > - else > - argmax = TYPE_MIN_VALUE (argtype); > + argmin = build_int_cst (pointer_sized_int_node, 0); > + argmax = build_all_ones_cst (pointer_sized_int_node); Curious why you didn't use the wide_int_to_tree API here. I'm not objecting to using build_XXX, it's more to help guide me when I need to make a choice between the APIs for building INTEGER_CST nodes. OK pending resolution of any conflicts with vla/flexible array changes from last night. jeff ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH] use zero as the lower bound for a signed-unsigned range (PR 79327) 2017-02-03 18:44 ` Jeff Law @ 2017-02-03 19:02 ` Jakub Jelinek 0 siblings, 0 replies; 36+ messages in thread From: Jakub Jelinek @ 2017-02-03 19:02 UTC (permalink / raw) To: Jeff Law; +Cc: Martin Sebor, Gcc Patch List On Fri, Feb 03, 2017 at 11:43:53AM -0700, Jeff Law wrote: > > + if (TREE_CODE (argtype) == POINTER_TYPE) > > { > > - if (POINTER_TYPE_P (argtype)) > > - argmax = build_all_ones_cst (argtype); > > - else if (TYPE_UNSIGNED (argtype)) > > - argmax = TYPE_MAX_VALUE (argtype); > > - else > > - argmax = TYPE_MIN_VALUE (argtype); > > + argmin = build_int_cst (pointer_sized_int_node, 0); > > + argmax = build_all_ones_cst (pointer_sized_int_node); > Curious why you didn't use the wide_int_to_tree API here. I'm not objecting > to using build_XXX, it's more to help guide me when I need to make a choice > between the APIs for building INTEGER_CST nodes. For wide_int_to_tree I need to have some wide_int, which I have from the VR_RANGE query, but not here. If I have an integer rather than wide-int, then build_int_cst is the right API (I could use build_zero_cst, but that in the end just checks if it is INTEGER_TYPE and calls build_int_cst with 0 in that case). > OK pending resolution of any conflicts with vla/flexible array changes from > last night. Thanks, I think there aren't any conflicts. Jakub ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH] use zero as the lower bound for a signed-unsigned range (PR 79327) 2017-02-02 9:52 ` Jakub Jelinek 2017-02-02 14:12 ` Jakub Jelinek @ 2017-02-02 16:58 ` Martin Sebor 2017-02-02 19:59 ` Martin Sebor ` (2 more replies) 1 sibling, 3 replies; 36+ messages in thread From: Martin Sebor @ 2017-02-02 16:58 UTC (permalink / raw) To: Jakub Jelinek, Jeff Law; +Cc: Gcc Patch List On 02/02/2017 02:52 AM, Jakub Jelinek wrote: > On Thu, Feb 02, 2017 at 08:37:07AM +0100, Jakub Jelinek wrote: >>> + if (res.range.max < res.range.min) >>> + { >>> + unsigned HOST_WIDE_INT tmp = res.range.max; >>> + res.range.max = res.range.min; >>> + res.range.min = tmp; >> >> These 3 lines are >> std::swap (res.range.max, res.range.min); >> also note the spaces instead of tabs and indentation by 7/9 spaces >> instead of 1 tab and 1 tab + 2 spaces. >> >>> + } >>> + >>> + if (!TYPE_UNSIGNED (argtype) >>> + && tree_int_cst_lt (integer_zero_node, argmax) >>> + && tree_int_cst_lt (argmin, integer_zero_node)) >> >> And these 2 lines >> && tree_int_cst_sgn (argmax) > 0 >> && tree_int_cst_sgn (argmin) < 0 > > That said, as I've said in the PR and earlier in December on gcc-patches, > the way format_integer is structured is not really maintainable, has > very different code paths for arguments with known ranges and without them > intermixed together and I still ran into wrong-code issues or wrong warnings > with your patch. See below. Thus I'd like to propose to just very much > simplify the code: > > gimple-ssa-sprintf.c | 108 ++++++++++++--------------------------------------- > 1 file changed, 27 insertions(+), 81 deletions(-) > > So far I haven't done full bootstrap/regtest on this, just > make check-gcc -j16 -k RUNTESTFLAGS=tree-ssa.exp > which revealed the need for gcc.dg/tree-ssa/builtin-sprintf-warn-1.c hunk > below. Here it is turned into a short wrong-code without the patch below: > volatile int a; > > int > main (void) > { > int i; > char buf[64]; > if (__builtin_sprintf (buf, "%#hho", a) > 1) > __builtin_abort (); > if (__builtin_sprintf (buf, "%#hhx", a) > 1) > __builtin_abort (); > return 0; > } > Current trunk as well as trunk + your patch computes ranges [2, 4] and > [3, 4], but the correct ranges are [1, 4] and [1, 4], because for a 0 > (or say -131072 etc.) it sets buf to "0" in both cases. That's right. This is a good find. This case is being tested in builtin-sprintf-warn-1.c but the assertions (on lines 1154 and 1155) seem to expect the wrong numbers. I would expect your fix to cause failures here. (And now that I've read the rest of the patch I see it does.) > Otherwise all the tests succeeded, though looking e.g. at the diff > between builtin-sprintf-warn-1.c diagnostics with your patch and with > the patch below instead, there are also differences like: > -builtin-sprintf-warn-1.c:1119:3: note: using the range [1, -128] for directive argument > +builtin-sprintf-warn-1.c:1119:3: note: using the range [-128, 127] for directive argument > where [1, -128] looks clearly wrong, that isn't a valid range, The range in the note is the artificial one the pass uses to compute the range of output. I went back and forth on this as I think it's debatable if [-128, 127] is clearer than [1, -128] (or [-128, 1]). The informative notes aren't completely consistent in what ranges they print. It would be nice to nail down what we think is the most useful output and make them all consistent. > and > this test is -O0 anyway, plus a doesn't have a known range: > T (0, "%hhd", a); /* { dg-warning ".%hhd. directive writing between 1 and . bytes into a region of size 0" } */ > (so to some extent even the note with the patch isn't accurate, the > printed value range is the range of the value after an implicit conversion > from int to signed char). > Or > -builtin-sprintf-warn-1.c:1122:3: note: using the range [1, 255] for directive argument > +builtin-sprintf-warn-1.c:1122:3: note: using the range [0, 255] for directive argument Same as above. > T (0, "%hhu", a); /* { dg-warning "into a region" } */ > similarly, a has [INT_MIN, INT_MAX] range, but after implicit conversion to unsigned > char it is [0, 255], certainly not [1, 255]. Ditto. > Plus there are certain notes removed: > -builtin-sprintf-warn-1.c:1311:3: note: using the range [1, -2147483648] for directive argument > T (0, "%d", a); /* { dg-warning "into a region" } */ > without replacement, here a has [-2147483648, 2147483647] range, but as that > is equal to the type's range, I bet it omits it. Could be. As I said, there are opportunities for improvements in what the notes print. Someone pointed out, for example, that very large numbers are hard to read. It might be clearer to print some of them using constants like LONG_MAX - 2, or in hex, etc. > > 2017-02-02 Jakub Jelinek <jakub@redhat.com> > Martin Sebor <msebor@redhat.com> > > PR tree-optimization/79327 > * gimple-ssa-sprintf.c (adjust_range_for_overflow): If returning > true, always set *argmin and *argmax to TYPE_{MIN,MAX}_VALUE of > dirtype. > (format_integer): Use wide_int_to_tree instead of build_int_cst > + to_?hwi. If argmin is NULL, just set argmin and argmax to > TYPE_{MIN,MAX}_VALUE of argtype. Simplify and fix computation > of shortest and longest sequence. > > * gcc.dg/tree-ssa/builtin-sprintf-warn-12.c: New test. > * gcc.dg/tree-ssa/pr79327.c: New test. > * gcc.dg/tree-ssa/builtin-sprintf-warn-1.c > (test_sprintf_chk_hh_nonconst): Don't expect 2 bogus warnings. > > --- gcc/gimple-ssa-sprintf.c.jj 2017-01-31 09:26:00.692873226 +0100 > +++ gcc/gimple-ssa-sprintf.c 2017-02-02 10:20:06.965884495 +0100 > @@ -1014,8 +1014,8 @@ get_int_range (tree arg, tree type, HOST > determined by checking for the actual argument being in the range > of the type of the directive. If it isn't it must be assumed to > take on the full range of the directive's type. > - Return true when the range has been adjusted to the full unsigned > - range of DIRTYPE, or [0, DIRTYPE_MAX], and false otherwise. */ > + Return true when the range has been adjusted to the full range > + of DIRTYPE, and false otherwise. */ > > static bool > adjust_range_for_overflow (tree dirtype, tree *argmin, tree *argmax) > @@ -1051,20 +1051,8 @@ adjust_range_for_overflow (tree dirtype, > return false; > } > > - tree dirmin = TYPE_MIN_VALUE (dirtype); > - tree dirmax = TYPE_MAX_VALUE (dirtype); > - > - if (TYPE_UNSIGNED (dirtype)) > - { > - *argmin = dirmin; > - *argmax = dirmax; > - } > - else > - { > - *argmin = integer_zero_node; > - *argmax = dirmin; > - } > - > + *argmin = TYPE_MIN_VALUE (dirtype); > + *argmax = TYPE_MAX_VALUE (dirtype); This is likely behind the changes in the notes you pointed out above. The code here is intentional to reflect the range synthesized by the pass to compute the output. I don't have a big issue with changing the notes as you suggest but I don't want it to happen willy-nilly. We should agree what we want the notes to print and document it somewhere (e.g., in a test dedicated just to those notes). As for the rest of the patch, while I appreciate your help and acknowledge it's not my call to make I'm not entirely comfortable with this much churn at this stage. My preference would to just fix the bugs and do the restructuring in stage 1. Martin > return true; > } > > @@ -1260,10 +1248,8 @@ format_integer (const directive &dir, tr > enum value_range_type range_type = get_range_info (arg, &min, &max); > if (range_type == VR_RANGE) > { > - argmin = build_int_cst (argtype, wi::fits_uhwi_p (min) > - ? min.to_uhwi () : min.to_shwi ()); > - argmax = build_int_cst (argtype, wi::fits_uhwi_p (max) > - ? max.to_uhwi () : max.to_shwi ()); > + argmin = wide_int_to_tree (argtype, min); > + argmax = wide_int_to_tree (argtype, max); > > /* Set KNOWNRANGE if the argument is in a known subrange > of the directive's type (KNOWNRANGE may be reset below). */ > @@ -1307,47 +1293,8 @@ format_integer (const directive &dir, tr > > if (!argmin) > { > - /* For an unknown argument (e.g., one passed to a vararg function) > - or one whose value range cannot be determined, create a T_MIN > - constant if the argument's type is signed and T_MAX otherwise, > - and use those to compute the range of bytes that the directive > - can output. When precision may be zero, use zero as the minimum > - since it results in no bytes on output (unless width is specified > - to be greater than 0). */ > - bool zero = dir.prec[0] <= 0 && dir.prec[1] >= 0; > - argmin = build_int_cst (argtype, !zero); > - > - int typeprec = TYPE_PRECISION (dirtype); > - int argprec = TYPE_PRECISION (argtype); > - > - if (argprec < typeprec) > - { > - if (POINTER_TYPE_P (argtype)) > - argmax = build_all_ones_cst (argtype); > - else if (TYPE_UNSIGNED (argtype)) > - argmax = TYPE_MAX_VALUE (argtype); > - else > - argmax = TYPE_MIN_VALUE (argtype); > - } > - else > - { > - if (POINTER_TYPE_P (dirtype)) > - argmax = build_all_ones_cst (dirtype); > - else if (TYPE_UNSIGNED (dirtype)) > - argmax = TYPE_MAX_VALUE (dirtype); > - else > - argmax = TYPE_MIN_VALUE (dirtype); > - } > - > - res.argmin = argmin; > - res.argmax = argmax; > - } > - > - if (tree_int_cst_lt (argmax, argmin)) > - { > - tree tmp = argmax; > - argmax = argmin; > - argmin = tmp; > + argmin = TYPE_MIN_VALUE (argtype); > + argmax = TYPE_MAX_VALUE (argtype); > } > > /* Clear KNOWNRANGE if the range has been adjusted to the maximum > @@ -1361,34 +1308,33 @@ format_integer (const directive &dir, tr > res.argmax = argmax; > } > > - /* Recursively compute the minimum and maximum from the known range, > - taking care to swap them if the lower bound results in longer > - output than the upper bound (e.g., in the range [-1, 0]. */ > - > - if (TYPE_UNSIGNED (dirtype)) > - { > - /* For unsigned conversions/directives, use the minimum (i.e., 0 > - or 1) and maximum to compute the shortest and longest output, > - respectively. */ > + /* Recursively compute the minimum and maximum from the known range. */ > + if (TYPE_UNSIGNED (dirtype) || tree_int_cst_sgn (argmin) >= 0) > + { > + /* For unsigned conversions/directives or signed when > + the minimum is positive, use the minimum and maximum to compute > + the shortest and longest output, respectively. */ > res.range.min = format_integer (dir, argmin).range.min; > res.range.max = format_integer (dir, argmax).range.max; > } > - else > + else if (tree_int_cst_sgn (argmax) < 0) > { > - /* For signed conversions/directives, use the maximum (i.e., 0) > - to compute the shortest output and the minimum (i.e., TYPE_MIN) > - to compute the longest output. This is important when precision > - is specified but unknown because otherwise both output lengths > - would reflect the largest possible precision (i.e., INT_MAX). */ > + /* For signed conversions/directives if maximum is negative, > + use the minimum as the longest output and maximum as the > + shortest output. */ > res.range.min = format_integer (dir, argmax).range.min; > res.range.max = format_integer (dir, argmin).range.max; > } > - > - if (res.range.max < res.range.min) > + else > { > - unsigned HOST_WIDE_INT tmp = res.range.max; > - res.range.max = res.range.min; > - res.range.min = tmp; > + /* Otherwise, 0 is inside of the range and minimum negative. Use 0 > + as the shortest output and for the longest output compute the > + length of the output of both minimum and maximum and pick the > + longer. */ > + unsigned HOST_WIDE_INT max1 = format_integer (dir, argmin).range.max; > + unsigned HOST_WIDE_INT max2 = format_integer (dir, argmax).range.max; > + res.range.min = format_integer (dir, integer_zero_node).range.min; > + res.range.max = MAX (max1, max2); > } > > res.range.likely = res.knownrange ? res.range.max : res.range.min; > --- gcc/testsuite/gcc.dg/tree-ssa/pr79327.c.jj 2017-02-02 08:56:42.278163030 +0100 > +++ gcc/testsuite/gcc.dg/tree-ssa/pr79327.c 2017-02-02 08:56:42.278163030 +0100 > @@ -0,0 +1,31 @@ > +/* PR tree-optimization/79327 - wrong code at -O2 and -fprintf-return-value > + { dg-do run } > + { dg-options "-O2 -Wall" } */ > + > +volatile int a, b = -1; > +char buf[64]; > + > +#define FMT "%+03d%02d" > +const char* fmt = FMT; > + > +int main () > +{ > + int c = a; > + int d = b; > + if (c >= -35791395 && c < 35791394 && d >= -1 && d < __INT_MAX__) > + { > + /* In the following the range of return values can be computed > + by GCC. */ > + int n1 = __builtin_sprintf (buf, FMT, c + 1, d + 1); > + if (n1 > 7) > + __builtin_abort (); > + > + /* Here GCC can't see the format string so the return value > + must be computed by a libc call. */ > + int n2 = __builtin_sprintf (buf, fmt, c + 1, d + 1); > + > + if (n1 != n2) > + __builtin_abort (); > + } > + return 0; > +} > --- gcc/testsuite/gcc.dg/tree-ssa/builtin-sprintf-warn-12.c.jj 2017-02-02 08:56:42.278163030 +0100 > +++ gcc/testsuite/gcc.dg/tree-ssa/builtin-sprintf-warn-12.c 2017-02-02 08:56:42.278163030 +0100 > @@ -0,0 +1,228 @@ > +/* PR tree-optimization79/327 - wrong code at -O2 and -fprintf-return-value > + { dg-do compile } > + { dg-options "-O2 -Wall -Wformat-overflow=1 -ftrack-macro-expansion=0" } > + { dg-require-effective-target int32plus } */ > + > +typedef __SIZE_TYPE__ size_t; > +typedef __WCHAR_TYPE__ wchar_t; > + > +#define INT_MAX __INT_MAX__ > +#define INT_MIN (-INT_MAX - 1) > + > +/* When debugging, define LINE to the line number of the test case to exercise > + and avoid exercising any of the others. The buffer and objsize macros > + below make use of LINE to avoid warnings for other lines. */ > +#ifndef LINE > +# define LINE 0 > +#endif > + > +void sink (char*, char*); > + > +int dummy_sprintf (char*, const char*, ...); > + > +char buffer [256]; > +extern char *ptr; > + > +int int_range (int min, int max) > +{ > + extern int int_value (void); > + int n = int_value (); > + return n < min || max < n ? min : n; > +} > + > +unsigned uint_range (unsigned min, unsigned max) > +{ > + extern unsigned uint_value (void); > + unsigned n = uint_value (); > + return n < min || max < n ? min : n; > +} > + > +/* Evaluate to an array of SIZE characters when non-negative, or to > + a pointer to an unknown object otherwise. */ > +#define buffer(size) \ > + ((0 <= size) ? buffer + sizeof buffer - (size) : ptr) > + > +/* Helper to expand function to either __builtin_f or dummy_f to > + make debugging GCC easy. */ > +#define FUNC(f) \ > + ((!LINE || LINE == __LINE__) ? __builtin_ ## f : dummy_ ## f) > + > +/* Macro to verify that calls to __builtin_sprintf (i.e., with no size > + argument) issue diagnostics by correctly determining the size of > + the destination buffer. */ > +#define T(size, ...) \ > + (FUNC (sprintf) (buffer (size), __VA_ARGS__), \ > + sink (buffer, ptr)) > + > +/* Return a signed integer in the range [MIN, MAX]. */ > +#define R(min, max) int_range (min, max) > + > +/* Return a unsigned integer in the range [MIN, MAX]. */ > +#define U(min, max) uint_range (min, max) > + > +/* Exercise the hh length modifier with ranges. */ > +void test_hh (void) > +{ > + T (0, "%hhi", R ( -1, 0)); /* { dg-warning "between 1 and 2 bytes" } */ > + T (0, "%hhi", R ( -1, 1)); /* { dg-warning "between 1 and 2 bytes" } */ > + T (0, "%hhi", R ( -1, 12)); /* { dg-warning "between 1 and 2 bytes" } */ > + T (0, "%hhi", R ( -1, 123)); /* { dg-warning "between 1 and 3 bytes" } */ > + T (0, "%hhi", R ( -1, 128)); /* { dg-warning "between 1 and 4 bytes" } */ > + T (0, "%hhi", R ( -1, 257)); /* { dg-warning "between 1 and 4 bytes" } */ > + T (0, "%hhi", R ( -1, 1234)); /* { dg-warning "between 1 and 4 bytes" } */ > + T (0, "%hhi", R ( -12, -11)); /* { dg-warning "writing 3 bytes" } */ > + T (0, "%hhi", R ( -12, -1)); /* { dg-warning "between 2 and 3 bytes" } */ > + T (0, "%hhi", R ( -12, 0)); /* { dg-warning "between 1 and 3 bytes" } */ > + T (0, "%hhi", R ( -12, 1)); /* { dg-warning "between 1 and 3 bytes" } */ > + T (0, "%hhi", R ( -12, 12)); /* { dg-warning "between 1 and 3 bytes" } */ > + T (0, "%hhi", R ( -12, 123)); /* { dg-warning "between 1 and 3 bytes" } */ > + T (0, "%hhi", R ( -12, 128)); /* { dg-warning "between 1 and 4 bytes" } */ > + T (0, "%hhi", R ( -12, 257)); /* { dg-warning "between 1 and 4 bytes" } */ > + T (0, "%hhi", R ( -12, 1234)); /* { dg-warning "between 1 and 4 bytes" } */ > + T (0, "%hhi", R ( -99, -10)); /* { dg-warning "writing 3 bytes" } */ > + T (0, "%hhi", R (-123, -1)); /* { dg-warning "between 2 and 4 bytes" } */ > + T (0, "%hhi", R (-123, 0)); /* { dg-warning "between 1 and 4 bytes" } */ > + T (0, "%hhi", R (-123, 1)); /* { dg-warning "between 1 and 4 bytes" } */ > + T (0, "%hhi", R (-123, 12)); /* { dg-warning "between 1 and 4 bytes" } */ > + T (0, "%hhi", R (-123, 123)); /* { dg-warning "between 1 and 4 bytes" } */ > + T (0, "%hhi", R (-123, 257)); /* { dg-warning "between 1 and 4 bytes" } */ > + T (0, "%hhi", R (-123, 1234)); /* { dg-warning "between 1 and 4 bytes" } */ > + T (0, "%hhi", R (-129, 1)); /* { dg-warning "between 1 and 4 bytes" } */ > + T (0, "%hhi", R (-130, -129)); /* { dg-warning "writing 3 bytes" } */ > + > + T (0, "%hhi", U ( 0, 127)); /* { dg-warning "between 1 and 3 bytes" } */ > + > + /* The following results in either "127" and "-128" so the ideal result > + should be "between 3 and 4 bytes" but because of the overflow from > + 128 to -128 in the %hhi directive the input range is reset to that > + of char, or [CHAR_MIN, CHAR_MAX], and the warning reflects that. */ > + T (0, "%hhi", U ( 127, 128)); /* { dg-warning "between \[13\] and 4 bytes" } */ > + /* The following results in either "-128" or "-127". */ > + T (0, "%hhi", U ( 128, 129)); /* { dg-warning "writing 4 bytes" } */ > + /* The following results in between "-128" and "-99". */ > + T (0, "%hhi", U ( 128, 157)); /* { dg-warning "writing between 3 and 4 bytes" } */ > + /* Between "-128" and "-1". */ > + T (0, "%hhi", U ( 128, 255)); /* { dg-warning "writing between 2 and 4 bytes" } */ > + /* Between "-128" and "0". */ > + T (0, "%hhi", U ( 128, 256)); /* { dg-warning "writing between 1 and 4 bytes" } */ > + /* Between "-128" and "" (zero formats as nothing with zero precision). */ > + T (0, "%.0hhi", U ( 128, 256)); /* { dg-warning "writing up to 4 bytes" } */ > + /* Same as above but with a range of precisions including zero. */ > + T (0, "%.*hhi", /* { dg-warning "writing up to 4 bytes" } */ > + R (0, 1), U ( 128, 256)); > + /* Same as above but with a positive range of precisions. */ > + T (0, "%.*hhi", /* { dg-warning "between 1 and 4 bytes" } */ > + R (1, 2), U ( 128, 256)); > + /* Precision range includes zero but width is non-zero so output cannot > + be empty. */ > + T (0, "%1.*hhi", /* { dg-warning "between 1 and 4 bytes" } */ > + R (0, 2), U ( 128, 256)); > + /* Same as above but with a width range. */ > + T (0, "%*.*hhi", /* { dg-warning "between 1 and 4 bytes" } */ > + R (1, 2), R (0, 2), U ( 128, 256)); > + /* Same as above but this time width range does include zero. */ > + T (0, "%*.*hhi", /* { dg-warning "up to 4 bytes" } */ > + R (0, 2), R (0, 2), U ( 128, 256)); > + > + /* Range of precisions in excess of the number of digits and sign. */ > + T (0, "%.*hhi", /* { dg-warning "between 5 and 8 bytes" } */ > + R (5, 7), U ( 128, 256)); > + > + /* Between "-128" and "+0". */ > + T (0, "%+hhi", U ( 128, 256)); /* { dg-warning "between 2 and 4 bytes" } */ > + /* Between "-128" and " 0". */ > + T (0, "% hhi", U ( 128, 256)); /* { dg-warning "between 2 and 4 bytes" } */ > + > + T (0, "%hhu", R ( -1, 1)); /* { dg-warning "between 1 and 3 bytes" } */ > + T (0, "%hhu", R ( -1, 12)); /* { dg-warning "between 1 and 3 bytes" } */ > + T (0, "%hhu", R ( -1, 123)); /* { dg-warning "between 1 and 3 bytes" } */ > + T (0, "%hhu", R ( -1, 128)); /* { dg-warning "between 1 and 3 bytes" } */ > + T (0, "%hhu", R ( -1, 257)); /* { dg-warning "between 1 and 3 bytes" } */ > + T (0, "%hhu", R ( -1, 1234)); /* { dg-warning "between 1 and 3 bytes" } */ > + T (0, "%hhu", R ( -12, 1)); /* { dg-warning "between 1 and 3 bytes" } */ > + T (0, "%hhu", R ( -12, 12)); /* { dg-warning "between 1 and 3 bytes" } */ > + T (0, "%hhu", R ( -12, 123)); /* { dg-warning "between 1 and 3 bytes" } */ > + T (0, "%hhu", R ( -12, 128)); /* { dg-warning "between 1 and 3 bytes" } */ > + T (0, "%hhu", R ( -12, 257)); /* { dg-warning "between 1 and 3 bytes" } */ > + T (0, "%hhu", R ( -12, 1234)); /* { dg-warning "between 1 and 3 bytes" } */ > + T (0, "%hhu", R (-123, 1)); /* { dg-warning "between 1 and 3 bytes" } */ > + T (0, "%hhu", R (-123, 12)); /* { dg-warning "between 1 and 3 bytes" } */ > + T (0, "%hhu", R (-123, 123)); /* { dg-warning "between 1 and 3 bytes" } */ > + T (0, "%hhu", R (-123, 257)); /* { dg-warning "between 1 and 3 bytes" } */ > + T (0, "%hhu", R (-123, 1234)); /* { dg-warning "between 1 and 3 bytes" } */ > + T (0, "%hhu", R (-129, 1)); /* { dg-warning "between 1 and 3 bytes" } */ > + T (0, "%hhu", R (-199, -159)); /* { dg-warning "writing 2 bytes" } */ > + T (0, "%hhu", R (-255, -250)); /* { dg-warning "writing 1 byte" } */ > +} > + > +/* Exercise the h length modifier. */ > +void test_h (void) > +{ > + T (0, "%hi", R ( -1, 0)); /* { dg-warning "between 1 and 2 bytes" } */ > + T (0, "%hi", R ( 0, 1)); /* { dg-warning "writing 1 byte" } */ > + T (0, "%hi", R ( -1, 1)); /* { dg-warning "between 1 and 2 bytes" } */ > + T (0, "%hi", R ( -1, 12)); /* { dg-warning "between 1 and 2 bytes" } */ > + T (0, "%hi", R ( -12, 1)); /* { dg-warning "between 1 and 3 bytes" } */ > + T (0, "%hi", R ( -99, -10)); /* { dg-warning "writing 3 bytes" } */ > + T (0, "%hi", R ( -123, 4)); /* { dg-warning "between 1 and 4 bytes" } */ > + T (0, "%hi", R ( -1234, 56)); /* { dg-warning "between 1 and 5 bytes" } */ > + T (0, "%hi", R ( -1234, 567)); /* { dg-warning "between 1 and 5 bytes" } */ > + T (0, "%hi", R ( -1234, 5678)); /* { dg-warning "between 1 and 5 bytes" } */ > + T (0, "%ho", R (-32768,-32767)); /* { dg-warning "writing 6 bytes" } */ > + > + T (0, "%ho", R ( -1, 0)); /* { dg-warning "between 1 and 6 bytes" } */ > + T (0, "%ho", R ( 0, 1)); /* { dg-warning "writing 1 byte" } */ > + T (0, "%ho", R ( -1, 1)); /* { dg-warning "between 1 and 6 bytes" } */ > + T (0, "%ho", R ( -1, 12)); /* { dg-warning "between 1 and 6 bytes" } */ > + T (0, "%ho", R ( -12, 1)); /* { dg-warning "between 1 and 6 bytes" } */ > + T (0, "%ho", R ( -123, 4)); /* { dg-warning "between 1 and 6 bytes" } */ > + T (0, "%ho", R ( -1234, 56)); /* { dg-warning "between 1 and 6 bytes" } */ > + T (0, "%ho", R ( -1234, 567)); /* { dg-warning "between 1 and 6 bytes" } */ > + T (0, "%ho", R ( -1234, 5678)); /* { dg-warning "between 1 and 6 bytes" } */ > + T (0, "%ho", R (-32768,-32767)); /* { dg-warning "writing 6 bytes" } */ > + > + T (0, "%hu", R ( -1, 0)); /* { dg-warning "between 1 and 5 bytes" } */ > + T (0, "%hu", R ( 0, 1)); /* { dg-warning "writing 1 byte" } */ > + T (0, "%hu", R ( -1, 1)); /* { dg-warning "between 1 and 5 bytes" } */ > + T (0, "%hu", R ( -1, 12)); /* { dg-warning "between 1 and 5 bytes" } */ > + T (0, "%hu", R ( -12, 1)); /* { dg-warning "between 1 and 5 bytes" } */ > + T (0, "%hu", R ( -123, 4)); /* { dg-warning "between 1 and 5 bytes" } */ > + T (0, "%hu", R ( -1234, 56)); /* { dg-warning "between 1 and 5 bytes" } */ > + T (0, "%hu", R ( -1234, 567)); /* { dg-warning "between 1 and 5 bytes" } */ > + T (0, "%hu", R ( -1234, 5678)); /* { dg-warning "between 1 and 5 bytes" } */ > + T (0, "%hu", R (-32768,-32767)); /* { dg-warning "writing 5 bytes" } */ > + > + T (0, "%hx", R (-32768,-32767)); /* { dg-warning "writing 4 bytes" } */ > +} > + > +/* Exercise integer directives with no length modifier. */ > +void test_diou (void) > +{ > + T (0, "%d", R ( -1, 0)); /* { dg-warning "between 1 and 2 bytes" } */ > + T (0, "%i", R ( 0, 1)); /* { dg-warning "writing 1 byte" } */ > + T (0, "%d", R ( -1, 1)); /* { dg-warning "between 1 and 2 bytes" } */ > + T (0, "%i", R ( -1, 12)); /* { dg-warning "between 1 and 2 bytes" } */ > + T (0, "%d", R ( -12, 1)); /* { dg-warning "between 1 and 3 bytes" } */ > + T (0, "%i", R ( -123, 4)); /* { dg-warning "between 1 and 4 bytes" } */ > + T (0, "%d", R ( -1234, 56)); /* { dg-warning "between 1 and 5 bytes" } */ > + T (0, "%i", R ( -1234, 567)); /* { dg-warning "between 1 and 5 bytes" } */ > + T (0, "%d", R ( -1234, 5678)); /* { dg-warning "between 1 and 5 bytes" } */ > + > + T (0, "%u", R ( -1, 0)); /* { dg-warning "between 1 and 10 bytes" } */ > + T (0, "%u", R ( 0, 1)); /* { dg-warning "writing 1 byte" } */ > + T (0, "%u", R ( -1, 1)); /* { dg-warning "between 1 and 10 bytes" } */ > + T (0, "%u", R ( -1, 12)); /* { dg-warning "between 1 and 10 bytes" } */ > + T (0, "%u", R ( -12, 1)); /* { dg-warning "between 1 and 10 bytes" } */ > + T (0, "%u", R ( -123, 4)); /* { dg-warning "between 1 and 10 bytes" } */ > + T (0, "%u", R ( -1234, 56)); /* { dg-warning "between 1 and 10 bytes" } */ > + T (0, "%u", R ( -1234, 567)); /* { dg-warning "between 1 and 10 bytes" } */ > + T (0, "%u", R ( -1234, 5678)); /* { dg-warning "between 1 and 10 bytes" } */ > + > + T (0, "%o", R ( -1, 0)); /* { dg-warning "between 1 and 11 bytes" } */ > + T (0, "%o", R ( -2, 1)); /* { dg-warning "between 1 and 11 bytes" } */ > + T (0, "%o", R ( 0, 1)); /* { dg-warning "writing 1 byte" } */ > + > + T (0, "%x", R ( -1, 0)); /* { dg-warning "between 1 and 8 bytes" } */ > + T (0, "%x", R ( -2, 1)); /* { dg-warning "between 1 and 8 bytes" } */ > + T (0, "%x", R ( 0, 1)); /* { dg-warning "writing 1 byte" } */ > +} > --- gcc/testsuite/gcc.dg/tree-ssa/builtin-sprintf-warn-1.c.jj 2017-01-30 09:31:45.000000000 +0100 > +++ gcc/testsuite/gcc.dg/tree-ssa/builtin-sprintf-warn-1.c 2017-02-02 10:21:22.014902923 +0100 > @@ -1151,8 +1151,8 @@ void test_sprintf_chk_hh_nonconst (int w > T (2, "% hhu", a); /* { dg-warning ". . flag used with .%u." } */ > T (2, "% hhx", a); /* { dg-warning ". . flag used with .%x." } */ > > - T (2, "%#hho", a); /* { dg-warning "nul past the end" } */ > - T (2, "%#hhx", a); /* { dg-warning ".%#hhx. directive writing between 3 and . bytes into a region of size 2" } */ > + T (2, "%#hho", a); > + T (2, "%#hhx", a); > > T (3, "%0hhd", a); > T (3, "%1hhd", a); > > > Jakub > ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH] use zero as the lower bound for a signed-unsigned range (PR 79327) 2017-02-02 16:58 ` Martin Sebor @ 2017-02-02 19:59 ` Martin Sebor 2017-02-02 23:23 ` Jakub Jelinek 2017-02-03 0:31 ` Martin Sebor 2017-02-02 22:15 ` Jakub Jelinek 2017-02-03 7:35 ` Jeff Law 2 siblings, 2 replies; 36+ messages in thread From: Martin Sebor @ 2017-02-02 19:59 UTC (permalink / raw) To: Jakub Jelinek, Jeff Law; +Cc: Gcc Patch List >> So far I haven't done full bootstrap/regtest on this, just >> make check-gcc -j16 -k RUNTESTFLAGS=tree-ssa.exp >> which revealed the need for gcc.dg/tree-ssa/builtin-sprintf-warn-1.c hunk >> below. Here it is turned into a short wrong-code without the patch >> below: >> volatile int a; >> >> int >> main (void) >> { >> int i; >> char buf[64]; >> if (__builtin_sprintf (buf, "%#hho", a) > 1) >> __builtin_abort (); >> if (__builtin_sprintf (buf, "%#hhx", a) > 1) >> __builtin_abort (); >> return 0; >> } >> Current trunk as well as trunk + your patch computes ranges [2, 4] and >> [3, 4], but the correct ranges are [1, 4] and [1, 4], because for a 0 >> (or say -131072 etc.) it sets buf to "0" in both cases. > > That's right. This is a good find. This case is being tested in > builtin-sprintf-warn-1.c but the assertions (on lines 1154 and > 1155) seem to expect the wrong numbers. I would expect your fix > to cause failures here. (And now that I've read the rest of the > patch I see it does.) ... >> - T (2, "%#hho", a); /* { dg-warning "nul past the end" } */ >> - T (2, "%#hhx", a); /* { dg-warning ".%#hhx. directive >> writing between 3 and . bytes into a region of size 2" } */ >> + T (2, "%#hho", a); >> + T (2, "%#hhx", a); On reflection, this isn't quite the right fix. We want to both set the correct range and warn because the call will likely overflow. This is an example of why the likely/unlikely counters have been introduced. By setting min = 1 and likely = 2 for the %#hho and 3 for the %#hhx we get the desired result. Martin ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH] use zero as the lower bound for a signed-unsigned range (PR 79327) 2017-02-02 19:59 ` Martin Sebor @ 2017-02-02 23:23 ` Jakub Jelinek 2017-02-03 0:50 ` Martin Sebor 2017-02-03 0:31 ` Martin Sebor 1 sibling, 1 reply; 36+ messages in thread From: Jakub Jelinek @ 2017-02-02 23:23 UTC (permalink / raw) To: Martin Sebor; +Cc: Jeff Law, Gcc Patch List On Thu, Feb 02, 2017 at 12:59:11PM -0700, Martin Sebor wrote: > > > - T (2, "%#hho", a); /* { dg-warning "nul past the end" } */ > > > - T (2, "%#hhx", a); /* { dg-warning ".%#hhx. directive > > > writing between 3 and . bytes into a region of size 2" } */ > > > + T (2, "%#hho", a); > > > + T (2, "%#hhx", a); > > On reflection, this isn't quite the right fix. We want to both set > the correct range and warn because the call will likely overflow. > This is an example of why the likely/unlikely counters have been > introduced. By setting min = 1 and likely = 2 for the %#hho and > 3 for the %#hhx we get the desired result. Then please first define what should likely mean and document that. That is unrelated to the patch, both in the current trunk, with your patch as well as with my patch there is just res.range.likely = res.knownrange ? res.range.max : res.range.min; res.range.unlikely = res.range.max; for these cases. Do you want likely 2 because that the shortest length for more than one value (only a single value has the shortest length)? Something else? Jakub ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH] use zero as the lower bound for a signed-unsigned range (PR 79327) 2017-02-02 23:23 ` Jakub Jelinek @ 2017-02-03 0:50 ` Martin Sebor 2017-02-03 19:07 ` Jeff Law 0 siblings, 1 reply; 36+ messages in thread From: Martin Sebor @ 2017-02-03 0:50 UTC (permalink / raw) To: Jakub Jelinek; +Cc: Jeff Law, Gcc Patch List On 02/02/2017 04:23 PM, Jakub Jelinek wrote: > On Thu, Feb 02, 2017 at 12:59:11PM -0700, Martin Sebor wrote: >>>> - T (2, "%#hho", a); /* { dg-warning "nul past the end" } */ >>>> - T (2, "%#hhx", a); /* { dg-warning ".%#hhx. directive >>>> writing between 3 and . bytes into a region of size 2" } */ >>>> + T (2, "%#hho", a); >>>> + T (2, "%#hhx", a); >> >> On reflection, this isn't quite the right fix. We want to both set >> the correct range and warn because the call will likely overflow. >> This is an example of why the likely/unlikely counters have been >> introduced. By setting min = 1 and likely = 2 for the %#hho and >> 3 for the %#hhx we get the desired result. > > Then please first define what should likely mean and document that. That's a fair request. I implemented the "likely counter" approach based on your suggestion (in comment #3 on bug 78703) so I'd expect the concept to feel intuitive to you, but documenting it is a good idea (and in fact on my list of things to do). > > That is unrelated to the patch, both in the current trunk, with your > patch as well as with my patch there is just > res.range.likely = res.knownrange ? res.range.max : res.range.min; > res.range.unlikely = res.range.max; > for these cases. > > Do you want likely 2 because that the shortest length for more than > one value (only a single value has the shortest length)? > Something else? For "%#o" the shortest output of one byte (for zero) is less likely than the next shortest output of 2 bytes (0 vs 01). For "%#x" it's one byte vs three bytes (0 vs 0x1). Since specifying '#' clearly indicates the user wants the base prefix it seems very likely that the argument will be non-zero. Whether it's going to be greater than 7 or 15 is not so clear. So I think setting the likely counter to 2 for octal and 3 for hexadecimal makes the most sense. That's what I did in the patch I just posted: https://gcc.gnu.org/ml/gcc-patches/2017-02/msg00223.html Martin ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH] use zero as the lower bound for a signed-unsigned range (PR 79327) 2017-02-03 0:50 ` Martin Sebor @ 2017-02-03 19:07 ` Jeff Law 0 siblings, 0 replies; 36+ messages in thread From: Jeff Law @ 2017-02-03 19:07 UTC (permalink / raw) To: Martin Sebor, Jakub Jelinek; +Cc: Gcc Patch List On 02/02/2017 05:49 PM, Martin Sebor wrote: >> >> That is unrelated to the patch, both in the current trunk, with your >> patch as well as with my patch there is just >> res.range.likely = res.knownrange ? res.range.max : res.range.min; >> res.range.unlikely = res.range.max; >> for these cases. >> >> Do you want likely 2 because that the shortest length for more than >> one value (only a single value has the shortest length)? >> Something else? > > For "%#o" the shortest output of one byte (for zero) is less likely > than the next shortest output of 2 bytes (0 vs 01). > > For "%#x" it's one byte vs three bytes (0 vs 0x1). > > Since specifying '#' clearly indicates the user wants the base > prefix it seems very likely that the argument will be non-zero. > Whether it's going to be greater than 7 or 15 is not so clear. > > So I think setting the likely counter to 2 for octal and 3 for > hexadecimal makes the most sense. We're trying to guess intent here, which I generally prefer to avoid. While we may get it right often, there's going to be cases where we don't -- and I see tuning that kind of decision as ultimately a losing battle. To some degree it may be inevitable in this code, but let's not let it get out of hand. Jeff ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH] use zero as the lower bound for a signed-unsigned range (PR 79327) 2017-02-02 19:59 ` Martin Sebor 2017-02-02 23:23 ` Jakub Jelinek @ 2017-02-03 0:31 ` Martin Sebor 2017-02-03 0:48 ` Jakub Jelinek ` (2 more replies) 1 sibling, 3 replies; 36+ messages in thread From: Martin Sebor @ 2017-02-03 0:31 UTC (permalink / raw) To: Jeff Law; +Cc: Jakub Jelinek, Gcc Patch List [-- Attachment #1: Type: text/plain, Size: 1048 bytes --] >>> - T (2, "%#hho", a); /* { dg-warning "nul past the end" } */ >>> - T (2, "%#hhx", a); /* { dg-warning ".%#hhx. directive >>> writing between 3 and . bytes into a region of size 2" } */ >>> + T (2, "%#hho", a); >>> + T (2, "%#hhx", a); > > On reflection, this isn't quite the right fix. We want to both set > the correct range and warn because the call will likely overflow. > This is an example of why the likely/unlikely counters have been > introduced. By setting min = 1 and likely = 2 for the %#hho and > 3 for the %#hhx we get the desired result. Attached is a simple patch that removes the vestigial setting of the minimum counter while preserving the warnings above by using the likely counter. I had overlooked this when I introduced the likely counter and so in the corner cases of "%#o" and "%#x" with a non-constant argument that could be zero, the minimum counter would be set to 2 and 3 respectively rather than 1 (because zero is formatted without the '0' or '0x' base prefix). Martin [-- Attachment #2: gcc-79327-2.diff --] [-- Type: text/x-patch, Size: 15388 bytes --] gcc/ChangeLog: * gimple-ssa-sprintf.c (format_integer): Adjust the likely counter to assume an unknown argument that may be zero is non-zero. gcc/testsuite/ChangeLog: * gcc.dg/tree-ssa/builtin-sprintf-warn-13.c: New test. diff --git a/gcc/gimple-ssa-sprintf.c b/gcc/gimple-ssa-sprintf.c index 9e099f0..84dd3671 100644 --- a/gcc/gimple-ssa-sprintf.c +++ b/gcc/gimple-ssa-sprintf.c @@ -1242,6 +1242,10 @@ format_integer (const directive &dir, tree arg) of the format string by returning [-1, -1]. */ return fmtresult (); + /* The adjustment to add to the MIN counter when computing the LIKELY + counter for arguments with unknown values. */ + unsigned likely_adjust = 0; + fmtresult res; /* Using either the range the non-constant argument is in, or its @@ -1273,6 +1277,15 @@ format_integer (const directive &dir, tree arg) res.argmin = argmin; res.argmax = argmax; + + /* Set the adjustment for an argument whose range includes + zero since that doesn't include the octal or hexadecimal + base prefix. */ + wide_int wzero = wi::zero (wi::get_precision (min)); + if (maybebase + && wi::le_p (min, wzero, SIGNED) + && wi::le_p (wzero, max, SIGNED)) + likely_adjust = base == 8 ? 1 : base == 16 ? 2 : 0; } else if (range_type == VR_ANTI_RANGE) { @@ -1311,11 +1324,14 @@ format_integer (const directive &dir, tree arg) or one whose value range cannot be determined, create a T_MIN constant if the argument's type is signed and T_MAX otherwise, and use those to compute the range of bytes that the directive - can output. When precision may be zero, use zero as the minimum - since it results in no bytes on output (unless width is specified - to be greater than 0). */ - bool zero = dir.prec[0] <= 0 && dir.prec[1] >= 0; - argmin = build_int_cst (argtype, !zero); + can output. */ + argmin = integer_zero_node; + + /* Set the adjustment for an argument whose range includes + zero since that doesn't include the octal or hexadecimal + base prefix. */ + if (maybebase) + likely_adjust = base == 8 ? 1 : base == 16 ? 2 : 0; int typeprec = TYPE_PRECISION (dirtype); int argprec = TYPE_PRECISION (argtype); @@ -1391,7 +1407,11 @@ format_integer (const directive &dir, tree arg) res.range.min = tmp; } - res.range.likely = res.knownrange ? res.range.max : res.range.min; + /* Add the adjustment for an argument whose range includes zero + since it doesn't include the octal or hexadecimal base prefix. */ + res.range.likely + = res.knownrange ? res.range.max : res.range.min + likely_adjust; + res.range.unlikely = res.range.max; res.adjust_for_width_or_precision (dir.width, dirtype, base, (sign | maybebase) + (base == 16)); diff --git a/gcc/testsuite/gcc.dg/tree-ssa/builtin-sprintf-warn-13.c b/gcc/testsuite/gcc.dg/tree-ssa/builtin-sprintf-warn-13.c new file mode 100644 index 0000000..deba705 --- /dev/null +++ b/gcc/testsuite/gcc.dg/tree-ssa/builtin-sprintf-warn-13.c @@ -0,0 +1,180 @@ +/* { dg-do compile } + { dg-options "-O2 -Wall -Wformat-overflow=1 -ftrack-macro-expansion=0" } + { dg-require-effective-target int32plus } */ + +typedef __SIZE_TYPE__ size_t; +typedef __WCHAR_TYPE__ wchar_t; + +#define INT_MAX __INT_MAX__ +#define INT_MIN (-INT_MAX - 1) + +/* When debugging, define LINE to the line number of the test case to exercise + and avoid exercising any of the others. The buffer and objsize macros + below make use of LINE to avoid warnings for other lines. */ +#ifndef LINE +# define LINE 0 +#endif + +void sink (char*, char*); + +int dummy_sprintf (char*, const char*, ...); + +char buffer [256]; +extern char *ptr; + +int int_range (int min, int max) +{ + extern int int_value (void); + int n = int_value (); + return n < min || max < n ? min : n; +} + +unsigned uint_range (unsigned min, unsigned max) +{ + extern unsigned uint_value (void); + unsigned n = uint_value (); + return n < min || max < n ? min : n; +} + +/* Evaluate to an array of SIZE characters when non-negative, or to + a pointer to an unknown object otherwise. */ +#define buffer(size) \ + ((0 <= size) ? buffer + sizeof buffer - (size) : ptr) + +/* Helper to expand function to either __builtin_f or dummy_f to + make debugging GCC easy. */ +#define FUNC(f) \ + ((!LINE || LINE == __LINE__) ? __builtin_ ## f : dummy_ ## f) + +/* Macro to verify that calls to __builtin_sprintf (i.e., with no size + argument) issue diagnostics by correctly determining the size of + the destination buffer. */ +#define T(size, ...) \ + (FUNC (sprintf) (buffer (size), __VA_ARGS__), \ + sink (buffer, ptr)) + +/* Return a signed integer in the range [MIN, MAX]. */ +#define R(min, max) int_range (min, max) + +/* Return a unsigned integer in the range [MIN, MAX]. */ +#define U(min, max) uint_range (min, max) + +/* Verify warnings and ranges for certain overflow. */ +void test_min_overflow (int i) +{ + T (0, "%#hho", i); /* { dg-warning "between 1 and 4 bytes" } */ + T (0, "%#hho", R (-1, 0)); /* { dg-warning "between 1 and 4 bytes" } */ + T (0, "%#hho", R (-1, 1)); /* { dg-warning "between 1 and 4 bytes" } */ + T (0, "%#hho", R ( 0, 1)); /* { dg-warning "between 1 and 2 bytes" } */ + T (0, "%#hho", R ( 1, 2)); /* { dg-warning "writing 2 bytes" } */ + + T (0, "%#ho", i); /* { dg-warning "between 1 and 7 bytes" } */ + T (0, "%#ho", R (-1, 0)); /* { dg-warning "between 1 and 7 bytes" } */ + T (0, "%#ho", R (-1, 1)); /* { dg-warning "between 1 and 7 bytes" } */ + T (0, "%#ho", R ( 0, 1)); /* { dg-warning "between 1 and 2 bytes" } */ + T (0, "%#ho", R ( 1, 2)); /* { dg-warning "writing 2 bytes" } */ + + T (0, "%#o", i); /* { dg-warning "between 1 and 12 bytes" } */ + T (0, "%#o", R (-1, 0)); /* { dg-warning "between 1 and 12 bytes" } */ + T (0, "%#o", R (-1, 1)); /* { dg-warning "between 1 and 12 bytes" } */ + T (0, "%#o", R ( 0, 1)); /* { dg-warning "between 1 and 2 bytes" } */ + T (0, "%#o", R ( 1, 2)); /* { dg-warning "writing 2 bytes" } */ + + T (0, "%#hhx", i); /* { dg-warning "between 1 and 4 bytes" } */ + T (0, "%#hhx", R (-1, 0)); /* { dg-warning "between 1 and 4 bytes" } */ + T (0, "%#hhx", R (-1, 1)); /* { dg-warning "between 1 and 4 bytes" } */ + T (0, "%#hhx", R ( 0, 1)); /* { dg-warning "between 1 and 3 bytes" } */ + T (0, "%#hhx", R ( 1, 2)); /* { dg-warning "writing 3 bytes" } */ + + T (0, "%#hx", i); /* { dg-warning "between 1 and 6 bytes" } */ + T (0, "%#hx", R (-1, 0)); /* { dg-warning "between 1 and 6 bytes" } */ + T (0, "%#hx", R (-1, 1)); /* { dg-warning "between 1 and 6 bytes" } */ + T (0, "%#hx", R ( 0, 1)); /* { dg-warning "between 1 and 3 bytes" } */ + T (0, "%#hx", R ( 1, 2)); /* { dg-warning "writing 3 bytes" } */ + + T (0, "%#x", i); /* { dg-warning "between 1 and 10 bytes" } */ + T (0, "%#x", R (-1, 0)); /* { dg-warning "between 1 and 10 bytes" } */ + T (0, "%#x", R (-1, 1)); /* { dg-warning "between 1 and 10 bytes" } */ + T (0, "%#x", R ( 0, 1)); /* { dg-warning "between 1 and 3 bytes" } */ + T (0, "%#x", R ( 1, 2)); /* { dg-warning "writing 3 bytes" } */ +} + +/* Verify warnings and ranges for likely overflow. */ +void test_likely_overflow (int i) +{ + T (2, "%#hho", i); /* { dg-warning "may write a terminating nul" } */ + T (2, "%#hho", R (-1, 0)); /* { dg-warning "may write a terminating nul" } */ + T (2, "%#hho", R (-1, 1)); /* { dg-warning "may write a terminating nul" } */ + T (2, "%#hho", R ( 0, 1)); /* { dg-warning "may write a terminating nul" } */ + T (2, "%#hho", R ( 1, 2)); /* { dg-warning "writing a terminating nul" } */ + + T (2, "%#ho", i); /* { dg-warning "may write a terminating nul" } */ + T (2, "%#ho", R (-1, 0)); /* { dg-warning "may write a terminating nul" } */ + T (2, "%#ho", R (-1, 1)); /* { dg-warning "may write a terminating nul" } */ + T (2, "%#ho", R ( 0, 1)); /* { dg-warning "may write a terminating nul" } */ + T (2, "%#ho", R ( 1, 2)); /* { dg-warning "writing a terminating nul" } */ + + T (2, "%#o", i); /* { dg-warning "may write a terminating nul" } */ + T (2, "%#o", R (-1, 0)); /* { dg-warning "may write a terminating nul" } */ + T (2, "%#o", R (-1, 1)); /* { dg-warning "may write a terminating nul" } */ + T (2, "%#o", R ( 0, 1)); /* { dg-warning "may write a terminating nul" } */ + T (2, "%#o", R ( 1, 2)); /* { dg-warning "writing a terminating nul" } */ + + T (2, "%#hhx", i); /* { dg-warning "between 1 and 4 bytes" } */ + T (2, "%#hhx", R (-1, 0)); /* { dg-warning "between 1 and 4 bytes" } */ + T (2, "%#hhx", R (-1, 1)); /* { dg-warning "between 1 and 4 bytes" } */ + T (2, "%#hhx", R ( 0, 1)); /* { dg-warning "between 1 and 3 bytes" } */ + T (2, "%#hhx", R ( 1, 2)); /* { dg-warning "writing 3 bytes" } */ + + T (2, "%#hx", i); /* { dg-warning "between 1 and 6 bytes" } */ + T (2, "%#hx", R (-1, 0)); /* { dg-warning "between 1 and 6 bytes" } */ + T (2, "%#hx", R (-1, 1)); /* { dg-warning "between 1 and 6 bytes" } */ + T (2, "%#hx", R ( 0, 1)); /* { dg-warning "between 1 and 3 bytes" } */ + T (2, "%#hx", R ( 1, 2)); /* { dg-warning "writing 3 bytes" } */ + + T (2, "%#x", i); /* { dg-warning "between 1 and 10 bytes" } */ + T (2, "%#x", R (-1, 0)); /* { dg-warning "between 1 and 10 bytes" } */ + T (2, "%#x", R (-1, 1)); /* { dg-warning "between 1 and 10 bytes" } */ + T (2, "%#x", R ( 0, 1)); /* { dg-warning "between 1 and 3 bytes" } */ + T (2, "%#x", R ( 1, 2)); /* { dg-warning "writing 3 bytes" } */ +} + +/* Verify warnings likely overflow due to the terminating nul. */ +void test_likely_nul_overflow (int i) +{ + T (3, "%#hho", i); + T (3, "%#hho", R (-1, 0)); + T (3, "%#hho", R (-1, 1)); + T (3, "%#hho", R ( 0, 1)); + T (3, "%#hho", R ( 1, 2)); + + T (3, "%#ho", i); + T (3, "%#ho", R (-1, 0)); + T (3, "%#ho", R (-1, 1)); + T (3, "%#ho", R ( 0, 1)); + T (3, "%#ho", R ( 1, 2)); + + T (3, "%#o", i); + T (3, "%#o", R (-1, 0)); + T (3, "%#o", R (-1, 1)); + T (3, "%#o", R ( 0, 1)); + T (3, "%#o", R ( 1, 2)); + + T (3, "%#hhx", i); /* { dg-warning "may write a terminating nul" } */ + T (3, "%#hhx", R (-1, 0)); /* { dg-warning "may write a terminating nul" } */ + T (3, "%#hhx", R (-1, 1)); /* { dg-warning "may write a terminating nul" } */ + T (3, "%#hhx", R ( 0, 1)); /* { dg-warning "may write a terminating nul" } */ + T (3, "%#hhx", R ( 1, 2)); /* { dg-warning "writing a terminating nul" } */ + + T (3, "%#hx", i); /* { dg-warning "may write a terminating nul" } */ + T (3, "%#hx", R (-1, 0)); /* { dg-warning "may write a terminating nul" } */ + T (3, "%#hx", R (-1, 1)); /* { dg-warning "may write a terminating nul" } */ + T (3, "%#hx", R ( 0, 1)); /* { dg-warning "may write a terminating nul" } */ + T (3, "%#hx", R ( 1, 2)); /* { dg-warning "writing a terminating nul" } */ + + T (3, "%#x", i); /* { dg-warning "may write a terminating nul" } */ + T (3, "%#x", R (-1, 0)); /* { dg-warning "may write a terminating nul" } */ + T (3, "%#x", R (-1, 1)); /* { dg-warning "may write a terminating nul" } */ + T (3, "%#x", R ( 0, 1)); /* { dg-warning "may write a terminating nul" } */ + T (3, "%#x", R ( 1, 2)); /* { dg-warning "writing a terminating nul" } */ +} diff --git a/gcc/testsuite/gcc.dg/tree-ssa/builtin-sprintf-warn-1.c b/gcc/testsuite/gcc.dg/tree-ssa/builtin-sprintf-warn-1.c index 9955326..2b5f7dd 100644 --- a/gcc/testsuite/gcc.dg/tree-ssa/builtin-sprintf-warn-1.c +++ b/gcc/testsuite/gcc.dg/tree-ssa/builtin-sprintf-warn-1.c @@ -1152,7 +1152,7 @@ void test_sprintf_chk_hh_nonconst (int w, int p, int a) T (2, "% hhx", a); /* { dg-warning ". . flag used with .%x." } */ T (2, "%#hho", a); /* { dg-warning "nul past the end" } */ - T (2, "%#hhx", a); /* { dg-warning ".%#hhx. directive writing between 3 and . bytes into a region of size 2" } */ + T (2, "%#hhx", a); /* { dg-warning ".%#hhx. directive writing between 1 and . bytes into a region of size 2" } */ T (3, "%0hhd", a); T (3, "%1hhd", a); diff --git a/gcc/testsuite/gcc.dg/tree-ssa/pr79327-2.c b/gcc/testsuite/gcc.dg/tree-ssa/pr79327-2.c new file mode 100644 index 0000000..2a2c297 --- /dev/null +++ b/gcc/testsuite/gcc.dg/tree-ssa/pr79327-2.c @@ -0,0 +1,129 @@ +/* { dg-compile } + { dg-options "-O2 -fdump-tree-optimized" } */ + +#define CAT(s, n) s ## n +#define FAIL(line) CAT (failure_on_line_, line) + +/* Emit a call to a function named failure_on_line_NNN when EXPR is false. */ +#define ASSERT(expr) \ + do { \ + extern void FAIL (__LINE__)(void); \ + if (!(expr)) FAIL (__LINE__)(); \ + } while (0) + +#define KEEP(line) CAT (keep_call_on_line_, line) + +/* Emit a call to a function named keep_call_on_line_NNN when EXPR is true. + Used to verify that the expression need not be the only one that holds. */ +#define ASSERT_MAYBE(expr) \ + do { \ + extern void KEEP (__LINE__)(void); \ + if (expr) KEEP (__LINE__)(); \ + } while (0) + +int test_hho (int i) +{ + int n = __builtin_snprintf (0, 0, "%#hho", i); + + ASSERT (0 < n && n < 5); + + ASSERT_MAYBE (1 == n); + ASSERT_MAYBE (2 == n); + ASSERT_MAYBE (3 == n); + ASSERT_MAYBE (4 == n); + + return n; +} + +int test_ho (int i) +{ + int n = __builtin_snprintf (0, 0, "%#ho", i); + + ASSERT (0 < n && n < 8); + + ASSERT_MAYBE (1 == n); + ASSERT_MAYBE (2 == n); + ASSERT_MAYBE (3 == n); + ASSERT_MAYBE (4 == n); + ASSERT_MAYBE (5 == n); + ASSERT_MAYBE (6 == n); + ASSERT_MAYBE (7 == n); + + return n; +} + +int test_o (int i) +{ + int n = __builtin_snprintf (0, 0, "%#o", i); + + ASSERT (0 < n && n < 13); + + ASSERT_MAYBE (1 == n); + ASSERT_MAYBE (2 == n); + ASSERT_MAYBE (3 == n); + ASSERT_MAYBE (4 == n); + ASSERT_MAYBE (5 == n); + ASSERT_MAYBE (6 == n); + ASSERT_MAYBE (7 == n); + ASSERT_MAYBE (8 == n); + ASSERT_MAYBE (9 == n); + ASSERT_MAYBE (10 == n); + ASSERT_MAYBE (11 == n); + ASSERT_MAYBE (12 == n); + + return n; +} + + +int test_hhx (int i) +{ + int n = __builtin_snprintf (0, 0, "%#hhx", i); + + ASSERT (0 < n && n < 5); + + ASSERT_MAYBE (1 == n); + ASSERT_MAYBE (2 == n); + ASSERT_MAYBE (3 == n); + ASSERT_MAYBE (4 == n); + + return n; +} + +int test_hx (int i) +{ + int n = __builtin_snprintf (0, 0, "%#hx", i); + + ASSERT (0 < n && n < 7); + + ASSERT_MAYBE (1 == n); + ASSERT_MAYBE (2 == n); + ASSERT_MAYBE (3 == n); + ASSERT_MAYBE (4 == n); + ASSERT_MAYBE (5 == n); + ASSERT_MAYBE (6 == n); + + return n; +} + +int test_x (int i) +{ + int n = __builtin_snprintf (0, 0, "%#x", i); + + ASSERT (0 < n && n < 11); + + ASSERT_MAYBE (1 == n); + ASSERT_MAYBE (2 == n); + ASSERT_MAYBE (3 == n); + ASSERT_MAYBE (4 == n); + ASSERT_MAYBE (5 == n); + ASSERT_MAYBE (6 == n); + ASSERT_MAYBE (7 == n); + ASSERT_MAYBE (8 == n); + ASSERT_MAYBE (9 == n); + ASSERT_MAYBE (10 == n); + + return n; +} + +/* { dg-final { scan-tree-dump-not "failure_on_line" "optimized"} } + { dg-final { scan-tree-dump-times "keep_call_on_line" 43 "optimized"} } */ ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH] use zero as the lower bound for a signed-unsigned range (PR 79327) 2017-02-03 0:31 ` Martin Sebor @ 2017-02-03 0:48 ` Jakub Jelinek 2017-02-03 5:37 ` Martin Sebor 2017-02-03 19:02 ` Jeff Law 2 siblings, 0 replies; 36+ messages in thread From: Jakub Jelinek @ 2017-02-03 0:48 UTC (permalink / raw) To: Martin Sebor; +Cc: Jeff Law, Gcc Patch List On Thu, Feb 02, 2017 at 05:31:19PM -0700, Martin Sebor wrote: > index 9e099f0..84dd3671 100644 > --- a/gcc/gimple-ssa-sprintf.c > +++ b/gcc/gimple-ssa-sprintf.c > @@ -1242,6 +1242,10 @@ format_integer (const directive &dir, tree arg) > of the format string by returning [-1, -1]. */ > return fmtresult (); > > + /* The adjustment to add to the MIN counter when computing the LIKELY > + counter for arguments with unknown values. */ > + unsigned likely_adjust = 0; > + > fmtresult res; > > /* Using either the range the non-constant argument is in, or its > @@ -1273,6 +1277,15 @@ format_integer (const directive &dir, tree arg) > > res.argmin = argmin; > res.argmax = argmax; > + > + /* Set the adjustment for an argument whose range includes > + zero since that doesn't include the octal or hexadecimal > + base prefix. */ > + wide_int wzero = wi::zero (wi::get_precision (min)); > + if (maybebase > + && wi::le_p (min, wzero, SIGNED) > + && wi::le_p (wzero, max, SIGNED)) > + likely_adjust = base == 8 ? 1 : base == 16 ? 2 : 0; > } > else if (range_type == VR_ANTI_RANGE) > { > @@ -1311,11 +1324,14 @@ format_integer (const directive &dir, tree arg) > or one whose value range cannot be determined, create a T_MIN > constant if the argument's type is signed and T_MAX otherwise, > and use those to compute the range of bytes that the directive > - can output. When precision may be zero, use zero as the minimum > - since it results in no bytes on output (unless width is specified > - to be greater than 0). */ > - bool zero = dir.prec[0] <= 0 && dir.prec[1] >= 0; > - argmin = build_int_cst (argtype, !zero); > + can output. */ > + argmin = integer_zero_node; > + > + /* Set the adjustment for an argument whose range includes > + zero since that doesn't include the octal or hexadecimal > + base prefix. */ > + if (maybebase) > + likely_adjust = base == 8 ? 1 : base == 16 ? 2 : 0; This is another sign why the patch I've posted is desirable. Now you have to tweak several places, with the patch just one, you have a final range and you can check whether it includes zero or not. wi::le_p (wzero, max, SIGNED) is !wi::neg_p (max) I think. That said, adding something to res.range.min looks wrong to me, you don't know without more analysis whether actually the length for 0 and length for say 1 or -1 isn't the same. Consider %#x where indeed 0 is 2 byte shorter than 1, but say %#3x where it is the same length, so if you still add your likely_adjust of 2 to that, the res.range.likely value will be suddenly 5 instead of correct 3. Easier would be to see if 0 is in the range, see if 1 is also in the range and set res.range.likely to the length of 1, or, if 0 is in the range, 1 is not but -1 is, to that of -1 or something like that. Jakub ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH] use zero as the lower bound for a signed-unsigned range (PR 79327) 2017-02-03 0:31 ` Martin Sebor 2017-02-03 0:48 ` Jakub Jelinek @ 2017-02-03 5:37 ` Martin Sebor 2017-02-03 19:02 ` Jeff Law 2 siblings, 0 replies; 36+ messages in thread From: Martin Sebor @ 2017-02-03 5:37 UTC (permalink / raw) To: Jeff Law; +Cc: Jakub Jelinek, Gcc Patch List [-- Attachment #1: Type: text/plain, Size: 1214 bytes --] On 02/02/2017 05:31 PM, Martin Sebor wrote: >>>> - T (2, "%#hho", a); /* { dg-warning "nul past the end" >>>> } */ >>>> - T (2, "%#hhx", a); /* { dg-warning ".%#hhx. directive >>>> writing between 3 and . bytes into a region of size 2" } */ >>>> + T (2, "%#hho", a); >>>> + T (2, "%#hhx", a); >> >> On reflection, this isn't quite the right fix. We want to both set >> the correct range and warn because the call will likely overflow. >> This is an example of why the likely/unlikely counters have been >> introduced. By setting min = 1 and likely = 2 for the %#hho and >> 3 for the %#hhx we get the desired result. > > Attached is a simple patch that removes the vestigial setting of > the minimum counter while preserving the warnings above by using > the likely counter. > > I had overlooked this when I introduced the likely counter and so > in the corner cases of "%#o" and "%#x" with a non-constant argument > that could be zero, the minimum counter would be set to 2 and 3 > respectively rather than 1 (because zero is formatted without > the '0' or '0x' base prefix). The attached is an updated version that addresses Jakub's comments in a separate post. Martin [-- Attachment #2: gcc-79327-2.diff --] [-- Type: text/x-patch, Size: 22719 bytes --] gcc/ChangeLog: * gimple-ssa-sprintf.c (tree_digits): Avoid adding octal prefix when precision has resulted in leading zeros. (format_integer): Adjust the likely counter to assume an unknown argument that may be zero is non-zero. gcc/testsuite/ChangeLog: * gcc.dg/tree-ssa/builtin-sprintf-warn-13.c: New test. Index: gcc/gimple-ssa-sprintf.c =================================================================== --- gcc/gimple-ssa-sprintf.c (revision 245142) +++ gcc/gimple-ssa-sprintf.c (working copy) @@ -762,7 +762,9 @@ tree_digits (tree x, int base, HOST_WIDE_INT prec, res += prec < ndigs ? ndigs : prec; - if (prefix && absval) + /* Adjust a non-zero value for the base prefix, either hexadecimal, + or, unless precision has resulted in a leading zero, also octal. */ + if (prefix && absval && (base == 16 || prec <= ndigs)) { if (base == 8) res += 1; @@ -1242,6 +1244,10 @@ format_integer (const directive &dir, tree arg) of the format string by returning [-1, -1]. */ return fmtresult (); + /* True if the LIKELY counter should be adjusted upward from the MIN + counter to account for arguments with unknown values. */ + bool likely_adjust = false; + fmtresult res; /* Using either the range the non-constant argument is in, or its @@ -1273,6 +1279,14 @@ format_integer (const directive &dir, tree arg) res.argmin = argmin; res.argmax = argmax; + + /* Set the adjustment for an argument whose range includes + zero since that doesn't include the octal or hexadecimal + base prefix. */ + wide_int wzero = wi::zero (wi::get_precision (min)); + if (wi::le_p (min, wzero, SIGNED) + && !wi::neg_p (max)) + likely_adjust = true; } else if (range_type == VR_ANTI_RANGE) { @@ -1311,12 +1325,14 @@ format_integer (const directive &dir, tree arg) or one whose value range cannot be determined, create a T_MIN constant if the argument's type is signed and T_MAX otherwise, and use those to compute the range of bytes that the directive - can output. When precision may be zero, use zero as the minimum - since it results in no bytes on output (unless width is specified - to be greater than 0). */ - bool zero = dir.prec[0] <= 0 && dir.prec[1] >= 0; - argmin = build_int_cst (argtype, !zero); + can output. */ + argmin = integer_zero_node; + /* Set the adjustment for an argument whose range includes + zero since that doesn't include the octal or hexadecimal + base prefix. */ + likely_adjust = true; + int typeprec = TYPE_PRECISION (dirtype); int argprec = TYPE_PRECISION (argtype); @@ -1391,7 +1407,24 @@ format_integer (const directive &dir, tree arg) res.range.min = tmp; } - res.range.likely = res.knownrange ? res.range.max : res.range.min; + /* Add the adjustment for an argument whose range includes zero + since it doesn't include the octal or hexadecimal base prefix. */ + if (res.knownrange) + res.range.likely = res.range.max; + else + { + res.range.likely = res.range.min; + if (likely_adjust && maybebase && base != 10) + { + if (res.range.min == 1) + res.range.likely += base == 8 ? 1 : 2; + else if (res.range.min == 2 + && base == 16 + && (dir.width[0] == 2 || dir.prec[0] == 2)) + ++res.range.likely; + } + } + res.range.unlikely = res.range.max; res.adjust_for_width_or_precision (dir.width, dirtype, base, (sign | maybebase) + (base == 16)); Index: gcc/testsuite/gcc.dg/tree-ssa/builtin-sprintf-warn-1.c =================================================================== --- gcc/testsuite/gcc.dg/tree-ssa/builtin-sprintf-warn-1.c (revision 245142) +++ gcc/testsuite/gcc.dg/tree-ssa/builtin-sprintf-warn-1.c (working copy) @@ -1152,7 +1152,7 @@ void test_sprintf_chk_hh_nonconst (int w, int p, i T (2, "% hhx", a); /* { dg-warning ". . flag used with .%x." } */ T (2, "%#hho", a); /* { dg-warning "nul past the end" } */ - T (2, "%#hhx", a); /* { dg-warning ".%#hhx. directive writing between 3 and . bytes into a region of size 2" } */ + T (2, "%#hhx", a); /* { dg-warning ".%#hhx. directive writing between 1 and . bytes into a region of size 2" } */ T (3, "%0hhd", a); T (3, "%1hhd", a); Index: gcc/testsuite/gcc.dg/tree-ssa/builtin-sprintf-warn-13.c =================================================================== --- gcc/testsuite/gcc.dg/tree-ssa/builtin-sprintf-warn-13.c (revision 0) +++ gcc/testsuite/gcc.dg/tree-ssa/builtin-sprintf-warn-13.c (working copy) @@ -0,0 +1,261 @@ +/* { dg-do compile } + { dg-options "-O2 -Wall -Wformat-overflow=1 -ftrack-macro-expansion=0" } + { dg-require-effective-target int32plus } */ + +typedef __SIZE_TYPE__ size_t; +typedef __WCHAR_TYPE__ wchar_t; + +#define INT_MAX __INT_MAX__ +#define INT_MIN (-INT_MAX - 1) + +/* When debugging, define LINE to the line number of the test case to exercise + and avoid exercising any of the others. The buffer and objsize macros + below make use of LINE to avoid warnings for other lines. */ +#ifndef LINE +# define LINE 0 +#endif + +void sink (char*, char*); + +int dummy_sprintf (char*, const char*, ...); + +char buffer [256]; +extern char *ptr; + +int int_range (int min, int max) +{ + extern int int_value (void); + int n = int_value (); + return n < min || max < n ? min : n; +} + +unsigned uint_range (unsigned min, unsigned max) +{ + extern unsigned uint_value (void); + unsigned n = uint_value (); + return n < min || max < n ? min : n; +} + +/* Evaluate to an array of SIZE characters when non-negative, or to + a pointer to an unknown object otherwise. */ +#define buffer(size) \ + ((0 <= size) ? buffer + sizeof buffer - (size) : ptr) + +/* Helper to expand function to either __builtin_f or dummy_f to + make debugging GCC easy. */ +#define FUNC(f) \ + ((!LINE || LINE == __LINE__) ? __builtin_ ## f : dummy_ ## f) + +/* Macro to verify that calls to __builtin_sprintf (i.e., with no size + argument) issue diagnostics by correctly determining the size of + the destination buffer. */ +#define T(size, ...) \ + (FUNC (sprintf) (buffer (size), __VA_ARGS__), \ + sink (buffer, ptr)) + +/* Return a signed integer in the range [MIN, MAX]. */ +#define R(min, max) int_range (min, max) + +/* Verify warnings and ranges for certain overflow. */ +void test_min_overflow (int i) +{ + T (0, "%#hho", i); /* { dg-warning "between 1 and 4 bytes" } */ + T (0, "%#1hho", i); /* { dg-warning "between 1 and 4 bytes" } */ + T (0, "%#2hho", i); /* { dg-warning "between 2 and 4 bytes" } */ + T (0, "%#3hho", i); /* { dg-warning "between 3 and 4 bytes" } */ + T (0, "%#4hho", i); /* { dg-warning "writing 4 bytes" } */ + T (0, "%#hho", R (-1, 0)); /* { dg-warning "between 1 and 4 bytes" } */ + T (0, "%#1hho", R (-1, 0)); /* { dg-warning "between 1 and 4 bytes" } */ + T (0, "%#2hho", R (-1, 0)); /* { dg-warning "between 2 and 4 bytes" } */ + T (0, "%#3hho", R (-1, 0)); /* { dg-warning "between 3 and 4 bytes" } */ + T (0, "%#4hho", R (-1, 0)); /* { dg-warning "writing 4 bytes" } */ + T (0, "%#hho", R (-1, 1)); /* { dg-warning "between 1 and 4 bytes" } */ + T (0, "%#1hho", R (-1, 1)); /* { dg-warning "between 1 and 4 bytes" } */ + T (0, "%#2hho", R (-1, 1)); /* { dg-warning "between 2 and 4 bytes" } */ + T (0, "%#3hho", R (-1, 1)); /* { dg-warning "between 3 and 4 bytes" } */ + T (0, "%#4hho", R (-1, 1)); /* { dg-warning "writing 4 bytes" } */ + T (0, "%#hho", R ( 0, 1)); /* { dg-warning "between 1 and 2 bytes" } */ + T (0, "%#1hho", R ( 0, 1)); /* { dg-warning "between 1 and 2 bytes" } */ + T (0, "%#2hho", R ( 0, 1)); /* { dg-warning "writing 2 bytes" } */ + T (0, "%#3hho", R ( 0, 1)); /* { dg-warning "writing 3 bytes" } */ + T (0, "%#4hho", R ( 0, 1)); /* { dg-warning "writing 4 bytes" } */ + T (0, "%#hho", R ( 1, 2)); /* { dg-warning "writing 2 bytes" } */ + T (0, "%#1hho", R ( 1, 2)); /* { dg-warning "writing 2 bytes" } */ + T (0, "%#2hho", R ( 1, 2)); /* { dg-warning "writing 2 bytes" } */ + T (0, "%#3hho", R ( 1, 2)); /* { dg-warning "writing 3 bytes" } */ + T (0, "%#4hho", R ( 1, 2)); /* { dg-warning "writing 4 bytes" } */ + + T (0, "%#ho", i); /* { dg-warning "between 1 and 7 bytes" } */ + T (0, "%#.*ho", /* { dg-warning "between 1 and 7 bytes" } */ + R (0, 2), i); + T (0, "%#.*ho", /* { dg-warning "between 1 and 7 bytes" } */ + R (1, 2), i); + T (0, "%#.*ho", /* { dg-warning "between 2 and 7 bytes" } */ + R (2, 3), i); + T (0, "%#.*ho", /* { dg-warning "between 3 and 7 bytes" } */ + R (3, 4), i); + T (0, "%#.*ho", /* { dg-warning "between 7 and 8 bytes" } */ + R (7, 8), i); + + T (0, "%#ho", R (-1, 0)); /* { dg-warning "between 1 and 7 bytes" } */ + T (0, "%#ho", R (-1, 1)); /* { dg-warning "between 1 and 7 bytes" } */ + T (0, "%#ho", R ( 0, 1)); /* { dg-warning "between 1 and 2 bytes" } */ + T (0, "%#ho", R ( 1, 2)); /* { dg-warning "writing 2 bytes" } */ + + T (0, "%#o", i); /* { dg-warning "between 1 and 12 bytes" } */ + T (0, "%#o", R (-1, 0)); /* { dg-warning "between 1 and 12 bytes" } */ + T (0, "%#o", R (-1, 1)); /* { dg-warning "between 1 and 12 bytes" } */ + T (0, "%#o", R ( 0, 1)); /* { dg-warning "between 1 and 2 bytes" } */ + T (0, "%#o", R ( 1, 2)); /* { dg-warning "writing 2 bytes" } */ + + T (0, "%#hhx", i); /* { dg-warning "between 1 and 4 bytes" } */ + T (0, "%#.*hhx", /* { dg-warning "writing up to 4 bytes" } */ + R (0, 2), i); + T (0, "%#.*hhx", /* { dg-warning "between 1 and 4 bytes" } */ + R (1, 2), i); + T (0, "%#.*hhx", /* { dg-warning "between 2 and 5 bytes" } */ + R (2, 3), i); + T (0, "%#.*hhx", /* { dg-warning "between 3 and 6 bytes" } */ + R (3, 4), i); + + T (0, "%#hhx", R (-1, 0)); /* { dg-warning "between 1 and 4 bytes" } */ + T (0, "%#hhx", R (-1, 1)); /* { dg-warning "between 1 and 4 bytes" } */ + T (0, "%#hhx", R ( 0, 1)); /* { dg-warning "between 1 and 3 bytes" } */ + T (0, "%#hhx", R ( 1, 2)); /* { dg-warning "writing 3 bytes" } */ + + T (0, "%#hx", i); /* { dg-warning "between 1 and 6 bytes" } */ + T (0, "%#hx", R (-1, 0)); /* { dg-warning "between 1 and 6 bytes" } */ + T (0, "%#hx", R (-1, 1)); /* { dg-warning "between 1 and 6 bytes" } */ + T (0, "%#hx", R ( 0, 1)); /* { dg-warning "between 1 and 3 bytes" } */ + T (0, "%#hx", R ( 1, 2)); /* { dg-warning "writing 3 bytes" } */ + + T (0, "%#x", i); /* { dg-warning "between 1 and 10 bytes" } */ + T (0, "%#x", R (-1, 0)); /* { dg-warning "between 1 and 10 bytes" } */ + T (0, "%#x", R (-1, 1)); /* { dg-warning "between 1 and 10 bytes" } */ + T (0, "%#x", R ( 0, 1)); /* { dg-warning "between 1 and 3 bytes" } */ + T (0, "%#x", R ( 1, 2)); /* { dg-warning "writing 3 bytes" } */ +} + +/* Verify warnings and ranges for likely overflow. */ +void test_likely_overflow (int i) +{ + T (2, "%#hho", i); /* { dg-warning "may write a terminating nul" } */ + T (2, "%#1hho", i); /* { dg-warning "may write a terminating nul" } */ + T (2, "%#2hho", i); /* { dg-warning "writing a terminating nul" } */ + T (2, "%#3hho", i); /* { dg-warning "between 3 and 4 bytes" } */ + T (2, "%#4hho", i); /* { dg-warning "writing 4 bytes" } */ + T (2, "%#hho", R (-1, 0)); /* { dg-warning "may write a terminating nul" } */ + T (2, "%#1hho", R (-1, 0));/* { dg-warning "may write a terminating nul" } */ + T (2, "%#2hho", R (-1, 0));/* { dg-warning "writing a terminating nul" } */ + T (2, "%#3hho", R (-1, 0));/* { dg-warning "between 3 and 4 bytes" } */ + T (2, "%#4hho", R (-1, 0));/* { dg-warning "writing 4 bytes" } */ + T (2, "%#hho", R (-1, 1)); /* { dg-warning "may write a terminating nul" } */ + T (2, "%#1hho", R (-1, 1));/* { dg-warning "may write a terminating nul" } */ + T (2, "%#2hho", R (-1, 1));/* { dg-warning "writing a terminating nul" } */ + T (2, "%#3hho", R (-1, 1));/* { dg-warning "between 3 and 4 bytes" } */ + T (2, "%#4hho", R (-1, 1));/* { dg-warning "writing 4 bytes" } */ + T (2, "%#hho", R ( 0, 1)); /* { dg-warning "may write a terminating nul" } */ + T (2, "%#1hho", R ( 0, 1));/* { dg-warning "may write a terminating nul" } */ + T (2, "%#2hho", R ( 0, 1));/* { dg-warning "writing a terminating nul" } */ + T (2, "%#3hho", R ( 0, 1));/* { dg-warning "writing 3 bytes" } */ + T (2, "%#4hho", R ( 0, 1));/* { dg-warning "writing 4 bytes" } */ + T (2, "%#hho", R ( 1, 2)); /* { dg-warning "writing a terminating nul" } */ + T (2, "%#1hho", R ( 1, 2));/* { dg-warning "writing a terminating nul" } */ + T (2, "%#2hho", R ( 1, 2));/* { dg-warning "writing a terminating nul" } */ + T (2, "%#3hho", R ( 1, 2));/* { dg-warning "writing 3 bytes" } */ + T (2, "%#4hho", R ( 1, 2));/* { dg-warning "writing 4 bytes" } */ + + T (2, "%#ho", i); /* { dg-warning "may write a terminating nul" } */ + T (2, "%#ho", R (-1, 0)); /* { dg-warning "may write a terminating nul" } */ + T (2, "%#ho", R (-1, 1)); /* { dg-warning "may write a terminating nul" } */ + T (2, "%#ho", R ( 0, 1)); /* { dg-warning "may write a terminating nul" } */ + T (2, "%#ho", R ( 1, 2)); /* { dg-warning "writing a terminating nul" } */ + + T (2, "%#o", i); /* { dg-warning "may write a terminating nul" } */ + T (2, "%#o", R (-1, 0)); /* { dg-warning "may write a terminating nul" } */ + T (2, "%#o", R (-1, 1)); /* { dg-warning "may write a terminating nul" } */ + T (2, "%#o", R ( 0, 1)); /* { dg-warning "may write a terminating nul" } */ + T (2, "%#o", R ( 1, 2)); /* { dg-warning "writing a terminating nul" } */ + + T (2, "%#hhx", i); /* { dg-warning "between 1 and 4 bytes" } */ + T (2, "%#1hhx", i); /* { dg-warning "between 1 and 4 bytes" } */ + T (2, "%#2hhx", i); /* { dg-warning "between 2 and 4 bytes" } */ + T (2, "%#3hhx", i); /* { dg-warning "between 3 and 4 bytes" } */ + T (2, "%#4hhx", i); /* { dg-warning "writing 4 bytes" } */ + T (2, "%#1hhx", R (-1, 0));/* { dg-warning "between 1 and 4 bytes" } */ + T (2, "%#2hhx", R (-1, 0));/* { dg-warning "between 2 and 4 bytes" } */ + T (2, "%#3hhx", R (-1, 0));/* { dg-warning "between 3 and 4 bytes" } */ + T (2, "%#4hhx", R (-1, 0));/* { dg-warning "writing 4 bytes" } */ + T (2, "%#hhx", R (-1, 0)); /* { dg-warning "between 1 and 4 bytes" } */ + T (2, "%#1hhx", R (-1, 0));/* { dg-warning "between 1 and 4 bytes" } */ + T (2, "%#2hhx", R (-1, 0));/* { dg-warning "between 2 and 4 bytes" } */ + T (2, "%#3hhx", R (-1, 0));/* { dg-warning "between 3 and 4 bytes" } */ + T (2, "%#4hhx", R (-1, 0));/* { dg-warning "writing 4 bytes" } */ + T (2, "%#hhx", R (-1, 1)); /* { dg-warning "between 1 and 4 bytes" } */ + T (2, "%#1hhx", R (-1, 1));/* { dg-warning "between 1 and 4 bytes" } */ + T (2, "%#2hhx", R (-1, 1));/* { dg-warning "between 2 and 4 bytes" } */ + T (2, "%#3hhx", R (-1, 1));/* { dg-warning "between 3 and 4 bytes" } */ + T (2, "%#4hhx", R (-1, 1));/* { dg-warning "writing 4 bytes" } */ + T (2, "%#hhx", R ( 0, 1)); /* { dg-warning "between 1 and 3 bytes" } */ + T (2, "%#1hhx", R ( 0, 1));/* { dg-warning "between 1 and 3 bytes" } */ + T (2, "%#2hhx", R ( 0, 1));/* { dg-warning "between 2 and 3 bytes" } */ + T (2, "%#3hhx", R ( 0, 1));/* { dg-warning "writing 3 bytes" } */ + T (2, "%#4hhx", R ( 0, 1));/* { dg-warning "writing 4 bytes" } */ + T (2, "%#hhx", R ( 1, 2)); /* { dg-warning "writing 3 bytes" } */ + T (2, "%#1hhx", R ( 1, 2));/* { dg-warning "writing 3 bytes" } */ + T (2, "%#2hhx", R ( 1, 2));/* { dg-warning "writing 3 bytes" } */ + T (2, "%#3hhx", R ( 1, 2));/* { dg-warning "writing 3 bytes" } */ + T (2, "%#4hhx", R ( 1, 2));/* { dg-warning "writing 4 bytes" } */ + + T (2, "%#hx", i); /* { dg-warning "between 1 and 6 bytes" } */ + T (2, "%#hx", R (-1, 0)); /* { dg-warning "between 1 and 6 bytes" } */ + T (2, "%#hx", R (-1, 1)); /* { dg-warning "between 1 and 6 bytes" } */ + T (2, "%#hx", R ( 0, 1)); /* { dg-warning "between 1 and 3 bytes" } */ + T (2, "%#hx", R ( 1, 2)); /* { dg-warning "writing 3 bytes" } */ + + T (2, "%#x", i); /* { dg-warning "between 1 and 10 bytes" } */ + T (2, "%#x", R (-1, 0)); /* { dg-warning "between 1 and 10 bytes" } */ + T (2, "%#x", R (-1, 1)); /* { dg-warning "between 1 and 10 bytes" } */ + T (2, "%#x", R ( 0, 1)); /* { dg-warning "between 1 and 3 bytes" } */ + T (2, "%#x", R ( 1, 2)); /* { dg-warning "writing 3 bytes" } */ +} + +/* Verify warnings likely overflow due to the terminating nul. */ +void test_likely_nul_overflow (int i) +{ + T (3, "%#hho", i); + T (3, "%#hho", R (-1, 0)); + T (3, "%#hho", R (-1, 1)); + T (3, "%#hho", R ( 0, 1)); + T (3, "%#hho", R ( 1, 2)); + + T (3, "%#ho", i); + T (3, "%#ho", R (-1, 0)); + T (3, "%#ho", R (-1, 1)); + T (3, "%#ho", R ( 0, 1)); + T (3, "%#ho", R ( 1, 2)); + + T (3, "%#o", i); + T (3, "%#o", R (-1, 0)); + T (3, "%#o", R (-1, 1)); + T (3, "%#o", R ( 0, 1)); + T (3, "%#o", R ( 1, 2)); + + T (3, "%#hhx", i); /* { dg-warning "may write a terminating nul" } */ + T (3, "%#hhx", R (-1, 0)); /* { dg-warning "may write a terminating nul" } */ + T (3, "%#hhx", R (-1, 1)); /* { dg-warning "may write a terminating nul" } */ + T (3, "%#hhx", R ( 0, 1)); /* { dg-warning "may write a terminating nul" } */ + T (3, "%#hhx", R ( 1, 2)); /* { dg-warning "writing a terminating nul" } */ + + T (3, "%#hx", i); /* { dg-warning "may write a terminating nul" } */ + T (3, "%#hx", R (-1, 0)); /* { dg-warning "may write a terminating nul" } */ + T (3, "%#hx", R (-1, 1)); /* { dg-warning "may write a terminating nul" } */ + T (3, "%#hx", R ( 0, 1)); /* { dg-warning "may write a terminating nul" } */ + T (3, "%#hx", R ( 1, 2)); /* { dg-warning "writing a terminating nul" } */ + + T (3, "%#x", i); /* { dg-warning "may write a terminating nul" } */ + T (3, "%#x", R (-1, 0)); /* { dg-warning "may write a terminating nul" } */ + T (3, "%#x", R (-1, 1)); /* { dg-warning "may write a terminating nul" } */ + T (3, "%#x", R ( 0, 1)); /* { dg-warning "may write a terminating nul" } */ + T (3, "%#x", R ( 1, 2)); /* { dg-warning "writing a terminating nul" } */ +} Index: gcc/testsuite/gcc.dg/tree-ssa/pr79327-2.c =================================================================== --- gcc/testsuite/gcc.dg/tree-ssa/pr79327-2.c (revision 0) +++ gcc/testsuite/gcc.dg/tree-ssa/pr79327-2.c (working copy) @@ -0,0 +1,158 @@ +/* { dg-compile } + { dg-options "-O2 -fdump-tree-optimized" } */ + +#define CAT(s, n) s ## n +#define FAIL(line) CAT (failure_on_line_, line) + +/* Emit a call to a function named failure_on_line_NNN when EXPR is false. */ +#define ASSERT(expr) \ + do { \ + extern void FAIL (__LINE__)(void); \ + if (!(expr)) FAIL (__LINE__)(); \ + } while (0) + +#define KEEP(line) CAT (keep_call_on_line_, line) + +/* Emit a call to a function named keep_call_on_line_NNN when EXPR is true. + Used to verify that the expression need not be the only one that holds. */ +#define ASSERT_MAYBE(expr) \ + do { \ + extern void KEEP (__LINE__)(void); \ + if (expr) KEEP (__LINE__)(); \ + } while (0) + +void test_hho_cst (void) +{ + ASSERT (1 == __builtin_snprintf (0, 0, "%#.1hho", 0)); + ASSERT (2 == __builtin_snprintf (0, 0, "%#.2hho", 0)); + + ASSERT (2 == __builtin_snprintf (0, 0, "%#.1hho", 1)); + ASSERT (2 == __builtin_snprintf (0, 0, "%#.2hho", 1)); + ASSERT (3 == __builtin_snprintf (0, 0, "%#.3hho", 1)); +} + +int test_hho_var (int i) +{ + int n = __builtin_snprintf (0, 0, "%#hho", i); + + ASSERT (0 < n && n < 5); + + ASSERT_MAYBE (1 == n); + ASSERT_MAYBE (2 == n); + ASSERT_MAYBE (3 == n); + ASSERT_MAYBE (4 == n); + + return n; +} + +int test_ho_var (int i) +{ + int n = __builtin_snprintf (0, 0, "%#ho", i); + + ASSERT (0 < n && n < 8); + + ASSERT_MAYBE (1 == n); + ASSERT_MAYBE (2 == n); + ASSERT_MAYBE (3 == n); + ASSERT_MAYBE (4 == n); + ASSERT_MAYBE (5 == n); + ASSERT_MAYBE (6 == n); + ASSERT_MAYBE (7 == n); + + return n; +} + +int test_o_var (int i) +{ + int n = __builtin_snprintf (0, 0, "%#o", i); + + ASSERT (0 < n && n < 13); + + ASSERT_MAYBE (1 == n); + ASSERT_MAYBE (2 == n); + ASSERT_MAYBE (3 == n); + ASSERT_MAYBE (4 == n); + ASSERT_MAYBE (5 == n); + ASSERT_MAYBE (6 == n); + ASSERT_MAYBE (7 == n); + ASSERT_MAYBE (8 == n); + ASSERT_MAYBE (9 == n); + ASSERT_MAYBE (10 == n); + ASSERT_MAYBE (11 == n); + ASSERT_MAYBE (12 == n); + + return n; +} + +void test_hhx_cst (void) +{ + ASSERT (1 == __builtin_snprintf (0, 0, "%#.1hhx", 0)); + ASSERT (2 == __builtin_snprintf (0, 0, "%#.2hhx", 0)); + + ASSERT (3 == __builtin_snprintf (0, 0, "%#.1hhx", 1)); + ASSERT (4 == __builtin_snprintf (0, 0, "%#.2hhx", 1)); + ASSERT (5 == __builtin_snprintf (0, 0, "%#.3hhx", 1)); +} + +int test_hhx_var (int i) +{ + int n = __builtin_snprintf (0, 0, "%#hhx", i); + + ASSERT (0 < n && n < 5); + + ASSERT_MAYBE (1 == n); + ASSERT_MAYBE (2 == n); + ASSERT_MAYBE (3 == n); + ASSERT_MAYBE (4 == n); + + return n; +} + +void test_hx_cst (void) +{ + ASSERT (1 == __builtin_snprintf (0, 0, "%#.1hx", 0)); + ASSERT (2 == __builtin_snprintf (0, 0, "%#.2hx", 0)); + + ASSERT (3 == __builtin_snprintf (0, 0, "%#.1hx", 1)); + ASSERT (4 == __builtin_snprintf (0, 0, "%#.2hx", 1)); + ASSERT (5 == __builtin_snprintf (0, 0, "%#.3hx", 1)); +} + +int test_hx_var (int i) +{ + int n = __builtin_snprintf (0, 0, "%#hx", i); + + ASSERT (0 < n && n < 7); + + ASSERT_MAYBE (1 == n); + ASSERT_MAYBE (2 == n); + ASSERT_MAYBE (3 == n); + ASSERT_MAYBE (4 == n); + ASSERT_MAYBE (5 == n); + ASSERT_MAYBE (6 == n); + + return n; +} + +int test_x_var (int i) +{ + int n = __builtin_snprintf (0, 0, "%#x", i); + + ASSERT (0 < n && n < 11); + + ASSERT_MAYBE (1 == n); + ASSERT_MAYBE (2 == n); + ASSERT_MAYBE (3 == n); + ASSERT_MAYBE (4 == n); + ASSERT_MAYBE (5 == n); + ASSERT_MAYBE (6 == n); + ASSERT_MAYBE (7 == n); + ASSERT_MAYBE (8 == n); + ASSERT_MAYBE (9 == n); + ASSERT_MAYBE (10 == n); + + return n; +} + +/* { dg-final { scan-tree-dump-not "failure_on_line" "optimized"} } + { dg-final { scan-tree-dump-times "keep_call_on_line" 43 "optimized"} } */ ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH] use zero as the lower bound for a signed-unsigned range (PR 79327) 2017-02-03 0:31 ` Martin Sebor 2017-02-03 0:48 ` Jakub Jelinek 2017-02-03 5:37 ` Martin Sebor @ 2017-02-03 19:02 ` Jeff Law 2017-02-03 19:08 ` Martin Sebor 2 siblings, 1 reply; 36+ messages in thread From: Jeff Law @ 2017-02-03 19:02 UTC (permalink / raw) To: Martin Sebor; +Cc: Jakub Jelinek, Gcc Patch List On 02/02/2017 05:31 PM, Martin Sebor wrote: >>>> - T (2, "%#hho", a); /* { dg-warning "nul past the end" >>>> } */ >>>> - T (2, "%#hhx", a); /* { dg-warning ".%#hhx. directive >>>> writing between 3 and . bytes into a region of size 2" } */ >>>> + T (2, "%#hho", a); >>>> + T (2, "%#hhx", a); >> >> On reflection, this isn't quite the right fix. We want to both set >> the correct range and warn because the call will likely overflow. >> This is an example of why the likely/unlikely counters have been >> introduced. By setting min = 1 and likely = 2 for the %#hho and >> 3 for the %#hhx we get the desired result. > > Attached is a simple patch that removes the vestigial setting of > the minimum counter while preserving the warnings above by using > the likely counter. > > I had overlooked this when I introduced the likely counter and so > in the corner cases of "%#o" and "%#x" with a non-constant argument > that could be zero, the minimum counter would be set to 2 and 3 > respectively rather than 1 (because zero is formatted without > the '0' or '0x' base prefix). This patch almost certainly conflicts with Jakub's. But I think if anything it may get simpler after Jakub applies his patch. Jakub, if you want to do the updates and commit after your patch so they can both get into any potential weekend gcc spin for Fedora, go right ahead :-) Otherwise it's good to go for Martin after making the minor updates. jeff ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH] use zero as the lower bound for a signed-unsigned range (PR 79327) 2017-02-03 19:02 ` Jeff Law @ 2017-02-03 19:08 ` Martin Sebor 0 siblings, 0 replies; 36+ messages in thread From: Martin Sebor @ 2017-02-03 19:08 UTC (permalink / raw) To: Jeff Law; +Cc: Jakub Jelinek, Gcc Patch List On 02/03/2017 12:02 PM, Jeff Law wrote: > On 02/02/2017 05:31 PM, Martin Sebor wrote: >>>>> - T (2, "%#hho", a); /* { dg-warning "nul past the end" >>>>> } */ >>>>> - T (2, "%#hhx", a); /* { dg-warning ".%#hhx. directive >>>>> writing between 3 and . bytes into a region of size 2" } */ >>>>> + T (2, "%#hho", a); >>>>> + T (2, "%#hhx", a); >>> >>> On reflection, this isn't quite the right fix. We want to both set >>> the correct range and warn because the call will likely overflow. >>> This is an example of why the likely/unlikely counters have been >>> introduced. By setting min = 1 and likely = 2 for the %#hho and >>> 3 for the %#hhx we get the desired result. >> >> Attached is a simple patch that removes the vestigial setting of >> the minimum counter while preserving the warnings above by using >> the likely counter. >> >> I had overlooked this when I introduced the likely counter and so >> in the corner cases of "%#o" and "%#x" with a non-constant argument >> that could be zero, the minimum counter would be set to 2 and 3 >> respectively rather than 1 (because zero is formatted without >> the '0' or '0x' base prefix). > This patch almost certainly conflicts with Jakub's. But I think if > anything it may get simpler after Jakub applies his patch. > > Jakub, if you want to do the updates and commit after your patch so they > can both get into any potential weekend gcc spin for Fedora, go right > ahead :-) > > Otherwise it's good to go for Martin after making the minor updates. Let's let Jakub go first so he can be done with his day/week. I'll deal with the conflicts and retest everything. Martin ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH] use zero as the lower bound for a signed-unsigned range (PR 79327) 2017-02-02 16:58 ` Martin Sebor 2017-02-02 19:59 ` Martin Sebor @ 2017-02-02 22:15 ` Jakub Jelinek 2017-02-03 18:53 ` Jeff Law 2017-02-03 7:35 ` Jeff Law 2 siblings, 1 reply; 36+ messages in thread From: Jakub Jelinek @ 2017-02-02 22:15 UTC (permalink / raw) To: Martin Sebor; +Cc: Jeff Law, Gcc Patch List Hi! Note, the second patch I've posted passed bootstrap/regtest on both x86_64-linux and i686-linux. On Thu, Feb 02, 2017 at 09:58:06AM -0700, Martin Sebor wrote: > > int > > main (void) > > { > > int i; > > char buf[64]; > > if (__builtin_sprintf (buf, "%#hho", a) > 1) > > __builtin_abort (); > > if (__builtin_sprintf (buf, "%#hhx", a) > 1) > > __builtin_abort (); > > return 0; > > } > > Current trunk as well as trunk + your patch computes ranges [2, 4] and > > [3, 4], but the correct ranges are [1, 4] and [1, 4], because for a 0 > > (or say -131072 etc.) it sets buf to "0" in both cases. > > That's right. This is a good find. This case is being tested in > builtin-sprintf-warn-1.c but the assertions (on lines 1154 and > 1155) seem to expect the wrong numbers. I would expect your fix > to cause failures here. (And now that I've read the rest of the > patch I see it does.) That testcase is derived from the builtin-sprintf-warn-1.c:115{4,5} FAILs I got with the patch + analysis (and included in the second patch in the testsuite as executable testcase too). > The range in the note is the artificial one the pass uses to compute > the range of output. I went back and forth on this as I think it's > debatable if [-128, 127] is clearer than [1, -128] (or [-128, 1]). > The informative notes aren't completely consistent in what ranges > they print. It would be nice to nail down what we think is the most > useful output and make them all consistent. You say in the wording range and print it as a range, then it really should be a range, and not an artificial one, but one reflecting actual possible values. If the user knows that the variable can have values -123 and 126 at that point and you still print [1, -128] or [-128, 1] as range, the users will just think the compiler is buggy and disable the warning. > > Plus there are certain notes removed: > > -builtin-sprintf-warn-1.c:1311:3: note: using the range [1, -2147483648] for directive argument > > T (0, "%d", a); /* { dg-warning "into a region" } */ > > without replacement, here a has [-2147483648, 2147483647] range, but as that > > is equal to the type's range, I bet it omits it. > > Could be. As I said, there are opportunities for improvements in > what the notes print. Someone pointed out, for example, that very > large numbers are hard to read. It might be clearer to print some > of them using constants like LONG_MAX - 2, or in hex, etc. The *_MAX or hex is a separate issue, that can be done or doesn't have to be done, the values are simply the same. But picking some random numbers in the ranges is just wrong. > > - tree dirmin = TYPE_MIN_VALUE (dirtype); > > - tree dirmax = TYPE_MAX_VALUE (dirtype); > > - > > - if (TYPE_UNSIGNED (dirtype)) > > - { > > - *argmin = dirmin; > > - *argmax = dirmax; > > - } > > - else > > - { > > - *argmin = integer_zero_node; > > - *argmax = dirmin; > > - } > > - > > + *argmin = TYPE_MIN_VALUE (dirtype); > > + *argmax = TYPE_MAX_VALUE (dirtype); > > This is likely behind the changes in the notes you pointed out above. > The code here is intentional to reflect the range synthesized by > the pass to compute the output. I don't have a big issue with The change I've done was completely intentional, it is again mixing of value ranges with already premature guessing on what values are shorter or longer. > As for the rest of the patch, while I appreciate your help and > acknowledge it's not my call to make I'm not entirely comfortable > with this much churn at this stage. My preference would to just > fix the bugs and do the restructuring in stage 1. The thing is, right now we have 3 independent but intermixed code paths, one for arguments with VR_RANGE that doesn't need conversion for dirtype (then argmin/argmax until very lately is actual range and the shortest/longest computation is done at the latest point), then VR_RANGE that does need conversion for dirtype (that one is prematurely partly converted to the shortest/longest above), and then another one that handles the non-VR_RANGE, which commits to the shortest/longest immediately. What my patch does is that it unifies all the 3 paths on essentially what the first path does. If we don't apply that patch, we are going to have 3 times as many possibilities for bugs; you will need to add some hack to get the above mentioned wrong-code fixed, and IMNSHO another hack to get the bogus printed ranges fixed. It is always better to remove code than to add it. Jakub ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH] use zero as the lower bound for a signed-unsigned range (PR 79327) 2017-02-02 22:15 ` Jakub Jelinek @ 2017-02-03 18:53 ` Jeff Law 0 siblings, 0 replies; 36+ messages in thread From: Jeff Law @ 2017-02-03 18:53 UTC (permalink / raw) To: Jakub Jelinek, Martin Sebor; +Cc: Gcc Patch List On 02/02/2017 03:15 PM, Jakub Jelinek wrote: > >>> Plus there are certain notes removed: >>> -builtin-sprintf-warn-1.c:1311:3: note: using the range [1, -2147483648] for directive argument >>> T (0, "%d", a); /* { dg-warning "into a region" } */ >>> without replacement, here a has [-2147483648, 2147483647] range, but as that >>> is equal to the type's range, I bet it omits it. >> >> Could be. As I said, there are opportunities for improvements in >> what the notes print. Someone pointed out, for example, that very >> large numbers are hard to read. It might be clearer to print some >> of them using constants like LONG_MAX - 2, or in hex, etc. > > The *_MAX or hex is a separate issue, that can be done or doesn't have to be > done, the values are simply the same. But picking some random numbers in > the ranges is just wrong. Agreed. Further refinements in this area should be distinct patches from the ones currently under consideration. One of the high level things that worries me is the level of patch dependencies I see as we work through the issues in this code. That is part of what led to the mega patch that I asked Martin to break down into pieces, and Martin has indicated levels of pain around keeping patches up-to-date WRT the trunk sources. This is often an indication we went forward with the original work too quickly. I'll take the heat on gating this stuff in too quickly. It happens once in a while (I did the same with the IPA ICF code a couple years back). It's a manageable problem as long as we're aware of and work to avoid dependencies. Jeff ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH] use zero as the lower bound for a signed-unsigned range (PR 79327) 2017-02-02 16:58 ` Martin Sebor 2017-02-02 19:59 ` Martin Sebor 2017-02-02 22:15 ` Jakub Jelinek @ 2017-02-03 7:35 ` Jeff Law 2017-02-03 16:13 ` Martin Sebor 2 siblings, 1 reply; 36+ messages in thread From: Jeff Law @ 2017-02-03 7:35 UTC (permalink / raw) To: Martin Sebor, Jakub Jelinek; +Cc: Gcc Patch List On 02/02/2017 09:58 AM, Martin Sebor wrote: >> Otherwise all the tests succeeded, though looking e.g. at the diff >> between builtin-sprintf-warn-1.c diagnostics with your patch and with >> the patch below instead, there are also differences like: >> -builtin-sprintf-warn-1.c:1119:3: note: using the range [1, -128] for >> directive argument >> +builtin-sprintf-warn-1.c:1119:3: note: using the range [-128, 127] >> for directive argument >> where [1, -128] looks clearly wrong, that isn't a valid range, > > The range in the note is the artificial one the pass uses to compute > the range of output. I went back and forth on this as I think it's > debatable if [-128, 127] is clearer than [1, -128] (or [-128, 1]). > The informative notes aren't completely consistent in what ranges > they print. It would be nice to nail down what we think is the most > useful output and make them all consistent. I don't think there's any argument against moving towards consistency, but I would well see folks not agreeing on which consistent style is preferable. I do think that we should strive to print ranges in the same format that we use to describe ranges internally. So a range like [1, -128] is not a valid range. That may argue against using the artificial range representation in the output. > > As for the rest of the patch, while I appreciate your help and > acknowledge it's not my call to make I'm not entirely comfortable > with this much churn at this stage. My preference would to just > fix the bugs and do the restructuring in stage 1. I'm really torn here. I'm keen to raise the bar in terms of what we're willing to do for gcc-7. But I'm also keen to have the codebase in a reasonable state that will allow for the ongoing maintenance of the gcc-7 branch. The sprintf stuff is fairly dense and there's almost certainly more dusty corner case issues we're going to have to work through. Thus we stand to be in a better state to maintain the code if we can do some refactoring. The fact that Jakub, one of the release managers, is proposing the change carries a lot of weight in terms of trying to make a decision here. Similarly your weight as the author of this code carries a lot of weight. Leaving us without a clearly preferable path. Jeff ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH] use zero as the lower bound for a signed-unsigned range (PR 79327) 2017-02-03 7:35 ` Jeff Law @ 2017-02-03 16:13 ` Martin Sebor 2017-02-03 16:39 ` Jakub Jelinek 0 siblings, 1 reply; 36+ messages in thread From: Martin Sebor @ 2017-02-03 16:13 UTC (permalink / raw) To: Jeff Law, Jakub Jelinek; +Cc: Gcc Patch List On 02/03/2017 12:35 AM, Jeff Law wrote: > On 02/02/2017 09:58 AM, Martin Sebor wrote: > >>> Otherwise all the tests succeeded, though looking e.g. at the diff >>> between builtin-sprintf-warn-1.c diagnostics with your patch and with >>> the patch below instead, there are also differences like: >>> -builtin-sprintf-warn-1.c:1119:3: note: using the range [1, -128] for >>> directive argument >>> +builtin-sprintf-warn-1.c:1119:3: note: using the range [-128, 127] >>> for directive argument >>> where [1, -128] looks clearly wrong, that isn't a valid range, >> >> The range in the note is the artificial one the pass uses to compute >> the range of output. I went back and forth on this as I think it's >> debatable if [-128, 127] is clearer than [1, -128] (or [-128, 1]). >> The informative notes aren't completely consistent in what ranges >> they print. It would be nice to nail down what we think is the most >> useful output and make them all consistent. > I don't think there's any argument against moving towards consistency, > but I would well see folks not agreeing on which consistent style is > preferable. > > I do think that we should strive to print ranges in the same format that > we use to describe ranges internally. So a range like [1, -128] is not > a valid range. That may argue against using the artificial range > representation in the output. > > > > >> >> As for the rest of the patch, while I appreciate your help and >> acknowledge it's not my call to make I'm not entirely comfortable >> with this much churn at this stage. My preference would to just >> fix the bugs and do the restructuring in stage 1. > I'm really torn here. I'm keen to raise the bar in terms of what we're > willing to do for gcc-7. But I'm also keen to have the codebase in a > reasonable state that will allow for the ongoing maintenance of the > gcc-7 branch. > > The sprintf stuff is fairly dense and there's almost certainly more > dusty corner case issues we're going to have to work through. Thus we > stand to be in a better state to maintain the code if we can do some > refactoring. > > The fact that Jakub, one of the release managers, is proposing the > change carries a lot of weight in terms of trying to make a decision > here. Similarly your weight as the author of this code carries a lot of > weight. Leaving us without a clearly preferable path. I'm not opposed to the changes, certainly not to cleaning things up, but I don't think now is the time to be making these tweaks. Jakub's patch is fine with me without those tweaks, and with the correction to keep the warning (and fix the octal base prefix) that I posted in the followup patch. (The followup patch is also necessary to avoid incorrect optimization.) Regarding the printed ranges: for integer arguments the pass prints one of two sets: 1) either the range the argument supplied by the caller is known to be in, or 2) the range synthesized by the pass for an argument in an unknown range, or after conversion to the type of the directive The notes distinguish between these two by using the two phrases: 1) directive argument in the range [MIN, MAX] 2) using the range [MIN, MAX] for directive argument I suspect this isn't entirely consistent with all the recent changes but that's the idea behind it. It's subtle and I'm not surprised Jakub missed it. I'm not opposed to changing this to make it more intuitive or useful but again, I would rather not spend time on it now. Instead, I would prefer to discuss what we want after the dust from the release has settled and implement a consistent solution in GCC 8. Martin ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH] use zero as the lower bound for a signed-unsigned range (PR 79327) 2017-02-03 16:13 ` Martin Sebor @ 2017-02-03 16:39 ` Jakub Jelinek 2017-02-03 17:02 ` Martin Sebor ` (2 more replies) 0 siblings, 3 replies; 36+ messages in thread From: Jakub Jelinek @ 2017-02-03 16:39 UTC (permalink / raw) To: Martin Sebor; +Cc: Jeff Law, Gcc Patch List On Fri, Feb 03, 2017 at 09:13:23AM -0700, Martin Sebor wrote: > I'm not opposed to the changes, certainly not to cleaning things up, > but I don't think now is the time to be making these tweaks. Jakub's > patch is fine with me without those tweaks, and with the correction What do you mean by my patch without those tweaks? The intent of the patch is not just fix the diagnostics and wrong-code issue, but primarily that any further needed fix will need to tweak just a single spot, not many, otherwise e.g. you need to have sufficient testsuite coverage for all those paths. Say in the http://gcc.gnu.org/ml/gcc-patches/2017-02/msg00234.html patch, you would with my patch need just the tree_digits part, and then the very last hunk in gimple-ssa-sprintf.c (with likely_adjust && removed). Because you do the adjustments only if !res.knownrange and in that case you know argmin/argmax are actually dirtype's min/max, so 0 must be in the range. > to keep the warning (and fix the octal base prefix) that I posted > in the followup patch. (The followup patch is also necessary to > avoid incorrect optimization.) > > Regarding the printed ranges: for integer arguments the pass prints > one of two sets: > > 1) either the range the argument supplied by the caller is known to > be in, or > 2) the range synthesized by the pass for an argument in an unknown > range, or after conversion to the type of the directive > > The notes distinguish between these two by using the two phrases: > > 1) directive argument in the range [MIN, MAX] > > 2) using the range [MIN, MAX] for directive argument > > I suspect this isn't entirely consistent with all the recent changes > but that's the idea behind it. It's subtle and I'm not surprised > Jakub missed it. You still print as range [MIN, MAX] something that is in no way a range, just some values picked from the range. > I'm not opposed to changing this to make it more intuitive or useful > but again, I would rather not spend time on it now. Instead, I would > prefer to discuss what we want after the dust from the release has > settled and implement a consistent solution in GCC 8. I fear this isn't the last fix needed, the code is very complex and not sufficiently covered by testcases, so if we don't want to make the code more maintainable now, I'd be strongly for just turning -fprintf-return-value off by default for the release. Bogus warnings can be lived with or worked around, silent wrong-code is much worse. Jakub ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH] use zero as the lower bound for a signed-unsigned range (PR 79327) 2017-02-03 16:39 ` Jakub Jelinek @ 2017-02-03 17:02 ` Martin Sebor 2017-02-03 17:17 ` Jakub Jelinek 2017-02-03 17:49 ` Jeff Law 2017-02-04 8:07 ` Jakub Jelinek 2 siblings, 1 reply; 36+ messages in thread From: Martin Sebor @ 2017-02-03 17:02 UTC (permalink / raw) To: Jakub Jelinek; +Cc: Jeff Law, Gcc Patch List On 02/03/2017 09:39 AM, Jakub Jelinek wrote: > On Fri, Feb 03, 2017 at 09:13:23AM -0700, Martin Sebor wrote: >> I'm not opposed to the changes, certainly not to cleaning things up, >> but I don't think now is the time to be making these tweaks. Jakub's >> patch is fine with me without those tweaks, and with the correction > > What do you mean by my patch without those tweaks? ... > I fear this isn't the last fix needed, the code is very complex and not > sufficiently covered by testcases, so if we don't want to make the code > more maintainable now, I'd be strongly for just turning > -fprintf-return-value off by default for the release. Bogus warnings > can be lived with or worked around, silent wrong-code is much worse. I mean without changes to the content of the notes. If you feel it important to simplify the code now that's okay with me. I'm not empowered to either approve or reject any changes so what say is obviously just my input into Jeff's decision. I also care a lot about this work so I'm not going to resist when faced with a threat of having it disabled. If leaving the optimization enabled is conditional on it then feel free to make whatever changes you see fit. Martin ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH] use zero as the lower bound for a signed-unsigned range (PR 79327) 2017-02-03 17:02 ` Martin Sebor @ 2017-02-03 17:17 ` Jakub Jelinek 0 siblings, 0 replies; 36+ messages in thread From: Jakub Jelinek @ 2017-02-03 17:17 UTC (permalink / raw) To: Martin Sebor; +Cc: Jeff Law, Gcc Patch List On Fri, Feb 03, 2017 at 10:02:48AM -0700, Martin Sebor wrote: > On 02/03/2017 09:39 AM, Jakub Jelinek wrote: > > On Fri, Feb 03, 2017 at 09:13:23AM -0700, Martin Sebor wrote: > > > I'm not opposed to the changes, certainly not to cleaning things up, > > > but I don't think now is the time to be making these tweaks. Jakub's > > > patch is fine with me without those tweaks, and with the correction > > > > What do you mean by my patch without those tweaks? > ... > > I fear this isn't the last fix needed, the code is very complex and not > > sufficiently covered by testcases, so if we don't want to make the code > > more maintainable now, I'd be strongly for just turning > > -fprintf-return-value off by default for the release. Bogus warnings > > can be lived with or worked around, silent wrong-code is much worse. > > I mean without changes to the content of the notes. If you feel > it important to simplify the code now that's okay with me. I haven't changed anything in order to change the notes, that is just the side-effect of arg{min,max} to always mean the range boundaries, and only res. If you want to print what values the pass considered as shortest or longest output, then it would need to remember (in other named fields) those values that it picked and use suitable wording to tell those values to the user. I think the ranges are very desirable to be printed in any cases, and if further info is added, if it doesn't clutter the message too much, why not. > I'm not empowered to either approve or reject any changes so what > say is obviously just my input into Jeff's decision. I also care I'm not a maintainer of that part of GCC, so I can only wait for some reviewer or maintainer too. > a lot about this work so I'm not going to resist when faced with > a threat of having it disabled. If leaving the optimization > enabled is conditional on it then feel free to make whatever > changes you see fit. Leaving the optimization enabled by default should be dependent on 1) what value it brings 2) what are the risks (how much we can trust it not to have other silent wrong-code issues lurking in). That decision is of course again something some reviewer would need to ack if I were to propose it, but with Fedora gcc package maintainer hat on, that is an option I'm seriously considering at least for the upcoming non-test mass rebuild, because we have at most days to resolve stuff. Jakub ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH] use zero as the lower bound for a signed-unsigned range (PR 79327) 2017-02-03 16:39 ` Jakub Jelinek 2017-02-03 17:02 ` Martin Sebor @ 2017-02-03 17:49 ` Jeff Law 2017-02-04 8:07 ` Jakub Jelinek 2 siblings, 0 replies; 36+ messages in thread From: Jeff Law @ 2017-02-03 17:49 UTC (permalink / raw) To: Jakub Jelinek, Martin Sebor; +Cc: Gcc Patch List On 02/03/2017 09:39 AM, Jakub Jelinek wrote: > On Fri, Feb 03, 2017 at 09:13:23AM -0700, Martin Sebor wrote: >> I'm not opposed to the changes, certainly not to cleaning things up, >> but I don't think now is the time to be making these tweaks. Jakub's >> patch is fine with me without those tweaks, and with the correction > > What do you mean by my patch without those tweaks? > The intent of the patch is not just fix the diagnostics and > wrong-code issue, but primarily that any further needed fix will need > to tweak just a single spot, not many, otherwise e.g. you need to have > sufficient testsuite coverage for all those paths. > Say in the http://gcc.gnu.org/ml/gcc-patches/2017-02/msg00234.html > patch, you would with my patch need just the tree_digits part, > and then the very last hunk in gimple-ssa-sprintf.c (with > likely_adjust && > removed). Because you do the adjustments only if !res.knownrange > and in that case you know argmin/argmax are actually dirtype's min/max, > so 0 must be in the range. Right. It's a combination of 3 things in Jakub patch. FIx the diagnostics, fix the codegen issue and refactoring to make the ongoing maintenance easier. >> I'm not opposed to changing this to make it more intuitive or useful >> but again, I would rather not spend time on it now. Instead, I would >> prefer to discuss what we want after the dust from the release has >> settled and implement a consistent solution in GCC 8. > > I fear this isn't the last fix needed, the code is very complex and not > sufficiently covered by testcases, so if we don't want to make the code > more maintainable now, I'd be strongly for just turning > -fprintf-return-value off by default for the release. Bogus warnings > can be lived with or worked around, silent wrong-code is much worse. I also expect we're going to be in this code more through the gcc-7 process and that's the primary reason why I'm considering the refactoring aspects of Jakub's patch. Normally I would have nixed those changes as inappropriate at this stage. Jeff ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH] use zero as the lower bound for a signed-unsigned range (PR 79327) 2017-02-03 16:39 ` Jakub Jelinek 2017-02-03 17:02 ` Martin Sebor 2017-02-03 17:49 ` Jeff Law @ 2017-02-04 8:07 ` Jakub Jelinek 2017-02-06 19:45 ` Jakub Jelinek 2017-02-14 0:15 ` Jeff Law 2 siblings, 2 replies; 36+ messages in thread From: Jakub Jelinek @ 2017-02-04 8:07 UTC (permalink / raw) To: Martin Sebor, Jeff Law; +Cc: Gcc Patch List On Fri, Feb 03, 2017 at 05:39:21PM +0100, Jakub Jelinek wrote: > Say in the http://gcc.gnu.org/ml/gcc-patches/2017-02/msg00234.html > patch, you would with my patch need just the tree_digits part, > and then the very last hunk in gimple-ssa-sprintf.c (with > likely_adjust && > removed). Because you do the adjustments only if !res.knownrange > and in that case you know argmin/argmax are actually dirtype's min/max, > so 0 must be in the range. You've committed the patch unnecessarily complicated, see above. The following gives the same testsuite result. dirtype is one of the standard {un,}signed {char,short,int,long,long long} types, all of them have 0 in their ranges. For VR_RANGE we almost always set res.knownrange to true: /* Set KNOWNRANGE if the argument is in a known subrange of the directive's type (KNOWNRANGE may be reset below). */ res.knownrange = (!tree_int_cst_equal (TYPE_MIN_VALUE (dirtype), argmin) || !tree_int_cst_equal (TYPE_MAX_VALUE (dirtype), argmax)); (the exception is in case that range clearly has to include zero), and reset it only if adjust_range_for_overflow returned true, which means it also set the range to TYPE_M{IN,AX}_VALUE (dirtype) and again includes zero. So IMNSHO likely_adjust in what you've committed is always true when you use it and thus just a useless computation and something to make the code harder to understand. Even if you don't trust this, with the ranges in argmin/argmax, it is IMHO undesirable to set it differently at the different code paths, if you want to check whether the final range includes zero and at least one another value, just do - if (likely_adjust && maybebase && base != 10) + if ((tree_int_cst_sgn (argmin) < 0 || tree_int_cst_sgn (argmax) > 0) && maybebase && base != 10) Though, it is useless both for the above reason and for the reason that you actually do something only: if (res.range.min == 1) res.range.likely += base == 8 ? 1 : 2; else if (res.range.min == 2 && base == 16 && (dir.width[0] == 2 || dir.prec[0] == 2)) ++res.range.likely; where if the range doesn't include zero, you would never get res.range.min of 1 and for base == 16 also not 2. 2017-02-04 Jakub Jelinek <jakub@redhat.com> PR tree-optimization/79327 * gimple-ssa-sprintf.c (format_integer): Remove likely_adjust variable, its initialization and use. --- gcc/gimple-ssa-sprintf.c.jj 2017-02-04 08:43:12.000000000 +0100 +++ gcc/gimple-ssa-sprintf.c 2017-02-04 08:45:33.173709580 +0100 @@ -1232,10 +1232,6 @@ format_integer (const directive &dir, tr of the format string by returning [-1, -1]. */ return fmtresult (); - /* True if the LIKELY counter should be adjusted upward from the MIN - counter to account for arguments with unknown values. */ - bool likely_adjust = false; - fmtresult res; /* Using either the range the non-constant argument is in, or its @@ -1265,14 +1261,6 @@ format_integer (const directive &dir, tr res.argmin = argmin; res.argmax = argmax; - - /* Set the adjustment for an argument whose range includes - zero since that doesn't include the octal or hexadecimal - base prefix. */ - wide_int wzero = wi::zero (wi::get_precision (min)); - if (wi::le_p (min, wzero, SIGNED) - && !wi::neg_p (max)) - likely_adjust = true; } else if (range_type == VR_ANTI_RANGE) { @@ -1307,11 +1295,6 @@ format_integer (const directive &dir, tr if (!argmin) { - /* Set the adjustment for an argument whose range includes - zero since that doesn't include the octal or hexadecimal - base prefix. */ - likely_adjust = true; - if (TREE_CODE (argtype) == POINTER_TYPE) { argmin = build_int_cst (pointer_sized_int_node, 0); @@ -1371,7 +1354,7 @@ format_integer (const directive &dir, tr else { res.range.likely = res.range.min; - if (likely_adjust && maybebase && base != 10) + if (maybebase && base != 10) { if (res.range.min == 1) res.range.likely += base == 8 ? 1 : 2; --- gcc/testsuite/ChangeLog.jj 2017-02-04 08:43:14.000000000 +0100 +++ gcc/testsuite/ChangeLog 2017-02-04 08:48:14.930627979 +0100 @@ -20,8 +20,8 @@ PR tree-optimization/79327 * gcc.dg/tree-ssa/builtin-sprintf-warn-1.c: Adjust. - * gcc/testsuite/gcc.dg/tree-ssa/builtin-sprintf-warn-13.c: New test. - * gcc/testsuite/gcc.dg/tree-ssa/pr79327-2.c: Ditto. + * gcc.dg/tree-ssa/builtin-sprintf-warn-13.c: New test. + * gcc.dg/tree-ssa/pr79327-2.c: Ditto. 2017-02-03 Jakub Jelinek <jakub@redhat.com> Martin Sebor <msebor@redhat.com> @@ -306,7 +306,7 @@ 2017-01-27 Vladimir Makarov <vmakarov@redhat.com> PR tree-optimization/71374 - * testsuite/gcc.target/i386/pr71374.c: New. + * gcc.target/i386/pr71374.c: New. 2017-01-27 Martin Sebor <msebor@redhat.com> @@ -484,9 +484,9 @@ * g++.dg/ext/flexary18.C: Same. * g++.dg/ext/flexary19.C: Same. * g++.dg/ext/flexary7.C: Same. - * gcc/testsuite/g++.dg/cpp1z/has-unique-obj-representations1.C: Same. - * gcc/testsuite/g++.dg/ubsan/object-size-1.C: Same. - * gcc/testsuite/obj-c++.dg/property/at-property-23.mm: Same. + * g++.dg/cpp1z/has-unique-obj-representations1.C: Same. + * g++.dg/ubsan/object-size-1.C: Same. + * obj-c++.dg/property/at-property-23.mm: Same. 2017-01-25 Jakub Jelinek <jakub@redhat.com> @@ -874,10 +874,10 @@ 2017-01-20 Jiong Wang <jiong.wang@arm.com> - * testsuite/gcc.target/aarch64/return_address_sign_1.c: Enable on LP64 + * gcc.target/aarch64/return_address_sign_1.c: Enable on LP64 only. - * testsuite/gcc.target/aarch64/return_address_sign_2.c: Likewise. - * testsuite/gcc.target/aarch64/return_address_sign_3.c: Likewise. + * gcc.target/aarch64/return_address_sign_2.c: Likewise. + * gcc.target/aarch64/return_address_sign_3.c: Likewise. 2017-01-20 Nathan Sidwell <nathan@acm.org> @@ -992,7 +992,7 @@ 2017-01-19 Tamar Christina <tamar.christina@arm.com> - * gcc/testsuite/lib/target-supports.exp + * lib/target-supports.exp (check_effective_target_vect_call_copysignf): Enable for AArch64. 2017-01-19 Rainer Orth <ro@CeBiTec.Uni-Bielefeld.DE> Jakub ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH] use zero as the lower bound for a signed-unsigned range (PR 79327) 2017-02-04 8:07 ` Jakub Jelinek @ 2017-02-06 19:45 ` Jakub Jelinek 2017-02-14 0:15 ` Jeff Law 1 sibling, 0 replies; 36+ messages in thread From: Jakub Jelinek @ 2017-02-06 19:45 UTC (permalink / raw) To: Martin Sebor, Jeff Law; +Cc: Gcc Patch List On Sat, Feb 04, 2017 at 09:07:23AM +0100, Jakub Jelinek wrote: > You've committed the patch unnecessarily complicated, see above. > The following gives the same testsuite result. > > dirtype is one of the standard {un,}signed {char,short,int,long,long long} > types, all of them have 0 in their ranges. > For VR_RANGE we almost always set res.knownrange to true: > /* Set KNOWNRANGE if the argument is in a known subrange > of the directive's type (KNOWNRANGE may be reset below). */ > res.knownrange > = (!tree_int_cst_equal (TYPE_MIN_VALUE (dirtype), argmin) > || !tree_int_cst_equal (TYPE_MAX_VALUE (dirtype), argmax)); > (the exception is in case that range clearly has to include zero), > and reset it only if adjust_range_for_overflow returned true, which means > it also set the range to TYPE_M{IN,AX}_VALUE (dirtype) and again > includes zero. > So IMNSHO likely_adjust in what you've committed is always true > when you use it and thus just a useless computation and something to make > the code harder to understand. > > Even if you don't trust this, with the ranges in argmin/argmax, it is > IMHO undesirable to set it differently at the different code paths, > if you want to check whether the final range includes zero and at least > one another value, just do > - if (likely_adjust && maybebase && base != 10) > + if ((tree_int_cst_sgn (argmin) < 0 || tree_int_cst_sgn (argmax) > 0) > && maybebase && base != 10) > Though, it is useless both for the above reason and for the reason that you > actually do something only: > if (res.range.min == 1) > res.range.likely += base == 8 ? 1 : 2; > else if (res.range.min == 2 > && base == 16 > && (dir.width[0] == 2 || dir.prec[0] == 2)) > ++res.range.likely; > where if the range doesn't include zero, you would never get > res.range.min of 1 and for base == 16 also not 2. > > 2017-02-04 Jakub Jelinek <jakub@redhat.com> > > PR tree-optimization/79327 > * gimple-ssa-sprintf.c (format_integer): Remove likely_adjust > variable, its initialization and use. Now bootstrapped/regtested on x86_64-linux and i686-linux (ignore the testsuite/ChangeLog part, I've committed that part in another commit already), ok for trunk? > --- gcc/gimple-ssa-sprintf.c.jj 2017-02-04 08:43:12.000000000 +0100 > +++ gcc/gimple-ssa-sprintf.c 2017-02-04 08:45:33.173709580 +0100 > @@ -1232,10 +1232,6 @@ format_integer (const directive &dir, tr > of the format string by returning [-1, -1]. */ > return fmtresult (); > > - /* True if the LIKELY counter should be adjusted upward from the MIN > - counter to account for arguments with unknown values. */ > - bool likely_adjust = false; > - > fmtresult res; > > /* Using either the range the non-constant argument is in, or its > @@ -1265,14 +1261,6 @@ format_integer (const directive &dir, tr > > res.argmin = argmin; > res.argmax = argmax; > - > - /* Set the adjustment for an argument whose range includes > - zero since that doesn't include the octal or hexadecimal > - base prefix. */ > - wide_int wzero = wi::zero (wi::get_precision (min)); > - if (wi::le_p (min, wzero, SIGNED) > - && !wi::neg_p (max)) > - likely_adjust = true; > } > else if (range_type == VR_ANTI_RANGE) > { > @@ -1307,11 +1295,6 @@ format_integer (const directive &dir, tr > > if (!argmin) > { > - /* Set the adjustment for an argument whose range includes > - zero since that doesn't include the octal or hexadecimal > - base prefix. */ > - likely_adjust = true; > - > if (TREE_CODE (argtype) == POINTER_TYPE) > { > argmin = build_int_cst (pointer_sized_int_node, 0); > @@ -1371,7 +1354,7 @@ format_integer (const directive &dir, tr > else > { > res.range.likely = res.range.min; > - if (likely_adjust && maybebase && base != 10) > + if (maybebase && base != 10) > { > if (res.range.min == 1) > res.range.likely += base == 8 ? 1 : 2; Jakub ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH] use zero as the lower bound for a signed-unsigned range (PR 79327) 2017-02-04 8:07 ` Jakub Jelinek 2017-02-06 19:45 ` Jakub Jelinek @ 2017-02-14 0:15 ` Jeff Law 2017-02-14 7:48 ` Jakub Jelinek 1 sibling, 1 reply; 36+ messages in thread From: Jeff Law @ 2017-02-14 0:15 UTC (permalink / raw) To: Jakub Jelinek, Martin Sebor; +Cc: Gcc Patch List On 02/04/2017 01:07 AM, Jakub Jelinek wrote: > On Fri, Feb 03, 2017 at 05:39:21PM +0100, Jakub Jelinek wrote: >> Say in the http://gcc.gnu.org/ml/gcc-patches/2017-02/msg00234.html >> patch, you would with my patch need just the tree_digits part, >> and then the very last hunk in gimple-ssa-sprintf.c (with >> likely_adjust && >> removed). Because you do the adjustments only if !res.knownrange >> and in that case you know argmin/argmax are actually dirtype's min/max, >> so 0 must be in the range. > > You've committed the patch unnecessarily complicated, see above. > The following gives the same testsuite result. As you know, just getting the same testsuite result is not sufficient ;-) > > dirtype is one of the standard {un,}signed {char,short,int,long,long long} > types, all of them have 0 in their ranges. > For VR_RANGE we almost always set res.knownrange to true: > /* Set KNOWNRANGE if the argument is in a known subrange > of the directive's type (KNOWNRANGE may be reset below). */ > res.knownrange > = (!tree_int_cst_equal (TYPE_MIN_VALUE (dirtype), argmin) > || !tree_int_cst_equal (TYPE_MAX_VALUE (dirtype), argmax)); > (the exception is in case that range clearly has to include zero), > and reset it only if adjust_range_for_overflow returned true, which means > it also set the range to TYPE_M{IN,AX}_VALUE (dirtype) and again > includes zero. > So IMNSHO likely_adjust in what you've committed is always true > when you use it and thus just a useless computation and something to make > the code harder to understand. If KNOWNRANGE is false, then LIKELY_ADJUST will be true. But I don't see how we can determine anything for LIKELY_ADJUST if KNOWNRANGE is true. > > Even if you don't trust this, with the ranges in argmin/argmax, it is > IMHO undesirable to set it differently at the different code paths, > if you want to check whether the final range includes zero and at least > one another value, just do > - if (likely_adjust && maybebase && base != 10) > + if ((tree_int_cst_sgn (argmin) < 0 || tree_int_cst_sgn (argmax) > 0) > && maybebase && base != 10) > Though, it is useless both for the above reason and for the reason that you > actually do something only: I'm not convinced it's useless, but it does seem advisable to bring test down to where it's actually used and to bse it strictly on argmin/argmax. Can you test a patch which does that? Jeff ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH] use zero as the lower bound for a signed-unsigned range (PR 79327) 2017-02-14 0:15 ` Jeff Law @ 2017-02-14 7:48 ` Jakub Jelinek 2017-02-14 13:57 ` Jakub Jelinek 2017-02-14 16:37 ` Martin Sebor 0 siblings, 2 replies; 36+ messages in thread From: Jakub Jelinek @ 2017-02-14 7:48 UTC (permalink / raw) To: Jeff Law; +Cc: Martin Sebor, Gcc Patch List On Mon, Feb 13, 2017 at 04:53:19PM -0700, Jeff Law wrote: > > dirtype is one of the standard {un,}signed {char,short,int,long,long long} > > types, all of them have 0 in their ranges. > > For VR_RANGE we almost always set res.knownrange to true: > > /* Set KNOWNRANGE if the argument is in a known subrange > > of the directive's type (KNOWNRANGE may be reset below). */ > > res.knownrange > > = (!tree_int_cst_equal (TYPE_MIN_VALUE (dirtype), argmin) > > || !tree_int_cst_equal (TYPE_MAX_VALUE (dirtype), argmax)); > > (the exception is in case that range clearly has to include zero), > > and reset it only if adjust_range_for_overflow returned true, which means > > it also set the range to TYPE_M{IN,AX}_VALUE (dirtype) and again > > includes zero. > > So IMNSHO likely_adjust in what you've committed is always true > > when you use it and thus just a useless computation and something to make > > the code harder to understand. > If KNOWNRANGE is false, then LIKELY_ADJUST will be true. But I don't see > how we can determine anything for LIKELY_ADJUST if KNOWNRANGE is true. We can't, but that doesn't matter, we only use it if KNOWNRANGE is false. The only user of LIKELY_ADJUST is: if (res.knownrange) res.range.likely = res.range.max; else { // -- Here we know res.knownrage is false res.range.likely = res.range.min; if (likely_adjust && maybebase && base != 10) // -- and here is the only user of likely_adjust { if (res.range.min == 1) res.range.likely += base == 8 ? 1 : 2; else if (res.range.min == 2 && base == 16 && (dir.width[0] == 2 || dir.prec[0] == 2)) ++res.range.likely; } } > > Even if you don't trust this, with the ranges in argmin/argmax, it is > > IMHO undesirable to set it differently at the different code paths, > > if you want to check whether the final range includes zero and at least > > one another value, just do > > - if (likely_adjust && maybebase && base != 10) > > + if ((tree_int_cst_sgn (argmin) < 0 || tree_int_cst_sgn (argmax) > 0) > > && maybebase && base != 10) > > Though, it is useless both for the above reason and for the reason that you > > actually do something only: > I'm not convinced it's useless, but it does seem advisable to bring test > down to where it's actually used and to bse it strictly on argmin/argmax. > Can you test a patch which does that? That would then be (the only difference compared to the previous patch is the last hunk) following. I can surely test that, I'm still convinced it would work equally if that (tree_int_cst_sgn (argmin) < 0 || tree_int_cst_sgn (argmax) > 0) is just gcc_checking_assert. 2017-02-14 Jakub Jelinek <jakub@redhat.com> PR tree-optimization/79327 * gimple-ssa-sprintf.c (format_integer): Remove likely_adjust variable, its initialization and use. --- gcc/gimple-ssa-sprintf.c.jj 2017-02-04 08:43:12.000000000 +0100 +++ gcc/gimple-ssa-sprintf.c 2017-02-04 08:45:33.173709580 +0100 @@ -1232,10 +1232,6 @@ format_integer (const directive &dir, tr of the format string by returning [-1, -1]. */ return fmtresult (); - /* True if the LIKELY counter should be adjusted upward from the MIN - counter to account for arguments with unknown values. */ - bool likely_adjust = false; - fmtresult res; /* Using either the range the non-constant argument is in, or its @@ -1265,14 +1261,6 @@ format_integer (const directive &dir, tr res.argmin = argmin; res.argmax = argmax; - - /* Set the adjustment for an argument whose range includes - zero since that doesn't include the octal or hexadecimal - base prefix. */ - wide_int wzero = wi::zero (wi::get_precision (min)); - if (wi::le_p (min, wzero, SIGNED) - && !wi::neg_p (max)) - likely_adjust = true; } else if (range_type == VR_ANTI_RANGE) { @@ -1307,11 +1295,6 @@ format_integer (const directive &dir, tr if (!argmin) { - /* Set the adjustment for an argument whose range includes - zero since that doesn't include the octal or hexadecimal - base prefix. */ - likely_adjust = true; - if (TREE_CODE (argtype) == POINTER_TYPE) { argmin = build_int_cst (pointer_sized_int_node, 0); @@ -1371,7 +1354,8 @@ format_integer (const directive &dir, tr else { res.range.likely = res.range.min; - if (likely_adjust && maybebase && base != 10) + if (maybebase && base != 10 + && (tree_int_cst_sgn (argmin) < 0 || tree_int_cst_sgn (argmax) > 0)) { if (res.range.min == 1) res.range.likely += base == 8 ? 1 : 2; Jakub ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH] use zero as the lower bound for a signed-unsigned range (PR 79327) 2017-02-14 7:48 ` Jakub Jelinek @ 2017-02-14 13:57 ` Jakub Jelinek 2017-02-16 23:22 ` Jeff Law 2017-02-14 16:37 ` Martin Sebor 1 sibling, 1 reply; 36+ messages in thread From: Jakub Jelinek @ 2017-02-14 13:57 UTC (permalink / raw) To: Jeff Law; +Cc: Martin Sebor, Gcc Patch List On Tue, Feb 14, 2017 at 08:18:13AM +0100, Jakub Jelinek wrote: > On Mon, Feb 13, 2017 at 04:53:19PM -0700, Jeff Law wrote: > > > dirtype is one of the standard {un,}signed {char,short,int,long,long long} > > > types, all of them have 0 in their ranges. > > > For VR_RANGE we almost always set res.knownrange to true: > > > /* Set KNOWNRANGE if the argument is in a known subrange > > > of the directive's type (KNOWNRANGE may be reset below). */ > > > res.knownrange > > > = (!tree_int_cst_equal (TYPE_MIN_VALUE (dirtype), argmin) > > > || !tree_int_cst_equal (TYPE_MAX_VALUE (dirtype), argmax)); > > > (the exception is in case that range clearly has to include zero), > > > and reset it only if adjust_range_for_overflow returned true, which means > > > it also set the range to TYPE_M{IN,AX}_VALUE (dirtype) and again > > > includes zero. > > > So IMNSHO likely_adjust in what you've committed is always true > > > when you use it and thus just a useless computation and something to make > > > the code harder to understand. > > If KNOWNRANGE is false, then LIKELY_ADJUST will be true. But I don't see > > how we can determine anything for LIKELY_ADJUST if KNOWNRANGE is true. > > We can't, but that doesn't matter, we only use it if KNOWNRANGE is false. > The only user of LIKELY_ADJUST is: > > if (res.knownrange) > res.range.likely = res.range.max; > else > { > // -- Here we know res.knownrage is false > res.range.likely = res.range.min; > if (likely_adjust && maybebase && base != 10) > // -- and here is the only user of likely_adjust > { > if (res.range.min == 1) > res.range.likely += base == 8 ? 1 : 2; > else if (res.range.min == 2 > && base == 16 > && (dir.width[0] == 2 || dir.prec[0] == 2)) > ++res.range.likely; > } > } Another argument I had was that if maybebase and base != 10, then if the range does not include zero (if it ever could be !res.knownrange in that case), then res.range.min won't be 1 and for base 16 won't be even 2, because all the values in the range will include the 0 or 0x prefixes. The only controversion then would be if the range was [0, 0], then bumping res.range.likely would not be appropriate. But such range is really res.knownrange and never anything else. Jakub ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH] use zero as the lower bound for a signed-unsigned range (PR 79327) 2017-02-14 13:57 ` Jakub Jelinek @ 2017-02-16 23:22 ` Jeff Law 0 siblings, 0 replies; 36+ messages in thread From: Jeff Law @ 2017-02-16 23:22 UTC (permalink / raw) To: Jakub Jelinek; +Cc: Martin Sebor, Gcc Patch List On 02/14/2017 06:43 AM, Jakub Jelinek wrote: > On Tue, Feb 14, 2017 at 08:18:13AM +0100, Jakub Jelinek wrote: >> On Mon, Feb 13, 2017 at 04:53:19PM -0700, Jeff Law wrote: >>>> dirtype is one of the standard {un,}signed {char,short,int,long,long long} >>>> types, all of them have 0 in their ranges. >>>> For VR_RANGE we almost always set res.knownrange to true: >>>> /* Set KNOWNRANGE if the argument is in a known subrange >>>> of the directive's type (KNOWNRANGE may be reset below). */ >>>> res.knownrange >>>> = (!tree_int_cst_equal (TYPE_MIN_VALUE (dirtype), argmin) >>>> || !tree_int_cst_equal (TYPE_MAX_VALUE (dirtype), argmax)); >>>> (the exception is in case that range clearly has to include zero), >>>> and reset it only if adjust_range_for_overflow returned true, which means >>>> it also set the range to TYPE_M{IN,AX}_VALUE (dirtype) and again >>>> includes zero. >>>> So IMNSHO likely_adjust in what you've committed is always true >>>> when you use it and thus just a useless computation and something to make >>>> the code harder to understand. >>> If KNOWNRANGE is false, then LIKELY_ADJUST will be true. But I don't see >>> how we can determine anything for LIKELY_ADJUST if KNOWNRANGE is true. >> >> We can't, but that doesn't matter, we only use it if KNOWNRANGE is false. >> The only user of LIKELY_ADJUST is: >> >> if (res.knownrange) >> res.range.likely = res.range.max; >> else >> { >> // -- Here we know res.knownrage is false >> res.range.likely = res.range.min; >> if (likely_adjust && maybebase && base != 10) >> // -- and here is the only user of likely_adjust >> { >> if (res.range.min == 1) >> res.range.likely += base == 8 ? 1 : 2; >> else if (res.range.min == 2 >> && base == 16 >> && (dir.width[0] == 2 || dir.prec[0] == 2)) >> ++res.range.likely; >> } >> } Duh. You're obviously right. Still reading the rest of the thread. jeff ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH] use zero as the lower bound for a signed-unsigned range (PR 79327) 2017-02-14 7:48 ` Jakub Jelinek 2017-02-14 13:57 ` Jakub Jelinek @ 2017-02-14 16:37 ` Martin Sebor 2017-02-14 16:40 ` Jakub Jelinek 1 sibling, 1 reply; 36+ messages in thread From: Martin Sebor @ 2017-02-14 16:37 UTC (permalink / raw) To: Jakub Jelinek, Jeff Law; +Cc: Gcc Patch List On 02/14/2017 12:18 AM, Jakub Jelinek wrote: > On Mon, Feb 13, 2017 at 04:53:19PM -0700, Jeff Law wrote: >>> dirtype is one of the standard {un,}signed {char,short,int,long,long long} >>> types, all of them have 0 in their ranges. >>> For VR_RANGE we almost always set res.knownrange to true: >>> /* Set KNOWNRANGE if the argument is in a known subrange >>> of the directive's type (KNOWNRANGE may be reset below). */ >>> res.knownrange >>> = (!tree_int_cst_equal (TYPE_MIN_VALUE (dirtype), argmin) >>> || !tree_int_cst_equal (TYPE_MAX_VALUE (dirtype), argmax)); >>> (the exception is in case that range clearly has to include zero), >>> and reset it only if adjust_range_for_overflow returned true, which means >>> it also set the range to TYPE_M{IN,AX}_VALUE (dirtype) and again >>> includes zero. >>> So IMNSHO likely_adjust in what you've committed is always true >>> when you use it and thus just a useless computation and something to make >>> the code harder to understand. >> If KNOWNRANGE is false, then LIKELY_ADJUST will be true. But I don't see >> how we can determine anything for LIKELY_ADJUST if KNOWNRANGE is true. > > We can't, but that doesn't matter, we only use it if KNOWNRANGE is false. > The only user of LIKELY_ADJUST is: > > if (res.knownrange) > res.range.likely = res.range.max; > else > { > // -- Here we know res.knownrage is false > res.range.likely = res.range.min; > if (likely_adjust && maybebase && base != 10) > // -- and here is the only user of likely_adjust > { > if (res.range.min == 1) > res.range.likely += base == 8 ? 1 : 2; > else if (res.range.min == 2 > && base == 16 > && (dir.width[0] == 2 || dir.prec[0] == 2)) > ++res.range.likely; > } > } > >>> Even if you don't trust this, with the ranges in argmin/argmax, it is >>> IMHO undesirable to set it differently at the different code paths, >>> if you want to check whether the final range includes zero and at least >>> one another value, just do >>> - if (likely_adjust && maybebase && base != 10) >>> + if ((tree_int_cst_sgn (argmin) < 0 || tree_int_cst_sgn (argmax) > 0) >>> && maybebase && base != 10) >>> Though, it is useless both for the above reason and for the reason that you >>> actually do something only: >> I'm not convinced it's useless, but it does seem advisable to bring test >> down to where it's actually used and to bse it strictly on argmin/argmax. >> Can you test a patch which does that? > > That would then be (the only difference compared to the previous patch is > the last hunk) following. I can surely test that, I'm still convinced it > would work equally if that > (tree_int_cst_sgn (argmin) < 0 || tree_int_cst_sgn (argmax) > 0) > is just gcc_checking_assert. > > 2017-02-14 Jakub Jelinek <jakub@redhat.com> > > PR tree-optimization/79327 > * gimple-ssa-sprintf.c (format_integer): Remove likely_adjust > variable, its initialization and use. > > --- gcc/gimple-ssa-sprintf.c.jj 2017-02-04 08:43:12.000000000 +0100 > +++ gcc/gimple-ssa-sprintf.c 2017-02-04 08:45:33.173709580 +0100 > @@ -1232,10 +1232,6 @@ format_integer (const directive &dir, tr > of the format string by returning [-1, -1]. */ > return fmtresult (); > > - /* True if the LIKELY counter should be adjusted upward from the MIN > - counter to account for arguments with unknown values. */ > - bool likely_adjust = false; > - > fmtresult res; > > /* Using either the range the non-constant argument is in, or its > @@ -1265,14 +1261,6 @@ format_integer (const directive &dir, tr > > res.argmin = argmin; > res.argmax = argmax; > - > - /* Set the adjustment for an argument whose range includes > - zero since that doesn't include the octal or hexadecimal > - base prefix. */ > - wide_int wzero = wi::zero (wi::get_precision (min)); > - if (wi::le_p (min, wzero, SIGNED) > - && !wi::neg_p (max)) > - likely_adjust = true; > } > else if (range_type == VR_ANTI_RANGE) > { > @@ -1307,11 +1295,6 @@ format_integer (const directive &dir, tr > > if (!argmin) > { > - /* Set the adjustment for an argument whose range includes > - zero since that doesn't include the octal or hexadecimal > - base prefix. */ > - likely_adjust = true; > - > if (TREE_CODE (argtype) == POINTER_TYPE) > { > argmin = build_int_cst (pointer_sized_int_node, 0); > @@ -1371,7 +1354,8 @@ format_integer (const directive &dir, tr > else > { > res.range.likely = res.range.min; > - if (likely_adjust && maybebase && base != 10) > + if (maybebase && base != 10 > + && (tree_int_cst_sgn (argmin) < 0 || tree_int_cst_sgn (argmax) > 0)) > { > if (res.range.min == 1) > res.range.likely += base == 8 ? 1 : 2; You've removed all the comments that explain what's going on. If you must make the change (I see no justification for it now) please at least document it. Martin ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH] use zero as the lower bound for a signed-unsigned range (PR 79327) 2017-02-14 16:37 ` Martin Sebor @ 2017-02-14 16:40 ` Jakub Jelinek 2017-02-14 19:24 ` Martin Sebor 0 siblings, 1 reply; 36+ messages in thread From: Jakub Jelinek @ 2017-02-14 16:40 UTC (permalink / raw) To: Martin Sebor; +Cc: Jeff Law, Gcc Patch List On Tue, Feb 14, 2017 at 09:36:44AM -0700, Martin Sebor wrote: > > @@ -1371,7 +1354,8 @@ format_integer (const directive &dir, tr > > else > > { > > res.range.likely = res.range.min; > > - if (likely_adjust && maybebase && base != 10) > > + if (maybebase && base != 10 > > + && (tree_int_cst_sgn (argmin) < 0 || tree_int_cst_sgn (argmax) > 0)) > > { > > if (res.range.min == 1) > > res.range.likely += base == 8 ? 1 : 2; > > You've removed all the comments that explain what's going on. If > you must make the change (I see no justification for it now) please > at least document it. I thought that is the: /* Add the adjustment for an argument whose range includes zero since it doesn't include the octal or hexadecimal base prefix. */ comment above the if. Jakub ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH] use zero as the lower bound for a signed-unsigned range (PR 79327) 2017-02-14 16:40 ` Jakub Jelinek @ 2017-02-14 19:24 ` Martin Sebor 2017-02-14 20:59 ` Jakub Jelinek 0 siblings, 1 reply; 36+ messages in thread From: Martin Sebor @ 2017-02-14 19:24 UTC (permalink / raw) To: Jakub Jelinek; +Cc: Jeff Law, Gcc Patch List On 02/14/2017 09:39 AM, Jakub Jelinek wrote: > On Tue, Feb 14, 2017 at 09:36:44AM -0700, Martin Sebor wrote: >>> @@ -1371,7 +1354,8 @@ format_integer (const directive &dir, tr >>> else >>> { >>> res.range.likely = res.range.min; >>> - if (likely_adjust && maybebase && base != 10) >>> + if (maybebase && base != 10 >>> + && (tree_int_cst_sgn (argmin) < 0 || tree_int_cst_sgn (argmax) > 0)) >>> { >>> if (res.range.min == 1) >>> res.range.likely += base == 8 ? 1 : 2; >> >> You've removed all the comments that explain what's going on. If >> you must make the change (I see no justification for it now) please >> at least document it. > > I thought that is the: > /* Add the adjustment for an argument whose range includes zero > since it doesn't include the octal or hexadecimal base prefix. */ > comment above the if. That comment explains how the likely_adjust variable ("the adjustment") is being used, or more precisely, how it was being used in the first version of the patch. The comment became somewhat out of date with the committed version of the patch (this was my bad). The variable is documented where it's defined and again where it's assigned to. With the removal of those comments it seems especially important that the only remaining description of what's going on be accurate. The comment is outdated because it refers to "the adjustment" which doesn't exist anymore. (It was replaced by a flag in my commit). To bring it up to date it should say something like: /* Set the LIKELY counter to MIN. In base 8 and 16, when the argument is in range that includes zero, adjust it upward to include the length of the base prefix since in that case the MIN counter does include it. */ On a separate note, while testing the patch I noticed that it's not exactly equivalent to what's on trunk. Trunk silently accepts the call below but with the patch it complains. That's great (it should complain) but the change should be tested. More to my point, while in this case your change happened to fix a subtle bug (which I'm certainly happy about), it could have just as easily introduced one. char d[2]; void f (unsigned i) { if (i < 1234 || 12345 < i) i = 1234; __builtin_sprintf (d, "%#hhx", i); } Martin ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH] use zero as the lower bound for a signed-unsigned range (PR 79327) 2017-02-14 19:24 ` Martin Sebor @ 2017-02-14 20:59 ` Jakub Jelinek 2017-02-14 22:17 ` Martin Sebor 2017-02-16 23:34 ` Jeff Law 0 siblings, 2 replies; 36+ messages in thread From: Jakub Jelinek @ 2017-02-14 20:59 UTC (permalink / raw) To: Martin Sebor; +Cc: Jeff Law, Gcc Patch List On Tue, Feb 14, 2017 at 12:15:59PM -0700, Martin Sebor wrote: > That comment explains how the likely_adjust variable ("the adjustment") > is being used, or more precisely, how it was being used in the first > version of the patch. The comment became somewhat out of date with > the committed version of the patch (this was my bad). > > The variable is documented where it's defined and again where it's > assigned to. With the removal of those comments it seems especially > important that the only remaining description of what's going on be > accurate. > > The comment is outdated because it refers to "the adjustment" which > doesn't exist anymore. (It was replaced by a flag in my commit). > To bring it up to date it should say something like: > > /* Set the LIKELY counter to MIN. In base 8 and 16, when > the argument is in range that includes zero, adjust it > upward to include the length of the base prefix since > in that case the MIN counter does include it. */ So for a comment, what about following then? With or without the IMNSHO useless && (tree_int_cst_sgn (argmin) < 0 || tree_int_cst_sgn (argmax) > 0) > On a separate note, while testing the patch I noticed that it's > not exactly equivalent to what's on trunk. Trunk silently accepts > the call below but with the patch it complains. That's great (it > should complain) but the change should be tested. More to my point, > while in this case your change happened to fix a subtle bug (which > I'm certainly happy about), it could have just as easily introduced > one. Yeah, indeed. That should be a clear argument for why writing it in so many places is bad, it is simply much more error-prone, there are too many cases to get right. > char d[2]; > > void f (unsigned i) > { > if (i < 1234 || 12345 < i) > i = 1234; > > __builtin_sprintf (d, "%#hhx", i); > } What happens is that because the original range doesn't contain zero you set likely_adjust to false and then never update it again because the implicit cast changed the range. If some version of the patch is approved, I'll leave addition of this testcase to you (incrementally). 2017-02-14 Jakub Jelinek <jakub@redhat.com> PR tree-optimization/79327 * gimple-ssa-sprintf.c (format_integer): Remove likely_adjust variable, its initialization and use. --- gcc/gimple-ssa-sprintf.c.jj 2017-02-14 21:21:56.048745037 +0100 +++ gcc/gimple-ssa-sprintf.c 2017-02-14 21:25:20.939033174 +0100 @@ -1232,10 +1232,6 @@ format_integer (const directive &dir, tr of the format string by returning [-1, -1]. */ return fmtresult (); - /* True if the LIKELY counter should be adjusted upward from the MIN - counter to account for arguments with unknown values. */ - bool likely_adjust = false; - fmtresult res; /* Using either the range the non-constant argument is in, or its @@ -1265,14 +1261,6 @@ format_integer (const directive &dir, tr res.argmin = argmin; res.argmax = argmax; - - /* Set the adjustment for an argument whose range includes - zero since that doesn't include the octal or hexadecimal - base prefix. */ - wide_int wzero = wi::zero (wi::get_precision (min)); - if (wi::le_p (min, wzero, SIGNED) - && !wi::neg_p (max)) - likely_adjust = true; } else if (range_type == VR_ANTI_RANGE) { @@ -1307,11 +1295,6 @@ format_integer (const directive &dir, tr if (!argmin) { - /* Set the adjustment for an argument whose range includes - zero since that doesn't include the octal or hexadecimal - base prefix. */ - likely_adjust = true; - if (TREE_CODE (argtype) == POINTER_TYPE) { argmin = build_int_cst (pointer_sized_int_node, 0); @@ -1364,14 +1347,19 @@ format_integer (const directive &dir, tr res.range.max = MAX (max1, max2); } - /* Add the adjustment for an argument whose range includes zero - since it doesn't include the octal or hexadecimal base prefix. */ + /* If the range is known, use the maximum as the likely length. */ if (res.knownrange) res.range.likely = res.range.max; else { + /* Otherwise, use the minimum. Except for the case where for %#x or + %#o the minimum is just for a single value in the range (0) and + for all other values it is something longer, like 0x1 or 01. + Use the length for value 1 in that case instead as the likely + length. */ res.range.likely = res.range.min; - if (likely_adjust && maybebase && base != 10) + if (maybebase && base != 10 + && (tree_int_cst_sgn (argmin) < 0 || tree_int_cst_sgn (argmax) > 0)) { if (res.range.min == 1) res.range.likely += base == 8 ? 1 : 2; Jakub ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH] use zero as the lower bound for a signed-unsigned range (PR 79327) 2017-02-14 20:59 ` Jakub Jelinek @ 2017-02-14 22:17 ` Martin Sebor 2017-02-16 23:34 ` Jeff Law 1 sibling, 0 replies; 36+ messages in thread From: Martin Sebor @ 2017-02-14 22:17 UTC (permalink / raw) To: Jakub Jelinek; +Cc: Jeff Law, Gcc Patch List On 02/14/2017 01:32 PM, Jakub Jelinek wrote: > On Tue, Feb 14, 2017 at 12:15:59PM -0700, Martin Sebor wrote: >> That comment explains how the likely_adjust variable ("the adjustment") >> is being used, or more precisely, how it was being used in the first >> version of the patch. The comment became somewhat out of date with >> the committed version of the patch (this was my bad). >> >> The variable is documented where it's defined and again where it's >> assigned to. With the removal of those comments it seems especially >> important that the only remaining description of what's going on be >> accurate. >> >> The comment is outdated because it refers to "the adjustment" which >> doesn't exist anymore. (It was replaced by a flag in my commit). >> To bring it up to date it should say something like: >> >> /* Set the LIKELY counter to MIN. In base 8 and 16, when >> the argument is in range that includes zero, adjust it >> upward to include the length of the base prefix since >> in that case the MIN counter does include it. */ > > So for a comment, what about following then? With or without > the IMNSHO useless > && (tree_int_cst_sgn (argmin) < 0 || tree_int_cst_sgn (argmax) > 0) If the condition is redundant (it seems like it could be) it shouldn't be included in the patch. It seems like an opportunity for further simplification. I'm sure it's not the only one, either. >> On a separate note, while testing the patch I noticed that it's >> not exactly equivalent to what's on trunk. Trunk silently accepts >> the call below but with the patch it complains. That's great (it >> should complain) but the change should be tested. More to my point, >> while in this case your change happened to fix a subtle bug (which >> I'm certainly happy about), it could have just as easily introduced >> one. > > Yeah, indeed. That should be a clear argument for why writing it in > so many places is bad, it is simply much more error-prone, there are > too many cases to get right. No argument there. There's always room for improvement, cleanup, or refactoring. Martin > >> char d[2]; >> >> void f (unsigned i) >> { >> if (i < 1234 || 12345 < i) >> i = 1234; >> >> __builtin_sprintf (d, "%#hhx", i); >> } > > What happens is that because the original range doesn't contain zero > you set likely_adjust to false and then never update it again because > the implicit cast changed the range. > > If some version of the patch is approved, I'll leave addition of this > testcase to you (incrementally). > > 2017-02-14 Jakub Jelinek <jakub@redhat.com> > > PR tree-optimization/79327 > * gimple-ssa-sprintf.c (format_integer): Remove likely_adjust > variable, its initialization and use. > > --- gcc/gimple-ssa-sprintf.c.jj 2017-02-14 21:21:56.048745037 +0100 > +++ gcc/gimple-ssa-sprintf.c 2017-02-14 21:25:20.939033174 +0100 > @@ -1232,10 +1232,6 @@ format_integer (const directive &dir, tr > of the format string by returning [-1, -1]. */ > return fmtresult (); > > - /* True if the LIKELY counter should be adjusted upward from the MIN > - counter to account for arguments with unknown values. */ > - bool likely_adjust = false; > - > fmtresult res; > > /* Using either the range the non-constant argument is in, or its > @@ -1265,14 +1261,6 @@ format_integer (const directive &dir, tr > > res.argmin = argmin; > res.argmax = argmax; > - > - /* Set the adjustment for an argument whose range includes > - zero since that doesn't include the octal or hexadecimal > - base prefix. */ > - wide_int wzero = wi::zero (wi::get_precision (min)); > - if (wi::le_p (min, wzero, SIGNED) > - && !wi::neg_p (max)) > - likely_adjust = true; > } > else if (range_type == VR_ANTI_RANGE) > { > @@ -1307,11 +1295,6 @@ format_integer (const directive &dir, tr > > if (!argmin) > { > - /* Set the adjustment for an argument whose range includes > - zero since that doesn't include the octal or hexadecimal > - base prefix. */ > - likely_adjust = true; > - > if (TREE_CODE (argtype) == POINTER_TYPE) > { > argmin = build_int_cst (pointer_sized_int_node, 0); > @@ -1364,14 +1347,19 @@ format_integer (const directive &dir, tr > res.range.max = MAX (max1, max2); > } > > - /* Add the adjustment for an argument whose range includes zero > - since it doesn't include the octal or hexadecimal base prefix. */ > + /* If the range is known, use the maximum as the likely length. */ > if (res.knownrange) > res.range.likely = res.range.max; > else > { > + /* Otherwise, use the minimum. Except for the case where for %#x or > + %#o the minimum is just for a single value in the range (0) and > + for all other values it is something longer, like 0x1 or 01. > + Use the length for value 1 in that case instead as the likely > + length. */ > res.range.likely = res.range.min; > - if (likely_adjust && maybebase && base != 10) > + if (maybebase && base != 10 > + && (tree_int_cst_sgn (argmin) < 0 || tree_int_cst_sgn (argmax) > 0)) > { > if (res.range.min == 1) > res.range.likely += base == 8 ? 1 : 2; > > > Jakub > ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH] use zero as the lower bound for a signed-unsigned range (PR 79327) 2017-02-14 20:59 ` Jakub Jelinek 2017-02-14 22:17 ` Martin Sebor @ 2017-02-16 23:34 ` Jeff Law 1 sibling, 0 replies; 36+ messages in thread From: Jeff Law @ 2017-02-16 23:34 UTC (permalink / raw) To: Jakub Jelinek, Martin Sebor; +Cc: Gcc Patch List On 02/14/2017 01:32 PM, Jakub Jelinek wrote: > On Tue, Feb 14, 2017 at 12:15:59PM -0700, Martin Sebor wrote: >> That comment explains how the likely_adjust variable ("the adjustment") >> is being used, or more precisely, how it was being used in the first >> version of the patch. The comment became somewhat out of date with >> the committed version of the patch (this was my bad). >> >> The variable is documented where it's defined and again where it's >> assigned to. With the removal of those comments it seems especially >> important that the only remaining description of what's going on be >> accurate. >> >> The comment is outdated because it refers to "the adjustment" which >> doesn't exist anymore. (It was replaced by a flag in my commit). >> To bring it up to date it should say something like: >> >> /* Set the LIKELY counter to MIN. In base 8 and 16, when >> the argument is in range that includes zero, adjust it >> upward to include the length of the base prefix since >> in that case the MIN counter does include it. */ > > So for a comment, what about following then? With or without > the IMNSHO useless > && (tree_int_cst_sgn (argmin) < 0 || tree_int_cst_sgn (argmax) > 0) > >> On a separate note, while testing the patch I noticed that it's >> not exactly equivalent to what's on trunk. Trunk silently accepts >> the call below but with the patch it complains. That's great (it >> should complain) but the change should be tested. More to my point, >> while in this case your change happened to fix a subtle bug (which >> I'm certainly happy about), it could have just as easily introduced >> one. > > Yeah, indeed. That should be a clear argument for why writing it in > so many places is bad, it is simply much more error-prone, there are > too many cases to get right. > >> char d[2]; >> >> void f (unsigned i) >> { >> if (i < 1234 || 12345 < i) >> i = 1234; >> >> __builtin_sprintf (d, "%#hhx", i); >> } > > What happens is that because the original range doesn't contain zero > you set likely_adjust to false and then never update it again because > the implicit cast changed the range. > > If some version of the patch is approved, I'll leave addition of this > testcase to you (incrementally). > > 2017-02-14 Jakub Jelinek <jakub@redhat.com> > > PR tree-optimization/79327 > * gimple-ssa-sprintf.c (format_integer): Remove likely_adjust > variable, its initialization and use. This is fine. And the addition of the test from Martin is pre-approved as well. jeff ^ permalink raw reply [flat|nested] 36+ messages in thread
end of thread, other threads:[~2017-02-16 23:22 UTC | newest] Thread overview: 36+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-02-01 23:52 [PATCH] use zero as the lower bound for a signed-unsigned range (PR 79327) Martin Sebor 2017-02-02 7:37 ` Jakub Jelinek 2017-02-02 9:52 ` Jakub Jelinek 2017-02-02 14:12 ` Jakub Jelinek 2017-02-03 18:44 ` Jeff Law 2017-02-03 19:02 ` Jakub Jelinek 2017-02-02 16:58 ` Martin Sebor 2017-02-02 19:59 ` Martin Sebor 2017-02-02 23:23 ` Jakub Jelinek 2017-02-03 0:50 ` Martin Sebor 2017-02-03 19:07 ` Jeff Law 2017-02-03 0:31 ` Martin Sebor 2017-02-03 0:48 ` Jakub Jelinek 2017-02-03 5:37 ` Martin Sebor 2017-02-03 19:02 ` Jeff Law 2017-02-03 19:08 ` Martin Sebor 2017-02-02 22:15 ` Jakub Jelinek 2017-02-03 18:53 ` Jeff Law 2017-02-03 7:35 ` Jeff Law 2017-02-03 16:13 ` Martin Sebor 2017-02-03 16:39 ` Jakub Jelinek 2017-02-03 17:02 ` Martin Sebor 2017-02-03 17:17 ` Jakub Jelinek 2017-02-03 17:49 ` Jeff Law 2017-02-04 8:07 ` Jakub Jelinek 2017-02-06 19:45 ` Jakub Jelinek 2017-02-14 0:15 ` Jeff Law 2017-02-14 7:48 ` Jakub Jelinek 2017-02-14 13:57 ` Jakub Jelinek 2017-02-16 23:22 ` Jeff Law 2017-02-14 16:37 ` Martin Sebor 2017-02-14 16:40 ` Jakub Jelinek 2017-02-14 19:24 ` Martin Sebor 2017-02-14 20:59 ` Jakub Jelinek 2017-02-14 22:17 ` Martin Sebor 2017-02-16 23:34 ` Jeff Law
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).