* [PATCH] Fix PR79345, better uninit warnings for memory @ 2017-03-01 14:03 Richard Biener 2017-03-01 19:33 ` [PATCH] Avoid UB in insn-emit.c (PR tree-optimization/79345) Jakub Jelinek ` (2 more replies) 0 siblings, 3 replies; 10+ messages in thread From: Richard Biener @ 2017-03-01 14:03 UTC (permalink / raw) To: gcc-patches The following addresses a regression in uninit warnings that happens because clobber stmts preclude the very simple-minded support we have for memory. The patch fixes this by instead implementing uninit warnings for memory properly, using the alias oracle walk_aliased_vdefs helper. The patch adds better limiting to that interface and fixes one false positive in fixed-value.c. Two other false positives are fixed by the wide-int.h patch posted a few hours ago and a patch to genemit from Jakub. Bootstrap and regtest running on x86_64-unknown-linux-gnu with those prerequesites included. One issue with the patch is duplicate warnings as TREE_NO_WARNING doesn't work very well on tcc_reference trees which are not shared. A followup could use some sort of hash table to mitigate this a bit. OTOH for maybe-uninit uses multiple locations may be in need to be fixed to silence the warning. Another thing is that we walk the function in random (BB) order and thus the alias oracle walk limiting may result in warnings popping up and going away in less predictable order (and also be reported in odd order, like not all must-uninits first). Comments? I realize this may introduce (a lot of?) false positives quite late in the game, more aggressive limiting, like to 2 disambiguations, could solve the testcase in the PR and give up most of the times while preserving the non-walking case of the old code. Richard. 2017-03-01 Richard Biener <rguenther@suse.de> * tree-ssa-alias.c (walk_aliased_vdefs_1): Take a limit param and abort the walk, returning -1 if it is hit. (walk_aliased_vdefs): Take a limit param and pass it on. * tree-ssa-alias.h (walk_aliased_vdefs): Add a limit param, defaulting to 0 and return a signed int. * tree-ssa-uninit.c (struct check_defs_data): New struct. (check_defs): New helper. (warn_uninitialized_vars): Use walk_aliased_vdefs to warn about uninitialized memory. * fixed-value.c (fixed_from_string): Use ulow/uhigh to avoid bogus uninitialized warning. (fixed_convert_from_real): Likewise. * g++.dg/warn/Wuninitialized-7.C: New testcase. * c-c++-common/ubsan/bounds-2.c: Add -Wno-uninitialized. * gcc.dg/uninit-pr19430-2.c: Add expected warning. Index: gcc/tree-ssa-alias.c =================================================================== --- gcc/tree-ssa-alias.c (revision 245803) +++ gcc/tree-ssa-alias.c (working copy) @@ -2897,13 +2897,15 @@ walk_non_aliased_vuses (ao_ref *ref, tre PHI argument (but only one walk continues on merge points), the return value is true if any of the walks was successful. - The function returns the number of statements walked. */ + The function returns the number of statements walked or -1 if + LIMIT stmts were walked and the walk was aborted at this point. + If LIMIT is zero the walk is not aborted. */ -static unsigned int +static int walk_aliased_vdefs_1 (ao_ref *ref, tree vdef, bool (*walker)(ao_ref *, tree, void *), void *data, bitmap *visited, unsigned int cnt, - bool *function_entry_reached) + bool *function_entry_reached, unsigned limit) { do { @@ -2925,14 +2927,22 @@ walk_aliased_vdefs_1 (ao_ref *ref, tree if (!*visited) *visited = BITMAP_ALLOC (NULL); for (i = 0; i < gimple_phi_num_args (def_stmt); ++i) - cnt += walk_aliased_vdefs_1 (ref, gimple_phi_arg_def (def_stmt, i), - walker, data, visited, 0, - function_entry_reached); + { + int res = walk_aliased_vdefs_1 (ref, + gimple_phi_arg_def (def_stmt, i), + walker, data, visited, 0, + function_entry_reached, limit); + if (res == -1) + return -1; + cnt += res; + } return cnt; } /* ??? Do we want to account this to TV_ALIAS_STMT_WALK? */ cnt++; + if (cnt == limit) + return -1; if ((!ref || stmt_may_clobber_ref_p_1 (def_stmt, ref)) && (*walker) (ref, vdef, data)) @@ -2943,14 +2953,14 @@ walk_aliased_vdefs_1 (ao_ref *ref, tree while (1); } -unsigned int +int walk_aliased_vdefs (ao_ref *ref, tree vdef, bool (*walker)(ao_ref *, tree, void *), void *data, bitmap *visited, - bool *function_entry_reached) + bool *function_entry_reached, unsigned int limit) { bitmap local_visited = NULL; - unsigned int ret; + int ret; timevar_push (TV_ALIAS_STMT_WALK); @@ -2959,7 +2969,7 @@ walk_aliased_vdefs (ao_ref *ref, tree vd ret = walk_aliased_vdefs_1 (ref, vdef, walker, data, visited ? visited : &local_visited, 0, - function_entry_reached); + function_entry_reached, limit); if (local_visited) BITMAP_FREE (local_visited); Index: gcc/tree-ssa-alias.h =================================================================== --- gcc/tree-ssa-alias.h (revision 245803) +++ gcc/tree-ssa-alias.h (working copy) @@ -131,10 +131,11 @@ extern void *walk_non_aliased_vuses (ao_ void *(*)(ao_ref *, tree, void *, bool *), tree (*)(tree), void *); -extern unsigned int walk_aliased_vdefs (ao_ref *, tree, - bool (*)(ao_ref *, tree, void *), - void *, bitmap *, - bool *function_entry_reached = NULL); +extern int walk_aliased_vdefs (ao_ref *, tree, + bool (*)(ao_ref *, tree, void *), + void *, bitmap *, + bool *function_entry_reached = NULL, + unsigned int limit = 0); extern void dump_alias_info (FILE *); extern void debug_alias_info (void); extern void dump_points_to_solution (FILE *, struct pt_solution *); Index: gcc/testsuite/g++.dg/warn/Wuninitialized-7.C =================================================================== --- gcc/testsuite/g++.dg/warn/Wuninitialized-7.C (nonexistent) +++ gcc/testsuite/g++.dg/warn/Wuninitialized-7.C (working copy) @@ -0,0 +1,20 @@ +// { dg-do compile } +// { dg-require-effective-target c++11 } +// { dg-options "-O -Wuninitialized" } + +struct A { + A (int); +}; + +struct B: A { + const bool x = true; + + B (): A (x ? 3 : 7) { } // { dg-warning "x. is used uninitialized" } +}; + +void f (void*); +void g () +{ + B b; + f (&b); +} Index: gcc/fixed-value.c =================================================================== --- gcc/fixed-value.c (revision 245803) +++ gcc/fixed-value.c (working copy) @@ -130,8 +130,8 @@ fixed_from_string (FIXED_VALUE_TYPE *f, real_arithmetic (&fixed_value, MULT_EXPR, &real_value, &base_value); wide_int w = real_to_integer (&fixed_value, &fail, GET_MODE_PRECISION (mode)); - f->data.low = w.elt (0); - f->data.high = w.elt (1); + f->data.low = w.ulow (); + f->data.high = w.uhigh (); if (temp == FIXED_MAX_EPS && ALL_FRACT_MODE_P (f->mode)) { @@ -1049,8 +1049,8 @@ fixed_convert_from_real (FIXED_VALUE_TYP wide_int w = real_to_integer (&fixed_value, &fail, GET_MODE_PRECISION (mode)); - f->data.low = w.elt (0); - f->data.high = w.elt (1); + f->data.low = w.ulow (); + f->data.high = w.uhigh (); temp = check_real_for_fixed_mode (&real_value, mode); if (temp == FIXED_UNDERFLOW) /* Minimum. */ { Index: gcc/testsuite/c-c++-common/ubsan/bounds-2.c =================================================================== --- gcc/testsuite/c-c++-common/ubsan/bounds-2.c (revision 245803) +++ gcc/testsuite/c-c++-common/ubsan/bounds-2.c (working copy) @@ -1,5 +1,5 @@ /* { dg-do run } */ -/* { dg-options "-fsanitize=bounds -Wall -Wextra -Wno-unused -Wno-array-bounds" } */ +/* { dg-options "-fsanitize=bounds -Wall -Wextra -Wno-unused -Wno-array-bounds -Wno-uninitialized" } */ /* Test runtime errors. */ Index: gcc/testsuite/gcc.dg/uninit-pr19430-2.c =================================================================== --- gcc/testsuite/gcc.dg/uninit-pr19430-2.c (revision 245803) +++ gcc/testsuite/gcc.dg/uninit-pr19430-2.c (working copy) @@ -10,7 +10,7 @@ int foo (int b) p = &i; q = &j; if (b) - x = p; + x = p; /* { dg-warning "i. may be used uninitialized" } */ else x = q; return *x; Index: gcc/tree-ssa-uninit.c =================================================================== --- gcc/tree-ssa-uninit.c (revision 245803) +++ gcc/tree-ssa-uninit.c (working copy) @@ -191,11 +191,39 @@ warn_uninit (enum opt_code wc, tree t, t } } +struct check_defs_data +{ + /* If we found any may-defs besides must-def clobbers. */ + bool found_may_defs; +}; + +/* Callback for walk_aliased_vdefs. */ + +static bool +check_defs (ao_ref *ref, tree vdef, void *data_) +{ + check_defs_data *data = (check_defs_data *)data_; + gimple *def_stmt = SSA_NAME_DEF_STMT (vdef); + /* If this is a clobber then if it is not a kill walk past it. */ + if (gimple_clobber_p (def_stmt)) + { + if (stmt_kills_ref_p (def_stmt, ref)) + return true; + return false; + } + /* Found a may-def on this path. */ + data->found_may_defs = true; + return true; +} + static unsigned int warn_uninitialized_vars (bool warn_possibly_uninitialized) { gimple_stmt_iterator gsi; basic_block bb; + unsigned int vdef_cnt = 0; + unsigned int oracle_cnt = 0; + unsigned limit = 0; FOR_EACH_BB_FN (bb, cfun) { @@ -236,39 +264,73 @@ warn_uninitialized_vars (bool warn_possi stmt, UNKNOWN_LOCATION); } - /* For memory the only cheap thing we can do is see if we - have a use of the default def of the virtual operand. - ??? Not so cheap would be to use the alias oracle via - walk_aliased_vdefs, if we don't find any aliasing vdef - warn as is-used-uninitialized, if we don't find an aliasing - vdef that kills our use (stmt_kills_ref_p), warn as - may-be-used-uninitialized. But this walk is quadratic and - so must be limited which means we would miss warning - opportunities. */ - use = gimple_vuse (stmt); - if (use - && gimple_assign_single_p (stmt) - && !gimple_vdef (stmt) - && SSA_NAME_IS_DEFAULT_DEF (use)) + /* For limiting the alias walk below we count all + vdefs in the function. We then give the alias walk an + overall budget (and simply not warn for further stmts). + ??? The interface doesn't give us the chance to assign + a maximum cost to each walk (and abort the walk if hit). */ + if (gimple_vdef (stmt)) + vdef_cnt++; + + if (gimple_assign_load_p (stmt) + && gimple_has_location (stmt)) { tree rhs = gimple_assign_rhs1 (stmt); - tree base = get_base_address (rhs); + if (TREE_NO_WARNING (rhs)) + continue; + + ao_ref ref; + ao_ref_init (&ref, rhs); /* Do not warn if it can be initialized outside this function. */ + tree base = ao_ref_base (&ref); if (!VAR_P (base) || DECL_HARD_REGISTER (base) - || is_global_var (base)) + || is_global_var (base) + || TREE_NO_WARNING (base)) + continue; + + /* Limit the walking to a constant number of stmts after + we overcommit quadratic behavior for small functions + and O(n) behavior. */ + if (oracle_cnt > 128 * 128 + && oracle_cnt > vdef_cnt * 2) + limit = 32; + check_defs_data data; + data.found_may_defs = false; + use = gimple_vuse (stmt); + int res = walk_aliased_vdefs (&ref, use, + check_defs, &data, NULL, + NULL, limit); + if (res == -1) + { + oracle_cnt += limit; + continue; + } + oracle_cnt += res; + if (data.found_may_defs) continue; + /* We didn't find any may-defs so on all paths either + reached function entry or a killing clobber. */ + location_t location + = linemap_resolve_location (line_table, gimple_location (stmt), + LRK_SPELLING_LOCATION, NULL); if (always_executed) - warn_uninit (OPT_Wuninitialized, use, gimple_assign_rhs1 (stmt), - base, "%qE is used uninitialized in this function", - stmt, UNKNOWN_LOCATION); + { + if (warning_at (location, OPT_Wuninitialized, + "%qE is used uninitialized in this function", + rhs)) + /* ??? This is only effective for decls as in + gcc.dg/uninit-B-O0.c. Avoid doing this for + maybe-uninit uses as it may hide important + locations. */ + TREE_NO_WARNING (rhs) = 1; + } else if (warn_possibly_uninitialized) - warn_uninit (OPT_Wmaybe_uninitialized, use, - gimple_assign_rhs1 (stmt), base, - "%qE may be used uninitialized in this function", - stmt, UNKNOWN_LOCATION); + warning_at (location, OPT_Wmaybe_uninitialized, + "%qE may be used uninitialized in this function", + rhs); } } } ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH] Avoid UB in insn-emit.c (PR tree-optimization/79345) 2017-03-01 14:03 [PATCH] Fix PR79345, better uninit warnings for memory Richard Biener @ 2017-03-01 19:33 ` Jakub Jelinek 2017-03-02 8:01 ` Richard Biener 2017-03-02 10:36 ` [PATCH] Fix PR79345, better uninit warnings for memory Jakub Jelinek 2017-03-07 18:12 ` [PATCH] Fix PR79345, better uninit warnings for memory Jeff Law 2 siblings, 1 reply; 10+ messages in thread From: Jakub Jelinek @ 2017-03-01 19:33 UTC (permalink / raw) To: Richard Biener; +Cc: gcc-patches On Wed, Mar 01, 2017 at 03:03:29PM +0100, Richard Biener wrote: > The patch adds better limiting to that interface and fixes one > false positive in fixed-value.c. Two other false positives are > fixed by the wide-int.h patch posted a few hours ago and a patch > to genemit from Jakub. Here is that patch. Right now insn-emit.c for match_scratch operands in expanders emits rtx operand7 ATTRIBUTE_UNUSED; ... rtx operands[8]; operands[0] = operand0; ... // but no operands[7] = something here; ... // C code from *.md file (which typically doesn't touch operands[7]) ... operand7 = operands[7]; (void) operand7; ... // generated code that doesn't use operand7 This triggers -Wuninitialized warning with Richard's patch and is really UB, even when we actually don't use it and so hopefully optimize away. The following patch just removes all operand7 and operands[7] references from the generated code (i.e. operands that are for match_scratch). Usually match_scratch numbers come after match_operand/match_dup etc. numbers, but as can be seen, there are few spots where that is not the case. The patch adds verification of this requirement in genemit and then fixes the issues it has diagnosed. Bootstrapped/regtested on x86_64-linux and i686-linux, plus tested with make s-emit in a cross to {powerpc64,aarch64,armv7hl,sparc64,s390x,cris,sh,ia64,hppa,mips}-linux, ok for trunk? 2017-03-01 Jakub Jelinek <jakub@redhat.com> PR tree-optimization/79345 * gensupport.h (struct pattern_stats): Add min_scratch_opno field. * gensupport.c (get_pattern_stats_1) <case MATCH_SCRATCH>: Update it. (get_pattern_stats): Initialize it. * genemit.c (gen_expand): Verify match_scratch numbers come after match_operand/match_dup numbers. * config/i386/i386.md (<s>mul<mode>3_highpart): Swap match_dup and match_scratch numbers. * config/i386/sse.md (avx2_gathersi<mode>, avx2_gatherdi<mode>): Likewise. * config/s390/s390.md (trunctdsd2): Likewise. --- gcc/gensupport.h.jj 2017-01-01 12:45:38.000000000 +0100 +++ gcc/gensupport.h 2017-03-01 12:06:21.816440102 +0100 @@ -199,7 +199,8 @@ struct pattern_stats /* The largest match_dup, match_op_dup or match_par_dup number found. */ int max_dup_opno; - /* The largest match_scratch number found. */ + /* The smallest and largest match_scratch number found. */ + int min_scratch_opno; int max_scratch_opno; /* The number of times match_dup, match_op_dup or match_par_dup appears --- gcc/gensupport.c.jj 2017-01-05 22:10:31.000000000 +0100 +++ gcc/gensupport.c 2017-03-01 12:21:24.830327207 +0100 @@ -3000,6 +3000,10 @@ get_pattern_stats_1 (struct pattern_stat break; case MATCH_SCRATCH: + if (stats->min_scratch_opno == -1) + stats->min_scratch_opno = XINT (x, 0); + else + stats->min_scratch_opno = MIN (stats->min_scratch_opno, XINT (x, 0)); stats->max_scratch_opno = MAX (stats->max_scratch_opno, XINT (x, 0)); break; @@ -3032,6 +3036,7 @@ get_pattern_stats (struct pattern_stats stats->max_opno = -1; stats->max_dup_opno = -1; + stats->min_scratch_opno = -1; stats->max_scratch_opno = -1; stats->num_dups = 0; --- gcc/genemit.c.jj 2017-01-01 12:45:35.000000000 +0100 +++ gcc/genemit.c 2017-03-01 12:16:28.391343302 +0100 @@ -448,6 +448,10 @@ gen_expand (md_rtx_info *info) /* Find out how many operands this function has. */ get_pattern_stats (&stats, XVEC (expand, 1)); + if (stats.min_scratch_opno != -1 + && stats.min_scratch_opno <= MAX (stats.max_opno, stats.max_dup_opno)) + fatal_at (info->loc, "define_expand for %s needs to have match_scratch " + "numbers above all other operands", XSTR (expand, 0)); /* Output the function name and argument declarations. */ printf ("rtx\ngen_%s (", XSTR (expand, 0)); @@ -479,8 +483,6 @@ gen_expand (md_rtx_info *info) make a local variable. */ for (i = stats.num_generator_args; i <= stats.max_dup_opno; i++) printf (" rtx operand%d;\n", i); - for (; i <= stats.max_scratch_opno; i++) - printf (" rtx operand%d ATTRIBUTE_UNUSED;\n", i); printf (" rtx_insn *_val = 0;\n"); printf (" start_sequence ();\n"); @@ -516,7 +518,7 @@ gen_expand (md_rtx_info *info) (unless we aren't going to use them at all). */ if (XVEC (expand, 1) != 0) { - for (i = 0; i < stats.num_operand_vars; i++) + for (i = 0; i <= MAX (stats.max_opno, stats.max_dup_opno); i++) { printf (" operand%d = operands[%d];\n", i, i); printf (" (void) operand%d;\n", i); --- gcc/config/i386/i386.md.jj 2017-02-22 18:15:48.000000000 +0100 +++ gcc/config/i386/i386.md 2017-03-01 12:23:22.882736837 +0100 @@ -7364,11 +7364,11 @@ (define_expand "<s>mul<mode>3_highpart" (match_operand:SWI48 1 "nonimmediate_operand")) (any_extend:<DWI> (match_operand:SWI48 2 "register_operand"))) - (match_dup 4)))) - (clobber (match_scratch:SWI48 3)) + (match_dup 3)))) + (clobber (match_scratch:SWI48 4)) (clobber (reg:CC FLAGS_REG))])] "" - "operands[4] = GEN_INT (GET_MODE_BITSIZE (<MODE>mode));") + "operands[3] = GEN_INT (GET_MODE_BITSIZE (<MODE>mode));") (define_insn "*<s>muldi3_highpart_1" [(set (match_operand:DI 0 "register_operand" "=d") --- gcc/config/i386/sse.md.jj 2017-02-07 16:41:32.000000000 +0100 +++ gcc/config/i386/sse.md 2017-03-01 12:24:57.193471018 +0100 @@ -18873,7 +18873,7 @@ (define_expand "avx2_gathersi<mode>" (unspec:VEC_GATHER_MODE [(match_operand:VEC_GATHER_MODE 1 "register_operand") (mem:<ssescalarmode> - (match_par_dup 7 + (match_par_dup 6 [(match_operand 2 "vsib_address_operand") (match_operand:<VEC_GATHER_IDXSI> 3 "register_operand") @@ -18881,10 +18881,10 @@ (define_expand "avx2_gathersi<mode>" (mem:BLK (scratch)) (match_operand:VEC_GATHER_MODE 4 "register_operand")] UNSPEC_GATHER)) - (clobber (match_scratch:VEC_GATHER_MODE 6))])] + (clobber (match_scratch:VEC_GATHER_MODE 7))])] "TARGET_AVX2" { - operands[7] + operands[6] = gen_rtx_UNSPEC (Pmode, gen_rtvec (3, operands[2], operands[3], operands[5]), UNSPEC_VSIBADDR); }) @@ -18934,7 +18934,7 @@ (define_expand "avx2_gatherdi<mode>" (unspec:VEC_GATHER_MODE [(match_operand:<VEC_GATHER_SRCDI> 1 "register_operand") (mem:<ssescalarmode> - (match_par_dup 7 + (match_par_dup 6 [(match_operand 2 "vsib_address_operand") (match_operand:<VEC_GATHER_IDXDI> 3 "register_operand") @@ -18942,10 +18942,10 @@ (define_expand "avx2_gatherdi<mode>" (mem:BLK (scratch)) (match_operand:<VEC_GATHER_SRCDI> 4 "register_operand")] UNSPEC_GATHER)) - (clobber (match_scratch:VEC_GATHER_MODE 6))])] + (clobber (match_scratch:VEC_GATHER_MODE 7))])] "TARGET_AVX2" { - operands[7] + operands[6] = gen_rtx_UNSPEC (Pmode, gen_rtvec (3, operands[2], operands[3], operands[5]), UNSPEC_VSIBADDR); }) --- gcc/config/s390/s390.md.jj 2017-02-06 13:32:14.000000000 +0100 +++ gcc/config/s390/s390.md 2017-03-01 12:45:07.184184616 +0100 @@ -5074,15 +5074,15 @@ (define_insn "truncddsd2" (define_expand "trunctdsd2" [(parallel - [(set (match_dup 3) + [(set (match_dup 2) (float_truncate:DD (match_operand:TD 1 "register_operand" ""))) (unspec:DI [(const_int DFP_RND_PREP_FOR_SHORT_PREC)] UNSPEC_ROUND) - (clobber (match_scratch:TD 2 ""))]) + (clobber (match_scratch:TD 3 ""))]) (set (match_operand:SD 0 "register_operand" "") - (float_truncate:SD (match_dup 3)))] + (float_truncate:SD (match_dup 2)))] "TARGET_HARD_DFP" { - operands[3] = gen_reg_rtx (DDmode); + operands[2] = gen_reg_rtx (DDmode); }) ; Jakub ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] Avoid UB in insn-emit.c (PR tree-optimization/79345) 2017-03-01 19:33 ` [PATCH] Avoid UB in insn-emit.c (PR tree-optimization/79345) Jakub Jelinek @ 2017-03-02 8:01 ` Richard Biener 0 siblings, 0 replies; 10+ messages in thread From: Richard Biener @ 2017-03-02 8:01 UTC (permalink / raw) To: Jakub Jelinek; +Cc: gcc-patches On Wed, 1 Mar 2017, Jakub Jelinek wrote: > On Wed, Mar 01, 2017 at 03:03:29PM +0100, Richard Biener wrote: > > The patch adds better limiting to that interface and fixes one > > false positive in fixed-value.c. Two other false positives are > > fixed by the wide-int.h patch posted a few hours ago and a patch > > to genemit from Jakub. > > Here is that patch. Right now insn-emit.c for match_scratch > operands in expanders emits > rtx operand7 ATTRIBUTE_UNUSED; > ... > rtx operands[8]; > operands[0] = operand0; > ... > // but no operands[7] = something here; > ... > // C code from *.md file (which typically doesn't touch operands[7]) > ... > operand7 = operands[7]; > (void) operand7; > ... > // generated code that doesn't use operand7 > This triggers -Wuninitialized warning with Richard's patch and is really UB, > even when we actually don't use it and so hopefully optimize away. > The following patch just removes all operand7 and operands[7] references > from the generated code (i.e. operands that are for match_scratch). > > Usually match_scratch numbers come after match_operand/match_dup etc. > numbers, but as can be seen, there are few spots where that is not the case. > The patch adds verification of this requirement in genemit and then fixes > the issues it has diagnosed. > > Bootstrapped/regtested on x86_64-linux and i686-linux, plus tested with > make s-emit in a cross to > {powerpc64,aarch64,armv7hl,sparc64,s390x,cris,sh,ia64,hppa,mips}-linux, ok > for trunk? This is ok. Thanks for fixing this, Richard. > 2017-03-01 Jakub Jelinek <jakub@redhat.com> > > PR tree-optimization/79345 > * gensupport.h (struct pattern_stats): Add min_scratch_opno field. > * gensupport.c (get_pattern_stats_1) <case MATCH_SCRATCH>: Update it. > (get_pattern_stats): Initialize it. > * genemit.c (gen_expand): Verify match_scratch numbers come after > match_operand/match_dup numbers. > * config/i386/i386.md (<s>mul<mode>3_highpart): Swap match_dup and > match_scratch numbers. > * config/i386/sse.md (avx2_gathersi<mode>, avx2_gatherdi<mode>): > Likewise. > * config/s390/s390.md (trunctdsd2): Likewise. > > --- gcc/gensupport.h.jj 2017-01-01 12:45:38.000000000 +0100 > +++ gcc/gensupport.h 2017-03-01 12:06:21.816440102 +0100 > @@ -199,7 +199,8 @@ struct pattern_stats > /* The largest match_dup, match_op_dup or match_par_dup number found. */ > int max_dup_opno; > > - /* The largest match_scratch number found. */ > + /* The smallest and largest match_scratch number found. */ > + int min_scratch_opno; > int max_scratch_opno; > > /* The number of times match_dup, match_op_dup or match_par_dup appears > --- gcc/gensupport.c.jj 2017-01-05 22:10:31.000000000 +0100 > +++ gcc/gensupport.c 2017-03-01 12:21:24.830327207 +0100 > @@ -3000,6 +3000,10 @@ get_pattern_stats_1 (struct pattern_stat > break; > > case MATCH_SCRATCH: > + if (stats->min_scratch_opno == -1) > + stats->min_scratch_opno = XINT (x, 0); > + else > + stats->min_scratch_opno = MIN (stats->min_scratch_opno, XINT (x, 0)); > stats->max_scratch_opno = MAX (stats->max_scratch_opno, XINT (x, 0)); > break; > > @@ -3032,6 +3036,7 @@ get_pattern_stats (struct pattern_stats > > stats->max_opno = -1; > stats->max_dup_opno = -1; > + stats->min_scratch_opno = -1; > stats->max_scratch_opno = -1; > stats->num_dups = 0; > > --- gcc/genemit.c.jj 2017-01-01 12:45:35.000000000 +0100 > +++ gcc/genemit.c 2017-03-01 12:16:28.391343302 +0100 > @@ -448,6 +448,10 @@ gen_expand (md_rtx_info *info) > > /* Find out how many operands this function has. */ > get_pattern_stats (&stats, XVEC (expand, 1)); > + if (stats.min_scratch_opno != -1 > + && stats.min_scratch_opno <= MAX (stats.max_opno, stats.max_dup_opno)) > + fatal_at (info->loc, "define_expand for %s needs to have match_scratch " > + "numbers above all other operands", XSTR (expand, 0)); > > /* Output the function name and argument declarations. */ > printf ("rtx\ngen_%s (", XSTR (expand, 0)); > @@ -479,8 +483,6 @@ gen_expand (md_rtx_info *info) > make a local variable. */ > for (i = stats.num_generator_args; i <= stats.max_dup_opno; i++) > printf (" rtx operand%d;\n", i); > - for (; i <= stats.max_scratch_opno; i++) > - printf (" rtx operand%d ATTRIBUTE_UNUSED;\n", i); > printf (" rtx_insn *_val = 0;\n"); > printf (" start_sequence ();\n"); > > @@ -516,7 +518,7 @@ gen_expand (md_rtx_info *info) > (unless we aren't going to use them at all). */ > if (XVEC (expand, 1) != 0) > { > - for (i = 0; i < stats.num_operand_vars; i++) > + for (i = 0; i <= MAX (stats.max_opno, stats.max_dup_opno); i++) > { > printf (" operand%d = operands[%d];\n", i, i); > printf (" (void) operand%d;\n", i); > --- gcc/config/i386/i386.md.jj 2017-02-22 18:15:48.000000000 +0100 > +++ gcc/config/i386/i386.md 2017-03-01 12:23:22.882736837 +0100 > @@ -7364,11 +7364,11 @@ (define_expand "<s>mul<mode>3_highpart" > (match_operand:SWI48 1 "nonimmediate_operand")) > (any_extend:<DWI> > (match_operand:SWI48 2 "register_operand"))) > - (match_dup 4)))) > - (clobber (match_scratch:SWI48 3)) > + (match_dup 3)))) > + (clobber (match_scratch:SWI48 4)) > (clobber (reg:CC FLAGS_REG))])] > "" > - "operands[4] = GEN_INT (GET_MODE_BITSIZE (<MODE>mode));") > + "operands[3] = GEN_INT (GET_MODE_BITSIZE (<MODE>mode));") > > (define_insn "*<s>muldi3_highpart_1" > [(set (match_operand:DI 0 "register_operand" "=d") > --- gcc/config/i386/sse.md.jj 2017-02-07 16:41:32.000000000 +0100 > +++ gcc/config/i386/sse.md 2017-03-01 12:24:57.193471018 +0100 > @@ -18873,7 +18873,7 @@ (define_expand "avx2_gathersi<mode>" > (unspec:VEC_GATHER_MODE > [(match_operand:VEC_GATHER_MODE 1 "register_operand") > (mem:<ssescalarmode> > - (match_par_dup 7 > + (match_par_dup 6 > [(match_operand 2 "vsib_address_operand") > (match_operand:<VEC_GATHER_IDXSI> > 3 "register_operand") > @@ -18881,10 +18881,10 @@ (define_expand "avx2_gathersi<mode>" > (mem:BLK (scratch)) > (match_operand:VEC_GATHER_MODE 4 "register_operand")] > UNSPEC_GATHER)) > - (clobber (match_scratch:VEC_GATHER_MODE 6))])] > + (clobber (match_scratch:VEC_GATHER_MODE 7))])] > "TARGET_AVX2" > { > - operands[7] > + operands[6] > = gen_rtx_UNSPEC (Pmode, gen_rtvec (3, operands[2], operands[3], > operands[5]), UNSPEC_VSIBADDR); > }) > @@ -18934,7 +18934,7 @@ (define_expand "avx2_gatherdi<mode>" > (unspec:VEC_GATHER_MODE > [(match_operand:<VEC_GATHER_SRCDI> 1 "register_operand") > (mem:<ssescalarmode> > - (match_par_dup 7 > + (match_par_dup 6 > [(match_operand 2 "vsib_address_operand") > (match_operand:<VEC_GATHER_IDXDI> > 3 "register_operand") > @@ -18942,10 +18942,10 @@ (define_expand "avx2_gatherdi<mode>" > (mem:BLK (scratch)) > (match_operand:<VEC_GATHER_SRCDI> 4 "register_operand")] > UNSPEC_GATHER)) > - (clobber (match_scratch:VEC_GATHER_MODE 6))])] > + (clobber (match_scratch:VEC_GATHER_MODE 7))])] > "TARGET_AVX2" > { > - operands[7] > + operands[6] > = gen_rtx_UNSPEC (Pmode, gen_rtvec (3, operands[2], operands[3], > operands[5]), UNSPEC_VSIBADDR); > }) > --- gcc/config/s390/s390.md.jj 2017-02-06 13:32:14.000000000 +0100 > +++ gcc/config/s390/s390.md 2017-03-01 12:45:07.184184616 +0100 > @@ -5074,15 +5074,15 @@ (define_insn "truncddsd2" > > (define_expand "trunctdsd2" > [(parallel > - [(set (match_dup 3) > + [(set (match_dup 2) > (float_truncate:DD (match_operand:TD 1 "register_operand" ""))) > (unspec:DI [(const_int DFP_RND_PREP_FOR_SHORT_PREC)] UNSPEC_ROUND) > - (clobber (match_scratch:TD 2 ""))]) > + (clobber (match_scratch:TD 3 ""))]) > (set (match_operand:SD 0 "register_operand" "") > - (float_truncate:SD (match_dup 3)))] > + (float_truncate:SD (match_dup 2)))] > "TARGET_HARD_DFP" > { > - operands[3] = gen_reg_rtx (DDmode); > + operands[2] = gen_reg_rtx (DDmode); > }) > > ; > > > Jakub > > -- Richard Biener <rguenther@suse.de> SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nuernberg) ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] Fix PR79345, better uninit warnings for memory 2017-03-01 14:03 [PATCH] Fix PR79345, better uninit warnings for memory Richard Biener 2017-03-01 19:33 ` [PATCH] Avoid UB in insn-emit.c (PR tree-optimization/79345) Jakub Jelinek @ 2017-03-02 10:36 ` Jakub Jelinek 2017-03-02 11:14 ` Richard Biener 2017-03-07 18:12 ` [PATCH] Fix PR79345, better uninit warnings for memory Jeff Law 2 siblings, 1 reply; 10+ messages in thread From: Jakub Jelinek @ 2017-03-02 10:36 UTC (permalink / raw) To: Richard Biener; +Cc: gcc-patches On Wed, Mar 01, 2017 at 03:03:29PM +0100, Richard Biener wrote: > One issue with the patch is duplicate warnings as TREE_NO_WARNING > doesn't work very well on tcc_reference trees which are not > shared. A followup could use some sort of hash table to mitigate Can't we use TREE_NO_WARNING on get_base_address instead? That is shared... > @@ -2925,14 +2927,22 @@ walk_aliased_vdefs_1 (ao_ref *ref, tree > if (!*visited) > *visited = BITMAP_ALLOC (NULL); > for (i = 0; i < gimple_phi_num_args (def_stmt); ++i) > - cnt += walk_aliased_vdefs_1 (ref, gimple_phi_arg_def (def_stmt, i), > - walker, data, visited, 0, > - function_entry_reached); > + { > + int res = walk_aliased_vdefs_1 (ref, > + gimple_phi_arg_def (def_stmt, i), > + walker, data, visited, 0, > + function_entry_reached, limit); > + if (res == -1) > + return -1; > + cnt += res; > + } This doesn't look right. The recursive call starts with cnt of 0 again, so if say you have limit 20 and cnt 10 when you are about to recurse and that walk does 15 oracle comparisons, it won't return -1, but 15 and you cnt += 15 to get 25 and never reach the limit, so then you walk perhaps another 100000 times. Can't you just pass cnt instead of 0 and then use if (res == -1) return -1; cnt = res; ? Or you'd need to play gaves with decrementing the limit for the recursive call. > + /* For limiting the alias walk below we count all > + vdefs in the function. We then give the alias walk an > + overall budget (and simply not warn for further stmts). > + ??? The interface doesn't give us the chance to assign > + a maximum cost to each walk (and abort the walk if hit). */ I don't understand the last two lines, I thought you've added the ability to the interface above and you actually use it. Otherwise it LGTM, yes, we will end up with new warnings, but unless the alias oracle is wrong, there shouldn't be too many false positives, right? Jakub ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] Fix PR79345, better uninit warnings for memory 2017-03-02 10:36 ` [PATCH] Fix PR79345, better uninit warnings for memory Jakub Jelinek @ 2017-03-02 11:14 ` Richard Biener 2017-03-02 13:40 ` Richard Biener 0 siblings, 1 reply; 10+ messages in thread From: Richard Biener @ 2017-03-02 11:14 UTC (permalink / raw) To: Jakub Jelinek; +Cc: gcc-patches On Thu, 2 Mar 2017, Jakub Jelinek wrote: > On Wed, Mar 01, 2017 at 03:03:29PM +0100, Richard Biener wrote: > > One issue with the patch is duplicate warnings as TREE_NO_WARNING > > doesn't work very well on tcc_reference trees which are not > > shared. A followup could use some sort of hash table to mitigate > > Can't we use TREE_NO_WARNING on get_base_address instead? That is > shared... At least for VAR_DECL based refs, yes (the only ones we currently analyze). For MEM_REF bases it won't work. It would of course only warn once per aggregate and for a maybe-uninit warning it might be the false positive (the patch only sets TREE_NO_WARNING on the always-executed case). For the user maybe we should change the order we walk BBs to RPO and thus warn at the first maybe-uninit point so we can say "further uninit warnings suppressed for X"? > > @@ -2925,14 +2927,22 @@ walk_aliased_vdefs_1 (ao_ref *ref, tree > > if (!*visited) > > *visited = BITMAP_ALLOC (NULL); > > for (i = 0; i < gimple_phi_num_args (def_stmt); ++i) > > - cnt += walk_aliased_vdefs_1 (ref, gimple_phi_arg_def (def_stmt, i), > > - walker, data, visited, 0, > > - function_entry_reached); > > + { > > + int res = walk_aliased_vdefs_1 (ref, > > + gimple_phi_arg_def (def_stmt, i), > > + walker, data, visited, 0, > > + function_entry_reached, limit); > > + if (res == -1) > > + return -1; > > + cnt += res; > > + } > > This doesn't look right. The recursive call starts with cnt of 0 again, > so if say you have limit 20 and cnt 10 when you are about to recurse and > that walk does 15 oracle comparisons, it won't return -1, but 15 and > you cnt += 15 to get 25 and never reach the limit, so then you walk perhaps > another 100000 times. > Can't you just pass cnt instead of 0 and then use > if (res == -1) > return -1; > cnt = res; > ? Or you'd need to play gaves with decrementing the limit for the recursive > call. Ah, indeed. Will fix (passing cnt). > > + /* For limiting the alias walk below we count all > > + vdefs in the function. We then give the alias walk an > > + overall budget (and simply not warn for further stmts). > > + ??? The interface doesn't give us the chance to assign > > + a maximum cost to each walk (and abort the walk if hit). */ > > I don't understand the last two lines, I thought you've added the ability > to the interface above and you actually use it. Will adjust the comment. > Otherwise it LGTM, yes, we will end up with new warnings, but unless the > alias oracle is wrong, there shouldn't be too many false positives, right? Yes, we are not warning for the case where there is at least one path with an initialization, like void foo(int b) { S s; if (b) s.a = 1; return s.a; } will not warn (trivial to add but not necessary for the regression fix). We also do not warn for pointer-based accesses (so unfortunately no fix for PR2972). Also trivial to add, but see PR79814 for more bootstrap fallout. Thanks, Richard. ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] Fix PR79345, better uninit warnings for memory 2017-03-02 11:14 ` Richard Biener @ 2017-03-02 13:40 ` Richard Biener 2017-03-18 18:21 ` [BUILDROBOT] Maybe uninitialized warnings in mips targets (was: [PATCH] Fix PR79345, better uninit warnings for memory) Jan-Benedict Glaw 0 siblings, 1 reply; 10+ messages in thread From: Richard Biener @ 2017-03-02 13:40 UTC (permalink / raw) To: Jakub Jelinek; +Cc: gcc-patches On Thu, 2 Mar 2017, Richard Biener wrote: > On Thu, 2 Mar 2017, Jakub Jelinek wrote: > > > On Wed, Mar 01, 2017 at 03:03:29PM +0100, Richard Biener wrote: > > > One issue with the patch is duplicate warnings as TREE_NO_WARNING > > > doesn't work very well on tcc_reference trees which are not > > > shared. A followup could use some sort of hash table to mitigate > > > > Can't we use TREE_NO_WARNING on get_base_address instead? That is > > shared... > > At least for VAR_DECL based refs, yes (the only ones we currently > analyze). For MEM_REF bases it won't work. It would of > course only warn once per aggregate and > for a maybe-uninit warning it might be the false positive > (the patch only sets TREE_NO_WARNING on the always-executed case). > > For the user maybe we should change the order we walk BBs to RPO > and thus warn at the first maybe-uninit point so we can say > "further uninit warnings suppressed for X"? > > > > @@ -2925,14 +2927,22 @@ walk_aliased_vdefs_1 (ao_ref *ref, tree > > > if (!*visited) > > > *visited = BITMAP_ALLOC (NULL); > > > for (i = 0; i < gimple_phi_num_args (def_stmt); ++i) > > > - cnt += walk_aliased_vdefs_1 (ref, gimple_phi_arg_def (def_stmt, i), > > > - walker, data, visited, 0, > > > - function_entry_reached); > > > + { > > > + int res = walk_aliased_vdefs_1 (ref, > > > + gimple_phi_arg_def (def_stmt, i), > > > + walker, data, visited, 0, > > > + function_entry_reached, limit); > > > + if (res == -1) > > > + return -1; > > > + cnt += res; > > > + } > > > > This doesn't look right. The recursive call starts with cnt of 0 again, > > so if say you have limit 20 and cnt 10 when you are about to recurse and > > that walk does 15 oracle comparisons, it won't return -1, but 15 and > > you cnt += 15 to get 25 and never reach the limit, so then you walk perhaps > > another 100000 times. > > Can't you just pass cnt instead of 0 and then use > > if (res == -1) > > return -1; > > cnt = res; > > ? Or you'd need to play gaves with decrementing the limit for the recursive > > call. > > Ah, indeed. Will fix (passing cnt). > > > > + /* For limiting the alias walk below we count all > > > + vdefs in the function. We then give the alias walk an > > > + overall budget (and simply not warn for further stmts). > > > + ??? The interface doesn't give us the chance to assign > > > + a maximum cost to each walk (and abort the walk if hit). */ > > > > I don't understand the last two lines, I thought you've added the ability > > to the interface above and you actually use it. > > Will adjust the comment. > > > Otherwise it LGTM, yes, we will end up with new warnings, but unless the > > alias oracle is wrong, there shouldn't be too many false positives, right? > > Yes, we are not warning for the case where there is at least one path > with an initialization, like > > void foo(int b) > { > S s; > if (b) > s.a = 1; > return s.a; > } > > will not warn (trivial to add but not necessary for the regression fix). > > We also do not warn for pointer-based accesses (so unfortunately no > fix for PR2972). Also trivial to add, but see PR79814 for more > bootstrap fallout. On IRC we decided to wait&see for the TREE_NO_WARNING issue. So the following is what I committed. Bootstrapped / tested on x86_64-unknown-linux-gnu. Richard. 2017-03-02 Richard Biener <rguenther@suse.de> PR tree-optimization/79345 PR c++/42000 * tree-ssa-alias.c (walk_aliased_vdefs_1): Take a limit param and abort the walk, returning -1 if it is hit. (walk_aliased_vdefs): Take a limit param and pass it on. * tree-ssa-alias.h (walk_aliased_vdefs): Add a limit param, defaulting to 0 and return a signed int. * tree-ssa-uninit.c (struct check_defs_data): New struct. (check_defs): New helper. (warn_uninitialized_vars): Use walk_aliased_vdefs to warn about uninitialized memory. * fixed-value.c (fixed_from_string): Use ulow/uhigh to avoid bogus uninitialized warning. (fixed_convert_from_real): Likewise. * g++.dg/warn/Wuninitialized-7.C: New testcase. * c-c++-common/ubsan/bounds-2.c: Add -Wno-uninitialized. * gcc.dg/uninit-pr19430-2.c: Add expected warning. Index: gcc/tree-ssa-alias.h =================================================================== --- gcc/tree-ssa-alias.h (revision 245803) +++ gcc/tree-ssa-alias.h (working copy) @@ -131,10 +131,11 @@ extern void *walk_non_aliased_vuses (ao_ void *(*)(ao_ref *, tree, void *, bool *), tree (*)(tree), void *); -extern unsigned int walk_aliased_vdefs (ao_ref *, tree, - bool (*)(ao_ref *, tree, void *), - void *, bitmap *, - bool *function_entry_reached = NULL); +extern int walk_aliased_vdefs (ao_ref *, tree, + bool (*)(ao_ref *, tree, void *), + void *, bitmap *, + bool *function_entry_reached = NULL, + unsigned int limit = 0); extern void dump_alias_info (FILE *); extern void debug_alias_info (void); extern void dump_points_to_solution (FILE *, struct pt_solution *); Index: gcc/testsuite/g++.dg/warn/Wuninitialized-7.C =================================================================== --- gcc/testsuite/g++.dg/warn/Wuninitialized-7.C (nonexistent) +++ gcc/testsuite/g++.dg/warn/Wuninitialized-7.C (working copy) @@ -0,0 +1,20 @@ +// { dg-do compile } +// { dg-require-effective-target c++11 } +// { dg-options "-O -Wuninitialized" } + +struct A { + A (int); +}; + +struct B: A { + const bool x = true; + + B (): A (x ? 3 : 7) { } // { dg-warning "x. is used uninitialized" } +}; + +void f (void*); +void g () +{ + B b; + f (&b); +} Index: gcc/fixed-value.c =================================================================== --- gcc/fixed-value.c (revision 245803) +++ gcc/fixed-value.c (working copy) @@ -130,8 +130,8 @@ fixed_from_string (FIXED_VALUE_TYPE *f, real_arithmetic (&fixed_value, MULT_EXPR, &real_value, &base_value); wide_int w = real_to_integer (&fixed_value, &fail, GET_MODE_PRECISION (mode)); - f->data.low = w.elt (0); - f->data.high = w.elt (1); + f->data.low = w.ulow (); + f->data.high = w.uhigh (); if (temp == FIXED_MAX_EPS && ALL_FRACT_MODE_P (f->mode)) { @@ -1049,8 +1049,8 @@ fixed_convert_from_real (FIXED_VALUE_TYP wide_int w = real_to_integer (&fixed_value, &fail, GET_MODE_PRECISION (mode)); - f->data.low = w.elt (0); - f->data.high = w.elt (1); + f->data.low = w.ulow (); + f->data.high = w.uhigh (); temp = check_real_for_fixed_mode (&real_value, mode); if (temp == FIXED_UNDERFLOW) /* Minimum. */ { Index: gcc/testsuite/c-c++-common/ubsan/bounds-2.c =================================================================== --- gcc/testsuite/c-c++-common/ubsan/bounds-2.c (revision 245803) +++ gcc/testsuite/c-c++-common/ubsan/bounds-2.c (working copy) @@ -1,5 +1,5 @@ /* { dg-do run } */ -/* { dg-options "-fsanitize=bounds -Wall -Wextra -Wno-unused -Wno-array-bounds" } */ +/* { dg-options "-fsanitize=bounds -Wall -Wextra -Wno-unused -Wno-array-bounds -Wno-uninitialized" } */ /* Test runtime errors. */ Index: gcc/testsuite/gcc.dg/uninit-pr19430-2.c =================================================================== --- gcc/testsuite/gcc.dg/uninit-pr19430-2.c (revision 245803) +++ gcc/testsuite/gcc.dg/uninit-pr19430-2.c (working copy) @@ -10,7 +10,7 @@ int foo (int b) p = &i; q = &j; if (b) - x = p; + x = p; /* { dg-warning "i. may be used uninitialized" } */ else x = q; return *x; Index: gcc/tree-ssa-uninit.c =================================================================== --- gcc/tree-ssa-uninit.c (revision 245833) +++ gcc/tree-ssa-uninit.c (working copy) @@ -191,11 +191,39 @@ warn_uninit (enum opt_code wc, tree t, t } } +struct check_defs_data +{ + /* If we found any may-defs besides must-def clobbers. */ + bool found_may_defs; +}; + +/* Callback for walk_aliased_vdefs. */ + +static bool +check_defs (ao_ref *ref, tree vdef, void *data_) +{ + check_defs_data *data = (check_defs_data *)data_; + gimple *def_stmt = SSA_NAME_DEF_STMT (vdef); + /* If this is a clobber then if it is not a kill walk past it. */ + if (gimple_clobber_p (def_stmt)) + { + if (stmt_kills_ref_p (def_stmt, ref)) + return true; + return false; + } + /* Found a may-def on this path. */ + data->found_may_defs = true; + return true; +} + static unsigned int warn_uninitialized_vars (bool warn_possibly_uninitialized) { gimple_stmt_iterator gsi; basic_block bb; + unsigned int vdef_cnt = 0; + unsigned int oracle_cnt = 0; + unsigned limit = 0; FOR_EACH_BB_FN (bb, cfun) { @@ -236,39 +264,70 @@ warn_uninitialized_vars (bool warn_possi stmt, UNKNOWN_LOCATION); } - /* For memory the only cheap thing we can do is see if we - have a use of the default def of the virtual operand. - ??? Not so cheap would be to use the alias oracle via - walk_aliased_vdefs, if we don't find any aliasing vdef - warn as is-used-uninitialized, if we don't find an aliasing - vdef that kills our use (stmt_kills_ref_p), warn as - may-be-used-uninitialized. But this walk is quadratic and - so must be limited which means we would miss warning - opportunities. */ - use = gimple_vuse (stmt); - if (use - && gimple_assign_single_p (stmt) - && !gimple_vdef (stmt) - && SSA_NAME_IS_DEFAULT_DEF (use)) + /* For limiting the alias walk below we count all + vdefs in the function. */ + if (gimple_vdef (stmt)) + vdef_cnt++; + + if (gimple_assign_load_p (stmt) + && gimple_has_location (stmt)) { tree rhs = gimple_assign_rhs1 (stmt); - tree base = get_base_address (rhs); + if (TREE_NO_WARNING (rhs)) + continue; + + ao_ref ref; + ao_ref_init (&ref, rhs); /* Do not warn if it can be initialized outside this function. */ + tree base = ao_ref_base (&ref); if (!VAR_P (base) || DECL_HARD_REGISTER (base) - || is_global_var (base)) + || is_global_var (base) + || TREE_NO_WARNING (base)) + continue; + + /* Limit the walking to a constant number of stmts after + we overcommit quadratic behavior for small functions + and O(n) behavior. */ + if (oracle_cnt > 128 * 128 + && oracle_cnt > vdef_cnt * 2) + limit = 32; + check_defs_data data; + data.found_may_defs = false; + use = gimple_vuse (stmt); + int res = walk_aliased_vdefs (&ref, use, + check_defs, &data, NULL, + NULL, limit); + if (res == -1) + { + oracle_cnt += limit; + continue; + } + oracle_cnt += res; + if (data.found_may_defs) continue; + /* We didn't find any may-defs so on all paths either + reached function entry or a killing clobber. */ + location_t location + = linemap_resolve_location (line_table, gimple_location (stmt), + LRK_SPELLING_LOCATION, NULL); if (always_executed) - warn_uninit (OPT_Wuninitialized, use, gimple_assign_rhs1 (stmt), - base, "%qE is used uninitialized in this function", - stmt, UNKNOWN_LOCATION); + { + if (warning_at (location, OPT_Wuninitialized, + "%qE is used uninitialized in this function", + rhs)) + /* ??? This is only effective for decls as in + gcc.dg/uninit-B-O0.c. Avoid doing this for + maybe-uninit uses as it may hide important + locations. */ + TREE_NO_WARNING (rhs) = 1; + } else if (warn_possibly_uninitialized) - warn_uninit (OPT_Wmaybe_uninitialized, use, - gimple_assign_rhs1 (stmt), base, - "%qE may be used uninitialized in this function", - stmt, UNKNOWN_LOCATION); + warning_at (location, OPT_Wmaybe_uninitialized, + "%qE may be used uninitialized in this function", + rhs); } } } Index: gcc/tree-ssa-alias.c =================================================================== --- gcc/tree-ssa-alias.c (revision 245833) +++ gcc/tree-ssa-alias.c (working copy) @@ -2897,13 +2897,15 @@ walk_non_aliased_vuses (ao_ref *ref, tre PHI argument (but only one walk continues on merge points), the return value is true if any of the walks was successful. - The function returns the number of statements walked. */ + The function returns the number of statements walked or -1 if + LIMIT stmts were walked and the walk was aborted at this point. + If LIMIT is zero the walk is not aborted. */ -static unsigned int +static int walk_aliased_vdefs_1 (ao_ref *ref, tree vdef, bool (*walker)(ao_ref *, tree, void *), void *data, bitmap *visited, unsigned int cnt, - bool *function_entry_reached) + bool *function_entry_reached, unsigned limit) { do { @@ -2925,14 +2927,22 @@ walk_aliased_vdefs_1 (ao_ref *ref, tree if (!*visited) *visited = BITMAP_ALLOC (NULL); for (i = 0; i < gimple_phi_num_args (def_stmt); ++i) - cnt += walk_aliased_vdefs_1 (ref, gimple_phi_arg_def (def_stmt, i), - walker, data, visited, 0, - function_entry_reached); + { + int res = walk_aliased_vdefs_1 (ref, + gimple_phi_arg_def (def_stmt, i), + walker, data, visited, cnt, + function_entry_reached, limit); + if (res == -1) + return -1; + cnt = res; + } return cnt; } /* ??? Do we want to account this to TV_ALIAS_STMT_WALK? */ cnt++; + if (cnt == limit) + return -1; if ((!ref || stmt_may_clobber_ref_p_1 (def_stmt, ref)) && (*walker) (ref, vdef, data)) @@ -2943,14 +2953,14 @@ walk_aliased_vdefs_1 (ao_ref *ref, tree while (1); } -unsigned int +int walk_aliased_vdefs (ao_ref *ref, tree vdef, bool (*walker)(ao_ref *, tree, void *), void *data, bitmap *visited, - bool *function_entry_reached) + bool *function_entry_reached, unsigned int limit) { bitmap local_visited = NULL; - unsigned int ret; + int ret; timevar_push (TV_ALIAS_STMT_WALK); @@ -2959,7 +2969,7 @@ walk_aliased_vdefs (ao_ref *ref, tree vd ret = walk_aliased_vdefs_1 (ref, vdef, walker, data, visited ? visited : &local_visited, 0, - function_entry_reached); + function_entry_reached, limit); if (local_visited) BITMAP_FREE (local_visited); ^ permalink raw reply [flat|nested] 10+ messages in thread
* [BUILDROBOT] Maybe uninitialized warnings in mips targets (was: [PATCH] Fix PR79345, better uninit warnings for memory) 2017-03-02 13:40 ` Richard Biener @ 2017-03-18 18:21 ` Jan-Benedict Glaw 2017-03-19 16:06 ` [BUILDROBOT] Maybe uninitialized warnings in mips targets Jeff Law 2017-04-04 15:31 ` Jeff Law 0 siblings, 2 replies; 10+ messages in thread From: Jan-Benedict Glaw @ 2017-03-18 18:21 UTC (permalink / raw) To: Richard Biener, Matthew Fortune, Catherine Moore Cc: Jakub Jelinek, gcc-patches [-- Attachment #1: Type: text/plain, Size: 4777 bytes --] Hi Richard, Catherine, Matthew On Thu, 2017-03-02 14:40:46 +0100, Richard Biener <rguenther@suse.de> wrote: [...] > On IRC we decided to wait&see for the TREE_NO_WARNING issue. So the > following is what I committed. > > Bootstrapped / tested on x86_64-unknown-linux-gnu. [...] > 2017-03-02 Richard Biener <rguenther@suse.de> > > PR tree-optimization/79345 > PR c++/42000 > * tree-ssa-alias.c (walk_aliased_vdefs_1): Take a limit > param and abort the walk, returning -1 if it is hit. > (walk_aliased_vdefs): Take a limit param and pass it on. > * tree-ssa-alias.h (walk_aliased_vdefs): Add a limit param, > defaulting to 0 and return a signed int. > * tree-ssa-uninit.c (struct check_defs_data): New struct. > (check_defs): New helper. > (warn_uninitialized_vars): Use walk_aliased_vdefs to warn > about uninitialized memory. > > * fixed-value.c (fixed_from_string): Use ulow/uhigh to avoid > bogus uninitialized warning. > (fixed_convert_from_real): Likewise. > > * g++.dg/warn/Wuninitialized-7.C: New testcase. > * c-c++-common/ubsan/bounds-2.c: Add -Wno-uninitialized. > * gcc.dg/uninit-pr19430-2.c: Add expected warning. When building with config-list.mk, it seems to break for all of the listed MIPS targets, but not on any other architecture: [...] g++ -fno-PIE -c -g -O2 -DIN_GCC -DCROSS_DIRECTORY_STRUCTURE -fno-exceptions -fno-rtti -fasynchronous-unwind-tables -W -Wall -Wno-narrowing -Wwrite-strings -Wcast-qual -Wmissing-format-attribute -Woverloaded-virtual -pedantic -Wno-long-long -Wno-variadic-macros -Wno-overlength-strings -Werror -fno-common -DHAVE_CONFIG_H -I. -I. -I/scratch/4/jbglaw/configlist/repos/gcc/gcc -I/scratch/4/jbglaw/configlist/repos/gcc/gcc/. -I/scratch/4/jbglaw/configlist/repos/gcc/gcc/../include -I/scratch/4/jbglaw/configlist/repos/gcc/gcc/../libcpp/include -I/opt/cfarm/mpc/include -I/scratch/4/jbglaw/configlist/repos/gcc/gcc/../libdecnumber -I/scratch/4/jbglaw/configlist/repos/gcc/gcc/../libdecnumber/dpd -I../libdecnumber -I/scratch/4/jbglaw/configlist/repos/gcc/gcc/../libbacktrace -o mips.o -MT mips.o -MMD -MP -MF ./.deps/mips.TPo /scratch/4/jbglaw/configlist/repos/gcc/gcc/config/mips/mips.c In file included from /scratch/4/jbglaw/configlist/repos/gcc/gcc/hash-table.h:236:0, from /scratch/4/jbglaw/configlist/repos/gcc/gcc/coretypes.h:369, from /scratch/4/jbglaw/configlist/repos/gcc/gcc/config/mips/mips.c:26: /scratch/4/jbglaw/configlist/repos/gcc/gcc/vec.h: In function ‘mips_multi_member* mips_multi_add()’: /scratch/4/jbglaw/configlist/repos/gcc/gcc/vec.h:865:3: error: ‘empty’ may be used uninitialized in this function [-Werror=maybe-uninitialized] *slot = obj; ^ /scratch/4/jbglaw/configlist/repos/gcc/gcc/vec.h: In function ‘void mips_process_sync_loop(rtx_insn*, rtx_def**)’: /scratch/4/jbglaw/configlist/repos/gcc/gcc/vec.h:865:3: error: ‘empty’ may be used uninitialized in this function [-Werror=maybe-uninitialized] *slot = obj; ^ /scratch/4/jbglaw/configlist/repos/gcc/gcc/vec.h:865:3: error: ‘empty’ may be used uninitialized in this function [-Werror=maybe-uninitialized] *slot = obj; ^ /scratch/4/jbglaw/configlist/repos/gcc/gcc/vec.h:865:3: error: ‘empty’ may be used uninitialized in this function [-Werror=maybe-uninitialized] *slot = obj; ^ /scratch/4/jbglaw/configlist/repos/gcc/gcc/config/mips/mips.c: In function ‘bool mips_expand_vec_perm_const(rtx_def**)’: /scratch/4/jbglaw/configlist/repos/gcc/gcc/config/mips/mips.c:21362:10: error: ‘orig_perm’ may be used uninitialized in this function [-Werror=maybe-uninitialized] memcpy (d.perm, orig_perm, MAX_VECT_LEN); ~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ cc1plus: all warnings being treated as errors make[2]: *** [mips.o] Error 1 See eg. all the most recent builds: mips64el-st-linux-gnu: http://toolchain.lug-owl.de/buildbot/show_build_details.php?id=699675 mips64orion-elf: http://toolchain.lug-owl.de/buildbot/show_build_details.php?id=699678 mipsisa32r2-linux-gnu: http://toolchain.lug-owl.de/buildbot/show_build_details.php?id=699702 mipsisa32-elfoabi: http://toolchain.lug-owl.de/buildbot/show_build_details.php?id=699701 mipsisa64sb1-elf: http://toolchain.lug-owl.de/buildbot/show_build_details.php?id=699724 mipsisa64sr71k-elf: http://toolchain.lug-owl.de/buildbot/show_build_details.php?id=699726 mips-netbsd: http://toolchain.lug-owl.de/buildbot/show_build_details.php?id=699750 MfG, JBG -- Jan-Benedict Glaw jbglaw@lug-owl.de +49-172-7608481 Signature of: God put me on earth to accomplish a certain number of the second : things. Right now I am so far behind I will never die. [-- Attachment #2: Digital signature --] [-- Type: application/pgp-signature, Size: 181 bytes --] ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [BUILDROBOT] Maybe uninitialized warnings in mips targets 2017-03-18 18:21 ` [BUILDROBOT] Maybe uninitialized warnings in mips targets (was: [PATCH] Fix PR79345, better uninit warnings for memory) Jan-Benedict Glaw @ 2017-03-19 16:06 ` Jeff Law 2017-04-04 15:31 ` Jeff Law 1 sibling, 0 replies; 10+ messages in thread From: Jeff Law @ 2017-03-19 16:06 UTC (permalink / raw) To: Jan-Benedict Glaw, Richard Biener, Matthew Fortune, Catherine Moore Cc: Jakub Jelinek, gcc-patches On 03/18/2017 12:20 PM, Jan-Benedict Glaw wrote: > Hi Richard, Catherine, Matthew > > On Thu, 2017-03-02 14:40:46 +0100, Richard Biener <rguenther@suse.de> wrote: > [...] >> On IRC we decided to wait&see for the TREE_NO_WARNING issue. So the >> following is what I committed. >> >> Bootstrapped / tested on x86_64-unknown-linux-gnu. > [...] >> 2017-03-02 Richard Biener <rguenther@suse.de> >> >> PR tree-optimization/79345 >> PR c++/42000 >> * tree-ssa-alias.c (walk_aliased_vdefs_1): Take a limit >> param and abort the walk, returning -1 if it is hit. >> (walk_aliased_vdefs): Take a limit param and pass it on. >> * tree-ssa-alias.h (walk_aliased_vdefs): Add a limit param, >> defaulting to 0 and return a signed int. >> * tree-ssa-uninit.c (struct check_defs_data): New struct. >> (check_defs): New helper. >> (warn_uninitialized_vars): Use walk_aliased_vdefs to warn >> about uninitialized memory. >> >> * fixed-value.c (fixed_from_string): Use ulow/uhigh to avoid >> bogus uninitialized warning. >> (fixed_convert_from_real): Likewise. >> >> * g++.dg/warn/Wuninitialized-7.C: New testcase. >> * c-c++-common/ubsan/bounds-2.c: Add -Wno-uninitialized. >> * gcc.dg/uninit-pr19430-2.c: Add expected warning. > > When building with config-list.mk, it seems to break for all of the > listed MIPS targets, but not on any other architecture: Yes. The improved uninitialized memory warnings are catching some things that were previously missed. I've got fixes for all the MIPS thingies in a local tree, but haven't submitted them (yet). jeff ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [BUILDROBOT] Maybe uninitialized warnings in mips targets 2017-03-18 18:21 ` [BUILDROBOT] Maybe uninitialized warnings in mips targets (was: [PATCH] Fix PR79345, better uninit warnings for memory) Jan-Benedict Glaw 2017-03-19 16:06 ` [BUILDROBOT] Maybe uninitialized warnings in mips targets Jeff Law @ 2017-04-04 15:31 ` Jeff Law 1 sibling, 0 replies; 10+ messages in thread From: Jeff Law @ 2017-04-04 15:31 UTC (permalink / raw) To: Jan-Benedict Glaw, Richard Biener, Matthew Fortune, Catherine Moore Cc: Jakub Jelinek, gcc-patches On 03/18/2017 12:20 PM, Jan-Benedict Glaw wrote: > Hi Richard, Catherine, Matthew > > On Thu, 2017-03-02 14:40:46 +0100, Richard Biener <rguenther@suse.de> wrote: > [...] >> On IRC we decided to wait&see for the TREE_NO_WARNING issue. So the >> following is what I committed. >> >> Bootstrapped / tested on x86_64-unknown-linux-gnu. > [...] >> 2017-03-02 Richard Biener <rguenther@suse.de> >> >> PR tree-optimization/79345 >> PR c++/42000 >> * tree-ssa-alias.c (walk_aliased_vdefs_1): Take a limit >> param and abort the walk, returning -1 if it is hit. >> (walk_aliased_vdefs): Take a limit param and pass it on. >> * tree-ssa-alias.h (walk_aliased_vdefs): Add a limit param, >> defaulting to 0 and return a signed int. >> * tree-ssa-uninit.c (struct check_defs_data): New struct. >> (check_defs): New helper. >> (warn_uninitialized_vars): Use walk_aliased_vdefs to warn >> about uninitialized memory. >> >> * fixed-value.c (fixed_from_string): Use ulow/uhigh to avoid >> bogus uninitialized warning. >> (fixed_convert_from_real): Likewise. >> >> * g++.dg/warn/Wuninitialized-7.C: New testcase. >> * c-c++-common/ubsan/bounds-2.c: Add -Wno-uninitialized. >> * gcc.dg/uninit-pr19430-2.c: Add expected warning. > > When building with config-list.mk, it seems to break for all of the > listed MIPS targets, but not on any other architecture: I finally got around to installing the fixes to the MIPS backend for this problem. Jeff ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] Fix PR79345, better uninit warnings for memory 2017-03-01 14:03 [PATCH] Fix PR79345, better uninit warnings for memory Richard Biener 2017-03-01 19:33 ` [PATCH] Avoid UB in insn-emit.c (PR tree-optimization/79345) Jakub Jelinek 2017-03-02 10:36 ` [PATCH] Fix PR79345, better uninit warnings for memory Jakub Jelinek @ 2017-03-07 18:12 ` Jeff Law 2 siblings, 0 replies; 10+ messages in thread From: Jeff Law @ 2017-03-07 18:12 UTC (permalink / raw) To: Richard Biener, gcc-patches On 03/01/2017 07:03 AM, Richard Biener wrote: > > The following addresses a regression in uninit warnings that happens > because clobber stmts preclude the very simple-minded support we have > for memory. The patch fixes this by instead implementing uninit > warnings for memory properly, using the alias oracle walk_aliased_vdefs > helper. > > The patch adds better limiting to that interface and fixes one > false positive in fixed-value.c. Two other false positives are > fixed by the wide-int.h patch posted a few hours ago and a patch > to genemit from Jakub. > > Bootstrap and regtest running on x86_64-unknown-linux-gnu with those > prerequesites included. > > One issue with the patch is duplicate warnings as TREE_NO_WARNING > doesn't work very well on tcc_reference trees which are not > shared. A followup could use some sort of hash table to mitigate > this a bit. OTOH for maybe-uninit uses multiple locations may > be in need to be fixed to silence the warning. Another thing is > that we walk the function in random (BB) order and thus the > alias oracle walk limiting may result in warnings popping up > and going away in less predictable order (and also be reported > in odd order, like not all must-uninits first). > > Comments? I realize this may introduce (a lot of?) false positives > quite late in the game, more aggressive limiting, like to 2 > disambiguations, could solve the testcase in the PR and give up > most of the times while preserving the non-walking case of the > old code. I'm all for it. Even if it triggers some false positives, improved detection of uninit memory at compile time is a big win in my mind. jeff ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2017-04-04 15:31 UTC | newest] Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-03-01 14:03 [PATCH] Fix PR79345, better uninit warnings for memory Richard Biener 2017-03-01 19:33 ` [PATCH] Avoid UB in insn-emit.c (PR tree-optimization/79345) Jakub Jelinek 2017-03-02 8:01 ` Richard Biener 2017-03-02 10:36 ` [PATCH] Fix PR79345, better uninit warnings for memory Jakub Jelinek 2017-03-02 11:14 ` Richard Biener 2017-03-02 13:40 ` Richard Biener 2017-03-18 18:21 ` [BUILDROBOT] Maybe uninitialized warnings in mips targets (was: [PATCH] Fix PR79345, better uninit warnings for memory) Jan-Benedict Glaw 2017-03-19 16:06 ` [BUILDROBOT] Maybe uninitialized warnings in mips targets Jeff Law 2017-04-04 15:31 ` Jeff Law 2017-03-07 18:12 ` [PATCH] Fix PR79345, better uninit warnings for memory 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).