[PATCH] Add line debug info for virtual thunks (PR ipa/97937)

[PATCH] Add line debug info for virtual thunks (PR ipa/97937)

[Bernd Edlinger]

[-- Attachment #1: Type: text/plain, Size: 574 bytes --]

Hi,


currently there is a problem when debugging a virtual thunk.  That is
a decl with DECL_IGNORED_P.  Currently the line information displayed
in gdb is completely bogus, thus the last line of whatever function
is immediately before the PC of the thunk.

This patch improves the debug experience at least a bit by emitting
at the line number information where the thunk has been defined.
I do not dare to touch anything but dwarf2 debug info, therefore
the patch is a bit awkward.


Bootstrapped and reg-tested on x86_64-pc-linux-gnu.
Is it OK for trunk?


Thanks
Bernd.

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Add-line-debug-info-for-virtual-thunks.patch --]
[-- Type: text/x-patch; name="0001-Add-line-debug-info-for-virtual-thunks.patch", Size: 2156 bytes --]

From 0a44bb870e90623689cae484f8a8899706480876 Mon Sep 17 00:00:00 2001
From: Bernd Edlinger <bernd.edlinger@hotmail.de>
Date: Sun, 3 Jan 2021 11:18:39 +0100
Subject: [PATCH] Add line debug info for virtual thunks

There is no full debug info since the DECL_IGNORED_P
flag is set on virtual thunks.
But instead of no line info at all, emit at least
the location of the function decl.

2021-01-03  Bernd Edlinger  <bernd.edlinger@hotmail.de>

	PR ipa/97937
	* final.c (final_start_function_1): Always emit function start line
	information for dwarf2 debug.
	(final_end_function): Always call end_function for dwarf2 debug.
	* varasm.c (assemble_start_function): Always call begin_function
	for dwarf2 debug.
---
 gcc/final.c  | 9 +++++++--
 gcc/varasm.c | 2 +-
 2 files changed, 8 insertions(+), 3 deletions(-)

diff --git a/gcc/final.c b/gcc/final.c
index fc9a05e..5a274c1 100644
--- a/gcc/final.c
+++ b/gcc/final.c
@@ -1735,7 +1735,12 @@ final_start_function_1 (rtx_insn **firstp, FILE *file, int *seen,
 				 last_filename);
 
   if (!dwarf2_debug_info_emitted_p (current_function_decl))
-    dwarf2out_begin_prologue (0, 0, NULL);
+    {
+      if (write_symbols == DWARF2_DEBUG)
+	dwarf2out_begin_prologue (last_linenum, last_columnnum, last_filename);
+      else
+	dwarf2out_begin_prologue (0, 0, NULL);
+    }
 
 #ifdef LEAF_REG_REMAP
   if (crtl->uses_only_leaf_regs)
@@ -1879,7 +1884,7 @@ final_end_function (void)
 {
   app_disable ();
 
-  if (!DECL_IGNORED_P (current_function_decl))
+  if (!DECL_IGNORED_P (current_function_decl) || write_symbols == DWARF2_DEBUG)
     debug_hooks->end_function (high_function_linenum);
 
   /* Finally, output the function epilogue:
diff --git a/gcc/varasm.c b/gcc/varasm.c
index ce5d449..513922d 100644
--- a/gcc/varasm.c
+++ b/gcc/varasm.c
@@ -1930,7 +1930,7 @@ assemble_start_function (tree decl, const char *fnname)
   ASM_OUTPUT_FUNCTION_PREFIX (asm_out_file, fnname);
 #endif
 
-  if (!DECL_IGNORED_P (decl))
+  if (!DECL_IGNORED_P (decl) || write_symbols == DWARF2_DEBUG)
     (*debug_hooks->begin_function) (decl);
 
   /* Make function name accessible from other files, if appropriate.  */
-- 
1.9.1

Re: [PATCH] Add line debug info for virtual thunks (PR ipa/97937)

[Jeff Law]

On 1/4/21 1:06 PM, Bernd Edlinger wrote:
> Hi,
>
>
> currently there is a problem when debugging a virtual thunk.  That is
> a decl with DECL_IGNORED_P.  Currently the line information displayed
> in gdb is completely bogus, thus the last line of whatever function
> is immediately before the PC of the thunk.
>
> This patch improves the debug experience at least a bit by emitting
> at the line number information where the thunk has been defined.
> I do not dare to touch anything but dwarf2 debug info, therefore
> the patch is a bit awkward.
>
>
> Bootstrapped and reg-tested on x86_64-pc-linux-gnu.
> Is it OK for trunk?
>
>
> Thanks
> Bernd.
>
> 0001-Add-line-debug-info-for-virtual-thunks.patch
>
> From 0a44bb870e90623689cae484f8a8899706480876 Mon Sep 17 00:00:00 2001
> From: Bernd Edlinger <bernd.edlinger@hotmail.de>
> Date: Sun, 3 Jan 2021 11:18:39 +0100
> Subject: [PATCH] Add line debug info for virtual thunks
>
> There is no full debug info since the DECL_IGNORED_P
> flag is set on virtual thunks.
> But instead of no line info at all, emit at least
> the location of the function decl.
>
> 2021-01-03  Bernd Edlinger  <bernd.edlinger@hotmail.de>
>
> 	PR ipa/97937
> 	* final.c (final_start_function_1): Always emit function start line
> 	information for dwarf2 debug.
> 	(final_end_function): Always call end_function for dwarf2 debug.
> 	* varasm.c (assemble_start_function): Always call begin_function
> 	for dwarf2 debug.


> ---
>  gcc/final.c  | 9 +++++++--
>  gcc/varasm.c | 2 +-
>  2 files changed, 8 insertions(+), 3 deletions(-)
>
> diff --git a/gcc/final.c b/gcc/final.c
> index fc9a05e..5a274c1 100644
> --- a/gcc/final.c
> +++ b/gcc/final.c
> @@ -1735,7 +1735,12 @@ final_start_function_1 (rtx_insn **firstp, FILE *file, int *seen,
>  				 last_filename);
>  
>    if (!dwarf2_debug_info_emitted_p (current_function_decl))
> -    dwarf2out_begin_prologue (0, 0, NULL);
> +    {
> +      if (write_symbols == DWARF2_DEBUG)
> +	dwarf2out_begin_prologue (last_linenum, last_columnnum, last_filename);
> +      else
> +	dwarf2out_begin_prologue (0, 0, NULL);
> +    }
The only way you're getting into this code is for DEBUG_DWARF2 and
VMS_AND_DWARF2_DEBUG and in the latter case we want to make the same
fix.  So drop the newly added conditional and just make the code
something like this:


if (!dwarf2_debug_info_emitted_p (current_function_decl))
  dwarf2out_begin_prologue (last_linenum, last_columnnum, last_filename)


>  
>  #ifdef LEAF_REG_REMAP
>    if (crtl->uses_only_leaf_regs)
> @@ -1879,7 +1884,7 @@ final_end_function (void)
>  {
>    app_disable ();
>  
> -  if (!DECL_IGNORED_P (current_function_decl))
> +  if (!DECL_IGNORED_P (current_function_decl) || write_symbols == DWARF2_DEBUG)
>      debug_hooks->end_function (high_function_linenum);
>  
>    /* Finally, output the function epilogue:
> diff --git a/gcc/varasm.c b/gcc/varasm.c
> index ce5d449..513922d 100644
> --- a/gcc/varasm.c
> +++ b/gcc/varasm.c
> @@ -1930,7 +1930,7 @@ assemble_start_function (tree decl, const char *fnname)
>    ASM_OUTPUT_FUNCTION_PREFIX (asm_out_file, fnname);
>  #endif
>  
> -  if (!DECL_IGNORED_P (decl))
> +  if (!DECL_IGNORED_P (decl) || write_symbols == DWARF2_DEBUG)
>      (*debug_hooks->begin_function) (decl);
I'd drop the DWARF2_DEBUG conditionals in these two hunks as well. 
There's no reason why we wouldn't want to emit suitable debug
information for other formats. 

Jeff

Re: [PATCH] Add line debug info for virtual thunks (PR ipa/97937)

[Bernd Edlinger]

On 1/4/21 10:45 PM, Jeff Law wrote:
> 
> 
> On 1/4/21 1:06 PM, Bernd Edlinger wrote:
>> --- a/gcc/final.c
>> +++ b/gcc/final.c
>> @@ -1735,7 +1735,12 @@ final_start_function_1 (rtx_insn **firstp, FILE *file, int *seen,
>>  				 last_filename);
>>  
>>    if (!dwarf2_debug_info_emitted_p (current_function_decl))
>> -    dwarf2out_begin_prologue (0, 0, NULL);
>> +    {
>> +      if (write_symbols == DWARF2_DEBUG)
>> +	dwarf2out_begin_prologue (last_linenum, last_columnnum, last_filename);
>> +      else
>> +	dwarf2out_begin_prologue (0, 0, NULL);
>> +    }
> The only way you're getting into this code is for DEBUG_DWARF2 and
> VMS_AND_DWARF2_DEBUG and in the latter case we want to make the same
> fix.  So drop the newly added conditional and just make the code
> something like this:
> 
> 
> if (!dwarf2_debug_info_emitted_p (current_function_decl))
> Â  dwarf2out_begin_prologue (last_linenum, last_columnnum, last_filename)
> 
> 

No, this is block is entered iff
 (write_symbols != DWARF2_DEBUG && write_symbols != VMS_AND_DWARF2_DEBUG)
  || DECL_IGNORED (current_function_decl))

so emitting .loc info here could easily break DBX, XCOFF, and VMS w/o DWARF
while I have no way to test anything for these debug formats.


Bernd.

Re: [PATCH] Add line debug info for virtual thunks (PR ipa/97937)

[Richard Biener]

On Mon, 4 Jan 2021, Bernd Edlinger wrote:

> Hi,
> 
> 
> currently there is a problem when debugging a virtual thunk.  That is
> a decl with DECL_IGNORED_P.  Currently the line information displayed
> in gdb is completely bogus, thus the last line of whatever function
> is immediately before the PC of the thunk.

But isn't this a consumer issue then?  If there is no line info for
a PC range then gdb shouldn't display any.

> This patch improves the debug experience at least a bit by emitting
> at the line number information where the thunk has been defined.
> I do not dare to touch anything but dwarf2 debug info, therefore
> the patch is a bit awkward.

There's more DECL_IGNORED_P decls (like functions emitted by
profile instrumentation), which do not have any source correspondence.

So IMHO the fix should be to make a more nuanced DECL_IGNORED_P
for thunks if it is really necessary to emit debug info for them.
For example by making them DECL_ARTIFICIAL only (not sure why
we end up with them DECL_IGNORED_P - there might be a reason).

Richard.

> 
> Bootstrapped and reg-tested on x86_64-pc-linux-gnu.
> Is it OK for trunk?
> 
> 
> Thanks
> Bernd.
> 

-- 
Richard Biener <rguenther@suse.de>
SUSE Software Solutions Germany GmbH, Maxfeldstrasse 5, 90409 Nuernberg,
Germany; GF: Felix Imendörffer; HRB 36809 (AG Nuernberg)

Re: [PATCH] Add line debug info for virtual thunks (PR ipa/97937)

[Alexandre Oliva]

On Jan  5, 2021, Richard Biener <rguenther@suse.de> wrote:

> But isn't this a consumer issue then?  If there is no line info for
> a PC range then gdb shouldn't display any.

No, there *is* line info there, carried over from an earlier .loc
directive, as there isn't anything like ".noloc" to output with a
function that is not expected or supposed to have line number info.

Without that, the assembler just extends the previous .loc directive
onto the function.

-- 
Alexandre Oliva, happy hacker  https://FSFLA.org/blogs/lxo/
   Free Software Activist         GNU Toolchain Engineer
        Vim, Vi, Voltei pro Emacs -- GNUlius Caesar

Re: [PATCH] Add line debug info for virtual thunks (PR ipa/97937)

[Bernd Edlinger]

On 1/6/21 8:01 AM, Alexandre Oliva wrote:
> On Jan  5, 2021, Richard Biener <rguenther@suse.de> wrote:
> 
>> But isn't this a consumer issue then?  If there is no line info for
>> a PC range then gdb shouldn't display any.
> 
> No, there *is* line info there, carried over from an earlier .loc
> directive, as there isn't anything like ".noloc" to output with a
> function that is not expected or supposed to have line number info.
> 
> Without that, the assembler just extends the previous .loc directive
> onto the function.
> 

Theoretically we could exclude the range of the no-loc function
from the .debug_ranges, then gdb would not even step into the function.

However if we have at least a single line info as in the case of the thunks,
then that would be better than nothing (what this patch does).


Bernd.

Re: [PATCH] Add line debug info for virtual thunks (PR ipa/97937)

[Richard Biener]

On Wed, 6 Jan 2021, Bernd Edlinger wrote:

> On 1/6/21 8:01 AM, Alexandre Oliva wrote:
> > On Jan  5, 2021, Richard Biener <rguenther@suse.de> wrote:
> > 
> >> But isn't this a consumer issue then?  If there is no line info for
> >> a PC range then gdb shouldn't display any.
> > 
> > No, there *is* line info there, carried over from an earlier .loc
> > directive, as there isn't anything like ".noloc" to output with a
> > function that is not expected or supposed to have line number info.
> > 
> > Without that, the assembler just extends the previous .loc directive
> > onto the function.
> > 
> 
> Theoretically we could exclude the range of the no-loc function
> from the .debug_ranges, then gdb would not even step into the function.

I'd argue we're failing to emit a .endloc at the end of functions
(rather than issueing a .noloc at the start of functions with no 
locations).  I wonder if using a special file ID and switching to that
would be an effective workaround?  When gas is extended we could use
file ID zero for this (which gas currently rejects).

> However if we have at least a single line info as in the case of the thunks,
> then that would be better than nothing (what this patch does).

But the problem extends to functins which do not have any line, so what
line do you use in that case?

Richard.

Re: [PATCH] Add line debug info for virtual thunks (PR ipa/97937)

[Jakub Jelinek]

On Wed, Jan 06, 2021 at 08:50:43AM +0100, Richard Biener wrote:
> > Theoretically we could exclude the range of the no-loc function
> > from the .debug_ranges, then gdb would not even step into the function.
> 
> I'd argue we're failing to emit a .endloc at the end of functions
> (rather than issueing a .noloc at the start of functions with no 
> locations).  I wonder if using a special file ID and switching to that
> would be an effective workaround?  When gas is extended we could use
> file ID zero for this (which gas currently rejects).

If we had .endloc, emitting it at the end of functions would be
theoretically more correct, but I'd be afraid it would unnecessarily grow
the .debug_line size, because nobody should care about line information in
padding bytes between functions and the .endloc would add there a change
even when it would affect just one byte of padding (probably even when it
wouldn't affect anything, depending on how it is implemented).

	Jakub

Re: [PATCH] Add line debug info for virtual thunks (PR ipa/97937)

[Richard Biener]

On Wed, 6 Jan 2021, Jakub Jelinek wrote:

> On Wed, Jan 06, 2021 at 08:50:43AM +0100, Richard Biener wrote:
> > > Theoretically we could exclude the range of the no-loc function
> > > from the .debug_ranges, then gdb would not even step into the function.
> > 
> > I'd argue we're failing to emit a .endloc at the end of functions
> > (rather than issueing a .noloc at the start of functions with no 
> > locations).  I wonder if using a special file ID and switching to that
> > would be an effective workaround?  When gas is extended we could use
> > file ID zero for this (which gas currently rejects).
> 
> If we had .endloc, emitting it at the end of functions would be
> theoretically more correct, but I'd be afraid it would unnecessarily grow
> the .debug_line size, because nobody should care about line information in
> padding bytes between functions and the .endloc would add there a change
> even when it would affect just one byte of padding (probably even when it
> wouldn't affect anything, depending on how it is implemented).

Sure, but of course gas can optimize this case for bytes it emits
because of .align

Richard.

Re: [PATCH] Add line debug info for virtual thunks (PR ipa/97937)

[Alexandre Oliva]

On Jan  4, 2021, Bernd Edlinger <bernd.edlinger@hotmail.de> wrote:

> currently there is a problem when debugging a virtual thunk.  That is
> a decl with DECL_IGNORED_P.  Currently the line information displayed
> in gdb is completely bogus, thus the last line of whatever function
> is immediately before the PC of the thunk.

*nod*, I recall seeing such issues before, for compiler-generated
functions.  Ideally, there should be some .noloc directive in the
assembler that would signal "no line number info for this code
fragment", but I don't think we have anything like that.

I don't recall whether using compiler-generated line number programs
avoids the problem, but I do recall that -ffunction-sections works
around it, because IIRC switching to a new section has the same effect
of discontinuing the line number program that the desired .noloc
directive would.

-- 
Alexandre Oliva, happy hacker  https://FSFLA.org/blogs/lxo/
   Free Software Activist         GNU Toolchain Engineer
        Vim, Vi, Voltei pro Emacs -- GNUlius Caesar

Re: [PATCH] Add line debug info for virtual thunks (PR ipa/97937)

[Eric Botcazou]

> currently there is a problem when debugging a virtual thunk.  That is
> a decl with DECL_IGNORED_P.  Currently the line information displayed
> in gdb is completely bogus, thus the last line of whatever function
> is immediately before the PC of the thunk.

DECL_IGNORED_P means completely ignored for debug info purposes though and I 
think that the Ada compiler expects this semantic.

> This patch improves the debug experience at least a bit by emitting
> at the line number information where the thunk has been defined.
> I do not dare to touch anything but dwarf2 debug info, therefore
> the patch is a bit awkward.

Please run the GDB testsuite with Ada support on the patch.

-- 
Eric Botcazou

Re: [PATCH] Add line debug info for virtual thunks (PR ipa/97937)

[Bernd Edlinger]

On 1/6/21 12:49 PM, Eric Botcazou wrote:
>> currently there is a problem when debugging a virtual thunk.  That is
>> a decl with DECL_IGNORED_P.  Currently the line information displayed
>> in gdb is completely bogus, thus the last line of whatever function
>> is immediately before the PC of the thunk.
> 
> DECL_IGNORED_P means completely ignored for debug info purposes though and I 
> think that the Ada compiler expects this semantic.
> 
>> This patch improves the debug experience at least a bit by emitting
>> at the line number information where the thunk has been defined.
>> I do not dare to touch anything but dwarf2 debug info, therefore
>> the patch is a bit awkward.
> 
> Please run the GDB testsuite with Ada support on the patch.
> 

Yes, I always do that, and this patch did not affect any Ada tests.
Anyway, I guess this patch will need some rework, to probably attack
the DECL_IGNORED_P without line info.

One thing I remembered from previous testing, and I was not sure if
it is a correctness issue or not, is this:

As I had to call begin_function also for DECL_IGNORED_P,
I noticed that there are a number of Ada functions with
DECL_IGNORED_P, but their  crtl->has_bb_partition is set,
and therefore this block is executed with my patch, but
it is currently not executed:

See dwarf2out_begin_function:

  if (crtl->has_bb_partition && !cold_text_section)
    {
      gcc_assert (current_function_decl == fun);
      cold_text_section = unlikely_text_section ();
      switch_to_section (cold_text_section);
      ASM_OUTPUT_LABEL (asm_out_file, cold_text_section_label);
      switch_to_section (sec);
    }

I had expected that functions with DECL_IGNORED_P should not need
that side effect.  Anyway I had not seen any effect from this.


Bernd.