[PATCH] Fix pdftex miscompilation due to get_range_strlen (PR tree-optimization/84478)
Martin Sebor
msebor@gmail.com
Tue Feb 20 19:03:00 GMT 2018
On 02/20/2018 10:17 AM, Jakub Jelinek wrote:
> Hi!
>
> The following testcase distilled from pdftex is miscompiled on i?86,
> because gimple_fold_builtin_strlen sets incorrect value range on
> strlen call on SSA_NAME with def_stmt of PHI <"mu", something> where
> we can't determine anything about string length of something, so the
> right value range is [0, maximum_possible_strlen], but we actually used
> [2, INF].
>
> get_range_strlen has many modes, for get_maxval_strlen we check return
> value of get_range_strlen, don't set fuzzy and everything seems correct.
> Otherwise we have the fuzzy mode which is used in 2 arg get_range_strlen
> overload, which is used for warnings and recently also for
> gimple_fold_builtin_strlen which sets value ranges from it. Unlike
> warnings, which can perhaps afford some heuristics and guessing, for
> gimple_fold_builtin_strlen we need to be 100% conservative.
>
> The thing that isn't handled conservatively is PHIs and COND_EXPR.
> The current code, if we can't figure one of the args out, for PHIs in
> fuzzy mode increases the *maxval value to +INF, but doesn't touch
> *minval, for COND_EXPR doesn't adjust the *minval/*maxval at all and just
> returns false. Unfortunately, changing that breaks
It sounds like not setting *minlen is the problem then. Attached
is a slightly smaller patch that fixes the bug with no testsuite
regressions on x86_64-linux. How does it look to you?
Martin
> FAIL: gcc.dg/tree-ssa/builtin-sprintf-warn-11.c (test for warnings, line 165)
> FAIL: gcc.dg/tree-ssa/builtin-sprintf-warn-11.c (test for warnings, line 166)
> FAIL: gcc.dg/tree-ssa/builtin-sprintf-warn-11.c (test for warnings, line 167)
> FAIL: gcc.dg/tree-ssa/builtin-sprintf-warn-11.c (test for warnings, line 168)
> FAIL: gcc.dg/tree-ssa/builtin-sprintf-warn-11.c (test for warnings, line 309)
> FAIL: gcc.dg/tree-ssa/builtin-sprintf-warn-11.c (test for warnings, line 310)
> FAIL: gcc.dg/tree-ssa/builtin-sprintf-warn-11.c (test for warnings, line 311)
> FAIL: gcc.dg/tree-ssa/builtin-sprintf-warn-11.c (test for warnings, line 312)
> FAIL: gcc.dg/tree-ssa/builtin-sprintf-warn-11.c (test for excess errors)
> FAIL: gcc.dg/tree-ssa/builtin-sprintf-warn-3.c (test for warnings, line 62)
> FAIL: gcc.dg/tree-ssa/builtin-sprintf-warn-3.c (test for warnings, line 63)
> FAIL: gcc.dg/tree-ssa/builtin-sprintf-warn-3.c (test for warnings, line 402)
> FAIL: gcc.dg/tree-ssa/builtin-sprintf-warn-3.c (test for warnings, line 403)
> FAIL: gcc.dg/tree-ssa/builtin-sprintf-warn-3.c (test for warnings, line 430)
> FAIL: gcc.dg/tree-ssa/builtin-sprintf-warn-3.c (test for warnings, line 431)
> FAIL: gcc.dg/tree-ssa/builtin-sprintf-warn-3.c (test for warnings, line 458)
> FAIL: gcc.dg/tree-ssa/builtin-sprintf-warn-3.c (test for warnings, line 459)
> so this patch instead stops using the 2 argument get_range_strlen
> in gimple_fold_builtin_strlen and makes sure we handle that case
> conservatively, i.e. PHI of something known + unknown is unknown, likewise
> COND_EXPR.
>
> Bootstrapped on x86_64-linux and i686-linux, regtest pending, ok for trunk
> if it passes regtest on both?
>
> For stage1 I guess Martin can tweak the warning code paths and if they will
> become the same as the conservatively correct ones, what is in this patch
> can be adjusted again.
>
> 2018-02-20 Jakub Jelinek <jakub@redhat.com>
>
> PR tree-optimization/84478
> * gimple-fold.c (get_range_strlen): Make minlen const and assume it
> can't be NULL. Add type 3 support which is conservatively correct
> in PHIs. Formatting and comment capitalization fixes. Add warning
> that the 2 argument get_range_strlen is only usable for warnings.
> (gimple_fold_builtin_strlen): Use the 6 arg get_range_strlen overload
> rather than 2 arg, use it only if it returns true and flexarray is
> false, pass 3 as type to it.
>
> * gcc.c-torture/execute/pr84478.c: New test.
>
> --- gcc/gimple-fold.c.jj 2018-02-19 19:57:03.424279589 +0100
> +++ gcc/gimple-fold.c 2018-02-20 16:21:34.583020305 +0100
> @@ -1283,10 +1283,11 @@ gimple_fold_builtin_memset (gimple_stmt_
> value of ARG in LENGTH[0] and LENGTH[1], respectively.
> If ARG is an SSA name variable, follow its use-def chains. When
> TYPE == 0, if LENGTH[1] is not equal to the length we determine or
> - if we are unable to determine the length or value, return False.
> + if we are unable to determine the length or value, return false.
> VISITED is a bitmap of visited variables.
> TYPE is 0 if string length should be obtained, 1 for maximum string
> - length and 2 for maximum value ARG can have.
> + length and 2 for maximum value ARG can have, 3 is like 1, but provide
> + conservatively correct rather than optimistic answer.
> When FUZZY is set and the length of a string cannot be determined,
> the function instead considers as the maximum possible length the
> size of a character array it may refer to.
> @@ -1302,9 +1303,8 @@ get_range_strlen (tree arg, tree length[
> tree var, val = NULL_TREE;
> gimple *def_stmt;
>
> - /* The minimum and maximum length. The MAXLEN pointer stays unchanged
> - but MINLEN may be cleared during the execution of the function. */
> - tree *minlen = length;
> + /* The minimum and maximum length. */
> + tree *const minlen = length;
> tree *const maxlen = length + 1;
>
> if (TREE_CODE (arg) != SSA_NAME)
> @@ -1445,12 +1445,11 @@ get_range_strlen (tree arg, tree length[
> if (!val)
> return false;
>
> - if (minlen
> - && (!*minlen
> - || (type > 0
> - && TREE_CODE (*minlen) == INTEGER_CST
> - && TREE_CODE (val) == INTEGER_CST
> - && tree_int_cst_lt (val, *minlen))))
> + if (!*minlen
> + || (type > 0
> + && TREE_CODE (*minlen) == INTEGER_CST
> + && TREE_CODE (val) == INTEGER_CST
> + && tree_int_cst_lt (val, *minlen)))
> *minlen = val;
>
> if (*maxlen)
> @@ -1503,18 +1502,16 @@ get_range_strlen (tree arg, tree length[
> {
> tree op2 = gimple_assign_rhs2 (def_stmt);
> tree op3 = gimple_assign_rhs3 (def_stmt);
> - return get_range_strlen (op2, length, visited, type, fuzzy, flexp)
> - && get_range_strlen (op3, length, visited, type, fuzzy, flexp);
> + return (get_range_strlen (op2, length, visited, type, fuzzy, flexp)
> + && get_range_strlen (op3, length, visited, type, fuzzy,
> + flexp));
> }
> return false;
>
> case GIMPLE_PHI:
> - {
> - /* All the arguments of the PHI node must have the same constant
> - length. */
> - unsigned i;
> -
> - for (i = 0; i < gimple_phi_num_args (def_stmt); i++)
> + /* All the arguments of the PHI node must have the same constant
> + length. */
> + for (unsigned i = 0; i < gimple_phi_num_args (def_stmt); i++)
> {
> tree arg = gimple_phi_arg (def_stmt, i)->def;
>
> @@ -1529,13 +1526,12 @@ get_range_strlen (tree arg, tree length[
>
> if (!get_range_strlen (arg, length, visited, type, fuzzy, flexp))
> {
> - if (fuzzy)
> + if (fuzzy && type != 3)
> *maxlen = build_all_ones_cst (size_type_node);
> else
> return false;
> }
> }
> - }
> return true;
>
> default:
> @@ -1554,7 +1550,10 @@ get_range_strlen (tree arg, tree length[
> Return true if the range of the string lengths has been obtained
> from the upper bound of an array at the end of a struct. Such
> an array may hold a string that's longer than its upper bound
> - due to it being used as a poor-man's flexible array member. */
> + due to it being used as a poor-man's flexible array member.
> +
> + This function should be only used for warning code, as it doesn't
> + handle PHIs in a conservatively correct way. */
>
> bool
> get_range_strlen (tree arg, tree minmaxlen[2])
> @@ -3533,8 +3532,12 @@ gimple_fold_builtin_strlen (gimple_stmt_
> wide_int minlen;
> wide_int maxlen;
>
> - tree lenrange[2];
> - if (!get_range_strlen (gimple_call_arg (stmt, 0), lenrange)
> + tree lenrange[2] = { NULL_TREE, NULL_TREE };
> + bitmap visited = NULL;
> + bool flexarray = false;
> + if (get_range_strlen (gimple_call_arg (stmt, 0), lenrange, &visited,
> + 3, true, &flexarray)
> + && !flexarray
> && lenrange[0] && TREE_CODE (lenrange[0]) == INTEGER_CST
> && lenrange[1] && TREE_CODE (lenrange[1]) == INTEGER_CST)
> {
> @@ -3554,6 +3557,9 @@ gimple_fold_builtin_strlen (gimple_stmt_
> maxlen = wi::to_wide (max_object_size (), prec) - 2;
> }
>
> + if (visited)
> + BITMAP_FREE (visited);
> +
> if (minlen == maxlen)
> {
> lenrange[0] = force_gimple_operand_gsi (gsi, lenrange[0], true, NULL,
> --- gcc/testsuite/gcc.c-torture/execute/pr84478.c.jj 2018-02-20 16:32:00.683086212 +0100
> +++ gcc/testsuite/gcc.c-torture/execute/pr84478.c 2018-02-20 16:31:33.497081640 +0100
> @@ -0,0 +1,49 @@
> +/* PR tree-optimization/84478 */
> +
> +long poolptr;
> +unsigned char *strpool;
> +static const char *poolfilearr[] = {
> + "mu",
> + "",
> +#define A "x",
> +#define B A "xx", A A "xxx", A A A A A
> +#define C B B B B B B B B B B
> +#define D C C C C C C C C C C
> + D C C C C C C C B B B
> + ((void *)0)
> +};
> +
> +__attribute__((noipa)) long
> +makestring (void)
> +{
> + return 1;
> +}
> +
> +__attribute__((noipa)) long
> +loadpoolstrings (long spare_size)
> +{
> + const char *s;
> + long g = 0;
> + int i = 0, j = 0;
> + while ((s = poolfilearr[j++]))
> + {
> + int l = __builtin_strlen (s);
> + i += l;
> + if (i >= spare_size) return 0;
> + while (l-- > 0) strpool[poolptr++] = *s++;
> + g = makestring ();
> + }
> + return g;
> +}
> +
> +int
> +main ()
> +{
> + strpool = __builtin_malloc (4000);
> + if (!strpool)
> + return 0;
> + asm volatile ("" : : : "memory");
> + volatile int r = loadpoolstrings (4000);
> + __builtin_free (strpool);
> + return 0;
> +}
>
> Jakub
>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: gcc-84478.diff
Type: text/x-patch
Size: 2436 bytes
Desc: not available
URL: <http://gcc.gnu.org/pipermail/gcc-patches/attachments/20180220/b26d10d3/attachment.bin>
More information about the Gcc-patches
mailing list