| Message ID | alpine.LSU.2.20.1803221411440.18265@zhemvz.fhfr.qr |
|---|---|
| State | New |
| Headers | show |
| Series | Fix PR85020 | expand |
On Thu, Mar 22, 2018 at 03:16:22PM +0100, Richard Biener wrote: > the upper bound decl is global as well). Jakub, do you remember > a reason for having any constraints here? I've reworked the I don't, but it has been added in the https://gcc.gnu.org/ml/gcc-patches/2017-02/msg01499.html patch. It matches what we were using before: - if (loc == NULL - && early_dwarf - && current_function_decl - && DECL_CONTEXT (rszdecl) == current_function_decl) when emitting the DW_OP_call4 stuff. But more importantly, it seems that resolve_variable_value_in_expr will not change anything if tree decl = loc->dw_loc_oprnd1.v.val_decl_ref; if (DECL_CONTEXT (decl) != current_function_decl) continue; Which means resolve_variable_values would never process those DW_OP_GNU_variable_value you've added, and I believe later on note_variable_value_in_expr will just ICE on it: if (loc->dw_loc_opc == DW_OP_GNU_variable_value && loc->dw_loc_oprnd1.val_class == dw_val_class_decl_ref) ... if (! ref && (flag_generate_lto || flag_generate_offload)) { /* ??? This is somewhat a hack because we do not create DIEs for variables not in BLOCK trees early but when generating early LTO output we need the dw_val_class_decl_ref to be fully resolved. For fat LTO objects we'd also like to undo this after LTO dwarf output. */ gcc_assert (DECL_CONTEXT (decl)); Because DECL_CONTEXT (decl) is NULL, right? Or is DECL_CONTEXT never NULL in LTO, just TRANSLATION_UNIT_DECL? If so, perhaps note_variable_value_in_expr will handle those fine, but for these we really don't want to force any DIEs, but rather just give up if lookup_decl fails by the time we've processed the whole TU. Jakub
> This should then trigger the use of DW_OP_GNU_variable_value but > for the Ada ACATS testcase I was looking at to debug the issue > (c330001, resp. c330001_0-c330001_1.ads) this doesn't work > because the constraints are not met (we're in global context and > the upper bound decl is global as well). Jakub, do you remember > a reason for having any constraints here? I've reworked the > constraints to allow the globals (eventually using decl_function_context > on this may also allow other not-quite-local cases? But it already > allowed local statics). Maybe we can simply drop the restriction > alltogether? Thanks for looking into this. > Eric, can you generate a gnat.dg LTO testcase from (reduced) c330001? Attached. * gnat.dg/lto22.adb: New test. * gnat.dg/lto22_pkg1.ad[sb]: New helper. * gnat.dg/lto22_pkg2.ads: Likewise.
On Thu, 22 Mar 2018, Jakub Jelinek wrote: > On Thu, Mar 22, 2018 at 03:16:22PM +0100, Richard Biener wrote: > > the upper bound decl is global as well). Jakub, do you remember > > a reason for having any constraints here? I've reworked the > > I don't, but it has been added in the > https://gcc.gnu.org/ml/gcc-patches/2017-02/msg01499.html > patch. It matches what we were using before: > - if (loc == NULL > - && early_dwarf > - && current_function_decl > - && DECL_CONTEXT (rszdecl) == current_function_decl) > when emitting the DW_OP_call4 stuff. > > But more importantly, it seems that resolve_variable_value_in_expr > will not change anything if > tree decl = loc->dw_loc_oprnd1.v.val_decl_ref; > if (DECL_CONTEXT (decl) != current_function_decl) > continue; > Which means resolve_variable_values would never process those > DW_OP_GNU_variable_value you've added, Ah, ok. I guess this check can be simply removed. > and I believe later on > note_variable_value_in_expr will just ICE on it: > if (loc->dw_loc_opc == DW_OP_GNU_variable_value > && loc->dw_loc_oprnd1.val_class == dw_val_class_decl_ref) > ... > if (! ref && (flag_generate_lto || flag_generate_offload)) > { > /* ??? This is somewhat a hack because we do not create DIEs > for variables not in BLOCK trees early but when generating > early LTO output we need the dw_val_class_decl_ref to be > fully resolved. For fat LTO objects we'd also like to > undo this after LTO dwarf output. */ > gcc_assert (DECL_CONTEXT (decl)); > Because DECL_CONTEXT (decl) is NULL, right? It's TRANSLATION_UNIT_DECL for the Ada case (we do have FEs where decls may have NULL context which then means file-scope). > Or is DECL_CONTEXT never NULL in LTO, just TRANSLATION_UNIT_DECL? > If so, perhaps note_variable_value_in_expr will handle those fine, > but for these we really don't want to force any DIEs, but rather just give > up if lookup_decl fails by the time we've processed the whole TU. Yeah, I guess the whole ! ref case should go and instead drop the location expression. Is there some convenient "NULL" OP we can use for <optimized out> or do we really have to prune the attribute refering to it? As the comment says for the FAT part we'd like to keep the decl and defer for later fixup so if there's a <optimized out> way to emit DWARF for a val_decl_ref we should maybe simply fix it up at emission time? I see there's DW_OP_nop but I guess a nop in random places of a DWARF location expression isn't a good way to say <optimized out> for that part. That said, the second hunk of my patch fixes the bootstrap issue with Ada (will double-check), the first hunk is only to try preserving some debug info for the Ada case where the variable parts are not necessarily function-local. Let's delay that for now. I suppose the 2nd hunk is ok. For reference that was Index: gcc/dwarf2out.c =================================================================== --- gcc/dwarf2out.c (revision 258684) +++ gcc/dwarf2out.c (working copy) @@ -19878,6 +19878,7 @@ rtl_for_decl_location (tree decl) in the current CU, resolve_addr will remove the expression referencing it. */ if (rtl == NULL_RTX + && !(early_dwarf && (flag_generate_lto || flag_generate_offload)) && VAR_P (decl) && !DECL_EXTERNAL (decl) && TREE_STATIC (decl) Richard.
On Fri, Mar 23, 2018 at 10:58:15AM +0100, Richard Biener wrote: > On Thu, 22 Mar 2018, Jakub Jelinek wrote: > > > On Thu, Mar 22, 2018 at 03:16:22PM +0100, Richard Biener wrote: > > > the upper bound decl is global as well). Jakub, do you remember > > > a reason for having any constraints here? I've reworked the > > > > I don't, but it has been added in the > > https://gcc.gnu.org/ml/gcc-patches/2017-02/msg01499.html > > patch. It matches what we were using before: > > - if (loc == NULL > > - && early_dwarf > > - && current_function_decl > > - && DECL_CONTEXT (rszdecl) == current_function_decl) > > when emitting the DW_OP_call4 stuff. > > > > But more importantly, it seems that resolve_variable_value_in_expr > > will not change anything if > > tree decl = loc->dw_loc_oprnd1.v.val_decl_ref; > > if (DECL_CONTEXT (decl) != current_function_decl) > > continue; > > Which means resolve_variable_values would never process those > > DW_OP_GNU_variable_value you've added, > > Ah, ok. I guess this check can be simply removed. Actually it should be kept. resolve_variable_values works on expressions that were added to the hash table by the note_variable_value_in_expr function, and there a hash table entry for each containing function and we want to process it only when doing resolve_variable_values for the particular function e.g. if some expression happens to be referenced from multiple hash tables. > > and I believe later on > > note_variable_value_in_expr will just ICE on it: > > if (loc->dw_loc_opc == DW_OP_GNU_variable_value > > && loc->dw_loc_oprnd1.val_class == dw_val_class_decl_ref) > > ... > > if (! ref && (flag_generate_lto || flag_generate_offload)) > > { > > /* ??? This is somewhat a hack because we do not create DIEs > > for variables not in BLOCK trees early but when generating > > early LTO output we need the dw_val_class_decl_ref to be > > fully resolved. For fat LTO objects we'd also like to > > undo this after LTO dwarf output. */ > > gcc_assert (DECL_CONTEXT (decl)); > > Because DECL_CONTEXT (decl) is NULL, right? > > It's TRANSLATION_UNIT_DECL for the Ada case (we do have FEs where > decls may have NULL context which then means file-scope). Anyway, the above code in note_variable_value_in_expr for -flto will ensure nothing is ever added into variable_value_hash and thus resolve_variable_values isn't reall invoked in that case. > Yeah, I guess the whole ! ref case should go and instead drop the > location expression. Is there some convenient "NULL" OP we can There is none, except newly DW_OP_GNU_variable_value referencing a DIE (say DW_TAG_dwarf_procedure with no DW_AT_location/DW_AT_const_value) - that is something that never has a usable value, so it is always optimized out. That said, resolve_addr_in_expr should handle the removal of expressions containing DW_OP_GNU_variable_value with dw_val_class_decl_ref if lookup_decl_die fails. > I suppose the 2nd hunk is ok. For reference that was > > Index: gcc/dwarf2out.c > =================================================================== > --- gcc/dwarf2out.c (revision 258684) > +++ gcc/dwarf2out.c (working copy) > @@ -19878,6 +19878,7 @@ rtl_for_decl_location (tree decl) > in the current CU, resolve_addr will remove the expression > referencing > it. */ > if (rtl == NULL_RTX > + && !(early_dwarf && (flag_generate_lto || flag_generate_offload)) > && VAR_P (decl) > && !DECL_EXTERNAL (decl) > && TREE_STATIC (decl) Sure, this is obviously ok. Jakub
On Fri, 23 Mar 2018, Eric Botcazou wrote: > > This should then trigger the use of DW_OP_GNU_variable_value but > > for the Ada ACATS testcase I was looking at to debug the issue > > (c330001, resp. c330001_0-c330001_1.ads) this doesn't work > > because the constraints are not met (we're in global context and > > the upper bound decl is global as well). Jakub, do you remember > > a reason for having any constraints here? I've reworked the > > constraints to allow the globals (eventually using decl_function_context > > on this may also allow other not-quite-local cases? But it already > > allowed local statics). Maybe we can simply drop the restriction > > alltogether? > > Thanks for looking into this. > > > Eric, can you generate a gnat.dg LTO testcase from (reduced) c330001? > > Attached. > > > * gnat.dg/lto22.adb: New test. > * gnat.dg/lto22_pkg1.ad[sb]: New helper. > * gnat.dg/lto22_pkg2.ads: Likewise. Thanks, committed. Richard.
Index: gcc/dwarf2out.c =================================================================== --- gcc/dwarf2out.c (revision 258720) +++ gcc/dwarf2out.c (working copy) @@ -18204,14 +18204,15 @@ loc_list_from_tree_1 (tree loc, int want rtl = rtl_for_decl_location (loc); if (rtl == NULL_RTX) { + tree fn_ctx; if (TREE_CODE (loc) != FUNCTION_DECL && early_dwarf - && current_function_decl && want_address != 1 && ! DECL_IGNORED_P (loc) && (INTEGRAL_TYPE_P (TREE_TYPE (loc)) || POINTER_TYPE_P (TREE_TYPE (loc))) - && DECL_CONTEXT (loc) == current_function_decl + && (!(fn_ctx = decl_function_context (loc)) + || fn_ctx == current_function_decl) && (GET_MODE_SIZE (SCALAR_INT_TYPE_MODE (TREE_TYPE (loc))) <= DWARF2_ADDR_SIZE)) { @@ -19878,6 +19879,7 @@ rtl_for_decl_location (tree decl) in the current CU, resolve_addr will remove the expression referencing it. */ if (rtl == NULL_RTX + && !(early_dwarf && (flag_generate_lto || flag_generate_offload)) && VAR_P (decl) && !DECL_EXTERNAL (decl) && TREE_STATIC (decl)
The following fixes profiledbootstrap with LTO for Ada where we end up with references to later optimized out decls in the early debug parts. The fix is to avoid running into /* Try harder to get a rtl. If this symbol ends up not being emitted in the current CU, resolve_addr will remove the expression referencing it. */ if (rtl == NULL_RTX ... rtl = make_decl_rtl_for_debug (decl); in rtl_for_decl_location as the comment already says the symbol can end up not being emitted which is fatal for early debug. For now this is conditionalized on LTO emission. This should then trigger the use of DW_OP_GNU_variable_value but for the Ada ACATS testcase I was looking at to debug the issue (c330001, resp. c330001_0-c330001_1.ads) this doesn't work because the constraints are not met (we're in global context and the upper bound decl is global as well). Jakub, do you remember a reason for having any constraints here? I've reworked the constraints to allow the globals (eventually using decl_function_context on this may also allow other not-quite-local cases? But it already allowed local statics). Maybe we can simply drop the restriction alltogether? Bootstrap and regtest running on x86_64-unknown-linux-gnu, profiledbootstrap with LTO on x86_64-unknown-linux-gnu. Eric, can you generate a gnat.dg LTO testcase from (reduced) c330001? OK for trunk? Thanks, Richard. 2018-03-22 Richard Biener <rguenther@suse.de> PR debug/85020 * dwarf2out.c (loc_list_from_tree_1): Relax restriction on what decls we refer to via DW_OP_GNU_variable_value, respective allow globals from global context. (rtl_for_decl_location): Do not generate RTL early when we are going to emit early debug for LTO.