| Message ID | 20100720163614.GF19172@tyan-ft48-01.lab.bos.redhat.com |
|---|---|
| State | New |
| Headers | show |
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~
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
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~
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
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
--- 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);