| Message ID | AM8PR10MB47082F75837E0CD6AE97ADBEE4E89@AM8PR10MB4708.EURPRD10.PROD.OUTLOOK.COM |
|---|---|
| State | New |
| Headers | show |
| Series | Fix bogus line info in DECL_IGNORED_P functions | expand |
> It was pointed out in PR101598 to be inappropriate, that > ignored Ada decls receive the source line number which was > recorded in the function decl's DECL_SOURCE_LOCATION. > Therefore set all front-end-generated Ada decls with > DECL_IGNORED_P to UNKNOWN_LOCATION. > > 2021-07-24 Bernd Edlinger <bernd.edlinger@hotmail.de> > > PR debug/101598 > * gcc-interface/trans.c (Subprogram_Body_to_gnu): Set the > DECL_SOURCE_LOCATION of DECL_IGNORED_P gnu_subprog_decl to > UNKNOWN_LOCATION. Is that really needed in DWARF 5? If no, I'm not sure that we want it.
On 8/2/21 3:07 PM, Eric Botcazou wrote: >> It was pointed out in PR101598 to be inappropriate, that >> ignored Ada decls receive the source line number which was >> recorded in the function decl's DECL_SOURCE_LOCATION. >> Therefore set all front-end-generated Ada decls with >> DECL_IGNORED_P to UNKNOWN_LOCATION. >> >> 2021-07-24 Bernd Edlinger <bernd.edlinger@hotmail.de> >> >> PR debug/101598 >> * gcc-interface/trans.c (Subprogram_Body_to_gnu): Set the >> DECL_SOURCE_LOCATION of DECL_IGNORED_P gnu_subprog_decl to >> UNKNOWN_LOCATION. > > Is that really needed in DWARF 5? If no, I'm not sure that we want it. > No, this one is completely optional, only DWARF 4 may have additional issues without part 1/2 of this patch. The location of these ignored Ada decls looks completely sane to me. However, it was an unintentional side effect of the patch to allow minimal debugging of ignored decls. This means we can now step into those functions or set line breakpoints there, while previously that was not possible. And I guess it could be considered an improvement. So it's your choice, how you want these functions to be debugged. Thanks Bernd.
> The location of these ignored Ada decls looks completely sane to me. > However, it was an unintentional side effect of the patch to allow > minimal debugging of ignored decls. This means we can now step into > those functions or set line breakpoints there, while previously that > was not possible. And I guess it could be considered an improvement. > > So it's your choice, how you want these functions to be debugged. The requirement on the GDB side is that these functions *cannot* be stepped into, i.e. that they be completely transparent for the GDB user. But we still want to have location information in the compiler itself to debug it.
On 8/4/21 4:33 PM, Eric Botcazou wrote: >> The location of these ignored Ada decls looks completely sane to me. >> However, it was an unintentional side effect of the patch to allow >> minimal debugging of ignored decls. This means we can now step into >> those functions or set line breakpoints there, while previously that >> was not possible. And I guess it could be considered an improvement. >> >> So it's your choice, how you want these functions to be debugged. > > The requirement on the GDB side is that these functions *cannot* be stepped > into, i.e. that they be completely transparent for the GDB user. But we still > want to have location information in the compiler itself to debug it. > Well, I see. But it is okay that we can set a breakpoint on defs__struct1IP, in the test case of PR 101598. And the debugger shall only show assembler here. Right? Do you have an example where this location information is used in the compiler itself for debugging? Of course we could do something like diff --git a/gcc/dwarf2out.c b/gcc/dwarf2out.c index b91a9b5..c0ff4c6 100644 --- a/gcc/dwarf2out.c +++ b/gcc/dwarf2out.c @@ -28546,6 +28546,9 @@ dwarf2out_set_ignored_loc (unsigned int line, unsigned i { dw_fde_ref fde = cfun->fde; + if (is_ada ()) + return; + fde->ignored_debug = false; set_cur_line_info_table (function_section (fde->decl)); But it would regress the attached test case (the Ada-equivalent of PR 97937): $ gnatmake -O2 main.adb -g -save-temps -f produces line info for Test2: test__test2: .LFB8: .cfi_startproc .loc 1 8 4 view .LVU3 movl %edi, %eax ret .cfi_endproc while with the above patch we would get something like test__test2: .LFB8: .cfi_startproc movl %edi, %eax ret .cfi_endproc and, indeed it is impossible to step into test2 or get the source line if we insert a breakpoint at the label test__test2. I assume You would agree that having the location for Test2 is better than no debug info at all? So Maybe something like the following might work for You? diff --git a/gcc/dwarf2out.c b/gcc/dwarf2out.c index b91a9b5..c0ff4c6 100644 --- a/gcc/dwarf2out.c +++ b/gcc/dwarf2out.c @@ -28546,6 +28546,9 @@ dwarf2out_set_ignored_loc (unsigned int line, unsigned i { dw_fde_ref fde = cfun->fde; + if (is_ada () && DECL_ARTIFICIAL (cfun->decl)) + return; + fde->ignored_debug = false; set_cur_line_info_table (function_section (fde->decl)); This would remove the location info in the test case of PR 101598, and still have location info in the ada variant of PR 97937. What do you think? Thanks Bernd.
> But it is okay that we can set a breakpoint on defs__struct1IP, > in the test case of PR 101598. > And the debugger shall only show assembler here. > Right? The defs__struct1IP procedure should be totally transparent for the user, so setting a breakpoint in it is not supposed to come into play. > Do you have an example where this location information is used in the > compiler itself for debugging? That's useful when you compile the code with -gnatD, i.e when you debug the intermediate code generated by the front-end. > I assume You would agree that having the location for Test2 is better > than no debug info at all? But we want no debug info at all for these IP functions. > What do you think? I guess I still don't understand why DECL_IGNORED_P was changed.
On Mon, 9 Aug 2021, Eric Botcazou wrote: > > But it is okay that we can set a breakpoint on defs__struct1IP, > > in the test case of PR 101598. > > And the debugger shall only show assembler here. > > Right? > > The defs__struct1IP procedure should be totally transparent for the user, so > setting a breakpoint in it is not supposed to come into play. > > > Do you have an example where this location information is used in the > > compiler itself for debugging? > > That's useful when you compile the code with -gnatD, i.e when you debug the > intermediate code generated by the front-end. > > > I assume You would agree that having the location for Test2 is better > > than no debug info at all? > > But we want no debug info at all for these IP functions. > > > What do you think? > > I guess I still don't understand why DECL_IGNORED_P was changed. ISTR it was changed because at least with location info generated by gas there's no way to have "no location" for a portion of code. Instead the assigned location will be that of the previous .loc directive which results in random and confusing results for the pc range of the DECL_INGORED_P function. I guess we should really revisit the decision to rely on gas to produce line info. What's the advantage of doing so (apart from "nice" annotated assembly)? Richard.
On 8/9/21 4:37 PM, Eric Botcazou wrote: >> But it is okay that we can set a breakpoint on defs__struct1IP, >> in the test case of PR 101598. >> And the debugger shall only show assembler here. >> Right? > > The defs__struct1IP procedure should be totally transparent for the user, so > setting a breakpoint in it is not supposed to come into play. > The symbol defs__struct1IP is a global symbol, so once a user know its name, that user can set a breakpoint there, has never been changed, and I don't know how to avoid that. >> Do you have an example where this location information is used in the >> compiler itself for debugging? > > That's useful when you compile the code with -gnatD, i.e when you debug the > intermediate code generated by the front-end. > Ah, thanks, I tried it but the defs__struct1IP function has DECL_IGNORED_P = false when I build it with -gnatD. So that would not be affected by setting DECL_SOURCE_LOCATION to UNKNOWN_LOCATION as done in the proposed patch, because that is only done for functions with DECL_IGNORED_P = true. >> I assume You would agree that having the location for Test2 is better >> than no debug info at all? > > But we want no debug info at all for these IP functions. > >> What do you think? > > I guess I still don't understand why DECL_IGNORED_P was changed. > We have several issues here. First we have DECL_IGNORED_P = true functions, where DECL_SOURCE_LOCATION is UNKNOWN_LOCATION, but they have *wrong* line numbers, either because a preceding function's last line number extends to the ignored function, or because of a bug in the assembler which mostly affects -gdwarf-4 with certain versions if the binutils toolchain. Then we have ordinary functions with DECL_IGNORED_P = false, but they are morphed into a thunk calling a similar looking function and the thunk has now DECL_IGNORED_P = true, and all debug info removed, except the DECL_SOURCE_LOCATION. To make this even worse, the similar looking function is inlined into the thunk again, and all debug info is again stripped away. And when this happens it is desirable to at least see the source line where the original function was declared. As an example, please consider this test case: $ cat test.ads package test is type Func_Ptr is access function (X : Integer) return Integer; function Test1 (X : Integer) return Integer; function Test2 (X : Integer) return Integer; function DoIt (X : Integer; Func : Func_Ptr) return Integer; end test; $ cat test.ads package test is type Func_Ptr is access function (X : Integer) return Integer; function Test1 (X : Integer) return Integer; function Test2 (X : Integer) return Integer; function DoIt (X : Integer; Func : Func_Ptr) return Integer; end test; $ cat test.adb package body test is function Test1 (X : Integer) return Integer is begin return X; end Test1; function Test2 (X : Integer) return Integer is begin return X; end Test2; function DoIt (X : Integer; Func : Func_Ptr) return Integer is begin return Func (X); end DoIt; end test; $ cat main.adb with Ada.Text_IO; use Ada.Text_IO; with test; use test; procedure Main is X : Integer := 7; Y : Integer := DoIt (X, Test1'Access); Z : Integer := DoIt (X, Test2'Access); begin Put_Line (X'Img & " " & Y'Img & " " & Z'Img); end Main; $ gnatmake -f -g -O2 main.adb -save-temp -fdump-tree-all-lineno Now Test1 and Test2 are ordinary functions, but Test2 ends up as DECL_IGNORED_P = true, that happens between test.adb.052t.local-fnsummary2 and test.adb.092t.fixup_cfg3: in test.adb.052t.local-fnsummary2 we have: integer test.test1 (integer x) { <bb 2> [local count: 1073741824]: [test.adb:5:7] return x_1(D); } and integer test.test2 (integer x) { <bb 2> [local count: 1073741824]: [test.adb:10:7] return x_1(D); } but in test.adb.092t.fixup_cfg3 we have integer test.test1 (integer x) { <bb 2> [local count: 1073741824]: [test.adb:5:7] return x_1(D); } and integer test.test2 (integer x) { integer D.4674; integer retval.5; integer _4; <bb 2> [local count: 1073741824]: # DEBUG x => x_1(D) _4 = x_1(D); # DEBUG x => NULL retval.5_2 = _4; return retval.5_2; } the line numbers are gone, and the function has DECL_IGNORED_P, but still a useful DECL_SOURCE_LOCATION, I don't know where the DECL_SOURCE_LOCATION can be seen in the dump files, but from debugging this effect, I know that quite well. This second effect is why as a special case DECL_IGNORED_P functions with valid looking DECL_SOURCE_LOCATION have now a .loc statement, because that is less surprising to a user than having no line numbers at all here. Thanks Bernd.
> ISTR it was changed because at least with location info generated > by gas there's no way to have "no location" for a portion of code. > Instead the assigned location will be that of the previous .loc > directive which results in random and confusing results for the > pc range of the DECL_INGORED_P function. Yes in the general case, but no if you put them at the beginning of the assembly file (what the Ada compiler precisely does), at least if you do not pass any -gdwarf-n switch now. This had worked for 2 decades at least... > I guess we should really revisit the decision to rely on gas > to produce line info. What's the advantage of doing so (apart > from "nice" annotated assembly)? Not a small advantage in my opinion, and I don't think that we want to change it because of a corner case in Ada in any case. I guess Bernd's patch is acceptable, modulo a small tweak for -gnatD. Let me experiment a little bit though.
> Ah, thanks, I tried it but the defs__struct1IP function has > DECL_IGNORED_P = false when I build it with -gnatD. > > So that would not be affected by setting DECL_SOURCE_LOCATION to > UNKNOWN_LOCATION as done in the proposed patch, because that is only > done for functions with DECL_IGNORED_P = true. Ouch, I should have checked it myself... Thanks for doing the investigation. > Then we have ordinary functions with DECL_IGNORED_P = false, > but they are morphed into a thunk calling a similar looking function > and the thunk has now DECL_IGNORED_P = true, and all debug info > removed, except the DECL_SOURCE_LOCATION. To make this even worse, > the similar looking function is inlined into the thunk again, and > all debug info is again stripped away. > > And when this happens it is desirable to at least see the source line where > the original function was declared. > > > As an example, please consider this test case: > > $ cat test.ads > package test is > > type Func_Ptr is access function (X : Integer) return Integer; > > function Test1 (X : Integer) return Integer; > function Test2 (X : Integer) return Integer; > function DoIt (X : Integer; Func : Func_Ptr) return Integer; > > end test; > $ cat test.ads > package test is > > type Func_Ptr is access function (X : Integer) return Integer; > > function Test1 (X : Integer) return Integer; > function Test2 (X : Integer) return Integer; > function DoIt (X : Integer; Func : Func_Ptr) return Integer; > > end test; > > $ cat test.adb > package body test is > > function Test1 (X : Integer) return Integer is > begin > return X; > end Test1; > > function Test2 (X : Integer) return Integer is > begin > return X; > end Test2; > > function DoIt (X : Integer; Func : Func_Ptr) return Integer is > begin > return Func (X); > end DoIt; > > end test; > > $ cat main.adb > with Ada.Text_IO; use Ada.Text_IO; > with test; use test; > > procedure Main is > > X : Integer := 7; > Y : Integer := DoIt (X, Test1'Access); > Z : Integer := DoIt (X, Test2'Access); > > begin > Put_Line (X'Img & " " & Y'Img & " " & Z'Img); > end Main; > > $ gnatmake -f -g -O2 main.adb -save-temp -fdump-tree-all-lineno > > Now Test1 and Test2 are ordinary functions, but Test2 ends up as > DECL_IGNORED_P = true, that happens between test.adb.052t.local-fnsummary2 > and test.adb.092t.fixup_cfg3: Ouch (bis). Quite unexpected that DECL_IGNORED_P is set out of nowhere. It's Identical Code Folding which turns Test2 into a thunk? > in test.adb.052t.local-fnsummary2 we have: > > integer test.test1 (integer x) > { > <bb 2> [local count: 1073741824]: > [test.adb:5:7] return x_1(D); > > } > > and > > integer test.test2 (integer x) > { > <bb 2> [local count: 1073741824]: > [test.adb:10:7] return x_1(D); > > } > > > but in test.adb.092t.fixup_cfg3 > > we have > > integer test.test1 (integer x) > { > <bb 2> [local count: 1073741824]: > [test.adb:5:7] return x_1(D); > > } > > and > > integer test.test2 (integer x) > { > integer D.4674; > integer retval.5; > integer _4; > > <bb 2> [local count: 1073741824]: > # DEBUG x => x_1(D) > _4 = x_1(D); > # DEBUG x => NULL > retval.5_2 = _4; > return retval.5_2; > > } > > > the line numbers are gone, and the function has DECL_IGNORED_P, > but still a useful DECL_SOURCE_LOCATION, I don't know > where the DECL_SOURCE_LOCATION can be seen in the dump files, > but from debugging this effect, I know that quite well. > > This second effect is why as a special case DECL_IGNORED_P functions > with valid looking DECL_SOURCE_LOCATION have now a .loc statement, > because that is less surprising to a user than having no line numbers > at all here. OK, thanks for the explanation, the patch is OK then.
On 8/10/21 10:56 PM, Eric Botcazou wrote: >> Ah, thanks, I tried it but the defs__struct1IP function has >> DECL_IGNORED_P = false when I build it with -gnatD. >> >> So that would not be affected by setting DECL_SOURCE_LOCATION to >> UNKNOWN_LOCATION as done in the proposed patch, because that is only >> done for functions with DECL_IGNORED_P = true. > > Ouch, I should have checked it myself... Thanks for doing the investigation. > no problem. >> Then we have ordinary functions with DECL_IGNORED_P = false, >> but they are morphed into a thunk calling a similar looking function >> and the thunk has now DECL_IGNORED_P = true, and all debug info >> removed, except the DECL_SOURCE_LOCATION. To make this even worse, >> the similar looking function is inlined into the thunk again, and >> all debug info is again stripped away. >> >> And when this happens it is desirable to at least see the source line where >> the original function was declared. >> >> >> As an example, please consider this test case: >> >> $ cat test.ads >> package test is >> >> type Func_Ptr is access function (X : Integer) return Integer; >> >> function Test1 (X : Integer) return Integer; >> function Test2 (X : Integer) return Integer; >> function DoIt (X : Integer; Func : Func_Ptr) return Integer; >> >> end test; >> $ cat test.ads >> package test is >> >> type Func_Ptr is access function (X : Integer) return Integer; >> >> function Test1 (X : Integer) return Integer; >> function Test2 (X : Integer) return Integer; >> function DoIt (X : Integer; Func : Func_Ptr) return Integer; >> >> end test; >> >> $ cat test.adb >> package body test is >> >> function Test1 (X : Integer) return Integer is >> begin >> return X; >> end Test1; >> >> function Test2 (X : Integer) return Integer is >> begin >> return X; >> end Test2; >> >> function DoIt (X : Integer; Func : Func_Ptr) return Integer is >> begin >> return Func (X); >> end DoIt; >> >> end test; >> >> $ cat main.adb >> with Ada.Text_IO; use Ada.Text_IO; >> with test; use test; >> >> procedure Main is >> >> X : Integer := 7; >> Y : Integer := DoIt (X, Test1'Access); >> Z : Integer := DoIt (X, Test2'Access); >> >> begin >> Put_Line (X'Img & " " & Y'Img & " " & Z'Img); >> end Main; >> >> $ gnatmake -f -g -O2 main.adb -save-temp -fdump-tree-all-lineno >> >> Now Test1 and Test2 are ordinary functions, but Test2 ends up as >> DECL_IGNORED_P = true, that happens between test.adb.052t.local-fnsummary2 >> and test.adb.092t.fixup_cfg3: > > Ouch (bis). Quite unexpected that DECL_IGNORED_P is set out of nowhere. It's > Identical Code Folding which turns Test2 into a thunk? > Yes, the identical functions trigger this folding. Originally I discovered this when I wanted to debug the arm back-end, where we have several identical functions: static bool arm_align_anon_bitfield (void) { return TARGET_AAPCS_BASED; } [...] static bool arm_cxx_guard_mask_bit (void) { return TARGET_AAPCS_BASED; } the second one had no line numbers, but since it was not at the beginning of the assembly gdb did show a totally unrelated line. In general I can say that debugging optimized code is not a very pleasant experience. But this issue happens only rarely, while another issue is much more dominant. You will probably see it quite often while debugging the gcc middle end that stepping over inline functions does not work right, one often steps over some code, and ends up in the tree_check inline function. That's because of an ambiguity in the dwarf debug info of inline ranges. It does not have the view number where the inline range ends exactly, but only the PC. I have been working on some gdb patches to work around the problem, but they are stuck unfortunately :-( "Improve debugging of optimized code" https://sourceware.org/pipermail/gdb-patches/2021-January/175617.html >> in test.adb.052t.local-fnsummary2 we have: >> >> integer test.test1 (integer x) >> { >> <bb 2> [local count: 1073741824]: >> [test.adb:5:7] return x_1(D); >> >> } >> >> and >> >> integer test.test2 (integer x) >> { >> <bb 2> [local count: 1073741824]: >> [test.adb:10:7] return x_1(D); >> >> } >> >> >> but in test.adb.092t.fixup_cfg3 >> >> we have >> >> integer test.test1 (integer x) >> { >> <bb 2> [local count: 1073741824]: >> [test.adb:5:7] return x_1(D); >> >> } >> >> and >> >> integer test.test2 (integer x) >> { >> integer D.4674; >> integer retval.5; >> integer _4; >> >> <bb 2> [local count: 1073741824]: >> # DEBUG x => x_1(D) >> _4 = x_1(D); >> # DEBUG x => NULL >> retval.5_2 = _4; >> return retval.5_2; >> >> } >> >> >> the line numbers are gone, and the function has DECL_IGNORED_P, >> but still a useful DECL_SOURCE_LOCATION, I don't know >> where the DECL_SOURCE_LOCATION can be seen in the dump files, >> but from debugging this effect, I know that quite well. >> >> This second effect is why as a special case DECL_IGNORED_P functions >> with valid looking DECL_SOURCE_LOCATION have now a .loc statement, >> because that is less surprising to a user than having no line numbers >> at all here. > > OK, thanks for the explanation, the patch is OK then. > Thank You very much, I will push it now. Bernd.
diff --git a/gcc/ada/gcc-interface/trans.c b/gcc/ada/gcc-interface/trans.c index f61183d..3df56aa 100644 --- a/gcc/ada/gcc-interface/trans.c +++ b/gcc/ada/gcc-interface/trans.c @@ -3885,7 +3885,9 @@ Subprogram_Body_to_gnu (Node_Id gnat_node) } /* Set the line number in the decl to correspond to that of the body. */ - if (!Sloc_to_locus (Sloc (gnat_node), &locus, false, gnu_subprog_decl)) + if (DECL_IGNORED_P (gnu_subprog_decl)) + locus = UNKNOWN_LOCATION; + else if (!Sloc_to_locus (Sloc (gnat_node), &locus, false, gnu_subprog_decl)) locus = input_location; DECL_SOURCE_LOCATION (gnu_subprog_decl) = locus;