Patchwork Improve debug info for small structures passed by reference

login
register
mail settings
Submitter Eric Botcazou
Date June 16, 2013, 10:51 a.m.
Message ID <15074732.mILDtaeVB4@polaris>
Download mbox | patch
Permalink /patch/251667/
State New
Headers show

Comments

Eric Botcazou - June 16, 2013, 10:51 a.m.
Hi,

the subject is slightly misleading since it's actually about structures with 
integral modes which are passed by reference on some platforms, e.g. SPARC or 
PowerPC 32-bit.  There are 3 issues:

 1. At -O0, the parameters of this kind are not homed on the stack, which 
means that backtraces can display <optimized out> for them.

 2. Since the fix for PR debug/48163, the locations emitted at -O1 and above 
for them are totally bogus (there is a bogus additional dereference).

 3. The location lists are quickly wrong for them because they fail to take 
into account that the register parameter is clobbered.

The attached patch addresses 1. and 2. fully and 3. partially (for now).  It 
has been tested on x86-64 and PowerPC/Linux.  OK for the mainline?


2013-06-16  Eric Botcazou  <ebotcazou@adacore.com>

	* function.c (assign_parm_setup_reg): For a parameter passed by pointer
	and which can live in a register, always retrieve the value on entry.
	* var-tracking.c (add_stores): Treat the copy on entry for a parameter
	passed by invisible reference specially.
	(emit_notes_in_bb) <MO_VAL_USE>: Emit notes before the instruction.
	(vt_add_function_parameter): Correctly deal with a parameter passed by
	invisible reference.
Jakub Jelinek - June 16, 2013, 11:12 a.m.
On Sun, Jun 16, 2013 at 12:51:25PM +0200, Eric Botcazou wrote:
> the subject is slightly misleading since it's actually about structures with 
> integral modes which are passed by reference on some platforms, e.g. SPARC or 
> PowerPC 32-bit.  There are 3 issues:
> 
>  1. At -O0, the parameters of this kind are not homed on the stack, which 
> means that backtraces can display <optimized out> for them.
> 
>  2. Since the fix for PR debug/48163, the locations emitted at -O1 and above 
> for them are totally bogus (there is a bogus additional dereference).
> 
>  3. The location lists are quickly wrong for them because they fail to take 
> into account that the register parameter is clobbered.
> 
> The attached patch addresses 1. and 2. fully and 3. partially (for now).  It 
> has been tested on x86-64 and PowerPC/Linux.  OK for the mainline?

This really should come with testcases (e.g. guality ones).

	Jakub
Eric Botcazou - June 17, 2013, 7:46 a.m.
> This really should come with testcases (e.g. guality ones).

guality testcases are barely maintainable, especially on non-x86 platforms, so 
I'd rather not enter this business.  I'll discuss with Joel and see whether we 
can coordinate with the GDB side.
Jakub Jelinek - June 17, 2013, 7:51 a.m.
On Mon, Jun 17, 2013 at 09:46:30AM +0200, Eric Botcazou wrote:
> > This really should come with testcases (e.g. guality ones).
> 
> guality testcases are barely maintainable, especially on non-x86 platforms, so 
> I'd rather not enter this business.  I'll discuss with Joel and see whether we 
> can coordinate with the GDB side.

Especially if it is -O0 only, I don't see why you think so.  Just dg-skip-if
it for -O1+ if you believe it is unreliable for some reason, but if you
look at the parameter value after the prologue, not showing the right value
at -O0 would be a serious bug everywhere.  Having some GDB testcase also
doesn't hurt, but having it in GCC testsuite has significant advantages over
that, most GCC developers don't run GDB testsuite after any changes they do.

	Jakub
Eric Botcazou - June 19, 2013, 3:19 p.m.
> Especially if it is -O0 only, I don't see why you think so.  Just dg-skip-if
> it for -O1+ if you believe it is unreliable for some reason, but if you
> look at the parameter value after the prologue, not showing the right value
> at -O0 would be a serious bug everywhere.  Having some GDB testcase also
> doesn't hurt, but having it in GCC testsuite has significant advantages
> over that, most GCC developers don't run GDB testsuite after any changes
> they do.

All right, I've attached a couple of guality testcases (param-1.c for -O0 and 
param-2.c for -O1 -fno-var-tracking-assignments, a param-3.c for bare -O1 will 
require further adjustments in var-tracking.c).  Unfortunately they don't fail 
without the patch e.g. on PowerPC, they are reported as unsupported instead 
because of "Cannot access memory at address" messages from GDB...


	* gcc.dg/guality/param-1.c: New test.
	* gcc.dg/guality/param-2.c: Likewise.

Patch

Index: function.c
===================================================================
--- function.c	(revision 200122)
+++ function.c	(working copy)
@@ -3084,17 +3084,27 @@  assign_parm_setup_reg (struct assign_par
     emit_move_insn (parmreg, validated_mem);
 
   /* If we were passed a pointer but the actual value can safely live
-     in a register, put it in one.  */
-  if (data->passed_pointer
-      && TYPE_MODE (TREE_TYPE (parm)) != BLKmode
-      /* If by-reference argument was promoted, demote it.  */
-      && (TYPE_MODE (TREE_TYPE (parm)) != GET_MODE (DECL_RTL (parm))
-	  || use_register_for_decl (parm)))
+     in a register, retrieve it and use it directly.  */
+  if (data->passed_pointer && TYPE_MODE (TREE_TYPE (parm)) != BLKmode)
     {
       /* We can't use nominal_mode, because it will have been set to
 	 Pmode above.  We must use the actual mode of the parm.  */
-      parmreg = gen_reg_rtx (TYPE_MODE (TREE_TYPE (parm)));
-      mark_user_reg (parmreg);
+      if (use_register_for_decl (parm))
+	{
+	  parmreg = gen_reg_rtx (TYPE_MODE (TREE_TYPE (parm)));
+	  mark_user_reg (parmreg);
+	}
+      else
+	{
+	  int align = STACK_SLOT_ALIGNMENT (TREE_TYPE (parm),
+					    TYPE_MODE (TREE_TYPE (parm)),
+					    TYPE_ALIGN (TREE_TYPE (parm)));
+	  parmreg
+	    = assign_stack_local (TYPE_MODE (TREE_TYPE (parm)),
+				  GET_MODE_SIZE (TYPE_MODE (TREE_TYPE (parm))),
+				  align);
+	  set_mem_attributes (parmreg, parm, 1);
+	}
 
       if (GET_MODE (parmreg) != GET_MODE (DECL_RTL (parm)))
 	{
Index: var-tracking.c
===================================================================
--- var-tracking.c	(revision 200122)
+++ var-tracking.c	(working copy)
@@ -5836,7 +5836,24 @@  add_stores (rtx loc, const_rtx expr, voi
 	    {
 	      rtx xexpr = gen_rtx_SET (VOIDmode, loc, src);
 	      if (same_variable_part_p (src, REG_EXPR (loc), REG_OFFSET (loc)))
-		mo.type = MO_COPY;
+		{
+		  /* If this is an instruction copying (part of) a parameter
+		     passed by invisible reference to its register location,
+		     pretend it's a SET so that the initial memory location
+		     is discarded, as the parameter register can be reused
+		     for other purposes and we do not track locations based
+		     on generic registers.  */
+		  if (MEM_P (src)
+		      && REG_EXPR (loc)
+		      && TREE_CODE (REG_EXPR (loc)) == PARM_DECL
+		      && DECL_MODE (REG_EXPR (loc)) != BLKmode
+		      && MEM_P (DECL_INCOMING_RTL (REG_EXPR (loc)))
+		      && XEXP (DECL_INCOMING_RTL (REG_EXPR (loc)), 0)
+			 != arg_pointer_rtx)
+		    mo.type = MO_SET;
+		  else
+		    mo.type = MO_COPY;
+		}
 	      else
 		mo.type = MO_SET;
 	      mo.u.loc = xexpr;
@@ -9086,7 +9103,7 @@  emit_notes_in_bb (basic_block bb, datafl
 	      else
 		var_mem_set (set, loc, VAR_INIT_STATUS_UNINITIALIZED, NULL);
 
-	      emit_notes_for_changes (insn, EMIT_NOTE_AFTER_INSN, set->vars);
+	      emit_notes_for_changes (insn, EMIT_NOTE_BEFORE_INSN, set->vars);
 	    }
 	    break;
 
@@ -9533,12 +9550,11 @@  vt_add_function_parameter (tree parm)
 
   if (!vt_get_decl_and_offset (incoming, &decl, &offset))
     {
-      if (REG_P (incoming) || MEM_P (incoming))
+      if (MEM_P (incoming))
 	{
 	  /* This means argument is passed by invisible reference.  */
 	  offset = 0;
 	  decl = parm;
-	  incoming = gen_rtx_MEM (GET_MODE (decl_rtl), incoming);
 	}
       else
 	{