[Bug debug/54796] New: [4.8 Regression] Non-addressable stack parameter debug quality regression

[Bug debug/54796] New: [4.8 Regression] Non-addressable stack parameter debug quality regression

[jakub at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=54796

             Bug #: 54796
           Summary: [4.8 Regression] Non-addressable stack parameter debug
                    quality regression
    Classification: Unclassified
           Product: gcc
           Version: 4.8.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: debug
        AssignedTo: unassigned@gcc.gnu.org
        ReportedBy: jakub@gcc.gnu.org
                CC: aoliva@gcc.gnu.org
            Target: i686-linux


/* PR debug/NNNNN */
/* { dg-do run } */
/* { dg-options "-g" } */

__attribute__((noinline, noclone)) void
bar (char *a, int b)
{
  __asm volatile ("" : "+r" (a), "+r" (b) : : "memory");
}

__attribute__((noinline, noclone)) void
foo (int a, int b)
{
  int c = a;
  char d[b];/* { dg-final { gdb-test 17 "a" "5" } } */
  bar (d, 2);/* { dg-final { gdb-test 17 "b" "6" } } */
  bar (d, 4);/* { dg-final { gdb-test 17 "c" "5" } } */
}

int
main ()
{
  foo (5, 6);
  return 0;
}

(line 17 is the second call to bar) regressed for -m32 -Os -g on
{i686,x86_64}-linux supposedly with the
http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=188871
change.  I've mentioned this already in
http://gcc.gnu.org/ml/gcc-patches/2012-10/msg00262.html
but filing it now on a testcase that is not dependent on any GCC patches.

The problem is that vt_canon_true_dep/canon_true_dependence doesn't figure out
that the stack stores (after prologue where hard fp is initialized) can't alias
argp.  Without the VLA or other reasons forcing use of frame pointer
var-tracking replaces sp accesses with argp + offset accesses and
canon_true_dependence then disambiguates it using the offsets.

[Bug debug/54796] [4.8 Regression] Non-addressable stack parameter debug quality regression

[jakub at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=54796

Jakub Jelinek <jakub at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Target Milestone|---                         |4.8.0

--- Comment #1 from Jakub Jelinek <jakub at gcc dot gnu.org> 2012-10-03 16:40:04 UTC ---
I think the problem is that by the time these vt_canon_true_dep calls are made,
mloc's addr is some VALUE for which alias.c unfortunately no longer can figure
out that they are sp based, as sp REG has been removed from all the VALUE locs.
 If we could somehow somewhere preserve the information that some VALUEs are
sp based (i.e. find_base_term (val) ==
static_reg_base_value[STACK_POINTER_REGNUM]), then we could disambiguate at
least that the sp based stores can't clobber global vars or hard frame pointer
based MEMs.

[Bug debug/54796] [4.8 Regression] Non-addressable stack parameter debug quality regression

[jakub at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=54796

--- Comment #2 from Jakub Jelinek <jakub at gcc dot gnu.org> 2012-10-03 17:35:44 UTC ---
Perhaps it would suffice to reserve another rtx flag bit on VALUE for
SP_BASED_VALUE_P, and just from var-tracking if hard_frame_pointer_adjustment
is != -1 (i.e. frame_pointer_needed and hfp = assignment has been seen) mark
that way sp values that are created afterwards, and change find_base_term in
alias.c
so that it returns static_reg_base_value[STACK_POINTER_REGNUM] when seeing
VALUEs marked that way.

[Bug debug/54796] [4.8 Regression] Non-addressable stack parameter debug quality regression

[jakub at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=54796

Jakub Jelinek <jakub at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |ASSIGNED
   Last reconfirmed|                            |2012-10-04
         AssignedTo|unassigned at gcc dot       |jakub at gcc dot gnu.org
                   |gnu.org                     |
     Ever Confirmed|0                           |1

--- Comment #3 from Jakub Jelinek <jakub at gcc dot gnu.org> 2012-10-04 14:08:49 UTC ---
Created attachment 28357
  --> http://gcc.gnu.org/bugzilla/attachment.cgi?id=28357
gcc48-pr54796.patch

Untested patch that looks promissing to me.
For -m32 guality.exp results improve:
+XPASS: gcc.dg/guality/example.c  -Os  execution test
-FAIL: gcc.dg/guality/pr43051-1.c  -Os  line 39 c == &a[0]
-FAIL: gcc.dg/guality/pr54519-3.c  -Os  line 23 y == 117
-FAIL: gcc.dg/guality/pr54519-3.c  -Os  line 23 z == 8  
(plus all gcc.dg/guality/pr54796.c -O1 and above results which fail without the
non-testsuite part of the patch).  For -m64 no change (expectedly, one would
need to have more than 6 arguments in teststo trigger stack slot passing).

[Bug debug/54796] [4.8 Regression] Non-addressable stack parameter debug quality regression

[aoliva at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=54796

--- Comment #4 from Alexandre Oliva <aoliva at gcc dot gnu.org> 2012-10-08 14:37:53 UTC ---
I'm a bit uncomfortable with this approach.  On the one hand, it's quite
simple, which is nice, but if all we get from it is the base term, we'll still
have trouble given multiple sp-based non-overlapping memory regions, won't we? 
The patch in revision 188871 and others in that batch were meant to let alias
analysis within vta figure out the relationship between argp, fp and sp and
disambiguate accesses.  I wonder why it didn't work in this case, with or
without the still-pending patch to canonicalize stack pointer tracking.  I'd
rather go with the latter (even if with additional changes) because it would
enable alias analysis, through more precise sp+offset tracking, to distinguish
not only between argp and sp-based variables, but also between automatic
varying-sized objects.  Assuming my understanding of your patch is correct in
that it adds information on whether some object is at an sp-relative offset but
without making the offset available to alias analysis.  Is that so?

[Bug debug/54796] [4.8 Regression] Non-addressable stack parameter debug quality regression

[jakub at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=54796

--- Comment #5 from Jakub Jelinek <jakub at gcc dot gnu.org> 2012-10-16 11:21:28 UTC ---
Author: jakub
Date: Tue Oct 16 11:21:20 2012
New Revision: 192494

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=192494
Log:
    PR debug/54796
    * rtl.h: Document jump flag on VALUE.
    * cselib.h (cselib_set_value_sp_based,
    cselib_sp_based_value_p): New prototypes.
    * alias.c (find_base_term): For cselib_sp_based_value_p
    return static_reg_base_value[STACK_POINTER_REGNUM].
    * cselib.c (SP_BASED_VALUE_P): Define.
    (cselib_set_value_sp_based, cselib_sp_based_value_p): New functions.
    * var-tracking.c (add_stores): Call cselib_set_value_sp_based
    for not yet preserved VALUEs of sp on sp assignments if
    hard_frame_pointer_adjustment != -1.
    (vt_initialize): When setting hard_frame_pointer_adjustment,
    disassociate sp from its previous value and call
    cselib_set_value_sp_based on a new VALUE created for sp.

    * gcc.dg/guality/pr54796.c: New test.

Added:
    trunk/gcc/testsuite/gcc.dg/guality/pr54796.c
Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/alias.c
    trunk/gcc/cselib.c
    trunk/gcc/cselib.h
    trunk/gcc/rtl.h
    trunk/gcc/testsuite/ChangeLog
    trunk/gcc/var-tracking.c

[Bug debug/54796] [4.8 Regression] Non-addressable stack parameter debug quality regression

[jakub at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=54796

Jakub Jelinek <jakub at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|ASSIGNED                    |RESOLVED
         Resolution|                            |FIXED

--- Comment #6 from Jakub Jelinek <jakub at gcc dot gnu.org> 2012-10-23 15:14:04 UTC ---
Fixed.