| Message ID | ory5o1d04j.fsf@livre.localdomain |
|---|---|
| State | New |
| Headers | show |
On 2012-06-05 12:33, Alexandre Oliva wrote: > for gcc/ChangeLog > from Alexandre Oliva <aoliva@redhat.com> > > PR debug/49888 > * var-tracking.c: Include alias.h. > (overlapping_mems): New struct. > (drop_overlapping_mem_locs): New. > (clobber_overlapping_mems): New. > (var_mem_delete_and_set, var_mem_delete): Call it. > (val_bind): Likewise, but only if modified. > (compute_bb_dataflow, emit_notes_in_bb): Call it on MEMs. > * Makefile.in (var-tracking.o): Depend in $(ALIAS_H). > > for gcc/testsuite/ChangeLog > from Alexandre Oliva <aoliva@redhat.com> > > PR debug/49888 > * gcc.dg/guality/pr49888.c: New. Ok. r~
On Tue, Jun 12, 2012 at 1:42 PM, Richard Henderson <rth@redhat.com> wrote: > On 2012-06-05 12:33, Alexandre Oliva wrote: >> for gcc/ChangeLog >> from Alexandre Oliva <aoliva@redhat.com> >> >> PR debug/49888 >> * var-tracking.c: Include alias.h. >> (overlapping_mems): New struct. >> (drop_overlapping_mem_locs): New. >> (clobber_overlapping_mems): New. >> (var_mem_delete_and_set, var_mem_delete): Call it. >> (val_bind): Likewise, but only if modified. >> (compute_bb_dataflow, emit_notes_in_bb): Call it on MEMs. >> * Makefile.in (var-tracking.o): Depend in $(ALIAS_H). >> >> for gcc/testsuite/ChangeLog >> from Alexandre Oliva <aoliva@redhat.com> >> >> PR debug/49888 >> * gcc.dg/guality/pr49888.c: New. > > Ok. > > It caused: http://gcc.gnu.org/bugzilla/show_bug.cgi?id=53671
On Jun 14, 2012, "H.J. Lu" <hjl.tools@gmail.com> wrote: > On Tue, Jun 12, 2012 at 1:42 PM, Richard Henderson <rth@redhat.com> wrote: >> On 2012-06-05 12:33, Alexandre Oliva wrote: >>> for gcc/ChangeLog >>> from Alexandre Oliva <aoliva@redhat.com> >>> >>> PR debug/49888 >>> * var-tracking.c: Include alias.h. >>> (overlapping_mems): New struct. >>> (drop_overlapping_mem_locs): New. >>> (clobber_overlapping_mems): New. >>> (var_mem_delete_and_set, var_mem_delete): Call it. >>> (val_bind): Likewise, but only if modified. >>> (compute_bb_dataflow, emit_notes_in_bb): Call it on MEMs. >>> * Makefile.in (var-tracking.o): Depend in $(ALIAS_H). >>> >>> for gcc/testsuite/ChangeLog >>> from Alexandre Oliva <aoliva@redhat.com> >>> >>> PR debug/49888 >>> * gcc.dg/guality/pr49888.c: New. >> >> Ok. > It caused: > http://gcc.gnu.org/bugzilla/show_bug.cgi?id=53671 I see a few of these failures myself. They're in my test results. I guess I was so caught up in assessing the debug info quality changes with this patch that I completely failed to look at the test results, because they've been around for a while, and I have a few pristine-build baselines in between. Apologies for this mistake. Anyway... The problem is not too hard to understand, but it may be somewhat hard to fix. Basically, pushing registers to save them on the stack implies writes that are currently thought to conflict with the MEMs holding incoming arguments, and apparently there isn't enough information in the cselib static table for us to realize the write doesn't alias with any of the incoming arguments. Using the dynamic tables during alias testing is one possibility I'm looking into, but this won't be trivial and it could get expensive; another, that has just occurred to me while composing this message, is to use the cselib static table itself, for it *should* have enough info for us to realize that argp and sp offset are related and, given proper offsets, non-overlapping. Now, neither approach is going to be an immediate fix. Should I revert the patch, or can we live with some debug info completeness regressions for a bit? I wouldn't mind reverting it, but I won't unless the broken patch is actually causing trouble to any of us. Again, sorry about the breakage.
On Fri, Jun 15, 2012 at 3:21 PM, Alexandre Oliva <aoliva@redhat.com> wrote: > On Jun 14, 2012, "H.J. Lu" <hjl.tools@gmail.com> wrote: > >> On Tue, Jun 12, 2012 at 1:42 PM, Richard Henderson <rth@redhat.com> wrote: >>> On 2012-06-05 12:33, Alexandre Oliva wrote: >>>> for gcc/ChangeLog >>>> from Alexandre Oliva <aoliva@redhat.com> >>>> >>>> PR debug/49888 >>>> * var-tracking.c: Include alias.h. >>>> (overlapping_mems): New struct. >>>> (drop_overlapping_mem_locs): New. >>>> (clobber_overlapping_mems): New. >>>> (var_mem_delete_and_set, var_mem_delete): Call it. >>>> (val_bind): Likewise, but only if modified. >>>> (compute_bb_dataflow, emit_notes_in_bb): Call it on MEMs. >>>> * Makefile.in (var-tracking.o): Depend in $(ALIAS_H). >>>> >>>> for gcc/testsuite/ChangeLog >>>> from Alexandre Oliva <aoliva@redhat.com> >>>> >>>> PR debug/49888 >>>> * gcc.dg/guality/pr49888.c: New. >>> >>> Ok. > >> It caused: > >> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=53671 > > I see a few of these failures myself. They're in my test results. I > guess I was so caught up in assessing the debug info quality changes > with this patch that I completely failed to look at the test results, > because they've been around for a while, and I have a few pristine-build > baselines in between. Apologies for this mistake. > > Anyway... The problem is not too hard to understand, but it may be > somewhat hard to fix. Basically, pushing registers to save them on the > stack implies writes that are currently thought to conflict with the > MEMs holding incoming arguments, and apparently there isn't enough > information in the cselib static table for us to realize the write > doesn't alias with any of the incoming arguments. > > Using the dynamic tables during alias testing is one possibility I'm > looking into, but this won't be trivial and it could get expensive; > another, that has just occurred to me while composing this message, is > to use the cselib static table itself, for it *should* have enough info > for us to realize that argp and sp offset are related and, given proper > offsets, non-overlapping. > > Now, neither approach is going to be an immediate fix. Should I revert > the patch, or can we live with some debug info completeness regressions > for a bit? I wouldn't mind reverting it, but I won't unless the broken > patch is actually causing trouble to any of us. > If I understand it correctly, the new approach fails to handle push properly. If it is true, I think the patch should be reverted. Thanks.
for gcc/ChangeLog from Alexandre Oliva <aoliva@redhat.com> PR debug/49888 * var-tracking.c: Include alias.h. (overlapping_mems): New struct. (drop_overlapping_mem_locs): New. (clobber_overlapping_mems): New. (var_mem_delete_and_set, var_mem_delete): Call it. (val_bind): Likewise, but only if modified. (compute_bb_dataflow, emit_notes_in_bb): Call it on MEMs. * Makefile.in (var-tracking.o): Depend in $(ALIAS_H). for gcc/testsuite/ChangeLog from Alexandre Oliva <aoliva@redhat.com> PR debug/49888 * gcc.dg/guality/pr49888.c: New. Index: gcc/var-tracking.c =================================================================== --- gcc/var-tracking.c.orig 2012-06-05 03:03:47.154714459 -0300 +++ gcc/var-tracking.c 2012-06-05 12:50:57.724756717 -0300 @@ -115,6 +115,7 @@ #include "pointer-set.h" #include "recog.h" #include "tm_p.h" +#include "alias.h" /* var-tracking.c assumes that tree code with the same value as VALUE rtx code has no chance to appear in REG_EXPR/MEM_EXPRs and isn't a decl. @@ -1954,6 +1955,111 @@ var_regno_delete (dataflow_set *set, int *reg = NULL; } +/* Hold parameters for the hashtab traversal function + drop_overlapping_mem_locs, see below. */ + +struct overlapping_mems +{ + dataflow_set *set; + rtx loc, addr; +}; + +/* Remove all MEMs that overlap with COMS->LOC from the location list + of a hash table entry for a value. COMS->ADDR must be a + canonicalized form of COMS->LOC's address, and COMS->LOC must be + canonicalized itself. */ + +static int +drop_overlapping_mem_locs (void **slot, void *data) +{ + struct overlapping_mems *coms = (struct overlapping_mems *)data; + dataflow_set *set = coms->set; + rtx mloc = coms->loc, addr = coms->addr; + variable var = (variable) *slot; + + if (var->onepart == ONEPART_VALUE) + { + location_chain loc, *locp; + bool changed = false; + rtx cur_loc; + + gcc_assert (var->n_var_parts == 1); + + if (shared_var_p (var, set->vars)) + { + for (loc = var->var_part[0].loc_chain; loc; loc = loc->next) + if (GET_CODE (loc->loc) == MEM + && canon_true_dependence (mloc, GET_MODE (mloc), addr, + loc->loc, NULL)) + break; + + if (!loc) + return 1; + + slot = unshare_variable (set, slot, var, VAR_INIT_STATUS_UNKNOWN); + var = (variable)*slot; + gcc_assert (var->n_var_parts == 1); + } + + if (VAR_LOC_1PAUX (var)) + cur_loc = VAR_LOC_FROM (var); + else + cur_loc = var->var_part[0].cur_loc; + + for (locp = &var->var_part[0].loc_chain, loc = *locp; + loc; loc = *locp) + { + if (GET_CODE (loc->loc) != MEM + || !canon_true_dependence (mloc, GET_MODE (mloc), addr, + loc->loc, NULL)) + { + locp = &loc->next; + continue; + } + + *locp = loc->next; + /* If we have deleted the location which was last emitted + we have to emit new location so add the variable to set + of changed variables. */ + if (cur_loc == loc->loc) + { + changed = true; + var->var_part[0].cur_loc = NULL; + if (VAR_LOC_1PAUX (var)) + VAR_LOC_FROM (var) = NULL; + } + pool_free (loc_chain_pool, loc); + } + + if (!var->var_part[0].loc_chain) + { + var->n_var_parts--; + changed = true; + } + if (changed) + variable_was_changed (var, set); + } + + return 1; +} + +/* Remove from SET all VALUE bindings to MEMs that overlap with LOC. */ + +static void +clobber_overlapping_mems (dataflow_set *set, rtx loc) +{ + struct overlapping_mems coms; + + coms.set = set; + coms.loc = canon_rtx (loc); + coms.addr = canon_rtx (get_addr (XEXP (loc, 0))); + + set->traversed_vars = set->vars; + htab_traverse (shared_hash_htab (set->vars), + drop_overlapping_mem_locs, &coms); + set->traversed_vars = NULL; +} + /* Set the location of DV, OFFSET as the MEM LOC. */ static void @@ -1996,6 +2102,7 @@ var_mem_delete_and_set (dataflow_set *se tree decl = MEM_EXPR (loc); HOST_WIDE_INT offset = INT_MEM_OFFSET (loc); + clobber_overlapping_mems (set, loc); decl = var_debug_decl (decl); if (initialized == VAR_INIT_STATUS_UNKNOWN) @@ -2016,6 +2123,7 @@ var_mem_delete (dataflow_set *set, rtx l tree decl = MEM_EXPR (loc); HOST_WIDE_INT offset = INT_MEM_OFFSET (loc); + clobber_overlapping_mems (set, loc); decl = var_debug_decl (decl); if (clobber) clobber_variable_part (set, NULL, dv_from_decl (decl), offset, NULL); @@ -2059,6 +2167,9 @@ val_bind (dataflow_set *set, rtx val, rt { struct elt_loc_list *l = CSELIB_VAL_PTR (val)->locs; + if (modified) + clobber_overlapping_mems (set, loc); + if (l && GET_CODE (l->loc) == VALUE) l = canonical_cselib_val (CSELIB_VAL_PTR (l->loc))->locs; @@ -6371,6 +6482,8 @@ compute_bb_dataflow (basic_block bb) } else if (REG_P (uloc)) var_regno_delete (out, REGNO (uloc)); + else if (MEM_P (uloc)) + clobber_overlapping_mems (out, uloc); val_store (out, val, dstv, insn, true); } @@ -8870,6 +8983,8 @@ emit_notes_in_bb (basic_block bb, datafl } else if (REG_P (uloc)) var_regno_delete (set, REGNO (uloc)); + else if (MEM_P (uloc)) + clobber_overlapping_mems (set, uloc); val_store (set, val, dstv, insn, true); Index: gcc/Makefile.in =================================================================== --- gcc/Makefile.in.orig 2012-06-05 03:02:32.904609594 -0300 +++ gcc/Makefile.in 2012-06-05 12:54:21.000000000 -0300 @@ -3130,8 +3130,8 @@ var-tracking.o : var-tracking.c $(CONFIG $(RTL_H) $(TREE_H) hard-reg-set.h insn-config.h reload.h $(FLAGS_H) \ $(BASIC_BLOCK_H) bitmap.h alloc-pool.h $(FIBHEAP_H) $(HASHTAB_H) \ $(REGS_H) $(EXPR_H) $(TIMEVAR_H) $(TREE_PASS_H) $(TREE_FLOW_H) \ - cselib.h $(TARGET_H) $(DIAGNOSTIC_CORE_H) $(PARAMS_H) $(DIAGNOSTIC_H) pointer-set.h \ - $(RECOG_H) $(TM_P_H) $(TREE_PRETTY_PRINT_H) + cselib.h $(TARGET_H) $(DIAGNOSTIC_CORE_H) $(PARAMS_H) $(DIAGNOSTIC_H) \ + pointer-set.h $(RECOG_H) $(TM_P_H) $(TREE_PRETTY_PRINT_H) $(ALIAS_H) profile.o : profile.c $(CONFIG_H) $(SYSTEM_H) coretypes.h $(TM_H) $(RTL_H) \ $(TREE_H) $(FLAGS_H) $(REGS_H) $(EXPR_H) $(FUNCTION_H) $(BASIC_BLOCK_H) \ $(DIAGNOSTIC_CORE_H) $(COVERAGE_H) $(TREE_FLOW_H) value-prof.h cfghooks.h \ Index: gcc/testsuite/gcc.dg/guality/pr49888.c =================================================================== --- /dev/null 1970-01-01 00:00:00.000000000 +0000 +++ gcc/testsuite/gcc.dg/guality/pr49888.c 2012-06-05 12:50:57.878744571 -0300 @@ -0,0 +1,25 @@ +/* PR debug/49888 */ +/* { dg-do run } */ +/* { dg-options "-g" } */ + +static int v; + +static void __attribute__((noinline, noclone)) +f (int *p) +{ + int c = *p; + v = c; + *p = 1; /* { dg-final { gdb-test 12 "c" "0" } } */ + /* c may very well be optimized out at this point, so we test !c, + which will evaluate to the expected value. We just want to make + sure it doesn't remain bound to *p as it did before, in which + case !c would evaluate to 0. */ + v = 0; /* { dg-final { gdb-test 17 "!c" "1" } } */ +} +int +main () +{ + int a = 0; + f (&a); + return 0; +}