* [PATCH] Unbreak ia64 bootstrap (PR debug/45006)
@ 2010-07-20 16:34 Jakub Jelinek
2010-07-20 17:55 ` Richard Henderson
2010-07-20 21:59 ` cris-elf still broken with "[PATCH] Unbreak ia64 bootstrap (PR debug/45006)" Hans-Peter Nilsson
0 siblings, 2 replies; 6+ messages in thread
From: Jakub Jelinek @ 2010-07-20 16:34 UTC (permalink / raw)
To: Richard Guenther; +Cc: gcc-patches
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
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] Unbreak ia64 bootstrap (PR debug/45006)
2010-07-20 16:34 [PATCH] Unbreak ia64 bootstrap (PR debug/45006) Jakub Jelinek
@ 2010-07-20 17:55 ` Richard Henderson
2010-07-20 18:33 ` Jakub Jelinek
2010-07-20 21:59 ` cris-elf still broken with "[PATCH] Unbreak ia64 bootstrap (PR debug/45006)" Hans-Peter Nilsson
1 sibling, 1 reply; 6+ messages in thread
From: Richard Henderson @ 2010-07-20 17:55 UTC (permalink / raw)
To: Jakub Jelinek; +Cc: Richard Guenther, gcc-patches
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~
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] Unbreak ia64 bootstrap (PR debug/45006)
2010-07-20 17:55 ` Richard Henderson
@ 2010-07-20 18:33 ` Jakub Jelinek
2010-07-20 20:11 ` Richard Henderson
0 siblings, 1 reply; 6+ messages in thread
From: Jakub Jelinek @ 2010-07-20 18:33 UTC (permalink / raw)
To: Richard Henderson; +Cc: Richard Guenther, gcc-patches
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
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] Unbreak ia64 bootstrap (PR debug/45006)
2010-07-20 18:33 ` Jakub Jelinek
@ 2010-07-20 20:11 ` Richard Henderson
2010-07-21 9:45 ` Jakub Jelinek
0 siblings, 1 reply; 6+ messages in thread
From: Richard Henderson @ 2010-07-20 20:11 UTC (permalink / raw)
To: Jakub Jelinek; +Cc: Richard Guenther, gcc-patches
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~
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] Unbreak ia64 bootstrap (PR debug/45006)
2010-07-20 20:11 ` Richard Henderson
@ 2010-07-21 9:45 ` Jakub Jelinek
0 siblings, 0 replies; 6+ messages in thread
From: Jakub Jelinek @ 2010-07-21 9:45 UTC (permalink / raw)
To: Richard Henderson; +Cc: Richard Guenther, gcc-patches
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* cris-elf still broken with "[PATCH] Unbreak ia64 bootstrap (PR debug/45006)"
2010-07-20 16:34 [PATCH] Unbreak ia64 bootstrap (PR debug/45006) Jakub Jelinek
2010-07-20 17:55 ` Richard Henderson
@ 2010-07-20 21:59 ` Hans-Peter Nilsson
1 sibling, 0 replies; 6+ messages in thread
From: Hans-Peter Nilsson @ 2010-07-20 21:59 UTC (permalink / raw)
To: Jakub Jelinek; +Cc: gcc-patches
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
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2010-07-21 9:45 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2010-07-20 16:34 [PATCH] Unbreak ia64 bootstrap (PR debug/45006) Jakub Jelinek
2010-07-20 17:55 ` Richard Henderson
2010-07-20 18:33 ` Jakub Jelinek
2010-07-20 20:11 ` Richard Henderson
2010-07-21 9:45 ` Jakub Jelinek
2010-07-20 21:59 ` cris-elf still broken with "[PATCH] Unbreak ia64 bootstrap (PR debug/45006)" Hans-Peter Nilsson
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).