[PATCH] Unbreak ia64 bootstrap (PR debug/45006)

[PATCH] Unbreak ia64 bootstrap (PR debug/45006)

[Jakub Jelinek]

Hi!

My PR45003 fix apparently broke IA-64 bootstrap, because there
FUNCTION_DECLs have DECL_MODE DImode, while TYPE_MODE of FUNCTION_TYPE
is TImode.  Guess that's related to fn descriptors.

The following patch is an attempt to fix that.  For FUNCTION_DECLs,
I don't see how SIGN_EXTEND/ZERO_EXTEND on the MEM would be ever useful,
FUNCTION_DECLs ought to appear in DEBUG stmts only inside of ADDR_EXPR
where the MEM is not used anyway and we just use the address.

The adjust_mode path isn't hit just for unary expressions
(PAREN_EXPR/NOP_EXPR/CONVERT_EXPR), but also for decls and SSA_NAMEs.
The latter ones of course don't have TREE_OPERAND (exp, 0), while usually
the modes ought to match, it is IMHO better to try to be cautious.

Either of the hunks should fix the ICE, though without the first hunk
we just won't be able to emit useful debug info (as ADDR_EXPR <FUNCTION_DECL>
will return NULL on ia64 then), and without the second hunk we risk we hit
similar ICE later on.

Ok for trunk?

2010-07-20  Jakub Jelinek  <jakub@redhat.com>

	PR debug/45006
	* cfgexpand.c (expand_debug_expr): Don't go through adjust_mode
	path for FUNCTION_DECLs.  Only look at TYPE_UNSIGNED of
	operand's type if exp is tcc_unary class tree.

--- gcc/cfgexpand.c.jj	2010-07-20 12:12:14.000000000 +0200
+++ gcc/cfgexpand.c	2010-07-20 18:06:13.000000000 +0200
@@ -2369,7 +2369,10 @@ expand_debug_expr (tree exp)
 	     below would ICE.  While it is likely a FE bug,
 	     try to be robust here.  See PR43166.  */
 	  || mode == BLKmode
-	  || (mode == VOIDmode && GET_MODE (op0) != VOIDmode))
+	  || (mode == VOIDmode && GET_MODE (op0) != VOIDmode)
+	  /* On some targets DECL_MODE of a FUNCTION_DECL is
+	     different from TYPE_MODE of its type.  */
+	  || (TREE_CODE (exp) == FUNCTION_DECL && MEM_P (op0)))
 	{
 	  gcc_assert (MEM_P (op0));
 	  op0 = adjust_address_nv (op0, mode, 0);
@@ -2427,7 +2430,9 @@ expand_debug_expr (tree exp)
 	  op0 = simplify_gen_subreg (mode, op0, inner_mode,
 				     subreg_lowpart_offset (mode,
 							    inner_mode));
-	else if (TYPE_UNSIGNED (TREE_TYPE (TREE_OPERAND (exp, 0))))
+	else if (TREE_CODE_CLASS (TREE_CODE (exp)) == tcc_unary
+		 ? TYPE_UNSIGNED (TREE_TYPE (TREE_OPERAND (exp, 0)))
+		 : unsignedp)
 	  op0 = gen_rtx_ZERO_EXTEND (mode, op0);
 	else
 	  op0 = gen_rtx_SIGN_EXTEND (mode, op0);

	Jakub

Re: [PATCH] Unbreak ia64 bootstrap (PR debug/45006)

[Richard Henderson]

On 07/20/2010 09:36 AM, Jakub Jelinek wrote:
> My PR45003 fix apparently broke IA-64 bootstrap, because there
> FUNCTION_DECLs have DECL_MODE DImode, while TYPE_MODE of FUNCTION_TYPE
> is TImode.  Guess that's related to fn descriptors.

How do the two modes get installed?  Because, really, these two should
match.  That this isn't the case doesn't make sense in the type model.

> Either of the hunks should fix the ICE, though without the first hunk
> we just won't be able to emit useful debug info (as ADDR_EXPR <FUNCTION_DECL>
> will return NULL on ia64 then), and without the second hunk we risk we hit
> similar ICE later on.

&function on ia64 may be resolved by ld.so.  I don't know if there's
anything particularly useful we could install here in the debug section.

> @@ -2427,7 +2430,9 @@ expand_debug_expr (tree exp)
>  	  op0 = simplify_gen_subreg (mode, op0, inner_mode,
>  				     subreg_lowpart_offset (mode,
>  							    inner_mode));
> -	else if (TYPE_UNSIGNED (TREE_TYPE (TREE_OPERAND (exp, 0))))
> +	else if (TREE_CODE_CLASS (TREE_CODE (exp)) == tcc_unary
> +		 ? TYPE_UNSIGNED (TREE_TYPE (TREE_OPERAND (exp, 0)))
> +		 : unsignedp)
>  	  op0 = gen_rtx_ZERO_EXTEND (mode, op0);
>  	else
>  	  op0 = gen_rtx_SIGN_EXTEND (mode, op0);

This bit certainly is ok.  We obviously don't want to look through
TREE_OPERAND when it isn't present.


r~

Re: [PATCH] Unbreak ia64 bootstrap (PR debug/45006)

[Jakub Jelinek]

On Tue, Jul 20, 2010 at 10:55:31AM -0700, Richard Henderson wrote:
> On 07/20/2010 09:36 AM, Jakub Jelinek wrote:
> > My PR45003 fix apparently broke IA-64 bootstrap, because there
> > FUNCTION_DECLs have DECL_MODE DImode, while TYPE_MODE of FUNCTION_TYPE
> > is TImode.  Guess that's related to fn descriptors.
> 
> How do the two modes get installed?  Because, really, these two should
> match.  That this isn't the case doesn't make sense in the type model.

TYPE_MODE in layout_type:
  SET_TYPE_MODE (type, mode_for_size (FUNCTION_BOUNDARY, MODE_INT, 0));
and DECL_MODE in make_node_stat:
		  if (code == FUNCTION_DECL)
		    {
		      DECL_ALIGN (t) = FUNCTION_BOUNDARY;
		      DECL_MODE (t) = FUNCTION_MODE;
		    }

> > Either of the hunks should fix the ICE, though without the first hunk
> > we just won't be able to emit useful debug info (as ADDR_EXPR <FUNCTION_DECL>
> > will return NULL on ia64 then), and without the second hunk we risk we hit
> > similar ICE later on.
> 
> &function on ia64 may be resolved by ld.so.  I don't know if there's
> anything particularly useful we could install here in the debug section.

With the patch it emits
.LLST8:
        data8.ua        .LVL18-.Ltext0  // Location list begin address (*.LLST8)
        data8.ua        .LVL24-.Ltext0  // Location list end address (*.LLST8)
        data2.ua        0xa     // Location expression size
        data1   0x3     // DW_OP_addr
        data8.ua        emutls_init#
        data1   0x9f    // DW_OP_stack_value
        data8.ua        0       // Location list terminator begin (*.LLST8)
        data8.ua        0       // Location list terminator end (*.LLST8)

(for __func parameter of __gthread_once inline).  If you want, I can leave
that first hunk of the patch out, so it will provide anything on ia64 again.

Or should we return NULL for ADDR_EXPR of FUNCTION_DECL in expand_debug_expr
on fndesc targets right away?  Not sure if we have any macro for them
though...

> 
> > @@ -2427,7 +2430,9 @@ expand_debug_expr (tree exp)
> >  	  op0 = simplify_gen_subreg (mode, op0, inner_mode,
> >  				     subreg_lowpart_offset (mode,
> >  							    inner_mode));
> > -	else if (TYPE_UNSIGNED (TREE_TYPE (TREE_OPERAND (exp, 0))))
> > +	else if (TREE_CODE_CLASS (TREE_CODE (exp)) == tcc_unary
> > +		 ? TYPE_UNSIGNED (TREE_TYPE (TREE_OPERAND (exp, 0)))
> > +		 : unsignedp)
> >  	  op0 = gen_rtx_ZERO_EXTEND (mode, op0);
> >  	else
> >  	  op0 = gen_rtx_SIGN_EXTEND (mode, op0);
> 
> This bit certainly is ok.  We obviously don't want to look through
> TREE_OPERAND when it isn't present.

Ok, thanks.

	Jakub

Re: [PATCH] Unbreak ia64 bootstrap (PR debug/45006)

[Richard Henderson]

On 07/20/2010 11:35 AM, Jakub Jelinek wrote:
> On Tue, Jul 20, 2010 at 10:55:31AM -0700, Richard Henderson wrote:
>> On 07/20/2010 09:36 AM, Jakub Jelinek wrote:
>>> My PR45003 fix apparently broke IA-64 bootstrap, because there
>>> FUNCTION_DECLs have DECL_MODE DImode, while TYPE_MODE of FUNCTION_TYPE
>>> is TImode.  Guess that's related to fn descriptors.
>>
>> How do the two modes get installed?  Because, really, these two should
>> match.  That this isn't the case doesn't make sense in the type model.
> 
> TYPE_MODE in layout_type:
>   SET_TYPE_MODE (type, mode_for_size (FUNCTION_BOUNDARY, MODE_INT, 0));

I think this usage of FUNCTION_BOUNDARY is wrong.  More below.

> and DECL_MODE in make_node_stat:
> 		  if (code == FUNCTION_DECL)
> 		    {
> 		      DECL_ALIGN (t) = FUNCTION_BOUNDARY;
> 		      DECL_MODE (t) = FUNCTION_MODE;
> 		    }
> 
>>> Either of the hunks should fix the ICE, though without the first hunk
>>> we just won't be able to emit useful debug info (as ADDR_EXPR <FUNCTION_DECL>
>>> will return NULL on ia64 then), and without the second hunk we risk we hit
>>> similar ICE later on.
>>
>> &function on ia64 may be resolved by ld.so.  I don't know if there's
>> anything particularly useful we could install here in the debug section.
> 
> With the patch it emits
> .LLST8:
>         data8.ua        .LVL18-.Ltext0  // Location list begin address (*.LLST8)
>         data8.ua        .LVL24-.Ltext0  // Location list end address (*.LLST8)
>         data2.ua        0xa     // Location expression size
>         data1   0x3     // DW_OP_addr
>         data8.ua        emutls_init#
>         data1   0x9f    // DW_OP_stack_value
>         data8.ua        0       // Location list terminator begin (*.LLST8)
>         data8.ua        0       // Location list terminator end (*.LLST8)
> 
> (for __func parameter of __gthread_once inline).  If you want, I can leave
> that first hunk of the patch out, so it will provide anything on ia64 again.
> 
> Or should we return NULL for ADDR_EXPR of FUNCTION_DECL in expand_debug_expr
> on fndesc targets right away?  Not sure if we have any macro for them
> though...

Hmm.  I tried to see what ppc64 would generate here, but couldn't seem to
prod the test case into stuffing the constant into the debug info.

I will say that this debug info quoted above is wrong, because the variable
does not contain a code address.

As for the MODEs, there's definitely some confusion here.

I see two possible solutions: (1) use a target macro/hook that indicates that
the target is using function descriptors, or (2) strictly disassociate the
FUNCTION_MODE and the FUNCTION_BOUNDARY, and if they don't match then infer
that a function descriptor must be in use.  The hook may be easier to manage,
and gives us a third piece of data against which we could detect inconsistencies.

With either solution we would need to audit all uses of FUNCTION_BOUNDARY and
DECL_ALIGN (function_decl) in generic code.

I'll also point out that rs6000 FUNCTION_MODE could be considered incorrect,
since it's hard-coded to SImode despite the Pmode function descriptor in use
for certain sub-targets.


r~

cris-elf still broken with "[PATCH] Unbreak ia64 bootstrap (PR debug/45006)"

[Hans-Peter Nilsson]

On Tue, 20 Jul 2010, Jakub Jelinek wrote:
> Hi!
>
> My PR45003 fix apparently broke IA-64 bootstrap, because there
> FUNCTION_DECLs have DECL_MODE DImode, while TYPE_MODE of FUNCTION_TYPE
> is TImode.  Guess that's related to fn descriptors.

Probably not; I saw the same for my cris-elf autotester.  No fn
descriptors there.

> The following patch is an attempt to fix that.

But doesn't fix all introduced problems (read: r162348 doesn't
work), see PR45009.

>  For FUNCTION_DECLs,
> I don't see how SIGN_EXTEND/ZERO_EXTEND on the MEM would be ever useful,
> FUNCTION_DECLs ought to appear in DEBUG stmts only inside of ADDR_EXPR
> where the MEM is not used anyway and we just use the address.

It might not matter in the context of a FUNCTION_DECL, but
anyway FWIW, sign-extend and zero-extend on mem are valid insns
(by themselves or as operands for add or sub) for cris/crisv32.

> 	PR debug/45006
> 	* cfgexpand.c (expand_debug_expr): Don't go through adjust_mode
> 	path for FUNCTION_DECLs.  Only look at TYPE_UNSIGNED of
> 	operand's type if exp is tcc_unary class tree.

brgds, H-P

Re: [PATCH] Unbreak ia64 bootstrap (PR debug/45006)

[Jakub Jelinek]

On Tue, Jul 20, 2010 at 01:11:00PM -0700, Richard Henderson wrote:
> Hmm.  I tried to see what ppc64 would generate here, but couldn't seem to
> prod the test case into stuffing the constant into the debug info.

static void foo (void) __attribute__((used));
static void foo (void)
{
}

void bar (void)
{
  void (*p) () = foo;
}

has it for ppc64 (for ia64 not right now, as DECL_MODE != TYPE_MODE in
expand_debug_expr prevents it).  ppc64 produces at -O2 -g -dA -m64:
	.byte	0x4	 # uleb128 0x4; (DIE (0x9b) DW_TAG_variable)
	.ascii "p\0"	 # DW_AT_name
	.byte	0x1	 # DW_AT_decl_file (tt.c)
	.byte	0x8	 # DW_AT_decl_line
	.4byte	0xb7	 # DW_AT_type
	.byte	0xa	 # DW_AT_location
	.byte	0x3	 # DW_OP_addr
	.8byte	foo
	.byte	0x9f	 # DW_OP_stack_value
which is I think correct, as unlike ia64 on ppc64 foo here is the function
descriptor symbol in .opd.

	Jakub