| Message ID | AS1PR01MB9465CA59822CED782BA56D23E48E2@AS1PR01MB9465.eurprd01.prod.exchangelabs.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v3] RISC-V: Enable -gvariable-location-views by default | expand |
Hello, Bernd, Thanks for the fixes and improvements, your patch looks good to me. I stand behind its approval by someone with authority to do so. I believe location views are somewhat problematic on RISC-V, because of the object code relaxations. Its usefulness is also limited without support from debug consumers. It's unfortunate, in hindsight, that inline entry points ended up tied to location views. I can't recall any strong reason for that. I suppose lifting that requirement, so as to enable inline points even without location views, would be desirable. Would you be interested in pursuing that? On Aug 21, 2024, Bernd Edlinger <bernd.edlinger@hotmail.de> wrote: > gcc/ChangeLog: > * dwarf2out.cc (dwarf2out_maybe_output_loclist_view_pair, > output_loc_list): Correct handling of -gno-as-loc-support, > use ZERO_VIEW_P to output view number as zero value. > * toplev.cc (process_options): Do not automatically disable > -gvariable-location-views when -gno-as-loc-support or > -gno-as-locview-support is used. > gcc/testsuite/ChangeLog: > * gcc.dg/debug/dwarf2/inline2.c: Add checks for inline entry_pc. > * gcc.dg/debug/dwarf2/inline6.c: Add -gno-as-loc-support and check > the resulting location views.
Hello Alexandre, On 8/22/24 05:39, Alexandre Oliva wrote: > Hello, Bernd, > > Thanks for the fixes and improvements, your patch looks good to me. I > stand behind its approval by someone with authority to do so. > > I believe location views are somewhat problematic on RISC-V, because of > the object code relaxations. Its usefulness is also limited without > support from debug consumers. > The main deficiency with the location view line-numbers I see, is that it is possible to have different line numbers on the same PC, which have different view numbers, but when debugging an inlined function, it is sometimes impossible to tell if the line number is from the inline function or from the calling function, especially at the end of an inline code range, that is because the DW_AT_ranges value of the inlined_subroutine has only begin and end PC, but to make it clear the begin and end PC+View number would be necessary. This means it is ambiguous, and that probably stated with the non-is-stmt line numbers. > It's unfortunate, in hindsight, that inline entry points ended up tied > to location views. I can't recall any strong reason for that. I > suppose lifting that requirement, so as to enable inline points even > without location views, would be desirable. Would you be interested in > pursuing that? > Thanks a lot for your support, yes, that sounds reasonable, I can look into breaking this connection. PS: I need to make a small fixup to the test cases, as the linaro folks indicated that my test cases should not include the comment sign in "# DW_AT_entry_pc", as that is target dependent. Thanks Bernd. > On Aug 21, 2024, Bernd Edlinger <bernd.edlinger@hotmail.de> wrote: > >> gcc/ChangeLog: > >> * dwarf2out.cc (dwarf2out_maybe_output_loclist_view_pair, >> output_loc_list): Correct handling of -gno-as-loc-support, >> use ZERO_VIEW_P to output view number as zero value. >> * toplev.cc (process_options): Do not automatically disable >> -gvariable-location-views when -gno-as-loc-support or >> -gno-as-locview-support is used. > >> gcc/testsuite/ChangeLog: > >> * gcc.dg/debug/dwarf2/inline2.c: Add checks for inline entry_pc. >> * gcc.dg/debug/dwarf2/inline6.c: Add -gno-as-loc-support and check >> the resulting location views. >
diff --git a/gcc/dwarf2out.cc b/gcc/dwarf2out.cc index 346feeb53c8..79f97b5a55e 100644 --- a/gcc/dwarf2out.cc +++ b/gcc/dwarf2out.cc @@ -10374,7 +10374,7 @@ dwarf2out_maybe_output_loclist_view_pair (dw_loc_list_ref curr) #ifdef DW_LLE_view_pair dw2_asm_output_data (1, DW_LLE_view_pair, "DW_LLE_view_pair"); - if (dwarf2out_as_locview_support) + if (dwarf2out_as_locview_support && dwarf2out_as_loc_support) { if (ZERO_VIEW_P (curr->vbegin)) dw2_asm_output_data_uleb128 (0, "Location view begin"); @@ -10396,8 +10396,10 @@ dwarf2out_maybe_output_loclist_view_pair (dw_loc_list_ref curr) } else { - dw2_asm_output_data_uleb128 (curr->vbegin, "Location view begin"); - dw2_asm_output_data_uleb128 (curr->vend, "Location view end"); + dw2_asm_output_data_uleb128 (ZERO_VIEW_P (curr->vbegin) + ? 0 : curr->vbegin, "Location view begin"); + dw2_asm_output_data_uleb128 (ZERO_VIEW_P (curr->vend) + ? 0 : curr->vend, "Location view end"); } #endif /* DW_LLE_view_pair */ @@ -10430,7 +10432,7 @@ output_loc_list (dw_loc_list_ref list_head) vcount++; /* ?? dwarf_split_debug_info? */ - if (dwarf2out_as_locview_support) + if (dwarf2out_as_locview_support && dwarf2out_as_loc_support) { char label[MAX_ARTIFICIAL_LABEL_BYTES]; @@ -10460,10 +10462,12 @@ output_loc_list (dw_loc_list_ref list_head) } else { - dw2_asm_output_data_uleb128 (curr->vbegin, + dw2_asm_output_data_uleb128 (ZERO_VIEW_P (curr->vbegin) + ? 0 : curr->vbegin, "View list begin (%s)", list_head->vl_symbol); - dw2_asm_output_data_uleb128 (curr->vend, + dw2_asm_output_data_uleb128 (ZERO_VIEW_P (curr->vend) + ? 0 : curr->vend, "View list end (%s)", list_head->vl_symbol); } diff --git a/gcc/testsuite/gcc.dg/debug/dwarf2/inline2.c b/gcc/testsuite/gcc.dg/debug/dwarf2/inline2.c index 9c36450ae2d..03d1a73c0c4 100644 --- a/gcc/testsuite/gcc.dg/debug/dwarf2/inline2.c +++ b/gcc/testsuite/gcc.dg/debug/dwarf2/inline2.c @@ -30,6 +30,9 @@ layout. */ /* { dg-final { scan-assembler-times "\\(DIE \\(\[^\n\]*\\) DW_TAG_lexical_block" 0 } } */ +/* Each inline instance should have DW_AT_entry_pc and DW_AT_GNU_entry_view. */ +/* { dg-final { scan-assembler-times "# DW_AT_entry_pc" 6 } } */ +/* { dg-final { scan-assembler-times "# DW_AT_GNU_entry_view" 6 } } */ /* There are 3 DW_AT_inline attributes: one per abstract inline instance. The value of the attribute must be 0x3, meaning the function was diff --git a/gcc/testsuite/gcc.dg/debug/dwarf2/inline6.c b/gcc/testsuite/gcc.dg/debug/dwarf2/inline6.c index fde8c27820d..a3b0e6dac18 100644 --- a/gcc/testsuite/gcc.dg/debug/dwarf2/inline6.c +++ b/gcc/testsuite/gcc.dg/debug/dwarf2/inline6.c @@ -16,7 +16,7 @@ /* Explicitly use dwarf-5 which uses DW_FORM_implicit_const. */ /* { dg-do compile } */ -/* { dg-options "-O -g3 -gdwarf-5 -dA -fgnu89-inline" } */ +/* { dg-options "-O -g3 -gdwarf-5 -dA -fgnu89-inline -gno-as-loc-support" } */ /* There are 6 inlined subroutines: - One for each subroutine inlined into main, that's 3. @@ -29,6 +29,11 @@ layout. */ /* { dg-final { scan-assembler-times "\\(DIE \\(\[^\n\]*\\) DW_TAG_lexical_block" 0 } } */ +/* Each inline instance should have DW_AT_entry_pc and DW_AT_GNU_entry_view. */ +/* { dg-final { scan-assembler-times "# DW_AT_entry_pc" 6 } } */ +/* { dg-final { scan-assembler-times "# DW_AT_GNU_entry_view" 6 } } */ +/* { dg-final { scan-assembler-times "uleb128\[^\n\]*LVU\[^\n\]*# View list (begin|end)" 0 } } */ +/* { dg-final { scan-assembler-times "uleb128\[^\n\]*0xffffffff\[^\n\]*# View list (begin|end)" 0 } } */ /* There are 3 DW_AT_inline attributes: one per abstract inline instance. The value of the attribute must be 0x3, meaning the function was diff --git a/gcc/toplev.cc b/gcc/toplev.cc index eee4805b504..292948122de 100644 --- a/gcc/toplev.cc +++ b/gcc/toplev.cc @@ -1475,9 +1475,7 @@ process_options () = (flag_var_tracking && debug_info_level >= DINFO_LEVEL_NORMAL && dwarf_debuginfo_p () - && !dwarf_strict - && dwarf2out_as_loc_support - && dwarf2out_as_locview_support); + && !dwarf_strict); } else if (debug_variable_location_views == -1 && dwarf_version != 5) {
This affects only the RISC-V targets, where the compiler options -gvariable-location-views and consequently also -ginline-points are disabled by default, which is unexpected and disables some useful features of the generated debug info. Due to a bug in the gas assembler the .loc statement is not usable to generate location view debug info. That is detected by configure: configure:31500: checking assembler for dwarf2 debug_view support configure:31509: .../riscv-unknown-elf/bin/as -o conftest.o conftest.s >&5 conftest.s: Assembler messages: conftest.s:5: Error: .uleb128 only supports constant or subtract expressions conftest.s:6: Error: .uleb128 only supports constant or subtract expressions configure:31512: $? = 1 configure: failed program was .file 1 "conftest.s" .loc 1 3 0 view .LVU1 nop .data .uleb128 .LVU1 .uleb128 .LVU1 configure:31523: result: no This results in dwarf2out_as_locview_support being set to false, and that creates a sequence of events, with the end result that most inlined functions either have no DW_AT_entry_pc, or one with a wrong entry pc value. But the location views can also be generated without using any .loc statements, therefore we should enable the option -gvariable-location-views by default, regardless of the status of -gas-locview-support. Note however, that the combination of the following compiler options -g -O2 -gvariable-location-views -gno-as-loc-support turned out to create invalid assembler intermediate files, with lots of assembler errors like: Error: leb128 operand is an undefined symbol: .LVU3 This affects all targets, except RISC-V of course ;-) and is fixed by the changes in dwarf2out.cc Finally the .debug_loclists created without assembler locview support did contain location view pairs like v0000000ffffffff v000000000000000 which is the value from FORCE_RESET_NEXT_VIEW, but that is most likely not as expected either, so change that as well. gcc/ChangeLog: * dwarf2out.cc (dwarf2out_maybe_output_loclist_view_pair, output_loc_list): Correct handling of -gno-as-loc-support, use ZERO_VIEW_P to output view number as zero value. * toplev.cc (process_options): Do not automatically disable -gvariable-location-views when -gno-as-loc-support or -gno-as-locview-support is used. gcc/testsuite/ChangeLog: * gcc.dg/debug/dwarf2/inline2.c: Add checks for inline entry_pc. * gcc.dg/debug/dwarf2/inline6.c: Add -gno-as-loc-support and check the resulting location views. --- gcc/dwarf2out.cc | 16 ++++++++++------ gcc/testsuite/gcc.dg/debug/dwarf2/inline2.c | 3 +++ gcc/testsuite/gcc.dg/debug/dwarf2/inline6.c | 7 ++++++- gcc/toplev.cc | 4 +--- 4 files changed, 20 insertions(+), 10 deletions(-) v2: fixed the boot-strap error triggered by v1 on any target, except RISC-V, the issue was triggered by libstdc++-v3/src/c++11/cxx11-ios_failure-lt.s which is generated using -gno-as-loc-support, which triggered a latent issue. v3: added some tests for the fixed bugs. Regression-tested on x86_64-pc-linux-gnu, riscv-unknown-elf and riscv64-unknown-elf, OK for trunk?