[Bug debug/48220] New: DW_OP_GNU_entry_value/DW_TAG_GNU_call_site_parameter vs. register window targets

[Bug debug/48220] New: DW_OP_GNU_entry_value/DW_TAG_GNU_call_site_parameter vs. register window targets

[jakub at gcc dot gnu.org]

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

           Summary: DW_OP_GNU_entry_value/DW_TAG_GNU_call_site_parameter
                    vs. register window targets
           Product: gcc
           Version: 4.7.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: debug
        AssignedTo: unassigned@gcc.gnu.org
        ReportedBy: jakub@gcc.gnu.org


I haven't thought too much about register window issues when designing this
extension, so it would be good to decide what exactly to do for such targets.

Let's start with some testcases:

__attribute__((noinline, noclone))
void fn1 (long int x, long int y, long int z)
{
  asm volatile ("" : : "r" (x), "r" (y), "r" (z) : "memory");
}

__attribute__((noinline, noclone)) long int
fn2 (long int a, long int b, long int c) 
{
  long int q = 2 * a;
  fn1 (5, 6, 7); 
  return 0;
}

long int
fn3 (long int x, long int (*fn4) (long int *))
{
  long int v, w, w2, z;
  w = (*fn4) (&w2);
  v = (*fn4) (&w2);
  z = fn2 (1, v + 1, w);
  {
    int v1 = v + 4;
    z += fn2 (w, v * 2, x);
  }
  return z;
}

int
main (void)
{
  return 0;
}

and testcase from http://gcc.gnu.org/ml/gcc-patches/2011-03/msg01224.html

While GDB support has been written for these extensions on
http://sourceware.org/git/?p=archer.git;a=shortlog;h=refs/heads/archer-jankratochvil-entryval
I bet it hasn't been tested on sparc/ia64/and other register window targets
yet.

For SPARC, my preference would be if DW_OP_GNU_entry_value contained the output
regs, after all when it is defined as value at the start of the function (==
put a breakpoint on the very first insn and use value of that register), then
the parameters are passed in %o[0-5] registers rather than %i[0-5].
And I guess the gdb changes should then magically work when using
DW_TAG_call_site info - it will just look up the same registers with no
translation.  OT, seems if we wanted to be exact we'd need to model the
register window save/restrore insns in var-tracking adjust_insn and start in
vt_add_function_parameter with the outgoing regs instead of incoming that are
in DECL_INCOMING_RTL.
Current SVN trunk uses %i* instead of %o* in DW_OP_GNU_entry_value operands
and %o* registers in DW_AT_location of DW_OP_GNU_entry_value.

IA-64 is more challenging.  On SPARC just the save/restore insns are relevant
to the register windows, while on IA-64 both br{,l}.call and alloc insns.
With the definition that DW_OP_GNU_entry_value should be defined as put a
breakpoint on the very first insn in a function, remember register values there
I'd say thus that DW_OP_GNU_entry_value should reference r32/r33/etc. regs. 
The question is what should be used in DW_AT_location of
DW_TAG_GNU_call_site_parameter.  Either it can be the state on the br.call insn
itself (which would mean they are the outN registers mapped accordinly to the
hard register numbers given the current alloc insn), or after the call insn is
executed (then we'd need to map the outN registers to the inNs).
The first option would mean more work for the debugger on IA-64, it would need
to map the outNs to inS, but perhaps is more in line with the proposal that
DW_TAG_GNU_call_site_parameter lists the location at the call.  And it would
require no changes to GCC.  The other option would mean less work for the
debugger, but GCC would need to map the parameter registers in
prepare_call_arguments using INCOMING_REGNO for ia64.  If we go this route, the
question is if registers mentioned in DW_AT_GNU_call_site_value etc. shouldn't
be remapped too, so I very much prefer the first variant for IA-64 if it can be
implemented.

[Bug debug/48220] DW_OP_GNU_entry_value/DW_TAG_GNU_call_site_parameter vs. register window targets

[jakub at gcc dot gnu.org]

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

--- Comment #1 from Jakub Jelinek <jakub at gcc dot gnu.org> 2011-03-21 16:09:03 UTC ---
Created attachment 23739
  --> http://gcc.gnu.org/bugzilla/attachment.cgi?id=23739
gcc47-pr48220.patch

Untested patch, which changes using %iN to %oN registers in
DW_OP_GNU_entry_value on SPARC, and doesn't change anything on IA-64 (if we
choose to use variant 1 there).

No idea about mep/mmix/xtensa - the patch might make a difference on xtensa
only I think, as mep doesn't define OUTGOING_REGNO and mmix doesn't define
LEAF_REGISTERS.

[Bug debug/48220] DW_OP_GNU_entry_value/DW_TAG_GNU_call_site_parameter vs. register window targets

[ebotcazou at gcc dot gnu.org]

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

Eric Botcazou <ebotcazou at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |NEW
   Last reconfirmed|                            |2011.03.22 20:34:34
     Ever Confirmed|0                           |1

--- Comment #2 from Eric Botcazou <ebotcazou at gcc dot gnu.org> 2011-03-22 20:34:34 UTC ---
> For SPARC, my preference would be if DW_OP_GNU_entry_value contained the
> output regs, after all when it is defined as value at the start of the
> function (== put a breakpoint on the very first insn and use value of that 
> register), then the parameters are passed in %o[0-5] registers rather than 
> %i[0-5].

That depends on what you call the start of the function though.  Doesn't GDB
put the breakpoint at the end of the prologue (in fact 2nd loc directive) when
you say "break my_function"?  There the arguments are in %i[0-5] already.

> And I guess the gdb changes should then magically work when using
> DW_TAG_call_site info - it will just look up the same registers with no
> translation.  OT, seems if we wanted to be exact we'd need to model the
> register window save/restrore insns in var-tracking adjust_insn and start in
> vt_add_function_parameter with the outgoing regs instead of incoming that are
> in DECL_INCOMING_RTL.
> Current SVN trunk uses %i* instead of %o* in DW_OP_GNU_entry_value operands
> and %o* registers in DW_AT_location of DW_OP_GNU_entry_value.

"DW_AT_location of DW_TAG_GNU_call_site_parameter" I presume?

I think that the current situation is consistent with what we do at -O0, i.e.
home the arguments onto the stack in the prologue and emit a single location
for them in the debug info, the location in the stack.  This yields correct
results with GDB because of the 2nd loc directive trick.

Of course you have a small window of invalidity (we got complaints about it at
AdaCore from folks working on other debuggers) so modeling the register window
instructions would indeed be better.  Would that be hard to do?

[Bug debug/48220] DW_OP_GNU_entry_value/DW_TAG_GNU_call_site_parameter vs. register window targets

[jakub at gcc dot gnu.org]

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

--- Comment #3 from Jakub Jelinek <jakub at gcc dot gnu.org> 2011-03-23 16:45:05 UTC ---
The aim of the extension is to allow correct debug info, not almost correct, so
I think defining "upon entering of the current subprogram" as anything but
before the first insn in it is wrong and would make it not possible to use
DW_OP_GNU_entry_value before the point (end of prologue or what?).
Consider e.g. shrink-wrapping Bernd just proposed for GCC too...
So I think what we currently do on SPARC has to change.

As for teaching var-tracking about save/restore on SPARC, it would be a matter
of adding probably two target hooks, one that would be run e.g. at the end of
adjust_insn and would be supposed to change it using validate_change (...,
true); in whatever way var-tracking should understand the insn.
So e.g. for save you'd add into the parallel things like:
  (set (reg:P 24) (reg:P 8))
  (clobber (reg:P 8))
and similarly for all the other param regs.  Perhaps even make it explicit what
exactly is subtracted from %sp.
And the second target hook would return a different rtx for DECL_INCOMING_RTL,
with registers adjusted back.  Because for -O0 if we don't do var-tracking we
probably want DECL_INCOMING_RTL to still refer to %i0 etc., even when it is not
correct before the save.

[Bug debug/48220] DW_OP_GNU_entry_value/DW_TAG_GNU_call_site_parameter vs. register window targets

[ebotcazou at gcc dot gnu.org]

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

--- Comment #4 from Eric Botcazou <ebotcazou at gcc dot gnu.org> 2011-03-23 17:33:54 UTC ---
> The aim of the extension is to allow correct debug info, not almost correct,
> so I think defining "upon entering of the current subprogram" as anything but
> before the first insn in it is wrong and would make it not possible to use
> DW_OP_GNU_entry_value before the point (end of prologue or what?).

Yes, end of the prologue.  I was under the impression that, with your alternate
scheme, the opposite situation would arise, i.e. it wouldn't be possible to use
DW_OP_GNU_entry_value after the end of the prologue.  Given how GDB works, this
sounded far less appealing than the current scheme.  Sorry if I misunderstood.

> As for teaching var-tracking about save/restore on SPARC, it would be a matter
> of adding probably two target hooks, one that would be run e.g. at the end of
> adjust_insn and would be supposed to change it using validate_change (...,
> true); in whatever way var-tracking should understand the insn.
> So e.g. for save you'd add into the parallel things like:
>   (set (reg:P 24) (reg:P 8))
>   (clobber (reg:P 8))
> and similarly for all the other param regs.  Perhaps even make it explicit
> what exactly is subtracted from %sp.
> And the second target hook would return a different rtx for DECL_INCOMING_RTL,
> with registers adjusted back.  Because for -O0 if we don't do var-tracking we
> probably want DECL_INCOMING_RTL to still refer to %i0 etc., even when it is
> not correct before the save.

I see, thanks for the explanation.

[Bug debug/48220] DW_OP_GNU_entry_value/DW_TAG_GNU_call_site_parameter vs. register window targets

[jakub at gcc dot gnu.org]

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

--- Comment #5 from Jakub Jelinek <jakub at gcc dot gnu.org> 2011-03-23 20:30:53 UTC ---
(In reply to comment #4)
> Yes, end of the prologue.  I was under the impression that, with your alternate
> scheme, the opposite situation would arise, i.e. it wouldn't be possible to use
> DW_OP_GNU_entry_value after the end of the prologue.  Given how GDB works, this
> sounded far less appealing than the current scheme.  Sorry if I misunderstood.

If say reference to first parameter's original value is
DW_OP_GNU_entry_value 1 <DW_OP_reg8> (i.e. %o0), then it will work just fine
anywhere in the function.  The debugger either looks up matching entry
for %o0 in DW_TAG_GNU_call_site, or a debugger could put a breakpoint on the
first insn in a function and remember the %o0 value there, then just use the
remembered value anywhere where DW_OP_GNU_entry_value appears.
And for SPARC, using %o0 is also much easier for the debugger, it doesn't have
to translate anything.

On IA-64 things are different because there it is mainly the call insn which
moves the register window, not some insn executed somewhere in function's
prologue.

[Bug debug/48220] DW_OP_GNU_entry_value/DW_TAG_GNU_call_site_parameter vs. register window targets

[ebotcazou at gcc dot gnu.org]

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

Eric Botcazou <ebotcazou at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |ASSIGNED
         AssignedTo|unassigned at gcc dot       |ebotcazou at gcc dot
                   |gnu.org                     |gnu.org

--- Comment #6 from Eric Botcazou <ebotcazou at gcc dot gnu.org> 2011-06-02 19:39:58 UTC ---
I'm going to do some testing on the SPARC.

[Bug debug/48220] DW_OP_GNU_entry_value/DW_TAG_GNU_call_site_parameter vs register window

[ebotcazou at gcc dot gnu.org]

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

Eric Botcazou <ebotcazou at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #23739|0                           |1
        is obsolete|                            |

--- Comment #7 from Eric Botcazou <ebotcazou at gcc dot gnu.org> 2011-07-11 16:02:28 UTC ---
Created attachment 24740
  --> http://gcc.gnu.org/bugzilla/attachment.cgi?id=24740
Tentative patch for SPARC

This appears to do the trick for the SPARC.  In particular:

(gdb) run
Starting program: /nile.build/botcazou/gcc-head/sparc-sun-solaris2.8/pr48220

Breakpoint 1, fn2 (a=1, b=2, c=1) at pr48220.c:13
13      {
(gdb) disass
Dump of assembler code for function fn2:
=> 0x000106cc <+0>:     save  %sp, -96, %sp
   0x000106d0 <+4>:     clr  %i0
   0x000106d4 <+8>:     mov  5, %o0
   0x000106d8 <+12>:    mov  6, %o1
   0x000106dc <+16>:    call  0x106c4 <fn1>
   0x000106e0 <+20>:    mov  7, %o2
   0x000106e4 <+24>:    rett  %i7 + 8
   0x000106e8 <+28>:    nop
End of assembler dump.
(gdb) p a
$1 = 1
(gdb) p &a
Address requested for identifier "a" which is in register $o0
(gdb) nexti
17      }
(gdb) p a
$2 = 1
(gdb) p &a
Address requested for identifier "a" which is in register $i0
(gdb) nexti
15        fn1 (5, 6, 7);
(gdb) p a
$3 = <optimized out>

and the location is DW_OP_GNU_entry_value pointing to $o0 from there on.


Jakub, what do you think?

[Bug debug/48220] DW_OP_GNU_entry_value/DW_TAG_GNU_call_site_parameter vs register window

[jakub at gcc dot gnu.org]

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

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

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

--- Comment #8 from Jakub Jelinek <jakub at gcc dot gnu.org> 2011-07-11 16:40:54 UTC ---
Looks reasonable to me.  Thanks for working on it.

[Bug debug/48220] DW_OP_GNU_entry_value/DW_TAG_GNU_call_site_parameter vs register window

[ebotcazou at gcc dot gnu.org]

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

Eric Botcazou <ebotcazou at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|ASSIGNED                    |NEW
         AssignedTo|ebotcazou at gcc dot        |unassigned at gcc dot
                   |gnu.org                     |gnu.org

--- Comment #9 from Eric Botcazou <ebotcazou at gcc dot gnu.org> 2011-07-14 21:46:38 UTC ---
Thanks, submitted at http://gcc.gnu.org/ml/gcc-patches/2011-07/msg01197.html

[Bug debug/48220] DW_OP_GNU_entry_value/DW_TAG_GNU_call_site_parameter vs register window

[ebotcazou at gcc dot gnu.org]

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

--- Comment #10 from Eric Botcazou <ebotcazou at gcc dot gnu.org> 2011-07-15 17:10:00 UTC ---
Author: ebotcazou
Date: Fri Jul 15 17:09:56 2011
New Revision: 176318

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=176318
Log:
    PR target/48220
    * doc/md.texi (Standard Names): Document window_save.
    * cfgexpand.c (expand_debug_parm_decl): New function extracted from
    expand_debug_expr and expand_debug_source_expr.  If the target has
    a window_save instruction, adjust the ENTRY_VALUE_EXP.
    (expand_debug_expr) <SSA_NAME>: Call expand_debug_parm_decl if the
    SSA_NAME_VAR is a parameter.
    (expand_debug_source_expr) <PARM_DECL>: Call expand_debug_parm_decl.
    * var-tracking.c (parm_reg_t): New type and associated vector type.
    (windowed_parm_regs): New variable.
    (adjust_insn): If the target has a window_save instruction and this
    is the instruction, make its effect on parameter registers explicit.
    (next_non_note_insn_var_location): New function.
    (emit_notes_in_bb): Use it instead of NEXT_INSN throughout.
    (vt_add_function_parameter): If the target has a window_save insn,
    adjust the incoming RTL and record that in windowed_parm_regs.
    (vt_finalize): Free windowed_parm_regs.

Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/cfgexpand.c
    trunk/gcc/doc/md.texi
    trunk/gcc/var-tracking.c

[Bug debug/48220] DW_OP_GNU_entry_value/DW_TAG_GNU_call_site_parameter vs register window

[jakub at gcc dot gnu.org]

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

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

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

--- Comment #11 from Jakub Jelinek <jakub at gcc dot gnu.org> 2011-08-04 11:51:19 UTC ---
Fixed.