public inbox for gcc-bugs@sourceware.org
help / color / mirror / Atom feed
* [Bug debug/102441] New: Incorrect location list in debug info
@ 2021-09-22  6:36 liyd2021 at gmail dot com
  2021-09-22  7:28 ` [Bug rtl-optimization/102441] [10/11/12 Regression] " rguenth at gcc dot gnu.org
                   ` (7 more replies)
  0 siblings, 8 replies; 9+ messages in thread
From: liyd2021 at gmail dot com @ 2021-09-22  6:36 UTC (permalink / raw)
  To: gcc-bugs

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=102441

            Bug ID: 102441
           Summary: Incorrect location list in debug info
           Product: gcc
           Version: 11.1.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: debug
          Assignee: unassigned at gcc dot gnu.org
          Reporter: liyd2021 at gmail dot com
  Target Milestone: ---

Affected versions: gcc 11.1.0 with gdb (Ubuntu 20.04.2)

(terminal) $ cat simple.c && gcc -g -O2 simple.c
void __attribute__((noipa)) not_opt_this(int *r) {}

int __attribute__((noinline)) foo(int x) {
    int r = 0;
    not_opt_this(&r);
    return r;
}

int main (void) {
    return foo (10);
}
--------------------

(terminal) $ cat run.gdb
b foo
b 5
r
c

(terminal) $ gdb -x run.gdb a.out
Breakpoint 1 at 0x401190: file simple.c, line 4.
Breakpoint 2 at 0x4011a1: file simple.c, line 5.

Breakpoint 1, foo (x=10) at simple.c:4
4           int r = 0;

Breakpoint 2, foo (x=-9492) at simple.c:5 <-- **BUG**: x should be 10
5           not_opt_this(&r);

--------------------
debug info and assembly:

<2><88>: Abbrev Number: 7 (DW_TAG_formal_parameter)
    <89>   DW_AT_name        : x
    <8b>   DW_AT_decl_file   : 1
    <8c>   DW_AT_decl_line   : 3
    <8d>   DW_AT_decl_column : 39
    <8e>   DW_AT_type        : <0x5f>
    <92>   DW_AT_location    : 0x4 (location list)
    <96>   DW_AT_GNU_locviews: 0x0

Contents of the .debug_loc section:

    Offset   Begin            End              Expression

    00000000 v000000000000000 v000000000000000 location view pair
    00000002 v000000000000000 v000000000000000 location view pair

    00000004 v000000000000000 v000000000000000 views at 00000000 for:
             0000000000401190 00000000004011a5 (DW_OP_reg5 (rdi))
    00000017 v000000000000000 v000000000000000 views at 00000002 for:
             00000000004011a5 00000000004011af (DW_OP_GNU_entry_value:
(DW_OP_reg5 (rdi)); DW_OP_stack_value)
    0000002d <End of list>

0x401190 <foo>                  sub    $0x18,%rsp
0x401194 <foo+4>                lea    0xc(%rsp),%rdi
0x401199 <foo+9>                movl   $0x0,0xc(%rsp)
0x4011a1 <foo+17>               callq  0x401180 <not_opt_this>
0x4011a6 <foo+22>               mov    0xc(%rsp),%eax
0x4011aa <foo+26>               add    $0x18,%rsp
0x4011ae <foo+30>               retq

Debug info shows x in register %rdi, but %rdi was clobbered after 0x401194.

^ permalink raw reply	[flat|nested] 9+ messages in thread

* [Bug rtl-optimization/102441] [10/11/12 Regression] Incorrect location list in debug info
  2021-09-22  6:36 [Bug debug/102441] New: Incorrect location list in debug info liyd2021 at gmail dot com
@ 2021-09-22  7:28 ` rguenth at gcc dot gnu.org
  2021-09-24  9:57 ` jakub at gcc dot gnu.org
                   ` (6 subsequent siblings)
  7 siblings, 0 replies; 9+ messages in thread
From: rguenth at gcc dot gnu.org @ 2021-09-22  7:28 UTC (permalink / raw)
  To: gcc-bugs

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=102441

Richard Biener <rguenth at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
      Known to fail|                            |10.3.1, 11.2.1
             Target|                            |x86_64-*-*
             Status|UNCONFIRMED                 |NEW
      Known to work|                            |7.5.0
            Summary|Incorrect location list in  |[10/11/12 Regression]
                   |debug info                  |Incorrect location list in
                   |                            |debug info
          Component|debug                       |rtl-optimization
   Target Milestone|---                         |10.4
   Last reconfirmed|                            |2021-09-22
           Keywords|                            |wrong-debug
           Priority|P3                          |P2
     Ever confirmed|0                           |1

--- Comment #1 from Richard Biener <rguenth at gcc dot gnu.org> ---
probably a var-tracking issue.  Works with GCC 7 and (noipa doesn't exist):

void __attribute__((noinline,noclone)) not_opt_this(int *r)
{ __asm__ volatile ("" : : :"memory"); }

int __attribute__((noinline)) foo(int x) {
    int r = 0;
    not_opt_this(&r);
    return r;
}

int main (void) {
    return foo (10);
}


GCC 7 prints the correct value, GCC 9 prints <optimized out>

^ permalink raw reply	[flat|nested] 9+ messages in thread

* [Bug rtl-optimization/102441] [10/11/12 Regression] Incorrect location list in debug info
  2021-09-22  6:36 [Bug debug/102441] New: Incorrect location list in debug info liyd2021 at gmail dot com
  2021-09-22  7:28 ` [Bug rtl-optimization/102441] [10/11/12 Regression] " rguenth at gcc dot gnu.org
@ 2021-09-24  9:57 ` jakub at gcc dot gnu.org
  2021-09-24 17:26 ` jakub at gcc dot gnu.org
                   ` (5 subsequent siblings)
  7 siblings, 0 replies; 9+ messages in thread
From: jakub at gcc dot gnu.org @ 2021-09-24  9:57 UTC (permalink / raw)
  To: gcc-bugs

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=102441

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |jakub at gcc dot gnu.org

--- Comment #2 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
So, I see 2 changes since 7-ish.  The first one is in
r8-5241-g8697bf9f46f36168ddba5752db582e673e3cbe8c
which added statement frontiers and therefore moved where the breakpoint is
actually placed.  Before/after that commit, the location info for x seems
correct, first %rdi and at the insn setting %rdi changing to entry_value(%rdi).
But entry_value(%rdi) doesn't seem to be usable in this case, because main tail
calls foo and so main is out of the backtrace, and the outer frame is
__libc_start_main which is in assembly, so the debugger can't try to
reconstruct the tail call path.
So, before r8-5241 the debugger prints for x at line 6 10 (#c1), at or after it
it prints <optimized out>.
The next change is r10-7665-g33c45e51b4914008064d9b77f2c1fc0eea1ad060 and that
causes wrong-debug for this case.  It isn't immediately clear to me why,
because %rdi certainly isn't sp based, but I'll need to debug.

^ permalink raw reply	[flat|nested] 9+ messages in thread

* [Bug rtl-optimization/102441] [10/11/12 Regression] Incorrect location list in debug info
  2021-09-22  6:36 [Bug debug/102441] New: Incorrect location list in debug info liyd2021 at gmail dot com
  2021-09-22  7:28 ` [Bug rtl-optimization/102441] [10/11/12 Regression] " rguenth at gcc dot gnu.org
  2021-09-24  9:57 ` jakub at gcc dot gnu.org
@ 2021-09-24 17:26 ` jakub at gcc dot gnu.org
  2021-10-10 10:17 ` [Bug debug/102441] " cvs-commit at gcc dot gnu.org
                   ` (4 subsequent siblings)
  7 siblings, 0 replies; 9+ messages in thread
From: jakub at gcc dot gnu.org @ 2021-09-24 17:26 UTC (permalink / raw)
  To: gcc-bugs

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=102441

--- Comment #3 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
Perhaps we need something like:
--- gcc/var-tracking.c.jj       2021-05-04 21:02:24.196799586 +0200
+++ gcc/var-tracking.c  2021-09-24 19:23:16.420154828 +0200
@@ -6133,7 +6133,9 @@ add_stores (rtx loc, const_rtx expr, voi
     {
       if (preserve)
        preserve_value (v);
-      return;
+      mo.type = MO_CLOBBER;
+      mo.u.loc = loc;
+      goto log_and_return;
     }

   nloc = replace_expr_with_values (oloc);
but am not 100% sure if that is the right thing to do in all cases.

^ permalink raw reply	[flat|nested] 9+ messages in thread

* [Bug debug/102441] [10/11/12 Regression] Incorrect location list in debug info
  2021-09-22  6:36 [Bug debug/102441] New: Incorrect location list in debug info liyd2021 at gmail dot com
                   ` (2 preceding siblings ...)
  2021-09-24 17:26 ` jakub at gcc dot gnu.org
@ 2021-10-10 10:17 ` cvs-commit at gcc dot gnu.org
  2021-10-10 10:18 ` cvs-commit at gcc dot gnu.org
                   ` (3 subsequent siblings)
  7 siblings, 0 replies; 9+ messages in thread
From: cvs-commit at gcc dot gnu.org @ 2021-10-10 10:17 UTC (permalink / raw)
  To: gcc-bugs

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=102441

--- Comment #4 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The master branch has been updated by Jakub Jelinek <jakub@gcc.gnu.org>:

https://gcc.gnu.org/g:9583b26f3701ea0456405d84f9a898451a2f7452

commit r12-4277-g9583b26f3701ea0456405d84f9a898451a2f7452
Author: Jakub Jelinek <jakub@redhat.com>
Date:   Sun Oct 10 12:13:22 2021 +0200

    var-tracking: Fix a wrong-debug issue caused by my r10-7665 var-tracking
change [PR102441]

    Since my r10-7665-g33c45e51b4914008064d9b77f2c1fc0eea1ad060 change, we get
    wrong-debug on e.g. the following testcase at -O2 -g on x86_64-linux for
the
    x parameter:
    void bar (int *r);
    int
    foo (int x)
    {
      int r = 0;
      bar (&r);
      return r;
    }
    At the start of function, we have
            subq    $24, %rsp
            leaq    12(%rsp), %rdi
    instructions.  The x parameter is passed in %rdi, but isn't used in the
    function and so the leaq instruction overwrites %rdi without remembering
    %rdi anywhere.  Before the r10-7665 change (which was trying to fix a large
    (3% for 32-bit, 1% for 64-bit x86-64) debug info/loc growth introduced with
    r10-7515), the leaq insn above resulted in a MO_VAL_SET micro-operation
that
    said that the value of sp + 12, a cselib_sp_derived_value_p, is stored into
    the %rdi register.  The r10-7665 change added a change to add_stores that
    added no micro-operation for the leaq store, with the rationale that the sp
    based values can be and will be always computable some other more compact
    and primarily more stable way (cfa based expression like DW_OP_fbreg, that
    is the same in the whole function).  That is true.  But by throwing the
    micro-operation on the floor, we miss another important part of the
    MO_VAL_SET, in particular that the destination of the store, %rdi in this
    case, now has a different value from what it had before, so the vt_*
    dataflow code thinks that even after the leaq instruction %rdi still holds
    the x argument value (and changes it to DW_OP_entry_value (%rdi) only in
the
    middle of the call to bar).  Previously and with the patches below,
    the location for x changes already at the end of leaq instruction to
    DW_OP_entry_value (%rdi).

    My first attempt to fix this was instead of dropping the MO_VAL_SET add
    a MO_CLOBBER operation:
    --- gcc/var-tracking.c.jj       2021-05-04 21:02:24.196799586 +0200
    +++ gcc/var-tracking.c  2021-09-24 19:23:16.420154828 +0200
    @@ -6133,7 +6133,9 @@ add_stores (rtx loc, const_rtx expr, voi
         {
           if (preserve)
            preserve_value (v);
    -      return;
    +      mo.type = MO_CLOBBER;
    +      mo.u.loc = loc;
    +      goto log_and_return;
         }

       nloc = replace_expr_with_values (oloc);
    so don't track that the value lives in the loc destination, but track
    that the previous value doesn't live there anymore.  That failed bootstrap
    miserably, the vt_* code isn't prepared to see MO_CLOBBER of a MEM that
    isn't tracked (e.g. has MEM_EXPR on it that the var-tracking code wants
    to track, i.e. track_p in add_stores).  On the other side, thinking about
    it more, in the most common case where a cselib_sp_derived_value_p value
    is stored into the sp register (and which is the reason why PR94495
    testcase got larger), dropping the micro-operation on the floor is the
    right thing, because we have that cselib_sp_derived_value_p tracking, any
    reads from the sp hard register will be treated as
    cselib_sp_derived_value_p.
    Then I've tried 3 different patches described below and in the end
    what is committed is patch2.
    Additionally, I've gathered statistics from cc1plus by always reverting the
    var-tracking.c change after finished bootstrap/regtest and rebuilding the
    stage3 var-tracking.o and cc1plus, such that it would be comparable.
    dwlocstat and .debug_{info,loclists} section sizes detailed below.
    patch3 uses MO_VAL_SET (i.e. essentially reversion of the r10-7665
    change) when destination is not a REG_P and !track_p, otherwise if
    destination is sp drops the micro-operation on the floor (i.e. no change),
    otherwise adds a MO_CLOBBER.
    patch1 is similar, except it checks for destination not equal to sp and
    !track_p, i.e. for !track_p REG_P destinations other than sp it will use
    MO_VAL_SET rather than MO_CLOBBER.
    Finally, patch2, the shortest patch, uses MO_VAL_SET whenever destination
    is not sp and otherwise drops the micro-operation on the floor.
    All the 3 patches don't affect the PR94495 testcase, all the changes
    there were caused by stores of sp based values into %rsp.

    While the patch2 (and patch1 which results in exactly the same sizes)
    causes the largest debug loclists/info growth from the 3, it is still quite
    minor (0.651% on 64-bit and 0.114% on 32-bit) compared
    to the 1% and 3% PR94495 was trying to solve, and I actually think it is
the
    best thing to do.  Because, if we have say
      int q[10];
      int *p = &q[0];
    or similar and we load the &q[0] sp based value into some hard register,
    by noting in the debug info that p lives in some hard reg for some part
    of the function and a user is trying to change the p var in the debugger,
    if we say it lives in some register or memory, there is some chance that
    the changing of the value could work successfully (of course, nothing
    is guaranteed, we don't have tracking of where each var lives at which
    moment for changing purposes (i.e. what register, memory or else you need
    to change in order to change behavior of the code)), while if we just say
    that p's location is DW_OP_fbreg 16 DW_OP_stack_value, that is a read-only
    value one can just print but not change.  Now, for stores of variable
    values into the sp register, I don't think we have such an issue, you don't
    want debugger to change your stack pointer when user asks to change value
    of some variable whose value lives in the stack pointer, that would pretty
    much always result in misbehavior of the program.
    So, my preference from these 3 is patch2 and that is being committed.

    64-bit cc1plus
    ==============
    vanilla
    cov%    samples cumul
    0..10   1064665/37%     1064665/37%
    11..20  35972/1%        1100637/38%
    21..30  47969/1%        1148606/40%
    31..40  45787/1%        1194393/42%
    41..50  57529/2%        1251922/44%
    51..60  53974/1%        1305896/46%
    61..70  112055/3%       1417951/50%
    71..80  79420/2%        1497371/52%
    81..90  126225/4%       1623596/57%
    91..100 1206682/42%     2830278/100%
      [34] .debug_info       PROGBITS        0000000000000000 2f1c74c a44949f
00      0   0  1
      [38] .debug_loclists   PROGBITS        0000000000000000 ff5d046 506e947
00      0   0  1
    patch1 (same as patch2)
    cov%    samples cumul
    0..10   1064685/37%     1064685/37%
    11..20  36011/1%        1100696/38%
    21..30  47975/1%        1148671/40%
    31..40  45799/1%        1194470/42%
    41..50  57566/2%        1252036/44%
    51..60  54011/1%        1306047/46%
    61..70  112068/3%       1418115/50%
    71..80  79421/2%        1497536/52%
    81..90  126171/4%       1623707/57%
    91..100 1206571/42%     2830278/100%
      [34] .debug_info       PROGBITS        0000000000000000 2f1c74c a448f27
00      0   0  1
      [38] .debug_loclists   PROGBITS        0000000000000000 ff608bc 52070dd
00      0   0  1
    patch3
    cov%    samples cumul
    0..10   1064698/37%     1064698/37%
    11..20  36018/1%        1100716/38%
    21..30  47977/1%        1148693/40%
    31..40  45804/1%        1194497/42%
    41..50  57562/2%        1252059/44%
    51..60  54018/1%        1306077/46%
    61..70  112071/3%       1418148/50%
    71..80  79424/2%        1497572/52%
    81..90  126172/4%       1623744/57%
    91..100 1206534/42%     2830278/100%
      [34] .debug_info       PROGBITS        0000000000000000 2f1c74c a449548
00      0   0  1
      [38] .debug_loclists   PROGBITS        0000000000000000 ff5df39 507acd8
00      0   0  1
    So, size of .debug_info+.debug_loclists grows for vanilla -> patch1 (or
patch2) by
    0.651% and for vanilla -> patch3 by 0.020%.

    32-bit cc1plus
    ==============
    vanilla
    cov%    samples cumul
    0..10   1061892/37%     1061892/37%
    11..20  34002/1%        1095894/39%
    21..30  43513/1%        1139407/40%
    31..40  41667/1%        1181074/42%
    41..50  59144/2%        1240218/44%
    51..60  47009/1%        1287227/45%
    61..70  105069/3%       1392296/49%
    71..80  72990/2%        1465286/52%
    81..90  125988/4%       1591274/56%
    91..100 1208726/43%     2800000/100%
      [33] .debug_info       PROGBITS        00000000 351ab10 8b1c83d 00      0
  0  1
      [37] .debug_loclists   PROGBITS        00000000 ebc816e 3fe44fd 00      0
  0  1
    patch1 (same as patch2)
    cov%    samples cumul
    0..10   1061999/37%     1061999/37%
    11..20  34065/1%        1096064/39%
    21..30  43557/1%        1139621/40%
    31..40  41690/1%        1181311/42%
    41..50  59191/2%        1240502/44%
    51..60  47143/1%        1287645/45%
    61..70  105045/3%       1392690/49%
    71..80  73021/2%        1465711/52%
    81..90  125885/4%       1591596/56%
    91..100 1208404/43%     2800000/100%
      [33] .debug_info       PROGBITS        00000000 351ab10 8b1c597 00      0
  0  1
      [37] .debug_loclists   PROGBITS        00000000 ebca915 401ffad 00      0
  0  1
    patch3
    cov%    samples cumul
    0..10   1062006/37%     1062006/37%
    11..20  34073/1%        1096079/39%
    21..30  43559/1%        1139638/40%
    31..40  41693/1%        1181331/42%
    41..50  59189/2%        1240520/44%
    51..60  47142/1%        1287662/45%
    61..70  105054/3%       1392716/49%
    71..80  73027/2%        1465743/52%
    81..90  125874/4%       1591617/56%
    91..100 1208383/43%     2800000/100%
      [33] .debug_info       PROGBITS        00000000 351ab10 8b1c690 00      0
  0  1
      [37] .debug_loclists   PROGBITS        00000000 ebca40a 4020a6e 00      0
  0  1
    So, size of .debug_info+.debug_loclists grows for vanilla -> patch1 (or
patch2) by
    0.114% and for vanilla -> patch3 by 0.116%.

    2021-10-10  Jakub Jelinek  <jakub@redhat.com>

            PR debug/102441
            * var-tracking.c (add_stores): For cselib_sp_derived_value_p values
            use MO_VAL_SET if loc is not sp.

^ permalink raw reply	[flat|nested] 9+ messages in thread

* [Bug debug/102441] [10/11/12 Regression] Incorrect location list in debug info
  2021-09-22  6:36 [Bug debug/102441] New: Incorrect location list in debug info liyd2021 at gmail dot com
                   ` (3 preceding siblings ...)
  2021-10-10 10:17 ` [Bug debug/102441] " cvs-commit at gcc dot gnu.org
@ 2021-10-10 10:18 ` cvs-commit at gcc dot gnu.org
  2021-10-10 11:29 ` [Bug debug/102441] [10 " jakub at gcc dot gnu.org
                   ` (2 subsequent siblings)
  7 siblings, 0 replies; 9+ messages in thread
From: cvs-commit at gcc dot gnu.org @ 2021-10-10 10:18 UTC (permalink / raw)
  To: gcc-bugs

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=102441

--- Comment #5 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The releases/gcc-11 branch has been updated by Jakub Jelinek
<jakub@gcc.gnu.org>:

https://gcc.gnu.org/g:309827c85f14011166178c6efcb721d87a4577bb

commit r11-9093-g309827c85f14011166178c6efcb721d87a4577bb
Author: Jakub Jelinek <jakub@redhat.com>
Date:   Sun Oct 10 12:13:22 2021 +0200

    var-tracking: Fix a wrong-debug issue caused by my r10-7665 var-tracking
change [PR102441]

    Since my r10-7665-g33c45e51b4914008064d9b77f2c1fc0eea1ad060 change, we get
    wrong-debug on e.g. the following testcase at -O2 -g on x86_64-linux for
the
    x parameter:
    void bar (int *r);
    int
    foo (int x)
    {
      int r = 0;
      bar (&r);
      return r;
    }
    At the start of function, we have
            subq    $24, %rsp
            leaq    12(%rsp), %rdi
    instructions.  The x parameter is passed in %rdi, but isn't used in the
    function and so the leaq instruction overwrites %rdi without remembering
    %rdi anywhere.  Before the r10-7665 change (which was trying to fix a large
    (3% for 32-bit, 1% for 64-bit x86-64) debug info/loc growth introduced with
    r10-7515), the leaq insn above resulted in a MO_VAL_SET micro-operation
that
    said that the value of sp + 12, a cselib_sp_derived_value_p, is stored into
    the %rdi register.  The r10-7665 change added a change to add_stores that
    added no micro-operation for the leaq store, with the rationale that the sp
    based values can be and will be always computable some other more compact
    and primarily more stable way (cfa based expression like DW_OP_fbreg, that
    is the same in the whole function).  That is true.  But by throwing the
    micro-operation on the floor, we miss another important part of the
    MO_VAL_SET, in particular that the destination of the store, %rdi in this
    case, now has a different value from what it had before, so the vt_*
    dataflow code thinks that even after the leaq instruction %rdi still holds
    the x argument value (and changes it to DW_OP_entry_value (%rdi) only in
the
    middle of the call to bar).  Previously and with the patches below,
    the location for x changes already at the end of leaq instruction to
    DW_OP_entry_value (%rdi).

    My first attempt to fix this was instead of dropping the MO_VAL_SET add
    a MO_CLOBBER operation:
    --- gcc/var-tracking.c.jj       2021-05-04 21:02:24.196799586 +0200
    +++ gcc/var-tracking.c  2021-09-24 19:23:16.420154828 +0200
    @@ -6133,7 +6133,9 @@ add_stores (rtx loc, const_rtx expr, voi
         {
           if (preserve)
            preserve_value (v);
    -      return;
    +      mo.type = MO_CLOBBER;
    +      mo.u.loc = loc;
    +      goto log_and_return;
         }

       nloc = replace_expr_with_values (oloc);
    so don't track that the value lives in the loc destination, but track
    that the previous value doesn't live there anymore.  That failed bootstrap
    miserably, the vt_* code isn't prepared to see MO_CLOBBER of a MEM that
    isn't tracked (e.g. has MEM_EXPR on it that the var-tracking code wants
    to track, i.e. track_p in add_stores).  On the other side, thinking about
    it more, in the most common case where a cselib_sp_derived_value_p value
    is stored into the sp register (and which is the reason why PR94495
    testcase got larger), dropping the micro-operation on the floor is the
    right thing, because we have that cselib_sp_derived_value_p tracking, any
    reads from the sp hard register will be treated as
    cselib_sp_derived_value_p.
    Then I've tried 3 different patches described below and in the end
    what is committed is patch2.
    Additionally, I've gathered statistics from cc1plus by always reverting the
    var-tracking.c change after finished bootstrap/regtest and rebuilding the
    stage3 var-tracking.o and cc1plus, such that it would be comparable.
    dwlocstat and .debug_{info,loclists} section sizes detailed below.
    patch3 uses MO_VAL_SET (i.e. essentially reversion of the r10-7665
    change) when destination is not a REG_P and !track_p, otherwise if
    destination is sp drops the micro-operation on the floor (i.e. no change),
    otherwise adds a MO_CLOBBER.
    patch1 is similar, except it checks for destination not equal to sp and
    !track_p, i.e. for !track_p REG_P destinations other than sp it will use
    MO_VAL_SET rather than MO_CLOBBER.
    Finally, patch2, the shortest patch, uses MO_VAL_SET whenever destination
    is not sp and otherwise drops the micro-operation on the floor.
    All the 3 patches don't affect the PR94495 testcase, all the changes
    there were caused by stores of sp based values into %rsp.

    While the patch2 (and patch1 which results in exactly the same sizes)
    causes the largest debug loclists/info growth from the 3, it is still quite
    minor (0.651% on 64-bit and 0.114% on 32-bit) compared
    to the 1% and 3% PR94495 was trying to solve, and I actually think it is
the
    best thing to do.  Because, if we have say
      int q[10];
      int *p = &q[0];
    or similar and we load the &q[0] sp based value into some hard register,
    by noting in the debug info that p lives in some hard reg for some part
    of the function and a user is trying to change the p var in the debugger,
    if we say it lives in some register or memory, there is some chance that
    the changing of the value could work successfully (of course, nothing
    is guaranteed, we don't have tracking of where each var lives at which
    moment for changing purposes (i.e. what register, memory or else you need
    to change in order to change behavior of the code)), while if we just say
    that p's location is DW_OP_fbreg 16 DW_OP_stack_value, that is a read-only
    value one can just print but not change.  Now, for stores of variable
    values into the sp register, I don't think we have such an issue, you don't
    want debugger to change your stack pointer when user asks to change value
    of some variable whose value lives in the stack pointer, that would pretty
    much always result in misbehavior of the program.
    So, my preference from these 3 is patch2 and that is being committed.

    64-bit cc1plus
    ==============
    vanilla
    cov%    samples cumul
    0..10   1064665/37%     1064665/37%
    11..20  35972/1%        1100637/38%
    21..30  47969/1%        1148606/40%
    31..40  45787/1%        1194393/42%
    41..50  57529/2%        1251922/44%
    51..60  53974/1%        1305896/46%
    61..70  112055/3%       1417951/50%
    71..80  79420/2%        1497371/52%
    81..90  126225/4%       1623596/57%
    91..100 1206682/42%     2830278/100%
      [34] .debug_info       PROGBITS        0000000000000000 2f1c74c a44949f
00      0   0  1
      [38] .debug_loclists   PROGBITS        0000000000000000 ff5d046 506e947
00      0   0  1
    patch1 (same as patch2)
    cov%    samples cumul
    0..10   1064685/37%     1064685/37%
    11..20  36011/1%        1100696/38%
    21..30  47975/1%        1148671/40%
    31..40  45799/1%        1194470/42%
    41..50  57566/2%        1252036/44%
    51..60  54011/1%        1306047/46%
    61..70  112068/3%       1418115/50%
    71..80  79421/2%        1497536/52%
    81..90  126171/4%       1623707/57%
    91..100 1206571/42%     2830278/100%
      [34] .debug_info       PROGBITS        0000000000000000 2f1c74c a448f27
00      0   0  1
      [38] .debug_loclists   PROGBITS        0000000000000000 ff608bc 52070dd
00      0   0  1
    patch3
    cov%    samples cumul
    0..10   1064698/37%     1064698/37%
    11..20  36018/1%        1100716/38%
    21..30  47977/1%        1148693/40%
    31..40  45804/1%        1194497/42%
    41..50  57562/2%        1252059/44%
    51..60  54018/1%        1306077/46%
    61..70  112071/3%       1418148/50%
    71..80  79424/2%        1497572/52%
    81..90  126172/4%       1623744/57%
    91..100 1206534/42%     2830278/100%
      [34] .debug_info       PROGBITS        0000000000000000 2f1c74c a449548
00      0   0  1
      [38] .debug_loclists   PROGBITS        0000000000000000 ff5df39 507acd8
00      0   0  1
    So, size of .debug_info+.debug_loclists grows for vanilla -> patch1 (or
patch2) by
    0.651% and for vanilla -> patch3 by 0.020%.

    32-bit cc1plus
    ==============
    vanilla
    cov%    samples cumul
    0..10   1061892/37%     1061892/37%
    11..20  34002/1%        1095894/39%
    21..30  43513/1%        1139407/40%
    31..40  41667/1%        1181074/42%
    41..50  59144/2%        1240218/44%
    51..60  47009/1%        1287227/45%
    61..70  105069/3%       1392296/49%
    71..80  72990/2%        1465286/52%
    81..90  125988/4%       1591274/56%
    91..100 1208726/43%     2800000/100%
      [33] .debug_info       PROGBITS        00000000 351ab10 8b1c83d 00      0
  0  1
      [37] .debug_loclists   PROGBITS        00000000 ebc816e 3fe44fd 00      0
  0  1
    patch1 (same as patch2)
    cov%    samples cumul
    0..10   1061999/37%     1061999/37%
    11..20  34065/1%        1096064/39%
    21..30  43557/1%        1139621/40%
    31..40  41690/1%        1181311/42%
    41..50  59191/2%        1240502/44%
    51..60  47143/1%        1287645/45%
    61..70  105045/3%       1392690/49%
    71..80  73021/2%        1465711/52%
    81..90  125885/4%       1591596/56%
    91..100 1208404/43%     2800000/100%
      [33] .debug_info       PROGBITS        00000000 351ab10 8b1c597 00      0
  0  1
      [37] .debug_loclists   PROGBITS        00000000 ebca915 401ffad 00      0
  0  1
    patch3
    cov%    samples cumul
    0..10   1062006/37%     1062006/37%
    11..20  34073/1%        1096079/39%
    21..30  43559/1%        1139638/40%
    31..40  41693/1%        1181331/42%
    41..50  59189/2%        1240520/44%
    51..60  47142/1%        1287662/45%
    61..70  105054/3%       1392716/49%
    71..80  73027/2%        1465743/52%
    81..90  125874/4%       1591617/56%
    91..100 1208383/43%     2800000/100%
      [33] .debug_info       PROGBITS        00000000 351ab10 8b1c690 00      0
  0  1
      [37] .debug_loclists   PROGBITS        00000000 ebca40a 4020a6e 00      0
  0  1
    So, size of .debug_info+.debug_loclists grows for vanilla -> patch1 (or
patch2) by
    0.114% and for vanilla -> patch3 by 0.116%.

    2021-10-10  Jakub Jelinek  <jakub@redhat.com>

            PR debug/102441
            * var-tracking.c (add_stores): For cselib_sp_derived_value_p values
            use MO_VAL_SET if loc is not sp.

    (cherry picked from commit 9583b26f3701ea0456405d84f9a898451a2f7452)

^ permalink raw reply	[flat|nested] 9+ messages in thread

* [Bug debug/102441] [10 Regression] Incorrect location list in debug info
  2021-09-22  6:36 [Bug debug/102441] New: Incorrect location list in debug info liyd2021 at gmail dot com
                   ` (4 preceding siblings ...)
  2021-10-10 10:18 ` cvs-commit at gcc dot gnu.org
@ 2021-10-10 11:29 ` jakub at gcc dot gnu.org
  2022-05-10  8:21 ` cvs-commit at gcc dot gnu.org
  2022-05-10 10:09 ` jakub at gcc dot gnu.org
  7 siblings, 0 replies; 9+ messages in thread
From: jakub at gcc dot gnu.org @ 2021-10-10 11:29 UTC (permalink / raw)
  To: gcc-bugs

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=102441

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
            Summary|[10/11/12 Regression]       |[10 Regression] Incorrect
                   |Incorrect location list in  |location list in debug info
                   |debug info                  |

--- Comment #6 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
Fixed for 11.3+ and 12.1+ so far.

^ permalink raw reply	[flat|nested] 9+ messages in thread

* [Bug debug/102441] [10 Regression] Incorrect location list in debug info
  2021-09-22  6:36 [Bug debug/102441] New: Incorrect location list in debug info liyd2021 at gmail dot com
                   ` (5 preceding siblings ...)
  2021-10-10 11:29 ` [Bug debug/102441] [10 " jakub at gcc dot gnu.org
@ 2022-05-10  8:21 ` cvs-commit at gcc dot gnu.org
  2022-05-10 10:09 ` jakub at gcc dot gnu.org
  7 siblings, 0 replies; 9+ messages in thread
From: cvs-commit at gcc dot gnu.org @ 2022-05-10  8:21 UTC (permalink / raw)
  To: gcc-bugs

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=102441

--- Comment #7 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The releases/gcc-10 branch has been updated by Jakub Jelinek
<jakub@gcc.gnu.org>:

https://gcc.gnu.org/g:a34be854e83c79229a7ece3682df7167f745dbfd

commit r10-10648-ga34be854e83c79229a7ece3682df7167f745dbfd
Author: Jakub Jelinek <jakub@redhat.com>
Date:   Sun Oct 10 12:13:22 2021 +0200

    var-tracking: Fix a wrong-debug issue caused by my r10-7665 var-tracking
change [PR102441]

    Since my r10-7665-g33c45e51b4914008064d9b77f2c1fc0eea1ad060 change, we get
    wrong-debug on e.g. the following testcase at -O2 -g on x86_64-linux for
the
    x parameter:
    void bar (int *r);
    int
    foo (int x)
    {
      int r = 0;
      bar (&r);
      return r;
    }
    At the start of function, we have
            subq    $24, %rsp
            leaq    12(%rsp), %rdi
    instructions.  The x parameter is passed in %rdi, but isn't used in the
    function and so the leaq instruction overwrites %rdi without remembering
    %rdi anywhere.  Before the r10-7665 change (which was trying to fix a large
    (3% for 32-bit, 1% for 64-bit x86-64) debug info/loc growth introduced with
    r10-7515), the leaq insn above resulted in a MO_VAL_SET micro-operation
that
    said that the value of sp + 12, a cselib_sp_derived_value_p, is stored into
    the %rdi register.  The r10-7665 change added a change to add_stores that
    added no micro-operation for the leaq store, with the rationale that the sp
    based values can be and will be always computable some other more compact
    and primarily more stable way (cfa based expression like DW_OP_fbreg, that
    is the same in the whole function).  That is true.  But by throwing the
    micro-operation on the floor, we miss another important part of the
    MO_VAL_SET, in particular that the destination of the store, %rdi in this
    case, now has a different value from what it had before, so the vt_*
    dataflow code thinks that even after the leaq instruction %rdi still holds
    the x argument value (and changes it to DW_OP_entry_value (%rdi) only in
the
    middle of the call to bar).  Previously and with the patches below,
    the location for x changes already at the end of leaq instruction to
    DW_OP_entry_value (%rdi).

    My first attempt to fix this was instead of dropping the MO_VAL_SET add
    a MO_CLOBBER operation:
    --- gcc/var-tracking.c.jj       2021-05-04 21:02:24.196799586 +0200
    +++ gcc/var-tracking.c  2021-09-24 19:23:16.420154828 +0200
    @@ -6133,7 +6133,9 @@ add_stores (rtx loc, const_rtx expr, voi
         {
           if (preserve)
            preserve_value (v);
    -      return;
    +      mo.type = MO_CLOBBER;
    +      mo.u.loc = loc;
    +      goto log_and_return;
         }

       nloc = replace_expr_with_values (oloc);
    so don't track that the value lives in the loc destination, but track
    that the previous value doesn't live there anymore.  That failed bootstrap
    miserably, the vt_* code isn't prepared to see MO_CLOBBER of a MEM that
    isn't tracked (e.g. has MEM_EXPR on it that the var-tracking code wants
    to track, i.e. track_p in add_stores).  On the other side, thinking about
    it more, in the most common case where a cselib_sp_derived_value_p value
    is stored into the sp register (and which is the reason why PR94495
    testcase got larger), dropping the micro-operation on the floor is the
    right thing, because we have that cselib_sp_derived_value_p tracking, any
    reads from the sp hard register will be treated as
    cselib_sp_derived_value_p.
    Then I've tried 3 different patches described below and in the end
    what is committed is patch2.
    Additionally, I've gathered statistics from cc1plus by always reverting the
    var-tracking.c change after finished bootstrap/regtest and rebuilding the
    stage3 var-tracking.o and cc1plus, such that it would be comparable.
    dwlocstat and .debug_{info,loclists} section sizes detailed below.
    patch3 uses MO_VAL_SET (i.e. essentially reversion of the r10-7665
    change) when destination is not a REG_P and !track_p, otherwise if
    destination is sp drops the micro-operation on the floor (i.e. no change),
    otherwise adds a MO_CLOBBER.
    patch1 is similar, except it checks for destination not equal to sp and
    !track_p, i.e. for !track_p REG_P destinations other than sp it will use
    MO_VAL_SET rather than MO_CLOBBER.
    Finally, patch2, the shortest patch, uses MO_VAL_SET whenever destination
    is not sp and otherwise drops the micro-operation on the floor.
    All the 3 patches don't affect the PR94495 testcase, all the changes
    there were caused by stores of sp based values into %rsp.

    While the patch2 (and patch1 which results in exactly the same sizes)
    causes the largest debug loclists/info growth from the 3, it is still quite
    minor (0.651% on 64-bit and 0.114% on 32-bit) compared
    to the 1% and 3% PR94495 was trying to solve, and I actually think it is
the
    best thing to do.  Because, if we have say
      int q[10];
      int *p = &q[0];
    or similar and we load the &q[0] sp based value into some hard register,
    by noting in the debug info that p lives in some hard reg for some part
    of the function and a user is trying to change the p var in the debugger,
    if we say it lives in some register or memory, there is some chance that
    the changing of the value could work successfully (of course, nothing
    is guaranteed, we don't have tracking of where each var lives at which
    moment for changing purposes (i.e. what register, memory or else you need
    to change in order to change behavior of the code)), while if we just say
    that p's location is DW_OP_fbreg 16 DW_OP_stack_value, that is a read-only
    value one can just print but not change.  Now, for stores of variable
    values into the sp register, I don't think we have such an issue, you don't
    want debugger to change your stack pointer when user asks to change value
    of some variable whose value lives in the stack pointer, that would pretty
    much always result in misbehavior of the program.
    So, my preference from these 3 is patch2 and that is being committed.

    64-bit cc1plus
    ==============
    vanilla
    cov%    samples cumul
    0..10   1064665/37%     1064665/37%
    11..20  35972/1%        1100637/38%
    21..30  47969/1%        1148606/40%
    31..40  45787/1%        1194393/42%
    41..50  57529/2%        1251922/44%
    51..60  53974/1%        1305896/46%
    61..70  112055/3%       1417951/50%
    71..80  79420/2%        1497371/52%
    81..90  126225/4%       1623596/57%
    91..100 1206682/42%     2830278/100%
      [34] .debug_info       PROGBITS        0000000000000000 2f1c74c a44949f
00      0   0  1
      [38] .debug_loclists   PROGBITS        0000000000000000 ff5d046 506e947
00      0   0  1
    patch1 (same as patch2)
    cov%    samples cumul
    0..10   1064685/37%     1064685/37%
    11..20  36011/1%        1100696/38%
    21..30  47975/1%        1148671/40%
    31..40  45799/1%        1194470/42%
    41..50  57566/2%        1252036/44%
    51..60  54011/1%        1306047/46%
    61..70  112068/3%       1418115/50%
    71..80  79421/2%        1497536/52%
    81..90  126171/4%       1623707/57%
    91..100 1206571/42%     2830278/100%
      [34] .debug_info       PROGBITS        0000000000000000 2f1c74c a448f27
00      0   0  1
      [38] .debug_loclists   PROGBITS        0000000000000000 ff608bc 52070dd
00      0   0  1
    patch3
    cov%    samples cumul
    0..10   1064698/37%     1064698/37%
    11..20  36018/1%        1100716/38%
    21..30  47977/1%        1148693/40%
    31..40  45804/1%        1194497/42%
    41..50  57562/2%        1252059/44%
    51..60  54018/1%        1306077/46%
    61..70  112071/3%       1418148/50%
    71..80  79424/2%        1497572/52%
    81..90  126172/4%       1623744/57%
    91..100 1206534/42%     2830278/100%
      [34] .debug_info       PROGBITS        0000000000000000 2f1c74c a449548
00      0   0  1
      [38] .debug_loclists   PROGBITS        0000000000000000 ff5df39 507acd8
00      0   0  1
    So, size of .debug_info+.debug_loclists grows for vanilla -> patch1 (or
patch2) by
    0.651% and for vanilla -> patch3 by 0.020%.

    32-bit cc1plus
    ==============
    vanilla
    cov%    samples cumul
    0..10   1061892/37%     1061892/37%
    11..20  34002/1%        1095894/39%
    21..30  43513/1%        1139407/40%
    31..40  41667/1%        1181074/42%
    41..50  59144/2%        1240218/44%
    51..60  47009/1%        1287227/45%
    61..70  105069/3%       1392296/49%
    71..80  72990/2%        1465286/52%
    81..90  125988/4%       1591274/56%
    91..100 1208726/43%     2800000/100%
      [33] .debug_info       PROGBITS        00000000 351ab10 8b1c83d 00      0
  0  1
      [37] .debug_loclists   PROGBITS        00000000 ebc816e 3fe44fd 00      0
  0  1
    patch1 (same as patch2)
    cov%    samples cumul
    0..10   1061999/37%     1061999/37%
    11..20  34065/1%        1096064/39%
    21..30  43557/1%        1139621/40%
    31..40  41690/1%        1181311/42%
    41..50  59191/2%        1240502/44%
    51..60  47143/1%        1287645/45%
    61..70  105045/3%       1392690/49%
    71..80  73021/2%        1465711/52%
    81..90  125885/4%       1591596/56%
    91..100 1208404/43%     2800000/100%
      [33] .debug_info       PROGBITS        00000000 351ab10 8b1c597 00      0
  0  1
      [37] .debug_loclists   PROGBITS        00000000 ebca915 401ffad 00      0
  0  1
    patch3
    cov%    samples cumul
    0..10   1062006/37%     1062006/37%
    11..20  34073/1%        1096079/39%
    21..30  43559/1%        1139638/40%
    31..40  41693/1%        1181331/42%
    41..50  59189/2%        1240520/44%
    51..60  47142/1%        1287662/45%
    61..70  105054/3%       1392716/49%
    71..80  73027/2%        1465743/52%
    81..90  125874/4%       1591617/56%
    91..100 1208383/43%     2800000/100%
      [33] .debug_info       PROGBITS        00000000 351ab10 8b1c690 00      0
  0  1
      [37] .debug_loclists   PROGBITS        00000000 ebca40a 4020a6e 00      0
  0  1
    So, size of .debug_info+.debug_loclists grows for vanilla -> patch1 (or
patch2) by
    0.114% and for vanilla -> patch3 by 0.116%.

    2021-10-10  Jakub Jelinek  <jakub@redhat.com>

            PR debug/102441
            * var-tracking.c (add_stores): For cselib_sp_derived_value_p values
            use MO_VAL_SET if loc is not sp.

    (cherry picked from commit 9583b26f3701ea0456405d84f9a898451a2f7452)

^ permalink raw reply	[flat|nested] 9+ messages in thread

* [Bug debug/102441] [10 Regression] Incorrect location list in debug info
  2021-09-22  6:36 [Bug debug/102441] New: Incorrect location list in debug info liyd2021 at gmail dot com
                   ` (6 preceding siblings ...)
  2022-05-10  8:21 ` cvs-commit at gcc dot gnu.org
@ 2022-05-10 10:09 ` jakub at gcc dot gnu.org
  7 siblings, 0 replies; 9+ messages in thread
From: jakub at gcc dot gnu.org @ 2022-05-10 10:09 UTC (permalink / raw)
  To: gcc-bugs

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=102441

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

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

--- Comment #8 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
Fixed.

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2022-05-10 10:09 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-09-22  6:36 [Bug debug/102441] New: Incorrect location list in debug info liyd2021 at gmail dot com
2021-09-22  7:28 ` [Bug rtl-optimization/102441] [10/11/12 Regression] " rguenth at gcc dot gnu.org
2021-09-24  9:57 ` jakub at gcc dot gnu.org
2021-09-24 17:26 ` jakub at gcc dot gnu.org
2021-10-10 10:17 ` [Bug debug/102441] " cvs-commit at gcc dot gnu.org
2021-10-10 10:18 ` cvs-commit at gcc dot gnu.org
2021-10-10 11:29 ` [Bug debug/102441] [10 " jakub at gcc dot gnu.org
2022-05-10  8:21 ` cvs-commit at gcc dot gnu.org
2022-05-10 10:09 ` jakub at gcc dot gnu.org

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).